starknet_committer,starknet_patricia: add serde errors

[ArielElp]

No description provided.

[reviewable-StarkWare]

This change is 

[ArielElp]

• starknet_committer,starknet_patricia: move node_serde to facts db #11644
• starknet_committer: reorganize db modules #11640
• starknet_committer: add commit e2e tests #11331
• starknet_committer: index layout forest creation tests #11286
• starknet_committer,starknet_patricia: layout-based original tree creation tests #11236
• starknet_committer: add original tree creation test templates #11715
• starknet_committer,starknet_patricia: layout-dependent db-key separator #11641
• starknet_committer,starknet_patricia: remove FilledNode impl #11637
• starknet_committer: db layout trait #11621
• starknet_committer,starknet_patricia: layout-dependent FilledTree serialization #10988
• starknet_committer,starknet_patricia: refactor key context #11592
• starknet_committer,starknet_patricia: add layout for base trait #11566
• starknet_committer,starknet_patricia: add serialization utilities to node layout #10987
• starknet_committer,starknet_patricia: move db_layout #10986
• starknet_committer: add index db #10685
• starknet_committer: abstract original tree creation #10684
• starknet_committer: enrich layout trait to abstract skeleton creation #11182
• starknet_committer: move trie creation logic to trie_traversal #11180
• starknet_committer: group generic layout logic #10852
• starknet_committer,starknet_patricia: remove generic config #10683
• starknet_committer: add index layout nodes #10682
• starknet_committer: add index layout subtree #10919
• starknet_committer: hash leaves of both layouts #10918
• starknet_committer: add index layout leaves #10681
• starknet_committer,starknet_patricia,starknet_patricia_storage: support contract address prefix #10858
• starknet_committer,starknet_patricia: add serde errors #10810 👈 (View in Graphite)
• starknet_committer,starknet_committer_and_os_cli,starknet_patricia: layout dependent fetch_nodes #10680
• starknet_committer,starknet_patricia: enrich subtree trait to support fetch nodes #10751
• starknet_committer,starknet_committer_and_os_cli,starknet_patricia: make FilledNode generic #10750
• starknet_committer,starknet_os,starknet_patricia: make node data generic #10679
• starknet_committer,starknet_patricia: add deserialization context #10704
• starknet_committer,starknet_patricia,starknet_patricia_storage: add context to prefix and serde #10678
• starknet_committer,starknet_patricia: add subtree trait #10694
• starknet_committer: reorganize facts db files #10677
• main-v0.14.1-committer

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

[yoavGrs]

@yoavGrs reviewed 13 of 13 files at r1, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @ArielElp and @nimrod-starkware)

---

crates/starknet_patricia_storage/src/errors.rs line 15 at r1 (raw file):

#[derive(thiserror::Error, Debug)]
pub enum SerializationError {

Consider define (and use)

pub type SerializationResult<T> = Result<T, SerializationError>;

---

crates/starknet_patricia_storage/src/errors.rs line 19 at r1 (raw file):

    JsonSerializeError(#[from] serde_json::Error),
    #[error(transparent)]
    FeltSerializeError(#[from] std::io::Error),

Why determine that any transparent IO error is on felts?

Suggestion:

IOSerializeError(#[from] std::io::Error),

---

crates/starknet_committer_cli/src/commands.rs line 363 at r1 (raw file):

            .expect("Failed to commit the given block.");
        time_measurement.start_measurement(Action::Write);
        let n_new_facts = facts_db.write(&filled_forest).await.unwrap();

Suggestion:

.except("message...");

[ArielElp]

@ArielElp made 7 comments.
Reviewable status: 10 of 16 files reviewed, 7 unresolved discussions (waiting on @dorimedini-starkware, @nimrod-starkware, and @yoavGrs).

---

crates/starknet_committer/src/db/facts_db/db.rs line 205 at r4 (raw file):

Previously, dorimedini-starkware wrote…

doesn't this work? less changes
(collect can collect a Result<_, _> from an iterator over Results)

Doesn't work as-is since flat_map gives you an iterator over HashMap here, and not an iterator over (k,v) pairs.

Chat suggested this:

        filled_forest
        .storage_tries
        .values()
        .map(|tree| tree.serialize(&EmptyKeyContext))
        .chain(std::iter::once(filled_forest.contracts_trie.serialize(&EmptyKeyContext)))
        .chain(std::iter::once(filled_forest.classes_trie.serialize(&EmptyKeyContext)))
        // Iterator<Item = Result<DbHashMap, SerializationError>>
        .try_fold(DbHashMap::new(), |mut acc, hm| {
            acc.extend(hm?);
            Ok(acc)
        })

Up to you

---

crates/starknet_patricia_storage/src/errors.rs line 21 at r4 (raw file):

Previously, dorimedini-starkware wrote…

alphabetize please

Done.

---

crates/starknet_patricia_storage/src/errors.rs line 21 at r4 (raw file):

Previously, dorimedini-starkware wrote…

I know the JsonSerializeError is not your fault but try to omit the Error suffix on error variants: SerializationError::IOSerialize is better than SerializationError::IOSerializeError

Done.

---

crates/starknet_patricia_storage/src/errors.rs line 48 at r4 (raw file):

Previously, dorimedini-starkware wrote…

while you're here: can you alphabetize this enum?

Done.

---

crates/starknet_patricia/src/patricia_merkle_tree/filled_tree/tree.rs line 49 at r4 (raw file):

        &self,
        key_context: &<L as HasStaticPrefix>::KeyContext,
    ) -> Result<DbHashMap, SerializationError>;

Done.

---

crates/starknet_patricia/src/patricia_merkle_tree/filled_tree/tree.rs line 375 at r4 (raw file):

        &self,
        key_context: &<L as HasStaticPrefix>::KeyContext,
    ) -> Result<DbHashMap, SerializationError> {

Done.

---

crates/starknet_patricia_storage/src/errors.rs line 48 at r4 (raw file):

    ValueError(Box<dyn std::error::Error>),
    #[error("Failed to deserialize raw felt: {0:?}")]
    FeltDeserializationError(DbValue),

Done.

[dorimedini-starkware]

@dorimedini-starkware reviewed 6 files and all commit messages, made 2 comments, and resolved 7 discussions.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @ArielElp).

---

crates/starknet_committer/src/db/facts_db/db.rs line 205 at r4 (raw file):

Previously, ArielElp wrote…

Doesn't work as-is since flat_map gives you an iterator over HashMap here, and not an iterator over (k,v) pairs.

Chat suggested this:

        filled_forest
        .storage_tries
        .values()
        .map(|tree| tree.serialize(&EmptyKeyContext))
        .chain(std::iter::once(filled_forest.contracts_trie.serialize(&EmptyKeyContext)))
        .chain(std::iter::once(filled_forest.classes_trie.serialize(&EmptyKeyContext)))
        // Iterator<Item = Result<DbHashMap, SerializationError>>
        .try_fold(DbHashMap::new(), |mut acc, hm| {
            acc.extend(hm?);
            Ok(acc)
        })

Up to you

nah

---

crates/starknet_committer_and_os_cli/src/committer_cli/tests/python_tests.rs line 81 at r5 (raw file):

    DeserializationTestFailure(#[from] DeserializationError),
    #[error(transparent)]
    SerializationError(#[from] SerializationError),

not-making-it-worse > consistency

Suggestion:

    #[error(transparent)]
    Serialization(#[from] SerializationError),

[ArielElp]

@ArielElp made 1 comment.
Reviewable status: 15 of 16 files reviewed, 1 unresolved discussion (waiting on @dorimedini-starkware).

---

crates/starknet_committer_and_os_cli/src/committer_cli/tests/python_tests.rs line 81 at r5 (raw file):

Previously, dorimedini-starkware wrote…

not-making-it-worse > consistency

Done.

[dorimedini-starkware]

@dorimedini-starkware reviewed 1 file and all commit messages, and resolved 1 discussion.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @ArielElp).

[dorimedini-starkware]

@dorimedini-starkware reviewed 6 files and all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @ArielElp).

[dorimedini-starkware]

@dorimedini-starkware reviewed 1 file and all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @ArielElp).

[dorimedini-starkware]

@dorimedini-starkware reviewed 1 file and all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @ArielElp).