clean up contracts

controller/src/display.rs

@@ -20,6 +20,7 @@ use crate::libwallet::{
 	AcctPathMapping, Error, OutputCommitMapping, OutputStatus, Slate, TxLogEntry, ViewWallet,
 	WalletInfo,
 };
+use crate::util::secp::pedersen::Commitment;
 use crate::util::ToHex;
 use grin_wallet_util::OnionV3Address;
 use prettytable;
@@ -673,12 +674,12 @@ fn contract_view_table(
 	table.add_row(row![bFC->"Transfer Amount", bGC->amount_to_hr_string(slate.amount, false)]);
 	let suggested = view
 		.suggested_net_change
-		.map(|change| format_net_change(Some(change)))
+		.map(format_net_change)
 		.unwrap_or_else(|| "Not applicable".to_string());
 	table.add_row(row![bFC->"Expected Amount Change (Before Fee)", bGC->suggested]);
 	let agreed = view
 		.agreed_net_change
-		.map(|change| format_net_change(Some(change)))
+		.map(format_net_change)
 		.unwrap_or_else(|| "Not agreed yet".to_string());
 	table.add_row(row![bFC->"Agreed Amount Change (Before Fee)", bGC->agreed]);
 	table.add_row(
@@ -689,10 +690,10 @@ fn contract_view_table(
 		.map(|fee| amount_to_hr_string(fee, false))
 		.unwrap_or_else(|| "Not known before signing".to_string());
 	table.add_row(row![bFC->"Your Fee", bGC->own_fee]);
-	let balance_change = match view.balance_change {
-		Some(change) => format_net_change(Some(change)),
-		None => "Not known before signing".to_string(),
-	};
+	let balance_change = view
+		.balance_change
+		.map(format_net_change)
+		.unwrap_or_else(|| "Not known before signing".to_string());
 	table.add_row(row![bFC->"Your Balance Change", bGC->balance_change]);
 	table.add_row(row![bFC->"Confirmed", bGC->yes_no(view.is_executed)]);
 	let (unexpected, warning) = match view.own_commitment_status {
@@ -720,22 +721,14 @@ fn contract_view_table(
 			}
 			Inputs::FeaturesAndCommit(inputs) => {
 				for (index, input) in inputs.iter().enumerate() {
-					let commitment = input.commitment().as_ref().to_hex();
-					let value = match input.features {
-						OutputFeatures::Plain => commitment,
-						OutputFeatures::Coinbase => format!("Coinbase {}", commitment),
-					};
+					let value = format_commitment(input.commitment(), input.features);
 					table.add_row(row![bFC->format!("Input {}", index + 1), bGC->value]);
 				}
 			}
 		}
 		table.add_row(row![bFC->"Outputs", bGC->tx.outputs().len()]);
 		for (index, output) in tx.outputs().iter().enumerate() {
-			let commitment = output.commitment().as_ref().to_hex();
-			let value = match output.features() {
-				OutputFeatures::Plain => commitment,
-				OutputFeatures::Coinbase => format!("Coinbase {}", commitment),
-			};
+			let value = format_commitment(output.commitment(), output.features());
 			table.add_row(row![bFC->format!("Output {}", index + 1), bGC->value]);
 		}
 	} else {
@@ -755,17 +748,22 @@ fn yes_no(value: bool) -> &'static str {
 	}
 }
 
-fn format_net_change(change: Option<i64>) -> String {
-	match change {
-		Some(value) => format!(
-			"{}{}",
-			if value < 0 { "-" } else { "+" },
-			amount_to_hr_string(value.unsigned_abs(), false)
-		),
-		None => String::from("None"),
+fn format_commitment(commitment: Commitment, features: OutputFeatures) -> String {
+	let commitment = commitment.as_ref().to_hex();
+	match features {
+		OutputFeatures::Plain => commitment,
+		OutputFeatures::Coinbase => format!("Coinbase {}", commitment),
 	}
 }
 
+fn format_net_change(change: i64) -> String {
+	format!(
+		"{}{}",
+		if change < 0 { "-" } else { "+" },
+		amount_to_hr_string(change.unsigned_abs(), false)
+	)
+}
+
 #[cfg(test)]
 mod tests {
 	use super::{contract_view_table, format_net_change};
@@ -791,9 +789,9 @@ mod tests {
 
 	#[test]
 	fn net_change_sign() {
-		assert_eq!(format_net_change(Some(1_000_000_000)), "+1.000000000");
-		assert_eq!(format_net_change(Some(-1_000_000_000)), "-1.000000000");
-		assert_eq!(format_net_change(None), "None");
+		assert_eq!(format_net_change(1_000_000_000), "+1.000000000");
+		assert_eq!(format_net_change(-1_000_000_000), "-1.000000000");
+		assert_eq!(format_net_change(0), "+0.000000000");
 	}
 
 	#[test]

libwallet/src/api_impl/foreign.rs

@@ -247,10 +247,8 @@ where
 }
 
 // The foreign API only handles the receiving side
-fn check_receiving_contract(net_change: Option<i64>, action: &str) -> Result<(), Error> {
-	let net_change = net_change.ok_or_else(|| {
-		Error::GenericError("Contract requires a net change (--send or --receive)".to_string())
-	})?;
+fn check_receiving_contract(args: &ContractSetupArgsAPI, action: &str) -> Result<(), Error> {
+	let net_change = args.required_net_change()?;
 	if net_change <= 0 {
 		return Err(Error::GenericError(format!(
 			"Can't {} a non-receiving contract from a foreign API.",
@@ -270,7 +268,7 @@ where
 	C: NodeClient,
 	K: Keychain,
 {
-	check_receiving_contract(args.setup_args.net_change, "create")?;
+	check_receiving_contract(&args.setup_args, "create")?;
 	owner_contract_new(w, keychain_mask, args)
 }
 
@@ -285,7 +283,7 @@ where
 	C: NodeClient,
 	K: Keychain,
 {
-	check_receiving_contract(args.net_change, "sign")?;
+	check_receiving_contract(args, "sign")?;
 	owner_contract_sign(w, keychain_mask, args, slate)
 }
 

libwallet/src/contract/actions/new.rs

@@ -62,9 +62,7 @@ where
 	C: NodeClient,
 	K: Keychain,
 {
-	let net_change = setup_args.net_change.ok_or_else(|| {
-		Error::GenericError("Contract requires a net change (--send or --receive)".to_string())
-	})?;
+	let net_change = setup_args.required_net_change()?;
 	debug!("contract::new => net_change passed: {}", net_change);
 
 	// Initialize a new contract (if net_change is positive, I'm the receiver meaning this is invoice flow)

libwallet/src/contract/actions/setup.rs

@@ -115,12 +115,12 @@ where
 	}
 	contract::proofs::commit_sender_nonce(&sl, &mut context, keychain.secp())?;
 	contract::slate::add_keys(&mut sl, &keychain, &mut context)?;
-	contract::slate::add_payment_proof(
+	contract::slate::maybe_add_payment_proof(
 		w,
 		&mut sl,
 		keychain_mask,
-		&mut context,
-		&setup_args.net_change,
+		&context,
+		setup_args.net_change,
 		&setup_args.proof_args,
 	)?; // noop for the sender
 

libwallet/src/contract/context.rs

@@ -85,9 +85,7 @@ where
 	updater::refresh_outputs(w, keychain_mask, parent_key_id, false)?;
 
 	// Fee contribution estimation
-	let net_change = setup_args.net_change.ok_or_else(|| {
-		Error::GenericError("Contract requires a net change (--send or --receive)".to_string())
-	})?;
+	let net_change = setup_args.required_net_change()?;
 	// select inputs to estimate fee cost
 	let (inputs, _, my_fee) =
 		prepare_outputs(w, &parent_key_id, current_height, &setup_args, None)?;
@@ -98,7 +96,7 @@ where
 	);
 	// Make sure `my_fee < net_change` holds for the receiver. This can't be true for a self-spend, because nobody
 	// has a net_change > 0 which makes a self-spend ok to be a net negative when fees are included.
-	if net_change > 0 && my_fee.fee() > net_change.abs() as u64 {
+	if net_change > 0 && my_fee.fee() > net_change as u64 {
 		return Err(Error::GenericError(format!(
 			"My contribution as a receiver would be net negative. my_fee: {}, net_change: {}",
 			my_fee.fee(),

libwallet/src/contract/selection.rs

@@ -95,9 +95,7 @@ fn select_inputs(
 	// For either party, the following MUST hold for the inputs the function returns:
 	//  Σmy_inputs >= Σmy_outputs + my_fee_cost
 	// Each party later balances the equation by adding an additional output (change output or receiver output)
-	let net_change = setup_args.net_change.ok_or_else(|| {
-		Error::GenericError("Contract requires a net change (--send or --receive)".to_string())
-	})?;
+	let net_change = setup_args.required_net_change()?;
 	let custom_outputs_amount_sum = setup_args.selection_args.sum_output_amounts()?;
 	// unsigned_abs, as abs() panics on i64::MIN
 	let pay_amount = if net_change < 0 {
@@ -304,9 +302,7 @@ fn build_output_amount_list(
 	my_fee_cost: u64,
 	setup_args: &ContractSetupArgsAPI,
 ) -> Result<Vec<u64>, Error> {
-	let expected_net_change = setup_args.net_change.ok_or_else(|| {
-		Error::GenericError("Contract requires a net change (--send or --receive)".to_string())
-	})?;
+	let expected_net_change = setup_args.required_net_change()?;
 	let mut my_output_amounts = setup_args.selection_args.output_amounts();
 	let custom_outputs_sum = my_output_amounts
 		.iter()

libwallet/src/contract/slate.rs

@@ -32,26 +32,26 @@ use std::collections::BTreeSet;
 use super::types::{OwnCommitmentStatus, ProofArgs};
 
 /// Add payment proof data to slate, noop for sender
-pub fn add_payment_proof<C, K>(
+pub fn maybe_add_payment_proof<C, K>(
 	w: &mut WalletBackend<C, K>,
 	slate: &mut Slate,
 	keychain_mask: Option<&SecretKey>,
 	context: &Context,
-	net_change: &Option<i64>,
+	net_change: Option<i64>,
 	proof_args: &ProofArgs,
 ) -> Result<(), Error>
 where
 	C: NodeClient,
 	K: Keychain,
 {
-	debug!("contract::slate::add_payment_proof => called");
+	debug!("contract::slate::maybe_add_payment_proof => called");
 	if !proof_args.suppress_proof {
 		crate::payment_proof::check_proof_type(&proof_args.proof_type)?;
 	}
-	// If we're a recipient, generate proof unless explicity told not to
-	if let Some(ref c) = net_change {
-		if *c > 0 && !proof_args.suppress_proof && slate.payment_proof.is_none() {
-			super::proofs::add_payment_proof(w, keychain_mask, slate, &context, proof_args)?;
+	// If we're a recipient, generate proof unless explicitly told not to
+	if let Some(change) = net_change {
+		if change > 0 && !proof_args.suppress_proof && slate.payment_proof.is_none() {
+			super::proofs::add_payment_proof(w, keychain_mask, slate, context, proof_args)?;
 		}
 	}
 

libwallet/src/contract/types.rs

@@ -136,6 +136,15 @@ pub struct ContractSetupArgsAPI {
 	pub proof_args: ProofArgs,
 }
 
+impl ContractSetupArgsAPI {
+	/// Return the net change or fail if it is missing
+	pub fn required_net_change(&self) -> Result<i64, Error> {
+		self.net_change.ok_or_else(|| {
+			Error::GenericError("Contract requires a net change (--send or --receive)".to_string())
+		})
+	}
+}
+
 impl Default for ContractSetupArgsAPI {
 	fn default() -> ContractSetupArgsAPI {
 		ContractSetupArgsAPI {
@@ -144,16 +153,14 @@ impl Default for ContractSetupArgsAPI {
 			num_participants: 2,
 			fee_rate: None,
 			add_outputs: false,
-			selection_args: OutputSelectionArgs {
-				..Default::default()
-			},
+			selection_args: OutputSelectionArgs::default(),
 			proof_args: ProofArgs::default(),
 		}
 	}
 }
 
 /// Contract New
-#[derive(Clone, Serialize, Deserialize)]
+#[derive(Clone, Default, Serialize, Deserialize)]
 pub struct ContractNewArgsAPI {
 	/// Setup args - contract new also initiates the setup by default
 	pub setup_args: ContractSetupArgsAPI,
@@ -163,15 +170,6 @@ pub struct ContractNewArgsAPI {
 	pub ttl_blocks: Option<u64>,
 }
 
-impl Default for ContractNewArgsAPI {
-	fn default() -> ContractNewArgsAPI {
-		ContractNewArgsAPI {
-			ttl_blocks: None,
-			setup_args: ContractSetupArgsAPI::default(),
-		}
-	}
-}
-
 /// ContractView
 #[derive(Clone, Serialize, Deserialize)]
 pub struct ContractView {
@@ -185,7 +183,7 @@ pub struct ContractView {
 	pub own_fee: Option<u64>,
 	/// Agreed balance change after this wallet's fee, or None while the fee is unknown
 	pub balance_change: Option<i64>,
-	/// Number of singatures on the contract
+	/// Number of signatures on the contract
 	pub num_sigs: u8,
 	/// Has the contract been executed on chain
 	pub is_executed: bool,
@@ -242,6 +240,19 @@ pub struct ContractRevokeArgsAPI {
 mod tests {
 	use super::*;
 
+	#[test]
+	fn required_net_change() {
+		let mut args = ContractSetupArgsAPI::default();
+		assert_eq!(
+			args.required_net_change().unwrap_err(),
+			Error::GenericError("Contract requires a net change (--send or --receive)".into())
+		);
+		for change in [i64::MIN, -1, 0, 1, i64::MAX] {
+			args.net_change = Some(change);
+			assert_eq!(args.required_net_change().unwrap(), change);
+		}
+	}
+
 	#[test]
 	fn proof_args_json() {
 		for (proof_type, value) in [

libwallet/src/contract/utils.rs

@@ -154,7 +154,7 @@ pub fn get_net_change(
 ) -> Result<i64, Error> {
 	let mut expected_net_change: Option<i64> = setup_args_net_change;
 	if let Some(context) = context {
-		debug!("contract::sign => context found");
+		debug!("contract::utils::get_net_change => context found");
 		// We have a context so we must have agreed on a certain net_change value in Context.net_change.
 		// If we have both Context.net_change and setup_args.net_change, then they must be equal.
 		let ctx_net_change = context.get_net_change()?;
@@ -296,11 +296,10 @@ where
 
 	// Update context
 	if is_signed && !is_step2 {
-		// NOTE: We MUST forget the context when we sign. Ideally, these two would be atomic or perhaps
-		// when we call slate::sigadd_partial_signaturen we could swap the secret key with a temporary one just to be safe.
-		// Keep the step2 context for contract view
+		// Forget the signing context in the same batch as the transaction log update
 		batch.delete_private_context(slate.id.as_bytes())?;
 	} else {
+		// Keep unsigned contexts for retries and the step2 context for contract view
 		batch.save_private_context(slate.id.as_bytes(), &context)?;
 	}
 

libwallet/src/payment_proof.rs

@@ -25,7 +25,7 @@ use crate::grin_util::secp::Secp256k1;
 use crate::grin_util::secp::Signature;
 use crate::grin_util::static_secp_instance;
 use crate::slate::{PaymentMemo, PaymentProofType, Slate};
-use crate::slate_versions::ser as dalek_ser;
+use crate::slate_versions::ser::{dalek_pubkey_serde, option_dalek_sig_serde, option_pubkey_serde};
 use crate::Error;
 use ed25519_dalek::Signature as DalekSignature;
 use ed25519_dalek::SigningKey as DalekSecretKey;
@@ -117,7 +117,7 @@ pub struct ProofWitness {
 	/// Untweaked sender nonce used by sender-nonce proofs
 	#[serde(
 		default,
-		with = "dalek_ser::option_pubkey_serde",
+		with = "option_pubkey_serde",
 		skip_serializing_if = "Option::is_none"
 	)]
 	pub sender_public_nonce: Option<PublicKey>,
@@ -138,7 +138,7 @@ pub struct EarlyPaymentProof {
 	#[serde(with = "secp_ser::pubkey_serde")]
 	pub receiver_public_excess: PublicKey,
 	/// Sender's address
-	#[serde(with = "dalek_ser::dalek_pubkey_serde")]
+	#[serde(with = "dalek_pubkey_serde")]
 	pub sender_address: DalekPublicKey,
 	/// Timestamp provided by recipient when signing
 	pub timestamp: i64,
@@ -146,7 +146,7 @@ pub struct EarlyPaymentProof {
 	#[serde(skip_serializing_if = "Option::is_none")]
 	pub memo: Option<PaymentMemo>,
 	/// Not serialized in binary format
-	#[serde(with = "dalek_ser::option_dalek_sig_serde")]
+	#[serde(with = "option_dalek_sig_serde")]
 	pub promise_signature: Option<DalekSignature>,
 	/// Not serialized in binary format, just a convenient place to insert
 	/// the witness kernel commitment index

libwallet/src/slate_versions/mod.rs

@@ -74,7 +74,7 @@ impl TryFrom<u16> for SlateVersion {
 impl SlateVersion {
 	/// Use V5 only when the slate has fields that V4 cannot store
 	/// V4 does not support the proof type, timestamp or memo
-	pub fn lowest_for(slate: &Slate) -> SlateVersion {
+	fn lowest_for(slate: &Slate) -> SlateVersion {
 		match &slate.payment_proof {
 			Some(p) if p.requires_v5() => SlateVersion::V5,
 			_ => SlateVersion::V4,

libwallet/src/types.rs

@@ -29,7 +29,7 @@ use crate::grin_util::secp::key::{PublicKey, SecretKey};
 use crate::grin_util::secp::{pedersen, Secp256k1, Signature};
 use crate::grin_util::{ToHex, ZeroingString};
 use crate::slate::PaymentMemo;
-use crate::slate_versions::ser as dalek_ser;
+use crate::slate_versions::ser::{dalek_pubkey_serde, option_dalek_sig_serde, option_pubkey_serde};
 use crate::{InitTxArgs, SlateState, WalletBackend};
 use chrono::prelude::*;
 use ed25519_dalek::Signature as DalekSignature;
@@ -776,18 +776,18 @@ impl TxLogEntry {
 #[derive(Serialize, Deserialize, Debug, Clone)]
 pub struct StoredProofInfo {
 	/// receiver address
-	#[serde(with = "dalek_ser::dalek_pubkey_serde")]
+	#[serde(with = "dalek_pubkey_serde")]
 	pub receiver_address: DalekPublicKey,
-	#[serde(with = "dalek_ser::option_dalek_sig_serde")]
+	#[serde(with = "option_dalek_sig_serde")]
 	/// Receiver signature for legacy proofs, or promise signature for early proofs
 	pub receiver_signature: Option<DalekSignature>,
 	/// sender address derivation path index
 	pub sender_address_path: u32,
 	/// sender address
-	#[serde(with = "dalek_ser::dalek_pubkey_serde")]
+	#[serde(with = "dalek_pubkey_serde")]
 	pub sender_address: DalekPublicKey,
 	/// Legacy sender signature
-	#[serde(with = "dalek_ser::option_dalek_sig_serde")]
+	#[serde(with = "option_dalek_sig_serde")]
 	pub sender_signature: Option<DalekSignature>,
 	// Fields beyond here are specific to early payment proofs
 	/// Assumed to be 0x00 (Legacy) if missing
@@ -796,14 +796,14 @@ pub struct StoredProofInfo {
 	/// receiver's public nonce from signing
 	#[serde(
 		default,
-		with = "dalek_ser::option_pubkey_serde",
+		with = "option_pubkey_serde",
 		skip_serializing_if = "Option::is_none"
 	)]
 	pub receiver_public_nonce: Option<PublicKey>,
 	/// receiver's public excess from signing
 	#[serde(
 		default,
-		with = "dalek_ser::option_pubkey_serde",
+		with = "option_pubkey_serde",
 		skip_serializing_if = "Option::is_none"
 	)]
 	pub receiver_public_excess: Option<PublicKey>,
@@ -823,7 +823,7 @@ pub struct StoredProofInfo {
 	/// Untweaked sender public nonce for a sender-nonce proof
 	#[serde(
 		default,
-		with = "dalek_ser::option_pubkey_serde",
+		with = "option_pubkey_serde",
 		skip_serializing_if = "Option::is_none"
 	)]
 	pub sender_public_nonce: Option<PublicKey>,