Use is_amount clap validator (solana-labs#7400)

* Fix up is_amount to handle floats for SOL; expand amount_of test * Use required_lamports_from and is_amount across CLI * Remove obsolete test (now handled by clap)
aeyakovenko · Dec 10, 2019 · 11521dc · 11521dc
1 parent 6f45729
commit 11521dc
Show file tree

Hide file tree

Showing 6 changed files with 49 additions and 23 deletions.
diff --git a/clap-utils/src/input_parsers.rs b/clap-utils/src/input_parsers.rs
@@ -216,11 +216,18 @@ mod tests {
             .clone()
             .get_matches_from(vec!["test", "--single", "50", "--unit", "lamports"]);
         assert_eq!(amount_of(&matches, "single", "unit"), Some(50));
+        assert_eq!(amount_of(&matches, "multiple", "unit"), None);
         let matches = app()
             .clone()
             .get_matches_from(vec!["test", "--single", "50", "--unit", "SOL"]);
         assert_eq!(amount_of(&matches, "single", "unit"), Some(50000000000));
-        assert_eq!(amount_of(&matches, "multiple", "unit"), None);
-        assert_eq!(amount_of(&matches, "multiple", "unit"), None);
+        let matches = app()
+            .clone()
+            .get_matches_from(vec!["test", "--single", "1.5", "--unit", "SOL"]);
+        assert_eq!(amount_of(&matches, "single", "unit"), Some(1500000000));
+        let matches = app()
+            .clone()
+            .get_matches_from(vec!["test", "--single", "1.5", "--unit", "lamports"]);
+        assert_eq!(amount_of(&matches, "single", "unit"), None);
     }
 }
diff --git a/clap-utils/src/input_validators.rs b/clap-utils/src/input_validators.rs
@@ -120,8 +120,12 @@ pub fn is_valid_percentage(percentage: String) -> Result<(), String> {
 }
 
 pub fn is_amount(amount: String) -> Result<(), String> {
-    amount
-        .parse::<u64>()
-        .map(|_| ())
-        .map_err(|e| format!("{:?}", e))
+    if amount.parse::<u64>().is_ok() || amount.parse::<f64>().is_ok() {
+        Ok(())
+    } else {
+        Err(format!(
+            "Unable to parse input amount as integer or float, provided: {}",
+            amount
+        ))
+    }
 }
diff --git a/cli/src/cli.rs b/cli/src/cli.rs
@@ -406,7 +406,7 @@ pub fn parse_command(matches: &ArgMatches<'_>) -> Result<CliCommandInfo, Box<dyn
             } else {
                 None
             };
-            let lamports = amount_of(matches, "amount", "unit").expect("Invalid amount");
+            let lamports = required_lamports_from(matches, "amount", "unit")?;
             let use_lamports_unit = matches.value_of("unit").is_some()
                 && matches.value_of("unit").unwrap() == "lamports";
             Ok(CliCommandInfo {
@@ -447,7 +447,7 @@ pub fn parse_command(matches: &ArgMatches<'_>) -> Result<CliCommandInfo, Box<dyn
             }
         },
         ("pay", Some(matches)) => {
-            let lamports = amount_of(matches, "amount", "unit").expect("Invalid amount");
+            let lamports = required_lamports_from(matches, "amount", "unit")?;
             let to = pubkey_of(&matches, "to").unwrap();
             let timestamp = if matches.is_present("timestamp") {
                 // Parse input for serde_json
@@ -1449,6 +1449,22 @@ where
     }
 }
 
+// If clap arg `name` is_required, and specifies an amount of either lamports or SOL, the only way
+// `amount_of()` can return None is if `name` is an f64 and `unit`== "lamports". This method
+// catches that case and converts it to an Error.
+pub(crate) fn required_lamports_from(
+    matches: &ArgMatches<'_>,
+    name: &str,
+    unit: &str,
+) -> Result<u64, CliError> {
+    amount_of(matches, name, unit).ok_or_else(|| {
+        CliError::BadParameter(format!(
+            "Lamports cannot be fractional: {}",
+            matches.value_of("amount").unwrap()
+        ))
+    })
+}
+
 pub(crate) fn build_balance_message(
     lamports: u64,
     use_lamports_unit: bool,
@@ -1516,6 +1532,7 @@ pub fn app<'ab, 'v>(name: &str, about: &'ab str, version: &'v str) -> App<'ab, '
                         .index(1)
                         .value_name("AMOUNT")
                         .takes_value(true)
+                        .validator(is_amount)
                         .required(true)
                         .help("The airdrop amount to request (default unit SOL)"),
                 )
@@ -1588,6 +1605,7 @@ pub fn app<'ab, 'v>(name: &str, about: &'ab str, version: &'v str) -> App<'ab, '
                         .index(2)
                         .value_name("AMOUNT")
                         .takes_value(true)
+                        .validator(is_amount)
                         .required(true)
                         .help("The amount to send (default unit SOL)"),
                 )
@@ -1759,14 +1777,6 @@ mod tests {
         path
     }
 
-    #[test]
-    #[should_panic]
-    fn test_bad_amount() {
-        let test_commands = app("test", "desc", "version");
-        let test_bad_airdrop = test_commands.get_matches_from(vec!["test", "airdrop", "notint"]);
-        let _ignored = parse_command(&test_bad_airdrop).unwrap();
-    }
-
     #[test]
     fn test_cli_parse_command() {
         let test_commands = app("test", "desc", "version");

diff --git a/cli/src/cluster_query.rs b/cli/src/cluster_query.rs
@@ -127,6 +127,7 @@ impl ClusterQuerySubCommands for App<'_, '_> {
                         .value_name("NUMBER")
                         .takes_value(true)
                         .default_value("1")
+                        .validator(is_amount)
                         .help("Number of lamports to transfer for each transaction"),
                 )
                 .arg(

diff --git a/cli/src/nonce.rs b/cli/src/nonce.rs
@@ -1,6 +1,7 @@
 use crate::cli::{
     build_balance_message, check_account_for_fee, check_unique_pubkeys,
-    log_instruction_custom_error, CliCommand, CliCommandInfo, CliConfig, CliError, ProcessResult,
+    log_instruction_custom_error, required_lamports_from, CliCommand, CliCommandInfo, CliConfig,
+    CliError, ProcessResult,
 };
 use clap::{App, Arg, ArgMatches, SubCommand};
 use solana_clap_utils::{input_parsers::*, input_validators::*};
@@ -142,7 +143,7 @@ impl NonceSubCommands for App<'_, '_> {
 
 pub fn parse_nonce_create_account(matches: &ArgMatches<'_>) -> Result<CliCommandInfo, CliError> {
     let nonce_account = keypair_of(matches, "nonce_account_keypair").unwrap();
-    let lamports = amount_of(matches, "amount", "unit").unwrap();
+    let lamports = required_lamports_from(matches, "amount", "unit")?;
 
     Ok(CliCommandInfo {
         command: CliCommand::CreateNonceAccount {
@@ -189,7 +190,7 @@ pub fn parse_withdraw_from_nonce_account(
 ) -> Result<CliCommandInfo, CliError> {
     let nonce_account = keypair_of(matches, "nonce_account_keypair").unwrap();
     let destination_account_pubkey = pubkey_of(matches, "destination_account_pubkey").unwrap();
-    let lamports = amount_of(matches, "amount", "unit").unwrap();
+    let lamports = required_lamports_from(matches, "amount", "unit")?;
 
     Ok(CliCommandInfo {
         command: CliCommand::WithdrawFromNonceAccount {

diff --git a/cli/src/stake.rs b/cli/src/stake.rs
@@ -1,7 +1,8 @@
 use crate::cli::{
     build_balance_message, check_account_for_fee, check_unique_pubkeys,
-    get_blockhash_fee_calculator, log_instruction_custom_error, replace_signatures, return_signers,
-    CliCommand, CliCommandInfo, CliConfig, CliError, ProcessResult,
+    get_blockhash_fee_calculator, log_instruction_custom_error, replace_signatures,
+    required_lamports_from, return_signers, CliCommand, CliCommandInfo, CliConfig, CliError,
+    ProcessResult,
 };
 use clap::{App, Arg, ArgMatches, SubCommand};
 use console::style;
@@ -51,6 +52,7 @@ impl StakeSubCommands for App<'_, '_> {
                         .index(2)
                         .value_name("AMOUNT")
                         .takes_value(true)
+                        .validator(is_amount)
                         .required(true)
                         .help("The amount of send to the vote account (default unit SOL)")
                 )
@@ -251,6 +253,7 @@ impl StakeSubCommands for App<'_, '_> {
                         .index(3)
                         .value_name("AMOUNT")
                         .takes_value(true)
+                        .validator(is_amount)
                         .required(true)
                         .help("The amount to withdraw from the stake account (default unit SOL)")
                 )
@@ -323,7 +326,7 @@ pub fn parse_stake_create_account(matches: &ArgMatches<'_>) -> Result<CliCommand
     let custodian = pubkey_of(matches, "custodian").unwrap_or_default();
     let staker = pubkey_of(matches, "authorized_staker");
     let withdrawer = pubkey_of(matches, "authorized_withdrawer");
-    let lamports = amount_of(matches, "amount", "unit").expect("Invalid amount");
+    let lamports = required_lamports_from(matches, "amount", "unit")?;
 
     Ok(CliCommandInfo {
         command: CliCommand::CreateStakeAccount {
@@ -407,7 +410,7 @@ pub fn parse_stake_deactivate_stake(matches: &ArgMatches<'_>) -> Result<CliComma
 pub fn parse_stake_withdraw_stake(matches: &ArgMatches<'_>) -> Result<CliCommandInfo, CliError> {
     let stake_account_pubkey = pubkey_of(matches, "stake_account_pubkey").unwrap();
     let destination_account_pubkey = pubkey_of(matches, "destination_account_pubkey").unwrap();
-    let lamports = amount_of(matches, "amount", "unit").expect("Invalid amount");
+    let lamports = required_lamports_from(matches, "amount", "unit")?;
 
     Ok(CliCommandInfo {
         command: CliCommand::WithdrawStake(