[clap-v3-utils] Add replace deprecated value_of and is_present with get_one and contains_id

[samkim-crypto]

Problem

The behavior of clap ArgMatches::value_of and ArgMatches::is_present in clap v2 and v3 are different making it hard to upgrade existing Solana cli tools (in particular the token-cli) from using clap v2 to clap v3.

The behavior of ArgMatches::value_of differs in clap v2 and v3 as follows:

• In v2, if ArgMatches::value_of succeeds in retrieving the argument say arg, then it returns Some(arg). If it fails in any other way, it returns None.
• In v3, if ArgMatches::value_of succeeds in retrieving the argument, then it returns Some(arg). If the queried argument id is valid (it was declared with Arg::new(...) when defining the command), but it was not provided by the user, the it returns None. If the argument id is not valid, then it panics.

The ArgMatches::is_present has the analogous behavior:

• In v2, if an argument say arg was provided by the user, then is_present(arg) returns true. In all other cases, it returns false.
• In v3, if the argument was provided by the user, it returns true. If arg is valid, but was not provided by the user, then it returns false. If arg is not valid, then it panics.

So if we try to upgrade a cli tools from using solana-clap-v2-utils to solana-clap-v3-utils, the tests fails due to the panicking behavior introduced with v3.

Also, value_of and is_present are technically deprecated functions, but they are still used in clap-v3-utils, solana-keygen, and solana-zk-keygen. They should really be replaced with ArgMatches::get_one/Argmatches::try_get_one and ArgMatches::contains_id/ArgMatches::try_contains_id. In particular, the try_get_one and try_contains_id returns an Option<_> type wrapped inside another Result<_,_> type.

• If an argument is valid and was provided by the user:
  • try_get_one and try_contains_id returns Ok(Some(arg_val)) and Ok(true) respectively
• If an argument is valid, but was not provided by the user:
  • try_get_one and try_contains_id returns Ok(None) and Ok(false) respectively.
• If an argument is invalid,
  • try_get_one and try_contains_id both returns MatchesError::UnknownArgument.

It would be nice to just replace value_of and is_present with get_one and contains_id in clap-v3-utils, but unfortunately, the deprecated functions and the newer functions behave differently in a non-trivial way, which would break existing cli tools that rely on clap-v3-utils.

Summary of Changes

• For key-related input parser functions, I added the try_... variants that use try_get_one or try_contains_id to return a suitable error when an argument is not valid.
• For functions that return a general error type Box<dyn error::Error>, I replaced the use of value_of and is_present with try_get_one and contains_id. This does not alter the behavior of the functions other than potentially returning a suitable error when it would otherwise panic.

This will unblock cli tools from upgrading to using clap-v3 without introducing breaking changes. On a follow-up PR, I will make some additional updates to clap-v3-utils to replace some deprecated functions.

Fixes #

[codecov]

Codecov Report

Merging #33184 (6776882) into master (3ffd78f) will increase coverage by 0.0%.
The diff coverage is 0.0%.

@@           Coverage Diff           @@
##           master   #33184   +/-   ##
=======================================
  Coverage    81.9%    81.9%           
=======================================
  Files         789      789           
  Lines      213988   214012   +24     
=======================================
+ Hits       175392   175424   +32     
+ Misses      38596    38588    -8     

[t-nelson]

lgtm!

might pay to add a doc comment to the non-try versions stating that they panic. fine to do in follow up though

[CriesofCarrots]

might pay to add a doc comment to the non-try versions stating that they panic. fine to do in follow up though

Would it also make sense to deprecate the non-try versions?

[samkim-crypto]

Yes, I think it would make sense to deprecate the non-try versions. I was actually thinking about doing that in this PR, but the keygen and zk-keygen rely on these functions, so the CI complained. I'll create a follow-up to this PR to remove the deprecated functions in keygen and zk-keygen (and clap-v3-utils) first and then do another follow-up to deprecate the functions.

[t-nelson]

please don't remove them outright, instead mark with #[deprecated(...)] and an appropriate message redirecting consumers to the new methods.