Skip to content
This repository has been archived by the owner on Jan 10, 2025. It is now read-only.

[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 subscribe to this conversation on GitHub. Already have an account? Sign in.
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