Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
First crack at serialization with backrefs. #119
base: main
Are you sure you want to change the base?
First crack at serialization with backrefs. #119
Changes from 3 commits
744fd33
fae87ba
e92f68f
d0b9b81
46c7e22
7ce9fce
ff4c538
2d2e72b
466ab05
1a15a07
136c657
3ee604f
ada04b3
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 found that in
clvm/object_cache.py
, hashing is done likehashlib.sha256(b"\2" + left_hash + right_hash).digest()
.So I think
def hash_blobs
should be like below for consistency.Or just replacing
hash_blobs(...)
byhashlib.sha256(...)digest()
would be direct and easy.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 first didn't see
root_hash
is actually what is calledsha256hash
ortreehash
in the Chia universe.I suggest to rename
root_hash
toroot_treehash
for consistency.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 tried this rename and I didn't like how it looked. I'll add a comment that says that all hashes correspond to tree hashes of a clvm object.
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.
How about renaming to
obj_treehash
?I don't push much because it feels a bit redundant after I understand what
obj_hash
means.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.
Adding a comment explaining that
0
meansleft
would greatly help future readers.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.
How about renaming
obj
toobj_treehash
?For example,
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.
Adding type hints would help future readers.
How about:
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.
Good idea.
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.
Small suggetion.
I think adding information showing that
left=0
andright=1
is helpful.Like