starknet_patricia_storage: two layer storage#13990
Conversation
PR SummaryLow Risk Overview Reads consult the overlay first; missing keys are fetched from the base (batch reads only hit the base for overlay misses). The design is aimed at use cases like Unit tests cover base fall-through, overlay shadowing, overlay delete revealing base values, and mixed Reviewed by Cursor Bugbot for commit dc4126c. 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 2 files and all commit messages, and made 4 comments.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on ArielElp).
a discussion (no related file):
Just a reminder, what do you need this for?
crates/starknet_patricia_storage/src/two_layer_storage.rs line 25 at r1 (raw file):
pub overlay: Overlay, pub base: &'a Base, }
Suggestion:
{
overlay: Overlay,
base: &'a Base,
}crates/starknet_patricia_storage/src/two_layer_storage.rs line 69 at r1 (raw file):
} #[cfg(test)]
Move tests to a testing file.
2de219b to
d746081
Compare
ArielElp
left a comment
There was a problem hiding this comment.
@ArielElp reviewed 1 file and made 3 comments.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on yoavGrs).
crates/starknet_patricia_storage/src/two_layer_storage.rs line 69 at r1 (raw file):
Previously, yoavGrs wrote…
Move tests to a testing file.
Done.
crates/starknet_patricia_storage/src/two_layer_storage.rs line 25 at r1 (raw file):
pub overlay: Overlay, pub base: &'a Base, }
Done.
d746081 to
6d22672
Compare
6d471bd to
3f8b451
Compare
yoavGrs
left a comment
There was a problem hiding this comment.
@yoavGrs reviewed all commit messages, made 2 comments, and resolved 2 discussions.
Reviewable status: 1 of 3 files reviewed, 4 unresolved discussions (waiting on ArielElp).
crates/starknet_patricia_storage/src/two_layer_storage.rs line 70 at r2 (raw file):
#[cfg(test)] #[path = "two_layer_storage_test.rs"]
Move it to the top of the file.
yoavGrs
left a comment
There was a problem hiding this comment.
@yoavGrs reviewed 2 files and all commit messages, made 1 comment, and resolved 2 discussions.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on ArielElp).
eb0084f to
7f68595
Compare
d65c881 to
806e180
Compare
806e180 to
b9ca6b2
Compare
7f68595 to
adada4b
Compare
b9ca6b2 to
c7f61b7
Compare
adada4b to
260ae4f
Compare
dorimedini-starkware
left a comment
There was a problem hiding this comment.
@dorimedini-starkware reviewed 3 files and all commit messages, and made 2 comments.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on ArielElp).
crates/starknet_patricia_storage/src/two_layer_storage.rs line 18 at r4 (raw file):
/// [`ReadOnlyStorage::get_mut`] / [`ReadOnlyStorage::mget_mut`] use the same immutable overlay and /// base paths on overlay misses so the composite implements [`ReadOnlyStorage`] while holding /// `&'a Base`. Patricia paths reads do not mutate the underlying storage.
had a stroke trying to read this first sentence, can you rephrase please?
Code quote:
/// [`ReadOnlyStorage::get_mut`] / [`ReadOnlyStorage::mget_mut`] use the same immutable overlay and
/// base paths on overlay misses so the composite implements [`ReadOnlyStorage`] while holding
/// `&'a Base`. Patricia paths reads do not mutate the underlying storage.crates/starknet_patricia_storage/src/two_layer_storage_test.rs line 40 at r4 (raw file):
two.overlay.delete(&key).await.unwrap(); assert_eq!(two.get_mut(&key).await.unwrap(), Some(base_val)); }
non-blocking - just easier to understand what the test is checking like this
Suggestion:
let mut two = TwoLayerStorage::new(MapStorage::default(), &base);
let over_val = DbValue(vec![99]);
two.overlay.set(key.clone(), over_val).await.unwrap();
assert_eq!(two.get_mut(&key).await.unwrap(), Some(over_val));
two.overlay.delete(&key).await.unwrap();
assert_eq!(two.get_mut(&key).await.unwrap(), Some(base_val));
}
yoavGrs
left a comment
There was a problem hiding this comment.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on ArielElp).
c7f61b7 to
adc19fa
Compare
2917668 to
9815587
Compare
adc19fa to
1e02db5
Compare
ArielElp
left a comment
There was a problem hiding this comment.
@ArielElp made 1 comment and resolved 2 discussions.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on ArielElp).
crates/starknet_patricia_storage/src/two_layer_storage.rs line 18 at r4 (raw file):
Previously, dorimedini-starkware wrote…
had a stroke trying to read this first sentence, can you rephrase please?
Done
9815587 to
827da80
Compare
dorimedini-starkware
left a comment
There was a problem hiding this comment.
@dorimedini-starkware reviewed 2 files and all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on ArielElp).
827da80 to
55283c6
Compare
55283c6 to
9d10dee
Compare
9d10dee to
dc4126c
Compare
dorimedini-starkware
left a comment
There was a problem hiding this comment.
@dorimedini-starkware reviewed 1 file and all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on ArielElp).

No description provided.