respond to PR comments

l1-contracts/test/portals/TokenPortal.sol

@@ -27,8 +27,8 @@ contract TokenPortal {
    * @param _to - The aztec address of the recipient
    * @param _amount - The amount to deposit
    * @param _deadline - The timestamp after which the entry can be cancelled
-   * @param _secretHash - The hash of the secret consumable message
-   * @param _canceller - The address that can cancel the L1 to L2 message.
+   * @param _secretHash - The hash of the secret consumable message. The hash should be 254 bits (so it can fit in a Field element)
+   * @param _canceller - The address that can cancel the L1 to L2 message
    * @return The key of the entry in the Inbox
    */
   function depositToAztecPublic(
@@ -39,7 +39,6 @@ contract TokenPortal {
     address _canceller
   ) external payable returns (bytes32) {
     // Preamble
-    // @todo: (issue #624) handle different versions
     IInbox inbox = registry.getInbox();
     DataStructures.L2Actor memory actor = DataStructures.L2Actor(l2TokenAddress, 1);
 
@@ -61,7 +60,7 @@ contract TokenPortal {
    * @param _deadline - The timestamp after which the entry can be cancelled
    * @param _secretHashForL2MessageConsumption - The hash of the secret consumable L1 to L2 message. The hash should be 254 bits (so it can fit in a Field element)
    * @param _secretHashForL2MessageConsumption - The hash of the secret to redeem minted notes privately on Aztec. The hash should be 254 bits (so it can fit in a Field element)
-   * @param _canceller - The address that can cancel the L1 to L2 message.
+   * @param _canceller - The address that can cancel the L1 to L2 message
    * @return The key of the entry in the Inbox
    */
   function depositToAztecPrivate(
@@ -72,7 +71,6 @@ contract TokenPortal {
     address _canceller
   ) external payable returns (bytes32) {
     // Preamble
-    // @todo: (issue #624) handle different versions
     IInbox inbox = registry.getInbox();
     DataStructures.L2Actor memory actor = DataStructures.L2Actor(l2TokenAddress, 1);
 
@@ -113,7 +111,6 @@ contract TokenPortal {
     bytes32 _secretHash,
     uint64 _fee
   ) external returns (bytes32) {
-    // @todo: (issue #624) handle different versions
     IInbox inbox = registry.getInbox();
     DataStructures.L1Actor memory l1Actor = DataStructures.L1Actor(address(this), block.chainid);
     DataStructures.L2Actor memory l2Actor = DataStructures.L2Actor(l2TokenAddress, 1);
@@ -151,7 +148,6 @@ contract TokenPortal {
     bytes32 _secretHashForRedeemingMintedNotes,
     uint64 _fee
   ) external returns (bytes32) {
-    // @todo: (issue #624) handle different versions
     IInbox inbox = registry.getInbox();
     DataStructures.L1Actor memory l1Actor = DataStructures.L1Actor(address(this), block.chainid);
     DataStructures.L2Actor memory l2Actor = DataStructures.L2Actor(l2TokenAddress, 1);
@@ -206,7 +202,6 @@ contract TokenPortal {
         )
     });
 
-    // @todo: (issue #624) handle different versions
     bytes32 entryKey = registry.getOutbox().consume(message);
 
     underlying.transfer(_recipient, _amount);

l1-contracts/test/portals/TokenPortal.t.sol

@@ -48,8 +48,10 @@ contract TokenPortalTest is Test {
   bytes32 internal to = bytes32(0x2d749407d8c364537cdeb799c1574929cb22ff1ece2b96d2a1c6fa287a0e0171);
   uint256 internal amount = 100;
   uint256 internal mintAmount = 1 ether;
+  // this hash is just a random 32 byte string
   bytes32 internal secretHashForL2MessageConsumption =
     0x147e4fec49805c924e28150fc4b36824679bc17ecb1d7d9f6a9effb7fde6b6a0;
+  // this hash is just a random 32 byte string
   bytes32 internal secretHashForRedeemingMintedNotes =
     0x157e4fec49805c924e28150fc4b36824679bc17ecb1d7d9f6a9effb7fde6b6a0;
   uint64 internal bid = 1 ether;
@@ -123,7 +125,7 @@ contract TokenPortalTest is Test {
       _createExpectedMintPrivateL1ToL2Message(address(this));
     bytes32 expectedEntryKey = inbox.computeEntryKey(expectedMessage);
 
-    // Check the even was emitted
+    // Check the event was emitted
     vm.expectEmit(true, true, true, true);
     // event we expect
     emit MessageAdded(
@@ -166,7 +168,7 @@ contract TokenPortalTest is Test {
       _createExpectedMintPublicL1ToL2Message(address(this));
     bytes32 expectedEntryKey = inbox.computeEntryKey(expectedMessage);
 
-    // Check the even was emitted
+    // Check the event was emitted
     vm.expectEmit(true, true, true, true);
     // event we expect
     emit MessageAdded(

yarn-project/aztec-nr/aztec/src/types/address.nr

@@ -29,7 +29,7 @@ impl EthereumAddress {
     // Check that it actually will fit. Spending a lot of constraints here :grimacing:
     let bytes = address.to_be_bytes(32);
     for i in 0..12 {
-        assert(bytes[i] == 0, "Value too large for SafeU120");
+        assert(bytes[i] == 0, "Value too large for an ethereum address");
     }
     Self {
       address

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

@@ -167,12 +167,10 @@ describe('e2e_cross_chain_messaging', () => {
         .withWallet(user2Wallet)
         .methods.claim_private(
           bridgeAmount,
+          secretHashForL2MessageConsumption,
+          { address: ethAccount.toField() },
           messageKey,
           secretForL2MessageConsumption,
-          secretHashForL2MessageConsumption,
-          {
-            address: ethAccount.toField(),
-          },
         )
         .simulate(),
     ).rejects.toThrowError("Cannot satisfy constraint 'l1_to_l2_message_data.message.content == content");
@@ -182,12 +180,10 @@ describe('e2e_cross_chain_messaging', () => {
       .withWallet(user2Wallet)
       .methods.claim_private(
         bridgeAmount,
+        secretHashForRedeemingMintedNotes,
+        { address: ethAccount.toField() },
         messageKey,
         secretForL2MessageConsumption,
-        secretHashForRedeemingMintedNotes,
-        {
-          address: ethAccount.toField(),
-        },
       )
       .send();
     const consumptionReceipt = await consumptionTx.wait();
@@ -217,9 +213,9 @@ describe('e2e_cross_chain_messaging', () => {
       l2Bridge
         .withWallet(user1Wallet)
         .methods.exit_to_l1_private(
+          { address: ethAccount.toField() },
           { address: l2Token.address },
           withdrawAmount,
-          { address: ethAccount.toField() },
           { address: EthAddress.ZERO.toField() },
           nonce,
         )

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

@@ -151,19 +151,27 @@ describe('e2e_public_cross_chain_messaging', () => {
     await expect(
       l2Bridge
         .withWallet(user2Wallet)
-        .methods.claim_public({ address: user2Wallet.getAddress() }, bridgeAmount, messageKey, secret, {
-          address: ownerEthAddress.toField(),
-        })
+        .methods.claim_public(
+          { address: user2Wallet.getAddress() },
+          bridgeAmount,
+          { address: ownerEthAddress.toField() },
+          messageKey,
+          secret,
+        )
         .simulate(),
     ).rejects.toThrow();
 
     // user2 consumes owner's L1-> L2 message on bridge contract and mints public tokens on L2
     logger("user2 consumes owner's message on L2 Publicly");
     const tx = l2Bridge
       .withWallet(user2Wallet)
-      .methods.claim_public({ address: ownerAddress }, bridgeAmount, messageKey, secret, {
-        address: ownerEthAddress.toField(),
-      })
+      .methods.claim_public(
+        { address: ownerAddress },
+        bridgeAmount,
+        { address: ownerEthAddress.toField() },
+        messageKey,
+        secret,
+      )
       .send();
     const receipt = await tx.wait();
     expect(receipt.status).toBe(TxStatus.MINED);
@@ -183,8 +191,8 @@ describe('e2e_public_cross_chain_messaging', () => {
       l2Bridge
         .withWallet(ownerWallet)
         .methods.exit_to_l1_public(
-          withdrawAmount,
           { address: ownerEthAddress.toField() },
+          withdrawAmount,
           { address: EthAddress.ZERO.toField() },
           nonce,
         )

yarn-project/end-to-end/src/fixtures/cross_chain_test_harness.ts

@@ -211,16 +211,20 @@ export class CrossChainTestHarness {
 
   async consumeMessageOnAztecAndMintSecretly(
     bridgeAmount: bigint,
+    secretHashForRedeemingMintedNotes: Fr,
     messageKey: Fr,
     secretForL2MessageConsumption: Fr,
-    secretHashForL2MessageConsumption: Fr,
   ) {
     this.logger('Consuming messages on L2 secretively');
     // Call the mint tokens function on the Aztec.nr contract
     const consumptionTx = this.l2Bridge.methods
-      .claim_private(bridgeAmount, messageKey, secretForL2MessageConsumption, secretHashForL2MessageConsumption, {
-        address: this.ethAccount.toField(),
-      })
+      .claim_private(
+        bridgeAmount,
+        secretHashForRedeemingMintedNotes,
+        { address: this.ethAccount.toField() },
+        messageKey,
+        secretForL2MessageConsumption,
+      )
       .send();
     const consumptionReceipt = await consumptionTx.wait();
     expect(consumptionReceipt.status).toBe(TxStatus.MINED);
@@ -230,9 +234,13 @@ export class CrossChainTestHarness {
     this.logger('Consuming messages on L2 Publicly');
     // Call the mint tokens function on the Aztec.nr contract
     const tx = this.l2Bridge.methods
-      .claim_public({ address: this.ownerAddress }, bridgeAmount, messageKey, secret, {
-        address: this.ethAccount.toField(),
-      })
+      .claim_public(
+        { address: this.ownerAddress },
+        bridgeAmount,
+        { address: this.ethAccount.toField() },
+        messageKey,
+        secret,
+      )
       .send();
     const receipt = await tx.wait();
     expect(receipt.status).toBe(TxStatus.MINED);
@@ -241,9 +249,9 @@ export class CrossChainTestHarness {
   async withdrawPrivateFromAztecToL1(withdrawAmount: bigint, nonce: Fr = Fr.ZERO) {
     const withdrawTx = this.l2Bridge.methods
       .exit_to_l1_private(
+        { address: this.ethAccount.toField() },
         { address: this.l2Token.address },
         withdrawAmount,
-        { address: this.ethAccount.toField() },
         { address: EthAddress.ZERO.toField() },
         nonce,
       )
@@ -255,8 +263,8 @@ export class CrossChainTestHarness {
   async withdrawPublicFromAztecToL1(withdrawAmount: bigint, nonce: Fr = Fr.ZERO) {
     const withdrawTx = this.l2Bridge.methods
       .exit_to_l1_public(
-        withdrawAmount,
         { address: this.ethAccount.toField() },
+        withdrawAmount,
         { address: EthAddress.ZERO.toField() },
         nonce,
       )

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

@@ -4,7 +4,7 @@ mod token_interface;
 // Minimal implementation of the token bridge that can move funds between L1 <> L2.
 // The bridge has a corresponding Portal contract on L1 that it is attached to 
 // And corresponds to a Token on L2 that uses the `AuthWit` accounts pattern.
-// Bridge has to be set as a minter on the token before it can be sued
+// Bridge has to be set as a minter on the token before it can be used
 
 contract TokenBridge {
     use dep::aztec::{
@@ -50,13 +50,13 @@ contract TokenBridge {
     fn claim_public(
         to: AztecAddress,
         amount: Field,
+        canceller: EthereumAddress,
         msg_key: Field,
         secret: Field,
-        canceller: EthereumAddress,
     ) -> Field {
         let storage = Storage::init(Context::public(&mut context));
 
-        let content_hash = get_mint_public_content_hash(amount, to.address, canceller.address);
+        let content_hash = get_mint_public_content_hash(to.address, amount, canceller.address);
         // Consume message and emit nullifier
         context.consume_l1_to_l2_message(msg_key, content_hash, secret);
 
@@ -70,15 +70,15 @@ contract TokenBridge {
     // Requires `msg.sender` to give approval to the bridge to burn tokens on their behalf using witness signatures 
     #[aztec(public)]
     fn exit_to_l1_public(
-        amount: Field,
         recipient: EthereumAddress, // ethereum address to withdraw to
+        amount: Field,
         callerOnL1: EthereumAddress, // ethereum address that can call this function on the L1 portal (0x0 if anyone can call)
-        nonce: Field,
+        nonce: Field, // nonce used in the approval message by `msg.sender` to let bridge burn their tokens on L2
     ) -> Field {
         let storage = Storage::init(Context::public(&mut context));
 
         // Send an L2 to L1 message
-        let content = get_withdraw_content_hash(amount, recipient.address, callerOnL1.address);
+        let content = get_withdraw_content_hash(recipient.address, amount, callerOnL1.address);
         context.message_portal(content);     
 
         // Burn tokens 
@@ -92,19 +92,19 @@ contract TokenBridge {
     #[aztec(private)]
     fn claim_private(
         amount: Field,
-        msg_key: Field, // L1 to L2 message key as derived from the inbox contract
-        secret_for_L1_to_L2_message_consumption: Field, // secret used to consume the L1 to L2 message
         secret_hash_for_redeeming_minted_notes: Field, // secret hash used to redeem minted notes at a later time. This enables anyone to call this function and mint tokens to a user on their behalf
         canceller: EthereumAddress,
+        msg_key: Field, // L1 to L2 message key as derived from the inbox contract
+        secret_for_L1_to_L2_message_consumption: Field, // secret used to consume the L1 to L2 message
     ) -> Field {
         // Consume L1 to L2 message and emit nullifier
         let content_hash = get_mint_private_content_hash(amount, secret_hash_for_redeeming_minted_notes, canceller.address);
         context.consume_l1_to_l2_message(msg_key, content_hash, secret_for_L1_to_L2_message_consumption);
 
         // Mint tokens on L2 
-        // We hash the secret privately and send the hash in a public call so the secret isn't leaked
-        // Furthermore, `mint_private` on token is public. So we can an internal public function 
-        // which then calls the token contract
+        // `mint_private` on token is public. So we call an internal public function 
+        // which then calls the public method on the token contract.
+        // Since the secret_hash is passed, no secret is leaked. 
         context.call_public_function(
             context.this_address(),
             compute_selector("_call_mint_on_token(Field,Field)"),
@@ -115,17 +115,17 @@ contract TokenBridge {
     }
 
     // Burns the appropriate amount of tokens and creates a L2 to L1 withdraw message privately
-    // Requires `from` to give approval to the bridge to burn tokens on their behalf using witness signatures 
+    // Requires `msg.sender` (caller of the method) to give approval to the bridge to burn tokens on their behalf using witness signatures 
     #[aztec(private)]
     fn exit_to_l1_private(
+        recipient: EthereumAddress, // ethereum address to withdraw to
         token: AztecAddress,
         amount: Field,
-        recipient: EthereumAddress, // ethereum address to withdraw to
         callerOnL1: EthereumAddress, // ethereum address that can call this function on the L1 portal (0x0 if anyone can call)
-        nonce: Field,
+        nonce: Field, // nonce used in the approval message by `msg.sender` to let bridge burn their tokens on L2
     ) -> Field {
         // Send an L2 to L1 message
-        let content = get_withdraw_content_hash(amount, recipient.address, callerOnL1.address);
+        let content = get_withdraw_content_hash(recipient.address, amount, callerOnL1.address);
         context.message_portal(content);
 
         // Assert that user provided token address is same as seen in storage. 

yarn-project/noir-contracts/src/contracts/token_bridge_contract/src/util.nr

@@ -49,7 +49,7 @@ fn get_mint_private_content_hash(amount: Field, secret_hash_for_redeeming_minted
 
 // Computes a content hash of a deposit/mint_public message.
 // Refer TokenPortal.sol for reference on L1.
-fn get_mint_public_content_hash(amount: Field, owner_address: Field, canceller: Field) -> Field {
+fn get_mint_public_content_hash(owner_address: Field, amount: Field, canceller: Field) -> Field {
     let mut hash_bytes: [u8; 100] = [0; 100];
     let amount_bytes = amount.to_be_bytes(32);
     let recipient_bytes = owner_address.to_be_bytes(32);
@@ -86,7 +86,7 @@ fn get_mint_public_content_hash(amount: Field, owner_address: Field, canceller:
 }
 
 // Computes a content hash of a withdraw message.
-fn get_withdraw_content_hash(amount: Field, recipient: Field, callerOnL1: Field) -> Field {
+fn get_withdraw_content_hash(recipient: Field, amount: Field, callerOnL1: Field) -> Field {
     // Compute the content hash
     // Compute sha256(selector || amount || recipient)
     // then convert to a single field element