diff --git a/controller/tests/contract_revoke.rs b/controller/tests/contract_revoke.rs index 8f6b50a..36d5f03 100644 --- a/controller/tests/contract_revoke.rs +++ b/controller/tests/contract_revoke.rs @@ -208,6 +208,16 @@ 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 { + setup_args: ContractSetupArgsAPI { + selection_args: common::contract_selection_args(), + net_change: Some(-1_000_000_000), + num_participants: 2, + add_outputs: true, + ..Default::default() + }, + ..Default::default() + }; // Send (with early lock), locking an input under the contract tx. let mut slate = Slate::blank(0, true); @@ -216,21 +226,26 @@ fn contract_revoke_resume_impl(test_dir: &'static str) -> Result<(), wallet::Err 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), - num_participants: 2, - add_outputs: true, - ..Default::default() - }, - ..Default::default() - }; - slate = api.contract_new(m, args)?; + 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( diff --git a/controller/tests/contract_srs.rs b/controller/tests/contract_srs.rs index 7d8776d..460a01b 100644 --- a/controller/tests/contract_srs.rs +++ b/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)); diff --git a/libwallet/src/contract/actions/README.md b/libwallet/src/contract/actions/README.md index d6e3df9..57b716a 100644 --- a/libwallet/src/contract/actions/README.md +++ b/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 diff --git a/libwallet/src/contract/actions/new.rs b/libwallet/src/contract/actions/new.rs index 318fd1b..b0e5a4a 100644 --- a/libwallet/src/contract/actions/new.rs +++ b/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( w: &mut WalletBackend, 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) } diff --git a/libwallet/src/contract/actions/revoke.rs b/libwallet/src/contract/actions/revoke.rs index 78e2543..15542ef 100644 --- a/libwallet/src/contract/actions/revoke.rs +++ b/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()); diff --git a/libwallet/src/contract/actions/setup.rs b/libwallet/src/contract/actions/setup.rs index 84a5936..56e1b54 100644 --- a/libwallet/src/contract/actions/setup.rs +++ b/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(w: &mut WalletBackend, 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( keychain_mask: Option<&SecretKey>, slate: &Slate, setup_args: &ContractSetupArgsAPI, + existing_context: Option, ) -> 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( diff --git a/libwallet/src/contract/actions/sign.rs b/libwallet/src/contract/actions/sign.rs index 375a103..e5c5ff1 100644 --- a/libwallet/src/contract/actions/sign.rs +++ b/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( - w, - keychain_mask, - slate, - setup_args, - existing_context.as_ref(), - )?; + let (sl, context) = compute(w, keychain_mask, slate, setup_args, existing_context)?; // Atomically commit state contract::utils::save_step(w, keychain_mask, &sl, context, will_add_outputs)?; @@ -92,20 +86,20 @@ pub fn compute( keychain_mask: Option<&SecretKey>, slate: &Slate, setup_args: &ContractSetupArgsAPI, - existing_context: Option<&Context>, + existing_context: Option, ) -> 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)?;