-
Notifications
You must be signed in to change notification settings - Fork 998
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
Simplify deposits #780
Simplify deposits #780
Conversation
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 👍
pending on CI to verify.
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 PR requires two things:
- a corresponding change in the deposit contract repo
- an assertion in the deposit contract that ensures that the
amount
contained in thedeposit_data
payload is equivalent to theamount
of eth that came in on the tx. Otherwise, a user could send a 1eth tx but put in thedeposit_data.amount
some value higher than 1
Opened an issue :) |
Opened the PR at I didn't change much: first compute |
@NIC619 The spec ordering was chosen to be consistent with the |
@JustinDrake Updated in |
Similar to #779 , @NIC619 I think we should merge ethereum/deposit_contract#25 into |
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.
Looks good. Added tests, fixed a bug.
Fix #760