fix contract context retries

controller/tests/contract_revoke.rs

@@ -208,15 +208,7 @@ fn contract_revoke_resume_impl(test_dir: &'static str) -> Result<(), wallet::Err
 		create_wallets(vec![vec![("default", 4)]], test_dir).unwrap();
 	let wallet1 = wallets[0].0.clone();
 	let mask1 = wallets[0].1.as_ref();
-
+	let args = ContractNewArgsAPI {
-	// Send (with early lock), locking an input under the contract tx.
-	let mut slate = Slate::blank(0, true);
-	wallet::controller::owner_single_use(
-		wallet1.clone(),
-		mask1,
-		PathBuf::from(test_dir),
-		|api, m| {
-			let args = &ContractNewArgsAPI {
 		setup_args: ContractSetupArgsAPI {
 			selection_args: common::contract_selection_args(),
 			net_change: Some(-1_000_000_000),
@@ -226,11 +218,34 @@ fn contract_revoke_resume_impl(test_dir: &'static str) -> Result<(), wallet::Err
 		},
 		..Default::default()
 	};
-			slate = api.contract_new(m, args)?;
+
+	// Send (with early lock), locking an input under the contract tx.
+	let mut slate = Slate::blank(0, true);
+	wallet::controller::owner_single_use(
+		wallet1.clone(),
+		mask1,
+		PathBuf::from(test_dir),
+		|api, m| {
+			slate = api.contract_new(m, &args)?;
 			Ok(())
 		},
 	)?;
 	assert_eq!(slate.state, SlateState::Standard1);
+	{
+		wallet_inst!(wallet1, w);
+		let retried =
+			libwallet::contract::new(w, mask1, &args.setup_args, args.ttl_blocks, Some(slate.id))?;
+		assert_eq!(retried.id, slate.id);
+		assert_eq!(retried.fee_fields, slate.fee_fields);
+		assert_eq!(
+			retried.participant_data[0].public_nonce,
+			slate.participant_data[0].public_nonce
+		);
+		assert_eq!(
+			retried.participant_data[0].public_blind_excess,
+			slate.participant_data[0].public_blind_excess
+		);
+	}
 
 	let mut tx_id = 0;
 	wallet::controller::owner_single_use(

controller/tests/contract_srs.rs

@@ -667,6 +667,34 @@ fn contract_srs_tx_impl(test_dir: &'static str) -> Result<(), libwallet::Error>
 		},
 	)?;
 
+	// Keep our fee when setup is retried with the original slate
+	let sender_args = ContractSetupArgsAPI {
+		selection_args: common::contract_selection_args(),
+		net_change: Some(-1_000_000_000),
+		..Default::default()
+	};
+	let receiver_args = ContractSetupArgsAPI {
+		selection_args: common::contract_selection_args(),
+		net_change: Some(1_000_000_000),
+		..Default::default()
+	};
+	let incoming = {
+		wallet_inst!(send_wallet, w);
+		libwallet::contract::new(w, send_mask, &sender_args, None, None)?
+	};
+	let signed = {
+		wallet_inst!(recv_wallet, w);
+		let first = libwallet::contract::setup(w, recv_mask, &incoming, &receiver_args)?;
+		let retried = libwallet::contract::setup(w, recv_mask, &incoming, &receiver_args)?;
+		assert_eq!(retried.fee_fields, first.fee_fields);
+		libwallet::contract::sign(w, recv_mask, &incoming, &receiver_args)?
+	};
+	{
+		wallet_inst!(send_wallet, w);
+		let signed = libwallet::contract::sign(w, send_mask, &signed, &sender_args)?;
+		assert_eq!(signed.state, SlateState::Standard3);
+	}
+
 	// let logging finish
 	stopper.store(false, Ordering::Relaxed);
 	thread::sleep(Duration::from_millis(200));

libwallet/src/contract/actions/README.md

@@ -32,7 +32,6 @@ the slate when signing, so the counterparty does not see them earlier.
 Ideally we'd also separate side effects out of these functions e.g. computing the current_height
 or refreshing the outputs with updater::refresh_outputs(...). The current_height could be
 communicated through a &ChainState parameter which would collect these values before the call.
-Setup still reads an existing Context again; passing it through would avoid that DB read.
 Separating side effects until the 'save_step' part would make these functions much easier to test.
 
 `new` and `sign` stay on the Rust Foreign API for now. They are not exposed over JSON-RPC

libwallet/src/contract/actions/new.rs

@@ -27,7 +27,7 @@ use uuid::Uuid;
 
 /// Create a new contract with initial setup done by the initiator. `slate_id`, when
 /// provided, fixes the slate id (rather than a random one) so the caller can make a
-/// retried creation idempotent: get_or_create reuses the existing context for that id.
+/// retried creation idempotent by reusing the existing context for that id.
 pub fn new<C, K>(
 	w: &mut WalletBackend<C, K>,
 	keychain_mask: Option<&SecretKey>,
@@ -73,19 +73,19 @@ where
 	// Contracts start with V4 and move to V5 when their proof data requires it
 	slate.version_info.version = 4;
 	// Use a caller-supplied id when given, so a retried creation reuses the same context.
-	let mut reused_context = false;
+	let mut existing_context = None;
 	if let Some(id) = slate_id {
 		slate.id = id;
 		match w.get_private_context(keychain_mask, slate.id.as_bytes()) {
 			Ok(context) => {
 				slate.ttl_cutoff_height = context.contract_ttl_cutoff_height.unwrap_or(0);
-				reused_context = true;
+				existing_context = Some(context);
 			}
 			Err(Error::NotFoundErr(_)) => {}
 			Err(e) => return Err(e),
 		}
 	}
-	if !reused_context {
+	if existing_context.is_none() {
 		if let Some(blocks) = ttl_blocks {
 			if blocks == 0 {
 				return Err(Error::GenericError(
@@ -104,5 +104,5 @@ where
 	debug!("contract::new => slate amount: {}", slate.amount);
 
 	// Perform setup for the slate
-	setup::compute(w, keychain_mask, &mut slate, setup_args)
+	setup::compute(w, keychain_mask, &slate, setup_args, existing_context)
 }

libwallet/src/contract/actions/revoke.rs

@@ -27,7 +27,7 @@ use uuid::Uuid;
 
 /// Deterministic slate id for the self-spend that revokes a given contract slate.
 /// Derived from the revoked slate id so a revoke interrupted between creating and
-/// signing the self-spend resumes by reusing the same context (get_or_create), rather
+/// signing the self-spend resumes by reusing the same context, rather
 /// than orphaning a fresh self-spend on each retry.
 fn self_spend_slate_id(revoked_slate_id: Uuid) -> Uuid {
 	let hash = blake2b(16, b"grin-contract-revoke", revoked_slate_id.as_bytes());

libwallet/src/contract/actions/setup.rs

@@ -23,6 +23,7 @@ use crate::grin_keychain::Keychain;
 use crate::grin_util::secp::key::SecretKey;
 use crate::slate::Slate;
 use crate::types::{Context, NodeClient};
+use grin_core::core::FeeFields;
 
 // Contract deadlines use the node tip because the active account may not own the context
 fn check_contract_ttl<C, K>(w: &mut WalletBackend<C, K>, slate: &Slate) -> Result<(), Error>
@@ -57,7 +58,7 @@ where
 	}
 	contract::utils::verify_not_signed(w, slate.id)?;
 	// Compute state for 'setup'
-	let (slate, context) = compute(w, keychain_mask, slate, setup_args)?;
+	let (slate, context) = compute(w, keychain_mask, slate, setup_args, None)?;
 
 	// Atomically commit state
 	contract::utils::save_step(w, keychain_mask, &slate, context, setup_args.add_outputs)?;
@@ -71,6 +72,7 @@ pub fn compute<C, K>(
 	keychain_mask: Option<&SecretKey>,
 	slate: &Slate,
 	setup_args: &ContractSetupArgsAPI,
+	existing_context: Option<Context>,
 ) -> Result<(Slate, Context), Error>
 where
 	C: NodeClient,
@@ -80,7 +82,10 @@ where
 	check_contract_ttl(w, &sl)?;
 
 	// Get or create the Context and check the setup arguments
-	let mut context = contract::context::get_or_create(w, keychain_mask, &mut sl, setup_args)?;
+	let mut context = match existing_context {
+		Some(context) => context,
+		None => contract::context::get_or_create(w, keychain_mask, &mut sl, setup_args)?,
+	};
 	contract::utils::verify_ttl(context.contract_ttl_cutoff_height, &sl)?;
 	let context_args = context
 		.setup_args
@@ -90,6 +95,19 @@ where
 
 	// Add keys and payment proof to slate (both are idempotent operations)
 	let keychain = w.keychain(keychain_mask)?;
+	// Restore our fee when a stored context is applied to a slate without our keys
+	if context.log_id.is_some() {
+		let has_our_keys = match sl.find_index_matching_context(&keychain, &context) {
+			Ok(_) => true,
+			Err(Error::ContextToIndex) => false,
+			Err(e) => return Err(e),
+		};
+		if !has_our_keys {
+			if let Some(fee) = context.fee {
+				sl.fee_fields = FeeFields::new(0, sl.fee_fields.fee() + fee.fee())?;
+			}
+		}
+	}
 	contract::proofs::commit_sender_nonce(&sl, &mut context, keychain.secp())?;
 	contract::slate::add_keys(&mut sl, &keychain, &mut context)?;
 	contract::slate::add_payment_proof(

libwallet/src/contract/actions/sign.rs

@@ -72,13 +72,7 @@ where
 		.map(|context| !context.has_inputs_or_outputs())
 		.unwrap_or(true);
 	// Compute state for 'sign'
-	let (sl, context) = compute(
+	let (sl, context) = compute(w, keychain_mask, slate, setup_args, existing_context)?;
-		w,
-		keychain_mask,
-		slate,
-		setup_args,
-		existing_context.as_ref(),
-	)?;
 
 	// Atomically commit state
 	contract::utils::save_step(w, keychain_mask, &sl, context, will_add_outputs)?;
@@ -92,20 +86,20 @@ pub fn compute<C, K>(
 	keychain_mask: Option<&SecretKey>,
 	slate: &Slate,
 	setup_args: &ContractSetupArgsAPI,
-	existing_context: Option<&Context>,
+	existing_context: Option<Context>,
 ) -> Result<(Slate, Context), Error>
 where
 	C: NodeClient,
 	K: Keychain,
 {
-	let mut sl = slate.clone();
+	let sl = slate.clone();
 	contract::utils::verify_not_signed(w, sl.id)?;
 	// Reject terminal and unknown states before setup reserves keys.
 	contract::slate::next_state(&sl.state)?;
 
 	// Ensure net_change has been provided
 	let expected_net_change =
-		contract::utils::get_net_change(existing_context, setup_args.net_change)?;
+		contract::utils::get_net_change(existing_context.as_ref(), setup_args.net_change)?;
 	if existing_context.is_none() {
 		verify_first_sign_net_change(&sl.state, sl.amount, expected_net_change)?;
 	}
@@ -118,7 +112,8 @@ where
 	contract::slate::verify_incoming_own_commitments(w, keychain_mask, &sl)?;
 
 	// Ensure Setup phase is done and that inputs/outputs have been added to the Context
-	let (mut sl, mut context) = setup::compute(w, keychain_mask, &mut sl, &setup_args)?;
+	let (mut sl, mut context) =
+		setup::compute(w, keychain_mask, &sl, &setup_args, existing_context)?;
 	// Add outputs to the slate, verify the payment proof and sign the slate
 	contract::slate::add_outputs(w, keychain_mask, &mut sl, &context)?;
 	contract::slate::verify_own_commitments(w, keychain_mask, &sl, &context)?;