-
Notifications
You must be signed in to change notification settings - Fork 598
feat: add CapsuleArray::for_each #13425
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
Merged
Merged
Changes from all commits
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -76,6 +76,42 @@ impl<T> CapsuleArray<T> { | |
| capsules::store(self.contract_address, self.base_slot, current_length - 1); | ||
| } | ||
|
|
||
| /// Iterates over the entire array, calling the callback with all values and their array index. The order in which | ||
| /// values are processed is arbitrary. | ||
| /// | ||
| /// It is safe to delete the current element (and only the current element) from inside the callback via `remove`: | ||
| /// ```noir | ||
| /// array.for_each(|index, value| { | ||
| /// if some_condition(value) { | ||
| /// array.remove(index); // safe only for this index | ||
| /// } | ||
| /// } | ||
| /// ``` | ||
| /// | ||
| /// If all elements in the array need to iterated over and then removed, then using `for_each` results in optimal | ||
| /// efficiency. | ||
| /// | ||
| /// It is **not** safe to push new elements into the array from inside the callback. | ||
| pub unconstrained fn for_each<Env, let N: u32>(self, f: unconstrained fn[Env](u32, T) -> ()) | ||
| where | ||
| T: Deserialize<N>, | ||
| { | ||
| // Iterating over all elements is simple, but we want to do it in such a way that a) deleting the current | ||
| // element is safe to do, and b) deleting *all* elements is optimally efficient. This is because CapsuleArrays | ||
| // are typically used to hold pending tasks, so iterating them while clearing completed tasks (sometimes | ||
| // unconditionally, resulting in a full clear) is a very common access pattern. | ||
| // | ||
| // The way we achieve this is by iterating backwards: each element can always be deleted since it won't change | ||
| // any preceding (lower) indices, and if every element is deleted then every element will (in turn) be the last | ||
| // element. This results in an optimal full clear since `remove` will be able to skip the `capsules::copy` call | ||
| // to shift any elements past the deleted one (because there will be none). | ||
| let mut i = self.len(); | ||
| while i > 0 { | ||
| i -= 1; | ||
| f(i, self.get(i)); | ||
| } | ||
| } | ||
|
|
||
| unconstrained fn slot_at(self, index: u32) -> Field { | ||
| // Elements are stored immediately after the base slot, so we add 1 to it to compute the slot for the first | ||
| // element. | ||
|
|
@@ -181,4 +217,78 @@ mod test { | |
|
|
||
| assert_eq(array.len(), 0); | ||
| } | ||
|
|
||
| #[test] | ||
| unconstrained fn for_each_called_with_all_elements() { | ||
| let contract_address = setup(); | ||
| let array = CapsuleArray::at(contract_address, SLOT); | ||
|
|
||
| array.push(4); | ||
| array.push(5); | ||
| array.push(6); | ||
|
|
||
| // We store all values that we were called with and check that all (value, index) tuples are present. Note that | ||
| // we do not care about the order in which each tuple was passed to the closure. | ||
| let called_with = &mut BoundedVec::<(u32, Field), 3>::new(); | ||
| array.for_each(|index, value| { called_with.push((index, value)); }); | ||
|
|
||
| assert_eq(called_with.len(), 3); | ||
| assert(called_with.any(|(index, value)| (index == 0) & (value == 4))); | ||
| assert(called_with.any(|(index, value)| (index == 1) & (value == 5))); | ||
| assert(called_with.any(|(index, value)| (index == 2) & (value == 6))); | ||
| } | ||
|
|
||
| #[test] | ||
| unconstrained fn for_each_remove_some() { | ||
| let contract_address = setup(); | ||
| let array = CapsuleArray::at(contract_address, SLOT); | ||
|
|
||
| array.push(4); | ||
| array.push(5); | ||
| array.push(6); | ||
|
|
||
| array.for_each(|index, _| { | ||
| if index == 1 { | ||
| array.remove(index); | ||
| } | ||
| }); | ||
|
|
||
| assert_eq(array.len(), 2); | ||
| assert_eq(array.get(0), 4); | ||
| assert_eq(array.get(1), 6); | ||
| } | ||
|
|
||
| #[test] | ||
| unconstrained fn for_each_remove_all() { | ||
| let contract_address = setup(); | ||
| let array = CapsuleArray::at(contract_address, SLOT); | ||
|
|
||
| array.push(4); | ||
| array.push(5); | ||
| array.push(6); | ||
|
|
||
| array.for_each(|index, _| { array.remove(index); }); | ||
|
|
||
| assert_eq(array.len(), 0); | ||
| } | ||
|
|
||
| // TODO: uncomment this test once OracleMock::count is implemented in the stdlib. | ||
| // #[test] | ||
| // unconstrained fn for_each_remove_all_no_copy() { | ||
| // let contract_address = setup(); | ||
| // let array = CapsuleArray::at(contract_address, SLOT); | ||
|
|
||
| // array.push(4); | ||
| // array.push(5); | ||
| // array.push(6); | ||
|
|
||
| // // We test that the copyCapsule was never called, which is the expensive operation we want to avoid. | ||
| // let mock = OracleMock::mock("copyCapsule"); | ||
|
|
||
| // array.for_each(|index, _| { | ||
| // array.remove(index); | ||
| // }); | ||
|
|
||
| // assert_eq(mock.count(), 0); | ||
| // } | ||
|
Comment on lines
+275
to
+293
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We'll be able uncomment this once noir-lang/noir#7996 goes in and we get a sync PR. |
||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Thank you for the above doc, very clear !