refactor: updated NFT flows

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

@@ -8,7 +8,7 @@ pub use crate::{
         map::Map, private_immutable::PrivateImmutable, private_mutable::PrivateMutable,
         public_immutable::PublicImmutable, public_mutable::PublicMutable, private_set::PrivateSet,
         shared_immutable::SharedImmutable, shared_mutable::SharedMutable, storage::Storable,
-    }, context::{PrivateContext, PackedReturns, FunctionReturns},
+    }, context::{PrivateContext, PackedReturns, FunctionReturns, PublicContext},
     note::{
         note_header::NoteHeader, note_interface::{NoteInterface, NullifiableNote},
         note_getter_options::NoteGetterOptions, note_viewer_options::NoteViewerOptions,

noir-projects/aztec-nr/aztec/src/utils/bytes.nr

@@ -75,7 +75,8 @@ mod test {
     #[test]
     fn test_bytes_to_1_field() {
         let input = [
-            1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24, 25, 26, 27, 28, 29, 30, 31
+            1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24,
+            25, 26, 27, 28, 29, 30, 31,
         ];
         let output = bytes_to_fields::<31, 1>(input);
 
@@ -88,9 +89,11 @@ mod test {
         let output = fields_to_bytes::<31, 1>(input);
 
         assert_eq(
-            output, [
-            1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24, 25, 26, 27, 28, 29, 30, 31
-        ]
+            output,
+            [
+                1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23,
+                24, 25, 26, 27, 28, 29, 30, 31,
+            ],
         );
     }
 
@@ -101,9 +104,13 @@ mod test {
 
         // Each field should occupy 31 bytes with the non-zero value being placed in the last one.
         assert_eq(
-            output, [
-            0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 1, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 2, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 3
-        ]
+            output,
+            [
+                0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
+                0, 0, 1, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
+                0, 0, 0, 0, 0, 2, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
+                0, 0, 0, 0, 0, 0, 0, 0, 3,
+            ],
         );
     }
 
@@ -116,16 +123,21 @@ mod test {
         // field should occupy 1 byte. There is not information destruction here because the last field fits into
         // 1 byte.
         assert_eq(
-            output, [
-            0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 1, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 2, 3
-        ]
+            output,
+            [
+                0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
+                0, 0, 1, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
+                0, 0, 0, 0, 0, 2, 3,
+            ],
         );
     }
 
     #[test]
     fn test_bytes_to_2_fields() {
         let input = [
-            1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24, 25, 26, 27, 28, 29, 30, 31, 32, 33, 34, 35, 36, 37, 38, 39, 40, 41, 42, 43, 44, 45, 46, 47, 48, 49, 50, 51, 52, 53, 54, 55, 56, 57, 58, 59
+            1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24,
+            25, 26, 27, 28, 29, 30, 31, 32, 33, 34, 35, 36, 37, 38, 39, 40, 41, 42, 43, 44, 45, 46,
+            47, 48, 49, 50, 51, 52, 53, 54, 55, 56, 57, 58, 59,
         ];
         let output = bytes_to_fields::<59, 2>(input);
 
@@ -136,14 +148,18 @@ mod test {
     #[test]
     fn test_2_fields_to_bytes() {
         let input = [
-            0x0102030405060708090a0b0c0d0e0f101112131415161718191a1b1c1d1e1f, 0x202122232425262728292a2b2c2d2e2f303132333435363738393a3b
+            0x0102030405060708090a0b0c0d0e0f101112131415161718191a1b1c1d1e1f,
+            0x202122232425262728292a2b2c2d2e2f303132333435363738393a3b,
         ];
         let output = fields_to_bytes::<62, 2>(input);
 
         assert_eq(
-            output, [
-            1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24, 25, 26, 27, 28, 29, 30, 31, 0, 0, 0, 32, 33, 34, 35, 36, 37, 38, 39, 40, 41, 42, 43, 44, 45, 46, 47, 48, 49, 50, 51, 52, 53, 54, 55, 56, 57, 58, 59
-        ]
+            output,
+            [
+                1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23,
+                24, 25, 26, 27, 28, 29, 30, 31, 0, 0, 0, 32, 33, 34, 35, 36, 37, 38, 39, 40, 41, 42,
+                43, 44, 45, 46, 47, 48, 49, 50, 51, 52, 53, 54, 55, 56, 57, 58, 59,
+            ],
         );
     }
 
@@ -165,11 +181,16 @@ mod test {
         input3: [u64; 5],
         input4: [u32; 5],
         input5: [u16; 5],
-        input6: [u8; 5]
+        input6: [u8; 5],
     ) {
         let mut input = [0; 5];
         for i in 0..5 {
-            input[i] = (input1[i] as Field * 2.pow_32(184)) + (input2[i] as Field * 2.pow_32(120)) + (input3[i] as Field * 2.pow_32(56)) + (input4[i] as Field * 2.pow_32(24)) + (input5[i] as Field * 2.pow_32(8)) + input6[i] as Field;
+            input[i] = (input1[i] as Field * 2.pow_32(184))
+                + (input2[i] as Field * 2.pow_32(120))
+                + (input3[i] as Field * 2.pow_32(56))
+                + (input4[i] as Field * 2.pow_32(24))
+                + (input5[i] as Field * 2.pow_32(8))
+                + input6[i] as Field;
         }
 
         let output = fields_to_bytes::<155, 5>(input);

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

@@ -12,7 +12,7 @@ contract NFT {
         oracle::random::random,
         prelude::{
             NoteGetterOptions, NoteViewerOptions, Map, PublicMutable, SharedImmutable, PrivateSet,
-            AztecAddress,
+            AztecAddress, PrivateContext, PublicContext,
         },
         encrypted_logs::encrypted_note_emission::{
             encode_and_encrypt_note, encrypt_and_emit_partial_log,
@@ -31,8 +31,6 @@ contract NFT {
     use std::{embedded_curve_ops::EmbeddedCurvePoint, meta::derive};
     use crate::types::nft_note::NFTNote;
 
-    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]
@@ -144,78 +142,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");
@@ -329,3 +366,4 @@ contract NFT {
         (owned_nft_ids, page_limit_reached)
     }
 }
+