chore: add get_path_to_descendant to NodeIndex

[TzahiTaub]

This change is 

[dorimedini-starkware]

Reviewed 7 of 7 files at r1, all commit messages.
Reviewable status: all files reviewed, 10 unresolved discussions (waiting on @amosStarkware and @TzahiTaub)

---

crates/committer/src/patricia_merkle_tree/errors.rs line 11 at r1 (raw file):

        reason: &'static str,
    },
}

is this possible? non-blocking

Suggestion:

#[derive(Debug, Error)]
pub(crate) enum TypesError {
    #[error("Failed to convert type {from:?} to {to}. Reason: {reason}.")]
    ConversionError<T: Sized + Debug> {
        from: T,
        to: &'static str,
        reason: &'static str,
    },
}

---

crates/committer/src/patricia_merkle_tree/types.rs line 93 at r1 (raw file):

    }

    pub(crate) fn get_path_to_descendant(&self, descendant: NodeIndex) -> PathToBottom {

docstring

---

crates/committer/src/patricia_merkle_tree/types.rs line 93 at r1 (raw file):

    }

    pub(crate) fn get_path_to_descendant(&self, descendant: NodeIndex) -> PathToBottom {

Suggestion:

pub(crate) fn get_path_to_descendant(&self, descendant: Self) -> PathToBottom {

---

crates/committer/src/patricia_merkle_tree/types.rs line 159 at r1 (raw file):

        Self(U256::from_be_bytes(value.to_bytes_be()))
    }
}

@nimrod-starkware FYI (please delete)

Code quote:

impl From<Felt> for NodeIndex {
    fn from(value: Felt) -> Self {
        Self(U256::from_be_bytes(value.to_bytes_be()))
    }
}

---

crates/committer/src/patricia_merkle_tree/types.rs line 161 at r1 (raw file):

}

impl TryInto<Felt> for NodeIndex {

IIRC in rust it is recommended to impl the From direction (you get the respective into for free)

Suggestion:

impl TryFrom<NodeIndex> for Felt {

---

crates/committer/src/patricia_merkle_tree/types_test.rs line 97 at r1 (raw file):

    assert_eq!(left_descendant.get_lca(&right_descendant), lca);
}

add tests for specific edge cases please (node==descendant case, check for panic if descendant is not a descendant... whatever you can think of that is interesting in your opinion).

---

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

    #[allow(dead_code)]
    fn get_path_to_lca(&self, root_index: &NodeIndex, leaf_indices: &[NodeIndex]) -> PathToBottom {

this function doesn't need a list as input; the calling code can take care of what is the "first and last" indices.
the calling code may even be able to do it without panicking.

(actually, maybe make the same change in the "has leaves on both sides" function..? does that make sense?)

Suggestion:

fn get_path_to_lca(&self, root_index: &NodeIndex, left_index: &NodeIndex, right_index: &NodeIndex)

---

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

        // * Sorted.
        // * Descendants of the given index.
        // * Non-empty list.

docstrings are above the declaration (can be before or after attributes),
and lead with triple-slashes

Suggestion:

    /// Returns the path from the given root_index to the LCA of the leaves. Assumes the leaves are:
    /// * Sorted.
    /// * Descendants of the given index.
    /// * Non-empty list.
    #[allow(dead_code)]
    fn get_path_to_lca(&self, root_index: &NodeIndex, leaf_indices: &[NodeIndex]) -> PathToBottom {

---

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

#[rstest]
#[case::small_tree_single_leaf(3, 1, vec![NodeIndex::from(8)], PathToBottom {path:EdgePath(Felt::ZERO), length:EdgePathLength(3)})]

add cases where the path is longer than 1? isn't that interesting? or am I missing something

---

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

        skeleton_tree.get_path_to_lca(&root_index, &leaf_indices),
        expected
    );

this change will reduce boilerplate in your #[case]s

Suggestion:

    #[case] leaf_indices: Vec<U256>,
    #[case] expected: PathToBottom,
) {
    let skeleton_tree = empty_skeleton(tree_height);
    let root_index = NodeIndex::new(root_index.into());
    assert_eq!(
        skeleton_tree.get_path_to_lca(&root_index, &leaf_indices.iter().map(NodeIndex::new).collect()),
        expected
    );

[nimrod-starkware]

Reviewable status: all files reviewed, 10 unresolved discussions (waiting on @amosStarkware, @dorimedini-starkware, and @TzahiTaub)

---

crates/committer/src/patricia_merkle_tree/types.rs line 159 at r1 (raw file):

Previously, dorimedini-starkware wrote…

@nimrod-starkware FYI (please delete)

in PR

[amosStarkware]

Reviewable status: all files reviewed, 10 unresolved discussions (waiting on @TzahiTaub)

---

crates/committer/src/patricia_merkle_tree/errors.rs line 9 at r1 (raw file):

        from: &'static str,
        to: &'static str,
        reason: &'static str,

why not use String instead of &'static str?
like here

committer/crates/committer/src/patricia_merkle_tree/updated_skeleton_tree/errors.rs

Line 15 in 4d3e772

NonDroppedPointer(String),

Code quote:

        from: &'static str,
        to: &'static str,
        reason: &'static str,

[TzahiTaub]

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

---

crates/committer/src/patricia_merkle_tree/errors.rs line 9 at r1 (raw file):

Previously, amosStarkware wrote…

why not use String instead of &'static str?
like here

committer/crates/committer/src/patricia_merkle_tree/updated_skeleton_tree/errors.rs

Line 15 in 4d3e772

NonDroppedPointer(String),

IIUC static str is more efficient, you have the string in the code, and pointers to it, instead of creating dynamic strings to be owned by the error.

---

crates/committer/src/patricia_merkle_tree/errors.rs line 11 at r1 (raw file):

Previously, dorimedini-starkware wrote…

is this possible? non-blocking

Almost.

---

crates/committer/src/patricia_merkle_tree/types.rs line 93 at r1 (raw file):

Previously, dorimedini-starkware wrote…

docstring

Done.

---

crates/committer/src/patricia_merkle_tree/types.rs line 93 at r1 (raw file):

    }

    pub(crate) fn get_path_to_descendant(&self, descendant: NodeIndex) -> PathToBottom {

Done.

---

crates/committer/src/patricia_merkle_tree/types.rs line 161 at r1 (raw file):

Previously, dorimedini-starkware wrote…

IIRC in rust it is recommended to impl the From direction (you get the respective into for free)

Done.

---

crates/committer/src/patricia_merkle_tree/types_test.rs line 97 at r1 (raw file):

Previously, dorimedini-starkware wrote…

add tests for specific edge cases please (node==descendant case, check for panic if descendant is not a descendant... whatever you can think of that is interesting in your opinion).

It this OK? It's more convenient to do the edge cases on small examples.

---

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

Previously, dorimedini-starkware wrote…

this function doesn't need a list as input; the calling code can take care of what is the "first and last" indices.
the calling code may even be able to do it without panicking.

(actually, maybe make the same change in the "has leaves on both sides" function..? does that make sense?)

Why is this better? To move the responsibility of the "sorted" list to the caller? I think this might code duplcation for the empty/length 1 checks.

---

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

Previously, dorimedini-starkware wrote…

docstrings are above the declaration (can be before or after attributes),
and lead with triple-slashes

I know that actually. Done.

---

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

Previously, dorimedini-starkware wrote…

add cases where the path is longer than 1? isn't that interesting? or am I missing something

The path is larget than 1 in the first and last cases (the value is 0, but the length is larger). I can add an explicit unique path if you want something different than 0.

---

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

Previously, dorimedini-starkware wrote…

this change will reduce boilerplate in your #[case]s

Not sure this was too useful, but I've learnt a bit of rust things.

[codecov-commenter]

Codecov Report

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

Project coverage is 77.43%. Comparing base (99d0118) to head (0ad95a1).
Report is 2 commits behind head on main.

Files	Patch %	Lines
...rates/committer/src/patricia_merkle_tree/errors.rs	0.00%	1 Missing ⚠️
...nal_skeleton_tree/compute_updated_skeleton_tree.rs	88.88%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #101      +/-   ##
==========================================
+ Coverage   76.47%   77.43%   +0.96%     
==========================================
  Files          25       26       +1     
  Lines         884      935      +51     
  Branches      884      935      +51     
==========================================
+ Hits          676      724      +48     
- Misses        174      177       +3     
  Partials       34       34              

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

[dorimedini-starkware]

Reviewed 5 of 10 files at r2, 5 of 5 files at r3, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @amosStarkware and @TzahiTaub)

---

crates/committer/src/patricia_merkle_tree/types.rs line 199 at r3 (raw file):

        let bytes = value.0.to_be_bytes();
        Ok(Felt::from_bytes_be_slice(&bytes))
    }

Self is now Felt.
non-blocking

Suggestion:

    fn try_from(value: NodeIndex) -> Result<Self, Self::Error> {
        if value.0 > U256::from_be_bytes(Self::MAX.to_bytes_be()) {
            return Err(TypesError::ConversionError {
                from: value,
                to: "Felt",
                reason: "NodeIndex is too large",
            });
        }
        let bytes = value.0.to_be_bytes();
        Ok(Self::from_bytes_be_slice(&bytes))
    }

---

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

Previously, TzahiTaub (Tzahi) wrote…

Why is this better? To move the responsibility of the "sorted" list to the caller? I think this might code duplcation for the empty/length 1 checks.

you're right, it doesn't help much.. unblocking

[TzahiTaub]

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