starknet_committer: add utility that returns sorted leaf indices#13992
Conversation
b0885bd to
b925279
Compare
PR SummaryLow Risk Overview
Reviewed by Cursor Bugbot for commit 5d766f1. Bugbot is set up for automated code reviews on this repo. Configure here. |
yoavGrs
left a comment
There was a problem hiding this comment.
@yoavGrs reviewed 1 file and all commit messages, and made 3 comments.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on ArielElp).
crates/starknet_committer/src/patricia_merkle_tree/tree.rs line 28 at r1 (raw file):
StarknetForestProofs, };
Restore the new line.
crates/starknet_committer/src/patricia_merkle_tree/tree.rs line 85 at r1 (raw file):
contract_addresses: &[ContractAddress], contract_storage_keys: &HashMap<ContractAddress, Vec<StarknetStorageKey>>, ) -> LeavesRequest {
Move it into impl LeavesRequest?
Suggestion:
pub fn from...(
class_hashes: &[ClassHash],
contract_addresses: &[ContractAddress],
contract_storage_keys: &HashMap<ContractAddress, Vec<StarknetStorageKey>>,
) -> Self {crates/starknet_committer/src/patricia_merkle_tree/tree.rs line 247 at r1 (raw file):
.await?; let (class_sorted, contract_sorted, storage_sorted) = leaves_request.get_sorted();
WDYT?
It’s logically equivalent, but I think this way makes it clearer that the code isn't doing any unnecessary work.
Suggestion:
let prev_proofs = fetch_all_patricia_paths::<FactsNodeLayout>(
storage,
classes_trie_root_hashes.previous_root_hash,
contracts_trie_root_hashes.previous_root_hash,
class_sorted.clone(),
contract_sorted.clone(),
&storage_sorted.clone(),
)
.await?;526b5f0 to
b6e6d57
Compare
b925279 to
13ea978
Compare
ArielElp
left a comment
There was a problem hiding this comment.
@ArielElp reviewed 1 file, made 3 comments, and resolved 2 discussions.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on yoavGrs).
crates/starknet_committer/src/patricia_merkle_tree/tree.rs line 28 at r1 (raw file):
Previously, yoavGrs wrote…
Restore the new line.
Done.
crates/starknet_committer/src/patricia_merkle_tree/tree.rs line 85 at r1 (raw file):
Previously, yoavGrs wrote…
Move it into
impl LeavesRequest?
Done
crates/starknet_committer/src/patricia_merkle_tree/tree.rs line 247 at r1 (raw file):
Previously, yoavGrs wrote…
WDYT?
It’s logically equivalent, but I think this way makes it clearer that the code isn't doing any unnecessary work.
Done (deleted the second unnecessary sorting)
13ea978 to
d58523d
Compare
70039f0 to
c579a4b
Compare
9cc1634 to
56288b6
Compare
dorimedini-starkware
left a comment
There was a problem hiding this comment.
@dorimedini-starkware reviewed 1 file and all commit messages, and made 2 comments.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on ArielElp).
crates/starknet_committer/src/patricia_merkle_tree/tree.rs line 57 at r4 (raw file):
/// per-contract storage leaves). Built via [`LeavesRequest::from_accessed_leaves`]. #[derive(Clone)] pub struct LeavesRequest {
we had an equivalent concept on the py side called StateSelector. consider renaming, for SN OGs to feel at home (non-blocking)
Code quote:
pub struct LeavesRequest {crates/starknet_committer/src/patricia_merkle_tree/tree.rs line 137 at r4 (raw file):
"contract_sorted_leaf_indices is missing an address with requested storage witnesses. \ contract_sorted_leaf_indices: {:?}, storage addresses: {:?}", contract_sorted_leaf_indices,
why the diff here? seems like it didn't have to change
Code quote:
contract_sorted_leaf_indices: {:?}, storage addresses: {:?}",
contract_sorted_leaf_indices,56288b6 to
d8a881d
Compare
c579a4b to
869a81f
Compare
d8a881d to
da84e37
Compare
869a81f to
96c5bcb
Compare
ArielElp
left a comment
There was a problem hiding this comment.
@ArielElp made 2 comments.
Reviewable status: 0 of 1 files reviewed, 2 unresolved discussions (waiting on dorimedini-starkware and yoavGrs).
crates/starknet_committer/src/patricia_merkle_tree/tree.rs line 57 at r4 (raw file):
Previously, dorimedini-starkware wrote…
we had an equivalent concept on the py side called
StateSelector. consider renaming, for SN OGs to feel at home (non-blocking)
I'm not sure if they fill the same role:
class StateSelector(StateSelectorBase):
"""
A class that contains a set of StarkNet contract addresses (sub-commitment tree root IDs)
and a set of hashes of relevant contract classes
affected by one/many transaction(s).
Used for fetching those sub-trees and classes from storage before transaction(s) processing.
"""
In any case, I'm for independence from our pythonic chains
crates/starknet_committer/src/patricia_merkle_tree/tree.rs line 137 at r4 (raw file):
Previously, dorimedini-starkware wrote…
why the diff here? seems like it didn't have to change
It didn't, reverted
dorimedini-starkware
left a comment
There was a problem hiding this comment.
@dorimedini-starkware reviewed 1 file and all commit messages, and resolved 2 discussions.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on ArielElp).
da84e37 to
d962c4f
Compare
96c5bcb to
2e17706
Compare
d962c4f to
b25db53
Compare
300a545 to
c381bc4
Compare
bbe1511 to
823105d
Compare
1b083d2 to
9885436
Compare
823105d to
780c725
Compare
9885436 to
5d766f1
Compare
Merge activity
|

No description provided.