-
Notifications
You must be signed in to change notification settings - Fork 31
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
Added additional tests to cover HexaryTrie MPT proofs. #650
Conversation
tests/trie/test_hexary_proof.nim
Outdated
|
||
check: | ||
trie.rootHash == keccakHash(emptyRlp) | ||
proof.len() == 1 # Maybe this should return an empty list? |
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.
I think it makes sense that it is len() == 1
considering it drops in the empty rlp value?
The fact that the result is InvalidProof
is however inconsistent for when you would add a "not-exist` into a not empty trie. The emptry trie thing is quite the edge case though.
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.
Yeah it is an edge case so probably not worth spending too much time on. I raised it because I noticed that this library behaves differently than the Rust implementation which returns an empty proof when the trie is empty.
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.
I'd suggest to add a Note in comment that explains this and perhaps also link to this rust library.
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.
Sure, will do
tests/trie/test_hexary_proof.nim
Outdated
let db = newMemoryDB() | ||
var trie = initHexaryTrie(db) | ||
|
||
let |
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.
I noticed quite some trailing whitespace in this and previous PR. You might want to setup up some marking and/or trimming of that.
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.
Do you mean at the bottom of the file? Or spaces after the let?
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.
spaces after the let and some other locations.
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.
Ok thanks
@kdeme I've updated the code as you suggested. Ready for the final review. Thanks |
No description provided.