-
Notifications
You must be signed in to change notification settings - Fork 995
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
Cleanup: making process deposit more spec readable and fork specific #14139
Conversation
…ic, breaking out components of pr 13944
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, mostly just moving tests around and some renaming. Might be good to wait for someone else to have a pass before merging
) | ||
|
||
// ProcessPreGenesisDeposits processes a deposit for the beacon state before chainstart. |
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.
Why are these in altair
? I think these additions are not part of altair hard fork right
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.
the alternative would be to break out process deposits into yet another for phase 0. I considered this but just decided to have the two ( altair and electra). if you think I should still break this out I can.
the issue with leaving it in the blocks package is the circular dependency, processPreGenesisDeposits calls process deposits which needs to specify a fork for process deposits, in this case it only makes sense to use the current version of process deposits
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
What type of PR is this?
Other
What does this PR do? Why is it needed?
Currently, the
ProcessDeposit
function lives in a shared fork repo in theblocks
package. To prepare for changes in 6110 I have broken the functions to match more closely with the consensus specs defined in https://github.com/ethereum/consensus-specs. As part of this PR I've moved the deposit functions from the blocks package to the altair package which will allow easier updating in future hard forks.breaking out pr #13944
Which issues(s) does this PR fix?
Fixes #
Other notes for review