-
Notifications
You must be signed in to change notification settings - Fork 871
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
rename premerge fork to paris, keep premerge as an alias #3615
rename premerge fork to paris, keep premerge as an alias #3615
Conversation
|
||
GenesisConfigFile parisOverAlias = | ||
GenesisConfigFile.fromConfig( | ||
"{\"config\":{\"parisBlock\":10,\"preMergeForkBlock\":11},\"baseFeePerGas\":\"0xa\"}"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change my mind: this should be an error not an override
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are a number of Genesis files out for kiln and devnet5 that reference the prior fork name. The alias is a way to ease the friction of the change.
I am thinking we should drop it before we merge any of the long lived test nets, possibly before we release 22.1.3. But I would like to make the UX for testers a bit smoother in the interim.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One or the other is fine. We did this with berlin and eip1559. But specifying both should be an error. If you went in to add Paris you should also remove preMergeFork
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah I gotcha. That makes sense. I thought you were referring to just having an alias 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good test coverage
assertThatThrownBy(() -> dupeOptions.getParisBlockNumber()) | ||
.isInstanceOf(RuntimeException.class); | ||
|
||
// assert empty in neither are present: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: s/in/if
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
got it 🍻
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. one tiny nit that I won't be upset about
edb3782
to
2811e62
Compare
Signed-off-by: garyschulte <garyschulte@gmail.com>
Signed-off-by: garyschulte <garyschulte@gmail.com>
…throw runtime Signed-off-by: garyschulte <garyschulte@gmail.com>
Signed-off-by: garyschulte <garyschulte@gmail.com>
Signed-off-by: garyschulte <garyschulte@gmail.com>
2811e62
to
4522748
Compare
Kudos, SonarCloud Quality Gate passed! |
…#3615) * rename premerge fork to paris, keep premerge as an alias Signed-off-by: garyschulte <garyschulte@gmail.com>
…#3615) * rename premerge fork to paris, keep premerge as an alias Signed-off-by: garyschulte <garyschulte@gmail.com>
…#3615) * rename premerge fork to paris, keep premerge as an alias Signed-off-by: garyschulte <garyschulte@gmail.com>
Signed-off-by: garyschulte garyschulte@gmail.com
PR description
rename the merge fork to
paris
keep
premerge
as an alias for parisFixed Issue(s)
fixes #3608
Changelog