-
Notifications
You must be signed in to change notification settings - Fork 2
feat: LinkedIterableMap #147
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
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #147 +/- ##
=======================================
Coverage ? 92.65%
=======================================
Files ? 48
Lines ? 2926
Branches ? 0
=======================================
Hits ? 2711
Misses ? 215
Partials ? 0 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
8d9b5e2 to
0e5b9d7
Compare
remollemo
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.
@remollemo reviewed 2 files and all commit messages, and made 2 comments.
Reviewable status: 2 of 4 files reviewed, 2 unresolved discussions (waiting on @RoeeGross).
packages/utils/src/storage/linked_iterable_map.cairo line 30 at r1 (raw file):
next: u64, is_deleted: bool, exists: bool,
why do we need both?
Code quote:
is_deleted: bool,
exists: bool,packages/utils/src/storage/linked_iterable_map.cairo line 92 at r1 (raw file):
tail: u64, length: u32, // Count of non-deleted items total_nodes: u32 // Total nodes in linked list (including deleted)
why we need both?
Code quote:
length: u32, // Count of non-deleted items
total_nodes: u32 // Total nodes in linked list (including deleted)|
Previously, remollemo wrote…
The first is the right length, and the second is for the iteration |
remollemo
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.
@remollemo made 1 comment and resolved 1 discussion.
Reviewable status: 2 of 4 files reviewed, 1 unresolved discussion (waiting on @RoeeGross).
packages/utils/src/storage/linked_iterable_map.cairo line 30 at r1 (raw file):
Previously, remollemo wrote…
why do we need both?
for zeroed key?
remollemo
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.
@remollemo made 6 comments.
Reviewable status: 2 of 4 files reviewed, 6 unresolved discussions (waiting on @RoeeGross).
packages/utils/src/storage/linked_iterable_map.cairo line 143 at r1 (raw file):
} // Generic implementation for any type that can be converted to StoragePath
what's this? have we gone generic Value as well?
packages/utils/src/storage/linked_iterable_map.cairo line 213 at r1 (raw file):
let new_entry = MapEntry { value: value_u128, next: entry.next, is_deleted: false, exists: true, };
don't we have a ... format for struct completion?
Code quote:
let new_entry = MapEntry {
value: value_u128, next: entry.next, is_deleted: false, exists: true,
};packages/utils/src/storage/linked_iterable_map.cairo line 277 at r1 (raw file):
} /// Iterator implementation for linked list traversal
This iterator iterates the items list of the time it was created, right?
anyhow, add a comment explaining the liveliness of the iterator.
packages/utils/src/storage/linked_iterable_map.cairo line 336 at r1 (raw file):
} pub impl LinkedMapIteratorMutImpl of Iterator<LinkedMapIteratorMut> {
any way more elegant to do this other than repeating the non-mut code verbatim?
Code quote:
pub impl LinkedMapIteratorMutImpl of Iterator<LinkedMapIteratorMut> {packages/utils/src/storage/linked_iterable_map.cairo line 422 at r1 (raw file):
let entry = MapEntryStorePacking::unpack(entry_packed); // Clear the entry (set to 0)
0 is the zeroed packed here, right? (i.e. an efficient way to clear all storage bits)
packages/utils/src/storage/linked_iterable_map.cairo line 445 at r1 (raw file):
if !entry.exists || entry.is_deleted { return; }
shouldn't we panic a key_error on a !exist case?
Code quote:
// If doesn't exist or already deleted, do nothing
if !entry.exists || entry.is_deleted {
return;
}
remollemo
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.
@remollemo made 1 comment.
Reviewable status: 2 of 4 files reviewed, 7 unresolved discussions (waiting on @RoeeGross).
packages/utils/src/storage/linked_iterable_map.cairo line 465 at r1 (raw file):
let entry_packed: felt252 = StorageMapReadAccess::read(self._entries, key); let entry = MapEntryStorePacking::unpack(entry_packed); entry.exists && entry.is_deleted
how can there be a entry.exist == false && entry.is_deleted == true?
why do we care here (or anywere) for the entry.exist flag?
Code quote:
entry.exists && entry.is_deleted
remollemo
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.
@remollemo made 4 comments.
Reviewable status: 2 of 4 files reviewed, 10 unresolved discussions (waiting on @RoeeGross).
packages/utils/src/storage/tests/test_linked_iterable_map.cairo line 40 at r1 (raw file):
fn get_all_values(ref self: ContractState) -> Span<(u64, u128)> { let mut array = array![]; for (key, value) in self.linked_map {
neat!!
Code quote:
for (key, value) in self.linked_map {packages/utils/src/storage/tests/test_linked_iterable_map.cairo line 180 at r1 (raw file):
#[test] fn test_single_element() {
i think this test is not needed
packages/utils/src/storage/tests/test_linked_iterable_map.cairo line 222 at r1 (raw file):
dispatcher.set_value(5_u64, 40_u128); assert_eq!(dispatcher.get_value(5_u64), 40_u128.into()); assert_eq!(dispatcher.get_len(), 1);
loop?
Code quote:
// Insert
dispatcher.set_value(5_u64, 10_u128);
assert_eq!(dispatcher.get_value(5_u64), 10_u128.into());
assert_eq!(dispatcher.get_len(), 1);
// Update multiple times - LinkedIterableMap avoids redundant reads!
dispatcher.set_value(5_u64, 20_u128);
assert_eq!(dispatcher.get_value(5_u64), 20_u128.into());
assert_eq!(dispatcher.get_len(), 1);
dispatcher.set_value(5_u64, 30_u128);
assert_eq!(dispatcher.get_value(5_u64), 30_u128.into());
assert_eq!(dispatcher.get_len(), 1);
dispatcher.set_value(5_u64, 40_u128);
assert_eq!(dispatcher.get_value(5_u64), 40_u128.into());
assert_eq!(dispatcher.get_len(), 1);packages/utils/src/storage/tests/test_linked_iterable_map.cairo line 237 at r1 (raw file):
#[test] fn test_insertion_order_preserved() {
what about order in case of insert-delete-reinsert?
Code quote:
test_insertion_order_preserved() {590b42a to
50f9c2d
Compare
|
Previously, remollemo wrote…
On the write action, we need to figure out if we need append or just edit |
|
Previously, remollemo wrote…
If we don't add this key |
50f9c2d to
4aa5593
Compare
RoeeGross
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.
@RoeeGross resolved 7 discussions.
Reviewable status: 1 of 5 files reviewed, 3 unresolved discussions (waiting on @remollemo).
remollemo
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.
@remollemo reviewed 4 files and all commit messages, made 1 comment, and resolved 3 discussions.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @RoeeGross).
Note
Adds a generic, iterable key-value storage
LinkedIterableMap<K, V>with packed on-chain representation and efficient traversal.storage/linked_iterable_map.cairoimplementingread/write,len, iteration (IntoIterator),clear,remove, andis_deletedusing a singly linked list over_entriesand packed metadata_head_tail_lengthlinked_iterable_map/packing.cairoforMapEntryandHeadTailLengthto combine value, next, flags, and head/tail/length/total_nodes into singlefelt252reads/writesLinkedMapIteratorand mutable variant) traversetotal_nodes, skipping deleted entries without extra storage accessstorage.cairo; adds extensive Forge tests validating iteration order, updates, clear/remove semantics, mixed ops, large maps, zero key/value, and multiple type pairs (u64→u128,u32→EthAddress,i64→u128) intests/test_linked_iterable_map.cairoWritten by Cursor Bugbot for commit 4aa5593. This will update automatically on new commits. Configure here.
This change is