feat: purge non native token + reorder params in token portal #2723

rahul-kothari · 2023-10-06T11:10:26Z

Fix Replace NonNativeToken everywhere + update token portal #2291
Create token_portal_content_hash_lib folder in noir-contracts that gets used by both token-bridge and test-contract.
reorder params in token portal and token bridge

Maddiaa0

have a small questions, overall looks great though!

Maddiaa0 · 2023-10-09T11:51:54Z

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

      const preimage = await buildL1ToL2Message(
-        'eeb73071',
-        [new Fr(bridgedAmount), recipient.toField(), canceller.toField()],
+        '25d46b0f',


If i recall correctly we have a utility method to calculate this? / Viem should, this code could be made more explicit by using that rather than using a string here

You should be able to use https://viem.sh/docs/utilities/getFunctionSelector.html#getfunctionselector

Maddiaa0 · 2023-10-09T11:53:42Z

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

-        params.map(a => a.toBuffer()),
-      );
-      const storageSlot = new Fr(2); // matches storage.nr
+      const expectedNoteHash = pedersenPlookupCommitInputs(wasm, [amount.toBuffer(), secretHash.toBuffer()]);


Did i read in daily updates that Jan is looking to remove all references to this, or was that another thing?

oo good catch - lemme figure that out

So Jan will end up covering over this in his PR - so I don't need to duplicate the effort here!

Just to give an update on this. We decided that we will keep on using direct pedersen calls for non-production code because it felt excessive to be implementing individual cbinds just for tests.

Maddiaa0 · 2023-10-09T12:00:37Z

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

@@ -236,7 +236,7 @@ describe('e2e_cross_chain_messaging', () => {
    await expect(
      l2Bridge
        .withWallet(user2Wallet)
-        .methods.claim_public(ownerAddress, bridgeAmount, ethAccount, messageKey, secretForL2MessageConsumption)
+        .methods.claim_public(bridgeAmount, ownerAddress, ethAccount, messageKey, secretForL2MessageConsumption)


Why the swap here between (to, amount) -> (amount, to)?

The erc20 standard is (to,amount) so this is likely what developers are used to

Currently, this is the only test where we do (to, amount). Everywhere else we do (amount, to). That said I agree with you - should use the erc20 standard.

Might be a bigger PR so might want to do it separately!

LHerskind

Some of the ordering of arguments is still a little funky, or at least following different flows whether deposit or claim which seems a little strange. I think having the address of the recipient first would be nicer, but at least it being consistent. Otherwise looks nice.

LHerskind · 2023-10-09T13:59:58Z

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

@@ -356,9 +356,9 @@ describe('ACIR public execution simulator', () => {
      const secret = new Fr(1n);
      const recipient = AztecAddress.random();

-      // Function selector: 0xeeb73071 keccak256('mint(uint256,bytes32,address)')
+      // Function selector: 0x63c9440d keccak256('mint_public(uint256,bytes32,address)')


Same here earlier, use the getFunctionSelector from viem.

LHerskind · 2023-10-09T14:05:11Z

yarn-project/noir-contracts/src/contracts/test_contract/src/utils.nr

+use dep::aztec::hash::{sha256_to_field};
+
+// Computes a content hash of a deposit/mint_public message.
+// Refer TokenPortal.sol for reference on L1.


This is a little funky. What benefit is there of having the same being reused for a test with the test_contract when testing it separately?

I couldn't call the appropriate methods in tokenbridge because the simulator would enqueue several other function calls across the token contract and the token_bridge contract and I would have to mock all of them, but they would emit their own nullifiers thus failing the point of the test.

Also for the utils, could you not just import it?

ah yea i could create a library for it

LHerskind · 2023-10-11T11:05:37Z

yarn-project/acir-simulator/package.json

@@ -36,7 +36,8 @@
    "@noir-lang/acvm_js": "0.26.1",
    "levelup": "^5.1.1",
    "memdown": "^6.1.1",
-    "tslib": "^2.4.0"
+    "tslib": "^2.4.0",
+    "viem": "^1.2.5"


I think you could have this in the devDependencies only? Are you not only using it in the test?

LHerskind · 2023-10-11T11:06:52Z

yarn-project/noir-contracts/src/contracts/test_contract/src/utils.nr

+use dep::aztec::hash::{sha256_to_field};
+
+// Computes a content hash of a deposit/mint_public message.
+// Refer TokenPortal.sol for reference on L1.


Also for the utils, could you not just import it?

LHerskind

Mostly looks fine.

Think the ordering should be addressed, since you changed it in your logic here, but might be nicer to do separately as you say.

Also address the failing test.

AztecBot · 2023-10-11T13:21:02Z

Benchmark results

All benchmarks are run on txs on the Benchmarking contract on the repository. Each tx consists of a batch call to create_note and increment_balance, which guarantees that each tx has a private call, a nested private call, a public call, and a nested public call, as well as an emitted private note, an unencrypted log, and public storage read and write.

This benchmark source data is available in JSON format on S3 here.

Values are compared against data from master at commit 082ab56d and shown if the difference exceeds 1%.

L2 block published to L1

Each column represents the number of txs on an L2 block published to L1.

Metric	8 txs	32 txs	128 txs
l1_rollup_calldata_size_in_bytes	45,444	179,588	716,132
l1_rollup_calldata_gas	222,828	867,764	3,449,588
l1_rollup_execution_gas	841,915	3,594,872	22,204,957
l2_block_processing_time_in_ms	⚠️ 1,248 (+21%)	⚠️ 4,753 (+20%)	⚠️ 18,812 (+20%)
note_successful_decrypting_time_in_ms	⚠️ 384 (+17%)	⚠️ 1,232 (+21%)	⚠️ 4,620 (+22%)
note_trial_decrypting_time_in_ms	⚠️ 87.0 (+190%)	110 (+3%)	147 (+7%)
l2_block_building_time_in_ms	⚠️ 11,057 (+23%)	⚠️ 44,361 (+23%)	⚠️ 187,125 (+23%)
l2_block_rollup_simulation_time_in_ms	⚠️ 8,284 (+24%)	⚠️ 33,264 (+24%)	⚠️ 132,138 (+24%)
l2_block_public_tx_process_time_in_ms	⚠️ 2,725 (+20%)	⚠️ 10,941 (+20%)	⚠️ 54,173 (+22%)

L2 chain processing

Each column represents the number of blocks on the L2 chain where each block has 16 txs.

Metric	5 blocks	10 blocks
node_history_sync_time_in_ms	14,137 (-10%)	31,031 (-8%)
note_history_successful_decrypting_time_in_ms	⚠️ 2,288 (-21%)	⚠️ 4,641 (-22%)
note_history_trial_decrypting_time_in_ms	122 (-2%)	147 (-6%)
node_database_size_in_bytes	1,648,517	1,190,286
pxe_database_size_in_bytes	27,188	54,187

Circuits stats

Stats on running time and I/O sizes collected for every circuit run across all benchmarks.

Circuit	circuit_simulation_time_in_ms	circuit_input_size_in_bytes	circuit_output_size_in_bytes
private-kernel-init	48.2	56,577	14,745
private-kernel-ordering	23.9	20,137	8,089
base-rollup	947 (+1%)	631,605	811
root-rollup	41.3 (-2%)	4,072	1,097
private-kernel-inner	40.9 (+1%)	72,288	14,745
public-kernel-private-input	52.6 (+2%)	37,359	14,745
public-kernel-non-first-iteration	31.1	37,401	14,745
merge-rollup	1.11 (+1%)	2,592	873

benesjan

Looks good to me

benesjan · 2023-10-12T06:48:46Z

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

-        params.map(a => a.toBuffer()),
-      );
-      const storageSlot = new Fr(2); // matches storage.nr
+      const expectedNoteHash = pedersenPlookupCommitInputs(wasm, [amount.toBuffer(), secretHash.toBuffer()]);


Just to give an update on this. We decided that we will keep on using direct pedersen calls for non-production code because it felt excessive to be implementing individual cbinds just for tests.

LHerskind

🤖 I have created a release *beep* *boop* --- <details><summary>aztec-packages: 0.8.11</summary> ## [0.8.11](aztec-packages-v0.8.10...aztec-packages-v0.8.11) (2023-10-13) ### Features * **archiver:** Use registry to fetch searchStartBlock ([#2830](#2830)) ([e5bc067](e5bc067)) * Configure sandbox for network ([#2818](#2818)) ([d393a59](d393a59)) * **docker-sandbox:** Allow forks in sandbox ([#2831](#2831)) ([ed8431c](ed8431c)), closes [#2726](#2726) * Goblin Translator Decomposition relation (Goblin Translator part 4) ([#2802](#2802)) ([3c3cd9f](3c3cd9f)) * Goblin Translator GenPermSort relation (Goblin Translator part 3) ([#2795](#2795)) ([b36fdc4](b36fdc4)) * Goblin translator opcode constraint and accumulator transfer relations (Goblin Translator part 5) ([#2805](#2805)) ([b3d1f28](b3d1f28)) * Goblin Translator Permutation relation (Goblin Translator part 2) ([#2790](#2790)) ([9a354c9](9a354c9)) * Integrate ZeroMorph into Honk ([#2774](#2774)) ([ea86869](ea86869)) * Purge non native token + reorder params in token portal ([#2723](#2723)) ([447dade](447dade)) * Throw compile error if read/write public state from private ([#2804](#2804)) ([a3649df](a3649df)) * Unencrypted log filtering ([#2600](#2600)) ([7ae554a](7ae554a)), closes [#1498](#1498) [#1500](#1500) * Update goblin translator circuit builder (Goblin Translator part 1) ([#2764](#2764)) ([32c69ae](32c69ae)) ### Bug Fixes * Outdated `noir:clean` ([#2821](#2821)) ([2ea199f](2ea199f)) ### Miscellaneous * Benchmark tx sizes in p2p pool ([#2810](#2810)) ([f63219c](f63219c)) * Change acir_tests branch to point to master ([#2815](#2815)) ([73f229d](73f229d)) * Fix typo ([#2839](#2839)) ([5afdf91](5afdf91)) * From < genesis allowed in getBlocks ([#2816](#2816)) ([5622b50](5622b50)) * Remove Ultra Grumpkin flavor ([#2825](#2825)) ([bde77b8](bde77b8)) * Remove work queue from honk ([#2814](#2814)) ([bca7d12](bca7d12)) * Spell check ([#2817](#2817)) ([4777a11](4777a11)) ### Documentation * Slight changes to update portal page ([#2799](#2799)) ([eb65819](eb65819)) * Update aztec_connect_sunset.mdx ([#2808](#2808)) ([5f659a7](5f659a7)) </details> <details><summary>barretenberg.js: 0.8.11</summary> ## [0.8.11](barretenberg.js-v0.8.10...barretenberg.js-v0.8.11) (2023-10-13) ### Miscellaneous * **barretenberg.js:** Synchronize aztec-packages versions </details> <details><summary>barretenberg: 0.8.11</summary> ## [0.8.11](barretenberg-v0.8.10...barretenberg-v0.8.11) (2023-10-13) ### Features * Goblin Translator Decomposition relation (Goblin Translator part 4) ([#2802](#2802)) ([3c3cd9f](3c3cd9f)) * Goblin Translator GenPermSort relation (Goblin Translator part 3) ([#2795](#2795)) ([b36fdc4](b36fdc4)) * Goblin translator opcode constraint and accumulator transfer relations (Goblin Translator part 5) ([#2805](#2805)) ([b3d1f28](b3d1f28)) * Goblin Translator Permutation relation (Goblin Translator part 2) ([#2790](#2790)) ([9a354c9](9a354c9)) * Integrate ZeroMorph into Honk ([#2774](#2774)) ([ea86869](ea86869)) * Update goblin translator circuit builder (Goblin Translator part 1) ([#2764](#2764)) ([32c69ae](32c69ae)) ### Miscellaneous * Change acir_tests branch to point to master ([#2815](#2815)) ([73f229d](73f229d)) * Remove Ultra Grumpkin flavor ([#2825](#2825)) ([bde77b8](bde77b8)) * Remove work queue from honk ([#2814](#2814)) ([bca7d12](bca7d12)) * Spell check ([#2817](#2817)) ([4777a11](4777a11)) </details> --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).

🤖 I have created a release *beep* *boop* --- <details><summary>aztec-packages: 0.8.11</summary> ## [0.8.11](AztecProtocol/aztec-packages@aztec-packages-v0.8.10...aztec-packages-v0.8.11) (2023-10-13) ### Features * **archiver:** Use registry to fetch searchStartBlock ([#2830](AztecProtocol/aztec-packages#2830)) ([e5bc067](AztecProtocol/aztec-packages@e5bc067)) * Configure sandbox for network ([#2818](AztecProtocol/aztec-packages#2818)) ([d393a59](AztecProtocol/aztec-packages@d393a59)) * **docker-sandbox:** Allow forks in sandbox ([#2831](AztecProtocol/aztec-packages#2831)) ([ed8431c](AztecProtocol/aztec-packages@ed8431c)), closes [#2726](AztecProtocol/aztec-packages#2726) * Goblin Translator Decomposition relation (Goblin Translator part 4) ([#2802](AztecProtocol/aztec-packages#2802)) ([3c3cd9f](AztecProtocol/aztec-packages@3c3cd9f)) * Goblin Translator GenPermSort relation (Goblin Translator part 3) ([#2795](AztecProtocol/aztec-packages#2795)) ([b36fdc4](AztecProtocol/aztec-packages@b36fdc4)) * Goblin translator opcode constraint and accumulator transfer relations (Goblin Translator part 5) ([#2805](AztecProtocol/aztec-packages#2805)) ([b3d1f28](AztecProtocol/aztec-packages@b3d1f28)) * Goblin Translator Permutation relation (Goblin Translator part 2) ([#2790](AztecProtocol/aztec-packages#2790)) ([9a354c9](AztecProtocol/aztec-packages@9a354c9)) * Integrate ZeroMorph into Honk ([#2774](AztecProtocol/aztec-packages#2774)) ([ea86869](AztecProtocol/aztec-packages@ea86869)) * Purge non native token + reorder params in token portal ([#2723](AztecProtocol/aztec-packages#2723)) ([447dade](AztecProtocol/aztec-packages@447dade)) * Throw compile error if read/write public state from private ([#2804](AztecProtocol/aztec-packages#2804)) ([a3649df](AztecProtocol/aztec-packages@a3649df)) * Unencrypted log filtering ([#2600](AztecProtocol/aztec-packages#2600)) ([7ae554a](AztecProtocol/aztec-packages@7ae554a)), closes [#1498](AztecProtocol/aztec-packages#1498) [#1500](AztecProtocol/aztec-packages#1500) * Update goblin translator circuit builder (Goblin Translator part 1) ([#2764](AztecProtocol/aztec-packages#2764)) ([32c69ae](AztecProtocol/aztec-packages@32c69ae)) ### Bug Fixes * Outdated `noir:clean` ([#2821](AztecProtocol/aztec-packages#2821)) ([2ea199f](AztecProtocol/aztec-packages@2ea199f)) ### Miscellaneous * Benchmark tx sizes in p2p pool ([#2810](AztecProtocol/aztec-packages#2810)) ([f63219c](AztecProtocol/aztec-packages@f63219c)) * Change acir_tests branch to point to master ([#2815](AztecProtocol/aztec-packages#2815)) ([73f229d](AztecProtocol/aztec-packages@73f229d)) * Fix typo ([#2839](AztecProtocol/aztec-packages#2839)) ([5afdf91](AztecProtocol/aztec-packages@5afdf91)) * From < genesis allowed in getBlocks ([#2816](AztecProtocol/aztec-packages#2816)) ([5622b50](AztecProtocol/aztec-packages@5622b50)) * Remove Ultra Grumpkin flavor ([#2825](AztecProtocol/aztec-packages#2825)) ([bde77b8](AztecProtocol/aztec-packages@bde77b8)) * Remove work queue from honk ([#2814](AztecProtocol/aztec-packages#2814)) ([bca7d12](AztecProtocol/aztec-packages@bca7d12)) * Spell check ([#2817](AztecProtocol/aztec-packages#2817)) ([4777a11](AztecProtocol/aztec-packages@4777a11)) ### Documentation * Slight changes to update portal page ([#2799](AztecProtocol/aztec-packages#2799)) ([eb65819](AztecProtocol/aztec-packages@eb65819)) * Update aztec_connect_sunset.mdx ([#2808](AztecProtocol/aztec-packages#2808)) ([5f659a7](AztecProtocol/aztec-packages@5f659a7)) </details> <details><summary>barretenberg.js: 0.8.11</summary> ## [0.8.11](AztecProtocol/aztec-packages@barretenberg.js-v0.8.10...barretenberg.js-v0.8.11) (2023-10-13) ### Miscellaneous * **barretenberg.js:** Synchronize aztec-packages versions </details> <details><summary>barretenberg: 0.8.11</summary> ## [0.8.11](AztecProtocol/aztec-packages@barretenberg-v0.8.10...barretenberg-v0.8.11) (2023-10-13) ### Features * Goblin Translator Decomposition relation (Goblin Translator part 4) ([#2802](AztecProtocol/aztec-packages#2802)) ([3c3cd9f](AztecProtocol/aztec-packages@3c3cd9f)) * Goblin Translator GenPermSort relation (Goblin Translator part 3) ([#2795](AztecProtocol/aztec-packages#2795)) ([b36fdc4](AztecProtocol/aztec-packages@b36fdc4)) * Goblin translator opcode constraint and accumulator transfer relations (Goblin Translator part 5) ([#2805](AztecProtocol/aztec-packages#2805)) ([b3d1f28](AztecProtocol/aztec-packages@b3d1f28)) * Goblin Translator Permutation relation (Goblin Translator part 2) ([#2790](AztecProtocol/aztec-packages#2790)) ([9a354c9](AztecProtocol/aztec-packages@9a354c9)) * Integrate ZeroMorph into Honk ([#2774](AztecProtocol/aztec-packages#2774)) ([ea86869](AztecProtocol/aztec-packages@ea86869)) * Update goblin translator circuit builder (Goblin Translator part 1) ([#2764](AztecProtocol/aztec-packages#2764)) ([32c69ae](AztecProtocol/aztec-packages@32c69ae)) ### Miscellaneous * Change acir_tests branch to point to master ([#2815](AztecProtocol/aztec-packages#2815)) ([73f229d](AztecProtocol/aztec-packages@73f229d)) * Remove Ultra Grumpkin flavor ([#2825](AztecProtocol/aztec-packages#2825)) ([bde77b8](AztecProtocol/aztec-packages@bde77b8)) * Remove work queue from honk ([#2814](AztecProtocol/aztec-packages#2814)) ([bca7d12](AztecProtocol/aztec-packages@bca7d12)) * Spell check ([#2817](AztecProtocol/aztec-packages#2817)) ([4777a11](AztecProtocol/aztec-packages@4777a11)) </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 requested review from LHerskind and Maddiaa0 October 6, 2023 11:10

rahul-kothari changed the title ~~purge non native~~ feat: purge non native token Oct 6, 2023

rahul-kothari force-pushed the rk/purge_non_native branch 5 times, most recently from 08db3b7 to 11f2ec4 Compare October 9, 2023 11:26

Maddiaa0 reviewed Oct 9, 2023

View reviewed changes

LHerskind reviewed Oct 9, 2023

View reviewed changes

rahul-kothari force-pushed the rk/purge_non_native branch from 11f2ec4 to 59f3579 Compare October 10, 2023 16:15

rahul-kothari requested a review from LHerskind October 10, 2023 16:55

rahul-kothari force-pushed the rk/purge_non_native branch 2 times, most recently from e0a02a4 to df45758 Compare October 11, 2023 10:56

LHerskind reviewed Oct 11, 2023

View reviewed changes

rahul-kothari force-pushed the rk/purge_non_native branch 5 times, most recently from 54bf4a5 to d9337dd Compare October 11, 2023 16:09

rahul-kothari added 6 commits October 11, 2023 16:09

purge non native

900bee6

reorder params in bridge

74a21b8

fix e2e tests

5118fd6

use fn selector

06bd42f

create separate lib for token portal content hash

e122e23

reorder (to,amount) across sol, hashes, nr and ts

2923073

rahul-kothari force-pushed the rk/purge_non_native branch from d9337dd to 2923073 Compare October 11, 2023 16:09

rahul-kothari requested a review from LHerskind October 11, 2023 16:45

rahul-kothari requested a review from benesjan October 11, 2023 16:45

rahul-kothari changed the title ~~feat: purge non native token~~ feat: purge non native token + reorder params in token portal Oct 11, 2023

benesjan approved these changes Oct 12, 2023

View reviewed changes

LHerskind approved these changes Oct 12, 2023

View reviewed changes

rahul-kothari merged commit 447dade into master Oct 12, 2023

rahul-kothari deleted the rk/purge_non_native branch October 12, 2023 09:35

AztecBot mentioned this pull request Oct 12, 2023

chore(master): Release 0.8.11 #2811

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: purge non native token + reorder params in token portal #2723

feat: purge non native token + reorder params in token portal #2723

rahul-kothari commented Oct 6, 2023 •

edited

Loading

Maddiaa0 left a comment •

edited

Loading

Maddiaa0 Oct 9, 2023

LHerskind Oct 9, 2023

Maddiaa0 Oct 9, 2023

rahul-kothari Oct 9, 2023

rahul-kothari Oct 9, 2023

benesjan Oct 12, 2023

Maddiaa0 Oct 9, 2023

rahul-kothari Oct 10, 2023

LHerskind left a comment

LHerskind Oct 9, 2023

LHerskind Oct 9, 2023

rahul-kothari Oct 10, 2023

LHerskind Oct 11, 2023

rahul-kothari Oct 11, 2023

LHerskind Oct 11, 2023

LHerskind Oct 11, 2023

LHerskind left a comment

AztecBot commented Oct 11, 2023 •

edited

Loading

benesjan left a comment

benesjan Oct 12, 2023

LHerskind left a comment

feat: purge non native token + reorder params in token portal #2723

feat: purge non native token + reorder params in token portal #2723

Conversation

rahul-kothari commented Oct 6, 2023 • edited Loading

Maddiaa0 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LHerskind left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LHerskind left a comment

Choose a reason for hiding this comment

AztecBot commented Oct 11, 2023 • edited Loading

Benchmark results

L2 block published to L1

L2 chain processing

Circuits stats

benesjan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LHerskind left a comment

Choose a reason for hiding this comment

rahul-kothari commented Oct 6, 2023 •

edited

Loading

Maddiaa0 left a comment •

edited

Loading

AztecBot commented Oct 11, 2023 •

edited

Loading