reject zero send amounts (#799)

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

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

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

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

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 {

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 {

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<Identifier, Option<String>> = 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)

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;