Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor: token naming cleanup #9904

Merged
merged 7 commits into from
Nov 13, 2024

Conversation

benesjan
Copy link
Contributor

Naming of the token functions was outdated. This PR addresses that.

Copy link
Contributor Author

benesjan commented Nov 12, 2024

@benesjan benesjan force-pushed the 11-09-refactor_final_token_cleanup branch from 0b19ee1 to d14b00e Compare November 12, 2024 15:55
@benesjan benesjan force-pushed the 11-12-refactor_final_token_naming_cleanup branch from a1a49b8 to 9f29ed8 Compare November 12, 2024 15:56
@benesjan benesjan marked this pull request as ready for review November 12, 2024 15:56
@benesjan benesjan added the e2e-all CI: Enables this CI job. label Nov 12, 2024
@benesjan benesjan force-pushed the 11-09-refactor_final_token_cleanup branch from d14b00e to 955f2e0 Compare November 12, 2024 18:06
@benesjan benesjan marked this pull request as draft November 12, 2024 18:06
@benesjan benesjan force-pushed the 11-12-refactor_final_token_naming_cleanup branch 3 times, most recently from 805aed2 to 3e8bffe Compare November 12, 2024 19:07
@benesjan benesjan marked this pull request as ready for review November 12, 2024 19:13
@benesjan benesjan changed the title refactor: final token naming cleanup refactor: token naming cleanup Nov 12, 2024
@benesjan benesjan force-pushed the 11-09-refactor_final_token_cleanup branch from 74ddb10 to b8eb9ce Compare November 12, 2024 20:50
@benesjan benesjan force-pushed the 11-12-refactor_final_token_naming_cleanup branch from 3e8bffe to 21e94ff Compare November 12, 2024 20:50
Base automatically changed from 11-09-refactor_final_token_cleanup to master November 12, 2024 21:36
@benesjan benesjan force-pushed the 11-12-refactor_final_token_naming_cleanup branch from 7c683a6 to 19f63ed Compare November 12, 2024 21:48
@AztecBot
Copy link
Collaborator

AztecBot commented Nov 12, 2024

Docs Preview

Hey there! 👋 You can check your preview at https://6734ca405bb1e925b793d957--aztec-docs-dev.netlify.app

@benesjan benesjan force-pushed the 11-12-refactor_final_token_naming_cleanup branch from 19f63ed to e335c48 Compare November 13, 2024 13:18
@benesjan benesjan mentioned this pull request Nov 13, 2024
@benesjan benesjan force-pushed the 11-12-refactor_final_token_naming_cleanup branch from e335c48 to 1078983 Compare November 13, 2024 14:06
@@ -81,6 +81,8 @@ contract DepositToAztecPublic is Test {
uint256 amount = 100 ether;
uint256 expectedIndex = 2 ** Constants.L1_TO_L2_MSG_SUBTREE_HEIGHT;

// The purpose of including the function selector is to make the message unique to that specific call. Note that
// it has nothing to do with calling the function.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I remember finding the selector on line 89 confusing so I decided to include the comment here.

@@ -0,0 +1,107 @@
use crate::test::utils;
Copy link
Contributor Author

@benesjan benesjan Nov 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just copied these test cases from burn so does not need to be reviewed.

@@ -0,0 +1,89 @@
use crate::test::utils;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just copied these test cases from burn so does not need to be reviewed.

@@ -0,0 +1,64 @@
use crate::test::utils;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just copied these test cases from transfer_private so does not need to be reviewed.

use dep::aztec::test::helpers::cheatcodes;

#[test]
unconstrained fn transfer_private() {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These test cases were moved to transfer.nr

@@ -14,21 +14,24 @@ pub fn get_mint_public_content_hash(owner: AztecAddress, amount: Field) -> Field
hash_bytes[i + 36] = amount_bytes[i];
}

// Function selector: 0x3e87b9be keccak256('mint_public(bytes32,uint256)')
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This hardcoded selector bytes were bad so I replaced it with comptime keccak.

@@ -0,0 +1,85 @@
import { AztecAddress, CompleteAddress } from '@aztec/aztec.js';
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just copied these test cases from transfer_private so does not need to be reviewed.

@benesjan benesjan requested a review from sklppy88 November 13, 2024 14:30
Copy link
Contributor

github-actions bot commented Nov 13, 2024

Changes to circuit sizes

Generated at commit: 6b542ae1a05936bebf4b8a0091542a2fd5f7e815, compared to commit: 3830e2a81cca4ff4782000847aefe011fbb6ee0c

🧾 Summary (100% most significant diffs)

Program ACIR opcodes (+/-) % Circuit size (+/-) %
private_kernel_init +23 ❌ +0.11% +21 ❌ +0.06%
private_kernel_inner +26 ❌ +0.07% +22 ❌ +0.04%
private_kernel_tail +3 ❌ +0.07% +3 ❌ +0.02%
parity_base +3 ❌ +0.07% +3 ❌ +0.01%
private_kernel_tail_to_public +3 ❌ +0.02% +3 ❌ +0.01%
private_kernel_reset_4_4_4_4_4_4_4_4_1 +1 ❌ +0.00% +1 ❌ +0.00%
rollup_base_private +38 ❌ +0.01% +38 ❌ +0.00%
rollup_base_public +38 ❌ +0.01% +38 ❌ +0.00%
rollup_merge +4 ❌ +0.12% +4 ❌ +0.00%
rollup_root +4 ❌ +0.03% +4 ❌ +0.00%
rollup_block_merge +4 ❌ +0.03% +4 ❌ +0.00%
private_kernel_reset +1 ❌ +0.00% +1 ❌ +0.00%
rollup_block_root +4 ❌ +0.09% +4 ❌ +0.00%
parity_root +3 ❌ +0.06% +3 ❌ +0.00%

Full diff report 👇
Program ACIR opcodes (+/-) % Circuit size (+/-) %
private_kernel_init 21,726 (+23) +0.11% 34,887 (+21) +0.06%
private_kernel_inner 38,093 (+26) +0.07% 57,530 (+22) +0.04%
private_kernel_tail 4,493 (+3) +0.07% 13,045 (+3) +0.02%
parity_base 4,301 (+3) +0.07% 30,698 (+3) +0.01%
private_kernel_tail_to_public 18,852 (+3) +0.02% 31,300 (+3) +0.01%
private_kernel_reset_4_4_4_4_4_4_4_4_1 32,455 (+1) +0.00% 86,600 (+1) +0.00%
rollup_base_private 332,907 (+38) +0.01% 3,432,553 (+38) +0.00%
rollup_base_public 470,092 (+38) +0.01% 3,770,598 (+38) +0.00%
rollup_merge 3,419 (+4) +0.12% 1,909,446 (+4) +0.00%
rollup_root 11,989 (+4) +0.03% 1,940,906 (+4) +0.00%
rollup_block_merge 12,005 (+4) +0.03% 1,940,920 (+4) +0.00%
private_kernel_reset 84,097 (+1) +0.00% 618,117 (+1) +0.00%
rollup_block_root 4,489 (+4) +0.09% 2,863,252 (+4) +0.00%
parity_root 5,034 (+3) +0.06% 3,801,552 (+3) +0.00%

Copy link
Contributor

@sklppy88 sklppy88 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍. Probably want devrel to take a pass at the docs to remove any other dangling refs in a follow up PR.

@@ -84,8 +84,6 @@ Let's say you have some storage in public and want to move them into the private

So you have to create a custom note in the public domain that is not encrypted by some owner - we call such notes a "TransparentNote" since it is created in public, anyone can see the amount and the note is not encrypted by some owner.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pinning for dev rel

@benesjan benesjan force-pushed the 11-12-refactor_final_token_naming_cleanup branch from 1078983 to ec2eb91 Compare November 13, 2024 15:12
@benesjan benesjan enabled auto-merge (squash) November 13, 2024 15:20
@benesjan benesjan force-pushed the 11-12-refactor_final_token_naming_cleanup branch from ec2eb91 to f6de81d Compare November 13, 2024 15:28
Copy link
Contributor

Changes to public function bytecode sizes

Generated at commit: 6b542ae1a05936bebf4b8a0091542a2fd5f7e815, compared to commit: 3830e2a81cca4ff4782000847aefe011fbb6ee0c

🧾 Summary (100% most significant diffs)

Program Bytecode size in bytes (+/-) %
TokenBridge::claim_public +476 ❌ +3.82%
Test::public_dispatch +461 ❌ +2.33%
TokenBridge::public_dispatch +201 ❌ +0.92%
TokenBridge::exit_to_l1_public -230 ✅ -2.80%

Full diff report 👇
Program Bytecode size in bytes (+/-) %
TokenBridge::claim_public 12,946 (+476) +3.82%
Test::public_dispatch 20,276 (+461) +2.33%
TokenBridge::public_dispatch 22,156 (+201) +0.92%
TokenBridge::exit_to_l1_public 7,996 (-230) -2.80%

@benesjan benesjan merged commit d77fc30 into master Nov 13, 2024
100 checks passed
@benesjan benesjan deleted the 11-12-refactor_final_token_naming_cleanup branch November 13, 2024 16:04
stevenplatt pushed a commit that referenced this pull request Nov 13, 2024
TomAFrench added a commit that referenced this pull request Nov 14, 2024
* master: (245 commits)
  chore: pull signed bitshifts from sync PR (#9939)
  chore: pull frontend changes from sync PR (#9935)
  feat: separate bytecode logs from unencrypted logs (#9891)
  git subrepo push --branch=master noir-projects/aztec-nr
  git_subrepo.sh: Fix parent in .gitrepo file. [skip ci]
  chore: replace relative paths to noir-protocol-circuits
  git subrepo push --branch=master barretenberg
  fix: token naming issue (#9950)
  feat: multiplier oracle (#9902)
  refactor: token refunds cleanup (#9943)
  chore: Use stack based recursion instead of function recursion (#9947)
  feat: parallelize DIE pass (#9933)
  feat(avm): Simulator enforces integral tag for DIV and field tag for FDIV (#9944)
  chore(avm): bugfixing witness generation for add, sub, mul for FF (#9938)
  feat: Google Cloud Kubernetes cluster + AWS Firewall Rules (#9915)
  chore: nuking ancient redundant test (#9941)
  fix: include 'master' version for aztec-up scripts (#9940)
  chore: fixing test contract fixture (#9909)
  refactor: token naming cleanup (#9904)
  chore: pull SSA parser from sync PR (#9928)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
e2e-all CI: Enables this CI job.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants