validate contract net change

controller/tests/contract_rsr.rs

@@ -69,6 +69,18 @@ fn contract_rsr_tx_impl(test_dir: &'static str) -> Result<(), libwallet::Error>
 		send_mask,
 		PathBuf::from(test_dir),
 		|api, m| {
+			let wrong_args = ContractSetupArgsAPI {
+				selection_args: common::contract_selection_args(),
+				net_change: Some(5_000_000_000),
+				..Default::default()
+			};
+			let err = api.contract_sign(m, &slate, &wrong_args).unwrap_err();
+			assert!(matches!(
+				err,
+				libwallet::Error::GenericError(ref msg)
+					if msg == "Expected net change -5000000000, got 5000000000 (did you mean --send instead of --receive?)"
+			));
+
 			// Send Wallet calls --send=5
 			let args = &ContractSetupArgsAPI {
 				selection_args: common::contract_selection_args(),

controller/tests/contract_srs.rs

@@ -70,6 +70,18 @@ fn contract_srs_tx_impl(test_dir: &'static str) -> Result<(), libwallet::Error>
 		recv_mask,
 		PathBuf::from(test_dir),
 		|api, m| {
+			let wrong_args = ContractSetupArgsAPI {
+				selection_args: common::contract_selection_args(),
+				net_change: Some(-5_000_000_000),
+				..Default::default()
+			};
+			let err = api.contract_sign(m, &slate, &wrong_args).unwrap_err();
+			assert!(matches!(
+				err,
+				libwallet::Error::GenericError(ref msg)
+					if msg == "Expected net change 5000000000, got -5000000000 (did you mean --receive instead of --send?)"
+			));
+
 			// Receive wallet calls --receive=5
 			let args = &mut ContractSetupArgsAPI {
 				selection_args: common::contract_selection_args(),

libwallet/src/contract/actions/README.md

@@ -48,7 +48,6 @@ Separating side effects until the 'save_step' part would make these functions mu
    clearer, for the API as much as the CLI.
  - `new` has no `--no-setup`.
  - Add custom fee rates and contract TTLs, or document them as unsupported.
- - Detect a reversed `--send` or `--receive` when signing a slate for the first time.
  - Let `view` report whether a slatepack was encrypted for this wallet.
  - Show all inputs, outputs, fees and resulting balance changes before the CLI signs.
  - Distinguish the transfer amount from the fee-adjusted balance change in `view`.

libwallet/src/contract/actions/mod.rs

@@ -20,8 +20,23 @@ mod setup;
 mod sign;
 mod view;
 
+use crate::error::Error;
+use crate::slate::SlateState;
+
 pub use self::new::new;
 pub use self::revoke::revoke;
 pub use self::setup::setup;
 pub use self::sign::sign;
 pub use self::view::view;
+
+// Net change expected from the other party at the first step
+fn initial_net_change(state: &SlateState, amount: u64) -> Result<Option<i64>, Error> {
+	let sign = match state {
+		SlateState::Standard1 => 1,
+		SlateState::Invoice1 => -1,
+		_ => return Ok(None),
+	};
+	let amount = i64::try_from(amount)
+		.map_err(|_| Error::GenericError(format!("Slate amount {} exceeds i64", amount)))?;
+	Ok(Some(sign * amount))
+}

libwallet/src/contract/actions/sign.rs

@@ -14,6 +14,7 @@
 
 //! Implementation of contract sign
 
+use super::initial_net_change;
 use crate::backend::WalletBackend;
 use crate::contract;
 use crate::contract::actions::setup;
@@ -21,9 +22,31 @@ use crate::contract::types::ContractSetupArgsAPI;
 use crate::error::Error;
 use crate::grin_keychain::Keychain;
 use crate::grin_util::secp::key::SecretKey;
-use crate::slate::Slate;
+use crate::slate::{Slate, SlateState};
 use crate::types::{Context, NodeClient};
 
+fn verify_first_sign_net_change(
+	state: &SlateState,
+	amount: u64,
+	net_change: i64,
+) -> Result<(), Error> {
+	if let Some(expected) = initial_net_change(state, amount)? {
+		if net_change != expected {
+			let reversed = expected.checked_neg() == Some(net_change);
+			let hint = match state {
+				SlateState::Standard1 if reversed => " (did you mean --receive instead of --send?)",
+				SlateState::Invoice1 if reversed => " (did you mean --send instead of --receive?)",
+				_ => "",
+			};
+			return Err(Error::GenericError(format!(
+				"Expected net change {}, got {}{}",
+				expected, net_change, hint
+			)));
+		}
+	}
+	Ok(())
+}
+
 /// Sign a contract
 pub fn sign<C, K>(
 	w: &mut WalletBackend<C, K>,
@@ -79,6 +102,9 @@ where
 	// Ensure net_change has been provided
 	let expected_net_change =
 		contract::utils::get_net_change(existing_context, setup_args.net_change)?;
+	if existing_context.is_none() {
+		verify_first_sign_net_change(&sl.state, sl.amount, expected_net_change)?;
+	}
 
 	// Define the values that must be provided in the setup phase at the sign step
 	let mut setup_args = setup_args.clone();
@@ -104,3 +130,37 @@ where
 
 	Ok((sl, context))
 }
+
+#[cfg(test)]
+mod tests {
+	use super::*;
+
+	#[test]
+	fn first_sign_net_change() {
+		for (state, amount, net_change) in [
+			(SlateState::Standard1, 10, 10),
+			(SlateState::Invoice1, 10, -10),
+			(SlateState::Standard1, 0, 0),
+			(SlateState::Invoice1, 0, 0),
+			(SlateState::Standard2, 10, -10),
+			(SlateState::Invoice2, 10, 10),
+			(SlateState::Invoice3, 10, 10),
+		] {
+			assert!(verify_first_sign_net_change(&state, amount, net_change).is_ok());
+		}
+
+		for (state, amount, net_change) in [
+			(SlateState::Standard1, 10, -10),
+			(SlateState::Invoice1, 10, 10),
+			(SlateState::Invoice1, 10, -9),
+		] {
+			assert!(verify_first_sign_net_change(&state, amount, net_change).is_err());
+		}
+
+		let err = verify_first_sign_net_change(&SlateState::Standard1, 10, 9).unwrap_err();
+		assert!(matches!(
+			err,
+			Error::GenericError(ref msg) if msg == "Expected net change 10, got 9"
+		));
+	}
+}

libwallet/src/contract/actions/view.rs

@@ -14,6 +14,7 @@
 
 //! Implementation of contract view
 
+use super::initial_net_change;
 use crate::backend::WalletBackend;
 use crate::contract;
 use crate::contract::types::{ContractView, OwnCommitmentStatus};
@@ -24,17 +25,6 @@ use crate::internal::updater;
 use crate::slate::{Slate, SlateState};
 use crate::types::NodeClient;
 
-fn suggested_net_change(state: &SlateState, amount: u64) -> Result<Option<i64>, Error> {
-	let sign = match state {
-		SlateState::Standard1 => 1,
-		SlateState::Invoice1 => -1,
-		_ => return Ok(None),
-	};
-	let amount = i64::try_from(amount)
-		.map_err(|_| Error::GenericError(format!("Slate amount {} exceeds i64", amount)))?;
-	Ok(Some(sign * amount))
-}
-
 /// View contract
 pub fn view<C, K>(
 	w: &mut WalletBackend<C, K>,
@@ -69,7 +59,7 @@ where
 		Err(Error::NotFoundErr(_)) => None,
 		Err(e) => return Err(e),
 	};
-	let suggested_net_change = suggested_net_change(&slate.state, slate.amount)?;
+	let suggested_net_change = initial_net_change(&slate.state, slate.amount)?;
 	// A contract is executed once the transaction it produced has confirmed. That is
 	// recorded on our own tx log entry for this slate, so no chain lookup is needed; a
 	// slate we have never signed simply has no entry and is not executed.
@@ -125,15 +115,15 @@ mod tests {
 	#[test]
 	fn suggested_change_direction() {
 		assert_eq!(
-			suggested_net_change(&SlateState::Standard1, 10).unwrap(),
+			initial_net_change(&SlateState::Standard1, 10).unwrap(),
 			Some(10)
 		);
 		assert_eq!(
-			suggested_net_change(&SlateState::Invoice1, 10).unwrap(),
+			initial_net_change(&SlateState::Invoice1, 10).unwrap(),
 			Some(-10)
 		);
 		assert_eq!(
-			suggested_net_change(&SlateState::Standard2, 10).unwrap(),
+			initial_net_change(&SlateState::Standard2, 10).unwrap(),
 			None
 		);
 	}