Bridging layer 2 native asset to L1 by LHerskind · Pull Request #552 · AztecProtocol/aztec-packages

LHerskind · 2023-05-12T15:25:58Z

Description

Working on #518, #519 towards #514

Please provide a paragraph or two giving a summary of the change, including relevant motivation and context.

Checklist:

I have reviewed my diff in github, line by line.
Every change is related to the PR description.
I have linked this pull request to the issue(s) that it resolves.
There are no unexpected formatting changes, superfluous debug logs, or commented-out code.
The branch has been merged or rebased against the head of its merge target.
I'm happy for the PR to be merged at the reviewer's next convenience.

Maddiaa0 · 2023-05-15T11:20:56Z

+            } else {
+                // @todo @LHerskind chain-ids and rollup version id should be added here. Right now, just hard coded.
+                new_l2_to_l1_msgs_to_insert[i] = compute_l2_to_l1_hash<NT>(storage_contract_address,
+                                                                           fr(1),  // rollup version id


should chainid and rollup version be global in constants.hpp

The rollup version should depend on the version of the rollup so should probably be in context or something, @iAmMichaelConnor we probably should talk to figure out where we should get this from.

Similarly, chainid should be from context. So again, lets talk to @iAmMichaelConnor.

Maddiaa0 · 2023-05-17T22:59:10Z

+
+        auto offset = i * 32;
+        std::copy(as_bytes.begin(), as_bytes.end(), calldata_hash_inputs_bytes.begin() + offset);
+    }


Is the point of this to pad all the values to 32 bytes? if theyre all already field elements of the same type would just copying it to the uint8 array in one go not be sufficient. Genuinely curious not sure if you can do it

I don't recall why it was done like this, think I just took what we had in the components 😅, so both might need an update.

Not sure if this is relevant to the topic raised Sean, but we would still need to still pad them to 32 bytes each. In solidity, before hashing, both L1Actor abd L2Actor is 64 bytes each (since they have 2 fields each) after doing abi.encode

Ok, I've made a draft issue in a3 faries to refactor this

Maddiaa0 · 2023-05-17T23:05:55Z

+    registry.setAddresses(address(rollup), address(inbox), address(outbox));
+
+    rna = new RollupNativeAsset();
+    vm.etch(address(0xbeef), address(rna).code);


guessing this is hardcoded for the entry key calculation?

Ye, this was just to match what was generated from the typescript, etching to make it a bit easier to match.

can you add a comment on why the etch?

Maddiaa0 · 2023-05-17T23:08:17Z

+import {Registry} from "@aztec/core/messagebridge/Registry.sol";
+
+contract RollupNativeAsset is ERC20 {
+  uint256 public constant P =


Proposal to make P a constant of DataStructures or the likes (ive done so here on an awaiting pr)

aztec-packages/l1-contracts/src/core/libraries/DataStructures.sol

Line 7 in f481b3d

uint256 constant P = 21888242871839275222246405745257275088696311157297823662689037894645226208583;

Yep, lets make a Constants.sol in there as well. Thinking if it also made sense to look at doing a library for errors, if full library is included in ABI generation, should provide more meaningful errors when simulating tx's in viem 🤔

Maddiaa0 · 2023-05-17T23:09:55Z

+
+const sha256ToField = (buf: Buffer): Fr => {
+  // Prime order of BN254 curve
+  const p = BigInt('21888242871839275222246405745257275088548364400416034343698204186575808495617');


Use Fr.MODULUS instead, its available as a static val

Maddiaa0 · 2023-05-17T23:10:22Z

+  let accounts: AztecAddress[];
+  let contract: Contract;
+  let portalAddress: EthAddress;
+  let portalContract: any; //<typeof RollupNativeAssetAbi, PublicClient<HttpTransport, Chain>>;


gotta love viem

Maddiaa0 · 2023-05-17T23:10:57Z

+    expect(balance).toBe(expectedBalance);
+  };
+
+  const pointToPublicKey = (point: Point) => {


This is used alot right? Should we make a helper function for it?

Will probably be useful to take this and a few other of the widely used End-To-End functions and move it into some utilities.

Maddiaa0 · 2023-05-17T23:18:20Z

-    //     }
-    // }
+    {
+        const auto& portal_contract_address = private_inputs.private_call.portal_contract_address;


Can a copy of this line commented out a few lines above be removed now?

Oh, ye will get rid of it.

Maddiaa0 · 2023-05-18T10:04:55Z

+
+        auto offset = i * 32;
+        std::copy(as_bytes.begin(), as_bytes.end(), calldata_hash_inputs_bytes.begin() + offset);
+    }


Ok, I've made a draft issue in a3 faries to refactor this

Maddiaa0 · 2023-05-18T10:13:02Z

+        const auto& new_l2_to_l1_msgs = private_call_public_inputs.new_l2_to_l1_msgs;
+        std::array<NT::fr, NEW_L2_TO_L1_MSGS_LENGTH> new_l2_to_l1_msgs_to_insert;
+        for (size_t i = 0; i < new_l2_to_l1_msgs.size(); ++i) {
+            if (new_l2_to_l1_msgs[i] == 0) {


You can call .is_zero() on fields

Maddiaa0 · 2023-05-18T10:13:54Z

+        std::array<NT::fr, NEW_L2_TO_L1_MSGS_LENGTH> new_l2_to_l1_msgs_to_insert;
+        for (size_t i = 0; i < new_l2_to_l1_msgs.size(); ++i) {
+            if (new_l2_to_l1_msgs[i] == 0) {
+                new_l2_to_l1_msgs_to_insert[i] = 0;


nit: similarly defining this to fr.zero is more expressive

rahul-kothari · 2023-05-18T10:01:57Z

+
+        auto offset = i * 32;
+        std::copy(as_bytes.begin(), as_bytes.end(), calldata_hash_inputs_bytes.begin() + offset);
+    }


Not sure if this is relevant to the topic raised Sean, but we would still need to still pad them to 32 bytes each. In solidity, before hashing, both L1Actor abd L2Actor is 64 bytes each (since they have 2 fields each) after doing abi.encode

rahul-kothari · 2023-05-18T10:06:54Z

+    registry.setAddresses(address(rollup), address(inbox), address(outbox));
+
+    rna = new RollupNativeAsset();
+    vm.etch(address(0xbeef), address(rna).code);


can you add a comment on why the etch?

rahul-kothari · 2023-05-18T10:08:34Z

+    outbox.sendL1Messages(entryKeys);
+
+    assertEq(rna.balanceOf(address(0xdead)), 0);
+    bytes32 entryKey = rna.withdraw(654, address(0xdead));


nit: also check that MessageConsumed event was fired

rahul-kothari · 2023-05-18T10:10:24Z

+} from 'viem';
+import { foundry } from 'viem/chains';
+
+const MNEMONIC = 'test test test test test test test test test test test junk';


to avoid a merge conflict with Sean's work, you can create a fixture.ts and import MNEMONIC from there! Ref

its all good im ok with refactoring it when i merge master

rahul-kothari · 2023-05-18T10:13:07Z

+    };
+  };
+
+  const deployContract = async (initialBalance = 0n, owner = { x: 0n, y: 0n }) => {


rename to deployNativeTokenContract. deployContract() is a bit vague (esp since this exact fn name is used across other E2Es too as far as I remember)

Its the same thing across the E2Es, part of what is referred to in #552 (comment)

rahul-kothari · 2023-05-18T10:15:08Z

+    await deployContract(initialBalance, pointToPublicKey(await aztecRpcServer.getAccountPublicKey(owner)));
+    await expectBalance(owner, initialBalance);
+
+    const { request: initRequest } = await publicClient.simulateContract({


why do we need to simulate contract? Is this ACVM stuff (which simulates before creating the kernel proof? For whatever reason I assumed this would be abstracted away)

This is viem simulating the call and then executing it.

rahul-kothari · 2023-05-18T10:16:13Z

+
+    const blockNumber = await node.getBlockHeight();
+    const blocks = await node.getBlocks(blockNumber, 1);
+    // If this is failing, it is likely because of wrong chain id


how do you set the right chain id?

Mainly that the two domains need to match, e.g., rollup and l1 need to specify the same chainid for messages to hit the expected hashes, if one use 1 and other use 31337 you won't find occurences of the messages correctly.

rahul-kothari · 2023-05-18T10:20:30Z

@@ -0,0 +1,51 @@
+use dep::aztec3::notes::value_note::Note;


For my understanding - whats the point of this file? Is all of this just syntactic sugar?

Its an implementation to getcompute the balance of a user.

rahul-kothari

amazing stuff!

LHerskind force-pushed the lh/l2-native-asset branch from 003ca53 to 83540ec Compare May 16, 2023 10:00

LHerskind added 11 commits May 17, 2023 15:24

fix: initial noir

d60b3dc

fix: rename because snakecase mess with me

2717028

chore: add rollup native asset contract to index

ebb347b

initial l2->l1 noir

4ceb8ec

fix: remove unneeded global

89d6aee

fix: rebase issues

5ccfeaf

chore: improve readability

a99ca17

feat: Partial message box for testing sol

c626ec6

chore: prettier

ad3ceff

chore: missing await

556b69f

fix: update chainids and hashing

571606c

LHerskind force-pushed the lh/l2-native-asset branch from 5e082f6 to 571606c Compare May 17, 2023 20:45

LHerskind changed the title ~~Bridging layer 2 native asset bridging to L1~~ Bridging layer 2 native asset to L1 May 17, 2023

This was linked to issues May 17, 2023

acir simulator to emit L2 -> L1 messages #518

Closed

Token bridge portal contract (l2 native asset) #519

Closed

fix: remove arrow + nit

edc407f

LHerskind marked this pull request as ready for review May 17, 2023 21:32

LHerskind requested review from Maddiaa0 and rahul-kothari May 17, 2023 21:37

Maddiaa0 reviewed May 17, 2023

View reviewed changes

fix: minor nits

d892807

AztecProtocol deleted a comment from LHerskind May 18, 2023

Maddiaa0 reviewed May 18, 2023

View reviewed changes

rahul-kothari reviewed May 18, 2023

View reviewed changes

LHerskind mentioned this pull request May 18, 2023

feat: bridge l1 assets to l2 acvm #578

Merged

6 tasks

LHerskind added 2 commits May 18, 2023 10:55

fix: skip when is_zero

fbc0f06

test: add comment + expect emit

fc3a197

rahul-kothari approved these changes May 18, 2023

View reviewed changes

Maddiaa0 approved these changes May 18, 2023

View reviewed changes

Maddiaa0 merged commit ddd3abd into master May 18, 2023

Maddiaa0 deleted the lh/l2-native-asset branch May 18, 2023 13:23

Conversation

LHerskind commented May 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Checklist:

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rahul-kothari left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

LHerskind commented May 12, 2023 •

edited

Loading