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

[token-cli] Enable configuring confidential transfer account #5540

Conversation

samkim-crypto
Copy link
Contributor

@samkim-crypto samkim-crypto commented Oct 15, 2023

Problem

The token-cli does not yet support initializing confidential transfer account

Summary of changes

Added configuring cli commands for confidential transfer accounts. I think each commits should be pretty straightforward and independent with the following note:

  • For configure-confidential-transfer-account, I made it so that the elgamal and aes keys are derived from the signer by default. This can be updated so that users have the option to initialize the account with custom keys once we upgrade to clap-v3-utils.

@samkim-crypto samkim-crypto added the WIP Work in progress label Oct 15, 2023
@samkim-crypto samkim-crypto removed the WIP Work in progress label Oct 16, 2023
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.

Looks great! Just a few little comments, but this should be ready to go soon

Comment on lines 4657 to 4665
.arg(
Arg::with_name("account")
.validator(is_valid_pubkey)
.value_name("TOKEN_ACCOUNT_ADDRESS")
.takes_value(true)
.index(1)
.required(true)
.help("The address of the token account to configure confidential transfers for")
)
Copy link
Contributor

Choose a reason for hiding this comment

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

For all of these, similar to create-account, how about taking in a "mint" and using the owner's ATA for that mint if "account" isn't specified?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, that is a good idea. It turned out it is slightly clunky since account and token can all be optional. Please take a look. I think this suggestion also applies for all other commands that take in account as a required argument.

token/cli/src/main.rs Outdated Show resolved Hide resolved
token/cli/src/main.rs Outdated Show resolved Hide resolved
token/cli/src/main.rs Outdated Show resolved Hide resolved
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.

Sorry, one last bit to make these flags consistent with other commands

token/cli/src/main.rs Outdated Show resolved Hide resolved
token/cli/src/main.rs Outdated Show resolved Hide resolved
joncinque
joncinque previously approved these changes Oct 18, 2023
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.

I noticed a few of the help texts got changed, so just changing those back. After that, this is good to go!

token/cli/src/main.rs Outdated Show resolved Hide resolved
token/cli/src/main.rs Outdated Show resolved Hide resolved
token/cli/src/main.rs Outdated Show resolved Hide resolved
token/cli/src/main.rs Outdated Show resolved Hide resolved
@mergify mergify bot dismissed joncinque’s stale review October 18, 2023 22:17

Pull request has been modified.

@samkim-crypto
Copy link
Contributor Author

Oh man sorry for all the mess and hassle this PR and thanks for your patience.

@samkim-crypto samkim-crypto merged commit acabef3 into solana-labs:master Oct 18, 2023
29 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants