-
Notifications
You must be signed in to change notification settings - Fork 1
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
fix: supporting commit for non modified trees #208
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #208 +/- ##
==========================================
+ Coverage 63.07% 63.67% +0.59%
==========================================
Files 36 36
Lines 1617 1660 +43
Branches 1617 1660 +43
==========================================
+ Hits 1020 1057 +37
- Misses 548 552 +4
- Partials 49 51 +2 ☔ View full report in Codecov by Sentry. |
25af418
to
5cf4e53
Compare
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.
Reviewed 5 of 5 files at r1, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @nimrod-starkware and @TzahiTaub)
crates/committer/src/patricia_merkle_tree/updated_skeleton_tree/compute_updated_skeleton_tree.rs
line 405 at r1 (raw file):
} pub(crate) fn create_unmodified(
this has to be pub(crate)
? if it's private you can't call it from create
?
Code quote:
pub(crate)
crates/committer/src/patricia_merkle_tree/updated_skeleton_tree/compute_updated_skeleton_tree.rs
line 413 at r1 (raw file):
.ok_or(UpdatedSkeletonTreeError::MissingNode(NodeIndex::ROOT))?; let OriginalSkeletonNode::UnmodifiedSubTree(root_hash) = original_root_node else { panic!("A root of tree without modifications is expected to be a sibling.")
Suggestion:
be an unmodified node
crates/committer/src/patricia_merkle_tree/updated_skeleton_tree/compute_updated_skeleton_tree_test.rs
line 502 at r1 (raw file):
#[rstest] #[case::empty_tree(0)]
is there an EMPTY_HASH or EMPTY_ROOT constant you can use?
Code quote:
0
e1339a2
to
7be9856
Compare
2ab72c2
to
db4608a
Compare
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.
Reviewable status: 4 of 6 files reviewed, 3 unresolved discussions (waiting on @dorimedini-starkware and @TzahiTaub)
crates/committer/src/patricia_merkle_tree/updated_skeleton_tree/compute_updated_skeleton_tree.rs
line 405 at r1 (raw file):
Previously, dorimedini-starkware wrote…
this has to be
pub(crate)
? if it's private you can't call it fromcreate
?
yes it can't be private, i think because i call it under impl UpdatedSkeletonTree for UpdatedSkeletonTreeImpl
and not under impl UpdatedSkeletonTree
crates/committer/src/patricia_merkle_tree/updated_skeleton_tree/compute_updated_skeleton_tree.rs
line 413 at r1 (raw file):
.ok_or(UpdatedSkeletonTreeError::MissingNode(NodeIndex::ROOT))?; let OriginalSkeletonNode::UnmodifiedSubTree(root_hash) = original_root_node else { panic!("A root of tree without modifications is expected to be a sibling.")
Done.
crates/committer/src/patricia_merkle_tree/updated_skeleton_tree/compute_updated_skeleton_tree_test.rs
line 502 at r1 (raw file):
Previously, dorimedini-starkware wrote…
is there an EMPTY_HASH or EMPTY_ROOT constant you can use?
Done.
db4608a
to
006181c
Compare
006181c
to
019e82b
Compare
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.
Reviewed 1 of 2 files at r2, 3 of 3 files at r3, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @TzahiTaub)
This change is