refactor: remove dependency between storage and starknet prefix

[AvivYossef-starkware]

This change is 

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 80.95238% with 4 lines in your changes missing coverage. Please review.

Project coverage is 70.54%. Comparing base (1d383c5) to head (0b1ec9d).

Files	Patch %	Lines
...src/patricia_merkle_tree/filled_tree/node_serde.rs	0.00%	2 Missing ⚠️
...r/src/patricia_merkle_tree/node_data/leaf_serde.rs	83.33%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #308      +/-   ##
==========================================
- Coverage   70.59%   70.54%   -0.06%     
==========================================
  Files          38       38              
  Lines        2095     2091       -4     
  Branches     2095     2091       -4     
==========================================
- Hits         1479     1475       -4     
  Misses        546      546              
  Partials       70       70              

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

[dorimedini-starkware]

Reviewed all commit messages.
Reviewable status: 0 of 7 files reviewed, 1 unresolved discussion (waiting on @AvivYossef-starkware)

---

crates/committer/src/patricia_merkle_tree/node_data/leaf_serde.rs line 40 at r1 (raw file):

        StarknetPrefix::CompiledClassLeaf.to_storage_prefix()
    }
}

I suggest you give a better return type than Vec<u8> in get_prefix, how about this?
(trait definition shouldn't be here, obviously, but this is my idea)

Suggestion:

#[cfg(test)]
#[path = "leaf_serde_test.rs"]
pub mod leaf_serde_test;

pub trait StoragePrefix {
    fn to_bytes(&self) -> Vec<u8>;
}

impl StoragePrefix for StarknetPrefix {
    ...
}

impl DBObject for StarknetStorageValue {
    /// Serializes the value into a 32-byte vector.
    fn serialize(&self) -> StorageValue {
        StorageValue(self.0.to_bytes_be().to_vec())
    }

    fn get_prefix(&self) -> impl StoragePrefix {
        StarknetPrefix::StorageLeaf
    }
}

impl DBObject for CompiledClassHash {
    /// Creates a json string describing the leaf and casts it into a byte vector.
    fn serialize(&self) -> StorageValue {
        let json_string = format!(r#"{{"compiled_class_hash": "{}"}}"#, self.0.to_hex());
        StorageValue(json_string.into_bytes())
    }

    fn get_prefix(&self) -> impl StoragePrefix {
        StarknetPrefix::CompiledClassLeaf
    }
}

[AvivYossef-starkware]

• refactor: use mock test utils in update skeleton tests #320
• refactor: use mock test utils in filled tree test #319
• refactor: use mock leaf in create tree test #316
• build: impl mock test utils #314
• refactor: remove dependency between storage and starknet prefix #308 👈
• main

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @AvivYossef-starkware and the rest of your teammates on Graphite

[github-actions]

Benchmark movements:
tree_computation_flow performance regressed!
tree_computation_flow time: [34.435 ms 34.845 ms 35.343 ms]
change: [+2.5922% +3.8014% +5.2853%] (p = 0.00 < 0.05)
Performance has regressed.
Found 7 outliers among 100 measurements (7.00%)
1 (1.00%) high mild
6 (6.00%) high severe

[dorimedini-starkware]

Reviewed 2 of 7 files at r1, 4 of 4 files at r2, 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @AvivYossef-starkware)

---

crates/committer/src/patricia_merkle_tree/original_skeleton_tree/create_tree.rs line 238 at r3 (raw file):

                        L::prefix()
                    } else {
                        StarknetPrefix::InnerNode.to_storage_prefix()

is calculate_subtree_roots part of the future patricia crate?
if so, how will this work without the StarknetPrefix (starknet implementation detail)?
please add a TODO about it if you're still unsure, out of scope of the current PR anyway

Code quote:

StarknetPrefix::InnerNode.to_storage_prefix()

---

crates/committer/src/storage/db_object.rs line 9 at r3 (raw file):

    /// Returns the storage key prefix of the DB object.
    fn get_prefix(&self) -> Vec<u8>;

Suggestion:

// TODO(Aviv, 1/8/2024): Define a trait `T` for storage prefix and return `impl T` here.
fn get_prefix(&self) -> Vec<u8>;

---

crates/committer/src/storage/db_object.rs line 22 at r3 (raw file):

    /// The prefix used to store in DB.
    fn prefix() -> Vec<u8>;

Suggestion:

    /// The prefix used to store in DB.
    // TODO(Aviv, 1/8/2024): Define a trait `T` for storage prefix and return `impl T` here.
    fn prefix() -> Vec<u8>;

---

crates/committer/src/storage/errors.rs line 33 at r3 (raw file):

    PathToBottomError(#[from] PathToBottomError),
    #[error("Unexpected prefix ({0:?}) variant when deserializing a leaf.")]
    LeafPrefixError(Vec<u8>),

Suggestion:

    #[error("Unexpected prefix ({0:?}) variant when deserializing a leaf.")]
    // TODO(Aviv, 1/8/2024): Define a trait `T` for storage prefix and return `impl T` here.
    LeafPrefixError(Vec<u8>),

---

crates/committer/src/storage/storage_trait.rs line 33 at r3 (raw file):

}

// TODO(Aviv, 17/07/2024); Split between Storage and node specific implementation.

Suggestion:

// TODO(Aviv, 17/07/2024); Split between Storage prefix representation (trait) and node
//   specific implementation (enum).

[AvivYossef-starkware]

Reviewable status: 3 of 7 files reviewed, 5 unresolved discussions (waiting on @dorimedini-starkware)

---

crates/committer/src/patricia_merkle_tree/original_skeleton_tree/create_tree.rs line 238 at r3 (raw file):

Previously, dorimedini-starkware wrote…

is calculate_subtree_roots part of the future patricia crate?
if so, how will this work without the StarknetPrefix (starknet implementation detail)?
please add a TODO about it if you're still unsure, out of scope of the current PR anyway

calculate_subtree_root is part of the committer logic. It uses StarknetPrefix only for the inner node. It should depend on the storage, as we discussed today.
I add TODO

Code snippet:

fn calculate_subtrees_roots<L: Leaf>(
        subtrees: &[SubTree<'a>],
        storage: &impl Storage,
    ) -> OriginalSkeletonTreeResult<Vec<FilledNode<L>>> {

---

crates/committer/src/storage/db_object.rs line 9 at r3 (raw file):

    /// Returns the storage key prefix of the DB object.
    fn get_prefix(&self) -> Vec<u8>;

Done.

---

crates/committer/src/storage/db_object.rs line 22 at r3 (raw file):

    /// The prefix used to store in DB.
    fn prefix() -> Vec<u8>;

Done.

---

crates/committer/src/storage/errors.rs line 33 at r3 (raw file):

    PathToBottomError(#[from] PathToBottomError),
    #[error("Unexpected prefix ({0:?}) variant when deserializing a leaf.")]
    LeafPrefixError(Vec<u8>),

Done.

---

crates/committer/src/storage/storage_trait.rs line 33 at r3 (raw file):

}

// TODO(Aviv, 17/07/2024); Split between Storage and node specific implementation.

Done.

[dorimedini-starkware]

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

[AvivYossef-starkware]

Merge activity

• Jul 17, 12:52 PM EDT: @AvivYossef-starkware started a stack merge that includes this pull request via Graphite.