-
Notifications
You must be signed in to change notification settings - Fork 9
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
Update deposit contract according to v0.6.1 #130
Conversation
Also I've not found this goddamn **** https://github.com/harmony-dev/beacon-chain-java/pull/130/files#diff-9b854443634f1745881ecae0970951edR174 anywhere except deposit contract sources. Am I correct it's really required and it couldn't be simplified? Also some docs suggests using call of |
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.
There is a couple of notes. Otherwise, it looks good to me and ready to get merged.
pow/core/src/main/java/org/ethereum/beacon/pow/AbstractDepositContract.java
Outdated
Show resolved
Hide resolved
return zeroHashes[distanceFromBottom]; | ||
} | ||
|
||
// # Compute a Merkle root of a right-zerobyte-padded 2**32 sized tree |
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.
Where does this Python code come from? Could we annotate its source in a class header?
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.
Done.
pow/core/src/main/java/org/ethereum/beacon/pow/MinimalMerkle.java
Outdated
Show resolved
Hide resolved
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.
If we don't want to make Merkle tree properly abstracted at this stage then this PR is ready to get merged; I'd only like to see a couple of renames.
@mkalinin done |
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.
Just some minor notes...
@Nashatyrev ready for re-review |
Some quotations to better understand this update:
As you see this PR doesn't look like a production version of deposit root/proofs supplier, so I will be glad to give any feedback and help to move it to something we could really use in production.