refactor: make get_notes fail if returning no notes #4988 (#5320)

Resolves #4988.
AztecProtocol · Mar 22, 2024 · be86ed3 · be86ed3
1 parent 7086f5d
commit be86ed3
Show file tree

Hide file tree

Showing 9 changed files with 39 additions and 82 deletions.
diff --git a/noir-projects/aztec-nr/aztec/src/note/note_getter.nr b/noir-projects/aztec-nr/aztec/src/note/note_getter.nr
@@ -126,6 +126,9 @@ pub fn get_notes<Note, N, FILTER_ARGS>(
     if options.limit != 0 {
         assert(num_notes <= options.limit, "Invalid number of return notes.");
     }
+
+    assert(num_notes != 0, "Cannot return zero notes");
+
     opt_notes
 }
 

diff --git a/noir-projects/noir-contracts/contracts/pending_note_hashes_contract/src/main.nr b/noir-projects/noir-contracts/contracts/pending_note_hashes_contract/src/main.nr
@@ -110,8 +110,7 @@ contract PendingNoteHashes {
         amount: Field,
         owner: AztecAddress,
         insert_fn_selector: FunctionSelector,
-        get_then_nullify_fn_selector: FunctionSelector,
-        get_note_zero_fn_selector: FunctionSelector
+        get_then_nullify_fn_selector: FunctionSelector
     ) {
         // nested call to create/insert note
         let _callStackItem1 = context.call_private_function(
@@ -125,12 +124,6 @@ contract PendingNoteHashes {
             get_then_nullify_fn_selector,
             [amount, owner.to_field()]
         );
-        // nested call to confirm that balance is zero
-        let _callStackItem3 = context.call_private_function(
-            inputs.call_context.storage_contract_address,
-            get_note_zero_fn_selector,
-            [owner.to_field()]
-        );
     }
 
     // same test as above, but insert 2, get 2, nullify 2
@@ -209,8 +202,7 @@ contract PendingNoteHashes {
         amount: Field,
         owner: AztecAddress,
         insert_fn_selector: FunctionSelector,
-        get_then_nullify_fn_selector: FunctionSelector,
-        get_note_zero_fn_selector: FunctionSelector
+        get_then_nullify_fn_selector: FunctionSelector
     ) {
         // args for nested calls
         let args = [amount, owner.to_field()];
@@ -232,12 +224,6 @@ contract PendingNoteHashes {
             get_then_nullify_fn_selector,
             args
         );
-
-        let _callStackItem4 = context.call_private_function(
-            inputs.call_context.storage_contract_address,
-            get_note_zero_fn_selector,
-            [owner.to_field()]
-        );
     }
     // Confirm cannot get/read a pending note hash in a nested call
     // that is created/inserted later in execution but in the parent.

diff --git a/yarn-project/end-to-end/src/e2e_blacklist_token_contract.test.ts b/yarn-project/end-to-end/src/e2e_blacklist_token_contract.test.ts
@@ -389,7 +389,7 @@ describe('e2e_blacklist_token_contract', () => {
             getMembershipCapsule(await getMembershipProof(accounts[0].address.toBigInt(), true)),
           );
           await expect(asset.methods.redeem_shield(accounts[0].address, amount, secret).simulate()).rejects.toThrow(
-            'Can only remove a note that has been read from the set.',
+            `Assertion failed: Cannot return zero notes`,
           );
         });
 

diff --git a/yarn-project/end-to-end/src/e2e_card_game.test.ts b/yarn-project/end-to-end/src/e2e_card_game.test.ts
@@ -172,7 +172,7 @@ describe('e2e_card_game', () => {
           .join_game(GAME_ID, [cardToField(firstPlayerCollection[0]), cardToField(firstPlayerCollection[1])])
           .send()
           .wait(),
-      ).rejects.toThrow(/Card not found/);
+      ).rejects.toThrow(`Assertion failed: Cannot return zero notes`);
 
       const collection = await contract.methods.view_collection_cards(firstPlayer, 0).view({ from: firstPlayer });
       expect(unwrapOptions(collection)).toHaveLength(1);

diff --git a/yarn-project/end-to-end/src/e2e_note_getter.test.ts b/yarn-project/end-to-end/src/e2e_note_getter.test.ts
@@ -176,7 +176,7 @@ describe('e2e_note_getter', () => {
     async function assertNoReturnValue(storageSlot: number, activeOrNullified: boolean) {
       await expect(contract.methods.call_view_notes(storageSlot, activeOrNullified).view()).rejects.toThrow('is_some');
       await expect(contract.methods.call_get_notes(storageSlot, activeOrNullified).send().wait()).rejects.toThrow(
-        'is_some',
+        `Assertion failed: Cannot return zero notes`,
       );
     }
 

diff --git a/yarn-project/end-to-end/src/e2e_pending_note_hashes_contract.test.ts b/yarn-project/end-to-end/src/e2e_pending_note_hashes_contract.test.ts
@@ -79,10 +79,12 @@ describe('e2e_pending_note_hashes_contract', () => {
         owner,
         deployedContract.methods.insert_note.selector,
         deployedContract.methods.get_then_nullify_note.selector,
-        deployedContract.methods.get_note_zero_balance.selector,
       )
       .send()
       .wait();
+    await expect(deployedContract.methods.get_note_zero_balance(owner).send().wait()).rejects.toThrow(
+      `Assertion failed: Cannot return zero notes`,
+    );
 
     await expectNoteHashesSquashedExcept(0);
     await expectNullifiersSquashedExcept(0);
@@ -154,10 +156,12 @@ describe('e2e_pending_note_hashes_contract', () => {
         owner,
         deployedContract.methods.insert_note.selector,
         deployedContract.methods.get_then_nullify_note.selector,
-        deployedContract.methods.get_note_zero_balance.selector,
       )
       .send()
       .wait();
+    await expect(deployedContract.methods.get_note_zero_balance(owner).send().wait()).rejects.toThrow(
+      `Assertion failed: Cannot return zero notes`,
+    );
 
     // second TX creates 1 note, but it is squashed!
     await expectNoteHashesSquashedExcept(0);
@@ -186,7 +190,6 @@ describe('e2e_pending_note_hashes_contract', () => {
         owner,
         deployedContract.methods.dummy.selector,
         deployedContract.methods.get_then_nullify_note.selector,
-        deployedContract.methods.get_note_zero_balance.selector,
       )
       .send()
       .wait();

diff --git a/yarn-project/end-to-end/src/e2e_static_calls.test.ts b/yarn-project/end-to-end/src/e2e_static_calls.test.ts
@@ -22,6 +22,9 @@ describe('e2e_static_calls', () => {
 
   describe('parent calls child', () => {
     it('performs legal private to private static calls', async () => {
+      // We create a note in the set, so...
+      await childContract.methods.privateSetValue(42n, wallet.getCompleteAddress().address).send().wait();
+      // ...this call doesn't fail due to get_notes returning 0 notes
       await parentContract.methods
         .privateStaticCall(childContract.address, childContract.methods.privateGetValue.selector, [
           42n,
@@ -32,6 +35,9 @@ describe('e2e_static_calls', () => {
     }, 100_000);
 
     it('performs legal (nested) private to private static calls', async () => {
+      // We create a note in the set, so...
+      await childContract.methods.privateSetValue(42n, wallet.getCompleteAddress().address).send().wait();
+      // ...this call doesn't fail due to get_notes returning 0 notes
       await parentContract.methods
         .privateStaticCallNested(childContract.address, childContract.methods.privateGetValue.selector, [
           42n,

diff --git a/yarn-project/end-to-end/src/e2e_token_contract.test.ts b/yarn-project/end-to-end/src/e2e_token_contract.test.ts
@@ -275,7 +275,7 @@ describe('e2e_token_contract', () => {
             'The note has been destroyed.',
           );
           await expect(asset.methods.redeem_shield(accounts[0].address, amount, secret).simulate()).rejects.toThrow(
-            'Can only remove a note that has been read from the set.',
+            `Assertion failed: Cannot return zero notes`,
           );
         });
 

diff --git a/yarn-project/simulator/src/client/private_execution.test.ts b/yarn-project/simulator/src/client/private_execution.test.ts
@@ -918,27 +918,15 @@ describe('Private Execution test suite', () => {
 
       const getThenNullifyArtifact = getFunctionArtifact(PendingNoteHashesContractArtifact, 'get_then_nullify_note');
 
-      const getZeroArtifact = getFunctionArtifact(PendingNoteHashesContractArtifact, 'get_note_zero_balance');
-
       const insertFnSelector = FunctionSelector.fromNameAndParameters(insertArtifact.name, insertArtifact.parameters);
       const getThenNullifyFnSelector = FunctionSelector.fromNameAndParameters(
         getThenNullifyArtifact.name,
         getThenNullifyArtifact.parameters,
       );
-      const getZeroFnSelector = FunctionSelector.fromNameAndParameters(
-        getZeroArtifact.name,
-        getZeroArtifact.parameters,
-      );
 
       oracle.getPortalContractAddress.mockImplementation(() => Promise.resolve(EthAddress.ZERO));
 
-      const args = [
-        amountToTransfer,
-        owner,
-        insertFnSelector.toField(),
-        getThenNullifyFnSelector.toField(),
-        getZeroFnSelector.toField(),
-      ];
+      const args = [amountToTransfer, owner, insertFnSelector.toField(), getThenNullifyFnSelector.toField()];
       const result = await runSimulator({
         args: args,
         artifact: artifact,
@@ -947,7 +935,6 @@ describe('Private Execution test suite', () => {
 
       const execInsert = result.nestedExecutions[0];
       const execGetThenNullify = result.nestedExecutions[1];
-      const getNotesAfterNullify = result.nestedExecutions[2];
 
       const storageSlot = computeSlotForMapping(new Fr(1n), owner);
       const noteTypeId = new Fr(869710811710178111116101n); // ValueNote
@@ -991,10 +978,6 @@ describe('Private Execution test suite', () => {
         siloedNullifierSecretKey.high,
       ]);
       expect(nullifier.value).toEqual(expectedNullifier);
-
-      // check that the last get_notes call return no note
-      const afterNullifyingNoteValue = getNotesAfterNullify.callStackItem.publicInputs.returnValues[0].value;
-      expect(afterNullifyingNoteValue).toEqual(0n);
     });
 
     it('cant read a commitment that is inserted later in same call', async () => {
@@ -1007,48 +990,13 @@ describe('Private Execution test suite', () => {
       const artifact = getFunctionArtifact(PendingNoteHashesContractArtifact, 'test_bad_get_then_insert_flat');
 
       const args = [amountToTransfer, owner];
-      const result = await runSimulator({
-        args: args,
-        artifact: artifact,
-        contractAddress,
-      });
-
-      const storageSlot = computeSlotForMapping(new Fr(1n), owner);
-      const noteTypeId = new Fr(869710811710178111116101n); // ValueNote
-
-      expect(result.newNotes).toHaveLength(1);
-      const noteAndSlot = result.newNotes[0];
-      expect(noteAndSlot.storageSlot).toEqual(storageSlot);
-      expect(noteAndSlot.noteTypeId).toEqual(noteTypeId);
-
-      expect(noteAndSlot.note.items[0]).toEqual(new Fr(amountToTransfer));
-
-      const newNoteHashes = sideEffectArrayToValueArray(
-        nonEmptySideEffects(result.callStackItem.publicInputs.newNoteHashes),
-      );
-      expect(newNoteHashes).toHaveLength(1);
-
-      const noteHash = newNoteHashes[0];
-      expect(noteHash).toEqual(
-        await acirSimulator.computeInnerNoteHash(
+      await expect(
+        runSimulator({
+          args: args,
+          artifact: artifact,
           contractAddress,
-          storageSlot,
-          noteAndSlot.noteTypeId,
-          noteAndSlot.note,
-        ),
-      );
-
-      // read requests should be empty
-      const readRequest = result.callStackItem.publicInputs.noteHashReadRequests[0].value;
-      expect(readRequest).toEqual(Fr.ZERO);
-
-      // should get note value 0 because it actually gets a fake note since the real one hasn't been inserted yet!
-      const gotNoteValue = result.callStackItem.publicInputs.returnValues[0];
-      expect(gotNoteValue).toEqual(Fr.ZERO);
-
-      // there should be no nullifiers
-      const nullifier = result.callStackItem.publicInputs.newNullifiers[0].value;
-      expect(nullifier).toEqual(Fr.ZERO);
+        }),
+      ).rejects.toThrow(`Assertion failed: Cannot return zero notes`);
     });
   });
 
@@ -1069,6 +1017,17 @@ describe('Private Execution test suite', () => {
     });
   });
 
+  describe('Get notes', () => {
+    it('fails if returning no notes', async () => {
+      const artifact = getFunctionArtifact(TestContractArtifact, 'call_get_notes');
+
+      const args = [2n, true];
+      oracle.getNotes.mockResolvedValue([]);
+
+      await expect(runSimulator({ artifact, args })).rejects.toThrow(`Assertion failed: Cannot return zero notes`);
+    });
+  });
+
   describe('Context oracles', () => {
     it("Should be able to get and return the contract's portal contract address", async () => {
       const portalContractAddress = EthAddress.random();