Skip to content

[ETCM-1015] Add Magneto block height to configs #1067

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 7 commits into from
Jul 19, 2021

Conversation

lukasz-golebiewski
Copy link
Contributor

Description

This PR adds Magneto block height to the app's config files

@lukasz-golebiewski lukasz-golebiewski force-pushed the feature/etcm-1015 branch 2 times, most recently from f96f24d to d81e4e7 Compare July 16, 2021 11:40
@@ -224,28 +224,12 @@ class BlockGeneratorSpec extends AnyFlatSpec with Matchers with ScalaCheckProper
gasTieBreaker = false,
ethCompatibleStorage = true,
treasuryAddress = Address(0),
forkBlockNumbers = ForkBlockNumbers(
forkBlockNumbers = ForkBlockNumbers.Empty.copy(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@@ -98,6 +98,10 @@
# https://ecips.ethereumclassic.org/ECIPs/ecip-1099
ecip1099-block-number = "1000000000000000000"

# Magneto EVM and Protocol Upgrades
# https://ecips.ethereumclassic.org/ECIPs/ecip-1103
magneto-block-number = "1000000000000000000"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of using a super high block number, why not define this config value as Option[BigInt], and omit it in the files where it does not make sense?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking about doing it like this, but in the end didn't to remain consistent. Perhaps it would make sense to refactor those configs with the approach above in a separate PR?

Copy link
Contributor

@leo-bogastry leo-bogastry Jul 19, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a configuration added by me, that is an Option already :) So total consistency is already gone 😅
I would love to see this refactored

@lukasz-golebiewski lukasz-golebiewski merged commit ced069a into develop Jul 19, 2021
@lukasz-golebiewski lukasz-golebiewski deleted the feature/etcm-1015 branch July 19, 2021 08:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants