feat(token portal standard): Create a token portal standard by rahul-kothari · Pull Request #2351 · AztecProtocol/aztec-packages

rahul-kothari · 2023-09-17T14:10:28Z

Part of #2167 - creates the tokenPortal Standard + updates cross chain tests (and cross chain harness file)
Doc is TBD

moved address types to aztec-nr
Created Replace NonNativeToken everywhere + update token portal #2291 - where I will purge NonNativeToken
In a future PR, I will update uniswap portal too. For now I have skipped the uniswap test (to keep this PR small-ish) as it uses crosschainHarness which is integral to the other cross-chain e2e tests
I have created a separate public/private flow for depositing to Portal (and therefore separate flow for cancelling such L1->L2 messages)

LHerskind · 2023-09-18T11:41:38Z

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

+
+    // Consumes a L1->L2 message and calls the token contract to mint the appropriate amount publicly
+    #[aztec(public)]
+    fn deposit_public(


The naming with deposit and withdraw can make it quite hard to figure out the direction. Like, am I depositing into L1 from this contract, or am I depositing into L2 using a prior deposit.

Think it would make sense to make it a bit more clear that this is the second part of a deposit as well.

Withdraw might become withdrawToL1 or exitToL1 or the like, and deposits becoming a claim of funds?

LHerskind · 2023-09-18T11:46:49Z

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

+    ) -> Field {
+        let storage = Storage::init(Context::public(&mut context));
+
+        let content_hash = get_mint_content_hash(amount, context.msg_sender(), canceller.address);


There should be two different content_hashes, one for public and one for private. Say you do:

bytes32 content = sha256ToField( abi.encodeWithSelector( "mint_public(bytes32,uint256,address)", to, amount, canceller );

And then a separate

bytes32 content = sha256ToField( abi.encodeWithSelector( "mint_private(uint256,bytes32,address)", amount, secretHash, canceller );

With these two, you will be unambigious on what you are calling. Also, it is easier for you to let someone else perform the deposit action on L2 on your behalf without, in private use the to that was part of the content, and for public use the secretHash for the secretHash that is used when minting the asset.

Note that this require two flows on the L1 contract as well.

For withdrawals, you can get away with just having one content hash function, as you cannot consume it in private or public when on L1, so no need for both options.

LHerskind · 2023-09-18T11:47:57Z

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

+
+    //  This way, user hashes their secret in private and only sends the hash in public
+    // meaning only user can `redeem_shield` at a later time with their secret.
+    #[aztec(public)]


Could be fine to add a comment in here that you are doing this as a public call because you need to read from public storage.

LHerskind · 2023-09-18T11:49:17Z

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

+    fn withdraw_private(
+        token: AztecAddress,
+        amount: Field,
+        recipient: EthereumAddress, // ethereum address to withdraw to


Good use of the types, makes it easier to spot if things make sense instead of Fields all over the place.

LHerskind · 2023-09-18T11:51:29Z

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

+        nonce: Field,
+    ) -> Field {
+        // Burn tokens
+        Token::at(token.address).burn(&mut context, context.msg_sender(), amount, nonce);


This could potentially be used to perform a re-entry before you are altering any of your local state. Should be moved to the bottom of the function to not allow it to influence much.

Note that, yes this is doing a public call, but it is not super clear from here, so better just follow checks-effects-interactions

LHerskind · 2023-09-18T11:52:39Z

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

+        hash_bytes[i + 68] = canceller_bytes[i];
+    }
+
+    // Function selector: 0xeeb73071 keccak256('mint(uint256,bytes32,address)')


Remember to alter these with the changes higher up.

LHerskind · 2023-09-18T11:52:58Z

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

        types::type_serialisation::field_serialisation::{
            FieldSerialisationMethods, FIELD_SERIALISED_LEN,
        },
+        types::address::{AztecAddress},


LHerskind · 2023-09-18T11:54:01Z

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

+}
+
+impl EthereumAddress {
+  fn new(address: Field) -> Self {


To be pedantic, we should probably constrain this to throw an error when using >160 bits. Can be constrained similarly to the SafeU120 just with more bits

LHerskind

Some nits but looks pretty nice 👍

l1-contracts/test/portals/TokenPortal.sol

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

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

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

l1-contracts/test/portals/TokenPortal.sol

… 20 bytes, rename fns in bridge for clarity

LHerskind · 2023-09-19T13:03:55Z

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

+    #[aztec(private)]
+    fn exit_to_l1_private(
+        recipient: EthereumAddress, // ethereum address to withdraw to
+        token: AztecAddress,


The token here is only really used for validation in here while the rest are passed into the hash. So I would prefer that we had that closer to the end, such that the tree values put into the hash are in a row, but practically don't matter. Should probably get a style-guide going for this kind of thing.

LHerskind

I'm ok with this as is. Had a minor comment on style, but probably better for us to do a style guide for that as well 🤷

🤖 I have created a release *beep* *boop* --- <details><summary>aztec-packages: 0.7.10</summary> ## [0.7.10](aztec-packages-v0.7.9...aztec-packages-v0.7.10) (2023-09-20) ### Features * Aztec-cli unbox "really empty box" ([#2388](#2388)) ([b57182d](b57182d)) * **docs:** Document noir macros ([#2016](#2016)) ([1f1a17f](1f1a17f)) * **docs:** Include aztec rpc interface typedoc output in docs ([#2255](#2255)) ([62c9e9b](62c9e9b)) * **token portal standard:** Create a token portal standard ([#2351](#2351)) ([426a3ea](426a3ea)) ### Bug Fixes * **build:** Fix build system post deployment tests ([#2420](#2420)) ([d509dc3](d509dc3)) * CLI encoding for arrays and structs ([#2407](#2407)) ([85283bd](85283bd)) * Correct sandbox addresses in up-quick-start test ([#2412](#2412)) ([974d859](974d859)) * **docs:** Revert include aztec rpc interface typedoc output in docs ([#2255](#2255)) ([f852432](f852432)) * Handle falsy bigints in json-rpc ([#2403](#2403)) ([d100650](d100650)), closes [#2402](#2402) * **nargo_check.sh:** UNIX standard grep ([#2396](#2396)) ([02e788a](02e788a)) ### Miscellaneous * **docs:** Note getter options ([#2411](#2411)) ([8a95d8c](8a95d8c)) * Update docs url in config ([#2386](#2386)) ([e44066d](e44066d)) </details> <details><summary>barretenberg.js: 0.7.10</summary> ## [0.7.10](barretenberg.js-v0.7.9...barretenberg.js-v0.7.10) (2023-09-20) ### Miscellaneous * **barretenberg.js:** Synchronize aztec-packages versions </details> <details><summary>barretenberg: 0.7.10</summary> ## [0.7.10](barretenberg-v0.7.9...barretenberg-v0.7.10) (2023-09-20) ### Miscellaneous * **barretenberg:** Synchronize aztec-packages versions </details> --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).

rahul-kothari changed the title ~~Rk/token portal standard~~ feat(token portal standard): Create a token portal standard Sep 17, 2023

rahul-kothari force-pushed the rk/token_portal_standard branch from b3ca162 to 978ce04 Compare September 17, 2023 15:42

rahul-kothari requested review from LHerskind and Maddiaa0 September 18, 2023 08:28

LHerskind reviewed Sep 18, 2023

View reviewed changes

rahul-kothari force-pushed the rk/token_portal_standard branch 4 times, most recently from 8ff0425 to 8bab903 Compare September 18, 2023 16:43

LHerskind reviewed Sep 19, 2023

View reviewed changes

rahul-kothari force-pushed the rk/token_portal_standard branch from 8bab903 to 16a3b94 Compare September 19, 2023 10:03

rahul-kothari added 6 commits September 19, 2023 10:49

portal contract

a133cf0

update cross chain tests

55ca3e8

move address types to aztec-nr and minor ts updates

1868a43

create separate public, private mint paths, constain ethaddress to be…

ea7beaa

… 20 bytes, rename fns in bridge for clarity

add more unit tests

23bc9e6

respond to PR comments

e9e07ac

rahul-kothari force-pushed the rk/token_portal_standard branch from 16a3b94 to e9e07ac Compare September 19, 2023 10:49

LHerskind reviewed Sep 19, 2023

View reviewed changes

This was referenced Sep 19, 2023

Multi-day Sandbox deep dive Getting Started guide AztecProtocol/dev-rel#39

Closed

Document token portal standard AztecProtocol/dev-rel#55

Closed

LHerskind approved these changes Sep 19, 2023

View reviewed changes

rahul-kothari merged commit 426a3ea into master Sep 19, 2023

rahul-kothari deleted the rk/token_portal_standard branch September 19, 2023 14:17

AztecBot mentioned this pull request Sep 19, 2023

chore(master): Release 0.7.10 #2401

Merged

LHerskind mentioned this pull request Sep 22, 2023

Write token portal for Standard token. #2170

Closed

Conversation

rahul-kothari commented Sep 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

LHerskind left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LHerskind left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

rahul-kothari commented Sep 17, 2023 •

edited

Loading