Fixes to grin wallet repost, cancel tx (#2029)

* fixes to wallet cancel, repost, ensure stored transaction is updated with final signatures

* rustfmt

* add tests for reposting

* fixes to tests

* repost

src/bin/cmd/wallet.rs

@@ -453,11 +453,12 @@ pub fn wallet_command(wallet_args: &ArgMatches, config: GlobalWalletConfig) -> i
 							Ok(())
 						})?;
 					}
+					api.tx_lock_outputs(&slate, lock_fn)?;
 					api.finalize_tx(&mut slate)?;
 				} else {
 					adapter.send_tx_async(dest, &slate)?;
+					api.tx_lock_outputs(&slate, lock_fn)?;
 				}
-				api.tx_lock_outputs(&slate, lock_fn)?;
 				if adapter.supports_sync() {
 					let result = api.post_tx(&slate, fluff);
 					match result {

wallet/src/libwallet/api.rs

@@ -222,9 +222,11 @@ where
 	/// propagation.
 	pub fn finalize_tx(&mut self, slate: &mut Slate) -> Result<(), Error> {
 		let mut w = self.wallet.lock();
+		let parent_key_id = w.parent_key_id();
 		w.open_with_credentials()?;
 		let context = w.get_private_context(slate.id.as_bytes())?;
 		tx::complete_tx(&mut *w, slate, &context)?;
+		tx::update_tx_hex(&mut *w, &parent_key_id, slate)?;
 		{
 			let mut batch = w.batch()?;
 			batch.delete_private_context(slate.id.as_bytes())?;
@@ -367,23 +369,13 @@ where
 			);
 			return Err(ErrorKind::TransactionBuildingNotCompleted(tx_id))?;
 		}
-
-		let res = client.post_tx(
+		client.post_tx(
 			&TxWrapper {
 				tx_hex: tx_hex.unwrap(),
 			},
 			fluff,
-		);
-		if let Err(e) = res {
-			error!("api: repost_tx: failed with error: {}", e);
-			Err(e)
-		} else {
-			debug!(
-				"api: repost_tx: successfully posted tx at: {}, fluff? {}",
-				tx_id, fluff
-			);
-			Ok(())
-		}
+		)?;
+		Ok(())
 	}
 
 	/// Attempt to restore contents of wallet

wallet/src/libwallet/error.rs

@@ -195,7 +195,19 @@ pub enum ErrorKind {
 
 impl Display for Error {
 	fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
-		Display::fmt(&self.inner, f)
+		let cause = match self.cause() {
+			Some(c) => format!("{}", c),
+			None => String::from("Unknown"),
+		};
+		let backtrace = match self.backtrace() {
+			Some(b) => format!("{}", b),
+			None => String::from("Unknown"),
+		};
+		let output = format!(
+			"{} \n Cause: {} \n Backtrace: {}",
+			self.inner, cause, backtrace
+		);
+		Display::fmt(&output, f)
 	}
 }
 

wallet/src/libwallet/internal/tx.rs

@@ -15,11 +15,12 @@
 //! Transaction building functions
 
 use std::sync::Arc;
-use util::RwLock;
+use util::{self, RwLock};
 use uuid::Uuid;
 
 use core::core::verifier_cache::LruVerifierCache;
 use core::core::Transaction;
+use core::ser;
 use keychain::{Identifier, Keychain};
 use libtx::slate::Slate;
 use libtx::{build, tx_fee};
@@ -206,6 +207,30 @@ where
 	Ok((tx.confirmed, tx.tx_hex))
 }
 
+/// 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,
+	slate: &Slate,
+) -> Result<(), Error>
+where
+	T: WalletBackend<C, K>,
+	C: NodeClient,
+	K: Keychain,
+{
+	let tx_hex = util::to_hex(ser::ser_vec(&slate.tx).unwrap());
+	let tx_vec = updater::retrieve_txs(wallet, None, Some(slate.id), parent_key_id)?;
+	if tx_vec.len() != 1 {
+		return Err(ErrorKind::TransactionDoesntExist(slate.id.to_string()))?;
+	}
+	let mut tx = tx_vec[0].clone();
+	tx.tx_hex = Some(tx_hex);
+	let batch = wallet.batch()?;
+	batch.save_tx_log_entry(tx, &parent_key_id)?;
+	batch.commit()?;
+	Ok(())
+}
+
 /// Issue a burn tx
 pub fn issue_burn_tx<T: ?Sized, C, K>(
 	wallet: &mut T,

wallet/src/libwallet/internal/updater.rs

@@ -91,7 +91,9 @@ where
 {
 	// just read the wallet here, no need for a write lock
 	let mut txs = if let Some(id) = tx_id {
-		let tx = wallet.tx_log_iter().find(|t| t.id == id);
+		let tx = wallet
+			.tx_log_iter()
+			.find(|t| t.id == id && t.parent_key_id == *parent_key_id);
 		if let Some(t) = tx {
 			vec![t]
 		} else {

wallet/tests/accounts.rs

@@ -192,8 +192,8 @@ fn accounts_test_impl(test_dir: &str) -> Result<(), libwallet::Error> {
 			true,   // select all outputs
 		)?;
 		slate = client1.send_tx_slate_direct("wallet2", &slate)?;
-		api.finalize_tx(&mut slate)?;
 		api.tx_lock_outputs(&slate, lock_fn)?;
+		api.finalize_tx(&mut slate)?;
 		api.post_tx(&slate, false)?;
 		Ok(())
 	})?;

wallet/tests/repost.rs

@@ -0,0 +1,259 @@
+// Copyright 2018 The Grin Developers
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+//! Test a wallet repost command
+extern crate grin_chain as chain;
+extern crate grin_core as core;
+extern crate grin_keychain as keychain;
+extern crate grin_store as store;
+extern crate grin_util as util;
+extern crate grin_wallet as wallet;
+extern crate rand;
+#[macro_use]
+extern crate log;
+extern crate chrono;
+extern crate serde;
+extern crate uuid;
+
+mod common;
+use common::testclient::{LocalWalletClient, WalletProxy};
+
+use std::fs;
+use std::thread;
+use std::time::Duration;
+
+use core::global;
+use core::global::ChainTypes;
+use keychain::ExtKeychain;
+use wallet::libtx::slate::Slate;
+use wallet::{libwallet, FileWalletCommAdapter};
+
+fn clean_output_dir(test_dir: &str) {
+	let _ = fs::remove_dir_all(test_dir);
+}
+
+fn setup(test_dir: &str) {
+	util::init_test_logger();
+	clean_output_dir(test_dir);
+	global::set_mining_mode(ChainTypes::AutomatedTesting);
+}
+
+/// self send impl
+fn file_repost_test_impl(test_dir: &str) -> Result<(), libwallet::Error> {
+	setup(test_dir);
+	// Create a new proxy to simulate server and wallet responses
+	let mut wallet_proxy: WalletProxy<LocalWalletClient, ExtKeychain> = WalletProxy::new(test_dir);
+	let chain = wallet_proxy.chain.clone();
+
+	let client1 = LocalWalletClient::new("wallet1", wallet_proxy.tx.clone());
+	let wallet1 = common::create_wallet(&format!("{}/wallet1", test_dir), client1.clone());
+	wallet_proxy.add_wallet("wallet1", client1.get_send_instance(), wallet1.clone());
+
+	let client2 = LocalWalletClient::new("wallet2", wallet_proxy.tx.clone());
+	let wallet2 = common::create_wallet(&format!("{}/wallet2", test_dir), client2.clone());
+	wallet_proxy.add_wallet("wallet2", client2.get_send_instance(), wallet2.clone());
+
+	// Set the wallet proxy listener running
+	thread::spawn(move || {
+		if let Err(e) = wallet_proxy.run() {
+			error!("Wallet Proxy error: {}", e);
+		}
+	});
+
+	// few values to keep things shorter
+	let reward = core::consensus::REWARD;
+
+	// add some accounts
+	wallet::controller::owner_single_use(wallet1.clone(), |api| {
+		api.new_account_path("mining")?;
+		api.new_account_path("listener")?;
+		Ok(())
+	})?;
+
+	// add some accounts
+	wallet::controller::owner_single_use(wallet2.clone(), |api| {
+		api.new_account_path("account1")?;
+		api.new_account_path("account2")?;
+		Ok(())
+	})?;
+
+	// Get some mining done
+	{
+		let mut w = wallet1.lock();
+		w.set_parent_key_id_by_name("mining")?;
+	}
+	let mut bh = 10u64;
+	let _ = common::award_blocks_to_wallet(&chain, wallet1.clone(), bh as usize);
+
+	let send_file = format!("{}/part_tx_1.tx", test_dir);
+	let receive_file = format!("{}/part_tx_2.tx", test_dir);
+
+	// Should have 5 in account1 (5 spendable), 5 in account (2 spendable)
+	wallet::controller::owner_single_use(wallet1.clone(), |api| {
+		let (wallet1_refreshed, wallet1_info) = api.retrieve_summary_info(true, 1)?;
+		assert!(wallet1_refreshed);
+		assert_eq!(wallet1_info.last_confirmed_height, bh);
+		assert_eq!(wallet1_info.total, bh * reward);
+		// send to send
+		let (mut slate, lock_fn) = api.initiate_tx(
+			Some("mining"),
+			reward * 2, // amount
+			2,          // minimum confirmations
+			500,        // max outputs
+			1,          // num change outputs
+			true,       // select all outputs
+		)?;
+		// output tx file
+		let file_adapter = FileWalletCommAdapter::new();
+		file_adapter.send_tx_async(&send_file, &mut slate)?;
+		api.tx_lock_outputs(&slate, lock_fn)?;
+		Ok(())
+	})?;
+
+	let _ = common::award_blocks_to_wallet(&chain, wallet1.clone(), 3);
+	bh += 3;
+
+	// wallet 1 receives file to different account, completes
+	{
+		let mut w = wallet1.lock();
+		w.set_parent_key_id_by_name("listener")?;
+	}
+
+	wallet::controller::foreign_single_use(wallet1.clone(), |api| {
+		let adapter = FileWalletCommAdapter::new();
+		let mut slate = adapter.receive_tx_async(&send_file)?;
+		api.receive_tx(&mut slate, None)?;
+		adapter.send_tx_async(&receive_file, &mut slate)?;
+		Ok(())
+	})?;
+
+	// wallet 1 receives file to different account, completes
+	{
+		let mut w = wallet1.lock();
+		w.set_parent_key_id_by_name("mining")?;
+	}
+
+	let mut slate = Slate::blank(2);
+	// wallet 1 finalize
+	wallet::controller::owner_single_use(wallet1.clone(), |api| {
+		let adapter = FileWalletCommAdapter::new();
+		slate = adapter.receive_tx_async(&receive_file)?;
+		api.finalize_tx(&mut slate)?;
+		Ok(())
+	})?;
+
+	// Now repost from cached
+	wallet::controller::owner_single_use(wallet1.clone(), |api| {
+		let (_, txs) = api.retrieve_txs(true, None, Some(slate.id))?;
+		api.post_stored_tx(txs[0].id, false)?;
+		bh += 1;
+		Ok(())
+	})?;
+
+	let _ = common::award_blocks_to_wallet(&chain, wallet1.clone(), 3);
+	bh += 3;
+
+	// update/test contents of both accounts
+	wallet::controller::owner_single_use(wallet1.clone(), |api| {
+		let (wallet1_refreshed, wallet1_info) = api.retrieve_summary_info(true, 1)?;
+		assert!(wallet1_refreshed);
+		assert_eq!(wallet1_info.last_confirmed_height, bh);
+		assert_eq!(wallet1_info.total, bh * reward - reward * 2);
+		Ok(())
+	})?;
+
+	{
+		let mut w = wallet1.lock();
+		w.set_parent_key_id_by_name("listener")?;
+	}
+
+	wallet::controller::owner_single_use(wallet1.clone(), |api| {
+		let (wallet2_refreshed, wallet2_info) = api.retrieve_summary_info(true, 1)?;
+		assert!(wallet2_refreshed);
+		assert_eq!(wallet2_info.last_confirmed_height, bh);
+		assert_eq!(wallet2_info.total, 2 * reward);
+		Ok(())
+	})?;
+
+	// as above, but syncronously
+	{
+		let mut w = wallet1.lock();
+		w.set_parent_key_id_by_name("mining")?;
+	}
+	{
+		let mut w = wallet2.lock();
+		w.set_parent_key_id_by_name("account1")?;
+	}
+
+	let mut slate = Slate::blank(2);
+	let amount = 60_000_000_000;
+
+	wallet::controller::owner_single_use(wallet1.clone(), |sender_api| {
+		// note this will increment the block count as part of the transaction "Posting"
+		let (slate_i, lock_fn) = sender_api.initiate_tx(
+			None,
+			amount * 2, // amount
+			2,          // minimum confirmations
+			500,        // max outputs
+			1,          // num change outputs
+			true,       // select all outputs
+		)?;
+		slate = client1.send_tx_slate_direct("wallet2", &slate_i)?;
+		sender_api.tx_lock_outputs(&slate, lock_fn)?;
+		sender_api.finalize_tx(&mut slate)?;
+		Ok(())
+	})?;
+
+	let _ = common::award_blocks_to_wallet(&chain, wallet1.clone(), 3);
+	bh += 3;
+
+	// Now repost from cached
+	wallet::controller::owner_single_use(wallet1.clone(), |api| {
+		let (_, txs) = api.retrieve_txs(true, None, Some(slate.id))?;
+		api.post_stored_tx(txs[0].id, false)?;
+		bh += 1;
+		Ok(())
+	})?;
+
+	let _ = common::award_blocks_to_wallet(&chain, wallet1.clone(), 3);
+	bh += 3;
+	//
+	// update/test contents of both accounts
+	wallet::controller::owner_single_use(wallet1.clone(), |api| {
+		let (wallet1_refreshed, wallet1_info) = api.retrieve_summary_info(true, 1)?;
+		assert!(wallet1_refreshed);
+		assert_eq!(wallet1_info.last_confirmed_height, bh);
+		assert_eq!(wallet1_info.total, bh * reward - reward * 4);
+		Ok(())
+	})?;
+
+	wallet::controller::owner_single_use(wallet2.clone(), |api| {
+		let (wallet2_refreshed, wallet2_info) = api.retrieve_summary_info(true, 1)?;
+		assert!(wallet2_refreshed);
+		assert_eq!(wallet2_info.last_confirmed_height, bh);
+		assert_eq!(wallet2_info.total, 2 * amount);
+		Ok(())
+	})?;
+
+	// let logging finish
+	thread::sleep(Duration::from_millis(200));
+	Ok(())
+}
+
+#[test]
+fn wallet_file_repost() {
+	let test_dir = "test_output/file_repost";
+	if let Err(e) = file_repost_test_impl(test_dir) {
+		panic!("Libwallet Error: {} - {}", e, e.backtrace().unwrap());
+	}
+}

wallet/tests/restore.rs

@@ -238,8 +238,8 @@ fn setup_restore(test_dir: &str) -> Result<(), libwallet::Error> {
 			true,   // select all outputs
 		)?;
 		slate = client1.send_tx_slate_direct("wallet2", &slate_i)?;
-		sender_api.finalize_tx(&mut slate)?;
 		sender_api.tx_lock_outputs(&slate, lock_fn)?;
+		sender_api.finalize_tx(&mut slate)?;
 		sender_api.post_tx(&slate, false)?;
 		Ok(())
 	})?;
@@ -259,8 +259,8 @@ fn setup_restore(test_dir: &str) -> Result<(), libwallet::Error> {
 			true,       // select all outputs
 		)?;
 		slate = client1.send_tx_slate_direct("wallet3", &slate_i)?;
-		sender_api.finalize_tx(&mut slate)?;
 		sender_api.tx_lock_outputs(&slate, lock_fn)?;
+		sender_api.finalize_tx(&mut slate)?;
 		sender_api.post_tx(&slate, false)?;
 		Ok(())
 	})?;
@@ -280,8 +280,8 @@ fn setup_restore(test_dir: &str) -> Result<(), libwallet::Error> {
 			true,       // select all outputs
 		)?;
 		slate = client3.send_tx_slate_direct("wallet2", &slate_i)?;
-		sender_api.finalize_tx(&mut slate)?;
 		sender_api.tx_lock_outputs(&slate, lock_fn)?;
+		sender_api.finalize_tx(&mut slate)?;
 		sender_api.post_tx(&slate, false)?;
 		Ok(())
 	})?;
@@ -307,8 +307,8 @@ fn setup_restore(test_dir: &str) -> Result<(), libwallet::Error> {
 			true,       // select all outputs
 		)?;
 		slate = client3.send_tx_slate_direct("wallet2", &slate_i)?;
-		sender_api.finalize_tx(&mut slate)?;
 		sender_api.tx_lock_outputs(&slate, lock_fn)?;
+		sender_api.finalize_tx(&mut slate)?;
 		sender_api.post_tx(&slate, false)?;
 		Ok(())
 	})?;

wallet/tests/transaction.rs

@@ -112,8 +112,8 @@ fn basic_transaction_api(test_dir: &str) -> Result<(), libwallet::Error> {
 			true,   // select all outputs
 		)?;
 		slate = client1.send_tx_slate_direct("wallet2", &slate_i)?;
-		sender_api.finalize_tx(&mut slate)?;
 		sender_api.tx_lock_outputs(&slate, lock_fn)?;
+		sender_api.finalize_tx(&mut slate)?;
 		Ok(())
 	})?;