slatepack: return concrete error on sync workflow instead of none, optimize sending requirement detection

api/src/foreign.rs

@@ -370,18 +370,22 @@ where
 		)?;
 		match r_addr {
 			Some(a) => {
-				let tor_config_lock = self.tor_config.lock();
-				let res = try_slatepack_sync_workflow(
-					&ret_slate,
-					&a,
-					tor_config_lock.clone(),
-					None,
-					true,
-					self.doctest_mode,
-				);
+				let tc = self.tor_config.lock();
+				let can_send = if let Some(tc) = tc.as_ref() {
+					tc.skip_send(None)
+				} else {
+					false
+				};
+				if self.doctest_mode || !can_send {
+					return Ok(ret_slate);
+				}
+				let res = try_slatepack_sync_workflow(&ret_slate, &a, tc.clone(), None, true);
 				match res {
-					Ok(s) => Ok(s.unwrap()),
-					Err(_) => Ok(ret_slate),
+					Ok(s) => Ok(s),
+					Err(e) => {
+						error!("Error on sending over Tor: {}", e);
+						Ok(ret_slate)
+					}
 				}
 			}
 			None => Ok(ret_slate),

api/src/owner.rs

@@ -37,6 +37,7 @@ use crate::util::logger::LoggingConfig;
 use crate::util::secp::{key::SecretKey, pedersen::Commitment};
 use crate::util::{from_hex, static_secp_instance, Mutex, ZeroingString};
 use grin_wallet_util::OnionV3Address;
+use libwallet::api_impl::types::update_tx_slate_state;
 use std::convert::TryFrom;
 use std::sync::atomic::{AtomicBool, Ordering};
 use std::sync::mpsc::{channel, Sender};
@@ -673,28 +674,27 @@ where
 			Some(sa) => {
 				let tor_config_lock = self.tor_config.lock();
 				let tc = tor_config_lock.clone();
-				let tc = match tc {
-					Some(mut c) => {
-						if let Some(skip_tor) = sa.skip_tor {
-							c.skip_send_attempt = Some(skip_tor);
-						}
-						Some(c)
-					}
-					None => None,
+				let can_send = if let Some(tc) = tc.as_ref() {
+					tc.skip_send(sa.skip_tor)
+				} else {
+					false
 				};
-				let res = try_slatepack_sync_workflow(
-					&slate,
-					&sa.dest,
-					tc,
-					None,
-					false,
-					self.doctest_mode,
-				);
+				if self.doctest_mode || !can_send {
+					return Ok(slate);
+				}
+				let res = try_slatepack_sync_workflow(&slate, &sa.dest, tc, None, false);
 				match res {
-					Ok(Some(s)) => {
+					Ok(s) => {
+						self.tx_lock_outputs(keychain_mask, &s)?;
+						let ret_slate = self.finalize_tx(keychain_mask, &s)?;
+						// Update slate state to actual.
+						{
+							let mut w_lock = self.wallet_inst.lock();
+							let w = w_lock.lc_provider()?.wallet_inst()?;
+							let parent_key_id = w.parent_key_id();
+							let _ = update_tx_slate_state(w, keychain_mask, &parent_key_id, &s);
+						}
 						if sa.post_tx {
-							self.tx_lock_outputs(keychain_mask, &s)?;
-							let ret_slate = self.finalize_tx(keychain_mask, &s)?;
 							let result = self.post_tx(keychain_mask, &ret_slate, sa.fluff);
 							match result {
 								Ok(_) => {
@@ -707,13 +707,13 @@ where
 								}
 							}
 						} else {
-							self.tx_lock_outputs(keychain_mask, &s)?;
-							let ret_slate = self.finalize_tx(keychain_mask, &s)?;
 							Ok(ret_slate)
 						}
 					}
-					Ok(None) => Ok(slate),
-					Err(_) => Ok(slate),
+					Err(e) => {
+						error!("Error on sending over Tor: {}", e);
+						Ok(slate)
+					}
 				}
 			}
 			None => Ok(slate),
@@ -841,26 +841,25 @@ where
 			Some(sa) => {
 				let tor_config_lock = self.tor_config.lock();
 				let tc = tor_config_lock.clone();
-				let tc = match tc {
-					Some(mut c) => {
-						if let Some(skip_tor) = sa.skip_tor {
-							c.skip_send_attempt = Some(skip_tor);
-						}
-						Some(c)
-					}
-					None => None,
+				let can_send = if let Some(tc) = tc.as_ref() {
+					tc.skip_send(sa.skip_tor)
+				} else {
+					false
 				};
-				let res = try_slatepack_sync_workflow(
-					&slate,
-					&sa.dest,
-					tc,
-					None,
-					true,
-					self.doctest_mode,
-				);
+				if self.doctest_mode || !can_send {
+					return Ok(slate);
+				}
+				let res = try_slatepack_sync_workflow(&slate, &sa.dest, tc, None, true);
 				match res {
-					Ok(s) => Ok(s.unwrap()),
-					Err(_) => Ok(slate),
+					Ok(s) => {
+						let parent_key_id = w.parent_key_id();
+						let _ = update_tx_slate_state(w, keychain_mask, &parent_key_id, &s);
+						Ok(s)
+					}
+					Err(e) => {
+						error!("Error on sending over Tor: {}", e);
+						Ok(slate)
+					}
 				}
 			}
 			None => Ok(slate),
@@ -2497,13 +2496,7 @@ pub fn try_slatepack_sync_workflow(
 	tor_config: Option<TorConfig>,
 	tor_sender: Option<TorSlateSender>,
 	send_to_finalize: bool,
-	test_mode: bool,
-) -> Result<Option<Slate>, Error> {
-	if let Some(tc) = &tor_config {
-		if tc.skip_send_attempt == Some(true) {
-			return Ok(None);
-		}
-	}
+) -> Result<Slate, Error> {
 	let mut ret_slate = Slate::blank(2, false);
 	let mut send_sync = |mut sender: TorSlateSender, method_str: &str| match sender
 		.send_tx(&slate, send_to_finalize)
@@ -2521,55 +2514,43 @@ pub fn try_slatepack_sync_workflow(
 		}
 	};
 
-	// Try parsing Slatepack address
+	// Try parsing Slatepack address.
 	match SlatepackAddress::try_from(dest) {
 		Ok(address) => {
-			let tor_addr = OnionV3Address::try_from(&address).unwrap();
-			// Try sending to the destination via TOR
+			let tor_addr = OnionV3Address::try_from(&address).map_err(|_| {
+				Error::SlatepackAddress(format!(
+					"Destination {} is not a valid Onion address.",
+					dest
+				))
+			})?;
+			// Try sending to the destination via Tor.
 			let sender = match tor_sender {
 				None => {
-					if test_mode {
-						None
-					} else {
-						if let Some(tc) = tor_config {
-							match TorSlateSender::new(&tor_addr.to_http_str(), tc) {
-								Ok(s) => Some(s),
-								Err(e) => {
-									debug!("Send (TOR): Cannot create TOR Slate sender {:?}", e);
-									None
-								}
+					if let Some(tc) = tor_config {
+						match TorSlateSender::new(&tor_addr.to_http_str(), tc) {
+							Ok(s) => s,
+							Err(e) => {
+								debug!("Send (Tor): Cannot create TOR Slate sender {:?}", e);
+								return Err(e);
 							}
-						} else {
-							return Err(Error::TorConfig("Tor config is not set".to_string()));
 						}
-					}
-				}
-				Some(s) => {
-					if test_mode {
-						None
 					} else {
-						Some(s)
+						return Err(Error::TorConfig("Tor config is not set".to_string()));
 					}
 				}
+				Some(s) => s,
 			};
-			if let Some(s) = sender {
-				warn!("Attempting to send transaction via TOR");
-				match send_sync(s, "TOR") {
-					Ok(_) => return Ok(Some(ret_slate)),
-					Err(e) => {
-						debug!("Unable to send via TOR: {}", e);
-						warn!("Unable to send transaction via TOR");
-					}
-				}
+			warn!("Attempting to send transaction via Tor");
+			match send_sync(sender, "Tor") {
+				Ok(_) => Ok(ret_slate),
+				Err(e) => Err(e),
 			}
 		}
 		Err(e) => {
-			debug!("Send (TOR): Destination is not SlatepackAddress {:?}", e);
-			warn!("Destination is not a valid Slatepack address. Will output Slatepack.")
+			error!("Destination {} is not a valid Slatepack address.", dest);
+			Err(e)
 		}
 	}
-
-	Ok(None)
 }
 
 #[doc(hidden)]

config/src/types.rs

@@ -164,18 +164,18 @@ impl From<io::Error> for ConfigError {
 pub struct TorConfig {
 	/// whether to use integrated Tor library
 	pub use_integrated: Option<bool>,
-	/// whether to skip any attempts to send via TOR
+	/// whether to skip any attempts to send via Tor
 	pub skip_send_attempt: Option<bool>,
-	/// Whether to start tor listener on listener startup (default true)
+	/// Whether to start Tor listener on listener startup (default true)
 	pub use_tor_listener: bool,
 	/// Just the address of the socks proxy for now
 	pub socks_proxy_addr: String,
 	/// Send configuration directory
 	pub send_config_dir: String,
-	/// tor bridge config
+	/// Tor bridge config
 	#[serde(default)]
 	pub bridge: TorBridgeConfig,
-	/// tor proxy config
+	/// Tor proxy config
 	#[serde(default)]
 	pub proxy: TorProxyConfig,
 }
@@ -194,6 +194,16 @@ impl Default for TorConfig {
 	}
 }
 
+impl TorConfig {
+	/// Check if attempt to send over Tor must be skipped using provided possible argument at priority.
+	pub fn skip_send(&self, skip_arg: Option<bool>) -> bool {
+		if let Some(skip_tor) = skip_arg {
+			return skip_tor;
+		}
+		self.skip_send_attempt.unwrap_or(false)
+	}
+}
+
 /// Tor Bridge Config
 #[derive(Debug, Clone, Serialize, Deserialize, PartialEq)]
 pub struct TorBridgeConfig {

controller/src/command.rs

@@ -431,18 +431,37 @@ where
 			if let Some(b) = args.bridge.clone() {
 				c.bridge.bridge_line = Some(b);
 			}
-			if let Some(s) = args.skip_tor {
-				c.skip_send_attempt = Some(s);
-			}
 			Some(c)
 		}
 		None => None,
 	};
 
-	let res = try_slatepack_sync_workflow(&slate, &args.dest, tor_config, None, false, test_mode);
+	let output_sp = || -> Result<(), Error> {
+		Ok(output_slatepack(
+			owner_api,
+			keychain_mask,
+			&slate,
+			args.dest.as_str(),
+			args.outfile,
+			true,
+			false,
+			args.slatepack_qr,
+		)?)
+	};
+
+	let can_send = if let Some(tc) = tor_config.as_ref() {
+		tc.skip_send(args.skip_tor)
+	} else {
+		false
+	};
+	if test_mode || !can_send {
+		return output_sp();
+	}
+
+	let res = try_slatepack_sync_workflow(&slate, &args.dest, tor_config, None, false);
 
 	match res {
-		Ok(Some(s)) => {
+		Ok(s) => {
 			controller::owner_single_use(None, keychain_mask, Some(owner_api), |api, m| {
 				api.tx_lock_outputs(m, &s)?;
 				let ret_slate = api.finalize_tx(m, &s)?;
@@ -459,19 +478,10 @@ where
 				}
 			})?;
 		}
-		Ok(None) => {
-			output_slatepack(
-				owner_api,
-				keychain_mask,
-				&slate,
-				args.dest.as_str(),
-				args.outfile,
-				true,
-				false,
-				args.slatepack_qr,
-			)?;
+		Err(e) => {
+			error!("Error sending slate sync: {}", e);
+			output_sp()?;
 		}
-		Err(e) => return Err(e.into()),
 	}
 	Ok(())
 }
@@ -668,9 +678,6 @@ where
 			if let Some(b) = args.bridge {
 				c.bridge.bridge_line = Some(b);
 			}
-			if let Some(s) = args.skip_tor {
-				c.skip_send_attempt = Some(s);
-			}
 			Some(c)
 		}
 		None => None,
@@ -686,33 +693,45 @@ where
 		None => String::from(""),
 	};
 
-	let res = try_slatepack_sync_workflow(&slate, &dest, tor_config, None, true, test_mode);
+	let output_sp = || -> Result<(), Error> {
+		Ok(output_slatepack(
+			owner_api,
+			keychain_mask,
+			&slate,
+			&dest,
+			args.outfile,
+			false,
+			false,
+			args.slatepack_qr,
+		)?)
+	};
+
+	let can_send = if let Some(tc) = tor_config.as_ref() {
+		tc.skip_send(args.skip_tor)
+	} else {
+		false
+	};
+	if test_mode || !can_send {
+		return output_sp();
+	}
+
+	let res = try_slatepack_sync_workflow(&slate, &dest, tor_config, None, true);
 
 	match res {
-		Ok(Some(_)) => {
+		Ok(_) => {
 			println!();
 			println!(
 				"Transaction received and sent back to sender at {} for finalization.",
 				dest
 			);
 			println!();
-			Ok(())
 		}
-		Ok(None) => {
-			output_slatepack(
-				owner_api,
-				keychain_mask,
-				&slate,
-				&dest,
-				args.outfile,
-				false,
-				false,
-				args.slatepack_qr,
-			)?;
-			Ok(())
+		Err(e) => {
+			error!("Error sending slate sync: {}", e);
+			output_sp()?;
 		}
-		Err(e) => Err(e.into()),
 	}
+	Ok(())
 }
 
 pub fn unpack<L, C, K>(
@@ -1012,41 +1031,50 @@ where
 			if let Some(b) = args.bridge {
 				c.bridge.bridge_line = Some(b);
 			}
-			if let Some(skip_tor) = args.skip_tor {
-				c.skip_send_attempt = Some(skip_tor);
-			}
 			Some(c)
 		}
 		None => None,
 	};
 
-	let res = try_slatepack_sync_workflow(&slate, &dest, tor_config, None, true, test_mode);
+	let output_sp = || -> Result<(), Error> {
+		Ok(output_slatepack(
+			owner_api,
+			keychain_mask,
+			&slate,
+			&dest,
+			args.outfile,
+			true,
+			false,
+			args.slatepack_qr,
+		)?)
+	};
+
+	let can_send = if let Some(tc) = tor_config.as_ref() {
+		tc.skip_send(args.skip_tor)
+	} else {
+		false
+	};
+	if test_mode || !can_send {
+		return output_sp();
+	}
+
+	let res = try_slatepack_sync_workflow(&slate, &dest, tor_config, None, true);
 
 	match res {
-		Ok(Some(_)) => {
+		Ok(_) => {
 			println!();
 			println!(
 				"Transaction paid and sent back to initiator at {} for finalization.",
 				dest
 			);
 			println!();
-			Ok(())
 		}
-		Ok(None) => {
-			output_slatepack(
-				owner_api,
-				keychain_mask,
-				&slate,
-				&dest,
-				args.outfile,
-				true,
-				false,
-				args.slatepack_qr,
-			)?;
-			Ok(())
+		Err(e) => {
+			error!("Error sending slate sync: {}", e);
+			output_sp()?;
 		}
-		Err(e) => Err(e.into()),
 	}
+	Ok(())
 }
 
 /// Info command args

libwallet/src/api_impl/foreign.rs

@@ -14,11 +14,11 @@
 
 //! Generic implementation of owner API functions
 
-use grin_keychain::Identifier;
 use strum::IntoEnumIterator;
 
 use super::owner::tx_lock_outputs;
 use crate::api_impl::owner::{check_ttl, post_tx};
+use crate::api_impl::types::update_tx_slate_state;
 use crate::backend::WalletBackend;
 use crate::grin_core::core::FeeFields;
 use crate::grin_keychain::Keychain;
@@ -236,39 +236,3 @@ where
 	}
 	Ok(sl)
 }
-
-/// Update transaction slate state.
-fn update_tx_slate_state<C, K>(
-	wallet: &mut WalletBackend<C, K>,
-	keychain_mask: Option<&SecretKey>,
-	parent_key_id: &Identifier,
-	slate: &Slate,
-) -> Result<(), Error>
-where
-	C: NodeClient,
-	K: Keychain,
-{
-	let mut bad_records = 0;
-	let tx = wallet
-		.tx_log_iter()?
-		.filter(|tx| {
-			if tx.is_err() {
-				bad_records += 1;
-			}
-			tx.is_ok()
-		})
-		.map(|tx| tx.unwrap())
-		.find(|tx| tx.tx_slate_id == Some(slate.id));
-	if let Some(mut tx) = tx {
-		let mut batch = wallet.batch(keychain_mask)?;
-		tx.tx_slate_state = Some(slate.state.clone());
-		batch.save_tx_log_entry(tx.clone(), parent_key_id)?;
-		batch.commit()?;
-	} else {
-		return Err(Error::Backend(format!(
-			"Tx log entry with slate id {} not found, there are {} bad tx log records",
-			slate.id, bad_records
-		)));
-	}
-	Ok(())
-}

libwallet/src/api_impl/types.rs

@@ -21,10 +21,12 @@ use crate::grin_util::secp::pedersen;
 use crate::slate_versions::ser as dalek_ser;
 use crate::slate_versions::SlateVersion;
 use crate::types::OutputData;
-use crate::SlatepackAddress;
+use crate::{Error, NodeClient, Slate, SlatepackAddress, WalletBackend};
 
 use chrono::prelude::*;
 use ed25519_dalek::Signature as DalekSignature;
+use grin_keychain::Keychain;
+use grin_util::secp::SecretKey;
 
 pub use crate::mwixnet::{Hop, MixnetReqCreationParams, SwapReq};
 
@@ -344,3 +346,39 @@ pub struct BuiltOutput {
 	/// Output
 	pub output: Output,
 }
+
+/// Update transaction slate state.
+pub fn update_tx_slate_state<C, K>(
+	wallet: &mut WalletBackend<C, K>,
+	keychain_mask: Option<&SecretKey>,
+	parent_key_id: &Identifier,
+	slate: &Slate,
+) -> Result<(), Error>
+where
+	C: NodeClient,
+	K: Keychain,
+{
+	let mut bad_records = 0;
+	let tx = wallet
+		.tx_log_iter()?
+		.filter(|tx| {
+			if tx.is_err() {
+				bad_records += 1;
+			}
+			tx.is_ok()
+		})
+		.map(|tx| tx.unwrap())
+		.find(|tx| tx.tx_slate_id == Some(slate.id));
+	if let Some(mut tx) = tx {
+		let mut batch = wallet.batch(keychain_mask)?;
+		tx.tx_slate_state = Some(slate.state.clone());
+		batch.save_tx_log_entry(tx.clone(), parent_key_id)?;
+		batch.commit()?;
+	} else {
+		return Err(Error::Backend(format!(
+			"Tx log entry with slate id {} not found, there are {} bad tx log records",
+			slate.id, bad_records
+		)));
+	}
+	Ok(())
+}