Skip to content

Bug fix: Graceful handling of absent clap arguments #5534

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

Merged
merged 5 commits into from
Apr 3, 2025
Merged

Bug fix: Graceful handling of absent clap arguments #5534

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
78e7e8e
clap: graceful handling of absent arguments
grod220 Mar 27, 2025
b6db266
co-locate signer util tests
grod220 Mar 27, 2025
dba44f3
Additional absent arg handling
grod220 Mar 28, 2025
8cf1f8d
Revert and add test
grod220 Mar 31, 2025
53cc0f2
Add positive test cases in Pubkey branches
grod220 Apr 1, 2025
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
163 changes: 137 additions & 26 deletions clap-v3-utils/src/input_parsers/signer.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,7 @@ impl SignerSource {
matches: &ArgMatches,
name: &str,
) -> Result<Option<Keypair>, Box<dyn error::Error>> {
let source = matches.try_get_one::<Self>(name)?;
let source = matches.try_get_one::<Self>(name).ok().flatten();
if let Some(source) = source {
keypair_from_source(matches, source, name, true).map(Some)
} else {
Expand All @@ -106,7 +106,7 @@ impl SignerSource {
matches: &ArgMatches,
name: &str,
) -> Result<Option<Vec<Keypair>>, Box<dyn error::Error>> {
let sources = matches.try_get_many::<Self>(name)?;
let sources = matches.try_get_many::<Self>(name).ok().flatten();
if let Some(sources) = sources {
let keypairs = sources
.filter_map(|source| keypair_from_source(matches, source, name, true).ok())
Expand All @@ -123,7 +123,7 @@ impl SignerSource {
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)?;
let source = matches.try_get_one::<Self>(name).ok().flatten();
if let Some(source) = source {
let signer = signer_from_source(matches, source, name, wallet_manager)?;
let signer_pubkey = signer.pubkey();
Expand All @@ -139,7 +139,7 @@ impl SignerSource {
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)?;
let sources = matches.try_get_many::<Self>(name).ok().flatten();
if let Some(sources) = sources {
let signers = sources
.filter_map(|source| {
Expand All @@ -159,7 +159,7 @@ impl SignerSource {
name: &str,
wallet_manager: &mut Option<Rc<RemoteWalletManager>>,
) -> Result<Option<Pubkey>, Box<dyn error::Error>> {
let source = matches.try_get_one::<Self>(name)?;
let source = matches.try_get_one::<Self>(name).ok().flatten();
if let Some(source) = source {
pubkey_from_source(matches, source, name, wallet_manager).map(Some)
} else {
Expand All @@ -172,7 +172,7 @@ impl SignerSource {
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)?;
let sources = matches.try_get_many::<Self>(name).ok().flatten();
if let Some(sources) = sources {
let pubkeys = sources
.filter_map(|source| pubkey_from_source(matches, source, name, wallet_manager).ok())
Expand All @@ -188,7 +188,7 @@ impl SignerSource {
name: &str,
wallet_manager: &mut Option<Rc<RemoteWalletManager>>,
) -> Result<Option<String>, Box<dyn std::error::Error>> {
let source = matches.try_get_one::<Self>(name)?;
let source = matches.try_get_one::<Self>(name).ok().flatten();
if let Some(source) = source {
resolve_signer_from_source(matches, source, name, wallet_manager)
} else {
Expand Down Expand Up @@ -343,7 +343,7 @@ pub fn try_keypair_of(
matches: &ArgMatches,
name: &str,
) -> Result<Option<Keypair>, Box<dyn error::Error>> {
if let Some(value) = matches.try_get_one::<String>(name)? {
if let Some(value) = matches.try_get_one::<String>(name).ok().flatten() {
extract_keypair(matches, name, value)
} else {
Ok(None)
Expand All @@ -354,11 +354,15 @@ pub fn try_keypairs_of(
matches: &ArgMatches,
name: &str,
) -> Result<Option<Vec<Keypair>>, Box<dyn error::Error>> {
Ok(matches.try_get_many::<String>(name)?.map(|values| {
values
.filter_map(|value| extract_keypair(matches, name, value).ok().flatten())
.collect()
}))
Ok(matches
.try_get_many::<String>(name)
.ok()
.flatten()
.map(|values| {
values
.filter_map(|value| extract_keypair(matches, name, value).ok().flatten())
.collect()
}))
}

fn extract_keypair(
Expand All @@ -367,7 +371,9 @@ fn extract_keypair(
path: &str,
) -> Result<Option<Keypair>, Box<dyn error::Error>> {
if path == ASK_KEYWORD {
let skip_validation = matches.try_contains_id(SKIP_SEED_PHRASE_VALIDATION_ARG.name)?;
let skip_validation = matches
.try_contains_id(SKIP_SEED_PHRASE_VALIDATION_ARG.name)
.unwrap_or(false);
keypair_from_seed_phrase(name, skip_validation, true, None, true).map(Some)
} else {
read_keypair_file(path).map(Some)
Expand All @@ -380,7 +386,7 @@ pub fn try_pubkey_of(
matches: &ArgMatches,
name: &str,
) -> Result<Option<Pubkey>, Box<dyn error::Error>> {
if let Some(pubkey) = matches.try_get_one::<Pubkey>(name)? {
if let Some(pubkey) = matches.try_get_one::<Pubkey>(name).ok().flatten() {
Ok(Some(*pubkey))
} else {
Ok(try_keypair_of(matches, name)?.map(|keypair| keypair.pubkey()))
Expand All @@ -391,7 +397,7 @@ pub fn try_pubkeys_of(
matches: &ArgMatches,
name: &str,
) -> Result<Option<Vec<Pubkey>>, Box<dyn error::Error>> {
if let Some(pubkey_strings) = matches.try_get_many::<String>(name)? {
if let Some(pubkey_strings) = matches.try_get_many::<String>(name).ok().flatten() {
let mut pubkeys = Vec::with_capacity(pubkey_strings.len());
for pubkey_string in pubkey_strings {
pubkeys.push(pubkey_string.parse::<Pubkey>()?);
Expand Down Expand Up @@ -424,7 +430,7 @@ pub fn try_pubkeys_sigs_of(
matches: &ArgMatches,
name: &str,
) -> Result<Option<Vec<(Pubkey, Signature)>>, Box<dyn error::Error>> {
if let Some(pubkey_signer_strings) = matches.try_get_many::<String>(name)? {
if let Some(pubkey_signer_strings) = matches.try_get_many::<String>(name).ok().flatten() {
let mut pubkey_sig_pairs = Vec::with_capacity(pubkey_signer_strings.len());
for pubkey_signer_string in pubkey_signer_strings {
let (pubkey_string, sig_string) = pubkey_signer_string
Expand All @@ -447,7 +453,7 @@ pub fn signer_of(
name: &str,
wallet_manager: &mut Option<Rc<RemoteWalletManager>>,
) -> Result<(Option<Box<dyn Signer>>, Option<Pubkey>), Box<dyn std::error::Error>> {
if let Some(location) = matches.try_get_one::<String>(name)? {
if let Some(location) = matches.try_get_one::<String>(name).ok().flatten() {
let signer = signer_from_path(matches, location, name, wallet_manager)?;
let signer_pubkey = signer.pubkey();
Ok((Some(signer), Some(signer_pubkey)))
Expand All @@ -461,7 +467,7 @@ pub fn pubkey_of_signer(
name: &str,
wallet_manager: &mut Option<Rc<RemoteWalletManager>>,
) -> Result<Option<Pubkey>, Box<dyn std::error::Error>> {
if let Some(location) = matches.try_get_one::<String>(name)? {
if let Some(location) = matches.try_get_one::<String>(name).ok().flatten() {
Ok(Some(pubkey_from_path(
matches,
location,
Expand All @@ -478,7 +484,7 @@ pub fn pubkeys_of_multiple_signers(
name: &str,
wallet_manager: &mut Option<Rc<RemoteWalletManager>>,
) -> Result<Option<Vec<Pubkey>>, Box<dyn std::error::Error>> {
if let Some(pubkey_matches) = matches.try_get_many::<String>(name)? {
if let Some(pubkey_matches) = matches.try_get_many::<String>(name).ok().flatten() {
let mut pubkeys: Vec<Pubkey> = vec![];
for signer in pubkey_matches {
pubkeys.push(pubkey_from_path(matches, signer, name, wallet_manager)?);
Expand All @@ -494,12 +500,12 @@ pub fn resolve_signer(
name: &str,
wallet_manager: &mut Option<Rc<RemoteWalletManager>>,
) -> Result<Option<String>, Box<dyn std::error::Error>> {
resolve_signer_from_path(
matches,
matches.try_get_one::<String>(name)?.unwrap(),
name,
wallet_manager,
)
let path = matches
.try_get_one::<String>(name)
.ok()
.flatten()
.ok_or_else(|| format!("Missing required path argument: {}", name))?;
resolve_signer_from_path(matches, path, name, wallet_manager)
}

#[derive(Clone, Copy, Debug, Eq, PartialEq)]
Expand Down Expand Up @@ -697,6 +703,20 @@ mod tests {
fs::remove_file(&outfile).unwrap();
}

#[test]
fn test_try_keypair_of() {
let matches = ArgMatches::default();
let result = try_keypair_of(&matches, "test").unwrap();
assert!(result.is_none());
}

#[test]
fn test_try_keypairs_of() {
let matches = ArgMatches::default();
let result = try_keypairs_of(&matches, "test").unwrap();
assert!(result.is_none());
}

#[test]
fn test_pubkey_of() {
let keypair = Keypair::new();
Expand Down Expand Up @@ -737,6 +757,20 @@ mod tests {
fs::remove_file(&outfile).unwrap();
}

#[test]
fn test_try_pubkeys_of() {
let matches = ArgMatches::default();
let result = try_pubkeys_of(&matches, "test").unwrap();
assert!(result.is_none());
}

#[test]
fn test_pubkeys_of_multiple_signers() {
let matches = ArgMatches::default();
let result = pubkeys_of_multiple_signers(&matches, "test", &mut None).unwrap();
assert!(result.is_none());
}

#[test]
fn test_pubkeys_sigs_of() {
let key1 = solana_pubkey::new_rand();
Expand All @@ -753,6 +787,13 @@ mod tests {
);
}

#[test]
fn test_try_pubkeys_sigs_of() {
let matches = ArgMatches::default();
let result = try_pubkeys_sigs_of(&matches, "test").unwrap();
assert!(result.is_none());
}

#[test]
fn test_parse_pubkey_signature() {
let command = Command::new("test").arg(
Expand Down Expand Up @@ -1048,4 +1089,74 @@ mod tests {
.unwrap_err();
assert_eq!(matches_error.kind, clap::error::ErrorKind::ValueValidation);
}

Copy link

@alexpyattaev alexpyattaev Mar 31, 2025

Choose a reason for hiding this comment

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

I would suggest we add also positive test cases as well. At the very least they would demonstrate how the things are supposed to work. Could be even within the same functions to avoid boilerplate.

#[test]
fn test_signer_of() {
let matches = ArgMatches::default();
let result = signer_of(&matches, "test", &mut None).unwrap();
assert_eq!(result, (None, None));
}

#[test]
fn test_try_get_signer() {
let matches = ArgMatches::default();
let result = SignerSource::try_get_signer(&matches, "test", &mut None).unwrap();
assert!(result.is_none());
}

#[test]
fn test_try_get_signers() {
let matches = ArgMatches::default();
let result = SignerSource::try_get_signers(&matches, "test", &mut None).unwrap();
assert!(result.is_none());
}

#[test]
fn test_try_get_keypair() {
let matches = ArgMatches::default();
let result = SignerSource::try_get_keypair(&matches, "test").unwrap();
assert!(result.is_none());
}

#[test]
fn test_try_get_keypairs() {
let matches = ArgMatches::default();
let result = SignerSource::try_get_keypairs(&matches, "test").unwrap();
assert!(result.is_none());
}

#[test]
fn test_pubkey_of_signer() {
let matches = ArgMatches::default();
let result = pubkey_of_signer(&matches, "test", &mut None).unwrap();
assert!(result.is_none());
}

#[test]
fn test_try_pubkey_of() {
let matches = ArgMatches::default();
let result = try_pubkey_of(&matches, "test").unwrap();
assert!(result.is_none());
}

#[test]
fn test_try_get_pubkey() {
let matches = ArgMatches::default();
let result = SignerSource::try_get_pubkey(&matches, "test", &mut None).unwrap();
assert!(result.is_none());
}

#[test]
fn test_try_get_pubkeys() {
let matches = ArgMatches::default();
let result = SignerSource::try_get_pubkeys(&matches, "test", &mut None).unwrap();
assert!(result.is_none());
}

#[test]
fn test_try_resolve() {
let matches = ArgMatches::default();
let result = SignerSource::try_resolve(&matches, "test", &mut None).unwrap();
assert!(result.is_none());
}
}
14 changes: 13 additions & 1 deletion clap-v3-utils/src/keygen/derivation_path.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ pub fn derivation_path_arg<'a>() -> Arg<'a> {
pub fn acquire_derivation_path(
matches: &ArgMatches,
) -> Result<Option<DerivationPath>, Box<dyn error::Error>> {
if matches.try_contains_id("derivation_path")? {
if matches.try_contains_id("derivation_path").unwrap_or(false) {
Ok(Some(DerivationPath::from_absolute_path_str(
matches
.try_get_one::<String>("derivation_path")?
Expand All @@ -33,3 +33,15 @@ pub fn acquire_derivation_path(
Ok(None)
}
}

#[cfg(test)]
mod tests {
use super::*;

#[test]
fn test_acquire_derivation_path() {
let matches = ArgMatches::default();
let result = acquire_derivation_path(&matches).unwrap();
assert!(result.is_none());
}
}
5 changes: 4 additions & 1 deletion clap-v3-utils/src/keygen/mnemonic.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,7 +121,10 @@ pub fn no_passphrase_and_message() -> (String, String) {
pub fn acquire_passphrase_and_message(
matches: &ArgMatches,
) -> Result<(String, String), Box<dyn error::Error>> {
if matches.try_contains_id(NO_PASSPHRASE_ARG.name)? {
if matches
.try_contains_id(NO_PASSPHRASE_ARG.name)
.unwrap_or(false)
{
Ok(no_passphrase_and_message())
} else {
match prompt_passphrase(
Expand Down
2 changes: 1 addition & 1 deletion clap-v3-utils/src/keygen/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ pub fn check_for_overwrite(
outfile: &str,
matches: &ArgMatches,
) -> Result<(), Box<dyn error::Error>> {
let force = matches.try_contains_id("force")?;
let force = matches.try_contains_id("force").unwrap_or(false);
if !force && Path::new(outfile).exists() {
let err_msg = format!("Refusing to overwrite {outfile} without --force flag");
return Err(err_msg.into());
Expand Down
Loading
Loading