chore: add deserialization types and basic casting implementation

[nimrod-starkware]

Adding deserialized types that will be sent from python to the committer

---

This change is 

[codecov-commenter]

Codecov Report

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

Project coverage is 60.74%. Comparing base (f1572e3) to head (f8a07db).

Files	Patch %	Lines
crates/committer/src/deserialization/types.rs	0.00%	0 Missing and 6 partials ⚠️
crates/committer/src/deserialization/cast.rs	95.74%	0 Missing and 4 partials ⚠️
crates/committer/src/deserialization/errors.rs	0.00%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##             main      #27       +/-   ##
===========================================
+ Coverage   49.25%   60.74%   +11.48%     
===========================================
  Files          11       15        +4     
  Lines         270      377      +107     
  Branches      270      377      +107     
===========================================
+ Hits          133      229       +96     
  Misses        120      120               
- Partials       17       28       +11     

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

[dorimedini-starkware]

Reviewed 8 of 8 files at r1, 2 of 2 files at r2, all commit messages.
Reviewable status: all files reviewed, 20 unresolved discussions (waiting on @nimrod-starkware)

---

crates/committer/Cargo.toml line 17 at r1 (raw file):

[dependencies]
starknet-types-core = "0.0.11"
starknet_api = "0.12.0-dev.0"

it is a mistake that these are here... so while you're making changes:

1. starknet_api is not actually in use; please remove it (udeps should fail)
2. all dependencies should be declared in the root Cargo.toml; please move starknet-types-core = "0.0.11" there and use workspace = true here

Suggestion:

starknet-types-core.workspace = true

---

crates/committer/Cargo.toml line 19 at r1 (raw file):

starknet_api = "0.12.0-dev.0"
serde = { version = "1.0.197", features = ["derive"] }
serde_json = "1.0"

Suggestion:

serde.workspace = true
serde_json.workspace = true

---

crates/committer/src/deserialization_impl.rs line 0 at r1 (raw file):
you are introducing possible panics to business logic; I would rather you use Result types.

1. instead of implementing From, implement TryFrom
2. define an error type for non-unique hashmap deserialization (that's what the assert!s are for, right?)
3. talk to Amos for how to do this; he already introduced errors types, he will have an opinion on where your new error type should be added

---

crates/committer/src/deserialization_impl.rs line 7 at r1 (raw file):

};
use crate::patricia_merkle_tree::filled_node::{ClassHash, Nonce};
use starknet_types_core::felt::Felt;

use internal felt representation

Suggestion:

use crate::types::Felt;

---

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

#[allow(dead_code)]
impl DeserializedInput {
    pub fn actual_input(self) -> ActualInput {

when implementing type conversions, use the standard From trait when possible

Suggestion:

#[allow(dead_code)]
impl From<DeserializedInput> for ActualInput {
    pub fn from(deserialized_input: DeserializedInput) -> Self {

---

crates/committer/src/deserialization_impl.rs line 14 at r1 (raw file):

        let mut storage = HashMap::new();
        for entry in self.storage.into_iter() {
            assert!(storage.insert(entry.key, entry.value).is_none())

remove (see above)

Code quote:

assert!

---

crates/committer/src/deserialization_impl.rs line 19 at r1 (raw file):

        let mut address_to_class_hash = HashMap::new();
        for entry in self.state_diff.address_to_class_hash {
            assert!(address_to_class_hash

remove (see above)

Code quote:

assert!

---

crates/committer/src/deserialization_impl.rs line 29 at r1 (raw file):

        let mut address_to_nonce = HashMap::new();
        for entry in self.state_diff.address_to_nonce {
            assert!(address_to_nonce

remove (see above)

Code quote:

assert!

---

crates/committer/src/deserialization_impl.rs line 39 at r1 (raw file):

        let mut class_hash_to_compiled_class_hash = HashMap::new();
        for entry in self.state_diff.class_hash_to_compiled_class_hash {
            assert!(class_hash_to_compiled_class_hash

remove (see above)

Code quote:

assert!

---

crates/committer/src/deserialization_impl.rs line 49 at r1 (raw file):

        let mut current_contract_state_leaves = HashMap::new();
        for entry in self.state_diff.current_contract_state_leaves {
            assert!(current_contract_state_leaves

remove (see above)

Code quote:

assert!

---

crates/committer/src/deserialization_impl.rs line 74 at r1 (raw file):

                .collect();
            storage_updates.insert(Felt::from_bytes_be_slice(&outer_entry.address), tmp_map);
        }

you don't check for uniqueness here?
(not with assert, but you could return an Err)

Code quote:

        for outer_entry in self.state_diff.storage_updates {
            let tmp_map: HashMap<Felt, Felt> = outer_entry
                .storage_updates
                .iter()
                .map(|inner_entry| {
                    (
                        Felt::from_bytes_be_slice(&inner_entry.key),
                        Felt::from_bytes_be_slice(&inner_entry.value),
                    )
                })
                .collect();
            storage_updates.insert(Felt::from_bytes_be_slice(&outer_entry.address), tmp_map);
        }

---

crates/committer/src/deserialization_types.rs line 0 at r1 (raw file):
WDYM by "deserialized" in this context?
It looks like the ActualX types are "deserialized";
what you call DeserializedX is actually the serialized version of X, right?

---

crates/committer/src/deserialization_types.rs line 3 at r1 (raw file):

use crate::patricia_merkle_tree::filled_node::{ClassHash, Nonce};
use serde::Deserialize;
use starknet_types_core::felt::Felt;

Suggestion:

use crate::types::Felt;

---

crates/committer/src/deserialization_types.rs line 6 at r1 (raw file):

use std::collections::HashMap;

type DeserializedFelt = Vec<u8>;

field elements are fixed size

Suggestion:

type SerializedFelt = [u8; 32];

---

crates/committer/src/deserialization_types.rs line 7 at r1 (raw file):

type DeserializedFelt = Vec<u8>;
type Bytes = Vec<u8>;

1. split key and value types
2. wrap, not type alias

Suggestion:

struct StorageKey(Vec<u8>);
struct StorageValue(Vec<u8>);

---

crates/committer/src/deserialization_types.rs line 8 at r1 (raw file):

type DeserializedFelt = Vec<u8>;
type Bytes = Vec<u8>;
type Address = Felt;

1. you are overloading this type to mean two things; split the types
2. use wrapper types (struct) not type aliases (type) to make sure these types are not interchangeable
3. these "should" be defined in starknet-api but they use the "wrong" Felt type, so put TODOs
4. use the name StarknetStorageKey to differentiate between the storage key of a leaf in a starknet contract state tree, and a storage key in our storage abstraction

Suggestion:

// TODO(Nimrod, 1/6/2024): Swap to starknet-types-core types once implemented.
struct ContractAddress(Felt);
// TODO(Nimrod, 1/6/2024): Swap to starknet-types-core types once implemented.
struct StarknetStorageKey(Felt);

---

crates/committer/src/deserialization_types.rs line 22 at r1 (raw file):

#[allow(dead_code)]
pub(crate) struct ActualInput {
    pub storage: HashMap<Bytes, Bytes>,

Suggestion:

HashMap<StorageKey, StorageValue>

---

crates/committer/src/deserialization_types.rs line 31 at r1 (raw file):

pub(crate) struct DeserializedStorageEntry {
    pub key: Bytes,
    pub value: Bytes,

Suggestion:

    pub key: StorageKey,
    pub value: StorageValue,

---

crates/committer/src/deserialization_types.rs line 82 at r1 (raw file):

    pub class_hash_to_compiled_class_hash: HashMap<ClassHash, ClassHash>,
    pub current_contract_state_leaves: HashMap<Address, ContractState>,
    pub storage_updates: HashMap<Address, HashMap<Address, Felt>>,

Suggestion:

HashMap<ContractAddress, HashMap<StarknetStorageKey, Felt>>

---

crates/committer/src/deserialization_types.rs line 89 at r1 (raw file):

    pub nonce: Nonce,
    pub class_hash: ClassHash,
    pub storage_root_hash: Felt,

is this the correct type? WDYT?

Suggestion:

pub storage_root_hash: HashOutput,

[nimrod-starkware]

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

---

crates/committer/Cargo.toml line 17 at r1 (raw file):

Previously, dorimedini-starkware wrote…

it is a mistake that these are here... so while you're making changes:

1. starknet_api is not actually in use; please remove it (udeps should fail)
2. all dependencies should be declared in the root Cargo.toml; please move starknet-types-core = "0.0.11" there and use workspace = true here

Done.

---

crates/committer/Cargo.toml line 19 at r1 (raw file):

starknet_api = "0.12.0-dev.0"
serde = { version = "1.0.197", features = ["derive"] }
serde_json = "1.0"

Done.

---

crates/committer/src/deserialization_impl.rs line at r1 (raw file):

Previously, dorimedini-starkware wrote…

you are introducing possible panics to business logic; I would rather you use Result types.

1. instead of implementing From, implement TryFrom
2. define an error type for non-unique hashmap deserialization (that's what the assert!s are for, right?)
3. talk to Amos for how to do this; he already introduced errors types, he will have an opinion on where your new error type should be added

will do.

---

crates/committer/src/deserialization_types.rs line 3 at r1 (raw file):

use crate::patricia_merkle_tree::filled_node::{ClassHash, Nonce};
use serde::Deserialize;
use starknet_types_core::felt::Felt;

Done.

---

crates/committer/src/deserialization_types.rs line 6 at r1 (raw file):

Previously, dorimedini-starkware wrote…

field elements are fixed size

Done.

---

crates/committer/src/deserialization_types.rs line 7 at r1 (raw file):

Previously, dorimedini-starkware wrote…

1. split key and value types
2. wrap, not type alias

Done.

---

crates/committer/src/deserialization_types.rs line 8 at r1 (raw file):

Previously, dorimedini-starkware wrote…

1. you are overloading this type to mean two things; split the types
2. use wrapper types (struct) not type aliases (type) to make sure these types are not interchangeable
3. these "should" be defined in starknet-api but they use the "wrong" Felt type, so put TODOs
4. use the name StarknetStorageKey to differentiate between the storage key of a leaf in a starknet contract state tree, and a storage key in our storage abstraction

Done.

---

crates/committer/src/deserialization_types.rs line 22 at r1 (raw file):

#[allow(dead_code)]
pub(crate) struct ActualInput {
    pub storage: HashMap<Bytes, Bytes>,

Done.

---

crates/committer/src/deserialization_types.rs line 89 at r1 (raw file):

Previously, dorimedini-starkware wrote…

is this the correct type? WDYT?

Done.

---

crates/committer/src/deserialization_types.rs line at r1 (raw file):

Previously, dorimedini-starkware wrote…

WDYM by "deserialized" in this context?
It looks like the ActualX types are "deserialized";
what you call DeserializedX is actually the serialized version of X, right?

What i mean is:
DeserializedX is some way to represent X that can be easily deserialized with a json reader.
ActualX is how we actually want to use those objects, with the correct types.
The casting is always DeserializedX => ActualX

I know the names are bad though...

Maybe i should change it?
changing DeserializedX => RawX, ActualX =>DeserializedX is better?

[dorimedini-starkware]

Reviewed 5 of 5 files at r3, all commit messages.
Reviewable status: all files reviewed, 12 unresolved discussions (waiting on @nimrod-starkware)

---

Cargo.toml line 22 at r3 (raw file):

[workspace.lints.rust]

remove extra newline (non blocking)

Suggestion:

starknet-types-core = "0.0.11"

[workspace.lints.rust]

---

crates/committer/src/deserialization_impl.rs line 75 at r3 (raw file):

                    (
                        StarknetStorageKey(Felt::from_bytes_be_slice(&inner_entry.key)),
                        Felt::from_bytes_be_slice(&inner_entry.value),

define a wrapper type for this please

Suggestion:

StarknetStorageValue(Felt::from_bytes_be_slice(&inner_entry.value))

---

crates/committer/src/deserialization_types.rs line at r1 (raw file):

Previously, nimrod-starkware wrote…

What i mean is:
DeserializedX is some way to represent X that can be easily deserialized with a json reader.
ActualX is how we actually want to use those objects, with the correct types.
The casting is always DeserializedX => ActualX

I know the names are bad though...

Maybe i should change it?
changing DeserializedX => RawX, ActualX =>DeserializedX is better?

yes, raw and deserialized is better.
although, instead of DeserializedX you could just go with X

[nimrod-starkware]

Reviewable status: 2 of 11 files reviewed, 12 unresolved discussions (waiting on @dorimedini-starkware)

---

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

Previously, dorimedini-starkware wrote…

when implementing type conversions, use the standard From trait when possible

Done.

---

crates/committer/src/deserialization_impl.rs line 14 at r1 (raw file):

Previously, dorimedini-starkware wrote…

remove (see above)

Done.

---

crates/committer/src/deserialization_impl.rs line 19 at r1 (raw file):

Previously, dorimedini-starkware wrote…

remove (see above)

Done.

---

crates/committer/src/deserialization_impl.rs line 29 at r1 (raw file):

Previously, dorimedini-starkware wrote…

remove (see above)

Done.

---

crates/committer/src/deserialization_impl.rs line 39 at r1 (raw file):

Previously, dorimedini-starkware wrote…

remove (see above)

Done.

---

crates/committer/src/deserialization_impl.rs line 49 at r1 (raw file):

Previously, dorimedini-starkware wrote…

remove (see above)

Done.

---

crates/committer/src/deserialization_impl.rs line at r1 (raw file):

Previously, nimrod-starkware wrote…

will do.

added a key duplicate error with this crate (it's used in the blockifier repository)

---

crates/committer/src/deserialization_impl.rs line 75 at r3 (raw file):

Previously, dorimedini-starkware wrote…

define a wrapper type for this please

Done.

---

crates/committer/src/deserialization_types.rs line at r1 (raw file):

Previously, dorimedini-starkware wrote…

yes, raw and deserialized is better.
although, instead of DeserializedX you could just go with X

Done.

[dorimedini-starkware]

Reviewed 6 of 9 files at r4, 2 of 2 files at r5, 2 of 2 files at r6, 6 of 6 files at r7, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @nimrod-starkware)

---

crates/committer/src/deserialization/cast.rs line 1 at r5 (raw file):

use super::errors::DeserializationError;

please implement and use this helper function for your hashmap constructions

fn<K, V> add_unique(map: &mut HashMap<K, V>, map_name: &str, key: &K, value: &V) -> Result<(), DeserializationError> {
    if map.insert(key, value).is_some() {
        return Err(DeserializationError::KeyDuplicate(format!("{map_name}: {key:?}"));
    }
    Ok(())
}

---

crates/committer/src/deserialization/cast.rs line 7 at r5 (raw file):

use crate::patricia_merkle_tree::filled_node::{ClassHash, Nonce};
use crate::types::Felt;
use std::collections::HashMap;

please add a simple test for this: add a cast_test.rs module, create a dummy RawInput struct and assert the induced Input struct is as you expect

Suggestion:

use std::collections::HashMap;

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

---

crates/committer/src/deserialization/types.rs line 33 at r7 (raw file):

    /// The height of the patricia tree.
    // TODO(Nimrod,20/4/2024): Strong assumption - all trees have same height. How can I get
    // rid of it?

(not in this PR) you can:

1. define a const in this repo
2. add a test in python that checks consistency between the height on the python side and the height here

Code quote:

    // TODO(Nimrod,20/4/2024): Strong assumption - all trees have same height. How can I get
    // rid of it?

[dorimedini-starkware]

Reviewed 6 of 6 files at r8, all commit messages.
Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @nimrod-starkware)

---

crates/committer/src/deserialization/cast.rs line 1 at r8 (raw file):

use super::errors::DeserializationError;

Is it possible to do this entire deserialization using only serde attributes? without defining any intermediate Raw structs?

---

crates/committer/src/deserialization/cast.rs line 2 at r8 (raw file):

use super::errors::DeserializationError;
use super::types::{Input, RawInput, StarknetStorageKey, StarknetStorageValue, StateDiff};

didn't notice before: don't use super in non-test modules, import by crate:: path explicitly

Code quote:

use super::errors::DeserializationError;
use super::types::{Input, RawInput, StarknetStorageKey, StarknetStorageValue, StateDiff};

---

crates/committer/src/deserialization/cast.rs line 50 at r8 (raw file):

                "class hash to compiled class hash",
                ClassHash(Felt::from_bytes_be_slice(&entry.key)),
                ClassHash(Felt::from_bytes_be_slice(&entry.value)),

also didn't notice this before:
these two shouldn't be the same type.
the key is the hash of the sierra code, and the value is the hash of the CASM (compiled) code.
I think the terminology we use for the latter is CompiledClassHash

Suggestion:

                ClassHash(Felt::from_bytes_be_slice(&entry.key)),
                CompiledClassHash(Felt::from_bytes_be_slice(&entry.value)),

---

crates/committer/src/deserialization/cast.rs line 104 at r8 (raw file):

}

pub(crate) fn add_unique<K, V>(

this util is only needed for this module

Suggestion:

fn add_unique<K, V>(

[dorimedini-starkware]

Reviewed 2 of 2 files at r9, all commit messages.
Reviewable status: all files reviewed, 10 unresolved discussions (waiting on @nimrod-starkware)

---

crates/committer/src/deserialization/cast.rs line 9 at r9 (raw file):

use crate::storage::storage_trait::{StorageKey, StorageValue};
use crate::types::Felt;
use starknet_types_core::felt::Felt as StarknetTypesFelt;

do not use this type explicitly. only use our wrapper type.
implement a from_bytes method on our internal Felt instead

Code quote:

starknet_types_core::felt::Felt as StarknetTypesFelt;

[nimrod-starkware]

Reviewable status: 12 of 13 files reviewed, 10 unresolved discussions (waiting on @dorimedini-starkware)

---

crates/committer/src/deserialization/cast.rs line 1 at r5 (raw file):

Previously, dorimedini-starkware wrote…

please implement and use this helper function for your hashmap constructions

fn<K, V> add_unique(map: &mut HashMap<K, V>, map_name: &str, key: &K, value: &V) -> Result<(), DeserializationError> {
    if map.insert(key, value).is_some() {
        return Err(DeserializationError::KeyDuplicate(format!("{map_name}: {key:?}"));
    }
    Ok(())
}

Done.

---

crates/committer/src/deserialization/cast.rs line 7 at r5 (raw file):

Previously, dorimedini-starkware wrote…

please add a simple test for this: add a cast_test.rs module, create a dummy RawInput struct and assert the induced Input struct is as you expect

will do

---

crates/committer/src/deserialization/cast.rs line 1 at r8 (raw file):

Previously, dorimedini-starkware wrote…

Is it possible to do this entire deserialization using only serde attributes? without defining any intermediate Raw structs?

we agreed to keep it that way

---

crates/committer/src/deserialization/cast.rs line 2 at r8 (raw file):

Previously, dorimedini-starkware wrote…

didn't notice before: don't use super in non-test modules, import by crate:: path explicitly

Done.

---

crates/committer/src/deserialization/cast.rs line 50 at r8 (raw file):

Previously, dorimedini-starkware wrote…

also didn't notice this before:
these two shouldn't be the same type.
the key is the hash of the sierra code, and the value is the hash of the CASM (compiled) code.
I think the terminology we use for the latter is CompiledClassHash

Done.

---

crates/committer/src/deserialization/cast.rs line 104 at r8 (raw file):

Previously, dorimedini-starkware wrote…

this util is only needed for this module

Done.

---

crates/committer/src/deserialization/cast.rs line 9 at r9 (raw file):

Previously, dorimedini-starkware wrote…

do not use this type explicitly. only use our wrapper type.
implement a from_bytes method on our internal Felt instead

Done.

[dorimedini-starkware]

Reviewed 1 of 1 files at r10, all commit messages.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @nimrod-starkware)

---

crates/committer/src/deserialization/cast.rs line 2 at r8 (raw file):

Previously, nimrod-starkware wrote…

Done.

I still see super:: here

---

crates/committer/src/deserialization/cast.rs line 50 at r8 (raw file):

Previously, nimrod-starkware wrote…

Done.

don't see it

---

crates/committer/src/deserialization/cast.rs line 9 at r9 (raw file):

Previously, nimrod-starkware wrote…

Done.

still here

[dorimedini-starkware]

Reviewed 11 of 11 files at r11, all commit messages.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @nimrod-starkware)

---

crates/committer/src/types.rs line 44 at r11 (raw file):

    pub fn pow(&self, exponent: impl Into<u128>) -> Self {
        Self(self.0.pow(exponent.into()))
    }

Suggestion:

    pub(crate) const ZERO: Felt = Felt(StarknetTypesFelt::ZERO);
    pub(crate) const ONE: Felt = Felt(StarknetTypesFelt::ONE);
    pub(crate) const TWO: Felt = Felt(StarknetTypesFelt::TWO);
    
    pub(crate) fn from_bytes_be_slice(bytes: &[u8]) -> Self {
        Self(StarknetTypesFelt::from_bytes_be_slice(bytes))
    }
    
    /// Raises `self` to the power of `exponent`.
    pub(crate) fn pow(&self, exponent: impl Into<u128>) -> Self {
        Self(self.0.pow(exponent.into()))
    }

---

crates/committer/src/deserialization/errors.rs line 18 at r11 (raw file):

        DeserializationError::ParsingError
    }
}

Suggestion:

    #[error("Couldn't read and parse the given input JSON: {0}")]
    ParsingError(#[from] serde_json::Error),
}

---

crates/committer/src/deserialization/read.rs line 10 at r11 (raw file):

pub(crate) fn parse_input(input: String) -> DeserializationResult<Input> {
    let raw_input: RawInput = serde_json::from_str(&input)?;
    raw_input.try_into()

Suggestion:

    serde_json::from_str::<RawInput>(&input)?.try_into()

---

crates/committer/src/deserialization/read.rs line 17 at r11 (raw file):

fn test_input_parsing() {
    todo!()
}

1. move this test to it's own module
2. move the pub mod instruction to the top, just under all the uses

Suggestion:

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

[nimrod-starkware]

Reviewable status: 11 of 14 files reviewed, 8 unresolved discussions (waiting on @dorimedini-starkware)

---

crates/committer/src/types.rs line 44 at r11 (raw file):

    pub fn pow(&self, exponent: impl Into<u128>) -> Self {
        Self(self.0.pow(exponent.into()))
    }

Done.

---

crates/committer/src/deserialization/errors.rs line 18 at r11 (raw file):

        DeserializationError::ParsingError
    }
}

Done.

---

crates/committer/src/deserialization/read.rs line 10 at r11 (raw file):

pub(crate) fn parse_input(input: String) -> DeserializationResult<Input> {
    let raw_input: RawInput = serde_json::from_str(&input)?;
    raw_input.try_into()

Done.

---

crates/committer/src/deserialization/read.rs line 17 at r11 (raw file):

Previously, dorimedini-starkware wrote…

1. move this test to it's own module
2. move the pub mod instruction to the top, just under all the uses

Done.

[dorimedini-starkware]

Reviewed 3 of 3 files at r12, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @nimrod-starkware)

---

crates/committer/src/types.rs line 44 at r11 (raw file):

Previously, nimrod-starkware wrote…

Done.

no, add (crate) to pubs

---

crates/committer/src/deserialization/read.rs line 22 at r12 (raw file):

        assert!(parse_input(input.to_string()).is_ok());
    }
}

not quite :)
put the contents in a new file (read_test.rs)

Suggestion:

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

[nimrod-starkware]

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

---

crates/committer/src/types.rs line 44 at r11 (raw file):

Previously, dorimedini-starkware wrote…

no, add (crate) to pubs

done

---

crates/committer/src/deserialization/read.rs line 22 at r12 (raw file):

Previously, dorimedini-starkware wrote…

not quite :)
put the contents in a new file (read_test.rs)

Done.

[dorimedini-starkware]

Reviewed 4 of 4 files at r13, all commit messages.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @nimrod-starkware)

---

crates/committer/src/deserialization/read_test.rs line 89 at r13 (raw file):

"#;

    assert!(parse_input(input.to_string()).is_ok());

for a basic test, I would prefer you compare explicitly to an expected deserialized object.
use pretty_assertions::assert_eq (may need to add the crate in cargo toml), construct the expected Input object explicitly and compare with assert_eq (may need to add derive(Eq, PartialEq) on Input for this to work)

Code quote:

assert!(parse_input(input.to_string()).is_ok());

---

crates/committer/src/deserialization/read_test.rs line 119 at r13 (raw file):

    assert!(parse_input(input.to_string())
        .is_err_and(|err| { matches!(err, DeserializationError::KeyDuplicate(_)) }));

add assert_matches to our cargo toml and use it to check the expected error more explicitly

Suggestion:

    assert_matches!(
        parse_input(input.to_string()),
        Err(DeserializationError::KeyDuplicate(key))
        if key == "[14,6,78,90]"
    );

---

crates/committer/src/deserialization/read_test.rs line 158 at r13 (raw file):

    assert!(parse_input(input.to_string())
        .is_err_and(|err| { matches!(err, DeserializationError::KeyDuplicate(_)) }));

use assert_matches (see above)

Code quote:

    assert!(parse_input(input.to_string())
        .is_err_and(|err| { matches!(err, DeserializationError::KeyDuplicate(_)) }));

---

crates/committer/src/deserialization/read_test.rs line 188 at r13 (raw file):

    assert!(parse_input(input.to_string())
        .is_err_and(|err| { matches!(err, DeserializationError::ParsingError(_)) }));

use assert_matches (see above)

Code quote:

    assert!(parse_input(input.to_string())
        .is_err_and(|err| { matches!(err, DeserializationError::ParsingError(_)) }));

[nimrod-starkware]

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

---

crates/committer/src/deserialization/read_test.rs line 89 at r13 (raw file):

Previously, dorimedini-starkware wrote…

for a basic test, I would prefer you compare explicitly to an expected deserialized object.
use pretty_assertions::assert_eq (may need to add the crate in cargo toml), construct the expected Input object explicitly and compare with assert_eq (may need to add derive(Eq, PartialEq) on Input for this to work)

Done.

---

crates/committer/src/deserialization/read_test.rs line 119 at r13 (raw file):

Previously, dorimedini-starkware wrote…

add assert_matches to our cargo toml and use it to check the expected error more explicitly

vscode doesn't like assert_matches!
rust-lang/rust#82775

---

crates/committer/src/deserialization/read_test.rs line 158 at r13 (raw file):

Previously, dorimedini-starkware wrote…

use assert_matches (see above)

vscode doesn't like assert_matches!
rust-lang/rust#82775

---

crates/committer/src/deserialization/read_test.rs line 188 at r13 (raw file):

Previously, dorimedini-starkware wrote…

use assert_matches (see above)

vscode doesn't like assert_matches!
rust-lang/rust#82775

[dorimedini-starkware]

Reviewed 10 of 10 files at r14, all commit messages.
Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @nimrod-starkware)

---

crates/committer/src/deserialization/read_test.rs line 214 at r14 (raw file):

"#;
    let expected_error = "storage: StorageKey([14, 6, 78, 90])";
    assert!(parse_input(input.to_string()).is_err_and(|err| {

use unwrap_err as mentioned below

---

crates/committer/src/deserialization/read_test.rs line 217 at r14 (raw file):

        matches!(err, DeserializationError::KeyDuplicate(key) if key == expected_error)
    }));
}

use unwrap_err

Suggestion:

    assert!(matches!(
        parse_input(input.to_string()).unwrap_err(),
        DeserializationError::KeyDuplicate(key) if key == expected_error
    ));
}

---

crates/committer/src/deserialization/read_test.rs line 255 at r14 (raw file):

    let expected_error = 
    "address to class hash: ContractAddress(Felt(Felt(FieldElement { value: UnsignedInteger { limbs: [72718179, 18446744073709551615, 6917529027641073992, 16140901064500135204] } })))";
    assert!(parse_input(input.to_string()).is_err_and(|err| {

use unwrap_err as above

---

crates/committer/src/deserialization/read_test.rs line 286 at r14 (raw file):

"#;

    assert!(parse_input(input.to_string())

use unwrap_err as above

---

crates/committer/src/deserialization/types.rs line 18 at r14 (raw file):

#[allow(dead_code)]
#[derive(Debug, Eq, PartialEq)]
pub(crate) struct StarknetStorageValue(pub Felt);

newlines around structs

Suggestion:

type RawFelt = [u8; 32];

#[derive(Debug, PartialEq, Eq, Hash)]
// TODO(Nimrod, 1/6/2024): Swap to starknet-types-core types once implemented.
pub(crate) struct ContractAddress(pub Felt);

#[derive(Debug, PartialEq, Eq, Hash)]
// TODO(Nimrod, 1/6/2024): Swap to starknet-types-core types once implemented.
pub(crate) struct StarknetStorageKey(pub Felt);

#[allow(dead_code)]
#[derive(Debug, Eq, PartialEq)]
pub(crate) struct StarknetStorageValue(pub Felt);

---

crates/committer/src/deserialization/types.rs line 92 at r14 (raw file):

}
#[allow(dead_code)]
#[derive(Debug, Eq, PartialEq)]

newline between structs

Code quote:

}
#[allow(dead_code)]
#[derive(Debug, Eq, PartialEq)]

[nimrod-starkware]

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

---

crates/committer/src/deserialization/cast.rs line 7 at r5 (raw file):

Previously, nimrod-starkware wrote…

will do

Done.

---

crates/committer/src/deserialization/read_test.rs line 214 at r14 (raw file):

Previously, dorimedini-starkware wrote…

use unwrap_err as mentioned below

Done.

---

crates/committer/src/deserialization/read_test.rs line 217 at r14 (raw file):

Previously, dorimedini-starkware wrote…

use unwrap_err

Done.

---

crates/committer/src/deserialization/read_test.rs line 255 at r14 (raw file):

Previously, dorimedini-starkware wrote…

use unwrap_err as above

Done.

---

crates/committer/src/deserialization/read_test.rs line 286 at r14 (raw file):

Previously, dorimedini-starkware wrote…

use unwrap_err as above

Done.

---

crates/committer/src/deserialization/types.rs line 18 at r14 (raw file):

Previously, dorimedini-starkware wrote…

newlines around structs

Done.

---

crates/committer/src/deserialization/types.rs line 92 at r14 (raw file):

Previously, dorimedini-starkware wrote…

newline between structs

Done.

[dorimedini-starkware]

Reviewed 6 of 6 files at r15, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @nimrod-starkware)

---

crates/committer/src/deserialization/read.rs line 14 at r15 (raw file):

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

sorry, missed this...

1. newlines
2. move the test declaration to under the uses

Suggestion:

use crate::deserialization::types::RawInput;

use crate::deserialization::errors::DeserializationError;

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

#[allow(dead_code)]
type DeserializationResult<T> = Result<T, DeserializationError>;

#[allow(dead_code)]
pub(crate) fn parse_input(input: String) -> DeserializationResult<Input> {
    serde_json::from_str::<RawInput>(&input)?.try_into()
}

---

crates/committer/src/deserialization/read_test.rs line 216 at r15 (raw file):

    assert!(matches!(parse_input(input.to_string()).unwrap_err(),
    DeserializationError::KeyDuplicate(key) if key == expected_error
    ));

no auto-formatting in macros... try like this (non blocking)

Suggestion:

    assert!(matches!(
        parse_input(input.to_string()).unwrap_err(),
        DeserializationError::KeyDuplicate(key) if key == expected_error
    ));

---

crates/committer/src/deserialization/read_test.rs line 255 at r15 (raw file):

    let expected_error = 
    "address to class hash: ContractAddress(Felt(Felt(FieldElement { value: UnsignedInteger { limbs: [72718179, 18446744073709551615, 6917529027641073992, 16140901064500135204] } })))";
    assert!(matches!(parse_input(input.to_string()).unwrap_err(),

formatting

---

crates/committer/src/deserialization/read_test.rs line 286 at r15 (raw file):

"#;

    assert!(matches!(parse_input(input.to_string()).unwrap_err(),

formatting

[dorimedini-starkware]

Reviewed 1 of 1 files at r16, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @nimrod-starkware)

[nimrod-starkware]

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

---

crates/committer/src/deserialization/read.rs line 14 at r15 (raw file):

Previously, dorimedini-starkware wrote…

sorry, missed this...

1. newlines
2. move the test declaration to under the uses

Done.

[dorimedini-starkware]

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