Skip to content

Conversation

@ArielElp
Copy link
Contributor

No description provided.

@reviewable-StarkWare
Copy link

This change is Reviewable

Copy link
Contributor Author

ArielElp commented Dec 21, 2025

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

Copy link
Contributor

@nimrod-starkware nimrod-starkware left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

@nimrod-starkware reviewed 3 files and all commit messages, and made 1 comment.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @yoavGrs).

Copy link
Contributor

@yoavGrs yoavGrs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@yoavGrs made 2 comments.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @ArielElp).


crates/starknet_committer/src/hash_function/hash.rs line 83 at r1 (raw file):

    let compiled_class_hash: &CompiledClassHash = compiled_class_hash_leaf.as_ref();
    let contract_class_leaf_version: Felt = Felt::from_hex(
        TreeHashFunctionImpl::CONTRACT_CLASS_LEAF_V0,

In the "global" namespace (TreeHashFunctionImpl), there is a constant defined only for compiled class hashes. In the same way, utils for compiled class hashes should be defined in the same place.

In addition, I prefer that the common place won't beTreeHashFunctionImpl. WDYT?

Code quote:

TreeHashFunctionImpl::CONTRACT_CLASS_LEAF_V0

crates/starknet_committer/src/hash_function/hash.rs line 84 at r1 (raw file):

    let contract_class_leaf_version: Felt = Felt::from_hex(
        TreeHashFunctionImpl::CONTRACT_CLASS_LEAF_V0,
    )

It's worth using the const function Felt::from_hex_unchecked for const strings.

Code quote:

    let contract_class_leaf_version: Felt = Felt::from_hex(
        TreeHashFunctionImpl::CONTRACT_CLASS_LEAF_V0,
    )

Copy link
Contributor

@yoavGrs yoavGrs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@yoavGrs reviewed 4 files and all commit messages, and resolved 1 discussion.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @ArielElp).

@ArielElp ArielElp force-pushed the ariel/generic_hash_leaves branch from f345f44 to ee08d41 Compare December 24, 2025 13:16
@ArielElp ArielElp force-pushed the ariel/add_index_layout_leaves branch from 4f38211 to 47793bf Compare December 24, 2025 13:16
Copy link
Collaborator

@dorimedini-starkware dorimedini-starkware left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


crates/starknet_committer/src/hash_function/hash.rs line 18 at r5 (raw file):

// The hex string corresponding to b'CONTRACT_CLASS_LEAF_V0' in big-endian.
pub const CONTRACT_CLASS_LEAF_V0: Felt =
    Felt::from_hex_unchecked("0x434f4e54524143545f434c4153535f4c4541465f5630");

this move of constants should have been a separate PR - broad change (many small diffs in different places), very easy to LGTM quickly, and self-contained

Code quote:

pub const CONTRACT_STATE_HASH_VERSION: Felt = Felt::ZERO;

// The hex string corresponding to b'CONTRACT_CLASS_LEAF_V0' in big-endian.
pub const CONTRACT_CLASS_LEAF_V0: Felt =
    Felt::from_hex_unchecked("0x434f4e54524143545f434c4153535f4c4541465f5630");

crates/starknet_committer/src/patricia_merkle_tree/leaf/leaf_impl.rs line 28 at r5 (raw file):

        self
    }
}

ditto

Code quote:

impl AsRef<ContractState> for ContractState {
    fn as_ref(&self) -> &ContractState {
        self
    }
}

crates/starknet_committer/src/patricia_merkle_tree/types.rs line 29 at r5 (raw file):

    fn as_ref(&self) -> &CompiledClassHash {
        self
    }

also, can't you derive_more::AsRef it?

Suggestion:

    fn as_ref(&self) -> &Self {
        self
    }

@ArielElp ArielElp force-pushed the ariel/add_index_layout_leaves branch from 47793bf to c1448aa Compare December 24, 2025 13:33
@ArielElp ArielElp force-pushed the ariel/generic_hash_leaves branch from ee08d41 to 450bebb Compare December 24, 2025 13:33
@ArielElp ArielElp force-pushed the ariel/add_index_layout_leaves branch from c1448aa to f89d76a Compare December 24, 2025 22:05
@ArielElp ArielElp force-pushed the ariel/generic_hash_leaves branch from 450bebb to 031bb06 Compare December 24, 2025 22:05
Copy link
Contributor Author

@ArielElp ArielElp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ArielElp made 2 comments.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @dorimedini-starkware).


crates/starknet_committer/src/patricia_merkle_tree/types.rs line 29 at r5 (raw file):

Previously, dorimedini-starkware wrote…

also, can't you derive_more::AsRef it?

Nope, derive_more is for trivial wrappers Outer(inner).


crates/starknet_committer/src/patricia_merkle_tree/leaf/leaf_impl.rs line 28 at r5 (raw file):

Previously, dorimedini-starkware wrote…

ditto

Same as above

Copy link
Collaborator

@dorimedini-starkware dorimedini-starkware left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dorimedini-starkware resolved 2 discussions.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @ArielElp).

@ArielElp ArielElp force-pushed the ariel/generic_hash_leaves branch from 031bb06 to 225adf4 Compare December 25, 2025 13:10
@ArielElp ArielElp force-pushed the ariel/add_index_layout_leaves branch from f89d76a to b2de623 Compare December 25, 2025 13:10
@ArielElp ArielElp changed the base branch from ariel/add_index_layout_leaves to graphite-base/10918 December 25, 2025 15:40
@ArielElp ArielElp force-pushed the ariel/generic_hash_leaves branch from 225adf4 to 4a916eb Compare December 25, 2025 15:46
@ArielElp ArielElp force-pushed the graphite-base/10918 branch from b2de623 to 8189934 Compare December 25, 2025 15:46
@ArielElp ArielElp changed the base branch from graphite-base/10918 to ariel/add_index_layout_leaves December 25, 2025 15:47
@ArielElp ArielElp changed the base branch from ariel/add_index_layout_leaves to graphite-base/10918 December 25, 2025 18:34
@ArielElp ArielElp force-pushed the graphite-base/10918 branch from 8189934 to fddb4ee Compare December 25, 2025 18:34
@ArielElp ArielElp force-pushed the ariel/generic_hash_leaves branch from 4a916eb to 793aaec Compare December 25, 2025 18:34
@ArielElp ArielElp changed the base branch from graphite-base/10918 to main-v0.14.1-committer December 25, 2025 18:34
@ArielElp ArielElp force-pushed the ariel/generic_hash_leaves branch from 793aaec to ede901b Compare December 26, 2025 07:09
Copy link
Collaborator

@dorimedini-starkware dorimedini-starkware left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@ArielElp ArielElp added this pull request to the merge queue Dec 28, 2025
Merged via the queue into main-v0.14.1-committer with commit 857730a Dec 28, 2025
37 of 64 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Dec 29, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants