From be86ed3a6a2fa1c35ec7613da1a18bbd2327b18e Mon Sep 17 00:00:00 2001 From: esau <152162806+sklppy88@users.noreply.github.com> Date: Fri, 22 Mar 2024 09:07:50 +0100 Subject: [PATCH] refactor: make get_notes fail if returning no notes #4988 (#5320) Resolves #4988. --- .../aztec-nr/aztec/src/note/note_getter.nr | 3 + .../pending_note_hashes_contract/src/main.nr | 18 +---- .../src/e2e_blacklist_token_contract.test.ts | 2 +- .../end-to-end/src/e2e_card_game.test.ts | 2 +- .../end-to-end/src/e2e_note_getter.test.ts | 2 +- .../e2e_pending_note_hashes_contract.test.ts | 9 ++- .../end-to-end/src/e2e_static_calls.test.ts | 6 ++ .../end-to-end/src/e2e_token_contract.test.ts | 2 +- .../src/client/private_execution.test.ts | 77 +++++-------------- 9 files changed, 39 insertions(+), 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 index 679f097ee65..a2f1b30f6bc 100644 --- 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( 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 index a46df1a92db..7d3fcd593b8 100644 --- 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 index 8d6e232fbb1..efb35e65902 100644 --- 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 index 6e26b459fad..1520f91c92a 100644 --- 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 index 6641b391149..402b50f5b05 100644 --- 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 index 37e1cbdefdb..1e30daec290 100644 --- 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 index 974d3ce3f45..6461ce22a6f 100644 --- 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 index a64490a6571..f7e60748f2e 100644 --- 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 index 2abe169627f..32b6d1f8b86 100644 --- 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();