Skip to content

Commit

Permalink
refactor: update note_getter to NOT fail if returning 0 notes
Browse files Browse the repository at this point in the history
  • Loading branch information
LHerskind committed Jul 5, 2024
1 parent c3c0f2f commit 44f8fbe
Show file tree
Hide file tree
Showing 23 changed files with 32 additions and 54 deletions.
2 changes: 0 additions & 2 deletions noir-projects/aztec-nr/aztec/src/note/note_getter/mod.nr
Original file line number Diff line number Diff line change
Expand Up @@ -161,8 +161,6 @@ fn constrain_get_notes_internal<Note, N, M, FILTER_ARGS>(
assert(filtered_notes[i].is_none(), "Got more notes than limit.");
}

assert(returned_notes.len() != 0, "Cannot return zero notes");

returned_notes
}

Expand Down
11 changes: 0 additions & 11 deletions noir-projects/aztec-nr/aztec/src/note/note_getter/test.nr
Original file line number Diff line number Diff line change
Expand Up @@ -92,17 +92,6 @@ 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() {
let mut env = setup();
let mut context = env.private();

let opt_notes: [Option<MockNote>; 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);
}

#[test(should_fail_with="Got more notes than limit.")]
fn rejects_mote_notes_than_limit() {
let mut env = setup();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,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`
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,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 {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ contract Delegator {
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 {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -59,19 +59,14 @@ contract PendingNoteHashes {
let options = NoteGetterOptions::with_filter(filter_notes_min_sum, amount);
// 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();
let owner_npk_m_hash = header.get_npk_m_hash(&mut context, owner);

// 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
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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`
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -472,7 +472,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);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -295,7 +295,8 @@ contract Token {
Option::none()
).set_limit(1);
let notes = pending_shields.get_notes(options);
let note = notes.get_unchecked(0);
assert(notes.len() == 1, "No pending shield found");
let note = notes.get(0);
// Remove the note from the pending shields set
pending_shields.remove(note);

Expand Down Expand Up @@ -354,7 +355,6 @@ contract Token {
* Accumulate value by spending notes until we hit the missing amount
* If not enough notes are available, recurse to do it again.
* Will use MORE notes than the transfer directly since there are much smaller usual overhead on this function.
* Will fail with `Cannot return zero notes` if there are not enough notes to cover the amount.
*/
#[aztec(private)]
#[aztec(internal)]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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="No pending shield found")]
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);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -123,7 +123,7 @@ impl<T> BalancesMap<T, &mut PrivateContext> {
let options = NoteGetterOptions::with_filter(filter_notes_min_sum, subtrahend).set_limit(limit);
let notes = self.map.at(owner).get_notes(options);

assert(notes.len() > 0, "No notes found");
assert(notes.len() > 0, "Balance too low");

let mut minuend: U128 = U128::from_integer(0);
for i in 0..options.limit {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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`,
"Cannot satisfy constraint 'index < self.len'",
);
});

Expand Down
2 changes: 1 addition & 1 deletion yarn-project/end-to-end/src/e2e_card_game.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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("Cannot satisfy constraint 'index < self.len'");

const collection = await contract.methods.view_collection_cards(firstPlayer, 0).simulate({ from: firstPlayer });
expect(boundedVecToArray(collection)).toHaveLength(1);
Expand Down
2 changes: 1 addition & 1 deletion yarn-project/end-to-end/src/e2e_note_getter.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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`,
"Cannot satisfy constraint 'index < self.len'",
);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,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`,
"Cannot satisfy constraint 'index < self.len'",
);

await expectNoteHashesSquashedExcept(0);
Expand Down Expand Up @@ -245,7 +245,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`,
"Cannot satisfy constraint 'index < self.len'",
);

// second TX creates 1 note, but it is squashed!
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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`,
"Cannot satisfy constraint 'index < self.len'",
);
});

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ describe('e2e_token_contract transfer private', () => {
const amount = balance0 + 1n;
expect(amount).toBeGreaterThan(0n);
await expect(asset.methods.transfer(accounts[1].address, amount).simulate()).rejects.toThrow(
"Assertion failed: Cannot return zero notes 'returned_notes.len() != 0'",
'Assertion failed: Balance too low',
);
});
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,7 @@ describe('e2e_token_contract transfer private', () => {
const amount = balance0 + 1n;
expect(amount).toBeGreaterThan(0n);
await expect(asset.methods.transfer(accounts[1].address, amount).simulate()).rejects.toThrow(
"Assertion failed: Cannot return zero notes 'returned_notes.len() != 0'",
'Assertion failed: Balance too low',
);
});

Expand Down
8 changes: 2 additions & 6 deletions yarn-project/end-to-end/src/guides/dapp_testing.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -215,18 +215,14 @@ describe('guides/dapp/testing', () => {
it('asserts a local transaction simulation fails by calling simulate', async () => {
// docs:start:local-tx-fails
const call = token.methods.transfer(recipient.getAddress(), 200n);
await expect(call.prove()).rejects.toThrow(
"Assertion failed: Cannot return zero notes 'returned_notes.len() != 0'",
);
await expect(call.prove()).rejects.toThrow('Assertion failed: Balance too low');
// docs:end:local-tx-fails
});

it('asserts a local transaction simulation fails by calling send', async () => {
// docs:start:local-tx-fails-send
const call = token.methods.transfer(recipient.getAddress(), 200n);
await expect(call.send().wait()).rejects.toThrow(
"Assertion failed: Cannot return zero notes 'returned_notes.len() != 0'",
);
await expect(call.send().wait()).rejects.toThrow('Assertion failed: Balance too low');
// docs:end:local-tx-fails-send
});

Expand Down
16 changes: 8 additions & 8 deletions yarn-project/simulator/src/client/private_execution.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1109,13 +1109,13 @@ 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 can read the note before it was inserted.
await runSimulator({
args: args,
artifact: artifact,
contractAddress,
});
});
});

Expand Down Expand Up @@ -1143,7 +1143,7 @@ describe('Private Execution test suite', () => {
oracle.getNotes.mockResolvedValue([]);

await expect(() => runSimulator({ artifact, args })).rejects.toThrow(
`Assertion failed: Cannot return zero notes`,
"Cannot satisfy constraint 'index < self.len'",
);
});
});
Expand Down

0 comments on commit 44f8fbe

Please sign in to comment.