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.

Sign up for GitHub

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

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

Merged
merged 4 commits into from
May 2, 2024
Merged

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

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
9b5e50c
add `try_get_keypair` and `try_get_keypairs`
samkim-crypto Apr 25, 2024
44b588b
add `try_get_signer` and `try_get_signers`
samkim-crypto Apr 26, 2024
6db47dd
add `try_get_pubkey` and `try_get_pubkeys`
samkim-crypto Apr 26, 2024
7bfcf3a
add `try_resolve`
samkim-crypto Apr 26, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
109 changes: 108 additions & 1 deletion clap-v3-utils/src/input_parsers/signer.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
use {
crate::keypair::{
keypair_from_seed_phrase, pubkey_from_path, resolve_signer_from_path, signer_from_path,
keypair_from_seed_phrase, keypair_from_source, pubkey_from_path, pubkey_from_source,
resolve_signer_from_path, resolve_signer_from_source, signer_from_path, signer_from_source,
ASK_KEYWORD, SKIP_SEED_PHRASE_VALIDATION_ARG,
},
clap::{builder::ValueParser, ArgMatches},
Expand Down Expand Up @@ -89,6 +90,112 @@ impl SignerSource {
}
}

pub fn try_get_keypair(
matches: &ArgMatches,
name: &str,
) -> Result<Option<Keypair>, Box<dyn error::Error>> {
Copy link

@CriesofCarrots CriesofCarrots Apr 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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?

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link

Choose a reason for hiding this comment

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

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...

Copy link
Author

@samkim-crypto samkim-crypto May 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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 🥲?

Copy link

Choose a reason for hiding this comment

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

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 :)

Copy link
Author

Choose a reason for hiding this comment

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

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

let source = matches.try_get_one::<Self>(name)?;
if let Some(source) = source {
keypair_from_source(matches, source, name, true).map(Some)
} else {
Ok(None)
}
}

pub fn try_get_keypairs(
matches: &ArgMatches,
name: &str,
) -> Result<Option<Vec<Keypair>>, Box<dyn error::Error>> {
let sources = matches.try_get_many::<Self>(name)?;
if let Some(sources) = sources {
let keypairs = sources
.filter_map(|source| keypair_from_source(matches, source, name, true).ok())
.collect();
Ok(Some(keypairs))
} else {
Ok(None)
}
}

#[allow(clippy::type_complexity)]
pub fn try_get_signer(
matches: &ArgMatches,
name: &str,
wallet_manager: &mut Option<Rc<RemoteWalletManager>>,
) -> Result<Option<(Box<dyn Signer>, Pubkey)>, Box<dyn error::Error>> {
let source = matches.try_get_one::<Self>(name)?;
if let Some(source) = source {
let signer = signer_from_source(matches, source, name, wallet_manager)?;
let signer_pubkey = signer.pubkey();
Ok(Some((signer, signer_pubkey)))
} else {
Ok(None)
}
}

#[allow(clippy::type_complexity)]
pub fn try_get_signers(
matches: &ArgMatches,
name: &str,
wallet_manager: &mut Option<Rc<RemoteWalletManager>>,
) -> Result<Option<Vec<(Box<dyn Signer>, Pubkey)>>, Box<dyn error::Error>> {
let sources = matches.try_get_many::<Self>(name)?;
if let Some(sources) = sources {
let signers = sources
.filter_map(|source| {
let signer = signer_from_source(matches, source, name, wallet_manager).ok()?;
let signer_pubkey = signer.pubkey();
Some((signer, signer_pubkey))
})
.collect();
Ok(Some(signers))
} else {
Ok(None)
}
}

pub fn try_get_pubkey(
matches: &ArgMatches,
name: &str,
wallet_manager: &mut Option<Rc<RemoteWalletManager>>,
) -> Result<Option<Pubkey>, Box<dyn error::Error>> {
let source = matches.try_get_one::<Self>(name)?;
if let Some(source) = source {
pubkey_from_source(matches, source, name, wallet_manager).map(Some)
} else {
Ok(None)
}
}

pub fn try_get_pubkeys(
matches: &ArgMatches,
name: &str,
wallet_manager: &mut Option<Rc<RemoteWalletManager>>,
) -> Result<Option<Vec<Pubkey>>, Box<dyn std::error::Error>> {
let sources = matches.try_get_many::<Self>(name)?;
if let Some(sources) = sources {
let pubkeys = sources
.filter_map(|source| pubkey_from_source(matches, source, name, wallet_manager).ok())
.collect();
Ok(Some(pubkeys))
} else {
Ok(None)
}
}

pub fn try_resolve(
matches: &ArgMatches,
name: &str,
wallet_manager: &mut Option<Rc<RemoteWalletManager>>,
) -> Result<Option<String>, Box<dyn std::error::Error>> {
let source = matches.try_get_one::<Self>(name)?;
if let Some(source) = source {
resolve_signer_from_source(matches, source, name, wallet_manager)
} else {
Ok(None)
}
}

pub fn parse<S: AsRef<str>>(source: S) -> Result<Self, SignerSourceError> {
let source = source.as_ref();
let source = {
Expand Down
Loading