From ef3fadbd249bfa8b1ed401ad63995a2b54226bc0 Mon Sep 17 00:00:00 2001 From: cliik <107021905+cliik@users.noreply.github.com> Date: Tue, 26 Jul 2022 09:15:53 +0000 Subject: [PATCH] Implement fee inclusive transactions (#657) * Add amount_includes_fee option in TX building * Add --amount_includes_fee CLI option * Implement send 'max' amount Co-authored-by: cliik --- controller/src/command.rs | 21 ++- controller/tests/transaction.rs | 41 ++++++ libwallet/src/api_impl/owner.rs | 4 + libwallet/src/api_impl/types.rs | 3 + libwallet/src/internal/selection.rs | 38 +++++- libwallet/src/internal/tx.rs | 5 + src/bin/grin-wallet.yml | 5 +- src/cmd/wallet_args.rs | 9 +- tests/cmd_line_basic.rs | 160 ++++++++++++++++++++++- tests/data/v3_reqs/init_send_tx.req.json | 1 + 10 files changed, 270 insertions(+), 17 deletions(-) diff --git a/controller/src/command.rs b/controller/src/command.rs index 8528b7ad..733222e4 100644 --- a/controller/src/command.rs +++ b/controller/src/command.rs @@ -322,6 +322,8 @@ where #[derive(Clone)] pub struct SendArgs { pub amount: u64, + pub amount_includes_fee: bool, + pub use_max_amount: bool, pub minimum_confirmations: u64, pub selection_strategy: String, pub estimate_selection_strategies: bool, @@ -353,6 +355,15 @@ where K: keychain::Keychain + 'static, { let mut slate = Slate::blank(2, false); + let mut amount = args.amount; + if args.use_max_amount { + controller::owner_single_use(None, keychain_mask, Some(owner_api), |api, m| { + let (_, wallet_info) = + api.retrieve_summary_info(m, true, args.minimum_confirmations)?; + amount = wallet_info.amount_currently_spendable; + Ok(()) + })?; + }; controller::owner_single_use(None, keychain_mask, Some(owner_api), |api, m| { if args.estimate_selection_strategies { let strategies = vec!["smallest", "all"] @@ -360,7 +371,8 @@ where .map(|strategy| { let init_args = InitTxArgs { src_acct_name: None, - amount: args.amount, + amount: amount, + amount_includes_fee: Some(args.amount_includes_fee), minimum_confirmations: args.minimum_confirmations, max_outputs: args.max_outputs as u32, num_change_outputs: args.change_outputs as u32, @@ -372,12 +384,13 @@ where Ok((strategy, slate.amount, slate.fee_fields)) }) .collect::, grin_wallet_libwallet::Error>>()?; - display::estimate(args.amount, strategies, dark_scheme); + display::estimate(amount, strategies, dark_scheme); return Ok(()); } else { let init_args = InitTxArgs { src_acct_name: None, - amount: args.amount, + amount: amount, + amount_includes_fee: Some(args.amount_includes_fee), minimum_confirmations: args.minimum_confirmations, max_outputs: args.max_outputs as u32, num_change_outputs: args.change_outputs as u32, @@ -394,7 +407,7 @@ where Ok(s) => { info!( "Tx created: {} grin to {} (strategy '{}')", - core::amount_to_hr_string(args.amount, false), + core::amount_to_hr_string(amount, false), args.dest, args.selection_strategy, ); diff --git a/controller/tests/transaction.rs b/controller/tests/transaction.rs index 1a66672d..6add296c 100644 --- a/controller/tests/transaction.rs +++ b/controller/tests/transaction.rs @@ -354,6 +354,47 @@ fn basic_transaction_api(test_dir: &'static str) -> Result<(), libwallet::Error> Ok(()) })?; + // try to send a transaction with amount inclusive of fees, but amount too + // small to cover fees. Should fail. + wallet::controller::owner_single_use(Some(wallet1.clone()), mask1, None, |sender_api, m| { + // note this will increment the block count as part of the transaction "Posting" + let args = InitTxArgs { + src_acct_name: None, + amount: 1, + amount_includes_fee: Some(true), + minimum_confirmations: 2, + max_outputs: 500, + num_change_outputs: 1, + selection_strategy_is_use_all: true, + ..Default::default() + }; + let res = sender_api.init_send_tx(m, args); + assert!(res.is_err()); + Ok(()) + })?; + + // try to build a transaction with amount inclusive of fees. Confirm that tx + // amount + fee is equal to the originally specified amount + let amount = 60_000_000_000; + wallet::controller::owner_single_use(Some(wallet1.clone()), mask1, None, |sender_api, m| { + // note this will increment the block count as part of the transaction "Posting" + let args = InitTxArgs { + src_acct_name: None, + amount: amount, + amount_includes_fee: Some(true), + minimum_confirmations: 2, + max_outputs: 500, + num_change_outputs: 1, + selection_strategy_is_use_all: true, + ..Default::default() + }; + let slate_i = sender_api.init_send_tx(m, args)?; + assert_eq!(slate_i.state, SlateState::Standard1); + let total_spend: u64 = slate_i.amount + slate_i.fee_fields.fee(); + assert_eq!(amount, total_spend); + Ok(()) + })?; + // let logging finish stopper.store(false, Ordering::Relaxed); thread::sleep(Duration::from_millis(200)); diff --git a/libwallet/src/api_impl/owner.rs b/libwallet/src/api_impl/owner.rs index b3e368d7..c93e4d7c 100644 --- a/libwallet/src/api_impl/owner.rs +++ b/libwallet/src/api_impl/owner.rs @@ -508,6 +508,7 @@ where &mut *w, keychain_mask, args.amount, + args.amount_includes_fee.unwrap_or(false), args.minimum_confirmations, args.max_outputs as usize, args.num_change_outputs as usize, @@ -543,6 +544,7 @@ where &parent_key_id, true, use_test_rng, + args.amount_includes_fee.unwrap_or(false), )? }; @@ -696,6 +698,7 @@ where &parent_key_id, false, use_test_rng, + false, )?; let keychain = w.keychain(keychain_mask)?; @@ -823,6 +826,7 @@ where parent_key_id.clone(), false, true, + false, )?; // Add inputs and outputs to original context diff --git a/libwallet/src/api_impl/types.rs b/libwallet/src/api_impl/types.rs index 5a245554..f54102d7 100644 --- a/libwallet/src/api_impl/types.rs +++ b/libwallet/src/api_impl/types.rs @@ -40,6 +40,8 @@ pub struct InitTxArgs { #[serde(with = "secp_ser::string_or_u64")] /// The amount to send, in nanogrins. (`1 G = 1_000_000_000nG`) pub amount: u64, + /// Does the amount include the fee, or will fees be spent in addition to the amount? + pub amount_includes_fee: Option, #[serde(with = "secp_ser::string_or_u64")] /// The minimum number of confirmations an output /// should have in order to be included in the transaction. @@ -105,6 +107,7 @@ impl Default for InitTxArgs { InitTxArgs { src_acct_name: None, amount: 0, + amount_includes_fee: None, minimum_confirmations: 10, max_outputs: 500, num_change_outputs: 1, diff --git a/libwallet/src/internal/selection.rs b/libwallet/src/internal/selection.rs index 7c66b967..441bad86 100644 --- a/libwallet/src/internal/selection.rs +++ b/libwallet/src/internal/selection.rs @@ -51,6 +51,7 @@ pub fn build_send_tx<'a, T: ?Sized, C, K>( parent_key_id: Identifier, use_test_nonce: bool, is_initiator: bool, + amount_includes_fee: bool, ) -> Result where T: WalletBackend<'a, C, K>, @@ -61,6 +62,7 @@ where wallet, keychain_mask, slate.amount, + amount_includes_fee, current_height, minimum_confirmations, max_outputs, @@ -69,6 +71,14 @@ where &parent_key_id, false, )?; + if amount_includes_fee { + slate.amount = slate + .amount + .checked_sub(fee) + .ok_or(ErrorKind::GenericError( + format!("Transaction amount is too small to include fee").into(), + ))?; + }; if fixed_fee.map(|f| fee != f).unwrap_or(false) { return Err(ErrorKind::Fee("The initially selected fee is not sufficient".into()).into()); @@ -312,6 +322,7 @@ pub fn select_send_tx<'a, T: ?Sized, C, K, B>( wallet: &mut T, keychain_mask: Option<&SecretKey>, amount: u64, + amount_includes_fee: bool, current_height: u64, minimum_confirmations: u64, max_outputs: usize, @@ -337,6 +348,7 @@ where let (coins, _total, amount, fee) = select_coins_and_fee( wallet, amount, + amount_includes_fee, current_height, minimum_confirmations, max_outputs, @@ -363,6 +375,7 @@ where pub fn select_coins_and_fee<'a, T: ?Sized, C, K>( wallet: &mut T, amount: u64, + amount_includes_fee: bool, current_height: u64, minimum_confirmations: u64, max_outputs: usize, @@ -401,7 +414,10 @@ where // First attempt to spend without change let mut fee = tx_fee(coins.len(), 1, 1); let mut total: u64 = coins.iter().map(|c| c.value).sum(); - let mut amount_with_fee = amount + fee; + let mut amount_with_fee = match amount_includes_fee { + true => amount, + false => amount + fee, + }; if total == 0 { return Err(ErrorKind::NotEnoughFunds { @@ -429,7 +445,10 @@ where // We need to add a change address or amount with fee is more than total if total != amount_with_fee { fee = tx_fee(coins.len(), num_outputs, 1); - amount_with_fee = amount + fee; + amount_with_fee = match amount_includes_fee { + true => amount, + false => amount + fee, + }; // Here check if we have enough outputs for the amount including fee otherwise // look for other outputs and check again @@ -458,10 +477,21 @@ where .1; fee = tx_fee(coins.len(), num_outputs, 1); total = coins.iter().map(|c| c.value).sum(); - amount_with_fee = amount + fee; + amount_with_fee = match amount_includes_fee { + true => amount, + false => amount + fee, + }; } } - Ok((coins, total, amount, fee)) + // If original amount includes fee, the new amount should + // be reduced, to accommodate the fee. + let new_amount = match amount_includes_fee { + true => amount.checked_sub(fee).ok_or(ErrorKind::GenericError( + format!("Transaction amount is too small to include fee").into(), + ))?, + false => amount, + }; + Ok((coins, total, new_amount, fee)) } /// Selects inputs and change for a transaction diff --git a/libwallet/src/internal/tx.rs b/libwallet/src/internal/tx.rs index 505b554d..8b67c4da 100644 --- a/libwallet/src/internal/tx.rs +++ b/libwallet/src/internal/tx.rs @@ -96,6 +96,7 @@ pub fn estimate_send_tx<'a, T: ?Sized, C, K>( wallet: &mut T, keychain_mask: Option<&SecretKey>, amount: u64, + amount_includes_fee: bool, minimum_confirmations: u64, max_outputs: usize, num_change_outputs: usize, @@ -128,6 +129,7 @@ where let (_coins, total, _amount, fee) = selection::select_coins_and_fee( wallet, amount, + amount_includes_fee, current_height, minimum_confirmations, max_outputs, @@ -151,6 +153,7 @@ pub fn add_inputs_to_slate<'a, T: ?Sized, C, K>( parent_key_id: &Identifier, is_initiator: bool, use_test_rng: bool, + amount_includes_fee: bool, ) -> Result where T: WalletBackend<'a, C, K>, @@ -181,6 +184,7 @@ where parent_key_id.clone(), use_test_rng, is_initiator, + amount_includes_fee, )?; // Generate a kernel offset and subtract from our context's secret key. Store @@ -270,6 +274,7 @@ where let (_coins, _total, _amount, fee) = selection::select_coins_and_fee( wallet, init_tx_args.amount, + init_tx_args.amount_includes_fee.unwrap_or(false), current_height, init_tx_args.minimum_confirmations, init_tx_args.max_outputs as usize, diff --git a/src/bin/grin-wallet.yml b/src/bin/grin-wallet.yml index 0f15afea..bd8401f1 100644 --- a/src/bin/grin-wallet.yml +++ b/src/bin/grin-wallet.yml @@ -109,7 +109,7 @@ subcommands: about: Builds a transaction to send coins and sends to the recipient via the Slatepack workflow args: - amount: - help: Number of coins to send with optional fraction, e.g. 12.423 + help: Number of coins to send with optional fraction, e.g. 12.423. Keyword 'max' will send maximum amount. index: 1 - minimum_confirmations: help: Minimum number of confirmations required for an output to be spendable @@ -181,6 +181,9 @@ subcommands: help: Show slatepack data as QR code short: q long: slatepack_qr + - amount_includes_fee: + help: Transaction amount includes transaction fee. Recipient will receive (amount - fee). + long: amount_includes_fee - unpack: about: Unpack and display an armored Slatepack Message, decrypting if possible args: diff --git a/src/cmd/wallet_args.rs b/src/cmd/wallet_args.rs index 87fb5243..596a7862 100644 --- a/src/cmd/wallet_args.rs +++ b/src/cmd/wallet_args.rs @@ -444,7 +444,11 @@ pub fn parse_account_args(account_args: &ArgMatches) -> Result Result { // amount let amount = parse_required(args, "amount")?; - let amount = core::core::amount_from_hr_string(amount); + let (amount, spend_max) = if amount.eq_ignore_ascii_case("max") { + (Ok(0), true) + } else { + (core::core::amount_from_hr_string(amount), false) + }; let amount = match amount { Ok(a) => a, Err(e) => { @@ -455,6 +459,7 @@ pub fn parse_send_args(args: &ArgMatches) -> Result Result Result<(), grin_wallet_controller:: }, )?; + // Send to wallet 2 with --amount_includes_fee + let mut old_balance = 0; + grin_wallet_controller::controller::owner_single_use( + Some(wallet1.clone()), + mask1, + None, + |api, m| { + api.set_active_account(m, "mining")?; + let (_, wallet1_info) = api.retrieve_summary_info(m, true, 1)?; + old_balance = wallet1_info.amount_currently_spendable; + Ok(()) + }, + )?; + let arg_vec = vec![ + "grin-wallet", + "-p", + "password1", + "-a", + "mining", + "send", + "--amount_includes_fee", + "10", + ]; + execute_command(&app, test_dir, "wallet1", &client1, arg_vec)?; + let file_name = format!( + "{}/wallet1/slatepack/0436430c-2b02-624c-2032-570501212b01.S1.slatepack", + test_dir + ); + let arg_vec = vec![ + "grin-wallet", + "-p", + "password2", + "-a", + "account_1", + "receive", + "-i", + &file_name, + ]; + execute_command(&app, test_dir, "wallet2", &client2, arg_vec.clone())?; + let file_name = format!( + "{}/wallet2/slatepack/0436430c-2b02-624c-2032-570501212b01.S2.slatepack", + test_dir + ); + let arg_vec = vec![ + "grin-wallet", + "-a", + "mining", + "-p", + "password1", + "finalize", + "-i", + &file_name, + ]; + execute_command(&app, test_dir, "wallet1", &client1, arg_vec)?; + bh += 1; + + // Mine some blocks to confirm the transaction + let _ = test_framework::award_blocks_to_wallet(&chain, wallet1.clone(), mask1, 10, false); + bh += 10; + + // Check the new balance of wallet 1 reduced by EXACTLY the tx amount (instead of amount + fee) + // This confirms that the TX amount was correctly computed to allow for the fee + grin_wallet_controller::controller::owner_single_use( + Some(wallet1.clone()), + mask1, + None, + |api, m| { + api.set_active_account(m, "mining")?; + let (_, wallet1_info) = api.retrieve_summary_info(m, true, 1)?; + // make sure the new balance is exactly equal to the old balance - the tx amount + the amount mined since then + let amt_mined = 10 * 60_000_000_000; + assert_eq!( + wallet1_info.amount_currently_spendable + 10_000_000_000, + old_balance + amt_mined + ); + Ok(()) + }, + )?; + // Send encrypted from wallet 1 to wallet 2 // output wallet 2's address for test creation purposes, let arg_vec = vec![ @@ -308,7 +387,7 @@ fn command_line_test_impl(test_dir: &str) -> Result<(), grin_wallet_controller:: execute_command(&app, test_dir, "wallet1", &client1, arg_vec)?; let file_name = format!( - "{}/wallet1/slatepack/0436430c-2b02-624c-2032-570501212b01.S1.slatepack", + "{}/wallet1/slatepack/0436430c-2b02-624c-2032-570501212b02.S1.slatepack", test_dir ); let arg_vec = vec![ @@ -324,7 +403,7 @@ fn command_line_test_impl(test_dir: &str) -> Result<(), grin_wallet_controller:: execute_command(&app, test_dir, "wallet2", &client2, arg_vec.clone())?; let file_name = format!( - "{}/wallet2/slatepack/0436430c-2b02-624c-2032-570501212b01.S2.slatepack", + "{}/wallet2/slatepack/0436430c-2b02-624c-2032-570501212b02.S2.slatepack", test_dir ); @@ -381,7 +460,7 @@ fn command_line_test_impl(test_dir: &str) -> Result<(), grin_wallet_controller:: execute_command(&app, test_dir, "wallet1", &client1, arg_vec)?; let file_name = format!( - "{}/wallet1/slatepack/0436430c-2b02-624c-2032-570501212b02.S1.slatepack", + "{}/wallet1/slatepack/0436430c-2b02-624c-2032-570501212b03.S1.slatepack", test_dir ); let arg_vec = vec![ @@ -397,7 +476,7 @@ fn command_line_test_impl(test_dir: &str) -> Result<(), grin_wallet_controller:: execute_command(&app, test_dir, "wallet1", &client1, arg_vec.clone())?; let file_name = format!( - "{}/wallet1/slatepack/0436430c-2b02-624c-2032-570501212b02.S2.slatepack", + "{}/wallet1/slatepack/0436430c-2b02-624c-2032-570501212b03.S2.slatepack", test_dir ); @@ -475,7 +554,7 @@ fn command_line_test_impl(test_dir: &str) -> Result<(), grin_wallet_controller:: "mining", "cancel", "-i", - "25", + "36", ]; execute_command(&app, test_dir, "wallet1", &client1, arg_vec)?; @@ -483,7 +562,7 @@ fn command_line_test_impl(test_dir: &str) -> Result<(), grin_wallet_controller:: let arg_vec = vec!["grin-wallet", "-p", "password2", "invoice", "65"]; execute_command(&app, test_dir, "wallet2", &client2, arg_vec)?; let file_name = format!( - "{}/wallet2/slatepack/0436430c-2b02-624c-2032-570501212b05.I1.slatepack", + "{}/wallet2/slatepack/0436430c-2b02-624c-2032-570501212b06.I1.slatepack", test_dir ); @@ -501,7 +580,7 @@ fn command_line_test_impl(test_dir: &str) -> Result<(), grin_wallet_controller:: execute_command(&app, test_dir, "wallet1", &client1, arg_vec)?; let file_name = format!( - "{}/wallet1/slatepack/0436430c-2b02-624c-2032-570501212b05.I2.slatepack", + "{}/wallet1/slatepack/0436430c-2b02-624c-2032-570501212b06.I2.slatepack", test_dir ); @@ -565,6 +644,73 @@ fn command_line_test_impl(test_dir: &str) -> Result<(), grin_wallet_controller:: let arg_vec = vec!["grin-wallet", "-p", "password2", "txs", "-t", &tx_id[..]]; execute_command(&app, test_dir, "wallet2", &client2, arg_vec)?; + // bit of mining + let _ = test_framework::award_blocks_to_wallet(&chain, wallet1.clone(), mask1, 10, false); + + // Test wallet sweep + let arg_vec = vec![ + "grin-wallet", + "-p", + "password1", + "-a", + "mining", + "send", + "max", + ]; + execute_command(&app, test_dir, "wallet1", &client1, arg_vec)?; + let file_name = format!( + "{}/wallet1/slatepack/0436430c-2b02-624c-2032-570501212b07.S1.slatepack", + test_dir + ); + let arg_vec = vec![ + "grin-wallet", + "-p", + "password2", + "-a", + "account_1", + "receive", + "-i", + &file_name, + ]; + execute_command(&app, test_dir, "wallet2", &client2, arg_vec.clone())?; + let file_name = format!( + "{}/wallet2/slatepack/0436430c-2b02-624c-2032-570501212b07.S2.slatepack", + test_dir + ); + let arg_vec = vec![ + "grin-wallet", + "-a", + "mining", + "-p", + "password1", + "finalize", + "-i", + &file_name, + ]; + execute_command(&app, test_dir, "wallet1", &client1, arg_vec)?; + + // Mine some blocks to confirm the transaction + let _ = test_framework::award_blocks_to_wallet(&chain, wallet1.clone(), mask1, 10, false); + + // Check wallet 1 is now empty, except for immature coinbase outputs from recent mining), + // and recently matured coinbase outputs, which were not mature at time of spending. + // This confirms that the TX amount was correctly computed to allow for the fee + grin_wallet_controller::controller::owner_single_use( + Some(wallet1.clone()), + mask1, + None, + |api, m| { + api.set_active_account(m, "mining")?; + let (_, wallet1_info) = api.retrieve_summary_info(m, true, 10)?; + // Entire 'spendable' wallet balance should have been swept, except the coinbase outputs + // which matured in the last batch of mining. Check that the new spendable balance is + // exactly equal to those matured coins. + let amt_mined = 10 * 60_000_000_000; + assert_eq!(wallet1_info.amount_currently_spendable, amt_mined); + Ok(()) + }, + )?; + // let logging finish thread::sleep(Duration::from_millis(200)); clean_output_dir(test_dir); diff --git a/tests/data/v3_reqs/init_send_tx.req.json b/tests/data/v3_reqs/init_send_tx.req.json index 3f9d6779..f73710d7 100644 --- a/tests/data/v3_reqs/init_send_tx.req.json +++ b/tests/data/v3_reqs/init_send_tx.req.json @@ -6,6 +6,7 @@ "args": { "src_acct_name": null, "amount": "600000000000", + "amount_includes_fee": false, "minimum_confirmations": 2, "max_outputs": 500, "num_change_outputs": 1,