-
Notifications
You must be signed in to change notification settings - Fork 65
starknet_committer,starknet_patricia: add deserialization context #10704
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 deserialization context #10704
Conversation
6d13cf1 to
a185e9a
Compare
9fcfd25 to
5d02a92
Compare
743c49b to
b50df9d
Compare
5d02a92 to
36684fd
Compare
b50df9d to
d41c9d5
Compare
36684fd to
41c08ff
Compare
d41c9d5 to
ae7da9d
Compare
1dc710b to
f515f75
Compare
ae7da9d to
17c75e3
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 of 7 files at r1, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @ArielElp)
crates/starknet_patricia/src/patricia_merkle_tree/filled_tree/node_serde.rs line 66 at r1 (raw file):
pub is_leaf: bool, pub node_hash: HashOutput, }
Name is too general IMO + doc
Suggestion:
/// Extra context required to deserialize [FilledNode].
/// See [DBObject::DeserializeContext] for more information.
pub struct FilledNodeDeserializeContext {
pub is_leaf: bool,
pub node_hash: HashOutput,
}crates/starknet_patricia/src/patricia_merkle_tree/filled_tree/node_serde.rs line 68 at r1 (raw file):
} impl<L: Leaf> DBObject for FilledNode<L> {
Do you plan to have a different type of FilledNode for the index layout?
Code quote:
impl<L: Leaf> DBObject for FilledNode<L> {crates/starknet_patricia/src/patricia_merkle_tree/node_data/leaf.rs line 14 at r1 (raw file):
pub trait Leaf: Clone + Sync + Send + HasStaticPrefix + DBObject<DeserializeContext = ()> + Default + Debug + Eq
Can you give an alias for this and use it here (and possibly other places)?
Does this work?
pub trait ContextLessDBObejct: DBObject<DeserializeContext = ()> {}
Code quote:
DBObject<DeserializeContext = ()>17c75e3 to
31faa1a
Compare
f515f75 to
1e1ccb1
Compare
31faa1a to
539461a
Compare
1e1ccb1 to
134e8a5
Compare
539461a to
ba38be7
Compare
4bf0fbf to
365cf40
Compare
e3d214c to
155bc91
Compare
1782242 to
b12953e
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.
Reviewable status: 5 of 7 files reviewed, 1 unresolved discussion (waiting on @nimrod-starkware and @yoavGrs)
crates/starknet_patricia/src/patricia_merkle_tree/filled_tree/node_serde.rs line 55 at r5 (raw file):
Previously, yoavGrs wrote…
Restore the empty line, please
Done.
crates/starknet_patricia_storage/src/db_object.rs line 46 at r4 (raw file):
Previously, yoavGrs wrote…
It would be done in 2 PRs, but NVM now
Ack
155bc91 to
3018130
Compare
1efd903 to
c231394
Compare
80a31a1 to
e7343be
Compare
c231394 to
38a0f23
Compare
e7343be to
ee3e662
Compare
38a0f23 to
1645dcb
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 2 of 2 files at r6, all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @ArielElp)
1645dcb to
cb4f9c2
Compare
ee3e662 to
12ec8ef
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 2 of 6 files at r2, 2 of 4 files at r4, 1 of 1 files at r5, 1 of 2 files at r6, 1 of 1 files at r7, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @ArielElp)
crates/starknet_patricia/src/patricia_merkle_tree/node_data/leaf.rs line 22 at r7 (raw file):
+ Send + HasStaticPrefix + DBObject<DeserializeContext = EmptyDeserializationContext>
why are leaves always with empty deserialization context? never required?
Code quote:
DBObject<DeserializeContext = EmptyDeserializationContext>crates/starknet_patricia/src/patricia_merkle_tree/filled_tree/node_serde.rs line 118 at r7 (raw file):
return Ok(Self { hash: deserialize_context.node_hash, data: NodeData::Leaf(L::deserialize(value, &EmptyDeserializationContext)?),
isn't this what you want...?
Suggestion:
NodeData::Leaf(L::deserialize(value, &L::DeserializeContext)?)
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.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @dorimedini-starkware)
crates/starknet_patricia/src/patricia_merkle_tree/filled_tree/node_serde.rs line 118 at r7 (raw file):
Previously, dorimedini-starkware wrote…
isn't this what you want...?
I need an instantiation, L::DeserializeContext is the type
crates/starknet_patricia/src/patricia_merkle_tree/node_data/leaf.rs line 22 at r7 (raw file):
Previously, dorimedini-starkware wrote…
why are leaves always with empty deserialization context? never required?
I never use it for leaves. Facts context is is_leaf+hash, index context is is_leaf, for leaves there's no ambiguity on db reads that I need to solve externally.
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)
f259a2a

No description provided.