Wallet tests, bug fixes (#2138)

* More tests for smallest selection, resulting bug fixes, remove unneeded TXLogEntryTypes

* rustfmt

src/bin/cmd/wallet_tests.rs

@@ -310,6 +310,17 @@ mod wallet_tests {
 		execute_command(&app, test_dir, "wallet1", &client1, arg_vec)?;
 		bh += 1;
 
+		let wallet1 = instantiate_wallet(config1.clone(), client1.clone(), "password", "default")?;
+
+		// Check our transaction log, should have 10 entries
+		grin_wallet::controller::owner_single_use(wallet1.clone(), |api| {
+			api.set_active_account("mining")?;
+			let (refreshed, txs) = api.retrieve_txs(true, None, None)?;
+			assert!(refreshed);
+			assert_eq!(txs.len(), bh as usize);
+			Ok(())
+		})?;
+
 		let _ = test_framework::award_blocks_to_wallet(&chain, wallet1.clone(), 10);
 		bh += 10;
 
@@ -338,10 +349,108 @@ mod wallet_tests {
 			Ok(())
 		})?;
 
-		// txs
+		// Self-send to same account, using smallest strategy
+		let arg_vec = vec![
+			"grin",
+			"wallet",
+			"-p",
+			"password",
+			"-a",
+			"mining",
+			"send",
+			"-m",
+			"file",
+			"-d",
+			&file_name,
+			"-g",
+			"Love, Yeast, Smallest",
+			"-s",
+			"smallest",
+			"10",
+		];
+		execute_command(&app, test_dir, "wallet1", &client1, arg_vec)?;
+
+		let arg_vec = vec![
+			"grin",
+			"wallet",
+			"-p",
+			"password",
+			"-a",
+			"mining",
+			"receive",
+			"-i",
+			&file_name,
+			"-g",
+			"Thanks, Yeast!",
+		];
+		execute_command(&app, test_dir, "wallet1", &client1, arg_vec.clone())?;
+
+		let arg_vec = vec![
+			"grin",
+			"wallet",
+			"-p",
+			"password",
+			"finalize",
+			"-i",
+			&response_file_name,
+		];
+		execute_command(&app, test_dir, "wallet1", &client1, arg_vec)?;
+		bh += 1;
+
+		// Check our transaction log, should have bh entries + one for the self receive
+		let wallet1 = instantiate_wallet(config1.clone(), client1.clone(), "password", "default")?;
+
+		grin_wallet::controller::owner_single_use(wallet1.clone(), |api| {
+			api.set_active_account("mining")?;
+			let (refreshed, txs) = api.retrieve_txs(true, None, None)?;
+			assert!(refreshed);
+			assert_eq!(txs.len(), bh as usize + 1);
+			Ok(())
+		})?;
+
+		// Try using the self-send method
+		let arg_vec = vec![
+			"grin",
+			"wallet",
+			"-p",
+			"password",
+			"-a",
+			"mining",
+			"send",
+			"-m",
+			"self",
+			"-d",
+			"mining",
+			"-g",
+			"Self love",
+			"-s",
+			"smallest",
+			"10",
+		];
+		execute_command(&app, test_dir, "wallet1", &client1, arg_vec)?;
+		bh += 1;
+
+		// Check our transaction log, should have bh entries + 2 for the self receives
+		let wallet1 = instantiate_wallet(config1.clone(), client1.clone(), "password", "default")?;
+
+		grin_wallet::controller::owner_single_use(wallet1.clone(), |api| {
+			api.set_active_account("mining")?;
+			let (refreshed, txs) = api.retrieve_txs(true, None, None)?;
+			assert!(refreshed);
+			assert_eq!(txs.len(), bh as usize + 2);
+			Ok(())
+		})?;
+
+		// txs and outputs (mostly spit out for a visual in test logs)
 		let arg_vec = vec!["grin", "wallet", "-p", "password", "-a", "mining", "txs"];
 		execute_command(&app, test_dir, "wallet1", &client1, arg_vec)?;
 
+		// txs and outputs (mostly spit out for a visual in test logs)
+		let arg_vec = vec![
+			"grin", "wallet", "-p", "password", "-a", "mining", "outputs",
+		];
+		execute_command(&app, test_dir, "wallet1", &client1, arg_vec)?;
+
 		// let logging finish
 		thread::sleep(Duration::from_millis(200));
 		Ok(())

wallet/src/command.rs

@@ -234,13 +234,13 @@ pub fn send(
 		};
 		if adapter.supports_sync() {
 			slate = adapter.send_tx_sync(&args.dest, &slate)?;
+			api.tx_lock_outputs(&slate, lock_fn)?;
 			if args.method == "self" {
 				controller::foreign_single_use(wallet, |api| {
 					api.receive_tx(&mut slate, Some(&args.dest), None)?;
 					Ok(())
 				})?;
 			}
-			api.tx_lock_outputs(&slate, lock_fn)?;
 			if let Err(e) = api.verify_slate_messages(&slate) {
 				error!("Error validating participant messages: {}", e);
 				return Err(e);

wallet/src/libwallet/api.rs

@@ -41,8 +41,8 @@ use crate::core::ser;
 use crate::keychain::{Identifier, Keychain};
 use crate::libwallet::internal::{keys, tx, updater};
 use crate::libwallet::types::{
-	AcctPathMapping, BlockFees, CbData, NodeClient, OutputData, TxLogEntry, TxWrapper,
+	AcctPathMapping, BlockFees, CbData, NodeClient, OutputData, TxLogEntry, TxLogEntryType,
-	WalletBackend, WalletInfo,
+	TxWrapper, WalletBackend, WalletInfo,
 };
 use crate::libwallet::{Error, ErrorKind};
 use crate::util;
@@ -634,7 +634,6 @@ where
 			num_change_outputs,
 			selection_strategy_is_use_all,
 			&parent_key_id,
-			false,
 			message,
 		)?;
 
@@ -834,10 +833,12 @@ where
 		};
 		// Don't do this multiple times
 		let tx = updater::retrieve_txs(&mut *w, None, Some(slate.id), Some(&parent_key_id))?;
-		if tx.len() > 0 {
+		for t in &tx {
+			if t.tx_type == TxLogEntryType::TxReceived {
 				return Err(ErrorKind::TransactionAlreadyReceived(slate.id.to_string()).into());
 			}
-		let res = tx::receive_tx(&mut *w, slate, &parent_key_id, false, message);
+		}
+		let res = tx::receive_tx(&mut *w, slate, &parent_key_id, message);
 		w.close()?;
 
 		if let Err(e) = res {

wallet/src/libwallet/internal/selection.rs

@@ -36,7 +36,6 @@ pub fn build_send_tx_slate<T: ?Sized, C, K>(
 	change_outputs: usize,
 	selection_strategy_is_use_all: bool,
 	parent_key_id: Identifier,
-	is_self: bool,
 ) -> Result<
 	(
 		Slate,
@@ -99,9 +98,6 @@ where
 		let mut batch = wallet.batch()?;
 		let log_id = batch.next_tx_log_id(&parent_key_id)?;
 		let mut t = TxLogEntry::new(parent_key_id.clone(), TxLogEntryType::TxSent, log_id);
-		if is_self {
-			t.tx_type = TxLogEntryType::TxSentSelf;
-		}
 		t.tx_slate_id = Some(slate_id);
 		t.fee = Some(fee);
 		t.tx_hex = Some(tx_hex.to_owned());
@@ -148,7 +144,6 @@ pub fn build_recipient_output_with_slate<T: ?Sized, C, K>(
 	wallet: &mut T,
 	slate: &mut Slate,
 	parent_key_id: Identifier,
-	is_self: bool,
 ) -> Result<
 	(
 		Identifier,
@@ -190,9 +185,6 @@ where
 		let mut batch = wallet.batch()?;
 		let log_id = batch.next_tx_log_id(&parent_key_id)?;
 		let mut t = TxLogEntry::new(parent_key_id.clone(), TxLogEntryType::TxReceived, log_id);
-		if is_self {
-			t.tx_type = TxLogEntryType::TxReceivedSelf;
-		}
 		t.tx_slate_id = Some(slate_id);
 		t.amount_credited = amount;
 		t.num_outputs = 1;

wallet/src/libwallet/internal/tx.rs

@@ -30,7 +30,6 @@ pub fn receive_tx<T: ?Sized, C, K>(
 	wallet: &mut T,
 	slate: &mut Slate,
 	parent_key_id: &Identifier,
-	is_self: bool,
 	message: Option<String>,
 ) -> Result<(), Error>
 where
@@ -39,12 +38,8 @@ where
 	K: Keychain,
 {
 	// create an output using the amount in the slate
-	let (_, mut context, receiver_create_fn) = selection::build_recipient_output_with_slate(
+	let (_, mut context, receiver_create_fn) =
-		wallet,
+		selection::build_recipient_output_with_slate(wallet, slate, parent_key_id.clone())?;
-		slate,
-		parent_key_id.clone(),
-		is_self,
-	)?;
 
 	// fill public keys
 	let _ = slate.fill_round_1(
@@ -74,7 +69,6 @@ pub fn create_send_tx<T: ?Sized, C, K>(
 	num_change_outputs: usize,
 	selection_strategy_is_use_all: bool,
 	parent_key_id: &Identifier,
-	is_self: bool,
 	message: Option<String>,
 ) -> Result<
 	(
@@ -114,7 +108,6 @@ where
 		num_change_outputs,
 		selection_strategy_is_use_all,
 		parent_key_id.clone(),
-		is_self,
 	)?;
 
 	// Generate a kernel offset and subtract from our context's secret key. Store
@@ -210,7 +203,7 @@ where
 /// Update the stored hex transaction (this update needs to happen when the TX is finalised)
 pub fn update_tx_hex<T: ?Sized, C, K>(
 	wallet: &mut T,
-	parent_key_id: &Identifier,
+	_parent_key_id: &Identifier,
 	slate: &Slate,
 ) -> Result<(), Error>
 where
@@ -220,15 +213,23 @@ where
 {
 	let tx_hex = util::to_hex(ser::ser_vec(&slate.tx).unwrap());
 	// This will ignore the parent key, so no need to specify account on the
-	// finalise command
+	// finalize command
 	let tx_vec = updater::retrieve_txs(wallet, None, Some(slate.id), None)?;
-	if tx_vec.len() != 1 {
+	let mut tx = None;
-		return Err(ErrorKind::TransactionDoesntExist(slate.id.to_string()))?;
+	// don't want to assume this is the right tx, in case of self-sending
+	for t in tx_vec {
+		if t.tx_type == TxLogEntryType::TxSent {
+			tx = Some(t.clone());
+			break;
 		}
-	let mut tx = tx_vec[0].clone();
+	}
+	let mut tx = match tx {
+		Some(t) => t,
+		None => return Err(ErrorKind::TransactionDoesntExist(slate.id.to_string()))?,
+	};
 	tx.tx_hex = Some(tx_hex);
 	let batch = wallet.batch()?;
-	batch.save_tx_log_entry(tx, &parent_key_id)?;
+	batch.save_tx_log_entry(tx.clone(), &tx.parent_key_id)?;
 	batch.commit()?;
 	Ok(())
 }

wallet/src/libwallet/types.rs

@@ -544,10 +544,6 @@ pub enum TxLogEntryType {
 	TxReceived,
 	/// Inputs locked + change outputs when a transaction is created
 	TxSent,
-	/// As above, but self-transaction
-	TxReceivedSelf,
-	/// As Above
-	TxSentSelf,
 	/// Received transaction that was rolled back by user
 	TxReceivedCancelled,
 	/// Sent transaction that was rolled back by user
@@ -560,8 +556,6 @@ impl fmt::Display for TxLogEntryType {
 			TxLogEntryType::ConfirmedCoinbase => write!(f, "Confirmed \nCoinbase"),
 			TxLogEntryType::TxReceived => write!(f, "Received Tx"),
 			TxLogEntryType::TxSent => write!(f, "Sent Tx"),
-			TxLogEntryType::TxReceivedSelf => write!(f, "Received Tx (Self)"),
-			TxLogEntryType::TxSentSelf => write!(f, "Sent Tx (Self)"),
 			TxLogEntryType::TxReceivedCancelled => write!(f, "Received Tx\n- Cancelled"),
 			TxLogEntryType::TxSentCancelled => write!(f, "Send Tx\n- Cancelled"),
 		}