fix contract slatepack cli output

config/src/config.rs

@@ -336,7 +336,7 @@ pub fn initial_setup_wallet(
 		Some(p) => p,
 		None => get_wallet_path(chain_type, create_path)?,
 	};
-	println!("Wallet path: {}", wallet_path.display());
+	eprintln!("Wallet path: {}", wallet_path.display());
 	// Get path to the node directory,
 	let node_path = get_node_path(Some(wallet_path.clone()), chain_type)?;
 

controller/src/command.rs

@@ -649,24 +649,38 @@ impl SlatepackOut {
 		serde_json::to_string_pretty(&self).unwrap()
 	}
 
-	pub fn print(&self, as_json: bool) -> () {
+	fn render(&self, as_json: bool) -> String {
-		if !self.is_finalized {
+		if as_json {
-			if as_json {
+			self.as_json()
-				println!("{}", self.as_json());
-			} else {
-				println!("{}", self);
-			}
 		} else {
-			println!("Transaction was broadcasted."); // TODO: as_json makes no sense here, fix later.
+			self.to_string()
 		}
 	}
+
+	pub fn print(&self, as_json: bool) {
+		println!("{}", self.render(as_json));
+	}
+}
+
+fn slatepack_recipient(dest: Option<&str>) -> Result<Option<SlatepackAddress>, Error> {
+	dest.map(SlatepackAddress::try_from)
+		.transpose()
+		.map_err(Error::from)
+}
+
+fn print_contract_status(message: &str, as_json: bool) {
+	if as_json {
+		eprintln!("{}", message);
+	} else {
+		println!("{}", message);
+	}
 }
 
 pub fn print_slatepack<L, C, K>(
 	api: &mut Owner<L, C, K>,
 	keychain_mask: Option<&SecretKey>,
 	slate: &Slate,
-	counterparty_addr: &str,
+	counterparty_addr: Option<SlatepackAddress>,
 	out_file: Option<String>,
 	as_json: bool,
 ) -> Result<(), libwallet::Error>
@@ -678,7 +692,7 @@ where
 	// For now, we don't compact slates with sl.compact(). We first make them work without compaction.
 	// Writing the file, serializing and encrypting can all fail for ordinary reasons, so
 	// report them through the normal CLI error path rather than unwrapping.
-	let slate_out = prepare_slatepack(api, keychain_mask, &slate, &counterparty_addr, out_file)?;
+	let slate_out = prepare_slatepack(api, keychain_mask, &slate, counterparty_addr, out_file)?;
 	slate_out.print(as_json);
 	Ok(())
 }
@@ -687,7 +701,7 @@ pub fn prepare_slatepack<L, C, K>(
 	owner_api: &mut Owner<L, C, K>,
 	keychain_mask: Option<&SecretKey>,
 	slate: &Slate,
-	dest: &str,
+	dest: Option<SlatepackAddress>,
 	out_file_override: Option<String>,
 ) -> Result<SlatepackOut, libwallet::Error>
 where
@@ -699,17 +713,12 @@ where
 
 	// Output the slatepack file to stdout and to a file
 	let mut message = String::from("");
-	let mut address = None;
 	let mut tld = String::from("");
 	let wallet_inst = owner_api.wallet_inst.clone();
 	let config_path = owner_api.config_path();
 	controller::owner_single_use(wallet_inst, keychain_mask, config_path, |api, m| {
-		address = match SlatepackAddress::try_from(dest) {
-			Ok(a) => Some(a),
-			Err(_) => None,
-		};
 		// encrypt for recipient by default
-		let recipients = match address.clone() {
+		let recipients = match dest.clone() {
 			Some(a) => vec![a],
 			None => vec![],
 		};
@@ -739,7 +748,7 @@ where
 	let is_finalized = can_finalize(slate);
 
 	let slate_out = SlatepackOut {
-		is_encrypted: address.is_some(),
+		is_encrypted: dest.is_some(),
 		is_finalized: is_finalized,
 		out_file: out_file_name,
 		message: message,
@@ -1749,6 +1758,7 @@ where
 	K: keychain::Keychain + 'static,
 {
 	let contract_new_args = args.to_api_args()?;
+	let recipient = slatepack_recipient(args.counterparty_addr.as_deref())?;
 	let wallet_inst = owner_api.wallet_inst.clone();
 	let config_path = owner_api.config_path();
 	controller::owner_single_use(wallet_inst, keychain_mask, config_path, |api, m| {
@@ -1758,7 +1768,7 @@ where
 			api,
 			keychain_mask,
 			&slate,
-			args.counterparty_addr.as_deref().unwrap_or(""),
+			recipient.clone(),
 			args.outfile,
 			args.as_json,
 		)?;
@@ -1846,11 +1856,12 @@ where
 {
 	// Args for signing are just setup args
 	let contract_sign_args = args.to_api_args()?;
+	let recipient = slatepack_recipient(args.counterparty_addr.as_deref())?;
 	let wallet_inst = owner_api.wallet_inst.clone();
 	let config_path = owner_api.config_path();
 	controller::owner_single_use(wallet_inst, keychain_mask, config_path, |api, m| {
 		// Read the slatepack from stdin
-		println!("Paste slatepack:");
+		print_contract_status("Paste slatepack:", args.as_json);
 		let mut slatepack_msg = String::new();
 		io::stdin().read_line(&mut slatepack_msg).map_err(|e| {
 			libwallet::Error::GenericError(format!("Failed to read from stdin: {}", e))
@@ -1866,15 +1877,8 @@ where
 		)?;
 
 		// Encrypt the reply for --encrypt-for if given, else for the incoming slatepack's
-		// sender. If neither is known the incoming slate was plaintext, so reply plaintext
+		// sender. If neither is known the incoming slate was plaintext, so reply plaintext.
-		// (empty dest -> unencrypted slatepack).
+		let recipient = recipient.clone().or(slatepack.sender);
-		let counterparty_addr = if let Some(addr) = args.counterparty_addr {
-			addr
-		} else if let Some(sender) = slatepack.sender {
-			String::try_from(&sender)?
-		} else {
-			String::new()
-		};
 		let mut slate = owner::slate_from_slatepack_message(
 			api.wallet_inst.clone(),
 			keychain_mask,
@@ -1884,20 +1888,19 @@ where
 
 		slate = api.contract_sign(m, &slate, &contract_sign_args)?;
 
-		print_slatepack(
+		let slate_out = prepare_slatepack(api, keychain_mask, &slate, recipient, args.outfile)?;
-			api,
-			keychain_mask,
-			&slate,
-			&counterparty_addr,
-			args.outfile,
-			args.as_json,
-		)?;
 
-		if broadcast_tx {
+		if broadcast_tx && slate_out.is_finalized {
-			let is_finalized = can_finalize(&slate);
+			if let Err(e) = api.post_tx(keychain_mask, &slate, true) {
-			if is_finalized {
+				slate_out.print(args.as_json);
-				api.post_tx(keychain_mask, &slate, true)?;
+				return Err(e);
 			}
+			if args.as_json {
+				slate_out.print(true);
+			}
+			print_contract_status("Transaction was broadcasted.", args.as_json);
+		} else {
+			slate_out.print(args.as_json);
 		}
 
 		Ok(())
@@ -1970,7 +1973,7 @@ where
 		)?;
 		if let Some(slate) = slate_opt {
 			// A revoke has no counterparty, so write the replacement as plaintext.
-			let slate_out = prepare_slatepack(api, keychain_mask, &slate, "", None)?;
+			let slate_out = prepare_slatepack(api, keychain_mask, &slate, None, None)?;
 			println!("{}", slate_out);
 		} else {
 			println!("Contract revoked. No replacement transaction was created.");
@@ -2007,3 +2010,28 @@ mod send_tests {
 		assert_eq!(estimate_strategies(false), &["smallest", "all"]);
 	}
 }
+
+#[cfg(test)]
+mod contract_tests {
+	use super::*;
+
+	#[test]
+	fn invalid_recipient() {
+		assert!(slatepack_recipient(Some("invalid")).is_err());
+		assert!(slatepack_recipient(None).unwrap().is_none());
+	}
+
+	#[test]
+	fn finalized_json() {
+		let output = SlatepackOut {
+			is_encrypted: false,
+			is_finalized: true,
+			out_file: "slatepack/test.S3.slatepack".to_string(),
+			message: "BEGINSLATEPACK. test. ENDSLATEPACK.".to_string(),
+		};
+
+		let value: serde_json::Value = serde_json::from_str(&output.render(true)).unwrap();
+		assert_eq!(value["is_finalized"], true);
+		assert_eq!(value["message"], output.message);
+	}
+}

libwallet/src/contract/actions/README.md

@@ -65,7 +65,6 @@ Separating side effects until the 'save_step' part would make these functions mu
  - Preserve kernel features and their arguments across V4 and V5; see #793.
  - Preserve missing proof sender addresses and timestamps instead of replacing them with
    zero values.
- - Fix Slatepack output for invalid recipients, failed broadcasts and JSON output.
  - Choose the sender derivation path explicitly and keep encrypted outgoing files
    identifiable locally.
  - Decode the sender and slate from an incoming Slatepack in one pass.

src/bin/grin-wallet.rs

@@ -135,11 +135,11 @@ fn real_main() -> i32 {
 
 	// Load logging config
 	let mut l = config.members.logging.clone().unwrap();
-	// no logging to stdout if we're running cli
+	if cmd::wallet_args::contract_json_output(&args) {
-	match args.subcommand() {
+		l.log_to_stdout = false;
-		("cli", _) => l.log_to_stdout = true,
+	} else if let ("cli", _) = args.subcommand() {
-		_ => {}
+		l.log_to_stdout = true;
-	};
+	}
 	init_logger(Some(l), None);
 	info!(
 		"Using wallet configuration file at {}",

src/cmd/wallet.rs

@@ -22,6 +22,14 @@ use std::time::Duration;
 
 const MIN_COMPAT_NODE_VERSION: &str = "4.0.0-alpha.1";
 
+fn print_status(message: &str, stderr: bool) {
+	if stderr {
+		eprintln!("{}", message);
+	} else {
+		println!("{}", message);
+	}
+}
+
 pub fn wallet_command<C>(
 	wallet_args: &ArgMatches<'_>,
 	config: GlobalWalletConfig,
@@ -30,6 +38,7 @@ pub fn wallet_command<C>(
 where
 	C: NodeClient + 'static,
 {
+	let json_output = wallet_args::contract_json_output(wallet_args);
 	// Check the node version info, and exit with report if we're not compatible
 	let global_wallet_args = wallet_args::parse_global_args(&config.members.wallet, &wallet_args)
 		.expect("Can't read configuration file");
@@ -41,15 +50,15 @@ where
 		let min_version = Version::parse(MIN_COMPAT_NODE_VERSION).ok();
 		if let Ok(v) = node_version {
 			if Some(v.clone()) < min_version {
-				println!("The Grin Node in use (version {}) is outdated and incompatible with this wallet version.", v);
+				eprintln!("The Grin Node in use (version {}) is outdated and incompatible with this wallet version.", v);
-				println!(
+				eprintln!(
 					"Please update the node to version {} or later and try again.",
 					MIN_COMPAT_NODE_VERSION
 				);
 				return 1;
 			}
 		} else {
-			println!(
+			eprintln!(
 				"Can not parse node version {}. Minimal compatible node version: {}",
 				v.node_version, MIN_COMPAT_NODE_VERSION
 			);
@@ -64,12 +73,15 @@ where
 	thread::sleep(Duration::from_millis(100));
 
 	if let Err(e) = res {
-		println!("Wallet command failed: {}", e);
+		print_status(&format!("Wallet command failed: {}", e), json_output);
 		1
 	} else {
-		println!(
+		print_status(
-			"Command '{}' completed successfully",
+			&format!(
-			wallet_args.subcommand().0
+				"Command '{}' completed successfully",
+				wallet_args.subcommand().0
+			),
+			json_output,
 		);
 		0
 	}

src/cmd/wallet_args.rs

@@ -1194,6 +1194,18 @@ pub fn parse_contract_revoke_args(
 	Ok(command::ContractRevokeArgs { tx_id: tx_id })
 }
 
+pub fn contract_json_output(args: &ArgMatches<'_>) -> bool {
+	match args.subcommand() {
+		("contract", Some(contract_args)) => match contract_args.subcommand() {
+			("new", Some(command_args)) | ("sign", Some(command_args)) => {
+				command_args.is_present("as-json")
+			}
+			_ => false,
+		},
+		_ => false,
+	}
+}
+
 pub fn wallet_command<C, F>(
 	wallet_args: &ArgMatches,
 	config: GlobalWalletConfig,