Merge branch 'master' into sync-noir

* master:
  refactor: updated NFT flows (#9150)

noir-projects/aztec-nr/aztec/src/prelude.nr

@@ -1,6 +1,6 @@
 // docs:start:prelude
 pub use crate::{
-    context::{FunctionReturns, PackedReturns, PrivateContext},
+    context::{FunctionReturns, PackedReturns, PrivateContext, PublicContext},
     note::{
         note_getter_options::NoteGetterOptions,
         note_header::NoteHeader,

noir-projects/noir-contracts/contracts/nft_contract/src/main.nr

@@ -7,7 +7,6 @@ use dep::aztec::macros::aztec;
 // and private.
 #[aztec]
 contract NFT {
-    use crate::types::nft_note::NFTNote;
     use dep::authwit::auth::{
         assert_current_call_valid_authwit, assert_current_call_valid_authwit_public,
         compute_authwit_nullifier,
@@ -35,8 +34,6 @@ contract NFT {
     use dep::compressed_string::FieldCompressedString;
     use std::{embedded_curve_ops::EmbeddedCurvePoint, meta::derive};
 
-    global TRANSIENT_STORAGE_SLOT_PEDERSEN_INDEX = 3;
-
     // TODO(#8467): Rename this to Transfer - calling this NFTTransfer to avoid export conflict with the Transfer event
     // in the Token contract.
     #[event]
@@ -148,78 +145,117 @@ contract NFT {
         public_owners_storage.write(to);
     }
 
-    /// Prepares a transfer from public balance of `from` to a private balance of `to`. The transfer then needs to be
-    /// finalized by calling `finalize_transfer_to_private`. `transient_storage_slot_randomness` is passed
-    /// as an argument so that we can derive `transfer_preparer_storage_slot_commitment` off-chain and then pass it
-    /// as an argument to the followup call to `finalize_transfer_to_private`.
+    // Transfers token with `token_id` from public balance of message sender to a private balance of `to`.
     #[private]
-    fn prepare_transfer_to_private(
-        from: AztecAddress,
+    fn transfer_to_private(to: AztecAddress, token_id: Field) {
+        let from = context.msg_sender();
+
+        let nft = NFT::at(context.this_address());
+
+        // We prepare the transfer.
+        let hiding_point_slot = _prepare_transfer_to_private(to, &mut context, storage);
+
+        // At last we finalize the transfer. Usafe of the `unsafe` method here is safe because we set the `from`
+        // function argument to a message sender, guaranteeing that he can transfer only his own NFTs.
+        nft._finalize_transfer_to_private_unsafe(from, token_id, hiding_point_slot).enqueue(
+            &mut context,
+        );
+    }
+
+    /// Prepares a transfer to a private balance of `to`. The transfer then needs to be
+    /// finalized by calling `finalize_transfer_to_private`. Returns a hiding point slot.
+    #[private]
+    fn prepare_transfer_to_private(to: AztecAddress) -> Field {
+        _prepare_transfer_to_private(to, &mut context, storage)
+    }
+
+    /// This function exists separately from `prepare_transfer_to_private` solely as an optimization as it allows
+    /// us to have it inlined in the `transfer_to_private` function which results in one less kernel iteration.
+    ///
+    /// TODO(#9180): Consider adding macro support for functions callable both as an entrypoint and as an internal
+    /// function.
+    #[contract_library_method]
+    fn _prepare_transfer_to_private(
         to: AztecAddress,
-        transient_storage_slot_randomness: Field,
-    ) {
+        context: &mut PrivateContext,
+        storage: Storage<&mut PrivateContext>,
+    ) -> Field {
         let to_keys = get_public_keys(to);
         let to_npk_m_hash = to_keys.npk_m.hash();
         let to_note_slot = storage.private_nfts.at(to).storage_slot;
 
-        // We create a partial NFT note hiding point with unpopulated/zero token id for 'to'
+        // We create a setup payload with unpopulated/zero token id for 'to'
         // TODO(#7775): Manually fetching the randomness here is not great. If we decide to include randomness in all
         // notes we could just inject it in macros.
         let note_randomness = unsafe { random() };
         let note_setup_payload =
             NFTNote::setup_payload().new(to_npk_m_hash, note_randomness, to_note_slot);
 
         // We encrypt and emit the partial note log
-        encrypt_and_emit_partial_log(&mut context, note_setup_payload.log_plaintext, to_keys, to);
-
-        // We make the msg_sender/transfer_preparer part of the slot preimage to ensure he cannot interfere with
-        // non-sender's slots
-        let transfer_preparer_storage_slot_commitment: Field = pedersen_hash(
-            [context.msg_sender().to_field(), transient_storage_slot_randomness],
-            TRANSIENT_STORAGE_SLOT_PEDERSEN_INDEX,
-        );
-        // Then we hash the transfer preparer storage slot commitment with `from` and use that as the final slot
-        // --> by hashing it with a `from` we ensure that `from` cannot interfere with slots not assigned to him.
-        let slot: Field = pedersen_hash(
-            [from.to_field(), transfer_preparer_storage_slot_commitment],
-            TRANSIENT_STORAGE_SLOT_PEDERSEN_INDEX,
-        );
-
+        encrypt_and_emit_partial_log(context, note_setup_payload.log_plaintext, to_keys, to);
+
+        // Using the x-coordinate as a hiding point slot is safe against someone else interfering with it because
+        // we have a guarantee that the public functions of the transaction are executed right after the private ones
+        // and for this reason the protocol guarantees that nobody can front-run us in consuming the hiding point.
+        // This guarantee would break if `finalize_transfer_to_private` was not called in the same transaction. This
+        // however is not the flow we are currently concerned with. To support the multi-transaction flow we could
+        // introduce a `from` function argument, hash the x-coordinate with it and then repeat the hashing in
+        // `finalize_transfer_to_private`.
+        //
+        // We can also be sure that the `hiding_point_slot` will not overwrite any other value in the storage because
+        // in our state variables we derive slots using a different hash function from multi scalar multiplication
+        // (MSM).
+        let hiding_point_slot = note_setup_payload.hiding_point.x;
+
+        // We don't need to perform a check that the value overwritten by `_store_point_in_transient_storage_unsafe`
+        // is zero because the slot is the x-coordinate of the hiding point and hence we could only overwrite
+        // the value in the slot with the same value. This makes usage of the `unsafe` method safe.
         NFT::at(context.this_address())
-            ._store_point_in_transient_storage(note_setup_payload.hiding_point, slot)
-            .enqueue(&mut context);
+            ._store_point_in_transient_storage_unsafe(
+                hiding_point_slot,
+                note_setup_payload.hiding_point,
+            )
+            .enqueue(context);
+
+        hiding_point_slot
     }
 
     #[public]
     #[internal]
-    fn _store_point_in_transient_storage(point: Point, slot: Field) {
-        // We don't perform check for the overwritten value to be non-zero because the slots are siloed to `to`
-        // and hence `to` can interfere only with his own execution.
+    fn _store_point_in_transient_storage_unsafe(slot: Field, point: Point) {
         context.storage_write(slot, point);
     }
 
     /// 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_transfer_to_private` first.
-    /// The `transfer_preparer_storage_slot_commitment` has to be computed off-chain the same way as was done
-    /// in the preparation call.
+    /// The transfer must be prepared by calling `prepare_transfer_to_private` first and the resulting
+    /// `hiding_point_slot` must be passed as an argument to this function.
     #[public]
-    fn finalize_transfer_to_private(
+    fn finalize_transfer_to_private(token_id: Field, hiding_point_slot: Field) {
+        let from = context.msg_sender();
+        _finalize_transfer_to_private(from, token_id, hiding_point_slot, &mut context, storage);
+    }
+
+    #[public]
+    #[internal]
+    fn _finalize_transfer_to_private_unsafe(
+        from: AztecAddress,
         token_id: Field,
-        transfer_preparer_storage_slot_commitment: Field,
+        hiding_point_slot: Field,
+    ) {
+        _finalize_transfer_to_private(from, token_id, hiding_point_slot, &mut context, storage);
+    }
+
+    #[contract_library_method]
+    fn _finalize_transfer_to_private(
+        from: AztecAddress,
+        token_id: Field,
+        hiding_point_slot: Field,
+        context: &mut PublicContext,
+        storage: Storage<&mut PublicContext>,
     ) {
-        // We don't need to support authwit here because `prepare_transfer_to_private` allows us to set arbitrary
-        // `from` and `from` will always be the msg sender here.
-        let from = context.msg_sender();
         let public_owners_storage = storage.public_owners.at(token_id);
         assert(public_owners_storage.read().eq(from), "invalid NFT owner");
 
-        // Derive the slot from the transfer preparer storage slot commitment and the `from` address (declared
-        // as `from` in this function)
-        let hiding_point_slot = pedersen_hash(
-            [from.to_field(), transfer_preparer_storage_slot_commitment],
-            TRANSIENT_STORAGE_SLOT_PEDERSEN_INDEX,
-        );
-
         // Read the hiding point from "transient" storage and check it's not empty to ensure the transfer was prepared
         let hiding_point: Point = context.storage_read(hiding_point_slot);
         assert(!is_empty(hiding_point), "transfer not prepared");
@@ -333,3 +369,4 @@ contract NFT {
         (owned_nft_ids, page_limit_reached)
     }
 }
+

noir-projects/noir-contracts/contracts/nft_contract/src/test/transfer_to_private.nr

@@ -1,16 +1,19 @@
 use crate::{NFT, test::utils, types::nft_note::NFTNote};
 use dep::aztec::{
-    hash::pedersen_hash,
     keys::getters::get_public_keys,
     oracle::random::random,
     prelude::{AztecAddress, NoteHeader},
     protocol_types::storage::map::derive_storage_slot_in_map,
 };
 use std::test::OracleMock;
 
+/// Internal orchestration means that the calls to `prepare_transfer_to_private`
+/// and `finalize_transfer_to_private` are done by the NFT contract itself.
+/// In this test's case this is done by the `NFT::transfer_to_private(...)` function called
+/// in `utils::setup_mint_and_transfer_to_private`.
 #[test]
-unconstrained fn transfer_to_private_to_self() {
-    // The transfer to private to self is done in `utils::setup_mint_and_transfer_to_private` and for this reason
+unconstrained fn transfer_to_private_internal_orchestration() {
+    // The transfer to private is done in `utils::setup_mint_and_transfer_to_private` and for this reason
     // in this test we just call it and check the outcome.
     // Setup without account contracts. We are not using authwits here, so dummy accounts are enough
     let (env, nft_contract_address, user, _, token_id) =
@@ -23,32 +26,28 @@ unconstrained fn transfer_to_private_to_self() {
     utils::assert_owns_public_nft(env, nft_contract_address, AztecAddress::zero(), token_id);
 }
 
+/// External orchestration means that the calls to prepare and finalize are not done by the NFT contract. This flow
+/// will typically be used by a DEX.
 #[test]
-unconstrained fn transfer_to_private_to_a_different_account() {
+unconstrained fn transfer_to_private_external_orchestration() {
     // Setup without account contracts. We are not using authwits here, so dummy accounts are enough
-    let (env, nft_contract_address, sender, recipient, token_id) =
+    let (env, nft_contract_address, _, recipient, token_id) =
         utils::setup_and_mint( /* with_account_contracts */ false);
 
     let note_randomness = random();
-    let transient_storage_slot_randomness = random();
-    // Sender will be the msg_sender/transfer_preparer in prepare_transfer_to_private
-    let transfer_preparer_storage_slot_commitment = pedersen_hash(
-        [sender.to_field(), transient_storage_slot_randomness],
-        NFT::TRANSIENT_STORAGE_SLOT_PEDERSEN_INDEX,
-    );
 
     // We mock the Oracle to return the note randomness such that later on we can manually add the note
     let _ = OracleMock::mock("getRandomField").returns(note_randomness);
 
     // We prepare the transfer
-    NFT::at(nft_contract_address)
-        .prepare_transfer_to_private(sender, recipient, transient_storage_slot_randomness)
+    let hiding_point_slot: Field = NFT::at(nft_contract_address)
+        .prepare_transfer_to_private(recipient)
         .call(&mut env.private());
 
-    // Finalize the transfer of the NFT
-    NFT::at(nft_contract_address)
-        .finalize_transfer_to_private(token_id, transfer_preparer_storage_slot_commitment)
-        .call(&mut env.public());
+    // Finalize the transfer of the NFT (message sender owns the NFT in public)
+    NFT::at(nft_contract_address).finalize_transfer_to_private(token_id, hiding_point_slot).call(
+        &mut env.public(),
+    );
 
     // TODO(#8771): We need to manually add the note because in the partial notes flow `notify_created_note_oracle`
     // is not called and we don't have a `NoteProcessor` in TXE.
@@ -75,48 +74,18 @@ unconstrained fn transfer_to_private_to_a_different_account() {
 }
 
 #[test(should_fail_with = "transfer not prepared")]
-unconstrained fn transfer_to_private_to_self_transfer_not_prepared() {
+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) =
         utils::setup_and_mint( /* with_account_contracts */ false);
 
-    // Transfer was not prepared so we can use random value for the commitment
-    let transfer_preparer_storage_slot_commitment = random();
+    // Transfer was not prepared so we can use random value for the hiding point slot
+    let hiding_point_slot = random();
 
     // Try finalizing the transfer without preparing it
-    NFT::at(nft_contract_address)
-        .finalize_transfer_to_private(token_id, transfer_preparer_storage_slot_commitment)
-        .call(&mut env.public())
-}
-
-#[test(should_fail_with = "transfer not prepared")]
-unconstrained fn transfer_to_private_finalizing_from_incorrect_sender() {
-    // Setup without account contracts. We are not using authwits here, so dummy accounts are enough
-    let (env, nft_contract_address, incorrect_sender, recipient, token_id) =
-        utils::setup_and_mint( /* with_account_contracts */ false);
-
-    let correct_sender = AztecAddress::from_field(9);
-
-    let transient_storage_slot_randomness = random();
-    // Sender will be the msg_sender/transfer_preparer in prepare_transfer_to_private
-    let transfer_preparer_storage_slot_commitment = pedersen_hash(
-        [correct_sender.to_field(), transient_storage_slot_randomness],
-        NFT::TRANSIENT_STORAGE_SLOT_PEDERSEN_INDEX,
+    NFT::at(nft_contract_address).finalize_transfer_to_private(token_id, hiding_point_slot).call(
+        &mut env.public(),
     );
-
-    // We prepare the transfer
-    NFT::at(nft_contract_address)
-        .prepare_transfer_to_private(correct_sender, recipient, transient_storage_slot_randomness)
-        .call(&mut env.private());
-
-    // We impersonate incorrect sender and try to finalize the transfer of the NFT. The incorrect sender owns the NFT
-    // but tries to consume a prepared transfer not belonging to him. For this reason the test should fail with
-    // "transfer not prepared".
-    env.impersonate(incorrect_sender);
-
-    NFT::at(nft_contract_address)
-        .finalize_transfer_to_private(token_id, transfer_preparer_storage_slot_commitment)
-        .call(&mut env.public());
 }
 
 #[test(should_fail_with = "invalid NFT owner")]
@@ -125,13 +94,16 @@ unconstrained fn transfer_to_private_failure_not_an_owner() {
     let (env, nft_contract_address, _, not_owner, token_id) =
         utils::setup_and_mint( /* with_account_contracts */ false);
 
-    // We set random value for the commitment as the NFT owner check is before we use the value
-    let transfer_preparer_storage_slot_commitment = random();
+    // (For this specific test we could set a random value for the commitment and not do the call to `prepare...`
+    // as the NFT owner check is before we use the value but that would made the test less robust against changes
+    // in the contract.)
+    let hiding_point_slot: Field = NFT::at(nft_contract_address)
+        .prepare_transfer_to_private(not_owner)
+        .call(&mut env.private());
 
     // Try transferring someone else's public NFT
     env.impersonate(not_owner);
-
-    NFT::at(nft_contract_address)
-        .finalize_transfer_to_private(token_id, transfer_preparer_storage_slot_commitment)
-        .call(&mut env.public());
+    NFT::at(nft_contract_address).finalize_transfer_to_private(token_id, hiding_point_slot).call(
+        &mut env.public(),
+    );
 }

noir-projects/noir-contracts/contracts/nft_contract/src/test/utils.nr

@@ -63,24 +63,14 @@ pub unconstrained fn setup_mint_and_transfer_to_private(
         setup_and_mint(with_account_contracts);
 
     let note_randomness = random();
-    let transient_storage_slot_randomness = random();
-    let transfer_preparer_storage_slot_commitment = pedersen_hash(
-        [owner.to_field(), transient_storage_slot_randomness],
-        NFT::TRANSIENT_STORAGE_SLOT_PEDERSEN_INDEX,
-    );
 
     // We mock the Oracle to return the note randomness such that later on we can manually add the note
     let _ = OracleMock::mock("getRandomField").returns(note_randomness);
 
-    // We prepare the transfer with user being both the sender and the recipient (classical "shield" flow)
-    NFT::at(nft_contract_address)
-        .prepare_transfer_to_private(owner, owner, transient_storage_slot_randomness)
-        .call(&mut env.private());
-
-    // Finalize the transfer of the NFT
-    NFT::at(nft_contract_address)
-        .finalize_transfer_to_private(minted_token_id, transfer_preparer_storage_slot_commitment)
-        .call(&mut env.public());
+    // We transfer the public NFT to private.
+    NFT::at(nft_contract_address).transfer_to_private(owner, minted_token_id).call(
+        &mut env.private(),
+    );
 
     // TODO(#8771): We need to manually add the note because in the partial notes flow `notify_created_note_oracle`
     // is not called and we don't have a `NoteProcessor` in TXE.