Skip to content

Conversation

@AvivYossef-starkware
Copy link
Contributor

No description provided.

@reviewable-StarkWare
Copy link

This change is Reviewable

@AvivYossef-starkware AvivYossef-starkware marked this pull request as ready for review December 17, 2025 14:59
@AvivYossef-starkware AvivYossef-starkware force-pushed the aviv/from_merkle_node_to_preimage branch from f7f312b to 8a16efd Compare December 18, 2025 13:46
@AvivYossef-starkware AvivYossef-starkware force-pushed the aviv/from_merkle_node_to_preimage branch from 8a16efd to 1401601 Compare December 18, 2025 14:22
@AvivYossef-starkware AvivYossef-starkware force-pushed the aviv/move_commitment_info branch 2 times, most recently from 9263426 to fac9e2d Compare December 18, 2025 14:30
@AvivYossef-starkware AvivYossef-starkware force-pushed the aviv/from_merkle_node_to_preimage branch from 1401601 to e8f7aa8 Compare December 18, 2025 14:30
@AvivYossef-starkware AvivYossef-starkware changed the base branch from aviv/move_commitment_info to graphite-base/10889 December 21, 2025 07:43
Copy link
Collaborator

@noaov1 noaov1 left a comment

Choose a reason for hiding this comment

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

@noaov1 reviewed 5 files and all commit messages, and made 2 comments.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @avi-starkware and @AvivYossef-starkware).


crates/starknet_patricia/src/patricia_merkle_tree/node_data/inner_node_tests.rs line 38 at r1 (raw file):

#[case(Felt::ZERO, Felt::ZERO)]
#[case(Felt::from(0x1234_u128), Felt::from(0xABCD_u128))]
fn test_preimage_from_binary_merkle_node(#[case] left: Felt, #[case] right: Felt) {

What exactly do we test here?

Code quote:

fn test_preimage_from_binary_merkle_node(#[case] left: Felt, #[case] right: Felt) {

crates/starknet_patricia/src/patricia_merkle_tree/node_data/inner_node.rs line 242 at r1 (raw file):

}

impl From<&MerkleNode> for Preimage {

Why is it needed?

Code quote:

impl From<&MerkleNode> for Preimage {

@graphite-app
Copy link

graphite-app bot commented Dec 21, 2025

Merge activity

  • Dec 21, 9:49 AM UTC: This pull request can not be added to the Graphite merge queue. Please try rebasing and resubmitting to merge when ready.
  • Dec 21, 9:49 AM UTC: Graphite disabled "merge when ready" on this PR due to: a merge conflict with the target branch; resolve the conflict and try again..
  • Dec 23, 8:24 AM UTC: Graphite rebased this pull request, because this pull request is set to merge when ready.

Copy link
Contributor Author

@AvivYossef-starkware AvivYossef-starkware left a comment

Choose a reason for hiding this comment

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

@AvivYossef-starkware made 2 comments.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @avi-starkware and @noaov1).


crates/starknet_patricia/src/patricia_merkle_tree/node_data/inner_node.rs line 242 at r1 (raw file):

Previously, noaov1 (Noa Oved) wrote…

Why is it needed?

I use it in the next PR


crates/starknet_patricia/src/patricia_merkle_tree/node_data/inner_node_tests.rs line 38 at r1 (raw file):

Previously, noaov1 (Noa Oved) wrote…

What exactly do we test here?

That the conversion works as expected
It was more of a sanity check for me.
I can delete it if you'd like

@AvivYossef-starkware AvivYossef-starkware force-pushed the aviv/add_executed_class_hashes_to_virtual_block_execution_data branch from 71e966f to 74fa82a Compare December 23, 2025 07:23
@graphite-app graphite-app bot changed the base branch from aviv/add_executed_class_hashes_to_virtual_block_execution_data to graphite-base/10889 December 23, 2025 07:45
@AvivYossef-starkware AvivYossef-starkware force-pushed the aviv/from_merkle_node_to_preimage branch from b71433d to a424d9b Compare December 23, 2025 08:23
@graphite-app graphite-app bot changed the base branch from graphite-base/10889 to main December 23, 2025 08:24
Copy link
Collaborator

@meship-starkware meship-starkware left a comment

Choose a reason for hiding this comment

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

@meship-starkware reviewed 5 files and all commit messages, and made 1 comment.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @avi-starkware, @AvivYossef-starkware, and @noaov1).


crates/starknet_patricia/src/patricia_merkle_tree/node_data/inner_node_tests.rs line 38 at r1 (raw file):

Previously, AvivYossef-starkware wrote…

That the conversion works as expected
It was more of a sanity check for me.
I can delete it if you'd like

I am in favor of deleting them. I don't think they test anything that we won't get from using the function

Copy link
Contributor Author

@AvivYossef-starkware AvivYossef-starkware left a comment

Choose a reason for hiding this comment

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

@AvivYossef-starkware made 1 comment and resolved 1 discussion.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @avi-starkware and @meship-starkware).


crates/starknet_patricia/src/patricia_merkle_tree/node_data/inner_node_tests.rs line 38 at r1 (raw file):

Previously, meship-starkware (Meshi Peled) wrote…

I am in favor of deleting them. I don't think they test anything that we won't get from using the function

Done.

@AvivYossef-starkware AvivYossef-starkware force-pushed the aviv/from_merkle_node_to_preimage branch from a424d9b to add4651 Compare December 23, 2025 08:41
Copy link
Collaborator

@meship-starkware meship-starkware left a comment

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

@meship-starkware meship-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:

@meship-starkware made 1 comment.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @avi-starkware).

@AvivYossef-starkware AvivYossef-starkware added this pull request to the merge queue Dec 23, 2025
Merged via the queue into main with commit cbb6ed9 Dec 23, 2025
27 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Dec 25, 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.

5 participants