From 236e4031ade3cb2248408eca18920d69d3527764 Mon Sep 17 00:00:00 2001 From: benesjan Date: Mon, 19 May 2025 09:07:09 +0000 Subject: [PATCH 01/14] feat: validating partial note sender --- .../src/messages/discovery/nonce_discovery.nr | 2 +- .../contracts/app/nft_contract/src/main.nr | 25 ++++++++++++------- .../app/nft_contract/src/types/nft_note.nr | 17 ++++++++----- 3 files changed, 28 insertions(+), 16 deletions(-) diff --git a/noir-projects/aztec-nr/aztec/src/messages/discovery/nonce_discovery.nr b/noir-projects/aztec-nr/aztec/src/messages/discovery/nonce_discovery.nr index 83d0115f8663..c04034459664 100644 --- a/noir-projects/aztec-nr/aztec/src/messages/discovery/nonce_discovery.nr +++ b/noir-projects/aztec-nr/aztec/src/messages/discovery/nonce_discovery.nr @@ -32,7 +32,7 @@ pub unconstrained fn attempt_note_nonce_discovery( note_type_id: Field, packed_note: BoundedVec, ) -> BoundedVec { - let discovered_notes = &mut BoundedVec::new(); + let discovered_notes: &mut BoundedVec = &mut BoundedVec::new(); debug_log_format( "Attempting nonce discovery on {0} potential notes on contract {1} for storage slot {2}", diff --git a/noir-projects/noir-contracts/contracts/app/nft_contract/src/main.nr b/noir-projects/noir-contracts/contracts/app/nft_contract/src/main.nr index d6770db33f83..e8d88afdf172 100644 --- a/noir-projects/noir-contracts/contracts/app/nft_contract/src/main.nr +++ b/noir-projects/noir-contracts/contracts/app/nft_contract/src/main.nr @@ -197,16 +197,20 @@ pub contract NFT { context: &mut PrivateContext, storage: Storage<&mut PrivateContext>, ) -> PartialNFTNote { + let sender = context.msg_sender(); + // We create a partial note with unpopulated/zero token id for 'to' let partial_note = NFTNote::partial( to, storage.private_nfts.at(to).storage_slot, context, to, - context.msg_sender(), + sender, ); - NFT::at(context.this_address())._store_nft_set_partial_note(partial_note).enqueue(context); + NFT::at(context.this_address())._store_nft_set_partial_note(sender, partial_note).enqueue( + context, + ); partial_note } @@ -215,12 +219,12 @@ pub contract NFT { // docs:start:store_payload_in_transient_storage_unsafe #[public] #[internal] - fn _store_nft_set_partial_note(partial_note: PartialNFTNote) { + fn _store_nft_set_partial_note(sender: AztecAddress, partial_note: PartialNFTNote) { // We store the partial note in a slot equal to its commitment. This is safe because the commitment is computed // using a generator different from the one used to compute storage slots, so there can be no collisions. // We could consider storing all pending partial notes in e.g. some array, but ultimately this is pointless: all // we need to verify is that the note is valid. - context.storage_write(partial_note.commitment(), true); + context.storage_write(partial_note.compute_validity_commitment(sender), true); } // docs:end:store_payload_in_transient_storage_unsafe /// Finalizes a transfer of NFT with `token_id` from public balance of `from` to a private balance of `to`. @@ -260,11 +264,14 @@ pub contract NFT { // Set the public NFT owner to zero public_owners_storage.write(AztecAddress::zero()); - // We verify that the partial note we're completing is valid (i.e. it uses the correct state variable's storage - // slot, and it is internally consistent). We *could* clear the storage since each partial note should only be - // used once, but since the AVM offers no gas refunds for doing so this would just make the transaction be more - // expensive. - assert(context.storage_read(partial_note.commitment()), "Invalid partial note"); + // We verify that the partial note we're completing is valid (i.e. sender is correct, it uses the correct state + // variable's storage slot, and it is internally consistent). We *could* clear the storage since each partial + // note should only be used once, but since the AVM offers no gas refunds for doing so this would just make + // the transaction be more expensive. + assert( + context.storage_read(partial_note.compute_validity_commitment(from)), + "Invalid partial note", + ); partial_note.complete(token_id, context); } diff --git a/noir-projects/noir-contracts/contracts/app/nft_contract/src/types/nft_note.nr b/noir-projects/noir-contracts/contracts/app/nft_contract/src/types/nft_note.nr index 3d250f37df96..630acc1d1ec6 100644 --- a/noir-projects/noir-contracts/contracts/app/nft_contract/src/types/nft_note.nr +++ b/noir-projects/noir-contracts/contracts/app/nft_contract/src/types/nft_note.nr @@ -8,8 +8,8 @@ use dep::aztec::{ protocol_types::{ address::AztecAddress, constants::{GENERATOR_INDEX__NOTE_HASH, GENERATOR_INDEX__NOTE_NULLIFIER}, - hash::poseidon2_hash_with_separator, - traits::{Deserialize, Hash, Packable, Serialize}, + hash::{poseidon2_hash_with_separator, sha256_to_field}, + traits::{Deserialize, Hash, Packable, Serialize, ToField}, utils::arrays::array_concat, }, }; @@ -202,12 +202,17 @@ pub struct PartialNFTNote { } impl PartialNFTNote { - pub fn commitment(self) -> Field { - self.commitment + /// Computes a partial note validity commitment. This commitment is stored in public storage when creating the + /// partial note in private and then is used to check that sender and note are valid upon partial note completion + /// in public. + pub fn compute_validity_commitment(self, sender: AztecAddress) -> Field { + let commitment_bytes: [u8; 32] = self.commitment.to_be_bytes(); + let sender_bytes: [u8; 32] = sender.to_field().to_be_bytes(); + // TODO(#10537): Can we use poseidon2 here given that partial_note.commitment() was computed with it? Or at + // least pedersen? + sha256_to_field(array_concat(commitment_bytes, sender_bytes)) } -} -impl PartialNFTNote { /// Completes the partial note, creating a new note that can be used like any other NFTNote. pub fn complete(self, token_id: Field, context: &mut PublicContext) { // A note with a value of zero is valid, but we cannot currently complete a partial note with such a value From f99d4ec71fb56a13cd14d2e90755e7d1186594b0 Mon Sep 17 00:00:00 2001 From: benesjan Date: Mon, 19 May 2025 09:34:59 +0000 Subject: [PATCH 02/14] separator --- .../app/nft_contract/src/types/nft_note.nr | 20 +++++++++++++++++-- .../crates/types/src/constants.nr | 1 + 2 files changed, 19 insertions(+), 2 deletions(-) diff --git a/noir-projects/noir-contracts/contracts/app/nft_contract/src/types/nft_note.nr b/noir-projects/noir-contracts/contracts/app/nft_contract/src/types/nft_note.nr index 630acc1d1ec6..7064595c3fab 100644 --- a/noir-projects/noir-contracts/contracts/app/nft_contract/src/types/nft_note.nr +++ b/noir-projects/noir-contracts/contracts/app/nft_contract/src/types/nft_note.nr @@ -7,7 +7,10 @@ use dep::aztec::{ oracle::random::random, protocol_types::{ address::AztecAddress, - constants::{GENERATOR_INDEX__NOTE_HASH, GENERATOR_INDEX__NOTE_NULLIFIER}, + constants::{ + GENERATOR_INDEX__NOTE_HASH, GENERATOR_INDEX__NOTE_NULLIFIER, + GENERATOR_INDEX__PARTIAL_NOTE_VALIDITY_COMMITMENT, + }, hash::{poseidon2_hash_with_separator, sha256_to_field}, traits::{Deserialize, Hash, Packable, Serialize, ToField}, utils::arrays::array_concat, @@ -208,9 +211,22 @@ impl PartialNFTNote { pub fn compute_validity_commitment(self, sender: AztecAddress) -> Field { let commitment_bytes: [u8; 32] = self.commitment.to_be_bytes(); let sender_bytes: [u8; 32] = sender.to_field().to_be_bytes(); + // We use separator because the resulting commitment is used as a public storage slot in the partial note + // validity check scheme. + // (We use 4 bytes for the value because the constant is u32) + let separator_bytes: [u8; 4] = + (GENERATOR_INDEX__PARTIAL_NOTE_VALIDITY_COMMITMENT as Field).to_be_bytes(); // TODO(#10537): Can we use poseidon2 here given that partial_note.commitment() was computed with it? Or at // least pedersen? - sha256_to_field(array_concat(commitment_bytes, sender_bytes)) + // TODO: Is the hash really properly separated given that it's just appended to the array and has no "special + // place"? (e.g. how pedersen has a special generator for the separator) + // (Not dealing with the second TODO now as we will likely replace the hash function anyway). + sha256_to_field( + array_concat( + array_concat(commitment_bytes, sender_bytes), + separator_bytes, + ), + ) } /// Completes the partial note, creating a new note that can be used like any other NFTNote. diff --git a/noir-projects/noir-protocol-circuits/crates/types/src/constants.nr b/noir-projects/noir-protocol-circuits/crates/types/src/constants.nr index 6eccfe6f38bd..8ff6111fab4a 100644 --- a/noir-projects/noir-protocol-circuits/crates/types/src/constants.nr +++ b/noir-projects/noir-protocol-circuits/crates/types/src/constants.nr @@ -549,6 +549,7 @@ pub global GENERATOR_INDEX__SYMMETRIC_KEY: u8 = 54; pub global GENERATOR_INDEX__SYMMETRIC_KEY_2: u8 = 55; pub global GENERATOR_INDEX__PUBLIC_TX_HASH: u32 = 56; pub global GENERATOR_INDEX__PRIVATE_TX_HASH: u32 = 57; +pub global GENERATOR_INDEX__PARTIAL_NOTE_VALIDITY_COMMITMENT: u32 = 58; // AVM memory tags pub global MEM_TAG_FF: Field = 0; From 03c16a5eeeff2ea1779ec571fa556a289a145d80 Mon Sep 17 00:00:00 2001 From: benesjan Date: Mon, 19 May 2025 09:38:11 +0000 Subject: [PATCH 03/14] cleanup --- .../aztec-nr/aztec/src/messages/discovery/nonce_discovery.nr | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/noir-projects/aztec-nr/aztec/src/messages/discovery/nonce_discovery.nr b/noir-projects/aztec-nr/aztec/src/messages/discovery/nonce_discovery.nr index c04034459664..83d0115f8663 100644 --- a/noir-projects/aztec-nr/aztec/src/messages/discovery/nonce_discovery.nr +++ b/noir-projects/aztec-nr/aztec/src/messages/discovery/nonce_discovery.nr @@ -32,7 +32,7 @@ pub unconstrained fn attempt_note_nonce_discovery( note_type_id: Field, packed_note: BoundedVec, ) -> BoundedVec { - let discovered_notes: &mut BoundedVec = &mut BoundedVec::new(); + let discovered_notes = &mut BoundedVec::new(); debug_log_format( "Attempting nonce discovery on {0} potential notes on contract {1} for storage slot {2}", From f4c874a70d76185859b5cbdfdd7467765c289e24 Mon Sep 17 00:00:00 2001 From: benesjan Date: Mon, 19 May 2025 09:39:18 +0000 Subject: [PATCH 04/14] Revert "separator" This reverts commit 170e747d5bf9f03fba6744c278164d5eb5359a9a. --- .../app/nft_contract/src/types/nft_note.nr | 20 ++----------------- .../crates/types/src/constants.nr | 1 - 2 files changed, 2 insertions(+), 19 deletions(-) diff --git a/noir-projects/noir-contracts/contracts/app/nft_contract/src/types/nft_note.nr b/noir-projects/noir-contracts/contracts/app/nft_contract/src/types/nft_note.nr index 7064595c3fab..630acc1d1ec6 100644 --- a/noir-projects/noir-contracts/contracts/app/nft_contract/src/types/nft_note.nr +++ b/noir-projects/noir-contracts/contracts/app/nft_contract/src/types/nft_note.nr @@ -7,10 +7,7 @@ use dep::aztec::{ oracle::random::random, protocol_types::{ address::AztecAddress, - constants::{ - GENERATOR_INDEX__NOTE_HASH, GENERATOR_INDEX__NOTE_NULLIFIER, - GENERATOR_INDEX__PARTIAL_NOTE_VALIDITY_COMMITMENT, - }, + constants::{GENERATOR_INDEX__NOTE_HASH, GENERATOR_INDEX__NOTE_NULLIFIER}, hash::{poseidon2_hash_with_separator, sha256_to_field}, traits::{Deserialize, Hash, Packable, Serialize, ToField}, utils::arrays::array_concat, @@ -211,22 +208,9 @@ impl PartialNFTNote { pub fn compute_validity_commitment(self, sender: AztecAddress) -> Field { let commitment_bytes: [u8; 32] = self.commitment.to_be_bytes(); let sender_bytes: [u8; 32] = sender.to_field().to_be_bytes(); - // We use separator because the resulting commitment is used as a public storage slot in the partial note - // validity check scheme. - // (We use 4 bytes for the value because the constant is u32) - let separator_bytes: [u8; 4] = - (GENERATOR_INDEX__PARTIAL_NOTE_VALIDITY_COMMITMENT as Field).to_be_bytes(); // TODO(#10537): Can we use poseidon2 here given that partial_note.commitment() was computed with it? Or at // least pedersen? - // TODO: Is the hash really properly separated given that it's just appended to the array and has no "special - // place"? (e.g. how pedersen has a special generator for the separator) - // (Not dealing with the second TODO now as we will likely replace the hash function anyway). - sha256_to_field( - array_concat( - array_concat(commitment_bytes, sender_bytes), - separator_bytes, - ), - ) + sha256_to_field(array_concat(commitment_bytes, sender_bytes)) } /// Completes the partial note, creating a new note that can be used like any other NFTNote. diff --git a/noir-projects/noir-protocol-circuits/crates/types/src/constants.nr b/noir-projects/noir-protocol-circuits/crates/types/src/constants.nr index 8ff6111fab4a..6eccfe6f38bd 100644 --- a/noir-projects/noir-protocol-circuits/crates/types/src/constants.nr +++ b/noir-projects/noir-protocol-circuits/crates/types/src/constants.nr @@ -549,7 +549,6 @@ pub global GENERATOR_INDEX__SYMMETRIC_KEY: u8 = 54; pub global GENERATOR_INDEX__SYMMETRIC_KEY_2: u8 = 55; pub global GENERATOR_INDEX__PUBLIC_TX_HASH: u32 = 56; pub global GENERATOR_INDEX__PRIVATE_TX_HASH: u32 = 57; -pub global GENERATOR_INDEX__PARTIAL_NOTE_VALIDITY_COMMITMENT: u32 = 58; // AVM memory tags pub global MEM_TAG_FF: Field = 0; From 305663fdd9e561203f836d0039dd5e89ea3018e4 Mon Sep 17 00:00:00 2001 From: benesjan Date: Mon, 19 May 2025 09:46:36 +0000 Subject: [PATCH 05/14] comment fixes --- .../noir-contracts/contracts/app/nft_contract/src/main.nr | 8 ++++---- .../contracts/app/nft_contract/src/types/nft_note.nr | 2 ++ 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/noir-projects/noir-contracts/contracts/app/nft_contract/src/main.nr b/noir-projects/noir-contracts/contracts/app/nft_contract/src/main.nr index e8d88afdf172..6ab0dd324ee4 100644 --- a/noir-projects/noir-contracts/contracts/app/nft_contract/src/main.nr +++ b/noir-projects/noir-contracts/contracts/app/nft_contract/src/main.nr @@ -220,10 +220,10 @@ pub contract NFT { #[public] #[internal] fn _store_nft_set_partial_note(sender: AztecAddress, partial_note: PartialNFTNote) { - // We store the partial note in a slot equal to its commitment. This is safe because the commitment is computed - // using a generator different from the one used to compute storage slots, so there can be no collisions. - // We could consider storing all pending partial notes in e.g. some array, but ultimately this is pointless: all - // we need to verify is that the note is valid. + // We store the partial note validity flag in a slot equal to its validity commitment. This is safe because + // the commitment is computed using a generator different from the one used to compute storage slots, so there + // can be no collisions. We could consider storing all pending partial notes in e.g. some array, but ultimately + // this is pointless: all we need to verify is that the note is valid. context.storage_write(partial_note.compute_validity_commitment(sender), true); } // docs:end:store_payload_in_transient_storage_unsafe diff --git a/noir-projects/noir-contracts/contracts/app/nft_contract/src/types/nft_note.nr b/noir-projects/noir-contracts/contracts/app/nft_contract/src/types/nft_note.nr index 630acc1d1ec6..76a70f14d20d 100644 --- a/noir-projects/noir-contracts/contracts/app/nft_contract/src/types/nft_note.nr +++ b/noir-projects/noir-contracts/contracts/app/nft_contract/src/types/nft_note.nr @@ -208,6 +208,8 @@ impl PartialNFTNote { pub fn compute_validity_commitment(self, sender: AztecAddress) -> Field { let commitment_bytes: [u8; 32] = self.commitment.to_be_bytes(); let sender_bytes: [u8; 32] = sender.to_field().to_be_bytes(); + // We don't use separator here because `self.commitment` (computed by `compute_partial_commitment`) already + // includes it. // TODO(#10537): Can we use poseidon2 here given that partial_note.commitment() was computed with it? Or at // least pedersen? sha256_to_field(array_concat(commitment_bytes, sender_bytes)) From 54634eaf0679008f506b3bd7f598ed388f8f12f1 Mon Sep 17 00:00:00 2001 From: benesjan Date: Tue, 20 May 2025 07:30:26 +0000 Subject: [PATCH 06/14] poseidon instead of sha --- .../contracts/app/nft_contract/src/types/nft_note.nr | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/noir-projects/noir-contracts/contracts/app/nft_contract/src/types/nft_note.nr b/noir-projects/noir-contracts/contracts/app/nft_contract/src/types/nft_note.nr index 76a70f14d20d..e0ac5e7207c4 100644 --- a/noir-projects/noir-contracts/contracts/app/nft_contract/src/types/nft_note.nr +++ b/noir-projects/noir-contracts/contracts/app/nft_contract/src/types/nft_note.nr @@ -8,7 +8,7 @@ use dep::aztec::{ protocol_types::{ address::AztecAddress, constants::{GENERATOR_INDEX__NOTE_HASH, GENERATOR_INDEX__NOTE_NULLIFIER}, - hash::{poseidon2_hash_with_separator, sha256_to_field}, + hash::{poseidon2_hash, poseidon2_hash_with_separator}, traits::{Deserialize, Hash, Packable, Serialize, ToField}, utils::arrays::array_concat, }, @@ -206,13 +206,9 @@ impl PartialNFTNote { /// partial note in private and then is used to check that sender and note are valid upon partial note completion /// in public. pub fn compute_validity_commitment(self, sender: AztecAddress) -> Field { - let commitment_bytes: [u8; 32] = self.commitment.to_be_bytes(); - let sender_bytes: [u8; 32] = sender.to_field().to_be_bytes(); // We don't use separator here because `self.commitment` (computed by `compute_partial_commitment`) already // includes it. - // TODO(#10537): Can we use poseidon2 here given that partial_note.commitment() was computed with it? Or at - // least pedersen? - sha256_to_field(array_concat(commitment_bytes, sender_bytes)) + poseidon2_hash([self.commitment, sender.to_field()]) } /// Completes the partial note, creating a new note that can be used like any other NFTNote. From 5c7fc7f0a71dc8c5bdc33d12ec15d34397e3f676 Mon Sep 17 00:00:00 2001 From: benesjan Date: Tue, 20 May 2025 07:35:33 +0000 Subject: [PATCH 07/14] computing hash in private --- .../contracts/app/nft_contract/src/main.nr | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/noir-projects/noir-contracts/contracts/app/nft_contract/src/main.nr b/noir-projects/noir-contracts/contracts/app/nft_contract/src/main.nr index 6ab0dd324ee4..6a73172e49ba 100644 --- a/noir-projects/noir-contracts/contracts/app/nft_contract/src/main.nr +++ b/noir-projects/noir-contracts/contracts/app/nft_contract/src/main.nr @@ -208,7 +208,11 @@ pub contract NFT { sender, ); - NFT::at(context.this_address())._store_nft_set_partial_note(sender, partial_note).enqueue( + // We compute the validity commitment of the partial note and store it in public storage for later partial note + // validity check. + let validity_commitment = partial_note.compute_validity_commitment(sender); + + NFT::at(context.this_address())._store_nft_set_partial_note(validity_commitment).enqueue( context, ); @@ -219,12 +223,12 @@ pub contract NFT { // docs:start:store_payload_in_transient_storage_unsafe #[public] #[internal] - fn _store_nft_set_partial_note(sender: AztecAddress, partial_note: PartialNFTNote) { + fn _store_nft_set_partial_note(validity_commitment: Field) { // We store the partial note validity flag in a slot equal to its validity commitment. This is safe because // the commitment is computed using a generator different from the one used to compute storage slots, so there // can be no collisions. We could consider storing all pending partial notes in e.g. some array, but ultimately // this is pointless: all we need to verify is that the note is valid. - context.storage_write(partial_note.compute_validity_commitment(sender), true); + context.storage_write(validity_commitment, true); } // docs:end:store_payload_in_transient_storage_unsafe /// Finalizes a transfer of NFT with `token_id` from public balance of `from` to a private balance of `to`. From def3e2cd77fea9a90d695f2cfd7b038c154cb4a7 Mon Sep 17 00:00:00 2001 From: benesjan Date: Tue, 20 May 2025 07:36:48 +0000 Subject: [PATCH 08/14] _store_nft_set_partial_note --- .../noir-contracts/contracts/app/nft_contract/src/main.nr | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/noir-projects/noir-contracts/contracts/app/nft_contract/src/main.nr b/noir-projects/noir-contracts/contracts/app/nft_contract/src/main.nr index 6a73172e49ba..a71b3f42df97 100644 --- a/noir-projects/noir-contracts/contracts/app/nft_contract/src/main.nr +++ b/noir-projects/noir-contracts/contracts/app/nft_contract/src/main.nr @@ -212,7 +212,7 @@ pub contract NFT { // validity check. let validity_commitment = partial_note.compute_validity_commitment(sender); - NFT::at(context.this_address())._store_nft_set_partial_note(validity_commitment).enqueue( + NFT::at(context.this_address())._set_nft_partial_note_validity(validity_commitment).enqueue( context, ); @@ -223,7 +223,7 @@ pub contract NFT { // docs:start:store_payload_in_transient_storage_unsafe #[public] #[internal] - fn _store_nft_set_partial_note(validity_commitment: Field) { + fn _set_nft_partial_note_validity(validity_commitment: Field) { // We store the partial note validity flag in a slot equal to its validity commitment. This is safe because // the commitment is computed using a generator different from the one used to compute storage slots, so there // can be no collisions. We could consider storing all pending partial notes in e.g. some array, but ultimately From ff30bc5018985fe112ad7941d72b312d444fe54a Mon Sep 17 00:00:00 2001 From: benesjan Date: Tue, 20 May 2025 10:22:19 +0000 Subject: [PATCH 09/14] proper separation of validity commitment --- .../app/nft_contract/src/types/nft_note.nr | 14 +++++++++----- .../crates/types/src/constants.nr | 1 + yarn-project/constants/src/constants.gen.ts | 1 + 3 files changed, 11 insertions(+), 5 deletions(-) diff --git a/noir-projects/noir-contracts/contracts/app/nft_contract/src/types/nft_note.nr b/noir-projects/noir-contracts/contracts/app/nft_contract/src/types/nft_note.nr index e0ac5e7207c4..13aacb0ec62a 100644 --- a/noir-projects/noir-contracts/contracts/app/nft_contract/src/types/nft_note.nr +++ b/noir-projects/noir-contracts/contracts/app/nft_contract/src/types/nft_note.nr @@ -7,8 +7,11 @@ use dep::aztec::{ oracle::random::random, protocol_types::{ address::AztecAddress, - constants::{GENERATOR_INDEX__NOTE_HASH, GENERATOR_INDEX__NOTE_NULLIFIER}, - hash::{poseidon2_hash, poseidon2_hash_with_separator}, + constants::{ + GENERATOR_INDEX__NOTE_HASH, GENERATOR_INDEX__NOTE_NULLIFIER, + GENERATOR_INDEX__PARTIAL_NOTE_VALIDITY_COMMITMENT, + }, + hash::poseidon2_hash_with_separator, traits::{Deserialize, Hash, Packable, Serialize, ToField}, utils::arrays::array_concat, }, @@ -206,9 +209,10 @@ impl PartialNFTNote { /// partial note in private and then is used to check that sender and note are valid upon partial note completion /// in public. pub fn compute_validity_commitment(self, sender: AztecAddress) -> Field { - // We don't use separator here because `self.commitment` (computed by `compute_partial_commitment`) already - // includes it. - poseidon2_hash([self.commitment, sender.to_field()]) + poseidon2_hash_with_separator( + [self.commitment, sender.to_field()], + GENERATOR_INDEX__PARTIAL_NOTE_VALIDITY_COMMITMENT, + ) } /// Completes the partial note, creating a new note that can be used like any other NFTNote. diff --git a/noir-projects/noir-protocol-circuits/crates/types/src/constants.nr b/noir-projects/noir-protocol-circuits/crates/types/src/constants.nr index 6eccfe6f38bd..8ff6111fab4a 100644 --- a/noir-projects/noir-protocol-circuits/crates/types/src/constants.nr +++ b/noir-projects/noir-protocol-circuits/crates/types/src/constants.nr @@ -549,6 +549,7 @@ pub global GENERATOR_INDEX__SYMMETRIC_KEY: u8 = 54; pub global GENERATOR_INDEX__SYMMETRIC_KEY_2: u8 = 55; pub global GENERATOR_INDEX__PUBLIC_TX_HASH: u32 = 56; pub global GENERATOR_INDEX__PRIVATE_TX_HASH: u32 = 57; +pub global GENERATOR_INDEX__PARTIAL_NOTE_VALIDITY_COMMITMENT: u32 = 58; // AVM memory tags pub global MEM_TAG_FF: Field = 0; diff --git a/yarn-project/constants/src/constants.gen.ts b/yarn-project/constants/src/constants.gen.ts index b71862956438..ef0a9e3233ef 100644 --- a/yarn-project/constants/src/constants.gen.ts +++ b/yarn-project/constants/src/constants.gen.ts @@ -451,4 +451,5 @@ export enum GeneratorIndex { SYMMETRIC_KEY_2 = 55, PUBLIC_TX_HASH = 56, PRIVATE_TX_HASH = 57, + PARTIAL_NOTE_VALIDITY_COMMITMENT = 58, } From 7d2b8eea92334645ebd558bad1c7c523c4cfa88c Mon Sep 17 00:00:00 2001 From: benesjan Date: Tue, 20 May 2025 14:16:34 +0000 Subject: [PATCH 10/14] updating fungible token contract and introducing completer --- .../aztec-nr/uint-note/src/uint_note.nr | 17 ++- .../contracts/app/nft_contract/src/main.nr | 63 ++++++--- .../app/nft_contract/src/types/nft_note.nr | 8 +- .../app/simple_token_contract/src/main.nr | 72 +++++++--- .../contracts/app/token_contract/src/main.nr | 128 ++++++++++++------ 5 files changed, 203 insertions(+), 85 deletions(-) diff --git a/noir-projects/aztec-nr/uint-note/src/uint_note.nr b/noir-projects/aztec-nr/uint-note/src/uint_note.nr index ff61d069b480..c3054677bdff 100644 --- a/noir-projects/aztec-nr/uint-note/src/uint_note.nr +++ b/noir-projects/aztec-nr/uint-note/src/uint_note.nr @@ -7,7 +7,10 @@ use dep::aztec::{ oracle::random::random, protocol_types::{ address::AztecAddress, - constants::{GENERATOR_INDEX__NOTE_HASH, GENERATOR_INDEX__NOTE_NULLIFIER}, + constants::{ + GENERATOR_INDEX__NOTE_HASH, GENERATOR_INDEX__NOTE_NULLIFIER, + GENERATOR_INDEX__PARTIAL_NOTE_VALIDITY_COMMITMENT, + }, hash::poseidon2_hash_with_separator, traits::{Deserialize, Hash, Packable, Serialize, ToField}, utils::arrays::array_concat, @@ -201,12 +204,16 @@ pub struct PartialUintNote { } impl PartialUintNote { - pub fn commitment(self) -> Field { - self.commitment + /// Computes a partial note validity commitment. This commitment is stored in public storage when creating the + /// partial note in private and then is used to check that completer and note are valid upon partial note + /// completion in public (completer is the entity that can complete the partial note). + pub fn compute_validity_commitment(self, completer: AztecAddress) -> Field { + poseidon2_hash_with_separator( + [self.commitment, completer.to_field()], + GENERATOR_INDEX__PARTIAL_NOTE_VALIDITY_COMMITMENT, + ) } -} -impl PartialUintNote { /// Completes the partial note, creating a new note that can be used like any other UintNote. pub fn complete(self, value: u128, context: &mut PublicContext) { // A note with a value of zero is valid, but we cannot currently complete a partial note with such a value diff --git a/noir-projects/noir-contracts/contracts/app/nft_contract/src/main.nr b/noir-projects/noir-contracts/contracts/app/nft_contract/src/main.nr index a71b3f42df97..92a1c95be527 100644 --- a/noir-projects/noir-contracts/contracts/app/nft_contract/src/main.nr +++ b/noir-projects/noir-contracts/contracts/app/nft_contract/src/main.nr @@ -197,7 +197,7 @@ pub contract NFT { context: &mut PrivateContext, storage: Storage<&mut PrivateContext>, ) -> PartialNFTNote { - let sender = context.msg_sender(); + let sender_and_completer = context.msg_sender(); // We create a partial note with unpopulated/zero token id for 'to' let partial_note = NFTNote::partial( @@ -205,12 +205,18 @@ pub contract NFT { storage.private_nfts.at(to).storage_slot, context, to, - sender, + sender_and_completer, ); - // We compute the validity commitment of the partial note and store it in public storage for later partial note - // validity check. - let validity_commitment = partial_note.compute_validity_commitment(sender); + // We can't simply return the partial note because we won't be able to later on verify that it was created + // correctly (e.g. that the storage slot corresponds to the owner, and that we're using the `private_nfts` + // state variable and not another state variable) once this information is hidden in the partial note + // commitment. We also want to verify that only the caller of this function can complete the partial note. + // + // We achieve the above by storing a flag in public storage under a slot index equal to its + // `validity_commitment`. This commitment contains all the necessary information to verify the completer and + // to validate the partial note. + let validity_commitment = partial_note.compute_validity_commitment(sender_and_completer); NFT::at(context.this_address())._set_nft_partial_note_validity(validity_commitment).enqueue( context, @@ -231,49 +237,68 @@ pub contract NFT { context.storage_write(validity_commitment, true); } // docs:end:store_payload_in_transient_storage_unsafe - /// Finalizes a transfer of NFT with `token_id` from public balance of `from` to a private balance of `to`. - /// The transfer must be prepared by calling `prepare_private_balance_increase` first and the resulting - /// `partial_note` must be passed as an argument to this function. + /// Finalizes a transfer of NFT with `token_id` from public balance of `msg_sender` to a private balance of `to`. + /// The transfer must be prepared by calling `prepare_private_balance_increase` from `msg_sender` account and the + /// resulting `partial_note` must be passed as an argument to this function. // docs:start:finalize_transfer_to_private #[public] fn finalize_transfer_to_private(token_id: Field, partial_note: PartialNFTNote) { - let from = context.msg_sender(); - _finalize_transfer_to_private(from, token_id, partial_note, &mut context, storage); + // Completer is the entity that can complete the partial note. In this case, it's the same as the account + // `from` from whose account the token is being transferred. + let from_and_completer = context.msg_sender(); + _finalize_transfer_to_private( + from_and_completer, + token_id, + partial_note, + &mut context, + storage, + ); } // docs:end:finalize_transfer_to_private // docs:start:finalize_transfer_to_private_unsafe + /// This is a wrapper around `_finalize_transfer_to_private` placed here so that a call + /// to `_finalize_transfer_to_private` can be enqueued. Called unsafe as it does not check `from_and_completer` + /// (this has to be done in the calling function). #[public] #[internal] fn _finalize_transfer_to_private_unsafe( - from: AztecAddress, + from_and_completer: AztecAddress, token_id: Field, partial_note: PartialNFTNote, ) { - _finalize_transfer_to_private(from, token_id, partial_note, &mut context, storage); + _finalize_transfer_to_private( + from_and_completer, + token_id, + partial_note, + &mut context, + storage, + ); } // docs:end:finalize_transfer_to_private_unsafe + // In all the flows in this contract, `from` (the account from which we're transferring the NFT) and `completer` + // (the entity that can complete the partial note) are the same so we represent them with a single argument. #[contract_library_method] fn _finalize_transfer_to_private( - from: AztecAddress, + from_and_completer: AztecAddress, token_id: Field, partial_note: PartialNFTNote, context: &mut PublicContext, storage: Storage<&mut PublicContext>, ) { let public_owners_storage = storage.public_owners.at(token_id); - assert(public_owners_storage.read().eq(from), "invalid NFT owner"); + assert(public_owners_storage.read().eq(from_and_completer), "invalid NFT owner"); // Set the public NFT owner to zero public_owners_storage.write(AztecAddress::zero()); - // We verify that the partial note we're completing is valid (i.e. sender is correct, it uses the correct state - // variable's storage slot, and it is internally consistent). We *could* clear the storage since each partial - // note should only be used once, but since the AVM offers no gas refunds for doing so this would just make - // the transaction be more expensive. + // We verify that the partial note we're completing is valid (i.e. completer is correct, it uses the correct + // state variable's storage slot, and it is internally consistent). We *could* clear the storage since each + // partial note should only be used once, but since the AVM offers no gas refunds for doing so this would just + // make the transaction be more expensive. assert( - context.storage_read(partial_note.compute_validity_commitment(from)), + context.storage_read(partial_note.compute_validity_commitment(from_and_completer)), "Invalid partial note", ); partial_note.complete(token_id, context); diff --git a/noir-projects/noir-contracts/contracts/app/nft_contract/src/types/nft_note.nr b/noir-projects/noir-contracts/contracts/app/nft_contract/src/types/nft_note.nr index 13aacb0ec62a..bff02d8fbd37 100644 --- a/noir-projects/noir-contracts/contracts/app/nft_contract/src/types/nft_note.nr +++ b/noir-projects/noir-contracts/contracts/app/nft_contract/src/types/nft_note.nr @@ -206,11 +206,11 @@ pub struct PartialNFTNote { impl PartialNFTNote { /// Computes a partial note validity commitment. This commitment is stored in public storage when creating the - /// partial note in private and then is used to check that sender and note are valid upon partial note completion - /// in public. - pub fn compute_validity_commitment(self, sender: AztecAddress) -> Field { + /// partial note in private and then is used to check that completer and note are valid upon partial note completion + /// in public (completer is the entity that can complete the partial note). + pub fn compute_validity_commitment(self, completer: AztecAddress) -> Field { poseidon2_hash_with_separator( - [self.commitment, sender.to_field()], + [self.commitment, completer.to_field()], GENERATOR_INDEX__PARTIAL_NOTE_VALIDITY_COMMITMENT, ) } diff --git a/noir-projects/noir-contracts/contracts/app/simple_token_contract/src/main.nr b/noir-projects/noir-contracts/contracts/app/simple_token_contract/src/main.nr index b9c67cbd7160..cc1b23afcc16 100644 --- a/noir-projects/noir-contracts/contracts/app/simple_token_contract/src/main.nr +++ b/noir-projects/noir-contracts/contracts/app/simple_token_contract/src/main.nr @@ -29,7 +29,6 @@ pub contract SimpleToken { }; use dep::uint_note::uint_note::{PartialUintNote, UintNote}; - use aztec::protocol_types::traits::ToField; use dep::authwit::auth::{ assert_current_call_valid_authwit, assert_current_call_valid_authwit_public, @@ -232,8 +231,10 @@ pub contract SimpleToken { from, ); + let validity_commitment = partial_note.compute_validity_commitment(context.msg_sender()); + SimpleToken::at(context.this_address()) - ._store_balances_set_partial_note(partial_note) + ._set_uint_partial_note_validity(validity_commitment) .enqueue(context); partial_note @@ -241,32 +242,47 @@ pub contract SimpleToken { #[public] fn finalize_transfer_to_private(amount: u128, partial_note: PartialUintNote) { - let from = context.msg_sender(); - _finalize_transfer_to_private(from, amount, partial_note, &mut context, storage); + let from_and_completer = context.msg_sender(); + _finalize_transfer_to_private( + from_and_completer, + amount, + partial_note, + &mut context, + storage, + ); } #[public] #[internal] fn _finalize_transfer_to_private_unsafe( - from: AztecAddress, + from_and_completer: AztecAddress, amount: u128, partial_note: PartialUintNote, ) { - _finalize_transfer_to_private(from, amount, partial_note, &mut context, storage); + _finalize_transfer_to_private( + from_and_completer, + amount, + partial_note, + &mut context, + storage, + ); } #[contract_library_method] fn _finalize_transfer_to_private( - from: AztecAddress, + from_and_completer: AztecAddress, amount: u128, partial_note: PartialUintNote, context: &mut PublicContext, storage: Storage<&mut PublicContext>, ) { - let from_balance = storage.public_balances.at(from).read().sub(amount); - storage.public_balances.at(from).write(from_balance); + let from_balance = storage.public_balances.at(from_and_completer).read().sub(amount); + storage.public_balances.at(from_and_completer).write(from_balance); - assert(context.storage_read(partial_note.commitment()), "Invalid partial note"); + assert( + context.storage_read(partial_note.compute_validity_commitment(from_and_completer)), + "Invalid partial note", + ); partial_note.complete(amount, context); } @@ -274,22 +290,41 @@ pub contract SimpleToken { fn mint_privately(from: AztecAddress, to: AztecAddress, amount: u128) { let token = SimpleToken::at(context.this_address()); let partial_note = _prepare_private_balance_increase(from, to, &mut context, storage); - token._finalize_mint_to_private_unsafe(amount, partial_note).enqueue(&mut context); + token._finalize_mint_to_private_unsafe(context.msg_sender(), amount, partial_note).enqueue( + &mut context, + ); } #[public] fn finalize_mint_to_private(amount: u128, partial_note: PartialUintNote) { - _finalize_mint_to_private(amount, partial_note, &mut context, storage); + _finalize_mint_to_private( + context.msg_sender(), + amount, + partial_note, + &mut context, + storage, + ); } #[public] #[internal] - fn _finalize_mint_to_private_unsafe(amount: u128, partial_note: PartialUintNote) { - _finalize_mint_to_private(amount, partial_note, &mut context, storage); + fn _finalize_mint_to_private_unsafe( + minter_and_completer: AztecAddress, + amount: u128, + partial_note: PartialUintNote, + ) { + _finalize_mint_to_private( + minter_and_completer, + amount, + partial_note, + &mut context, + storage, + ); } #[contract_library_method] fn _finalize_mint_to_private( + completer: AztecAddress, amount: u128, partial_note: PartialUintNote, context: &mut PublicContext, @@ -298,14 +333,17 @@ pub contract SimpleToken { let supply = storage.total_supply.read().add(amount); storage.total_supply.write(supply); - assert(context.storage_read(partial_note.commitment()), "Invalid partial note"); + assert( + context.storage_read(partial_note.compute_validity_commitment(completer)), + "Invalid partial note", + ); partial_note.complete(amount, context); } #[public] #[internal] - fn _store_balances_set_partial_note(partial_note: PartialUintNote) { - context.storage_write(partial_note.commitment(), true); + fn _set_uint_partial_note_validity(validity_commitment: Field) { + context.storage_write(validity_commitment, true); } #[public] diff --git a/noir-projects/noir-contracts/contracts/app/token_contract/src/main.nr b/noir-projects/noir-contracts/contracts/app/token_contract/src/main.nr index da7c04a3107f..05385fa9b0dd 100644 --- a/noir-projects/noir-contracts/contracts/app/token_contract/src/main.nr +++ b/noir-projects/noir-contracts/contracts/app/token_contract/src/main.nr @@ -449,59 +449,84 @@ pub contract Token { // We can't simply return the partial note because we won't be able to later on verify that it was created // correctly (e.g. that the storage slot corresponds to the owner, and that we're using the balance set and not - // another state variable) once this information is hidden in the partial note commitment. We therefore store - // the partial note in our own public storage, so that we can later check that we're only completing correctly - // created partial notes. - Token::at(context.this_address())._store_balances_set_partial_note(partial_note).enqueue( - context, - ); + // another state variable) once this information is hidden in the partial note commitment. We also want to + // verify that only the caller of this function can complete the partial note. + // + // We achieve the above by storing a flag in public storage under a slot index equal to its + // `validity_commitment`. This commitment contains all the necessary information to verify the completer and + // to validate the partial note. + let validity_commitment = partial_note.compute_validity_commitment(context.msg_sender()); + + Token::at(context.this_address()) + ._set_uint_partial_note_validity(validity_commitment) + .enqueue(context); partial_note } // docs:start:finalize_transfer_to_private - /// Finalizes a transfer of token `amount` from public balance of `from` to a private balance of `to`. - /// The transfer must be prepared by calling `prepare_private_balance_increase` first and the resulting - /// `partial_note` must be passed as an argument to this function. + /// Finalizes a transfer of token `amount` from public balance of `msg_sender` to a private balance of `to`. + /// The transfer must be prepared by calling `prepare_private_balance_increase` from `msg_sender` account and + /// the resulting `partial_note` must be passed as an argument to this function. #[public] fn finalize_transfer_to_private(amount: u128, partial_note: PartialUintNote) { - let from = context.msg_sender(); - _finalize_transfer_to_private(from, amount, partial_note, &mut context, storage); + // Completer is the entity that can complete the partial note. In this case, it's the same as the account + // `from` from whose balance we're subtracting the `amount`. + let from_and_completer = context.msg_sender(); + _finalize_transfer_to_private( + from_and_completer, + amount, + partial_note, + &mut context, + storage, + ); } // docs:end:finalize_transfer_to_private // docs:start:finalize_transfer_to_private_unsafe /// This is a wrapper around `_finalize_transfer_to_private` placed here so that a call - /// to `_finalize_transfer_to_private` can be enqueued. Called unsafe as it does not check `from` (this has to be - /// done in the calling function). + /// to `_finalize_transfer_to_private` can be enqueued. Called unsafe as it does not check `from_and_completer` + /// (this has to be done in the calling function). #[public] #[internal] fn _finalize_transfer_to_private_unsafe( - from: AztecAddress, + from_and_completer: AztecAddress, amount: u128, partial_note: PartialUintNote, ) { - _finalize_transfer_to_private(from, amount, partial_note, &mut context, storage); + _finalize_transfer_to_private( + from_and_completer, + amount, + partial_note, + &mut context, + storage, + ); } // docs:end:finalize_transfer_to_private_unsafe + // In all the flows in this contract, `from` (the account from which we're subtracting the `amount`) and + // `completer` (the entity that can complete the partial note) are the same so we represent them with a single + // argument. #[contract_library_method] fn _finalize_transfer_to_private( - from: AztecAddress, + from_and_completer: AztecAddress, amount: u128, partial_note: PartialUintNote, context: &mut PublicContext, storage: Storage<&mut PublicContext>, ) { - // First we subtract the `amount` from the public balance of `from` - let from_balance = storage.public_balances.at(from).read().sub(amount); - storage.public_balances.at(from).write(from_balance); - - // We verify that the partial note we're completing is valid (i.e. it uses the correct state variable's storage - // slot, and it is internally consistent). We *could* clear the storage since each partial note should only be - // used once, but since the AVM offers no gas refunds for doing so this would just make the transaction be more - // expensive. - assert(context.storage_read(partial_note.commitment()), "Invalid partial note"); + // First we subtract the `amount` from the public balance of `from_and_completer` + let from_balance = storage.public_balances.at(from_and_completer).read().sub(amount); + storage.public_balances.at(from_and_completer).write(from_balance); + + // We verify that the partial note we're completing is valid (i.e. completer is correct, it uses the correct + // state variable's storage slot, and it is internally consistent). We *could* clear the storage since each + // partial note should only be used once, but since the AVM offers no gas refunds for doing so this would + // just make the transaction be more expensive. + assert( + context.storage_read(partial_note.compute_validity_commitment(from_and_completer)), + "Invalid partial note", + ); partial_note.complete(amount, context); } @@ -510,6 +535,7 @@ pub contract Token { /// in the enqueued call). #[private] fn mint_to_private( + // TODO(benesjan): This allows minter to set arbitrary `from`. That seems undesirable. Will nuke it in a followup PR. from: AztecAddress, // sender of the tag to: AztecAddress, amount: u128, @@ -519,9 +545,9 @@ pub contract Token { // We prepare the partial note to which we'll "send" the minted amount. let partial_note = _prepare_private_balance_increase(from, to, &mut context, storage); - // At last we finalize the mint. Usage of the `unsafe` method here is safe because we set the `from` - // function argument to a message sender, guaranteeing that only a message sender with minter permissions - // can successfully execute the function. + // At last we finalize the mint. Usage of the `unsafe` method here is safe because we set + // the `minter_and_completer` function argument to a message sender, guaranteeing that only a message sender + // with minter permissions can successfully execute the function. token._finalize_mint_to_private_unsafe(context.msg_sender(), amount, partial_note).enqueue( &mut context, ); @@ -538,28 +564,47 @@ pub contract Token { /// (e.g. used during token bridging, in AMM liquidity token etc.). #[public] fn finalize_mint_to_private(amount: u128, partial_note: PartialUintNote) { - assert(storage.minters.at(context.msg_sender()).read(), "caller is not minter"); + // Completer is the entity that can complete the partial note. In this case, it's the same as the minter + // account. + let minter_and_completer = context.msg_sender(); + assert(storage.minters.at(minter_and_completer).read(), "caller is not minter"); - _finalize_mint_to_private(amount, partial_note, &mut context, storage); + _finalize_mint_to_private( + minter_and_completer, + amount, + partial_note, + &mut context, + storage, + ); } // docs:end:finalize_mint_to_private // docs:start:finalize_mint_to_private_unsafe + /// This is a wrapper around `_finalize_mint_to_private` placed here so that a call + /// to `_finalize_mint_to_private` can be enqueued. Called unsafe as it does not check `minter_and_completer` (this + /// has to be done in the calling function). #[public] #[internal] fn _finalize_mint_to_private_unsafe( - from: AztecAddress, + minter_and_completer: AztecAddress, amount: u128, partial_note: PartialUintNote, ) { // We check the minter permissions as it was not done in `mint_to_private` function. - assert(storage.minters.at(from).read(), "caller is not minter"); - _finalize_mint_to_private(amount, partial_note, &mut context, storage); + assert(storage.minters.at(minter_and_completer).read(), "caller is not minter"); + _finalize_mint_to_private( + minter_and_completer, + amount, + partial_note, + &mut context, + storage, + ); } // docs:end:finalize_mint_to_private_unsafe #[contract_library_method] fn _finalize_mint_to_private( + completer: AztecAddress, // entity that can complete the partial note amount: u128, partial_note: PartialUintNote, context: &mut PublicContext, @@ -573,18 +618,21 @@ pub contract Token { // slot, and it is internally consistent). We *could* clear the storage since each partial note should only be // used once, but since the AVM offers no gas refunds for doing so this would just make the transaction be more // expensive. - assert(context.storage_read(partial_note.commitment()), "Invalid partial note"); + assert( + context.storage_read(partial_note.compute_validity_commitment(completer)), + "Invalid partial note", + ); partial_note.complete(amount, context); } #[public] #[internal] - fn _store_balances_set_partial_note(partial_note: PartialUintNote) { - // We store the partial note in a slot equal to its commitment. This is safe because the commitment is computed - // using a generator different from the one used to compute storage slots, so there can be no collisions. - // We could consider storing all pending partial notes in e.g. some array, but ultimately this is pointless: all - // we need to verify is that the note is valid. - context.storage_write(partial_note.commitment(), true); + fn _set_uint_partial_note_validity(validity_commitment: Field) { + // We store the partial note validity flag in a slot equal to its validity commitment. This is safe because + // the commitment is computed using a generator different from the one used to compute storage slots, so there + // can be no collisions. We could consider storing all pending partial notes in e.g. some array, but ultimately + // this is pointless: all we need to verify is that the note is valid. + context.storage_write(validity_commitment, true); } /// Internal /// From bfbb4bac0deadf4fb28de32dba95c24c04883c5f Mon Sep 17 00:00:00 2001 From: benesjan Date: Tue, 20 May 2025 14:42:21 +0000 Subject: [PATCH 11/14] fix --- .../apps_tests/amm_test.ts | 24 +++++++++---------- 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/yarn-project/simulator/src/public/public_tx_simulator/apps_tests/amm_test.ts b/yarn-project/simulator/src/public/public_tx_simulator/apps_tests/amm_test.ts index fbdd1d66ae93..6a629f155698 100644 --- a/yarn-project/simulator/src/public/public_tx_simulator/apps_tests/amm_test.ts +++ b/yarn-project/simulator/src/public/public_tx_simulator/apps_tests/amm_test.ts @@ -150,10 +150,10 @@ async function addLiquidity( args: [/*to=*/ amm.address, /*amount=*/ amount0Max], address: token0.address, }, - // token0.prepare_private_balance_increase enqueues a call to _store_balances_set_partial_note + // token0.prepare_private_balance_increase enqueues a call to _set_uint_partial_note_validity { sender: token0.address, // INTERNAL FUNCTION! Sender must be 'this'. - fnName: '_store_balances_set_partial_note', + fnName: '_set_uint_partial_note_validity', args: [refundToken0PartialNote], address: token0.address, }, @@ -164,17 +164,17 @@ async function addLiquidity( args: [/*to=*/ amm.address, /*amount=*/ amount1Max], address: token1.address, }, - // token1.prepare_private_balance_increase enqueues a call to _store_balances_set_partial_note + // token1.prepare_private_balance_increase enqueues a call to _set_uint_partial_note_validity { sender: token1.address, // INTERNAL FUNCTION! Sender must be 'this'. - fnName: '_store_balances_set_partial_note', + fnName: '_set_uint_partial_note_validity', args: [refundToken1PartialNote], address: token1.address, }, - // liquidityToken.prepare_private_balance_increase enqueues a call to _store_balances_set_partial_note + // liquidityToken.prepare_private_balance_increase enqueues a call to _set_uint_partial_note_validity { sender: liquidityToken.address, // INTERNAL FUNCTION! Sender must be 'this'. - fnName: '_store_balances_set_partial_note', + fnName: '_set_uint_partial_note_validity', args: [liquidityPartialNote], address: liquidityToken.address, }, @@ -229,10 +229,10 @@ async function swapExactTokensForTokens( args: [/*to=*/ amm.address, /*amount=*/ amountIn], address: tokenIn.address, }, - // tokenOut.prepare_private_balance_increase enqueues a call to _store_balances_set_partial_note + // tokenOut.prepare_private_balance_increase enqueues a call to _set_uint_partial_note_validity { sender: tokenOut.address, // INTERNAL FUNCTION! Sender must be 'this'. - fnName: '_store_balances_set_partial_note', + fnName: '_set_uint_partial_note_validity', args: [tokenOutPartialNote], address: tokenOut.address, }, @@ -278,17 +278,17 @@ async function removeLiquidity( args: [/*to=*/ amm.address, /*amount=*/ liquidity], address: liquidityToken.address, }, - // token0.prepare_private_balance_increase enqueues a call to _store_balances_set_partial_note + // token0.prepare_private_balance_increase enqueues a call to _set_uint_partial_note_validity { sender: token0.address, // INTERNAL FUNCTION! Sender must be 'this'. - fnName: '_store_balances_set_partial_note', + fnName: '_set_uint_partial_note_validity', args: [token0PartialNote], address: token0.address, }, - // token1.prepare_private_balance_increase enqueues a call to _store_balances_set_partial_note + // token1.prepare_private_balance_increase enqueues a call to _set_uint_partial_note_validity { sender: token1.address, // INTERNAL FUNCTION! Sender must be 'this'. - fnName: '_store_balances_set_partial_note', + fnName: '_set_uint_partial_note_validity', args: [token1PartialNote], address: token1.address, }, From 79c1482f98c30f1a5436a0ebdac7b4cc97733f83 Mon Sep 17 00:00:00 2001 From: benesjan Date: Tue, 20 May 2025 15:10:24 +0000 Subject: [PATCH 12/14] fixes --- .../apps_tests/amm_test.ts | 47 +++++++++++++++---- 1 file changed, 39 insertions(+), 8 deletions(-) diff --git a/yarn-project/simulator/src/public/public_tx_simulator/apps_tests/amm_test.ts b/yarn-project/simulator/src/public/public_tx_simulator/apps_tests/amm_test.ts index 6a629f155698..de97d386d36e 100644 --- a/yarn-project/simulator/src/public/public_tx_simulator/apps_tests/amm_test.ts +++ b/yarn-project/simulator/src/public/public_tx_simulator/apps_tests/amm_test.ts @@ -1,3 +1,5 @@ +import { GeneratorIndex } from '@aztec/constants'; +import { poseidon2HashWithSeparator } from '@aztec/foundation/crypto'; import { Fr } from '@aztec/foundation/fields'; import type { Logger } from '@aztec/foundation/log'; import { AMMContractArtifact } from '@aztec/noir-contracts.js/AMM'; @@ -137,7 +139,18 @@ async function addLiquidity( const liquidityPartialNote = { commitment: new Fr(99), }; - + const refundToken0PartialNoteValidityCommitment = await computePartialNoteValidityCommitment( + refundToken0PartialNote, + amm.address, + ); + const refundToken1PartialNoteValidityCommitment = await computePartialNoteValidityCommitment( + refundToken1PartialNote, + amm.address, + ); + const liquidityPartialNoteValidityCommitment = await computePartialNoteValidityCommitment( + liquidityPartialNote, + amm.address, + ); return await tester.simulateTxWithLabel( /*txLabel=*/ 'AMM/add_liquidity', /*sender=*/ sender, @@ -154,7 +167,7 @@ async function addLiquidity( { sender: token0.address, // INTERNAL FUNCTION! Sender must be 'this'. fnName: '_set_uint_partial_note_validity', - args: [refundToken0PartialNote], + args: [refundToken0PartialNoteValidityCommitment], address: token0.address, }, // token1.transfer_to_public enqueues a call to _increase_public_balance @@ -168,14 +181,14 @@ async function addLiquidity( { sender: token1.address, // INTERNAL FUNCTION! Sender must be 'this'. fnName: '_set_uint_partial_note_validity', - args: [refundToken1PartialNote], + args: [refundToken1PartialNoteValidityCommitment], address: token1.address, }, // liquidityToken.prepare_private_balance_increase enqueues a call to _set_uint_partial_note_validity { sender: liquidityToken.address, // INTERNAL FUNCTION! Sender must be 'this'. fnName: '_set_uint_partial_note_validity', - args: [liquidityPartialNote], + args: [liquidityPartialNoteValidityCommitment], address: liquidityToken.address, }, // amm.add_liquidity enqueues a call to _add_liquidity @@ -216,6 +229,10 @@ async function swapExactTokensForTokens( const tokenOutPartialNote = { commitment: new Fr(66), }; + const tokenOutPartialNoteValidityCommitment = await computePartialNoteValidityCommitment( + tokenOutPartialNote, + amm.address, + ); return await tester.simulateTxWithLabel( /*txLabel=*/ 'AMM/swap_exact_tokens_for_tokens', @@ -233,7 +250,7 @@ async function swapExactTokensForTokens( { sender: tokenOut.address, // INTERNAL FUNCTION! Sender must be 'this'. fnName: '_set_uint_partial_note_validity', - args: [tokenOutPartialNote], + args: [tokenOutPartialNoteValidityCommitment], address: tokenOut.address, }, @@ -265,7 +282,14 @@ async function removeLiquidity( const token1PartialNote = { commitment: new Fr(222), }; - + const token0PartialNoteValidityCommitment = await computePartialNoteValidityCommitment( + token0PartialNote, + amm.address, + ); + const token1PartialNoteValidityCommitment = await computePartialNoteValidityCommitment( + token1PartialNote, + amm.address, + ); return await tester.simulateTxWithLabel( /*txLabel=*/ 'AMM/remove_liquidity', /*sender=*/ sender, @@ -282,14 +306,14 @@ async function removeLiquidity( { sender: token0.address, // INTERNAL FUNCTION! Sender must be 'this'. fnName: '_set_uint_partial_note_validity', - args: [token0PartialNote], + args: [token0PartialNoteValidityCommitment], address: token0.address, }, // token1.prepare_private_balance_increase enqueues a call to _set_uint_partial_note_validity { sender: token1.address, // INTERNAL FUNCTION! Sender must be 'this'. fnName: '_set_uint_partial_note_validity', - args: [token1PartialNote], + args: [token1PartialNoteValidityCommitment], address: token1.address, }, // amm.remove_liquidity enqueues a call to _remove_liquidity @@ -314,3 +338,10 @@ async function removeLiquidity( ], ); } + +async function computePartialNoteValidityCommitment(partialNote: { commitment: Fr }, completer: AztecAddress) { + return await poseidon2HashWithSeparator( + [partialNote.commitment, completer], + GeneratorIndex.PARTIAL_NOTE_VALIDITY_COMMITMENT, + ); +} From 0f57d790cbf77c5c63fcb149f888ae5acc939515 Mon Sep 17 00:00:00 2001 From: benesjan Date: Tue, 20 May 2025 15:41:44 +0000 Subject: [PATCH 13/14] testing completer --- .../contracts/app/nft_contract/src/main.nr | 2 +- .../src/test/transfer_to_private.nr | 24 +++++++++++++++- .../app/simple_token_contract/src/main.nr | 4 +-- .../contracts/app/token_contract/src/main.nr | 4 +-- .../src/test/transfer_to_private.nr | 28 ++++++++++++++++++- 5 files changed, 55 insertions(+), 7 deletions(-) diff --git a/noir-projects/noir-contracts/contracts/app/nft_contract/src/main.nr b/noir-projects/noir-contracts/contracts/app/nft_contract/src/main.nr index 92a1c95be527..a3894019040f 100644 --- a/noir-projects/noir-contracts/contracts/app/nft_contract/src/main.nr +++ b/noir-projects/noir-contracts/contracts/app/nft_contract/src/main.nr @@ -299,7 +299,7 @@ pub contract NFT { // make the transaction be more expensive. assert( context.storage_read(partial_note.compute_validity_commitment(from_and_completer)), - "Invalid partial note", + "Invalid partial note or completer", ); partial_note.complete(token_id, context); } diff --git a/noir-projects/noir-contracts/contracts/app/nft_contract/src/test/transfer_to_private.nr b/noir-projects/noir-contracts/contracts/app/nft_contract/src/test/transfer_to_private.nr index 6a23909e114b..ff4a54de1572 100644 --- a/noir-projects/noir-contracts/contracts/app/nft_contract/src/test/transfer_to_private.nr +++ b/noir-projects/noir-contracts/contracts/app/nft_contract/src/test/transfer_to_private.nr @@ -47,7 +47,7 @@ unconstrained fn transfer_to_private_external_orchestration() { utils::assert_owns_public_nft(env, nft_contract_address, AztecAddress::zero(), token_id); } -#[test(should_fail_with = "Invalid partial note")] +#[test(should_fail_with = "Invalid partial note or completer")] unconstrained fn transfer_to_private_transfer_not_prepared() { // Setup without account contracts. We are not using authwits here, so dummy accounts are enough let (env, nft_contract_address, _, _, token_id) = @@ -81,3 +81,25 @@ unconstrained fn transfer_to_private_failure_not_an_owner() { &mut env.public(), ); } + +#[test(should_fail_with = "Invalid partial note or completer")] +unconstrained fn incorrect_completer_cannot_complete_partial_note() { + // Setup without account contracts. We are not using authwits here, so dummy accounts are enough + let (env, nft_contract_address, incorrect_completer, recipient, token_id) = + utils::setup_and_mint(/* with_account_contracts */ false); + + // We set up the completer + let completer = env.create_account(3); + + // We prepare the partial note as a completer + env.impersonate(completer); + let partial_note = NFT::at(nft_contract_address) + .prepare_private_balance_increase(recipient) + .call(&mut env.private()); + + // Now we try to complete the partial note as the incorrect completer + env.impersonate(incorrect_completer); + NFT::at(nft_contract_address).finalize_transfer_to_private(token_id, partial_note).call( + &mut env.public(), + ); +} diff --git a/noir-projects/noir-contracts/contracts/app/simple_token_contract/src/main.nr b/noir-projects/noir-contracts/contracts/app/simple_token_contract/src/main.nr index cc1b23afcc16..72bf0c4ef06d 100644 --- a/noir-projects/noir-contracts/contracts/app/simple_token_contract/src/main.nr +++ b/noir-projects/noir-contracts/contracts/app/simple_token_contract/src/main.nr @@ -281,7 +281,7 @@ pub contract SimpleToken { assert( context.storage_read(partial_note.compute_validity_commitment(from_and_completer)), - "Invalid partial note", + "Invalid partial note or completer", ); partial_note.complete(amount, context); } @@ -335,7 +335,7 @@ pub contract SimpleToken { assert( context.storage_read(partial_note.compute_validity_commitment(completer)), - "Invalid partial note", + "Invalid partial note or completer", ); partial_note.complete(amount, context); } diff --git a/noir-projects/noir-contracts/contracts/app/token_contract/src/main.nr b/noir-projects/noir-contracts/contracts/app/token_contract/src/main.nr index 05385fa9b0dd..37665e92a6e7 100644 --- a/noir-projects/noir-contracts/contracts/app/token_contract/src/main.nr +++ b/noir-projects/noir-contracts/contracts/app/token_contract/src/main.nr @@ -525,7 +525,7 @@ pub contract Token { // just make the transaction be more expensive. assert( context.storage_read(partial_note.compute_validity_commitment(from_and_completer)), - "Invalid partial note", + "Invalid partial note or completer", ); partial_note.complete(amount, context); } @@ -620,7 +620,7 @@ pub contract Token { // expensive. assert( context.storage_read(partial_note.compute_validity_commitment(completer)), - "Invalid partial note", + "Invalid partial note or completer", ); partial_note.complete(amount, context); } diff --git a/noir-projects/noir-contracts/contracts/app/token_contract/src/test/transfer_to_private.nr b/noir-projects/noir-contracts/contracts/app/token_contract/src/test/transfer_to_private.nr index 94a7967dd1bc..fe40e0f49c82 100644 --- a/noir-projects/noir-contracts/contracts/app/token_contract/src/test/transfer_to_private.nr +++ b/noir-projects/noir-contracts/contracts/app/token_contract/src/test/transfer_to_private.nr @@ -42,7 +42,7 @@ unconstrained fn transfer_to_private_external_orchestration() { utils::check_private_balance(token_contract_address, recipient, amount); } -#[test(should_fail_with = "Invalid partial note")] +#[test(should_fail_with = "Invalid partial note or completer")] unconstrained fn transfer_to_private_transfer_not_prepared() { // Setup without account contracts. We are not using authwits here, so dummy accounts are enough let (env, token_contract_address, _, _, amount) = @@ -76,3 +76,29 @@ unconstrained fn transfer_to_private_failure_not_an_owner() { &mut env.public(), ); } + +#[test(should_fail_with = "Invalid partial note or completer")] +unconstrained fn incorrect_completer_cannot_complete_partial_note() { + // Setup without account contracts. We are not using authwits here, so dummy accounts are enough + let (env, token_contract_address, owner, recipient, amount) = + utils::setup_and_mint_to_public(/* with_account_contracts */ false); + + // We set up the incorrect completer + let incorrect_completer = env.create_account(3); + + // We mint `amount` to incorrect completer to avoid the test failing due to the token balance check + Token::at(token_contract_address).mint_to_public(incorrect_completer, amount).call( + &mut env.public(), + ); + + // We prepare the transfer + let partial_uint_note = Token::at(token_contract_address) + .prepare_private_balance_increase(recipient, owner) + .call(&mut env.private()); + + // We impersonate the incorrect completer and try to finalize the transfer + env.impersonate(incorrect_completer); + Token::at(token_contract_address).finalize_transfer_to_private(amount, partial_uint_note).call( + &mut env.public(), + ); +} From 98bb5ea8ae4170bf05dfc56b99872da21c8dc7de Mon Sep 17 00:00:00 2001 From: benesjan Date: Tue, 20 May 2025 15:52:13 +0000 Subject: [PATCH 14/14] cleanup --- .../contracts/app/nft_contract/src/main.nr | 4 ++-- .../src/test/transfer_to_private.nr | 4 +++- .../contracts/app/token_contract/src/main.nr | 4 ++-- .../src/test/transfer_to_private.nr | 21 +++++++++---------- 4 files changed, 17 insertions(+), 16 deletions(-) diff --git a/noir-projects/noir-contracts/contracts/app/nft_contract/src/main.nr b/noir-projects/noir-contracts/contracts/app/nft_contract/src/main.nr index a3894019040f..bd16eb5c29a0 100644 --- a/noir-projects/noir-contracts/contracts/app/nft_contract/src/main.nr +++ b/noir-projects/noir-contracts/contracts/app/nft_contract/src/main.nr @@ -210,8 +210,8 @@ pub contract NFT { // We can't simply return the partial note because we won't be able to later on verify that it was created // correctly (e.g. that the storage slot corresponds to the owner, and that we're using the `private_nfts` - // state variable and not another state variable) once this information is hidden in the partial note - // commitment. We also want to verify that only the caller of this function can complete the partial note. + // state variable and not a different one) once this information is hidden in the partial note commitment. + // We also want to verify that only the caller of this function can complete the partial note. // // We achieve the above by storing a flag in public storage under a slot index equal to its // `validity_commitment`. This commitment contains all the necessary information to verify the completer and diff --git a/noir-projects/noir-contracts/contracts/app/nft_contract/src/test/transfer_to_private.nr b/noir-projects/noir-contracts/contracts/app/nft_contract/src/test/transfer_to_private.nr index ff4a54de1572..f93449680fc2 100644 --- a/noir-projects/noir-contracts/contracts/app/nft_contract/src/test/transfer_to_private.nr +++ b/noir-projects/noir-contracts/contracts/app/nft_contract/src/test/transfer_to_private.nr @@ -84,7 +84,9 @@ unconstrained fn transfer_to_private_failure_not_an_owner() { #[test(should_fail_with = "Invalid partial note or completer")] unconstrained fn incorrect_completer_cannot_complete_partial_note() { - // Setup without account contracts. We are not using authwits here, so dummy accounts are enough + // Setup without account contracts. We are not using authwits here, so dummy accounts are enough. + // We mint to the incorrect completer to avoid the test failing due to the NFT owner check (we want to fail on + // the partial note validity check). let (env, nft_contract_address, incorrect_completer, recipient, token_id) = utils::setup_and_mint(/* with_account_contracts */ false); diff --git a/noir-projects/noir-contracts/contracts/app/token_contract/src/main.nr b/noir-projects/noir-contracts/contracts/app/token_contract/src/main.nr index 37665e92a6e7..8f19b1498fe2 100644 --- a/noir-projects/noir-contracts/contracts/app/token_contract/src/main.nr +++ b/noir-projects/noir-contracts/contracts/app/token_contract/src/main.nr @@ -449,8 +449,8 @@ pub contract Token { // We can't simply return the partial note because we won't be able to later on verify that it was created // correctly (e.g. that the storage slot corresponds to the owner, and that we're using the balance set and not - // another state variable) once this information is hidden in the partial note commitment. We also want to - // verify that only the caller of this function can complete the partial note. + // a different one) once this information is hidden in the partial note commitment. We also want to verify that + // only the caller of this function can complete the partial note. // // We achieve the above by storing a flag in public storage under a slot index equal to its // `validity_commitment`. This commitment contains all the necessary information to verify the completer and diff --git a/noir-projects/noir-contracts/contracts/app/token_contract/src/test/transfer_to_private.nr b/noir-projects/noir-contracts/contracts/app/token_contract/src/test/transfer_to_private.nr index fe40e0f49c82..68a6132afaf6 100644 --- a/noir-projects/noir-contracts/contracts/app/token_contract/src/test/transfer_to_private.nr +++ b/noir-projects/noir-contracts/contracts/app/token_contract/src/test/transfer_to_private.nr @@ -79,21 +79,20 @@ unconstrained fn transfer_to_private_failure_not_an_owner() { #[test(should_fail_with = "Invalid partial note or completer")] unconstrained fn incorrect_completer_cannot_complete_partial_note() { - // Setup without account contracts. We are not using authwits here, so dummy accounts are enough - let (env, token_contract_address, owner, recipient, amount) = + // Setup without account contracts. We are not using authwits here, so dummy accounts are enough. + // We mint to the incorrect completer to avoid the test failing due to the NFT owner check (we want to fail on + // the partial note validity check). + let (env, token_contract_address, incorrect_completer, recipient, amount) = utils::setup_and_mint_to_public(/* with_account_contracts */ false); - // We set up the incorrect completer - let incorrect_completer = env.create_account(3); + // We set up the completer + let completer = env.create_account(3); + let arbitrary_sender = env.create_account(4); - // We mint `amount` to incorrect completer to avoid the test failing due to the token balance check - Token::at(token_contract_address).mint_to_public(incorrect_completer, amount).call( - &mut env.public(), - ); - - // We prepare the transfer + // We prepare the partial note as a completer + env.impersonate(completer); let partial_uint_note = Token::at(token_contract_address) - .prepare_private_balance_increase(recipient, owner) + .prepare_private_balance_increase(recipient, arbitrary_sender) .call(&mut env.private()); // We impersonate the incorrect completer and try to finalize the transfer