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

[rust-legacy-cli] Add scaled ui amount extension #93

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

[rust-legacy-cli] Add scaled ui amount extension #93

wants to merge 8 commits into from

Conversation

samkim-crypto
Copy link
Contributor

@samkim-crypto samkim-crypto commented Jan 21, 2025 •
edited
Loading

Problem

The rust legacy cli does not yet have support for the scaled ui amount extension.

Summary of Changes

Added support for the scaled ui amount extension.

  • I think everything should be straightforward. One exception is how the unix timestamp should be taken in as input and how it should be printed out. Currently, the unix timestamp is just taken in as i64 and printed out as i64. If you have a different suggestion, please let me know.
  • The solana account-decoder in the monorepo does not yet have support for the scaled ui amount extension, so I wasn't able to add changes to print out the extension. I can wait until the the extension is supported or I can create an issue for this and then follow-up on a subsequent PR.

@samkim-crypto samkim-crypto marked this pull request as ready for review January 21, 2025 09:37
joncinque
joncinque reviewed Jan 24, 2025
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.

Mostly tiny nits, feel free to take or leave them. Thanks for doing this!

clients/cli/src/clap_app.rs Outdated
Comment on lines 898 to 900
Arg::with_name("ui_multiplier")
.long("ui-multiplier")
.value_name("UI_MULTIPLIER")
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: about being a little more verbose and say ui-amount-multiplier?

Suggested change
Arg::with_name("ui_multiplier")
.long("ui-multiplier")
.value_name("UI_MULTIPLIER")
Arg::with_name("ui_amount_multiplier")
.long("ui-amount-multiplier")
.value_name("MULTIPLIER")

clients/cli/src/clap_app.rs Outdated
@@ -2688,4 +2703,46 @@ pub fn app<'a>(
.arg(multisig_signer_arg())
.nonce_args(true)
)
.subcommand(
SubCommand::with_name(CommandName::UpdateUiMultiplier.into())
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: same here UpdateUiAmountMultiplier

clients/cli/src/clap_app.rs Outdated
Comment on lines 2719 to 2720
Arg::with_name("ui-multiplier")
.value_name("MULTIPLIER")
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: we typically use underscores in the arg name, but maybe we can simplify it to multiplier while we're at it?

Suggested change
Arg::with_name("ui-multiplier")
.value_name("MULTIPLIER")
Arg::with_name("multiplier")
.value_name("MULTIPLIER")

clients/cli/src/clap_app.rs Outdated Show resolved Hide resolved
clients/cli/src/clap_app.rs Show resolved Hide resolved
clients/cli/src/clap_app.rs Outdated
Comment on lines 2735 to 2736
Arg::with_name("multiplier_authority")
.long("multiplier-authority")
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: feel free to disagree, but since it's called ScaledUiAmount elsewhere, maybe we should mirror that

Suggested change
Arg::with_name("multiplier_authority")
.long("multiplier-authority")
Arg::with_name("ui_amount_authority")
.long("ui-amount-authority")

clients/cli/src/clap_app.rs Outdated
Comment on lines 2727 to 2731
Arg::with_name("timestamp")
.value_name("TIMESTAMP")
.takes_value(true)
.index(3)
.required(true)
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't feel too strongly about this, but what do you think about making this arg optional and defaulting the current time?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes I think this is a good idea. I updated in e8b982e.

clients/cli/tests/command.rs
Comment on lines +4377 to +4390
// disable authority
process_test_command(
&config,
payer,
&[
"spl-token",
CommandName::Authorize.into(),
&token_pubkey.to_string(),
"scaled-ui-amount",
"--disable",
],
)
.await
.unwrap();
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this check that the authority is None aftewards?

Copy link
Contributor Author

@samkim-crypto samkim-crypto Jan 25, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Good call! This check was missing for the confidential transfer test, so I added it there too as well as in the other PR for the pausable extension.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joncinque joncinque joncinque left review comments

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

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