-
Notifications
You must be signed in to change notification settings - Fork 65
starknet_committer,starknet_patricia: add layout for base trait #11566
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
starknet_committer,starknet_patricia: add layout for base trait #11566
Conversation
0e438d6 to
45766b2
Compare
efe1bf3 to
8aae7d9
Compare
ArielElp
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ArielElp made 4 comments.
Reviewable status: 5 of 6 files reviewed, 4 unresolved discussions (waiting on @nimrod-starkware and @yoavGrs).
crates/starknet_patricia/src/db_layout.rs line 89 at r2 (raw file):
Previously, yoavGrs wrote…
Why? You can convert to the DB type.
If you only hold NodeLayout, you only know the db type (which is L), the "logical leaf" (or base leaf, however you like to call it) is not part of the NodeLayout trait.
crates/starknet_patricia/src/db_layout.rs line 96 at r2 (raw file):
Previously, yoavGrs wrote…
Is it needed?
Nope
crates/starknet_patricia/src/db_layout.rs line 96 at r2 (raw file):
Previously, yoavGrs wrote…
Consider a more informative name.
DbLeafConvert?
The agent's name is not bad IMO. It's not converting, it's a layout that grounds both base leaf and dbleaf, e.g. it is the layout of ContractState or layout of StarknetStorageValue.
Some other Claude suggestions:
NodeLayoutForBase
NodeLayoutOverLeaf
TrieLayoutFor
Leaving it open for now
crates/starknet_patricia/src/db_layout.rs line 92 at r2 (raw file):
/// /// We require that the BaseLeaf type and its DB representation NodeLayout::DbLeaf are isomorphic. /// The From<BaseLeaf> conversions are needed to convet incoming modifications, and the
Done.
9ee5e92 to
fd65895
Compare
d6d529c to
dd42241
Compare
yoavGrs
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@yoavGrs reviewed 3 files and all commit messages, and resolved 3 discussions.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @nimrod-starkware).
dd42241 to
b2e1b62
Compare
b9eb835 to
bfbb61b
Compare
0a4c103 to
c82e3fc
Compare
bfbb61b to
a453ec3
Compare
nimrod-starkware
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nimrod-starkware made 1 comment.
Reviewable status: 5 of 6 files reviewed, 1 unresolved discussion (waiting on @yoavGrs).
a453ec3 to
d3bbd34
Compare
dorimedini-starkware
left a comment
There was a problem hiding this 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 2 comments.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @yoavGrs).
a discussion (no related file):
much better :)
crates/starknet_patricia/src/db_layout.rs line 96 at r6 (raw file):
/// leaves. For more details see `create_contracts_trie`, `create_classes_trie`, and /// `create_storage_tries` in `starknet_committer`. pub trait NodeLayoutFor<BaseLeaf>: for<'a> NodeLayout<'a, Self::DbLeaf> {
no trait requirements for BaseLeaf?
Code quote:
NodeLayoutFor<BaseLeaf>
ArielElp
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ArielElp made 1 comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @yoavGrs).
crates/starknet_patricia/src/db_layout.rs line 96 at r6 (raw file):
Previously, dorimedini-starkware wrote…
no trait requirements for
BaseLeaf?
No constraints on BaseLeaf here other than conversions with DbLeaf, the Leaf trait is only used from NodeLayout.
ArielElp
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ArielElp resolved 1 discussion.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @ArielElp).
dorimedini-starkware
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status:
complete! all files reviewed, all discussions resolved (waiting on @ArielElp).
10654a9

No description provided.