feat: nullified note retrieval in get_notes and view_notes by nventuro · Pull Request #4208 · AztecProtocol/aztec-packages

nventuro · 2024-01-25T12:44:30Z

This adds an include_nullified flag to the note getter/viewer options which results in the oracle combining both nullified and active notes in its response. We may eventually add a nullified_only option, though that would require performing an inclusion tesf on the nullifier tree, which will be made easier once #4179 is addressed. At this point the flag would turn into an enum, but I decided to go with the simpler approach for now.

The get_note function was not given a way to query nullified notes, mostly to not break the current API as it does not have an options parameter. Ultimately though it is not hard to construct an equivalent call using get_notes by passing a limit value of 1 (which is what get_note does under the hood) and get a single potentially nullified note that way.

A potential issue that might arise with the current API is that the user does not know whether the notes they got are nullified or note, so they won't know if e.g. they need to nullify them after consuming them. However it is expected that use cases that require this option will not care about this distinction.

This PR has conflicts with #3927, as that one performs some refactors on the KV PXE database which change or remove the underlying data structures used to store nullified notes. We're still coordinating with @alexghr on how to address these conflicts, so we shouldn't merge this immediately.

Fixes #3755.

LHerskind

Looks good, have a few nits and think a test showing it read both nullified and active at the same time would be very neat to have 👍

yarn-project/aztec-nr/aztec/src/note/note_getter_options.nr

yarn-project/aztec-nr/aztec/src/note/note_viewer_options.nr

yarn-project/circuit-types/src/notes/note_filter.ts

yarn-project/noir-contracts/contracts/test_contract/src/main.nr

LHerskind · 2024-01-25T15:30:15Z

yarn-project/pxe/src/database/kv_pxe_database.ts

+
+    filter.status = filter.status ?? 'activeOnly';
+
+    if (filter.status == 'activeOnly' || filter.status == 'includeNullified') {


What is the benefit of this if? It includes both cases non-undefined cases and if undefined you will set the value to activeOnly on 187.

What more options did you have for the status? Also, can we use an enum or something instead, seems very easy to miss

I think was alredy considering the scenario in which we'd have three options (active only, include nullified, nullified only). Supporting such a case would simply require adding the new value to the second if (active notes are candidates if querying for active or both, nullified are candidates if querying for both or nullified).

More importantly though, this part is going to be affected by the refactor in #3927 and I wanted to keep the intent clear to help avoid issues when solving the merge conflicts.

LHerskind · 2024-01-25T15:35:58Z

yarn-project/noir-contracts/contracts/test_contract/src/main.nr

+
+        // We can't get the return value of a private function from the outside world in an end to end test, so we
+        // expose it via an unecrypted log instead.
+        let value = opt_notes[0].unwrap().value;


I think it would be interesting here to check more than the first note.

Btw I think you can get the return value by simulate() - should have a return_values or something similar?

simulate resolves to Tx - I couldn't find anything that seemed like it'd be return values there.

yarn-project/end-to-end/src/e2e_get_notes.test.ts

yarn-project/aztec-nr/aztec/src/note/note_getter.nr

rahul-kothari · 2024-01-25T16:40:46Z

yarn-project/noir-contracts/contracts/test_contract/src/main.nr

+
+        // We can't get the return value of a private function from the outside world in an end to end test, so we
+        // expose it via an unecrypted log instead.
+        let value = opt_notes[0].unwrap().value;


rahul-kothari · 2024-01-25T16:41:56Z

yarn-project/noir-contracts/contracts/test_contract/src/main.nr

+
+        // We can't get the return value of a private function from the outside world in an end to end test, so we
+        // expose it via an unecrypted log instead.
+        let value = opt_notes[0].unwrap().value;


Btw I think you can get the return value by simulate() - should have a return_values or something similar?

yarn-project/pxe/src/database/kv_pxe_database.ts

LHerskind

LGTM. The formatting is likely from a different pr or the like

LHerskind · 2024-01-26T15:51:45Z

barretenberg/cpp/src/barretenberg/common/fuzzer.hpp

-                            a.next()
-                            } -> std::convertible_to<uint32_t>;
-                    };
+    {


Is it a different formatter or how come it changes all of these cpp files 🤔 Likely addressed with a rebase

LHerskind · 2024-01-26T15:59:47Z

yarn-project/end-to-end/src/e2e_get_notes.test.ts

+      await assertNoteIsReturned(storageSlot, VALUE, activeOrNullified);
+    }, 30_000);
+
+    it('returns both active and nullified notes', async () => {


🙏 thanks for adding this one for my alzheimers brain

LHerskind · 2024-01-26T16:21:53Z

@nventuro realized that the extra test added is not added to run in ci. If you look at #4236, @sklppy88 is adding his test. Consider doing the same or since the tests are somewhat related, might make sense to put them into the same environment to reduce number of sandboxes needed to be run.

nventuro · 2024-01-26T16:49:09Z

Superceded by #4238, since we cannot run CI here due to this being created from a fork.

Supercedes #4208, which was closed as that one was created from a fork and we therefore cannot run CI there. Fixes #3755.

Supercedes AztecProtocol/aztec-packages#4208, which was closed as that one was created from a fork and we therefore cannot run CI there. Fixes AztecProtocol/aztec-packages#3755.

…ocol#4238) Supercedes AztecProtocol#4208, which was closed as that one was created from a fork and we therefore cannot run CI there. Fixes AztecProtocol#3755.

Supercedes AztecProtocol/aztec-packages#4208, which was closed as that one was created from a fork and we therefore cannot run CI there. Fixes AztecProtocol/aztec-packages#3755.

nventuro added 6 commits January 19, 2024 13:54

Add status to NoteFilter, basic KV db support

d90acf4

Add filtering test cases

185f525

Add filter option, create tests

12e71d2

Make includeNullified mandatory

d73827d

Slight param rename

1a96ef6

Provide methods for the options

f805574

nventuro requested a review from LHerskind January 25, 2024 12:44

nventuro added 2 commits January 25, 2024 09:47

Fix typo

70e81b4

Switch to camel case

fb05a44

LHerskind reviewed Jan 25, 2024

View reviewed changes

nventuro mentioned this pull request Jan 25, 2024

feat(Notes): Allow getNotes to only return nullified notes. #4217

Open

nventuro added 2 commits January 25, 2024 16:06

Link to new issue

62c0256

Formatting pass

8b63fe3

rahul-kothari reviewed Jan 25, 2024

View reviewed changes

nventuro added 9 commits January 25, 2024 18:06

Remove extraneous comment

c241d38

Remove todo comment

853ac0b

More formatting edits

6cc5b0e

Switch to enums

c05ab06

Add test for mixed retrievals

e2e8d33

Merge branch 'master' into get-nullified-notes

9b0e1a3

Increase timeouts

f3378ca

Remove superflous todo

67f6889

Merge branch 'master' into get-nullified-notes

d191c21

nventuro requested a review from LHerskind January 26, 2024 14:11

nventuro mentioned this pull request Jan 26, 2024

feat(PXE): Allow addNote to store nullified notes #4234

Closed

LHerskind approved these changes Jan 26, 2024

View reviewed changes

nventuro mentioned this pull request Jan 26, 2024

feat: nullified note retrieval in get_notes and view_notes #4238

Merged

nventuro closed this Jan 26, 2024

nventuro deleted the get-nullified-notes branch January 26, 2024 16:49

nventuro added a commit that referenced this pull request Jan 30, 2024

feat: nullified note retrieval in get_notes and view_notes (#4238)

8d02eb7

Supercedes #4208, which was closed as that one was created from a fork and we therefore cannot run CI there. Fixes #3755.


		filter.status = filter.status ?? 'activeOnly';

		if (filter.status == 'activeOnly' \|\| filter.status == 'includeNullified') {

Conversation

nventuro commented Jan 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

LHerskind left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

LHerskind left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LHerskind commented Jan 26, 2024

Uh oh!

nventuro commented Jan 26, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

nventuro commented Jan 25, 2024 •

edited

Loading