solana-program-library icon indicating copy to clipboard operation
solana-program-library copied to clipboard

[token-cli] Upgrade to clap-v3

Open samkim-crypto opened this issue 1 year ago • 3 comments

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 (https://github.com/clap-rs/clap/pull/4223, https://github.com/clap-rs/clap/issues/2150, https://github.com/clap-rs/clap/issues/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 https://github.com/solana-labs/solana-program-library/issues/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 https://github.com/solana-labs/solana/pull/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 avatar Mar 11 '24 01:03 samkim-crypto

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?

2501babe avatar Apr 29 '24 05:04 2501babe

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.

samkim-crypto avatar Apr 29 '24 07:04 samkim-crypto

I haven't been able to access my solan funds at they never came to my phantom wallet I even check Solflare but I added them to address beginning DYOC

cbates8979 avatar May 16 '24 00:05 cbates8979