[clap-v3-utils] Add functions to directly parse from SignerSource

[samkim-crypto]

Problem

Using the way we have set up in clap-v3-utils with SignerSource, downstream cli tools must parse keypair, signer, or pubkey` from signer source arguments in two steps:

1. parse the signer source
2. extract the keypair, signer, or pubkey from the signer source

let signer_source = matches.get_one::<SignerSource>("program_id).unwrap(); // step 1
let desired_pubkey = pubkey_from_source(matches, source, "program_id, &mut wallet_manager); // step 2

In an overwhelming number of cases, the cli tools proceed with steps 1 and 2 together. This requirement to parse arguments in two steps have resulted in more verbose code (for example, solana-labs/solana-program-library#6625. It would really simplify and make the code less verbose if there were utility functions that performs steps 1 and 2 together.

There exist functions like pubkey_of and signer_of that does this. Unfortunately, these functions parse the signer source arguments as String and hence results in a downcast error if the argument is defined using the updated SignerSourceParserBuilder.

Summary of Changes

Added utility functions try_get_{keypair, keypairs, signer, signers, pubkey, pubkeys} and try_resolve functions as static functions to SignerSource. These functions perform the steps 1 and 2 described above and should really help in making cli code easier and more concise.

This change was included in an old PR on clap-v3-utils solana-labs#33478, but I did not include it in subsequent PRs because I thought we don't necessarily need it. Now, after making some updates to a couple of cli tools, it seems like these functions would make life a lot easier.

Fixes #

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 0% with 55 lines in your changes are missing coverage. Please review.

Project coverage is 82.0%. Comparing base (8e331e1) to head (7bfcf3a).
Report is 7 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff            @@
##           master    #1062     +/-   ##
=========================================
- Coverage    82.0%    82.0%   -0.1%     
=========================================
  Files         860      860             
  Lines      232898   232946     +48     
=========================================
+ Hits       191071   191109     +38     
- Misses      41827    41837     +10     

[CriesofCarrots]

Is there value in returning Result<Option<T>> for all of these?
ie. is there meaningful difference, from the user's perspective, between an Err and Ok(None)?

All our original clap parsers return Option<T>, and the new ones (like keypair_from_source) return Result<T>. Can/should we coerce these combiner functions into one or the other of those patterns?

[samkim-crypto]

Yeah this is the return type of try_get_one::<T>(arg) function in clap-v3... the main difference is detailed in solana-labs#33184:

• Case 1: Return of Err means that the function was asked on an unknown/undefined argument:
  • MatchesError::UnknownArgument means that the arg is unknown
  • MatchesError::Downcast means that the arg is known, but is asked on an incorrect type
• Case 2: Return of Ok(None) means that the argument is known and of correct type, but was not provided by the user

In clap-v2, the value_of(arg) function did not distinguish between the two cases and always return None, which was why our original parsers returned Option<T>.

The unfortunate thing is that the function value_of(arg) (now deprecated) changed its behavior in clap-v3. It now panics in Case 1 and returns None in Case 2. All our original parsers (e.g. pubkey_of), which used value_of have inherited the same panicking behavior and this has been the source of many headaches when upgrading clap-v2 --> clap-v3. To cope with this, the try_ versions of these (e.g. try_pubkey_of) were added, which prevented panics, but since the original parsing functions expect to parse args as String as opposed to SignerSource, they are incompatible with SignerSourceParserBuilder (and hence this PR).

Given this, I think it does make sense that these combiner functions follow the clap-v3 syntax of try_get_one::<T>(arg) since it is what it relies on to parse the arguments.

Functions like {signer, keypair, pubkey}_from_source are not really arg parsers (though it does take ArgMatches...), but more like functions to extract signer, keypair, or pubkey from a SignerSource after the signer source is parsed from user args. Here, I think it makes sense to return just a Result<T> as in the {signer, keypair, pubkey}_from_path fucntions.

[CriesofCarrots]

Thanks for the thorough response. I did see that try_get_one and try_get_many have that return type, but afaict, the two Error cases would only get thrown if we mess up the code in our library (ie. at the call sites of try_get_one/try_get_many). If the user inputs the an arg that doesn't exist or supplies the wrong input type to an argument, they will receive a clap runtime error and not make it to these methods. Playground
So it doesn't seem like there are really any distinct cases being communicated by Err vs Ok(None) for these uses of try_get_one/try_get_many. Am I missing something there?

That said, I guess our v1 clap signer parsers return Result<Option<T>>, so remaining consistent with those is probably the best argument for persisting this return type. I do wish we had handled those better in the first place, though...

[samkim-crypto]

Ah yes, that is a good point. There is also try_get_matches, but it also returns a different error type than MatchesError.

I may be misunderstanding your point, but it does seem like the one case not considered in the playground example is when the user does provide the correct arguments as specified in the clap app, but the internal logic looks for an unknown or wrong type argument playground This was what I encountered when I was upgrading the token-cli. For example, the argument --signer was not defined in the Transfer command, but it was queried by signer_from_path on pubkey inputs causing a panic.

So in the context of this PR, it does seem like the downstream code can call SignerSource::try_get_keypair on an unknown argument that was not defined in the clap-app (independent of whether the user provided the correct inputs or not), which would produce a MatchesError. It would be nice if clap catches this on compile time, but it doesn't seem to do so. Does this sound right to you? or am I confusing myself 🥲?

[CriesofCarrots]

I think we are saying the same thing. 😅 But I think it is largely incumbent on us to ensure our internal logic is correct (ie. to ensure that clap Arg apis that might be called reflect things in the actual App). Your example is not really all that different than having a fn that calls matches.value_of("misspelled_arg") in clap v1, except that the old clap will just fail silently. I do acknowledge that ensuring our internal logic is correct is more difficult given the layers of clap utils.

But anyway, since I already conceded there are other reasons to keep the return type, go forth and do good :)

[samkim-crypto]

Ah yes, I see your point. Thanks a lot for the clarification!

[CriesofCarrots]

Sorry for keeping you waiting on this for so long

[samkim-crypto]

No problem! Thanks for taking a look. I will merge it as is for now, but feel free to correct me if I misunderstood anything in the thread above!