chore(skeleton): update_node_in_nonempty_tree in updated_skeleton

[TzahiTaub]

This change is 

[dorimedini-starkware]

Reviewed 4 of 4 files at r1, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @TzahiTaub)

---

crates/committer/src/patricia_merkle_tree/updated_skeleton_tree/compute_updated_skeleton_tree.rs line 135 at r1 (raw file):

            );
            return TempSkeletonNode::Original(original_node);
        }

suggestion: (a) use match and (b) print out the node index in panic

Suggestion:

        if leaf_indices.is_empty() {
            match original_node {
                OriginalSkeletonNode::Binary => panic!(
                    "An original binary node (at index {root_index}) without leaf modifications should produce a sibling."
                ),
                OriginalSkeletonNode::UnmodifiedBottom(_) => panic!(
                    "An unmodified bottom (at index {root_index}) must have an original edge parent without leaf modifications."
                )
                OriginalSkeletonNode::Edge(_)
                | OriginalSkeletonNode::LeafOrBinarySibling(_)
                | OriginalSkeletonNode::Leaf => return TempSkeletonNode::Original(original_node);
            }
        }

---

crates/committer/src/patricia_merkle_tree/updated_skeleton_tree/compute_updated_skeleton_tree.rs line 204 at r1 (raw file):

                    OriginalSkeletonNode::LeafOrBinarySibling(_)
                    | OriginalSkeletonNode::UnmodifiedBottom(_) => {
                        // Unmodified nodes are finalized upon updated skeleton creation.

Suggestion:

Unmodified nodes are finalized in the initial phase of updated skeleton creation.

---

crates/committer/src/patricia_merkle_tree/updated_skeleton_tree/compute_updated_skeleton_tree_test.rs line 384 at r1 (raw file):

    ],
)]
fn test_update_node_in_nonempty_tree(

what about test cases for edges? you covered leaves and binary nodes

---

crates/committer/src/patricia_merkle_tree/updated_skeleton_tree/compute_updated_skeleton_tree_test.rs line 387 at r1 (raw file):

    #[case] root_index: &NodeIndex,
    #[case] mut original_skeleton: HashMap<NodeIndex, OriginalSkeletonNode>,
    #[case] leaf_modifications: &[(U256, u8)],

all of your #[case]s do U256::from(NodeIndex::from(..., no point in converting to U256 and back again...
or do you need it for the updated_skeleton fixture? (non-blocking)

Suggestion:

#[case] leaf_modifications: &[(NodeIndex, u8)],

---

crates/committer/src/patricia_merkle_tree/updated_skeleton_tree/tree.rs line 82 at r1 (raw file):

                            },
                        )
                    }),

no need for panic, just filter during your map (I used a flat_map to skip unwanted values, maybe you can find a less hack way without panic)

Suggestion:

                    .flat_map(|(index, node)| {
                        match node {
                            OriginalSkeletonNode::LeafOrBinarySibling(hash) => {
                                [(*index, UpdatedSkeletonNode::Sibling(*hash))].iter()
                            }
                            OriginalSkeletonNode::UnmodifiedBottom(hash) => {
                                [(*index, UpdatedSkeletonNode::UnmodifiedBottom(*hash))].iter(),
                            }
                            OriginalSkeletonNode::Binary
                            | OriginalSkeletonNode::Edge(_)
                            | OriginalSkeletonNode::Leaf => [].iter()
                        },
                    }),

[TzahiTaub]

Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @dorimedini-starkware)

---

crates/committer/src/patricia_merkle_tree/updated_skeleton_tree/compute_updated_skeleton_tree.rs line 135 at r1 (raw file):

Previously, dorimedini-starkware wrote…

suggestion: (a) use match and (b) print out the node index in panic

Hopefully, I didn't over-comment.

---

crates/committer/src/patricia_merkle_tree/updated_skeleton_tree/compute_updated_skeleton_tree.rs line 204 at r1 (raw file):

                    OriginalSkeletonNode::LeafOrBinarySibling(_)
                    | OriginalSkeletonNode::UnmodifiedBottom(_) => {
                        // Unmodified nodes are finalized upon updated skeleton creation.

Done.

---

crates/committer/src/patricia_merkle_tree/updated_skeleton_tree/compute_updated_skeleton_tree_test.rs line 384 at r1 (raw file):

Previously, dorimedini-starkware wrote…

what about test cases for edges? you covered leaves and binary nodes

This is currently not implemented (update_edge_node). Will be added in the following PR.

---

crates/committer/src/patricia_merkle_tree/updated_skeleton_tree/compute_updated_skeleton_tree_test.rs line 387 at r1 (raw file):

Previously, dorimedini-starkware wrote…

all of your #[case]s do U256::from(NodeIndex::from(..., no point in converting to U256 and back again...
or do you need it for the updated_skeleton fixture? (non-blocking)

They are U256 because of the usage in the updated_skeleton fixture. I want the input their to be U256 because it's simpler for basic examples. I have to use NodeIndex here to get to the FIrstLeaf because of the hardcoded TreeHeight limitation we have ATM, otherwise, I would have used small indices here as well.

---

crates/committer/src/patricia_merkle_tree/updated_skeleton_tree/tree.rs line 82 at r1 (raw file):

Previously, dorimedini-starkware wrote…

no need for panic, just filter during your map (I used a flat_map to skip unwanted values, maybe you can find a less hack way without panic)

Done.

[dorimedini-starkware]

Reviewed 2 of 2 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @TzahiTaub)

---

crates/committer/src/patricia_merkle_tree/updated_skeleton_tree/compute_updated_skeleton_tree.rs line 204 at r1 (raw file):

Previously, TzahiTaub (Tzahi) wrote…

Done.

in line 184, yes.
here, no

[TzahiTaub]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @dorimedini-starkware)

---

crates/committer/src/patricia_merkle_tree/updated_skeleton_tree/compute_updated_skeleton_tree.rs line 204 at r1 (raw file):

Previously, dorimedini-starkware wrote…

in line 184, yes.
here, no

Now done :|

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 46.34146% with 44 lines in your changes are missing coverage. Please review.

Project coverage is 60.30%. Comparing base (3dfb531) to head (5c6260b).
Report is 1 commits behind head on main.

Files	Patch %	Lines
...ted_skeleton_tree/compute_updated_skeleton_tree.rs	62.29%	22 Missing and 1 partial ⚠️
...patricia_merkle_tree/updated_skeleton_tree/tree.rs	0.00%	18 Missing ⚠️
...atricia_merkle_tree/original_skeleton_tree/tree.rs	0.00%	3 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #162      +/-   ##
==========================================
- Coverage   60.94%   60.30%   -0.65%     
==========================================
  Files          36       36              
  Lines        1439     1577     +138     
  Branches     1439     1577     +138     
==========================================
+ Hits          877      951      +74     
- Misses        522      583      +61     
- Partials       40       43       +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[dorimedini-starkware]

Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @TzahiTaub)