final cleanup

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

@@ -12,7 +12,7 @@ unconstrained fn shielding_on_behalf_of_self() {
 
     let note_randomness = unsafe_rand();
     let transient_storage_slot_randomness = unsafe_rand();
-    // Owner will be the msg_sender in prepare_shield
+    // Owner will be the msg_sender/shield_preparer in prepare_shield
     let shield_preparer_storage_slot_commitment = pedersen_hash(
         [owner.to_field(), transient_storage_slot_randomness],
         NFT::TRANSIENT_STORAGE_SLOT_PEDERSEN_INDEX
@@ -56,7 +56,7 @@ unconstrained fn shielding_to_a_different_account() {
 
     let note_randomness = unsafe_rand();
     let transient_storage_slot_randomness = unsafe_rand();
-    // Owner will be the msg_sender in prepare_shield
+    // Owner will be the msg_sender/shield_preparer in prepare_shield
     let shield_preparer_storage_slot_commitment = pedersen_hash(
         [owner.to_field(), transient_storage_slot_randomness],
         NFT::TRANSIENT_STORAGE_SLOT_PEDERSEN_INDEX
@@ -109,14 +109,13 @@ unconstrained fn shielding_failure_on_behalf_of_self_shield_not_prepared() {
 #[test(should_fail_with="invalid NFT owner")]
 unconstrained fn shielding_failure_not_an_owner() {
     // Setup without account contracts. We are not using authwits here, so dummy accounts are enough
-    let (env, nft_contract_address, _, recipient, token_id) = utils::setup_and_mint(/* with_account_contracts */ false);
+    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 owner check is before we use the value
     let shield_preparer_storage_slot_commitment = unsafe_rand();
 
-    // Try sending our public NFT to the shield
+    // Try sending owner's public NFT to the shield while impersonating a different account
     let send_to_shield_call_interface = NFT::at(nft_contract_address).send_to_shield(token_id, shield_preparer_storage_slot_commitment);
-
-    env.impersonate(recipient);
+    env.impersonate(not_owner);
     env.call_public(send_to_shield_call_interface);
 }

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

@@ -1,5 +1,6 @@
 use crate::test::utils;
 use dep::aztec::test::helpers::cheatcodes;
+use aztec::oracle::unsafe_rand::unsafe_rand;
 use dep::authwit::cheatcodes as authwit_cheatcodes;
 use crate::NFT;
 
@@ -25,7 +26,7 @@ unconstrained fn transfer_private_to_self() {
     let transfer_private_call_interface = NFT::at(nft_contract_address).transfer_from(owner, owner, token_id, 0);
     env.call_private_void(transfer_private_call_interface);
 
-    // Owner should have the note in their private nfts
+    // NFT owner should stay the same
     utils::assert_owns_private_nft(nft_contract_address, owner, token_id);
 }
 
@@ -39,7 +40,7 @@ unconstrained fn transfer_private_to_non_deployed_account() {
     let transfer_private_call_interface = NFT::at(nft_contract_address).transfer_from(owner, not_deployed.address, token_id, 0);
     env.call_private_void(transfer_private_call_interface);
 
-    // Recipient should have the note in their private nfts
+    // Owner of the private NFT should be the not_deployed account
     utils::assert_owns_private_nft(nft_contract_address, not_deployed.address, token_id);
 }
 
@@ -57,7 +58,7 @@ unconstrained fn transfer_private_on_behalf_of_other() {
     // Transfer the NFT to the recipient
     env.call_private_void(transfer_private_call_interface);
 
-    // Recipient should have the note in their private nfts
+    // Recipient should be the private NFT owner
     utils::assert_owns_private_nft(nft_contract_address, recipient, token_id);
 }
 
@@ -74,35 +75,47 @@ unconstrained fn transfer_private_failure_not_an_owner() {
 
 #[test(should_fail_with="invalid nonce")]
 unconstrained fn transfer_private_failure_on_behalf_of_self_non_zero_nonce() {
-    // Setup without account contracts. We are not using authwits here, so dummy accounts are enough
-    let (env, nft_contract_address, owner, recipient, token_id) = utils::setup_mint_and_shield(/* with_account_contracts */ false);
-    // Add authwit
+    // Setup without account contracts. We are not using authwits here, so dummy accounts are enough.
+    // The nonce check is in the beginning so we don't need to waste time on minting and shielding the NFT.
+    let (env, nft_contract_address, owner, recipient) = utils::setup(/* with_account_contracts */ false);
+
+    // We set random value for the token_id as the nonce check is before we use the value.
+    let token_id = unsafe_rand();
+
     let transfer_private_from_call_interface = NFT::at(nft_contract_address).transfer_from(owner, recipient, token_id, 1);
-    // Transfer tokens
+    // Try transferring the NFT
     env.call_private_void(transfer_private_from_call_interface);
 }
 
 #[test(should_fail_with="Authorization not found for message hash")]
 unconstrained fn transfer_private_failure_on_behalf_of_other_without_approval() {
-    // Setup with account contracts. Slower since we actually deploy them, but needed for authwits.
-    let (env, nft_contract_address, owner, recipient, token_id) = utils::setup_mint_and_shield(/* with_account_contracts */ true);
-    // Add authwit
+    // Setup without account contracts. This is necessary for impersonation to work.
+    // The authwit check is in the beginning so we don't need to waste time on minting and shielding the NFT.
+    let (env, nft_contract_address, owner, recipient) = utils::setup(/* with_account_contracts */ true);
+
+    // We set random value for the token_id as the nonce check is before we use the value.
+    let token_id = unsafe_rand();
+
     let transfer_private_from_call_interface = NFT::at(nft_contract_address).transfer_from(owner, recipient, token_id, 1);
     // Impersonate recipient to perform the call
     env.impersonate(recipient);
-    // Transfer tokens
+    // Try transferring the NFT
     env.call_private_void(transfer_private_from_call_interface);
 }
 
 #[test(should_fail_with="Authorization not found for message hash")]
 unconstrained fn transfer_private_failure_on_behalf_of_other_wrong_caller() {
-    // Setup with account contracts. Slower since we actually deploy them, but needed for authwits.
-    let (env, nft_contract_address, owner, recipient, token_id) = utils::setup_mint_and_shield(/* with_account_contracts */ true);
-    // Add authwit
+    // Setup without account contracts. This is necessary for impersonation to work.
+    // The authwit check is in the beginning so we don't need to waste time on minting and shielding the NFT.
+    let (env, nft_contract_address, owner, recipient) = utils::setup(/* with_account_contracts */ true);
+
+    // We set random value for the token_id as the nonce check is before we use the value.
+    let token_id = unsafe_rand();
+
     let transfer_private_from_call_interface = NFT::at(nft_contract_address).transfer_from(owner, recipient, token_id, 1);
     authwit_cheatcodes::add_private_authwit_from_call_interface(owner, owner, transfer_private_from_call_interface);
     // Impersonate recipient to perform the call
     env.impersonate(recipient);
-    // Transfer tokens
+    // Try transferring the NFT
     env.call_private_void(transfer_private_from_call_interface);
 }

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

@@ -8,7 +8,7 @@ unconstrained fn public_transfer() {
     // Setup without account contracts. We are not using authwits here, so dummy accounts are enough
     let (env, nft_contract_address, owner, recipient, token_id) = utils::setup_and_mint(/* with_account_contracts */ false);
 
-    // Transfer the minted NFT
+    // Transfer the NFT
     let public_transfer_call_interface = NFT::at(nft_contract_address).transfer_public(owner, recipient, token_id, 0);
     env.call_public(public_transfer_call_interface);
 
@@ -20,10 +20,11 @@ unconstrained fn public_transfer_to_self() {
     // Setup without account contracts. We are not using authwits here, so dummy accounts are enough
     let (env, nft_contract_address, owner, _, token_id) = utils::setup_and_mint(/* with_account_contracts */ false);
 
-    // Transfer token
+    // Transfer the NFT
     let public_transfer_call_interface = NFT::at(nft_contract_address).transfer_public(owner, owner, token_id, 0);
     env.call_public(public_transfer_call_interface);
 
+    // The owner should stay the same
     utils::assert_owns_public_nft(env, nft_contract_address, owner, token_id);
 }
 
@@ -43,33 +44,34 @@ unconstrained fn public_transfer_on_behalf_of_other() {
     utils::assert_owns_public_nft(env, nft_contract_address, recipient, token_id);
 }
 
-#[test]
+#[test(should_fail_with="invalid nonce")]
 unconstrained fn public_transfer_failure_on_behalf_of_self_non_zero_nonce() {
-    // Setup without account contracts. We are not using authwits here, so dummy accounts are enough
-    let (env, nft_contract_address, owner, recipient, token_id) = utils::setup_and_mint(/* with_account_contracts */ true);
+    // Setup without account contracts. We are not using authwits here, so dummy accounts are enough.
+    // The authwit check is in the beginning so we don't need to waste time on minting and shielding the NFT.
+    let (env, nft_contract_address, owner, recipient) = utils::setup(/* with_account_contracts */ false);
+
+    // We set random value for the token_id as the nonce check is before we use the value.
+    let token_id = unsafe_rand();
 
     let public_transfer_call_interface = NFT::at(nft_contract_address).transfer_public(owner, recipient, token_id, unsafe_rand());
     // Try to transfer the NFT
-    env.assert_public_call_fails(public_transfer_call_interface);
-
-    // Check the owner was not changed
-    utils::assert_owns_public_nft(env, nft_contract_address, owner, token_id);
+    env.call_public(public_transfer_call_interface);
 }
 
-#[test]
+#[test(should_fail_with="invalid owner")]
 unconstrained fn public_transfer_non_existent_nft() {
     // Setup without account contracts. We are not using authwits here, so dummy accounts are enough
     let (env, nft_contract_address, owner, recipient) = utils::setup(/* with_account_contracts */ false);
 
     // Try to transfer the NFT
     let token_id = 612;
     let public_transfer_call_interface = NFT::at(nft_contract_address).transfer_public(owner, recipient, token_id, 0);
-    env.assert_public_call_fails(public_transfer_call_interface);
+    env.call_public(public_transfer_call_interface);
 }
 
 #[test]
 unconstrained fn public_transfer_failure_on_behalf_of_other_without_approval() {
-    // Setup with account contracts. Slower since we actually deploy them, but needed for authwits.
+    // Setup without account contracts. This is necessary for impersonation to work.
     let (env, nft_contract_address, owner, recipient, token_id) = utils::setup_and_mint(/* with_account_contracts */ true);
 
     let public_transfer_from_call_interface = NFT::at(nft_contract_address).transfer_public(owner, recipient, token_id, 1);

noir-projects/noir-contracts/contracts/token_contract/src/test/transfer_public.nr

@@ -63,18 +63,15 @@ unconstrained fn public_transfer_failure_more_than_balance() {
     utils::check_public_balance(token_contract_address, owner, mint_amount);
 }
 
-#[test]
+#[test(should_fail_with="invalid nonce")]
 unconstrained fn public_transfer_failure_on_behalf_of_self_non_zero_nonce() {
     // Setup without account contracts. We are not using authwits here, so dummy accounts are enough
     let (env, token_contract_address, owner, recipient, mint_amount) = utils::setup_and_mint(/* with_account_contracts */ false);
     // Transfer tokens
     let transfer_amount = mint_amount / 10;
     let public_transfer_call_interface = Token::at(token_contract_address).transfer_public(owner, recipient, transfer_amount, unsafe_rand());
     // Try to transfer tokens
-    env.assert_public_call_fails(public_transfer_call_interface);
-
-    // Check balances
-    utils::check_public_balance(token_contract_address, owner, mint_amount);
+    env.call_public(public_transfer_call_interface);
 }
 
 #[test]