-
Notifications
You must be signed in to change notification settings - Fork 65
starknet_committer,starknet_patricia: refactor key context #11592
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
base: main-v0.14.1-committer
Are you sure you want to change the base?
Conversation
eb59008 to
f352adb
Compare
4c16417 to
b9eb835
Compare
f352adb to
198e17d
Compare
b9eb835 to
bfbb61b
Compare
198e17d to
7add9e6
Compare
bfbb61b to
a453ec3
Compare
9b35b3d to
a6d209a
Compare
a453ec3 to
d3bbd34
Compare
a6d209a to
921a6ab
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 reviewed 7 files and all commit messages, and made 2 comments.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @ArielElp and @yoavGrs).
crates/starknet_committer/src/db/index_db/leaves.rs line 49 at r7 (raw file):
/// Set to FIRST_AVAILABLE_PREFIX_FELT + 1 static CLASSES_TREE_PREFIX: LazyLock<[u8; 32]> = LazyLock::new(|| (*FIRST_AVAILABLE_PREFIX_FELT + Felt::ONE).to_bytes_be());
WDYT of this? non-blocking
Suggestion:
static CLASSES_TREE_PREFIX: LazyLock<DbKeyPrefix> =
LazyLock::new(|| DbKeyPrefix((*FIRST_AVAILABLE_PREFIX_FELT + Felt::ONE).to_bytes_be().into()));crates/starknet_committer/src/db/index_db/leaves.rs line 75 at r7 (raw file):
DbKeyPrefix::new(prefix.into()) } }
Suggestion:
impl HasStaticPrefix for IndexLayoutStarknetStorageValue {
// For the storage tries, the prefix is the contract's address.
type KeyContext = ContractAddress;
fn get_static_prefix(key_context: &Self::KeyContext) -> DbKeyPrefix {
let prefix = key_context.to_bytes_be().to_vec();
DbKeyPrefix::new(prefix.into())
}
}d3bbd34 to
dab4f4b
Compare
921a6ab to
3cfa249
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 2 comments.
Reviewable status: 16 of 17 files reviewed, 6 unresolved discussions (waiting on @nimrod-starkware and @yoavGrs).
crates/starknet_committer/src/db/index_db/leaves.rs line 49 at r7 (raw file):
Previously, nimrod-starkware wrote…
WDYT of this? non-blocking
Can't if I want the borrowed variant of Cow, if the lock is [u8;32] I can send &value to db prefix, but I can't do &prefix.to_bytes_be() in the closure body because it's a temporary value.
crates/starknet_committer/src/db/index_db/leaves.rs line 75 at r7 (raw file):
DbKeyPrefix::new(prefix.into()) } }
Added it as a doc for the method, since KeyContext is also ContractAddress for facts layout where the prefix is something else.
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 and resolved 2 discussions.
Reviewable status: 16 of 17 files reviewed, 4 unresolved discussions (waiting on @ArielElp and @yoavGrs).
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 2 files and all commit messages, and resolved 3 discussions.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @ArielElp and @dorimedini-starkware).
3cfa249 to
a3bd7c3
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 17 files and all commit messages, and made 1 comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @ArielElp).
crates/starknet_committer/src/db/index_db/leaves.rs line 75 at r8 (raw file):
fn get_static_prefix(key_context: &Self::KeyContext) -> DbKeyPrefix { let prefix = key_context.to_bytes_be().to_vec(); DbKeyPrefix::new(prefix.into())
not for this PR, but, seems like the point of key_context is to define a prefix; so prefixifying should be a method, right?
it's just that key_context.to_bytes_be() seems a bit strange
Suggestion:
DbKeyPrefix::new(key_context.to_prefix_bytes())
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).

No description provided.