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] Upgrade to clap-v3 #6376

Merged
merged 22 commits into from
Oct 30, 2024

Conversation

samkim-crypto
Copy link
Contributor

@samkim-crypto samkim-crypto commented Mar 11, 2024

fd6ee0e: Upgrade to clap version 3.2.23 and change dependency solana-clap-utils --> solana-clap-v3-utils

145285f, 515ddc4, ad1fa87: Lifetimes were removed from ArgMatches, Arg, and App with clap-v3 (clap-rs/clap#4223, clap-rs/clap#2150, clap-rs/clap#1041), so they were removed.

e6f43c6: min_values and max_values now take usize instead of u64, so I updated these inputs.

7f40b38: short now takes char instead of string, so I updated these inputs.

97b288c: The requirement on validator functions have been updated from F: Fn(String) -> ... to F: FnMut(&str) -> ..., so I updated the syntax for custom validator functions.

a7c5773ArgMatches::subcommand now returns Option<(&str, &ArgMatches)> instead of (&str, Option<&ArgMatches>), so I updated these.

79e11b6: possible_values is now more generic and does not automatically infer a string type for its inputs, so I updated to specifically cast to string.

d2a1271: Some validation functions have been deprecated in clap-v3-utils. However, removing these deprecated functions will require another set of large changes, so I added #![allow(deprecated)] for now. I created #6393 for this and will update in a subsequent PR.

cb8cc28: This part is a little bit tricky, but the validator syntax changed from clap-v2 to clap-v3.

  • In clap-v2, a custom validator function had to be of type Fn(String) -> Result<...>
  • In clap-v3, a custom validator function has to be of type FnMut(&str) -> Result<...>
    The existing custom validator functions have the following syntax:
fn custom_validate<T>(string: T) -> Result<...>
where
    T: AsRef<str> + Display

In clap-v2, adding Arg::validator(custom_validate) was not a problem because the generic T could be inferred to be String. However, in clap-v3, the generic T must be inferred to be &str and since &str is a borrowed type,&'a str for some fixed lifetime 'a. However, Arg::validator requires custom_validate to be of any lifetime, which causes rust to complain. It seems like adding a level of indirection by wrapping the custom validator function inside a closure allows the lifetime associated with T: &'a str to be flexibly chosen on the spot is a workaround this, so I made these changes in this commit.

0daf1a9: Clap-v3 requires positional parameters either all or none of the arguments, so I added indices to some arguments.

ff00bfd: There is currently a useless alias owner for specifying KEYPAIRs in some subcommands. If the alias owner is used to specify a keypair, the argument is interpreted as OWNER_ADDRESS (from owner_address_arg()), so it can never really be invoked. I removed this alias since clap-v3 complains.

0afb444: In clap-v3, positional arguments can't have long or short names (which makes sense!), so I removed these.

b613066: In clap v2, value_of and is_present returns None if the argument id is not previously specified in Arg. In contract, in clap v3, value_of panics if the argument id is not specified previously as Arg (see solana-labs/solana#33184). I added custom logic to account for this difference in versions. In some places, these changes are a bit clunky, but cleaning them up will require larger changes, so I will do it in subsequent PRs.

ac068bf: This commit is related to the previous commit. The functions signer_from_path and signer_from_path_with_config in solana-clap-v3-utils panics early in a special case that we cannot easily account for from the caller side. I added custom versions of these functions that specifically checks for this special case.

a863f08: cmd.args([...]) mutates cmd to include the specified arguments. Currently, cmd.args(args).assert()..stderr("error: Could not find config file ~/nonexistent/config.yml\n"); and cmd.assert().code(1).failure(); adds args to cmd twice, so I fixed this.

@samkim-crypto samkim-crypto added the WIP Work in progress label Mar 11, 2024
@samkim-crypto samkim-crypto removed the WIP Work in progress label Mar 12, 2024
@2501babe 2501babe self-requested a review March 12, 2024 12:19
@github-actions github-actions bot added the stale [bot only] Added to stale content; will be closed soon label Mar 27, 2024
@samkim-crypto samkim-crypto removed the stale [bot only] Added to stale content; will be closed soon label Mar 28, 2024
@github-actions github-actions bot added the stale [bot only] Added to stale content; will be closed soon label Apr 15, 2024
@samkim-crypto samkim-crypto added do-not-close Add this tag to exempt a PR / issue from being closed automatically and removed stale [bot only] Added to stale content; will be closed soon labels Apr 16, 2024
@samkim-crypto samkim-crypto force-pushed the token-cli/clap-v3 branch 2 times, most recently from e2d57ff to 07450df Compare April 28, 2024 07:30
@2501babe
Copy link
Member

i took a first read through and it seems pretty straightforward. i was wondering about the removal of the --owner aliases tho. is this because v3 doesnt support that? will that break existing commands that people might be using, or is there something im missing there?

@samkim-crypto
Copy link
Contributor Author

Thanks for taking a look! and sorry the details were a bit terse with the issue surrounding --owner.

The issue with --owner alias is that this conflicts with the owner_address_arg. If you look at the Unwrap, Close, and WithdrawWithheldTokens commands, there is .alias("owner"), but also .arg(owner_address_arg()), which both specify an argument --owner. If an --owner argument is provided, then clap interprets it as a pubkey argument from .arg(owner_address_arg()) instead of as an alias for a keypair. This makes the .alias("owner") useless.

Clap-v2 did not really complain about this (the alias was just useless). But clap-v3 is smart enough to catch this and gives an error when invoked, which is why I removed the alias. This should not break existing commands because it was a useless alias from the start.

@cbates8979

This comment was marked as spam.

Tyman14888

This comment was marked as spam.

Comment on lines 805 to 842
.validator(is_amount)
.value_parser(clap::value_parser!(f64))
Copy link
Member

Choose a reason for hiding this comment

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

can this parse a number without a decimal?

Copy link
Contributor Author

@samkim-crypto samkim-crypto Oct 22, 2024

Choose a reason for hiding this comment

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

Yes you are completely right! The validation is_amount should actually be replaced with using the Amount type, but I thought making this change in this PR would add on a whole new set of changes, so I just tried using the built-in clap value parser without thinking through it carefully. Thanks for the catch!

I added the is_amount validation back in and updated the processor logic to parse as string first and then convert it to f64 since value_of is deprecated. On a subsequent PR, I will replace the whole validator with Amount so that we can parse more ergonomically/cleanly instead of parsing as string.

Comment on lines 1399 to 1437
.validator(is_amount)
.value_parser(clap::value_parser!(f64))
Copy link
Member

Choose a reason for hiding this comment

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

same here, not sure if these are supposed to stay is_smount

@samkim-crypto samkim-crypto merged commit 60a7ffe into solana-labs:master Oct 30, 2024
31 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
do-not-close Add this tag to exempt a PR / issue from being closed automatically
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants