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

Confidential mint burn extension: Token program changes #7319

Merged
merged 42 commits into from
Oct 31, 2024
Merged

Confidential mint burn extension: Token program changes #7319

merged 42 commits into from
Oct 31, 2024

Conversation

abcalphabet
Copy link
Contributor

PR separating out only the token program changes from #6881 6881

@abcalphabet
confidential mint-burn extension
8fbb0fb
@abcalphabet
Merge branch 'master' into confidential_mint_burn_ext
de3d227
@abcalphabet
add ciphertext validity proof to confidential mint
32d528e
@abcalphabet
simplify burn proofs
3a9318d
@abcalphabet
add confidential supply
fee8acb
@abcalphabet
remove debug statements
5bbafec
@abcalphabet
add tests for confidential mint-burn
d6724a0
@abcalphabet
Merge branch 'master' into confidential_mint_burn_ext
0ec8537
@abcalphabet
fix clippy & serde tests
5939b72
@abcalphabet
Merge branch 'master' into confidential_mint_burn_ext
d4ad146
@abcalphabet
fix some tests
8479bad
@abcalphabet
Merge branch 'master' into confidential_mint_burn_ext
96d8f92
@abcalphabet
clippy
0b20da3
@abcalphabet
Merge branch 'master' into confidential_mint_burn_ext
48fcb02
@abcalphabet
mint burn with new proof generation
18aa58d
@abcalphabet
remove old mintburn proof generation
59234ba
@abcalphabet
cleanup
89c7cd2
@abcalphabet
remove cli / client changes and mint-burn tests
d46407c
@abcalphabet
cleanup
3944195
@abcalphabet
more cleanup
e010003
@mergify mergify bot added the community Community contribution label Oct 1, 2024
@abcalphabet abcalphabet mentioned this pull request Oct 1, 2024
Open
@samkim-crypto samkim-crypto self-requested a review October 2, 2024 03:15
samkim-crypto
samkim-crypto reviewed Oct 4, 2024
View reviewed changes
Copy link
Contributor

@samkim-crypto samkim-crypto left a comment

Choose a reason for hiding this comment

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

Looks good to me overall! Many of the comments below are minor stylistic suggestions for consistency with the rest of the code base. There have also been some changes to how we deal with proof accounts after your original PR was made, so I made some comments regarding that (I apologize for this 🙏 ). Let me know what you think!

token/program-2022/src/extension/confidential_mint_burn/mod.rs Outdated Show resolved Hide resolved
token/program-2022/src/extension/confidential_mint_burn/instruction.rs Outdated Show resolved Hide resolved
token/program-2022/src/extension/confidential_mint_burn/instruction.rs Show resolved Hide resolved
token/program-2022/src/extension/confidential_mint_burn/instruction.rs Outdated Show resolved Hide resolved
token/program-2022/src/extension/confidential_mint_burn/instruction.rs Outdated Show resolved Hide resolved
token/program-2022/src/extension/confidential_mint_burn/verify_proof.rs Show resolved Hide resolved
token/program-2022/src/extension/mod.rs Outdated Show resolved Hide resolved
token/program-2022/src/extension/confidential_transfer/processor.rs Show resolved Hide resolved
token/program-2022/src/extension/confidential_transfer/processor.rs Show resolved Hide resolved
token/confidential-transfer/proof-generation/src/mint.rs Outdated Show resolved Hide resolved
@samkim-crypto samkim-crypto self-requested a review October 5, 2024 00:04
samkim-crypto
samkim-crypto reviewed Oct 7, 2024
View reviewed changes
Copy link
Contributor

@samkim-crypto samkim-crypto left a comment

Choose a reason for hiding this comment

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

Looking really good, thanks 🙏 ! Just some comments on some things that I missed on my first pass and also things that have not yet resolved from the previous reviews.

token/confidential-transfer/proof-generation/src/supply.rs Outdated Show resolved Hide resolved
token/confidential-transfer/proof-generation/src/mint.rs Outdated Show resolved Hide resolved
token/program-2022/src/extension/confidential_mint_burn/instruction.rs Outdated Show resolved Hide resolved
token/program-2022/src/extension/confidential_mint_burn/instruction.rs Outdated Show resolved Hide resolved
token/program-2022/src/extension/confidential_mint_burn/instruction.rs Outdated Show resolved Hide resolved
@samkim-crypto samkim-crypto self-requested a review October 7, 2024 09:37
joncinque
joncinque reviewed Oct 17, 2024
View reviewed changes
Copy link
Contributor

@joncinque joncinque left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution! Mostly small things, and I might have misunderstood some bits, so feel free to ignore things that don't correspond to your discussions with Sam.

libraries/pod/src/optional_keys.rs Outdated Show resolved Hide resolved
token/program-2022/src/extension/confidential_mint_burn/instruction.rs Outdated Show resolved Hide resolved
token/program-2022/src/extension/confidential_mint_burn/instruction.rs Outdated Show resolved Hide resolved
token/program-2022/src/extension/confidential_mint_burn/instruction.rs Outdated Show resolved Hide resolved
token/program-2022/src/extension/confidential_mint_burn/instruction.rs Outdated Show resolved Hide resolved
token/program-2022/src/extension/confidential_mint_burn/processor.rs Outdated Show resolved Hide resolved
token/program-2022/src/extension/confidential_mint_burn/processor.rs Outdated Show resolved Hide resolved
token/program-2022/src/serialization.rs Outdated Show resolved Hide resolved
token/program-2022/src/serialization.rs Outdated Show resolved Hide resolved
token/program-2022/src/extension/confidential_mint_burn/instruction.rs Outdated Show resolved Hide resolved
joncinque
joncinque reviewed Oct 25, 2024
View reviewed changes
Copy link
Contributor

@joncinque joncinque left a comment

Choose a reason for hiding this comment

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

Thanks for integrating all the feedback! This is looking very close to me, just a few more bits

token/program-2022/src/processor.rs Show resolved Hide resolved
token/program-2022/src/extension/confidential_mint_burn/instruction.rs Outdated Show resolved Hide resolved
token/program-2022/src/extension/confidential_mint_burn/processor.rs Outdated Show resolved Hide resolved
token/program-2022/src/extension/confidential_mint_burn/processor.rs Outdated Show resolved Hide resolved
token/program-2022/src/extension/confidential_mint_burn/processor.rs Outdated Show resolved Hide resolved
token/program-2022/src/serialization.rs Outdated Show resolved Hide resolved
joncinque
joncinque reviewed Oct 29, 2024
View reviewed changes
Copy link
Contributor

@joncinque joncinque left a comment

Choose a reason for hiding this comment

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

Just a few last little bits from me, then this should be ready to go from my side! Thanks for your patience

token/program-2022/src/extension/confidential_mint_burn/processor.rs Outdated Show resolved Hide resolved
token/program-2022/src/extension/confidential_mint_burn/processor.rs Outdated Show resolved Hide resolved
token/program-2022/src/extension/confidential_mint_burn/processor.rs Outdated Show resolved Hide resolved
token/program-2022/src/processor.rs Outdated Show resolved Hide resolved
joncinque
joncinque approved these changes Oct 30, 2024
View reviewed changes
Copy link
Contributor

@joncinque joncinque left a comment

Choose a reason for hiding this comment

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

This looks good to me! I'll @samkim-crypto give the final approval and merge

samkim-crypto
samkim-crypto approved these changes Oct 31, 2024
View reviewed changes
Copy link
Contributor

@samkim-crypto samkim-crypto left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thanks so much for your contribution and patience throughout! Everything looks fine on my side. Let's add tests for the extension to program-2022-test in a follow-up PR to be extra sure! I'll merge this one once CI passes.

@samkim-crypto samkim-crypto merged commit 29eae22 into solana-labs:master Oct 31, 2024
35 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@samkim-crypto samkim-crypto samkim-crypto approved these changes

@joncinque joncinque joncinque approved these changes

Assignees
No one assigned
Labels
community Community contribution
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@abcalphabet @joncinque @samkim-crypto