-
Notifications
You must be signed in to change notification settings - Fork 465
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
EIP-7251: Add EL triggered consolidations #7182
Conversation
src/Nethermind/Nethermind.Core/ConsensusRequests/ConsensusRequest.cs
Outdated
Show resolved
Hide resolved
0fd1e22
to
16b4477
Compare
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.
This can be done in next PR, but I think we should start leveraging ConsensusRequest polimorphism, so we can easily add more in the future with lesser code
@@ -28,6 +28,7 @@ protected Olympic() | |||
ValidateReceipts = true; | |||
|
|||
// The below addresses are added for all forks, but the given EIPs can be enabled at a specific timestamp or block. | |||
Eip7251ContractAddress = Eip7251Constants.ConsolidationRequestPredeployAddress; |
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.
@MarekM25 why are they added in Olympic, rather than fork they are enabled at? Seems messy to me.
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.
becuase you don't add them for transition. You don't need to have transition to specify address. We could seperate it to fork-agnostic properties cc: @rjnrohit
Changes
This PR implements : ethereum/EIPs#8625. more info at: https://eips.ethereum.org/EIPS/eip-7251 and ethereum/execution-apis#554
Types of changes
What types of changes does your code introduce?
Testing
Requires testing
If yes, did you write tests?