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.

Sign up for GitHub

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

Jump to bottom

fix: token naming issue #9950

Merged
merged 2 commits into from
Nov 13, 2024
Merged

fix: token naming issue #9950

merged 2 commits into from
Nov 13, 2024

Conversation

benesjan
Copy link
Contributor

_prepare_transfer_to_private function was used even in flows which are not a transfer (typically during minting). For this reson I renamed the function to _prepare_private_balance_increase.

@benesjan Graphite App
Copy link
Contributor Author

benesjan commented Nov 13, 2024 •
edited
Loading

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @benesjan and the rest of your teammates on Graphite Graphite

@benesjan benesjan mentioned this pull request Nov 13, 2024
Merged
@benesjan benesjan requested a review from sklppy88 November 13, 2024 19:19
sklppy88
sklppy88 approved these changes Nov 13, 2024
View reviewed changes
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.

How married are you to the new name ? 😄

@benesjan
Copy link
Contributor Author

How married are you to the new name ? 😄

@sklppy88 do you hate it?

@AztecBot
Copy link
Collaborator

AztecBot commented Nov 13, 2024 •
edited
Loading

Docs Preview

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

@sklppy88
Copy link
Contributor

sklppy88 commented Nov 13, 2024 •
edited
Loading

How married are you to the new name ? 😄

@sklppy88 do you hate it?

Hate is a strong word for sure, but it feels a bit off. Can't think of anything better now and I do agree it's better than the last hence the approval.

prepare_mint_or_transfer may be okay with a modifier somewhere in there to make it more clear.

But I think a better longer term solution would be to—just like how we coined a new term for "Shared" state—make a public facing term that we can use when referring to "partial notes" that clearly invokes the idea of the thing as a whole with the term itself staying somewhat simple.

@benesjan benesjan force-pushed the 11-13-refactor_token_refunds_cleanup branch from 3d4724b to 4bb0b7d Compare November 13, 2024 19:49
@benesjan benesjan force-pushed the 11-13-fix_token_naming_issue branch from e6f37b9 to b241173 Compare November 13, 2024 19:49
@benesjan
Copy link
Contributor Author

prepare_mint_or_transfer

@sklppy88 you are being severely judged now.

@benesjan Graphite App
Copy link
Contributor Author

benesjan commented Nov 13, 2024 •
edited
Loading

Merge activity

  • Nov 13, 3:45 PM EST: A user started a stack merge that includes this pull request via Graphite.
  • Nov 13, 3:48 PM EST: Graphite rebased this pull request as part of a merge.
  • Nov 13, 4:37 PM EST: Graphite couldn't merge this PR because it was not satisfying all requirements (Failed CI: 'merge-check', 'e2e (e2e_fees_private_payments)').

@benesjan benesjan changed the base branch from 11-13-refactor_token_refunds_cleanup to graphite-base/9950 November 13, 2024 20:45
@benesjan benesjan changed the base branch from graphite-base/9950 to master November 13, 2024 20:46
@benesjan benesjan force-pushed the 11-13-fix_token_naming_issue branch from b241173 to f719697 Compare November 13, 2024 20:47
@benesjan benesjan force-pushed the 11-13-fix_token_naming_issue branch from f719697 to 6079ecc Compare November 13, 2024 21:17
This was referenced Nov 13, 2024
Closed
Closed
@benesjan benesjan enabled auto-merge (squash) November 13, 2024 21:18
@benesjan benesjan merged commit 5997c82 into master Nov 13, 2024
66 checks passed
@benesjan benesjan deleted the 11-13-fix_token_naming_issue branch November 13, 2024 21:45
@AztecBot AztecBot mentioned this pull request Nov 13, 2024
Merged
TomAFrench added a commit that referenced this pull request Nov 14, 2024
@TomAFrench
Merge branch 'master' into sync-noir
3bbe74d 
* 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
Reviewers

@sklppy88 sklppy88 sklppy88 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@benesjan @AztecBot @sklppy88