Add Labels to ChainParameters to Enable Hardforks via Single Timestamp #7764
Conversation
| if (Eip4844TransitionTimestamp == Eip4788TransitionTimestamp && | ||
| Eip4788TransitionTimestamp == Eip1153TransitionTimestamp && | ||
| Eip1153TransitionTimestamp == Eip5656TransitionTimestamp && | ||
| Eip5656TransitionTimestamp == Eip6780TransitionTimestamp) |
There was a problem hiding this comment.
We should have a helper method to have such comparisions as we will do it for every hardfork.
There was a problem hiding this comment.
I created the helper method called "AreHardforkTimestampsEqual" in order to accomplish this one, which compares it instead of using the ifs inside each hardfork.
| } | ||
| set | ||
| { | ||
| _dencunTransitionTimestamp = value; |
There was a problem hiding this comment.
Potentially we could have _dencunTransitionTimestamp and individual EIP's out of sync, so maybe always calculate it and don't have a separate field?
There was a problem hiding this comment.
You are so right... I made the change using the helper method previously suggested and if it's true, we assign one of those timestamps values so it calculates it dynamically. (code lines 32-42 and 56-65)
| } | ||
| set | ||
| { | ||
| _dencunTransitionTimestamp = value; |
There was a problem hiding this comment.
Potentially we could have _dencunTransitionTimestamp and individual EIP's out of sync, so maybe always calculate it and don't have a separate field? Then you also don't need Validate methods
| if (Eip4844TransitionTimestamp == Eip4788TransitionTimestamp && | ||
| Eip4788TransitionTimestamp == Eip1153TransitionTimestamp && | ||
| Eip1153TransitionTimestamp == Eip5656TransitionTimestamp && | ||
| Eip5656TransitionTimestamp == Eip6780TransitionTimestamp) |
There was a problem hiding this comment.
You could potentially use more declarative style of Expression Api to map individual properties to Hardfork property, that might be useful outside of this class too?
There was a problem hiding this comment.
Sure! In this refactor commit I created a dictionary at lines 15-22 so we can access only using the key tag, also a public static method GetHardforkMapping in case we need to get any value from this dictionary from any other classes outside
There was a problem hiding this comment.
@LuisDi98 I think recent changes are step in wrong direction. Would prefer to avoid strings. Added complexity isn't bringing anything at the moment.
| public ulong? DencunTransitionTimestamp | ||
| { | ||
| get | ||
| { | ||
| if (AreHardforkTimestampsEqual( | ||
| "Dencun", "Cancun", "EIP-1153", | ||
| "EIP-5656", "EIP-6780")) | ||
| { | ||
| return Eip4844TransitionTimestamp; | ||
| } | ||
|
|
||
| return null; | ||
| } | ||
| set | ||
| { | ||
| Eip4844TransitionTimestamp = value; | ||
| Eip4788TransitionTimestamp = value; | ||
| Eip1153TransitionTimestamp = value; | ||
| Eip5656TransitionTimestamp = value; | ||
| Eip6780TransitionTimestamp = value; | ||
| } | ||
| } | ||
|
|
||
|
|
||
| public ulong? CancunTransitionTimestamp | ||
| { | ||
| get | ||
| { | ||
| if (AreHardforkTimestampsEqual( | ||
| "Dencun", "Cancun", "EIP-1153", | ||
| "EIP-5656", "EIP-6780")) | ||
| { | ||
| return Eip4844TransitionTimestamp; | ||
| } | ||
| return null; | ||
| } | ||
| set | ||
| { | ||
| Eip4844TransitionTimestamp = value; | ||
| Eip4788TransitionTimestamp = value; | ||
| Eip1153TransitionTimestamp = value; | ||
| Eip5656TransitionTimestamp = value; | ||
| Eip6780TransitionTimestamp = value; | ||
| } | ||
| } |
There was a problem hiding this comment.
Dencun and Cancun are same thing, confusing.
| private static readonly Dictionary<string, Func<ChainParameters, ulong?>> HardforkToEips = new() | ||
| { | ||
| { "Dencun", cp => cp.Eip4844TransitionTimestamp }, | ||
| { "Cancun", cp => cp.Eip4788TransitionTimestamp }, | ||
| { "EIP-1153", cp => cp.Eip1153TransitionTimestamp }, | ||
| { "EIP-5656", cp => cp.Eip5656TransitionTimestamp }, | ||
| { "EIP-6780", cp => cp.Eip6780TransitionTimestamp } | ||
| }; |
There was a problem hiding this comment.
I was thinking of something more like:
public static readonly IReadOnlyList<Expression<Func<ChainParameters, ulong?>>> DencunEIPs =
[
cp => cp.Eip4844TransitionTimestamp,
cp => cp.Eip4788TransitionTimestamp,
cp => cp.Eip1153TransitionTimestamp,
cp => cp.Eip5656TransitionTimestamp,
cp => cp.Eip6780TransitionTimestamp
];
If we had same interface for ChainParameters and ChainSpecParamsJson that could be potentially reusable.
But that maybe an overkill.
This PR introduces functionality to allow grouped EIP timestamps (e.g., dencun, cancun) in ChainParameters. This simplifies the process of activating multiple EIPs simultaneously through a single label, while preserving the ability to handle individual EIP timestamps.
Fixes #7738
Changes
Types of Changes
What types of changes does your code introduce?
Testing
Requires testing
If yes, did you write tests?
Notes on testing
Added unit tests in ChainParametersTests to verify:
Documentation
Requires documentation update
Requires explanation in Release Notes
Release Notes: