Nimrod/sort once

[nimrod-starkware]

This change is 

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 77.77778% with 10 lines in your changes missing coverage. Please review.

Project coverage is 70.68%. Comparing base (2fbab24) to head (8cd1275).

Files	Patch %	Lines
...rkle_tree/updated_skeleton_tree/skeleton_forest.rs	0.00%	8 Missing ⚠️
...kle_tree/original_skeleton_tree/skeleton_forest.rs	93.33%	0 Missing and 2 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #288      +/-   ##
==========================================
+ Coverage   70.65%   70.68%   +0.02%     
==========================================
  Files          38       38              
  Lines        2021     2040      +19     
  Branches     2021     2040      +19     
==========================================
+ Hits         1428     1442      +14     
- Misses        524      529       +5     
  Partials       69       69              

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

[dorimedini-starkware]

Reviewed 1 of 11 files at r1, all commit messages.
Reviewable status: 1 of 11 files reviewed, 1 unresolved discussion (waiting on @nimrod-starkware and @TzahiTaub)

---

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

    pub(crate) fn from_sorted(sorted_indices: &'a [NodeIndex]) -> Self {
        Self(sorted_indices)
    }

this kind of defeats the purpose of the type...
I see this is needed because the create function gets a sorted slice as input?
can you remove this from_sorted method and pass the create function an instance of SortedLeafIndices?

Code quote:

    /// Creates a new instance from the given indices. Assumes indices are sorted.
    pub(crate) fn from_sorted(sorted_indices: &'a [NodeIndex]) -> Self {
        Self(sorted_indices)
    }

[nimrod-starkware]

Reviewable status: 1 of 11 files reviewed, 1 unresolved discussion (waiting on @dorimedini-starkware and @TzahiTaub)

---

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

Previously, dorimedini-starkware wrote…

this kind of defeats the purpose of the type...
I see this is needed because the create function gets a sorted slice as input?
can you remove this from_sorted method and pass the create function an instance of SortedLeafIndices?

I agree; that is what I wanted to do in the first place, but it brings some lifetime issues with it

[TzahiTaub]

Reviewed all commit messages.
Reviewable status: 1 of 11 files reviewed, 2 unresolved discussions (waiting on @dorimedini-starkware and @nimrod-starkware)

---

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

        OriginalSkeletonTreeImpl,
        HashMap<NodeIndex, ContractState>,
        Vec<NodeIndex>,

1. Add the new returned value to the docstring.
2. We would like to return the SortedLeafIndices struct here. I'm not too familiar with specifying lifetimes and how complicated this is, if it's too much, then I suggest removing this struct and just given the name sorted_leaf_indices everywhere, and doc here that we return the sorted leaves. @dorimedini-starkware if we can do some lifetime magic and help the compiler enforce this, please say so.

Code quote:

   Vec<NodeIndex>,