Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor: showcase recursion case for transfer #7271

Closed
wants to merge 5 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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 Expand Up @@ -159,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);
}

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
30 changes: 28 additions & 2 deletions noir-projects/noir-contracts/contracts/token_contract/src/main.nr
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 @@ -335,13 +336,38 @@ contract Token {
let to_ivpk = header.get_ivpk_m(&mut context, to);

let amount = U128::from_integer(amount);
storage.balances.sub(from, amount).emit(encode_and_encrypt_note_with_keys_unconstrained(&mut context, from_ovpk, from_ivpk));
let mut (change, missing) = storage.balances.odd_sub(from, amount, 2);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you explain here why we try to fetch only 2 notes? It would be very non-obvious to outsiders reading the code unless they are very knowledgeable about circuit optimization and we want the repo contracts to be an example.


// If we are still missing some balance to cover the amount, call accumulate to spend more notes.
let call = Token::at(context.this_address())._accumulate(from, missing.to_field());
if !missing.eq(U128::zero()) {
change = call.call(&mut context);
}

storage.balances.add(from, change).emit(encode_and_encrypt_note_with_keys_unconstrained(&mut context, from_ovpk, from_ivpk));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
storage.balances.add(from, change).emit(encode_and_encrypt_note_with_keys_unconstrained(&mut context, from_ovpk, from_ivpk));
// We don't constrain encryption of the note log in `transfer` (unlike in `transfer_from`) because transfer
// function is only designed to be used in situations where it's not necessary (e.g. payment to another person
// where the payment is considered to be successful when the other party successfully decrypts a note).
storage.balances.add(from, change).emit(encode_and_encrypt_note_with_keys_unconstrained(&mut context, from_ovpk, from_ivpk));

storage.balances.add(to, amount).emit(encode_and_encrypt_note_with_keys_unconstrained(&mut context, from_ovpk, to_ivpk));

Transfer { from, to, amount: amount.to_field() }.emit(encode_and_encrypt_event_with_keys_unconstrained(&mut context, from_ovpk, to_ivpk));
}
// docs:end:transfer

/**
* 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.
*/
#[aztec(private)]
#[aztec(internal)]
fn _accumulate(owner: AztecAddress, missing_: Field) -> U128 {
// Since we are already spending a lot of doing the call, and we don't do anything else in here, we might as well go over more notes.
let mut (change, missing) = storage.balances.odd_sub(owner, U128::from_integer(missing_), 8);
let call = Token::at(context.this_address())._accumulate(owner, missing.to_field());
if !missing.eq(U128::zero()) {
change = call.call(&mut context);
}
change
}

/**
* Cancel a private authentication witness.
* @param inner_hash The inner hash of the authwit to cancel.
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 @@ -113,6 +113,39 @@ impl<T> BalancesMap<T, &mut PrivateContext> {

self.add(owner, minuend - subtrahend)
}

pub fn odd_sub<T_SERIALIZED_LEN, T_SERIALIZED_BYTES_LEN>(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
pub fn odd_sub<T_SERIALIZED_LEN, T_SERIALIZED_BYTES_LEN>(
/// Performs a subtraction operation on the owner's balance, allowing for partial subtraction.
///
/// This function attempts to subtract a given amount (subtrahend) from the owner's balance,
/// but can handle cases where the balance is insufficient. It returns either the remaining
/// balance after subtraction or the deficit amount.
///
/// # Arguments
/// * `self` - The current instance of the contract.
/// * `owner` - The AztecAddress of the account owner.
/// * `subtrahend` - The amount to be subtracted.
/// * `limit` - The maximum number of notes to consider when calculating the balance.
///
/// # Returns
/// A tuple `(U128, U128)` where:
/// - The first element is the remaining balance if subtraction was successful, or zero if not.
/// - The second element is zero if subtraction was successful, or the deficit amount if not.
///
/// # Type Parameters
/// * `T_SERIALIZED_LEN` - The serialized length of the note.
/// * `T_SERIALIZED_BYTES_LEN` - The serialized bytes length of the note.
/// * `T` - The type implementing NoteInterface and OwnedNote traits.
///
/// # Panics
/// Panics if no notes are found for the given owner (i.e., if the balance is zero).
pub fn odd_sub<T_SERIALIZED_LEN, T_SERIALIZED_BYTES_LEN>(

Claude ai generated nice docs here.

BTW agree with Grego that odd_sub is not that great of a name. Maybe sub_with_deficit would be better?

self: Self,
owner: AztecAddress,
subtrahend: U128,
limit: u32
) -> (U128, U128) where T: NoteInterface<T_SERIALIZED_LEN, T_SERIALIZED_BYTES_LEN> + OwnedNote {
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, "Balance too low");

let mut minuend: U128 = U128::from_integer(0);
for i in 0..options.limit {
if i < notes.len() {
let note = notes.get_unchecked(i);

// Removes the note from the owner's set of notes.
// This will call the the `compute_nullifer` function of the `token_note`
// which require knowledge of the secret key (currently the users encryption key).
// The contract logic must ensure that the spending key is used as well.
self.map.at(owner).remove(note);

minuend = minuend + note.get_amount();
}
}

if minuend >= subtrahend {
(minuend - subtrahend, U128::zero())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
(minuend - subtrahend, U128::zero())
// If balance is sufficient, return remaining balance and zero deficit
(minuend - subtrahend, U128::zero())

} else {
(U128::zero(), subtrahend - minuend)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
(U128::zero(), subtrahend - minuend)
// If balance is insufficient, return zero remaining and the deficit amount
(U128::zero(), subtrahend - minuend)

}
}
}

pub fn filter_notes_min_sum<T, T_SERIALIZED_LEN, T_SERIALIZED_BYTES_LEN>(
Expand Down
42 changes: 18 additions & 24 deletions noir/noir-repo/tooling/nargo/src/errors.rs
Original file line number Diff line number Diff line change
Expand Up @@ -64,31 +64,25 @@ impl<F: AcirField> NargoError<F> {
&self,
error_types: &BTreeMap<ErrorSelector, AbiErrorType>,
) -> Option<String> {
let execution_error = match self {
NargoError::ExecutionError(error) => error,
_ => return None,
};

match execution_error {
ExecutionError::AssertionFailed(payload, _) => match payload {
ResolvedAssertionPayload::String(message) => Some(message.to_string()),
ResolvedAssertionPayload::Raw(raw) => {
let abi_type = error_types.get(&raw.selector)?;
let decoded = display_abi_error(&raw.data, abi_type.clone());
Some(decoded.to_string())
}
},
ExecutionError::SolvingError(error, _) => match error {
OpcodeResolutionError::IndexOutOfBounds { .. }
| OpcodeResolutionError::OpcodeNotSolvable(_)
| OpcodeResolutionError::UnsatisfiedConstrain { .. }
| OpcodeResolutionError::AcirMainCallAttempted { .. }
| OpcodeResolutionError::BrilligFunctionFailed { .. }
| OpcodeResolutionError::AcirCallOutputsMismatch { .. } => None,
OpcodeResolutionError::BlackBoxFunctionFailed(_, reason) => {
Some(reason.to_string())
}
match self {
NargoError::ExecutionError(error) => match error {
ExecutionError::AssertionFailed(payload, _) => match payload {
ResolvedAssertionPayload::String(message) => Some(message.to_string()),
ResolvedAssertionPayload::Raw(raw) => {
let abi_type = error_types.get(&raw.selector)?;
let decoded = display_abi_error(&raw.data, abi_type.clone());
Some(decoded.to_string())
}
},
ExecutionError::SolvingError(error, _) => match error {
OpcodeResolutionError::BlackBoxFunctionFailed(_, reason) => {
Some(reason.to_string())
}
_ => None,
},
},
NargoError::ForeignCallError(error) => Some(error.to_string()),
_ => None,
}
}
}
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("Assertion failed: Card not found 'card_note.is_some()'");

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 @@ -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).prove();

await expectNoteHashesSquashedExcept(0);
await expectNullifiersSquashedExcept(0);
Expand Down Expand Up @@ -244,9 +242,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).prove();

// second TX creates 1 note, but it is squashed!
await expectNoteHashesSquashedExcept(0);
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`,
"Assertion failed: No pending shield found 'notes.len() == 1'",
);
});

Expand Down
Loading
Loading