Skip to content
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

Using hash_tree_root for DepositData #29

Closed
hwwhww opened this issue Apr 18, 2019 · 3 comments · Fixed by #31
Closed

Using hash_tree_root for DepositData #29

hwwhww opened this issue Apr 18, 2019 · 3 comments · Fixed by #31
Assignees

Comments

@hwwhww
Copy link
Contributor

hwwhww commented Apr 18, 2019

What was wrong?

Implement ethereum/consensus-specs#924 related changes

How can it be fixed?

  1. Update deposit APIs to: def deposit(pubkey: bytes[48], withdrawal_credentials: bytes[32], amount: bytes[8], signature: bytes[96])
  2. Implement hash_tree_root of DepositData in contract
  3. Add tests for it
@NIC619
Copy link
Collaborator

NIC619 commented Apr 19, 2019

Should we also incorporate the data length check as suggested in ethereum/consensus-specs#710 (comment) into this?

@djrtwo
Copy link
Contributor

djrtwo commented Apr 19, 2019

I think we'll need to still assert length checks on all of these bytes types or ensure that we pad them out. Length checks is probably safer for user.

I don't think we need to include "amount" explicitly anymore as it can be dynamically built and hashed into the tree hash

@hwwhww
Copy link
Contributor Author

hwwhww commented Apr 19, 2019

@djrtwo

I think we'll need to still assert length checks on all of these bytes types or ensure that we pad them out. Length checks is probably safer for user.
I don't think we need to include "amount" explicitly anymore as it can be dynamically built and hashed into the tree hash

If we remove the amount field, I think the only field we need to do length check is exactly the little-endian-encoded amount.

@daejunpark hello! And, and thought/consideration about it? :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants