chore(noir-libs): TransparentNote rework (#1412)

Closes #1194
Closes #1363

Will likely rename getCommitment alongside all instances of "commitment"
as a part of this other issue:
#1408

# Checklist:
Remove the checklist to signal you've completed it. Enable auto-merge if
the PR is ready to merge.
- [x] If the pull request requires a cryptography review (e.g.
cryptographic algorithm implementations) I have added the 'crypto' tag.
- [x] I have reviewed my diff in github, line by line and removed
unexpected formatting changes, testing logs, or commented-out code.
- [x] Every change is related to the PR description.
- [x] I have
[linked](https://docs.github.com/en/issues/tracking-your-work-with-issues/linking-a-pull-request-to-an-issue)
this pull request to relevant issues (if any exist).

---------

Co-authored-by: Michael Connor <[email protected]>

yarn-project/acir-simulator/src/client/client_execution_context.ts

@@ -208,6 +208,10 @@ export class ClientTxExecutionContext {
    * @returns The commitment data.
    */
   public async getCommitment(contractAddress: AztecAddress, commitment: ACVMField) {
+    // TODO(https://github.com/AztecProtocol/aztec-packages/issues/1386): only works
+    // for noteHashes/commitments created by public functions! Once public kernel or
+    // base rollup circuit injects nonces, this can be used with commitments created by
+    // private functions as well.
     const commitmentInputs = await this.db.getCommitmentOracle(contractAddress, fromACVMField(commitment));
     // TODO(https://github.com/AztecProtocol/aztec-packages/issues/1029): support pending commitments here
     this.readRequestPartialWitnesses.push(ReadRequestMembershipWitness.empty(commitmentInputs.index));

yarn-project/acir-simulator/src/client/private_execution.test.ts

@@ -492,17 +492,23 @@ describe('Private Execution test suite', () => {
       const wasm = await CircuitsWasm.get();
       const secret = new Fr(1n);
       const secretHash = computeSecretMessageHash(wasm, secret);
-      const commitment = Fr.fromBuffer(hash([toBufferBE(amount, 32), secretHash.toBuffer()]));
-      const siloedCommitment = siloCommitment(wasm, contractAddress, commitment);
+      const preimage = [toBufferBE(amount, 32), secretHash.toBuffer()];
+      const noteHash = Fr.fromBuffer(hash(preimage));
+      const storageSlot = new Fr(2);
+      const innerNoteHash = hash([storageSlot.toBuffer(), noteHash.toBuffer()]);
+      const siloedNoteHash = siloCommitment(wasm, contractAddress, Fr.fromBuffer(innerNoteHash));
+      // TODO(https://github.com/AztecProtocol/aztec-packages/issues/1386): should insert
+      // uniqueSiloedNoteHash in tree and that should be what is expected in Noir
+      //const uniqueSiloedNoteHash = computeUniqueCommitment(wasm, nonce, Fr.fromBuffer(innerNoteHash));
 
-      const tree = await insertLeaves([siloedCommitment]);
+      const tree = await insertLeaves([siloedNoteHash]);
 
       oracle.getCommitmentOracle.mockImplementation(async () => {
         // Check the calculated commitment is correct
         return Promise.resolve({
-          commitment: siloedCommitment,
-          index: 0n,
+          commitment: siloedNoteHash,
           siblingPath: (await tree.getSiblingPath(0n, false)).toFieldArray(),
+          index: 0n,
         });
       });
 
@@ -519,7 +525,7 @@ describe('Private Execution test suite', () => {
       // Check the commitment read request was created successfully.
       const readRequests = result.callStackItem.publicInputs.readRequests.filter(field => !field.equals(Fr.ZERO));
       expect(readRequests).toHaveLength(1);
-      expect(readRequests[0]).toEqual(siloedCommitment);
+      expect(readRequests[0]).toEqual(siloedNoteHash);
     });
   });
 

yarn-project/acir-simulator/src/public/index.test.ts

@@ -297,11 +297,13 @@ describe('ACIR public execution simulator', () => {
       // Assert the commitment was created
       expect(result.newCommitments.length).toEqual(1);
 
-      const expectedNewCommitmentValue = pedersenPlookupCommitInputs(
+      const expectedNoteHash = pedersenPlookupCommitInputs(
         wasm,
         params.map(a => a.toBuffer()),
       );
-      expect(result.newCommitments[0].toBuffer()).toEqual(expectedNewCommitmentValue);
+      const storageSlot = new Fr(2); // matches storage.nr
+      const expectedInnerNoteHash = pedersenPlookupCommitInputs(wasm, [storageSlot.toBuffer(), expectedNoteHash]);
+      expect(result.newCommitments[0].toBuffer()).toEqual(expectedInnerNoteHash);
     });
 
     it('Should be able to create a L2 to L1 message from the public context', async () => {

yarn-project/end-to-end/src/e2e_public_cross_chain_messaging.test.ts

@@ -82,7 +82,7 @@ describe('e2e_public_cross_chain_messaging', () => {
     // Generate a claim secret using pedersen
     const l1TokenBalance = 1000000n;
     const bridgeAmount = 100n;
-    const publicBalanceSlot = 2n;
+    const publicBalanceSlot = 3n; // check contract's storage.nr for slot assignment
 
     const [secret, secretHash] = await crossChainTestHarness.generateClaimSecret();
 

yarn-project/end-to-end/src/e2e_public_to_private_messaging.test.ts

@@ -65,7 +65,7 @@ describe('e2e_public_to_private_messaging', () => {
     const l1TokenBalance = 1000000n;
     const bridgeAmount = 100n;
     const shieldAmount = 50n;
-    const publicBalanceSlot = 2n;
+    const publicBalanceSlot = 3n; // check contract's storage.nr for slot assignment
 
     const [secret, secretHash] = await crossChainTestHarness.generateClaimSecret();
 

yarn-project/noir-contracts/src/contracts/non_native_token_contract/src/main.nr

@@ -24,7 +24,11 @@ contract NonNativeToken {
         value_note::{VALUE_NOTE_LEN, ValueNoteMethods},
     };
 
-    use crate::transparent_note::TransparentNote;
+    use crate::transparent_note::{
+        TransparentNote,
+        TransparentNoteMethods,
+        TRANSPARENT_NOTE_LEN,
+    };
 
     use crate::storage::Storage;
     use crate::hash::{get_mint_content_hash, get_withdraw_content_hash};
@@ -59,7 +63,7 @@ contract NonNativeToken {
         // Should eventually be hidden:
         inputs: PrivateContextInputs,
         //*********************************/
-        initial_supply: Field, 
+        initial_supply: Field,
         owner: Field,
     ) -> distinct pub abi::PrivateCircuitPublicInputs {
         let storage = Storage::init();
@@ -74,14 +78,14 @@ contract NonNativeToken {
     }
 
     // Mint Private Function
-    // This mint function differs to the typical token mint function as it only allows minting 
+    // This mint function differs to the typical token mint function as it only allows minting
     // upon consuming valid messages from a token portal contract
     fn mint(
         //*********************************/
         // Should eventually be hidden:
         inputs: PrivateContextInputs,
         //*********************************/
-        amount: Field, 
+        amount: Field,
         owner: Field,
         // This field should be hidden
         msg_key: Field,
@@ -114,8 +118,8 @@ contract NonNativeToken {
         // Should eventually be hidden:
         inputs: PrivateContextInputs,
         //*********************************/
-        amount: Field, 
-        sender: Field, 
+        amount: Field,
+        sender: Field,
         recipient: Field, // ethereum address in the field
         callerOnL1: Field, // ethereum address that can call this function on the L1 portal (0x0 if anyone can call)
     ) -> distinct pub abi::PrivateCircuitPublicInputs {
@@ -135,14 +139,14 @@ contract NonNativeToken {
     }
 
     // Mint Public Function
-    // This mint function differs to the typical token mint function as it only allows minting 
+    // This mint function differs to the typical token mint function as it only allows minting
     // upon consuming valid messages from a token portal contract
     open fn mintPublic(
         //*********************************/
         // Should eventually be hidden:
         inputs: PublicContextInputs,
         //*********************************/
-        amount: Field, 
+        amount: Field,
         owner_address: Field,
         // This field should be hidden
         msg_key: Field,
@@ -177,7 +181,7 @@ contract NonNativeToken {
     ) {
         let storage = Storage::init();
         let public_balances = storage.public_balances;
-        
+
         let sender = inputs.call_context.msg_sender;
         let sender_balance = public_balances.at(sender);
 
@@ -186,7 +190,7 @@ contract NonNativeToken {
         if (current_sender_balance as u120) > (amount as u120) {
             // User has sufficient balance so we decrement it by `amount`
             let _void1 = sender_balance.write(current_sender_balance - amount);
-        }  
+        }
         // TODO: Revert if there is not enough balance
 
         let content = get_withdraw_content_hash(amount, recipient, callerOnL1);
@@ -203,8 +207,8 @@ contract NonNativeToken {
         // Should eventually be hidden:
         inputs: PrivateContextInputs,
         //*********************************/
-        amount: Field, 
-        sender: Field, 
+        amount: Field,
+        sender: Field,
         recipient: Field,
     ) -> distinct pub abi::PrivateCircuitPublicInputs {
         let storage = Storage::init();
@@ -231,6 +235,7 @@ contract NonNativeToken {
     ) {
         let storage = Storage::init();
         let public_balances = storage.public_balances;
+        let pending_shields = storage.pending_shields;
 
         // Decrease user's balance.
         let sender = inputs.call_context.msg_sender;
@@ -241,31 +246,39 @@ contract NonNativeToken {
 
         // User has sufficient balance so we decrement it by `amount`
         let _void1 = sender_balance.write(current_sender_balance - amount);
-        
-        // Create a commitment to the amount
-        let note = TransparentNote::new(amount, secretHash);
-
-        // Public oracle call to emit new commitment.
-        create_commitment(note.get_commitment());
+
+        // Create a commitment to the "amount" using the "secretHash"
+        // and insert it into the set of "pending_shields" and therefore
+        // (eventually) the private data tree.
+        let mut note = TransparentNote::new(amount, secretHash);
+        pending_shields.insert_from_public(inputs, &mut note);
     }
 
     // The shield function takes a public balance, and creates a commitment containing the amount of tokens
-    // in the private context. 
+    // in the private context.
     fn redeemShield(
         inputs: PrivateContextInputs,
         amount: Field,
         secret: Field,
         owner: Field,
     ) -> distinct pub abi::PrivateCircuitPublicInputs {
         let storage = Storage::init();
+        let pending_shields = storage.pending_shields;
 
         let mut context = Context::new(inputs, abi::hash_args([
             amount, secret, owner
         ]));
 
         // Assert that the note exists within the tree
-        let public_note = TransparentNote::new_from_secret(amount, secret);
-        public_note.consume_in_secret(&mut context, inputs.roots.private_data_tree_root, secret);
+        let mut public_note = TransparentNote::new_from_secret(amount, secret);
+
+        // Ensure that the note exists in the tree
+        pending_shields.assert_contains_note_hash(&mut context, &mut public_note);
+        // The above call to `assert_contains()` also returns a modified note with
+        // the header which is necessary for the next step (remove).
+
+        // Consume note in secret!
+        pending_shields.remove(&mut context, public_note);
 
         // Mint the tokens
         let balance = storage.balances.at(owner);
@@ -291,7 +304,7 @@ contract NonNativeToken {
 
         // enqueue a public function to perform the public state update.
         let thisAddress = inputs.call_context.storage_contract_address;
-        
+
         // addUnshieldedBalance selector (in decimal)
         // recompute by: `cast keccak addUnshieldedBalance(field,field)` -> convert to decimal
         let pubEntryPointSelector = 753269941;
@@ -318,7 +331,7 @@ contract NonNativeToken {
     ) -> Field {
         let storage = Storage::init();
         let owner_balance = storage.balances.at(owner);
-        
+
         balance_utils::get_balance(owner_balance.storage_slot)
     }
 
@@ -327,6 +340,10 @@ contract NonNativeToken {
     // Note 2: Having it in all the contracts gives us the ability to compute the note hash and nullifier differently for different kind of notes.
     unconstrained fn compute_note_hash_and_nullifier(contract_address: Field, nonce: Field, storage_slot: Field, preimage: [Field; VALUE_NOTE_LEN]) -> [Field; 4] {
         let note_header = NoteHeader { contract_address, nonce, storage_slot };
-        note_utils::compute_note_hash_and_nullifier(ValueNoteMethods, note_header, preimage)
+        if (storage_slot == 2) {
+            note_utils::compute_note_hash_and_nullifier(TransparentNoteMethods, note_header, preimage)
+        } else {
+            note_utils::compute_note_hash_and_nullifier(ValueNoteMethods, note_header, preimage)
+        }
     }
 }

yarn-project/noir-contracts/src/contracts/non_native_token_contract/src/storage.nr

@@ -1,11 +1,9 @@
-use dep::aztec::state_vars::{
-    map::Map,
-    set::Set,
-    public_state::PublicState,
-    type_serialisation::field_serialisation::{
-        FIELD_SERIALISED_LEN,
-        FieldSerialisationMethods,
-    },
+mod transparent_note;
+
+use crate::transparent_note::{
+    TransparentNote,
+    TransparentNoteMethods,
+    TRANSPARENT_NOTE_LEN,
 };
 
 use dep::value_note::value_note::{
@@ -14,17 +12,28 @@ use dep::value_note::value_note::{
     VALUE_NOTE_LEN,
 };
 
+use dep::aztec::state_vars::{
+    map::Map,
+    set::Set,
+    public_state::PublicState,
+};
+use dep::aztec::state_vars::type_serialisation::field_serialisation::FieldSerialisationMethods;
+use dep::aztec::state_vars::type_serialisation::field_serialisation::FIELD_SERIALISED_LEN;
+
 struct Storage {
     balances: Map<Set<ValueNote, VALUE_NOTE_LEN>>,
 
+    pending_shields: Set<TransparentNote, TRANSPARENT_NOTE_LEN>,
+
     public_balances: Map<PublicState<Field, FIELD_SERIALISED_LEN>>,
 }
 
 impl Storage {
     fn init() -> Self {
         Storage {
-            balances: Map::new(1, |slot| Set::new(slot, ValueNoteMethods)),
-            public_balances: Map::new(2, |slot| PublicState::new(slot, FieldSerialisationMethods)),
+            balances: Map::new(1, |s| Set::new(s, ValueNoteMethods)),
+            pending_shields: Set::new(2, TransparentNoteMethods),
+            public_balances: Map::new(3, |s| PublicState::new(s, FieldSerialisationMethods)),
         }
     }
 }