diff --git a/controller/src/command.rs b/controller/src/command.rs index 83d0e26..b1543a5 100644 --- a/controller/src/command.rs +++ b/controller/src/command.rs @@ -372,12 +372,15 @@ where true, args.minimum_confirmations, )?; - if args.use_max_amount { - amount = wallet_info.amount_currently_spendable; - } if !info_updated && !update_skipped { warn!("Wallet info update failed: node connection error"); } + if args.use_max_amount { + amount = wallet_info.amount_currently_spendable; + if amount == 0 { + return Err(Error::GenericError("No spendable funds".to_string())); + } + } if args.estimate_selection_strategies { let strategies = estimate_strategies(args.use_max_amount) .iter() diff --git a/controller/tests/invoice.rs b/controller/tests/invoice.rs index 006d5d4..b48f66e 100644 --- a/controller/tests/invoice.rs +++ b/controller/tests/invoice.rs @@ -113,6 +113,17 @@ fn invoice_tx_impl(test_dir: &'static str) -> Result<(), libwallet::Error> { mask2, PathBuf::from(test_dir), |api, m| { + let error = api + .issue_invoice_tx( + m, + IssueInvoiceTxArgs { + amount: 0, + ..Default::default() + }, + ) + .unwrap_err(); + assert_eq!(error, libwallet::Error::InvalidAmount); + // Wallet 2 inititates an invoice transaction, requesting payment let args = IssueInvoiceTxArgs { amount: reward * 2, @@ -177,6 +188,14 @@ fn invoice_tx_impl(test_dir: &'static str) -> Result<(), libwallet::Error> { selection_strategy_is_use_all: true, ..Default::default() }; + let mut zero_amount_slate = slate.clone(); + zero_amount_slate.amount = 0; + assert_eq!( + api.process_invoice_tx(m, &zero_amount_slate, args.clone()) + .unwrap_err(), + libwallet::Error::InvalidAmount + ); + slate = api.process_invoice_tx(m, &slate, args)?; api.tx_lock_outputs(m, &slate)?; Ok(()) diff --git a/controller/tests/transaction.rs b/controller/tests/transaction.rs index 5425ef8..107c17d 100644 --- a/controller/tests/transaction.rs +++ b/controller/tests/transaction.rs @@ -112,10 +112,41 @@ fn basic_transaction_api(test_dir: &'static str) -> Result<(), libwallet::Error> mask1, PathBuf::from(test_dir), |sender_api, m| { + // Do not allow to send 0 + let error = sender_api + .init_send_tx( + m, + InitTxArgs { + amount: 0, + ..Default::default() + }, + ) + .unwrap_err(); + assert_eq!(error, libwallet::Error::InvalidAmount); + + // Result amount can not be 0 when amount includes fee + let mut init_args = InitTxArgs { + amount_includes_fee: Some(true), + amount: 23_000_000, + estimate_only: Some(true), + ..Default::default() + }; + let est = sender_api.init_send_tx(m, init_args.clone())?; + assert_eq!(init_args.amount, est.fee_fields.fee()); + + init_args.estimate_only = None; + let error = sender_api.init_send_tx(m, init_args).unwrap_err(); + assert_eq!( + error, + libwallet::Error::GenericError( + "Transaction amount is too small to include fee".to_string() + ) + ); + // note this will increment the block count as part of the transaction "Posting" let args = InitTxArgs { src_acct_name: None, - amount: amount, + amount, minimum_confirmations: 2, max_outputs: 500, num_change_outputs: 1, @@ -127,6 +158,21 @@ fn basic_transaction_api(test_dir: &'static str) -> Result<(), libwallet::Error> assert_eq!(slate_i.state, SlateState::Standard1); assert_eq!(slate_i.version_info.block_header_version, header_version); + wallet::controller::foreign_single_use( + wallet2.clone(), + PathBuf::from(test_dir), + mask2_i.clone(), + |api| { + let mut zero_amount_slate = slate_i.clone(); + zero_amount_slate.amount = 0; + assert_eq!( + api.receive_tx(&zero_amount_slate, None, None).unwrap_err(), + libwallet::Error::InvalidAmount + ); + Ok(()) + }, + )?; + // Check we are creating a tx with the expected lock_height of 0. // We will check this produces a Plain kernel later. assert_eq!(0, slate.kernel_features); diff --git a/libwallet/src/api_impl/foreign.rs b/libwallet/src/api_impl/foreign.rs index 4547b87..5c63a52 100644 --- a/libwallet/src/api_impl/foreign.rs +++ b/libwallet/src/api_impl/foreign.rs @@ -65,6 +65,10 @@ where C: NodeClient, K: Keychain, { + if slate.amount == 0 { + return Err(Error::InvalidAmount); + } + let mut ret_slate = slate.clone(); if ret_slate.state != SlateState::Standard1 { return Err(Error::SlateState); diff --git a/libwallet/src/api_impl/owner.rs b/libwallet/src/api_impl/owner.rs index 44aecae..58bfbd2 100644 --- a/libwallet/src/api_impl/owner.rs +++ b/libwallet/src/api_impl/owner.rs @@ -534,6 +534,10 @@ where C: NodeClient, K: Keychain, { + if args.amount == 0 { + return Err(Error::InvalidAmount); + } + let payment_proof_address = if let Some(a) = &args.payment_proof_recipient_address { if a.valid_network() { Some(a) @@ -657,6 +661,10 @@ where C: NodeClient, K: Keychain, { + if args.amount == 0 { + return Err(Error::InvalidAmount); + } + let parent_key_id = match args.dest_acct_name { Some(d) => { let pm = w.get_acct_path(d)?; @@ -710,6 +718,10 @@ where C: NodeClient, K: Keychain, { + if slate.amount == 0 { + return Err(Error::InvalidAmount); + } + let mut ret_slate = slate.clone(); check_ttl(w, &ret_slate)?; let parent_key_id = match args.src_acct_name { diff --git a/libwallet/src/error.rs b/libwallet/src/error.rs index 1c772bf..5300dd1 100644 --- a/libwallet/src/error.rs +++ b/libwallet/src/error.rs @@ -24,6 +24,10 @@ use grin_store; /// Wallet errors, mostly wrappers around underlying crypto or I/O errors. #[derive(Clone, Eq, PartialEq, Debug, thiserror::Error, Serialize, Deserialize)] pub enum Error { + /// Invalid transaction amount + #[error("Invalid amount (must be greater than zero)")] + InvalidAmount, + /// Not enough funds #[error("Not enough funds. Required: {needed_disp:?}, Available: {available_disp:?}")] NotEnoughFunds { diff --git a/libwallet/src/internal/selection.rs b/libwallet/src/internal/selection.rs index ac17dbd..bac6d92 100644 --- a/libwallet/src/internal/selection.rs +++ b/libwallet/src/internal/selection.rs @@ -73,9 +73,12 @@ where false, )?; if amount_includes_fee { - slate.amount = slate.amount.checked_sub(fee).ok_or(Error::GenericError( - "Transaction amount is too small to include fee".to_string(), - ))?; + let small_amount_err = + Error::GenericError("Transaction amount is too small to include fee".to_string()); + if slate.amount == fee { + return Err(small_amount_err); + } + slate.amount = slate.amount.checked_sub(fee).ok_or(small_amount_err)?; }; if fixed_fee.map(|f| fee != f).unwrap_or(false) { @@ -104,15 +107,9 @@ where context.add_input(&input.key_id, &input.mmr_index, input.value); } - let mut commits: HashMap> = HashMap::new(); - - // Store change output(s) and cached commits + // Store change output(s) for (change_amount, id, mmr_index) in &change_amounts_derivations { context.add_output(&id, &mmr_index, *change_amount); - commits.insert( - id.clone(), - wallet.calc_commit_for_cache(keychain_mask, *change_amount, &id)?, - ); } Ok(context) diff --git a/tests/cmd_line_basic.rs b/tests/cmd_line_basic.rs index 86f7675..a4d8308 100644 --- a/tests/cmd_line_basic.rs +++ b/tests/cmd_line_basic.rs @@ -147,6 +147,11 @@ fn command_line_test_impl(test_dir: &str) -> Result<(), grin_wallet_controller:: let arg_vec = vec!["grin-wallet", "-p", "password2", "account"]; execute_command(&app, test_dir, "wallet2", &client2, arg_vec)?; + // Return an explained error when trying to send max amount on empty wallet. + let arg_vec = vec!["grin-wallet", "-p", "password2", "send", "max"]; + let err = execute_command(&app, test_dir, "wallet2", &client2, arg_vec).unwrap_err(); + assert!(format!("{}", err).contains("No spendable funds")); + // Mine a bit into wallet 1 so we have something to send // (TODO: Be able to stop listeners so we can test this better) let wallet_config1 = config1.clone().members.wallet;