From e16452ab7640169e1235b91d598e3d90ba4ce57e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nicol=C3=A1s=20Venturo?= Date: Mon, 29 Jul 2024 15:38:33 -0300 Subject: [PATCH] feat: allow get_notes to return zero notes (#7621) `get_notes` used to fail if no notes were found ever since https://github.com/AztecProtocol/aztec-packages/issues/4988 was merged. The argument there was that since no constraints are applied, and note non-existence cannot be proved, `get_notes` returning no notes is always a mistake. However, what that mistake means depends on the caller. Since we don't want for the caller to have to pass an error message to `get_notes`, I think it's better to return no notes and leave it up to the caller to decide what to do. `BoundedVec` is a safe container: `get` will result in errors if used with indices out of bounds. I changed the callsites where we did `get_unchecked(0)` to `get(0)` - this means that the errors now change from 'got 0 notes' to 'out of bounds access'. Neither error message is great, so I didn't bother improving this. Maybe we could have a `get` variant that received an error message (like `Option::expect`), but we don't need to worry about this for now. Closes https://github.com/AztecProtocol/aztec-packages/issues/7059 (indirectly due to how we need to change certain tests to address this) --- .../aztec-nr/aztec/src/note/note_getter/mod.nr | 1 - .../aztec-nr/aztec/src/note/note_getter/test.nr | 6 +++--- .../contracts/benchmarking_contract/src/main.nr | 2 +- .../contracts/child_contract/src/main.nr | 2 +- .../contracts/delegated_on_contract/src/main.nr | 2 +- .../inclusion_proofs_contract/src/main.nr | 4 ++-- .../pending_note_hashes_contract/src/main.nr | 8 +------- .../contracts/static_child_contract/src/main.nr | 2 +- .../contracts/test_contract/src/main.nr | 2 +- .../token_blacklist_contract/src/main.nr | 2 +- .../contracts/token_contract/src/main.nr | 2 +- .../contracts/token_contract/src/test/minting.nr | 2 +- .../e2e_blacklist_token_contract/minting.test.ts | 2 +- yarn-project/end-to-end/src/e2e_card_game.test.ts | 2 +- .../src/e2e_delegate_calls/delegate.test.ts | 2 +- .../end-to-end/src/e2e_note_getter.test.ts | 2 +- .../src/e2e_pending_note_hashes_contract.test.ts | 9 +++------ .../src/e2e_token_contract/minting.test.ts | 2 +- .../src/client/private_execution.test.ts | 15 +++++++-------- 19 files changed, 29 insertions(+), 40 deletions(-) diff --git a/noir-projects/aztec-nr/aztec/src/note/note_getter/mod.nr b/noir-projects/aztec-nr/aztec/src/note/note_getter/mod.nr index b1176829c76..22c9857230d 100644 --- a/noir-projects/aztec-nr/aztec/src/note/note_getter/mod.nr +++ b/noir-projects/aztec-nr/aztec/src/note/note_getter/mod.nr @@ -133,7 +133,6 @@ fn constrain_get_notes_internal( // results in reduced gate counts when setting a limit value, since we guarantee that the limit is an upper bound // for the runtime length, and can therefore have fewer loop iterations. assert(notes.len() <= options.limit, "Got more notes than limit."); - assert(notes.len() != 0, "Cannot return zero notes"); let mut prev_fields = [0; N]; for i in 0..options.limit { diff --git a/noir-projects/aztec-nr/aztec/src/note/note_getter/test.nr b/noir-projects/aztec-nr/aztec/src/note/note_getter/test.nr index 2be975ac51c..52ba87c15c1 100644 --- a/noir-projects/aztec-nr/aztec/src/note/note_getter/test.nr +++ b/noir-projects/aztec-nr/aztec/src/note/note_getter/test.nr @@ -96,15 +96,15 @@ fn collapses_notes_at_the_beginning_of_the_array() { assert_equivalent_vec_and_array(returned, expected); } -#[test(should_fail_with="Cannot return zero notes")] -fn rejects_zero_notes() { +fn can_return_zero_notes() { let mut env = setup(); let mut context = env.private(); let opt_notes: [Option; MAX_NOTE_HASH_READ_REQUESTS_PER_CALL] = [Option::none(); MAX_NOTE_HASH_READ_REQUESTS_PER_CALL]; let options = NoteGetterOptions::new(); - let _ = constrain_get_notes_internal(&mut context, storage_slot, opt_notes, options); + let returned = constrain_get_notes_internal(&mut context, storage_slot, opt_notes, options); + assert_eq(returned.len(), 0); } #[test(should_fail_with="Got more notes than limit.")] diff --git a/noir-projects/noir-contracts/contracts/benchmarking_contract/src/main.nr b/noir-projects/noir-contracts/contracts/benchmarking_contract/src/main.nr index 7e2dde51083..50f2fa8bb0c 100644 --- a/noir-projects/noir-contracts/contracts/benchmarking_contract/src/main.nr +++ b/noir-projects/noir-contracts/contracts/benchmarking_contract/src/main.nr @@ -31,7 +31,7 @@ contract Benchmarking { let owner_notes = storage.notes.at(owner); let mut getter_options = NoteGetterOptions::new(); let notes = owner_notes.get_notes(getter_options.set_limit(1).set_offset(index)); - let note = notes.get_unchecked(0); + let note = notes.get(0); owner_notes.remove(note); increment(owner_notes, note.value, owner, outgoing_viewer); } diff --git a/noir-projects/noir-contracts/contracts/child_contract/src/main.nr b/noir-projects/noir-contracts/contracts/child_contract/src/main.nr index 4464dce3ea0..8599903cfd8 100644 --- a/noir-projects/noir-contracts/contracts/child_contract/src/main.nr +++ b/noir-projects/noir-contracts/contracts/child_contract/src/main.nr @@ -64,7 +64,7 @@ contract Child { let mut options = NoteGetterOptions::new(); options = options.select(ValueNote::properties().value, amount, Option::none()).set_limit(1); let notes = storage.a_map_with_private_values.at(owner).get_notes(options); - notes.get_unchecked(0).value + notes.get(0).value } // Increments `current_value` by `new_value` diff --git a/noir-projects/noir-contracts/contracts/delegated_on_contract/src/main.nr b/noir-projects/noir-contracts/contracts/delegated_on_contract/src/main.nr index 772d3021827..ca0dd94005c 100644 --- a/noir-projects/noir-contracts/contracts/delegated_on_contract/src/main.nr +++ b/noir-projects/noir-contracts/contracts/delegated_on_contract/src/main.nr @@ -34,7 +34,7 @@ contract DelegatedOn { let mut options = NoteGetterOptions::new(); options = options.select(ValueNote::properties().value, amount, Option::none()).set_limit(1); let notes = storage.a_map_with_private_values.at(owner).get_notes(options); - notes.get_unchecked(0).value + notes.get(0).value } unconstrained fn view_public_value() -> pub Field { diff --git a/noir-projects/noir-contracts/contracts/inclusion_proofs_contract/src/main.nr b/noir-projects/noir-contracts/contracts/inclusion_proofs_contract/src/main.nr index 2b370e1ec35..13d762ee531 100644 --- a/noir-projects/noir-contracts/contracts/inclusion_proofs_contract/src/main.nr +++ b/noir-projects/noir-contracts/contracts/inclusion_proofs_contract/src/main.nr @@ -55,7 +55,7 @@ contract InclusionProofs { if (nullified) { options = options.set_status(NoteStatus.ACTIVE_OR_NULLIFIED); } - let note = private_values.get_notes(options).get_unchecked(0); + let note = private_values.get_notes(options).get(0); // docs:end:get_note_from_pxe // 2) Prove the note inclusion @@ -106,7 +106,7 @@ contract InclusionProofs { if (fail_case) { options = options.set_status(NoteStatus.ACTIVE_OR_NULLIFIED); } - let note = private_values.get_notes(options).get_unchecked(0); + let note = private_values.get_notes(options).get(0); let header = if (use_block_number) { context.get_header_at(block_number) 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 caf4fd0ec9f..8c11b8f6986 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 @@ -59,10 +59,6 @@ contract PendingNoteHashes { // get note (note inserted at bottom of function shouldn't exist yet) let notes = owner_balance.get_notes(options); - // TODO https://github.com/AztecProtocol/aztec-packages/issues/7059: clean this test up - we won't actually hit - // this assert because the same one exists in get_notes (we always return at least one note), so all lines after - // this one are effectively dead code. We should find a different way to test what this function attempts to - // test. assert(notes.len() == 0); let header = context.get_header(); @@ -70,7 +66,7 @@ contract PendingNoteHashes { // Insert note let mut note = ValueNote::new(amount, owner_npk_m_hash); - owner_balance.insert(&mut note).emit(encode_and_encrypt_note(&mut context, context.msg_sender(), owner)); + owner_balance.insert(&mut note).discard(); 0 } @@ -158,8 +154,6 @@ contract PendingNoteHashes { let options = NoteGetterOptions::new(); let notes = owner_balance.get_notes(options); - // TODO https://github.com/AztecProtocol/aztec-packages/issues/7059: review what this test aimed to test, as - // it's now superfluous: get_notes never returns 0 notes. assert(notes.len() == 0); } diff --git a/noir-projects/noir-contracts/contracts/static_child_contract/src/main.nr b/noir-projects/noir-contracts/contracts/static_child_contract/src/main.nr index e0d113569fe..1a53f4b6be1 100644 --- a/noir-projects/noir-contracts/contracts/static_child_contract/src/main.nr +++ b/noir-projects/noir-contracts/contracts/static_child_contract/src/main.nr @@ -73,7 +73,7 @@ contract StaticChild { Option::none() ).set_limit(1); let notes = storage.a_private_value.get_notes(options); - notes.get_unchecked(0).value + notes.get(0).value } // Increments `current_value` by `new_value` diff --git a/noir-projects/noir-contracts/contracts/test_contract/src/main.nr b/noir-projects/noir-contracts/contracts/test_contract/src/main.nr index e58e9ca0ba4..3cf7eb92a08 100644 --- a/noir-projects/noir-contracts/contracts/test_contract/src/main.nr +++ b/noir-projects/noir-contracts/contracts/test_contract/src/main.nr @@ -475,7 +475,7 @@ contract Test { let mut options = NoteGetterOptions::new(); options = options.select(TestNote::properties().value, secret_hash, Option::none()).set_limit(1); let notes = notes_set.get_notes(options); - let note = notes.get_unchecked(0); + let note = notes.get(0); notes_set.remove(note); } diff --git a/noir-projects/noir-contracts/contracts/token_blacklist_contract/src/main.nr b/noir-projects/noir-contracts/contracts/token_blacklist_contract/src/main.nr index 114fa764515..73685be5bb9 100644 --- a/noir-projects/noir-contracts/contracts/token_blacklist_contract/src/main.nr +++ b/noir-projects/noir-contracts/contracts/token_blacklist_contract/src/main.nr @@ -173,7 +173,7 @@ contract TokenBlacklist { Option::none() ).set_limit(1); let notes = pending_shields.get_notes(options); - let note = notes.get_unchecked(0); + let note = notes.get(0); // Remove the note from the pending shields set pending_shields.remove(note); diff --git a/noir-projects/noir-contracts/contracts/token_contract/src/main.nr b/noir-projects/noir-contracts/contracts/token_contract/src/main.nr index 98dabaaa0ce..e12673267f4 100644 --- a/noir-projects/noir-contracts/contracts/token_contract/src/main.nr +++ b/noir-projects/noir-contracts/contracts/token_contract/src/main.nr @@ -295,7 +295,7 @@ contract Token { Option::none() ).set_limit(1); let notes = pending_shields.get_notes(options); - let note = notes.get_unchecked(0); + let note = notes.get(0); // Remove the note from the pending shields set pending_shields.remove(note); diff --git a/noir-projects/noir-contracts/contracts/token_contract/src/test/minting.nr b/noir-projects/noir-contracts/contracts/token_contract/src/test/minting.nr index 3888722febd..45a60898e62 100644 --- a/noir-projects/noir-contracts/contracts/token_contract/src/test/minting.nr +++ b/noir-projects/noir-contracts/contracts/token_contract/src/test/minting.nr @@ -88,7 +88,7 @@ unconstrained fn mint_private_success() { utils::check_private_balance(token_contract_address, owner, mint_amount); } -#[test(should_fail_with="Cannot return zero notes")] +#[test(should_fail_with="Attempted to read past end of BoundedVec")] unconstrained fn mint_private_failure_double_spend() { // Setup without account contracts. We are not using authwits here, so dummy accounts are enough let (env, token_contract_address, owner, recipient) = utils::setup(/* with_account_contracts */ false); diff --git a/yarn-project/end-to-end/src/e2e_blacklist_token_contract/minting.test.ts b/yarn-project/end-to-end/src/e2e_blacklist_token_contract/minting.test.ts index 6c7d700364c..37016cdbd20 100644 --- a/yarn-project/end-to-end/src/e2e_blacklist_token_contract/minting.test.ts +++ b/yarn-project/end-to-end/src/e2e_blacklist_token_contract/minting.test.ts @@ -111,7 +111,7 @@ describe('e2e_blacklist_token_contract mint', () => { 'The note has been destroyed.', ); await expect(asset.methods.redeem_shield(wallets[0].getAddress(), amount, secret).prove()).rejects.toThrow( - `Assertion failed: Cannot return zero notes`, + `Assertion failed: Attempted to read past end of BoundedVec`, ); }); 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 7f7f3e6fa98..895ba14085c 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 @@ -183,7 +183,7 @@ describe('e2e_card_game', () => { .join_game(GAME_ID, [cardToField(firstPlayerCollection[0]), cardToField(firstPlayerCollection[1])]) .send() .wait(), - ).rejects.toThrow(`Assertion failed: Cannot return zero notes`); + ).rejects.toThrow(`Card not found`); const collection = await contract.methods.view_collection_cards(firstPlayer, 0).simulate({ from: firstPlayer }); expect(boundedVecToArray(collection)).toHaveLength(1); diff --git a/yarn-project/end-to-end/src/e2e_delegate_calls/delegate.test.ts b/yarn-project/end-to-end/src/e2e_delegate_calls/delegate.test.ts index 4fa956862cb..3e7d77aeff8 100644 --- a/yarn-project/end-to-end/src/e2e_delegate_calls/delegate.test.ts +++ b/yarn-project/end-to-end/src/e2e_delegate_calls/delegate.test.ts @@ -30,7 +30,7 @@ describe.skip('e2e_delegate_calls', () => { await expect( delegatedOnContract.methods.get_private_value(sentValue, wallet.getCompleteAddress().address).simulate(), - ).rejects.toThrow(`Assertion failed: Cannot return zero notes 'num_notes != 0'`); + ).rejects.toThrow(`Assertion failed: Attempted to read past end of BoundedVec 'num_notes != 0'`); expect(delegatorValue).toEqual(sentValue); }); 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 be87db5bd48..bb81e137a06 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 @@ -182,7 +182,7 @@ describe('e2e_note_getter', () => { 'index < self.len', // from BoundedVec::get ); await expect(contract.methods.call_get_notes(storageSlot, activeOrNullified).prove()).rejects.toThrow( - `Assertion failed: Cannot return zero notes`, + `Assertion failed: Attempted to read past end of BoundedVec`, ); } 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 6e24dddfd32..ba938b63ef2 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 @@ -108,9 +108,7 @@ describe('e2e_pending_note_hashes_contract', () => { ) .send() .wait(); - await expect(deployedContract.methods.get_note_zero_balance(owner).prove()).rejects.toThrow( - `Assertion failed: Cannot return zero notes`, - ); + await deployedContract.methods.get_note_zero_balance(owner).send().wait(); await expectNoteHashesSquashedExcept(0); await expectNullifiersSquashedExcept(0); @@ -244,9 +242,8 @@ describe('e2e_pending_note_hashes_contract', () => { ) .send() .wait(); - await expect(deployedContract.methods.get_note_zero_balance(owner).prove()).rejects.toThrow( - `Assertion failed: Cannot return zero notes`, - ); + + await deployedContract.methods.get_note_zero_balance(owner).send().wait(); // second TX creates 1 note, but it is squashed! await expectNoteHashesSquashedExcept(0); diff --git a/yarn-project/end-to-end/src/e2e_token_contract/minting.test.ts b/yarn-project/end-to-end/src/e2e_token_contract/minting.test.ts index 88141ecdf85..83d3ea02ce6 100644 --- a/yarn-project/end-to-end/src/e2e_token_contract/minting.test.ts +++ b/yarn-project/end-to-end/src/e2e_token_contract/minting.test.ts @@ -101,7 +101,7 @@ describe('e2e_token_contract minting', () => { 'The note has been destroyed.', ); await expect(asset.methods.redeem_shield(accounts[0].address, amount, secret).simulate()).rejects.toThrow( - `Assertion failed: Cannot return zero notes`, + `Assertion failed: Attempted to read past end of BoundedVec`, ); }); diff --git a/yarn-project/simulator/src/client/private_execution.test.ts b/yarn-project/simulator/src/client/private_execution.test.ts index e3047d7b861..d2384e803c9 100644 --- a/yarn-project/simulator/src/client/private_execution.test.ts +++ b/yarn-project/simulator/src/client/private_execution.test.ts @@ -1092,13 +1092,12 @@ describe('Private Execution test suite', () => { const artifact = getFunctionArtifact(PendingNoteHashesContractArtifact, 'test_bad_get_then_insert_flat'); const args = [amountToTransfer, owner]; - await expect(() => - runSimulator({ - args: args, - artifact: artifact, - contractAddress, - }), - ).rejects.toThrow(`Assertion failed: Cannot return zero notes`); + // This will throw if we read the note before it was inserted + await runSimulator({ + args: args, + artifact: artifact, + contractAddress, + }); }); }); @@ -1126,7 +1125,7 @@ describe('Private Execution test suite', () => { oracle.getNotes.mockResolvedValue([]); await expect(() => runSimulator({ artifact, args })).rejects.toThrow( - `Assertion failed: Cannot return zero notes`, + `Assertion failed: Attempted to read past end of BoundedVec`, ); }); });