-
Notifications
You must be signed in to change notification settings - Fork 778
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
Implement EIP4895: Beacon Chain withdrawals #2353
Merged
Changes from all commits
Commits
Show all changes
9 commits
Select commit
Hold shift + click to select a range
5ea5434
common: add eip 4895
jochem-brouwer c0f0a7e
block: implement EIP4895
jochem-brouwer 0b01c78
vm: add EIP4895
jochem-brouwer 6e418c2
block: eip4895 tests
jochem-brouwer ba38e22
vm: add eip4895 tests
jochem-brouwer 819e63e
block: fix trest
jochem-brouwer f4782d6
vm: fix tests
jochem-brouwer c28d8b3
change withdrawal type to object format and add validator index
g11tech d02d71e
fix vm withdrawal spec
g11tech File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Thinking about this for some time now:
withdrawals
have this typeWithdrawal
here with all these non-deterministic types likeBigIntLike
:While this always comes in handy for input data I wonder if this is an optimal design choice for the internal representation of this data. In the case of withdrawals, this
Withdrawal
type atm actually serves for both puroposes.With this kind of design we just can't be sure what types these internal withdrawal parameters like
index
oraddress
really have. My fear is that this will lead to a myriad of follow-up problems. For test cases, client usages,... one can never really count on what the type really is here and either cast here, make code comments, add complex procedural choice conditional switches for values on processing, or minimally: one always need to convert withwithdrawalToBufferArray()
.I would have a relatively strong tendency that we should switch here to deterministic values, I guess that will safe us from some substantial amount of follow-up trouble. This also aligns with our standard handling of internally represented data.
So I would suggest to actually have two types here, the one from above renamed to
WithdrawalData
or similar and used for withdrawal inputs.And another one
Withdrawal
with deterministic types. Following our tendency to represent internal values not always with Buffer any more but with more "matching" types this would be something like the following I guess:I know that this is some extra effort to rewrite now with various code already in place but I am relatively sure that this more than pay off in the future.
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.
Update: Haha https://github.com/ethereumjs/ethereumjs-monorepo/pull/2401/files#diff-6ef8827503f0272ab6d926170261d659f841b39a00e354abcd69ea5635c8eae1
Just discovered on the follow up PR from @g11tech.
At least we think in impressively similar lines. 🤩 😂