feat: move leaf fetching logic to leaf data trait

[dorimedini-starkware]

This change is 

[codecov-commenter]

Codecov Report

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

Project coverage is 60.44%. Comparing base (9da95bf) to head (8d36711).

Files	Patch %	Lines
...itter/src/patricia_merkle_tree/filled_tree/tree.rs	0.00%	0 Missing and 4 partials ⚠️
...itter/src/patricia_merkle_tree/node_data/errors.rs	0.00%	1 Missing ⚠️
...mmitter/src/patricia_merkle_tree/node_data/leaf.rs	90.00%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #160      +/-   ##
==========================================
- Coverage   60.44%   60.44%   -0.01%     
==========================================
  Files          35       35              
  Lines        1603     1613      +10     
  Branches     1603     1613      +10     
==========================================
+ Hits          969      975       +6     
- Misses        589      590       +1     
- Partials       45       48       +3     

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

[aner-starkware]

[TzahiTaub]

Reviewed 2 of 2 files at r3, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @aner-starkware and @dorimedini-starkware)

---

crates/committer/src/patricia_merkle_tree/node_data/leaf.rs line 22 at r3 (raw file):

    // Use explicit desugaring of `async fn` to allow adding trait bounds to the return type, see
    // https://blog.rust-lang.org/2023/12/21/async-fn-rpit-in-traits.html#async-fn-in-public-traits
    // for details.

Suggestion:

    /// Returns / computes the NodeData.
    /// Use explicit desugaring of `async fn` to allow adding trait bounds to the return type, see
    /// https://blog.rust-lang.org/2023/12/21/async-fn-rpit-in-traits.html#async-fn-in-public-traits
    /// for details.

---

crates/committer/src/patricia_merkle_tree/node_data/leaf.rs line 63 at r3 (raw file):

        index: &NodeIndex,
        leaf_modifications: &LeafModifications<Self>,
    ) -> FilledTreeResult<NodeData<Self>, Self> {

Remove

Code quote:

FilledTreeResult

---

crates/committer/src/patricia_merkle_tree/node_data/leaf.rs line 66 at r3 (raw file):

        let leaf_data = leaf_modifications
            .get(index)
            .ok_or(FilledTreeError::<Self>::MissingDataForUpdate(*index))?

Need new errors and possible wrap them in the caller.

Suggestion:

LeafError::<Self>::MissingDataForUpdate(*index))?

---

crates/committer/src/patricia_merkle_tree/node_data/leaf.rs line 69 at r3 (raw file):

            .clone();
        if leaf_data.is_empty() {
            return Err(FilledTreeError::<Self>::DeletedLeafInSkeleton(*index));

Suggestion:

(LeafError::<Self>::DeletedLeafInSkeleton(*index)

[dorimedini-starkware]

Reviewable status: 0 of 4 files reviewed, 4 unresolved discussions (waiting on @aner-starkware and @TzahiTaub)

---

crates/committer/src/patricia_merkle_tree/node_data/leaf.rs line 22 at r3 (raw file):

    // Use explicit desugaring of `async fn` to allow adding trait bounds to the return type, see
    // https://blog.rust-lang.org/2023/12/21/async-fn-rpit-in-traits.html#async-fn-in-public-traits
    // for details.

this is not part of the docstring; it doesn't describe API, only explains an implementation detail

---

crates/committer/src/patricia_merkle_tree/node_data/leaf.rs line 63 at r3 (raw file):

Previously, TzahiTaub (Tzahi) wrote…

Remove

Done.

---

crates/committer/src/patricia_merkle_tree/node_data/leaf.rs line 66 at r3 (raw file):

Previously, TzahiTaub (Tzahi) wrote…

Need new errors and possible wrap them in the caller.

Done.

---

crates/committer/src/patricia_merkle_tree/node_data/leaf.rs line 69 at r3 (raw file):

            .clone();
        if leaf_data.is_empty() {
            return Err(FilledTreeError::<Self>::DeletedLeafInSkeleton(*index));

Done.

[TzahiTaub]

Reviewed 4 of 4 files at r4, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @aner-starkware and @dorimedini-starkware)

---

crates/committer/src/patricia_merkle_tree/node_data/leaf.rs line 66 at r3 (raw file):

Previously, dorimedini-starkware wrote…

Done.

Sorry, actually the name of the variants are still in the context of the fileedTree (what update? what skeleton? We are in a LeafData struct). Maybe switch to MissingLeafModificationData and EmptyLeafInModifications? You can keep the same name for the fileedTree transparency IMO.

---

crates/committer/src/patricia_merkle_tree/node_data/leaf.rs line 16 at r4 (raw file):

    fn is_empty(&self) -> bool;

    /// Creates a leaf.

Is this because of the possible future computations? Because get seems better here (as if the index is missing in the map we'll get an error)

Code quote:

 Creates a leaf.

[dorimedini-starkware]

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

---

crates/committer/src/patricia_merkle_tree/node_data/leaf.rs line 66 at r3 (raw file):

Previously, TzahiTaub (Tzahi) wrote…

Sorry, actually the name of the variants are still in the context of the fileedTree (what update? what skeleton? We are in a LeafData struct). Maybe switch to MissingLeafModificationData and EmptyLeafInModifications? You can keep the same name for the fileedTree transparency IMO.

Done.

---

crates/committer/src/patricia_merkle_tree/node_data/leaf.rs line 16 at r4 (raw file):

Previously, TzahiTaub (Tzahi) wrote…

Is this because of the possible future computations? Because get seems better here (as if the index is missing in the map we'll get an error)

it's for the future, yes; IMO it's better that the API is clear from the start. create sounds more like "this operation may be non-trivial" to me

[TzahiTaub]

Reviewed 4 of 4 files at r5, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @aner-starkware and @dorimedini-starkware)

---

crates/committer/src/patricia_merkle_tree/filled_tree/errors.rs line 10 at r5 (raw file):

pub enum FilledTreeError<L: LeafData> {
    #[error("Deleted leaf at index {0:?} appears in the updated skeleton tree.")]
    DeletedLeafInSkeleton(NodeIndex),

Revive error

Code quote:

    #[error("Deleted leaf at index {0:?} appears in the updated skeleton tree.")]
    DeletedLeafInSkeleton(NodeIndex),

---

crates/committer/src/patricia_merkle_tree/filled_tree/tree.rs line 176 at r5 (raw file):

            UpdatedSkeletonNode::Leaf => {
                let node_data =
                    NodeData::Leaf(LeafDataImpl::create(&index, leaf_modifications).await?);

Return the previous code block

Suggestion:

                let node_data =
                    NodeData::Leaf(LeafDataImpl::create(&index, leaf_modifications).await?);
                    if node_data.is_empty() {...FilledTreeError...

---

crates/committer/src/patricia_merkle_tree/node_data/errors.rs line 19 at r5 (raw file):

pub enum LeafError {
    #[error("Deleted leaf at index {0:?} appears in the updated skeleton tree.")]
    EmptyLeafInModifications(NodeIndex),

See the below comments.

---

crates/committer/src/patricia_merkle_tree/node_data/leaf.rs line 68 at r5 (raw file):

        if leaf_data.is_empty() {
            return Err(LeafError::EmptyLeafInModifications(*index));
        }

This isn't an error in this function, only in the context of appearing in as Leaf in the updated skeleton.

[dorimedini-starkware]

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

---

crates/committer/src/patricia_merkle_tree/filled_tree/errors.rs line 10 at r5 (raw file):

Previously, TzahiTaub (Tzahi) wrote…

Revive error

Done.

---

crates/committer/src/patricia_merkle_tree/filled_tree/tree.rs line 176 at r5 (raw file):

Previously, TzahiTaub (Tzahi) wrote…

Return the previous code block

Done.

---

crates/committer/src/patricia_merkle_tree/node_data/errors.rs line 19 at r5 (raw file):

Previously, TzahiTaub (Tzahi) wrote…

See the below comments.

Done.

---

crates/committer/src/patricia_merkle_tree/node_data/leaf.rs line 68 at r5 (raw file):

Previously, TzahiTaub (Tzahi) wrote…

This isn't an error in this function, only in the context of appearing in as Leaf in the updated skeleton.

ah, right you are, good catch!
moved things back a bit

[TzahiTaub]

Reviewed 4 of 4 files at r6, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @aner-starkware)