Class Hash

[marioiordanov]

The PR includes:

• Implementation of v0 contract class hash computation
• StarkFeltAsDecimal struct, which serialises to decimal string when being used
• Usage of "preserve_order", "arbitrary_precision" of the serde json crate

---

This change is 

[dan-starkware]

Thanks, looks great, sorry for the long delay. Some general comments:

• Please separate into smaller PRs. (keccak, Felt as decimal, class hash)
• Please add documentation.
• Consider reordering code "from more to less interesting for a reader" (public before private, etc.)
• Please add punctuation marks, specifically full stops, to comments.

Reviewed 2 of 10 files at r1, 7 of 9 files at r2, all commit messages.
Reviewable status: 9 of 11 files reviewed, 10 unresolved discussions (waiting on @marioiordanov)

---

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

indexmap = { version = "1.9.2", features = ["serde"] }
once_cell = { version = "1.16.0" }
sha3 = "0.10.6"

Please use { version = "0.10.6"} for consistency.

Code quote:

sha3 = "0.10.6"

---

src/core_test.rs line 70 at r3 (raw file):

#[test]
fn test_contract_class_hash_generation() {

Please remove test_ prefix.
How did you get the expected class hash, please add a comment on that.

Code quote:

test_contract_class_hash_generation

---

src/core_test.rs line 72 at r3 (raw file):

fn test_contract_class_hash_generation() {
    let data_str = std::fs::read_to_string("./resources/contract_compiled.json").unwrap();
    println!("{data_str}");

Remove?

Code quote:

println!("{data_str}");

---

src/core_test.rs line 73 at r3 (raw file):

    let data_str = std::fs::read_to_string("./resources/contract_compiled.json").unwrap();
    println!("{data_str}");
    let data: serde_json::Value = serde_json::from_str(&data_str).unwrap();

Consider renaming, maybe contract_class?

Code quote:

data

---

src/deprecated_contract_class.rs line 149 at r3 (raw file):

        Value::Number(number) => number
            .as_u64()
            .ok_or_else(|| DeserializationError::custom("Cannot cast number to usize."))?

Why the change?

Code quote:

        Value::Number(number) => number
            .as_u64()
            .ok_or_else(|| DeserializationError::custom("Cannot cast number to usize."))?
            as usize,

---

src/hash.rs line 48 at r3 (raw file):

}

/// Starknet Keccak Hash

Can you please move to a separate PR?
Consider adding a reference to https://docs.starknet.io/documentation/architecture_and_concepts/Hashing/hash-functions/#starknet-keccak.

Code quote:

/// Starknet Keccak Hash

---

src/hash.rs line 49 at r3 (raw file):

/// Starknet Keccak Hash
pub fn sn_keccak(data: &[u8]) -> String {

Consider starknet_keccak.
Please return a StarkFelt.
Please add a test.

Code quote:

sn_keccak

---

src/hash.rs line 52 at r3 (raw file):

    let keccak256 = sha3::Keccak256::digest(data);
    let number = U256::from_big_endian(keccak256.as_slice());
    let mask = U256::pow(U256::from(2), U256::from(250)) - U256::from(1);

Consider a const/static.

Code quote:

let mask = U256::pow(U256::from(2), U256::from(250)) - U256::from(1);

---

src/hash.rs line 66 at r3 (raw file):

#[derive(Clone, Copy, Eq, PartialEq, Default)]
pub struct StarkFeltAsDecimal(U256);

Can you please move to a separate PR?

Code quote:

pub struct StarkFeltAsDecimal(U256);

---

src/serde_utils.rs line 131 at r3 (raw file):

use std::io;

use serde_json::ser::Formatter;

Move to the beginning of file / separate the section with a comment block?

Code quote:

use std::io;

use serde_json::ser::Formatter;

---

src/serde_utils.rs line 134 at r3 (raw file):

pub struct StarknetFormatter;

impl Formatter for StarknetFormatter {

Please document.

Code quote:

impl Formatter for StarknetFormatter {

---

src/serde_utils_test.rs line 114 at r3 (raw file):

#[test]
fn serde_deserialize_big_numbers_without_scientific_notation() {

What is this test for, arbitrary_precision feature? If so, please document.

Code quote:

serde_deserialize_big_numbers_without_scientific_notation

---

src/utils.rs line 1 at r3 (raw file):

use serde_json::Value;

Consider moving serde_remove_elements_from_json test from serde_utils_test to utils_test.

Code quote (from src/serde_utils_test.rs):

serde_remove_elements_from_json

---

src/utils.rs line 60 at r3 (raw file):

/// deserializing to a serde_json::Value changes the order of the keys
/// Go through object's top level keys and remove those that pass the condition
pub fn traverse_and_exclude_top_level_keys<F>(value: &Value, condition: &F) -> serde_json::Value

Remove?

[elintul]

Reviewed 2 of 10 files at r1, all commit messages.
Reviewable status: 9 of 11 files reviewed, 18 unresolved discussions (waiting on @dan-starkware and @marioiordanov)

---

src/core.rs line 10 at r3 (raw file):

use once_cell::sync::Lazy;
use serde::{Deserialize, Serialize};
use serde_json::{json, Serializer, Value};

Consider using fully qualified names; Value is unclear.

Code quote:

use serde_json::{json, Serializer, Value};

---

src/core.rs line 77 at r3 (raw file):

}

fn compute_class_hash_from_json(contract_class: &Value) -> String {

Why not getting a contract class object as input?
Where are you planning to call this function?

Code quote:

contract_class: &Value

---

src/core.rs line 83 at r3 (raw file):

    });

    let program_json = abi_json.get_mut("program").expect("msg");

Informative error message?

Code quote:

"msg"

---

src/core.rs line 88 at r3 (raw file):

        program_json
            .as_object_mut()
            .expect("Not a json object")

In all strings.

Suggestion:

JSON

---

src/core.rs line 107 at r3 (raw file):

        Serializer::with_formatter(&mut writer, crate::serde_utils::StarknetFormatter);
    res.serialize(&mut serializer).expect("Unable to serialize with custom formatter");
    let str_json = unsafe { String::from_utf8_unchecked(writer) };

Is there an alternative for using unsafe code?

Code quote:

unsafe { String::from_utf8_unchecked(writer) }

---

src/core.rs line 108 at r3 (raw file):

    res.serialize(&mut serializer).expect("Unable to serialize with custom formatter");
    let str_json = unsafe { String::from_utf8_unchecked(writer) };
    println!("{}", str_json);

Remove.

Code quote:

println!("{}", str_json);

---

src/core.rs line 111 at r3 (raw file):

    let keccak_result = crate::hash::sn_keccak(str_json.as_bytes());
    keccak_result

Suggestion:

    crate::hash::sn_keccak(str_json.as_bytes())

---

src/utils.rs line 3 at r3 (raw file):

use serde_json::Value;

/// because of the preserve_order feature enabled in the serde_json crate

Please separate to a preliminary PR(s). Same for all JSON utils.
PRs should be self-contained and consist of a single unit of logic (and a test).

[marioiordanov]

Reviewable status: 9 of 11 files reviewed, 18 unresolved discussions (waiting on @dan-starkware and @elintul)

---

src/core.rs line 77 at r3 (raw file):

Previously, elintul (Elin) wrote…

Why not getting a contract class object as input?
Where are you planning to call this function?

because, when creating the json_object that will be hashed, there are keys in the Abi json property that are not part of the struct FunctionAbiEntry - state_mutability. So when hashing the json string created from object of type ContractClass and when hashing it created from json::Value, the result is different.

[marioiordanov]

Reviewable status: 5 of 12 files reviewed, 18 unresolved discussions (waiting on @dan-starkware and @elintul)

---

src/core.rs line 77 at r3 (raw file):

Previously, marioiordanov wrote…

because, when creating the json_object that will be hashed, there are keys in the Abi json property that are not part of the struct FunctionAbiEntry - state_mutability. So when hashing the json string created from object of type ContractClass and when hashing it created from json::Value, the result is different.

Also the order of the keys is essential, they should be in the order it is in the json file. When using the contract class the order of the keys is not guaranteed

---

src/hash.rs line 48 at r3 (raw file):

Previously, dan-starkware wrote…

Can you please move to a separate PR?
Consider adding a reference to https://docs.starknet.io/documentation/architecture_and_concepts/Hashing/hash-functions/#starknet-keccak.

#57

---

src/hash.rs line 49 at r3 (raw file):

Previously, dan-starkware wrote…

Consider starknet_keccak.
Please return a StarkFelt.
Please add a test.

#57

---

src/hash.rs line 52 at r3 (raw file):

Previously, dan-starkware wrote…

Consider a const/static.

#57

---

src/hash.rs line 66 at r3 (raw file):

Previously, dan-starkware wrote…

Can you please move to a separate PR?

#58

[marioiordanov]

Some of the requested changes about extracting to another PR are done, #57 #58

[0xLucqs]

I see a lot of places where it can panic can you refactor it to have a better error handling ? We don't want anything to panic in the sequencer otherwise it's easy to attack the network

[yair-starkware]

src/core.rs line 77 at r3 (raw file):

Previously, marioiordanov wrote…

Also the order of the keys is essential, they should be in the order it is in the json file. When using the contract class the order of the keys is not guaranteed

IMO it is a must to implement the hash computation on the ContractClass struct as a method, and if needed, we should modify the struct and/or the feeder gateway so it will be possible.

[marioiordanov]

I see a lot of places where it can panic can you refactor it to have a better error handling ? We don't want anything to panic in the sequencer otherwise it's easy to attack the network

@LucasLvy Done!

[marioiordanov]

IMO it is a must to implement the hash computation on the ContractClass struct as a method, and if needed, we should modify the struct and/or the feeder gateway so it will be possible.

@yair-starkware It is easy to be implemented, but that means the order of the properties of the struct should be alphabetically ordered, also the order of the inner structs should be as well, because the Cairo-lang compiler produces json file that is alphabetically sorted and If not sorted this way will produce different hash.