diff --git a/controller/tests/contract_rsr.rs b/controller/tests/contract_rsr.rs index fd252a5..9243643 100644 --- a/controller/tests/contract_rsr.rs +++ b/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(), diff --git a/controller/tests/contract_srs.rs b/controller/tests/contract_srs.rs index e50b3d5..d811437 100644 --- a/controller/tests/contract_srs.rs +++ b/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(), diff --git a/libwallet/src/contract/actions/README.md b/libwallet/src/contract/actions/README.md index b9faa82..f1f88a6 100644 --- a/libwallet/src/contract/actions/README.md +++ b/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`. diff --git a/libwallet/src/contract/actions/mod.rs b/libwallet/src/contract/actions/mod.rs index 5681e2a..9ff6102 100644 --- a/libwallet/src/contract/actions/mod.rs +++ b/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, 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)) +} diff --git a/libwallet/src/contract/actions/sign.rs b/libwallet/src/contract/actions/sign.rs index fa0f2d3..e0b8ecd 100644 --- a/libwallet/src/contract/actions/sign.rs +++ b/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( w: &mut WalletBackend, @@ -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" + )); + } +} diff --git a/libwallet/src/contract/actions/view.rs b/libwallet/src/contract/actions/view.rs index 65ee403..630ca34 100644 --- a/libwallet/src/contract/actions/view.rs +++ b/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, 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( w: &mut WalletBackend, @@ -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 ); }