Skip to content

starknet_patricia_storage: two layer storage#13990

Merged
ArielElp merged 1 commit into
mainfrom
ariel/add_two_layer_storage
May 27, 2026
Merged

starknet_patricia_storage: two layer storage#13990
ArielElp merged 1 commit into
mainfrom
ariel/add_two_layer_storage

Conversation

@ArielElp
Copy link
Copy Markdown
Contributor

@ArielElp ArielElp commented May 7, 2026

No description provided.

@reviewable-StarkWare
Copy link
Copy Markdown

This change is Reviewable

Copy link
Copy Markdown
Contributor Author

ArielElp commented May 7, 2026

@cursor
Copy link
Copy Markdown

cursor Bot commented May 7, 2026

PR Summary

Low Risk
New isolated module with tests only; no existing call sites or storage backends are modified in this diff.

Overview
Adds a two-layer read-only storage composite so callers can stack a writable overlay on a borrowed immutable base and still satisfy ReadOnlyStorage via get_mut / mget_mut.

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 fetch_all_patricia_paths, which need &mut ReadOnlyStorage without owning the underlying DB.

Unit tests cover base fall-through, overlay shadowing, overlay delete revealing base values, and mixed mget_mut behavior.

Reviewed by Cursor Bugbot for commit dc4126c. Bugbot is set up for automated code reviews on this repo. Configure here.

Comment thread crates/starknet_patricia_storage/src/two_layer_storage.rs Outdated
Copy link
Copy Markdown
Contributor

@yoavGrs yoavGrs left a 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 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.

Comment thread crates/starknet_patricia_storage/src/two_layer_storage.rs Outdated
@ArielElp ArielElp force-pushed the ariel/add_two_layer_storage branch from 2de219b to d746081 Compare May 14, 2026 08:43
@ArielElp ArielElp changed the base branch from ariel/move_tests_module to graphite-base/13990 May 14, 2026 09:03
Copy link
Copy Markdown
Contributor Author

@ArielElp ArielElp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Comment thread crates/starknet_patricia_storage/src/two_layer_storage.rs Outdated
@ArielElp ArielElp force-pushed the ariel/add_two_layer_storage branch from d746081 to 6d22672 Compare May 14, 2026 11:56
@ArielElp ArielElp force-pushed the graphite-base/13990 branch from 6d471bd to 3f8b451 Compare May 14, 2026 11:56
@ArielElp ArielElp changed the base branch from graphite-base/13990 to ariel/move_tests_module May 14, 2026 11:56
Comment thread crates/starknet_patricia_storage/src/two_layer_storage_test.rs
Copy link
Copy Markdown
Contributor

@yoavGrs yoavGrs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Comment thread crates/starknet_patricia_storage/src/two_layer_storage.rs Outdated
Copy link
Copy Markdown
Contributor

@yoavGrs yoavGrs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

@yoavGrs reviewed 2 files and all commit messages, made 1 comment, and resolved 2 discussions.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on ArielElp).

@ArielElp ArielElp force-pushed the ariel/add_two_layer_storage branch from eb0084f to 7f68595 Compare May 19, 2026 12:37
@ArielElp ArielElp force-pushed the ariel/move_tests_module branch from d65c881 to 806e180 Compare May 19, 2026 12:37
@ArielElp ArielElp force-pushed the ariel/move_tests_module branch from 806e180 to b9ca6b2 Compare May 20, 2026 09:03
@ArielElp ArielElp force-pushed the ariel/add_two_layer_storage branch from 7f68595 to adada4b Compare May 20, 2026 09:03
@ArielElp ArielElp force-pushed the ariel/move_tests_module branch from b9ca6b2 to c7f61b7 Compare May 20, 2026 09:25
@ArielElp ArielElp force-pushed the ariel/add_two_layer_storage branch from adada4b to 260ae4f Compare May 20, 2026 09:25
Copy link
Copy Markdown
Collaborator

@dorimedini-starkware dorimedini-starkware left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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));
}

Copy link
Copy Markdown
Contributor

@yoavGrs yoavGrs left a 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 ArielElp).

@ArielElp ArielElp force-pushed the ariel/move_tests_module branch from c7f61b7 to adc19fa Compare May 25, 2026 13:32
@ArielElp ArielElp force-pushed the ariel/add_two_layer_storage branch 2 times, most recently from 2917668 to 9815587 Compare May 25, 2026 13:45
@ArielElp ArielElp force-pushed the ariel/move_tests_module branch from adc19fa to 1e02db5 Compare May 25, 2026 13:45
Copy link
Copy Markdown
Contributor Author

@ArielElp ArielElp left a 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 and resolved 2 discussions.
Reviewable status: :shipit: 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

@ArielElp ArielElp force-pushed the ariel/add_two_layer_storage branch from 9815587 to 827da80 Compare May 25, 2026 13:58
Copy link
Copy Markdown
Collaborator

@dorimedini-starkware dorimedini-starkware left a 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 files and all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on ArielElp).

@graphite-app graphite-app Bot changed the base branch from ariel/move_tests_module to graphite-base/13990 May 26, 2026 08:36
@ArielElp ArielElp force-pushed the ariel/add_two_layer_storage branch from 827da80 to 55283c6 Compare May 26, 2026 10:08
@ArielElp ArielElp changed the base branch from graphite-base/13990 to ariel/move_tests_module May 26, 2026 10:09
@ArielElp ArielElp changed the base branch from ariel/move_tests_module to main May 26, 2026 12:55
@ArielElp ArielElp force-pushed the ariel/add_two_layer_storage branch from 55283c6 to 9d10dee Compare May 26, 2026 13:03
@ArielElp ArielElp force-pushed the ariel/add_two_layer_storage branch from 9d10dee to dc4126c Compare May 26, 2026 13:11
Copy link
Copy Markdown
Collaborator

@dorimedini-starkware dorimedini-starkware left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dorimedini-starkware reviewed 1 file and all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on ArielElp).

@ArielElp ArielElp added this pull request to the merge queue May 27, 2026
Merged via the queue into main with commit f667741 May 27, 2026
21 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants