fix contract handling and update docs

controller/src/command.rs

@@ -1961,12 +1961,19 @@ where
 	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| {
-		let slate_opt = api.contract_revoke(m, &ContractRevokeArgsAPI { tx_id: args.tx_id })?;
-		// TODO: replace dest="nope" with our own address and add --as-json support
-		if slate_opt.is_some() {
-			let slate_out =
-				prepare_slatepack(api, keychain_mask, &slate_opt.unwrap(), "nope", None)?;
+		let slate_opt = api.contract_revoke(
+			m,
+			&ContractRevokeArgsAPI {
+				tx_id: args.tx_id,
+				src_acct_name: None,
+			},
+		)?;
+		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)?;
 			println!("{}", slate_out);
+		} else {
+			println!("Contract revoked. No replacement transaction was created.");
 		}
 
 		Ok(())

controller/tests/contract_revoke.rs

@@ -31,16 +31,48 @@ mod common;
 use common::{clean_output_dir, create_wallets, setup};
 use std::path::PathBuf;
 
-/// Revoke a contract while a different account is active than the one that locked
-/// the inputs. The cancel + self-spend must still target the inputs' account.
+/// Revoke the transaction in the requested account when two accounts share a transaction id.
 fn contract_revoke_other_account_impl(test_dir: &'static str) -> Result<(), libwallet::Error> {
-	// One wallet: empty "default" account plus a funded "account1".
+	// Both accounts are funded equally, so their contract transactions get the same id.
 	let (wallets, _chain, stopper, _bh) =
-		create_wallets(vec![vec![("default", 0), ("account1", 4)]], test_dir).unwrap();
+		create_wallets(vec![vec![("default", 4), ("account1", 4)]], test_dir).unwrap();
 	let wallet1 = wallets[0].0.clone();
 	let mask1 = wallets[0].1.as_ref();
 
-	// Send (with early lock) from account1, locking one of its inputs.
+	// Create an early-locked contract in the default account.
+	wallet::controller::owner_single_use(
+		wallet1.clone(),
+		mask1,
+		PathBuf::from(test_dir),
+		|api, m| {
+			api.contract_new(
+				m,
+				&ContractNewArgsAPI {
+					setup_args: ContractSetupArgsAPI {
+						net_change: Some(-1_000_000_000),
+						num_participants: 2,
+						add_outputs: true,
+						..Default::default()
+					},
+					..Default::default()
+				},
+			)?;
+			Ok(())
+		},
+	)?;
+	let mut default_tx_id = 0;
+	wallet::controller::owner_single_use(
+		wallet1.clone(),
+		mask1,
+		PathBuf::from(test_dir),
+		|api, m| {
+			let (_, txs) = api.retrieve_txs(m, true, None, None, None)?;
+			default_tx_id = txs.last().unwrap().id;
+			Ok(())
+		},
+	)?;
+
+	// Create another early-locked contract in account1.
 	{
 		wallet_inst!(wallet1, w);
 		w.set_parent_key_id_by_name("account1")?;
@@ -80,6 +112,7 @@ fn contract_revoke_other_account_impl(test_dir: &'static str) -> Result<(), libw
 			Ok(())
 		},
 	)?;
+	assert_eq!(tx_id, default_tx_id);
 
 	// Switch the active account to "default" — different from the inputs' account.
 	{
@@ -87,15 +120,20 @@ fn contract_revoke_other_account_impl(test_dir: &'static str) -> Result<(), libw
 		w.set_parent_key_id_by_name("default")?;
 	}
 
-	// Revoke. With the active account wrong, this must still cancel + self-spend the
-	// account1 transaction (it derives the account from the locked input).
+	// Revoke account1 while the default account is active.
 	let mut revoked = None;
 	wallet::controller::owner_single_use(
 		wallet1.clone(),
 		mask1,
 		PathBuf::from(test_dir),
 		|api, m| {
-			revoked = api.contract_revoke(m, &ContractRevokeArgsAPI { tx_id })?;
+			revoked = api.contract_revoke(
+				m,
+				&ContractRevokeArgsAPI {
+					tx_id,
+					src_acct_name: Some("account1".to_string()),
+				},
+			)?;
 			Ok(())
 		},
 	)?;
@@ -104,6 +142,21 @@ fn contract_revoke_other_account_impl(test_dir: &'static str) -> Result<(), libw
 		"revoke should produce a self-spend slate"
 	);
 
+	// The default account's transaction is still locked.
+	wallet::controller::owner_single_use(
+		wallet1.clone(),
+		mask1,
+		PathBuf::from(test_dir),
+		|api, m| {
+			let (_, outs) = api.retrieve_outputs(m, true, false, None)?;
+			assert!(outs.iter().any(|o| {
+				o.output.status == OutputStatus::Locked
+					&& o.output.tx_log_entry == Some(default_tx_id)
+			}));
+			Ok(())
+		},
+	)?;
+
 	// Back on account1: the original tx is cancelled and no input is left locked.
 	{
 		wallet_inst!(wallet1, w);
@@ -212,7 +265,13 @@ fn contract_revoke_resume_impl(test_dir: &'static str) -> Result<(), libwallet::
 		mask1,
 		PathBuf::from(test_dir),
 		|api, m| {
-			revoked = api.contract_revoke(m, &ContractRevokeArgsAPI { tx_id })?;
+			revoked = api.contract_revoke(
+				m,
+				&ContractRevokeArgsAPI {
+					tx_id,
+					src_acct_name: None,
+				},
+			)?;
 			Ok(())
 		},
 	)?;

doc/contracts.md

@@ -1,7 +1,8 @@
 # Wallet contracts
 
 Contracts use the same flow for standard sends and invoice transactions. Signing is kept
-as an explicit step, so both sides can check the transaction before they sign it.
+as an explicit step. Each side can check the slate it receives and its own contribution,
+but not commitments added by the other side later in the flow.
 
 A two-party contract is `new`, `sign`, `sign`. A self-spend is `new`, `sign`. There is no
 separate `setup` command. Setup is done by `new`, or by the first `sign` when needed.
@@ -46,7 +47,8 @@ inputs, outputs and fees, and it cannot find a contract by id.
 self-spend of that input. The caller still has to post it, and either transaction can win
 if the original is already in the mempool. When the wallet added no input there is no
 replacement transaction. The replacement does not use a higher fee. An interrupted
-`revoke` can be run again.
+`revoke` can be run again. Transaction ids belong to an account, so the CLI uses the
+account selected with `--account`.
 
 ## Current limitations
 

libwallet/src/contract/actions/README.md

@@ -54,14 +54,21 @@ Separating side effects until the 'save_step' part would make these functions mu
  - 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`.
+ - Correct the suggested change direction in `view` and keep that direction in the CLI.
  - Raise the replacement fee when `revoke` races a transaction already in the mempool.
  - Merge invoice-proof retrieval with the existing payment-proof API.
  - Confirm that invoice-proof retrieval excludes the transaction fee from the amount.
  - Move payment-proof creation and verification onto `Slate` so it is versioned with the
    slate format.
- - Honour `ProofArgs::proof_type`; contract proofs currently always use the invoice type.
- - Clean up Slatepack output: return valid JSON after broadcasting, choose the sender
-   derivation path explicitly, and keep encrypted outgoing files identifiable locally.
+ - Verify an invoice promise when paying and derive the receiver from the participants.
+ - Bind and validate the contract proof type in the serialized proof.
+ - Keep V5 when decoding Slatepacks through Owner RPC.
+ - 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.
  - Decide whether `target_slate_version` should control Slatepack output or be removed.
  - Add contract history, lookup by id, transport support and configurable proof memos.
@@ -77,10 +84,10 @@ Separating side effects until the 'save_step' part would make these functions mu
  - a receiver with no spendable input, through lack of funds or of confirmations
  - invalid or inconsistent slates, including changed commitments and unexpected states
  - the CLI conflict between `--no-payjoin` and `--use-inputs`
- - revoke when different accounts have the same numeric transaction id
  - a committed fee that no longer covers the selected inputs and outputs
  - slate contents at each step, not only the end state
  - the foreign API entry points for `new` and `sign`
+ - Slatepack version negotiation with the last supported wallet release
 
 #### save_step
 

libwallet/src/contract/actions/revoke.rs

@@ -16,7 +16,7 @@
 use crate::backend::WalletBackend;
 use crate::blake2::blake2b::blake2b;
 use crate::contract::types::{ContractRevokeArgsAPI, ContractSetupArgsAPI, OutputSelectionArgs};
-use crate::contract::{new, sign};
+use crate::contract::{new, sign, utils};
 use crate::error::Error;
 use crate::grin_keychain::Keychain;
 use crate::grin_util::secp::key::SecretKey;
@@ -55,6 +55,7 @@ where
 	// cannot be built until the input is unlocked.
 	// FUTURE: we may want to boost fees if we notice the original tx in the mempool.
 	let tx_id = args.tx_id;
+	let parent_key_id = utils::parent_key_for(w, args.src_acct_name.as_ref())?;
 
 	// Inputs we contributed to tx_id that are still recoverable. Locked => the original tx
 	// is still active; Unspent => a previous revoke cancelled it but the self-spend did not
@@ -65,44 +66,39 @@ where
 		.iter()?
 		.filter(|out| {
 			out.tx_log_entry == Some(tx_id)
+				&& out.root_key_id == parent_key_id
 				&& (out.status == OutputStatus::Locked || out.status == OutputStatus::Unspent)
 		})
 		.collect::<Vec<OutputData>>();
 
-	// Determine the account that owns the inputs so the cancel and the self-spend target it
-	// rather than whatever account happens to be active.
-	let parent_key_id = match my_contributed_inputs.first() {
-		Some(out) => out.root_key_id.clone(),
-		None => w.parent_key_id(),
-	};
-
 	// Cancel the original tx only if it is still in a cancellable state. On a resumed revoke
 	// it is already a *Cancelled type (and the inputs are Unspent), so we skip straight to
 	// re-spending them.
-	let revoked = w.get_tx_log_entry_by_id(parent_key_id.clone(), tx_id)?;
-	let revoked_slate_id = revoked.as_ref().and_then(|e| e.tx_slate_id);
-	let needs_cancel = match revoked.as_ref() {
-		Some(e) => matches!(
-			e.tx_type,
-			TxLogEntryType::TxSent
-				| TxLogEntryType::TxReceived
-				| TxLogEntryType::TxReverted
-				| TxLogEntryType::TxSelfSpend
-		),
-		None => false,
-	};
+	let revoked = w
+		.get_tx_log_entry_by_id(parent_key_id.clone(), tx_id)?
+		.ok_or_else(|| Error::NotFoundErr(format!("Transaction {}", tx_id)))?;
+	let revoked_slate_id = revoked.tx_slate_id;
+	let needs_cancel = matches!(
+		revoked.tx_type,
+		TxLogEntryType::TxSent
+			| TxLogEntryType::TxReceived
+			| TxLogEntryType::TxReverted
+			| TxLogEntryType::TxSelfSpend
+	);
 	if needs_cancel {
 		// 1. Unlock the inputs by cancelling the original tx.
 		tx::cancel_tx(&mut *w, keychain_mask, &parent_key_id, Some(tx_id), None)?;
 		// Drop the canceled slate's private context if one still exists (signing already
 		// deletes it).
 		if let Some(slate_id) = revoked_slate_id {
-			if w.get_private_context(keychain_mask, slate_id.as_bytes())
-				.is_ok()
-			{
-				let mut batch = w.batch(keychain_mask)?;
-				batch.delete_private_context(slate_id.as_bytes())?;
-				batch.commit()?;
+			match w.get_private_context(keychain_mask, slate_id.as_bytes()) {
+				Ok(_) => {
+					let mut batch = w.batch(keychain_mask)?;
+					batch.delete_private_context(slate_id.as_bytes())?;
+					batch.commit()?;
+				}
+				Err(Error::NotFoundErr(_)) => {}
+				Err(e) => return Err(e),
 			}
 		}
 	}

libwallet/src/contract/proofs.rs

@@ -17,7 +17,7 @@
 //! by legacy transactions
 
 use crate::backend::WalletBackend;
-use crate::contract::types::ProofArgs;
+use crate::contract::types::{ProofArgs, ProofType};
 use crate::grin_core::libtx::aggsig;
 use crate::grin_core::libtx::secp_ser;
 use crate::grin_core::ser as grin_ser;
@@ -39,6 +39,15 @@ use ed25519_dalek::VerifyingKey as DalekPublicKey;
 use ed25519_dalek::{Signer, Verifier};
 use grin_util::secp::Message;
 
+pub(super) fn check_proof_type(proof_type: &ProofType) -> Result<(), Error> {
+	match proof_type {
+		ProofType::Invoice => Ok(()),
+		_ => Err(Error::GenericError(
+			"Only invoice contract proofs are supported".to_string(),
+		)),
+	}
+}
+
 /// All elements required to validate a proof within a single struct
 #[derive(Serialize, Deserialize, Debug, Clone, PartialEq, Eq)]
 pub struct ProofWitness {
@@ -368,7 +377,7 @@ impl serde::Serialize for InvoiceProofBin {
 }
 
 /// Adds all info needed for a payment proof to a slate, complete with signed recipient data
-pub fn add_payment_proof<C, K>(
+pub(super) fn add_payment_proof<C, K>(
 	wallet: &mut WalletBackend<C, K>,
 	keychain_mask: Option<&SecretKey>,
 	slate: &mut Slate,
@@ -379,8 +388,6 @@ where
 	C: NodeClient,
 	K: Keychain,
 {
-	// Invoice (type 1) is the only contract proof type today; other types are future
-	// work tied to the early-payment-proof RFC.
 	let (invoice_proof, promise_signature, receiver_address) =
 		generate_invoice_signature(wallet, keychain_mask, slate, context, proof_args)?;
 	// Carry over the timestamp the promise signature was made over rather than reading
@@ -438,6 +445,13 @@ mod tests {
 	use super::*;
 	use crate::slate_versions::tests::populate_test_slate;
 
+	#[test]
+	fn rejects_unsupported_proofs() {
+		assert!(check_proof_type(&ProofType::Invoice).is_ok());
+		assert!(check_proof_type(&ProofType::Legacy).is_err());
+		assert!(check_proof_type(&ProofType::SenderNonce).is_err());
+	}
+
 	#[test]
 	fn ser_invoice_proof_bin() -> Result<(), Error> {
 		let mut slate = populate_test_slate()?;

libwallet/src/contract/slate.rs

@@ -42,6 +42,9 @@ where
 {
 	// FUTURE: move proof handling onto Slate itself so it can be versioned (slate.add_payment_proof_data()).
 	debug!("contract::slate::add_payment_proof => called");
+	if !proof_args.suppress_proof {
+		super::proofs::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() {

libwallet/src/contract/types.rs

@@ -95,7 +95,7 @@ pub enum ProofType {
 pub struct ProofArgs {
 	/// If net change is positive during this step, whether to suppress the creation of payment proof
 	pub suppress_proof: bool,
-	/// Requested proof type. Contract proofs currently always use the invoice type.
+	/// Requested proof type. Only invoice proofs are currently supported.
 	pub proof_type: ProofType,
 	/// Sender address (required at some stage, may not necessarily be in slate so can be provided explicitly)
 	#[serde(with = "dalek_ser::option_dalek_pubkey_serde")]
@@ -108,7 +108,7 @@ impl Default for ProofArgs {
 			// Proofs are opt-in (#729): a `false` default made the receiver build an
 			// invoice promise with no sender address, failing with NoSenderAddressProvided.
 			suppress_proof: true,
-			proof_type: ProofType::Legacy,
+			proof_type: ProofType::Invoice,
 			sender_address: None,
 		}
 	}
@@ -211,4 +211,6 @@ impl Default for ContractView {
 pub struct ContractRevokeArgsAPI {
 	/// Tx id to cancel
 	pub tx_id: u32,
+	/// Account containing the transaction, or the active account when omitted
+	pub src_acct_name: Option<String>,
 }

libwallet/src/contract/utils.rs

@@ -210,12 +210,13 @@ where
 	let is_step2 = !context.log_id.is_some() && is_signed;
 
 	let mut tx_log_entry = {
-		if !context.log_id.is_some() {
+		if context.log_id.is_none() {
 			// We create a new entry with log_id=0 and but replace it with the real id before committing
 			create_tx_log_entry(slate, context.get_net_change()?, parent_key_id.clone(), 0)?
 		} else {
-			w.get_tx_log_entry_by_id(parent_key_id.clone(), context.log_id.unwrap())?
-				.unwrap()
+			let log_id = context.log_id.unwrap();
+			w.get_tx_log_entry_by_id(parent_key_id.clone(), log_id)?
+				.ok_or_else(|| Error::NotFoundErr(format!("Transaction log entry {}", log_id)))?
 		}
 	};
 
@@ -299,12 +300,14 @@ where
 	// gone so the nonce can never be reused. It is deleted in the is_signed && !is_step2
 	// branch above; verify the deletion actually took effect.
 	if is_signed && !is_step2 {
-		if w.get_private_context(keychain_mask, slate.id.as_bytes())
-			.is_ok()
-		{
-			return Err(Error::GenericError(
-				"signing context was not removed after signing".into(),
-			));
+		match w.get_private_context(keychain_mask, slate.id.as_bytes()) {
+			Err(Error::NotFoundErr(_)) => {}
+			Err(e) => return Err(e),
+			Ok(_) => {
+				return Err(Error::GenericError(
+					"signing context was not removed after signing".into(),
+				));
+			}
 		}
 	}
 
@@ -360,17 +363,15 @@ where
 {
 	// If we have a transaction log entry for that slatepack that has a kernel value, then
 	// we have already signed this slate.
-	let tx = w
-		.tx_log_iter()?
-		.flatten()
-		.find(|t| t.tx_slate_id.is_some() && t.tx_slate_id.unwrap() == slate_id);
-	let already_signed = tx.is_some() && tx.unwrap().kernel_excess.is_some();
-	if already_signed {
-		debug!("contract::utils::verify_not_signed => The slate has already been signed.");
-		return Err(Error::GenericError(
-			format!("Slate with id:{} has already been signed.", slate_id).into(),
-		)
-		.into());
+	for tx in w.tx_log_iter()? {
+		let tx = tx?;
+		if tx.tx_slate_id == Some(slate_id) && tx.kernel_excess.is_some() {
+			debug!("contract::utils::verify_not_signed => The slate has already been signed.");
+			return Err(Error::GenericError(format!(
+				"Slate with id:{} has already been signed.",
+				slate_id
+			)));
+		}
 	}
 
 	Ok(())