diff --git a/controller/src/command.rs b/controller/src/command.rs index b1f2032..64afbfa 100644 --- a/controller/src/command.rs +++ b/controller/src/command.rs @@ -1694,6 +1694,8 @@ pub struct ContractNewArgs { pub use_inputs: Option, /// Output amounts in nanogrin (one entry per output) pub make_outputs: Option>, + /// Minimum number of confirmations required for an input + pub minimum_confirmations: u64, // Future features /// Custom fee contribution @@ -1733,6 +1735,7 @@ impl ContractNewArgs { num_participants: self.num_participants, add_outputs: self.add_outputs, selection_args: OutputSelectionArgs { + minimum_confirmations: Some(self.minimum_confirmations), use_inputs: match self.use_inputs.as_ref() { Some(v) => Some(v.to_string()), None => None, @@ -1794,6 +1797,8 @@ pub struct ContractSetupArgs { pub use_inputs: Option, /// Output amounts in nanogrin (one entry per output) pub make_outputs: Option>, + /// Optional minimum number of confirmations required for an input + pub minimum_confirmations: Option, // Future features /// Whether we should automatically sign a receive of any value @@ -1831,6 +1836,7 @@ impl ContractSetupArgs { net_change: net_change, add_outputs: self.add_outputs, selection_args: OutputSelectionArgs { + minimum_confirmations: self.minimum_confirmations, use_inputs: match self.use_inputs.as_ref() { Some(v) => Some(v.to_string()), None => None, diff --git a/controller/tests/common/mod.rs b/controller/tests/common/mod.rs index 28115a5..8a41e94 100644 --- a/controller/tests/common/mod.rs +++ b/controller/tests/common/mod.rs @@ -87,6 +87,15 @@ pub fn clean_output_dir(test_dir: &str) { } } +#[allow(dead_code)] +pub fn contract_selection_args() -> libwallet::contract::types::OutputSelectionArgs { + // Contract fixtures mine only a few blocks. + libwallet::contract::types::OutputSelectionArgs { + minimum_confirmations: Some(1), + ..Default::default() + } +} + pub fn setup(test_dir: &str) { util::init_test_logger(); clean_output_dir(test_dir); diff --git a/controller/tests/contract_accounts.rs b/controller/tests/contract_accounts.rs index fb2fd82..1dec6df 100644 --- a/controller/tests/contract_accounts.rs +++ b/controller/tests/contract_accounts.rs @@ -169,6 +169,7 @@ fn contract_accounts_impl(test_dir: &'static str) -> Result<(), libwallet::Error // Send wallet inititates a standard transaction with --send=5 let args = &ContractNewArgsAPI { setup_args: ContractSetupArgsAPI { + selection_args: common::contract_selection_args(), net_change: Some(-5_000_000_000), ..Default::default() }, @@ -189,6 +190,7 @@ fn contract_accounts_impl(test_dir: &'static str) -> Result<(), libwallet::Error |api, m| { // Receive wallet calls --receive=5 let args = &mut ContractSetupArgsAPI { + selection_args: common::contract_selection_args(), net_change: Some(5_000_000_000), ..Default::default() }; @@ -209,6 +211,7 @@ fn contract_accounts_impl(test_dir: &'static str) -> Result<(), libwallet::Error PathBuf::from(test_dir), |api, m| { let args = &ContractSetupArgsAPI { + selection_args: common::contract_selection_args(), ..Default::default() }; slate = api.contract_sign(m, &slate, args)?; @@ -331,6 +334,7 @@ fn contract_unknown_account_impl(test_dir: &'static str) -> Result<(), libwallet |api, m| { let args = &ContractNewArgsAPI { setup_args: ContractSetupArgsAPI { + selection_args: common::contract_selection_args(), net_change: Some(-1_000_000_000), src_acct_name: Some("does_not_exist".to_string()), ..Default::default() @@ -372,6 +376,7 @@ fn contract_missing_net_change_impl(test_dir: &'static str) -> Result<(), libwal |api, m| { let args = &ContractNewArgsAPI { setup_args: ContractSetupArgsAPI { + selection_args: common::contract_selection_args(), net_change: None, ..Default::default() }, diff --git a/controller/tests/contract_accounts_switch.rs b/controller/tests/contract_accounts_switch.rs index d8fabe3..1f44ede 100644 --- a/controller/tests/contract_accounts_switch.rs +++ b/controller/tests/contract_accounts_switch.rs @@ -111,6 +111,7 @@ fn contract_accounts_switch_impl(test_dir: &'static str) -> Result<(), libwallet // Send wallet inititates a standard transaction with --send=5 let args = &ContractNewArgsAPI { setup_args: ContractSetupArgsAPI { + selection_args: common::contract_selection_args(), net_change: Some(-5_000_000_000), ..Default::default() }, @@ -134,6 +135,7 @@ fn contract_accounts_switch_impl(test_dir: &'static str) -> Result<(), libwallet |api, m| { // Receive wallet calls --receive=5 let args = &mut ContractSetupArgsAPI { + selection_args: common::contract_selection_args(), net_change: Some(5_000_000_000), ..Default::default() }; @@ -155,6 +157,7 @@ fn contract_accounts_switch_impl(test_dir: &'static str) -> Result<(), libwallet PathBuf::from(test_dir), |api, m| { let args = &ContractSetupArgsAPI { + selection_args: common::contract_selection_args(), ..Default::default() }; slate = api.contract_sign(m, &slate, args)?; diff --git a/controller/tests/contract_early_lock.rs b/controller/tests/contract_early_lock.rs index 63a7528..41bf7b3 100644 --- a/controller/tests/contract_early_lock.rs +++ b/controller/tests/contract_early_lock.rs @@ -68,6 +68,7 @@ fn contract_early_lock_tx_impl(test_dir: &'static str) -> Result<(), libwallet:: // Send wallet inititates a standard transaction with --send=80 let args = &ContractNewArgsAPI { setup_args: ContractSetupArgsAPI { + selection_args: common::contract_selection_args(), net_change: Some(-80_000_000_000), num_participants: 2, add_outputs: true, @@ -115,6 +116,7 @@ fn contract_early_lock_tx_impl(test_dir: &'static str) -> Result<(), libwallet:: m, &slate, &ContractSetupArgsAPI { + selection_args: common::contract_selection_args(), net_change: Some(80_000_000_000), ..Default::default() }, diff --git a/controller/tests/contract_early_proofs_rsr.rs b/controller/tests/contract_early_proofs_rsr.rs index b05c783..b8523fc 100644 --- a/controller/tests/contract_early_proofs_rsr.rs +++ b/controller/tests/contract_early_proofs_rsr.rs @@ -81,6 +81,7 @@ fn contract_early_proofs_rsr_test_impl(test_dir: &'static str) -> Result<(), lib // Receive wallet (invoice) calls --receive=5 let args = &mut ContractNewArgsAPI { setup_args: ContractSetupArgsAPI { + selection_args: common::contract_selection_args(), net_change: Some(5_000_000_000), ..Default::default() }, @@ -106,6 +107,7 @@ fn contract_early_proofs_rsr_test_impl(test_dir: &'static str) -> Result<(), lib |api, m| { // Sending wallet (invoice) signs let args = &ContractSetupArgsAPI { + selection_args: common::contract_selection_args(), net_change: Some(-5_000_000_000), ..Default::default() }; @@ -125,6 +127,7 @@ fn contract_early_proofs_rsr_test_impl(test_dir: &'static str) -> Result<(), lib PathBuf::from(test_dir), |api, m| { let args = &ContractSetupArgsAPI { + selection_args: common::contract_selection_args(), ..Default::default() }; slate = api.contract_sign(m, &slate, args)?; diff --git a/controller/tests/contract_early_proofs_srs.rs b/controller/tests/contract_early_proofs_srs.rs index 344cb79..5d61839 100644 --- a/controller/tests/contract_early_proofs_srs.rs +++ b/controller/tests/contract_early_proofs_srs.rs @@ -69,6 +69,7 @@ fn contract_early_proofs_srs_test_impl(test_dir: &'static str) -> Result<(), lib // Send wallet inititates a standard transaction with --send=5 let args = &ContractNewArgsAPI { setup_args: ContractSetupArgsAPI { + selection_args: common::contract_selection_args(), net_change: Some(-5_000_000_000), ..Default::default() }, @@ -90,6 +91,7 @@ fn contract_early_proofs_srs_test_impl(test_dir: &'static str) -> Result<(), lib |api, m| { // Receive wallet calls --receive=5 let args = &mut ContractSetupArgsAPI { + selection_args: common::contract_selection_args(), net_change: Some(5_000_000_000), ..Default::default() }; @@ -112,6 +114,7 @@ fn contract_early_proofs_srs_test_impl(test_dir: &'static str) -> Result<(), lib PathBuf::from(test_dir), |api, m| { let args = &ContractSetupArgsAPI { + selection_args: common::contract_selection_args(), ..Default::default() }; let mut tampered = slate.clone(); diff --git a/controller/tests/contract_revoke.rs b/controller/tests/contract_revoke.rs index e1169b8..3bdb3be 100644 --- a/controller/tests/contract_revoke.rs +++ b/controller/tests/contract_revoke.rs @@ -49,6 +49,7 @@ fn contract_revoke_other_account_impl(test_dir: &'static str) -> Result<(), libw m, &ContractNewArgsAPI { setup_args: ContractSetupArgsAPI { + selection_args: common::contract_selection_args(), net_change: Some(-1_000_000_000), num_participants: 2, add_outputs: true, @@ -85,6 +86,7 @@ fn contract_revoke_other_account_impl(test_dir: &'static str) -> Result<(), libw |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, @@ -215,6 +217,7 @@ fn contract_revoke_resume_impl(test_dir: &'static str) -> Result<(), libwallet:: |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, diff --git a/controller/tests/contract_rsr.rs b/controller/tests/contract_rsr.rs index 88a9ea5..fd252a5 100644 --- a/controller/tests/contract_rsr.rs +++ b/controller/tests/contract_rsr.rs @@ -52,6 +52,7 @@ fn contract_rsr_tx_impl(test_dir: &'static str) -> Result<(), libwallet::Error> // Receive wallet inititates an invoice transaction with --receive=5 let args = &mut ContractNewArgsAPI { setup_args: ContractSetupArgsAPI { + selection_args: common::contract_selection_args(), net_change: Some(5_000_000_000), ..Default::default() }, @@ -70,6 +71,7 @@ fn contract_rsr_tx_impl(test_dir: &'static str) -> Result<(), libwallet::Error> |api, m| { // Send Wallet calls --send=5 let args = &ContractSetupArgsAPI { + selection_args: common::contract_selection_args(), net_change: Some(-5_000_000_000), ..Default::default() }; @@ -86,6 +88,7 @@ fn contract_rsr_tx_impl(test_dir: &'static str) -> Result<(), libwallet::Error> PathBuf::from(test_dir), |api, m| { let args = &mut ContractSetupArgsAPI { + selection_args: common::contract_selection_args(), ..Default::default() }; slate = api.contract_sign(m, &slate, args)?; diff --git a/controller/tests/contract_self_spend.rs b/controller/tests/contract_self_spend.rs index 2909b6f..74e18d4 100644 --- a/controller/tests/contract_self_spend.rs +++ b/controller/tests/contract_self_spend.rs @@ -50,6 +50,7 @@ fn contract_self_spend_tx_impl(test_dir: &'static str) -> Result<(), libwallet:: // Send wallet inititates a standard transaction with --send=0 let args = &ContractNewArgsAPI { setup_args: ContractSetupArgsAPI { + selection_args: common::contract_selection_args(), net_change: Some(0), num_participants: 1, ..Default::default() @@ -69,6 +70,7 @@ fn contract_self_spend_tx_impl(test_dir: &'static str) -> Result<(), libwallet:: PathBuf::from(test_dir), |api, m| { let args = &ContractSetupArgsAPI { + selection_args: common::contract_selection_args(), ..Default::default() }; slate = api.contract_sign(m, &slate, args)?; diff --git a/controller/tests/contract_self_spend_cancel.rs b/controller/tests/contract_self_spend_cancel.rs index 6612580..e3b17cd 100644 --- a/controller/tests/contract_self_spend_cancel.rs +++ b/controller/tests/contract_self_spend_cancel.rs @@ -48,6 +48,7 @@ fn contract_self_spend_cancel_impl(test_dir: &'static str) -> Result<(), libwall // Send wallet initiates a standard transaction with --send=0 let args = &ContractNewArgsAPI { setup_args: ContractSetupArgsAPI { + selection_args: common::contract_selection_args(), net_change: Some(0), num_participants: 1, ..Default::default() @@ -137,6 +138,7 @@ fn contract_self_spend_cancel_missing_stored_tx_impl( |api, m| { let args = &ContractNewArgsAPI { setup_args: ContractSetupArgsAPI { + selection_args: common::contract_selection_args(), net_change: Some(0), num_participants: 1, ..Default::default() @@ -155,6 +157,7 @@ fn contract_self_spend_cancel_missing_stored_tx_impl( PathBuf::from(test_dir), |api, m| { let args = &ContractSetupArgsAPI { + selection_args: common::contract_selection_args(), ..Default::default() }; slate = api.contract_sign(m, &slate, args)?; diff --git a/controller/tests/contract_self_spend_custom.rs b/controller/tests/contract_self_spend_custom.rs index 376911a..fe24503 100644 --- a/controller/tests/contract_self_spend_custom.rs +++ b/controller/tests/contract_self_spend_custom.rs @@ -62,7 +62,7 @@ fn contract_self_spend_custom_tx_impl(test_dir: &'static str) -> Result<(), libw let mut slate = Slate::blank(0, true); // this gets overriden below let selection_args = OutputSelectionArgs { - min_input_confirmation: 0, + minimum_confirmations: Some(0), use_inputs: Some(use_inputs.clone()), // we will use two coinbase inputs // the sum is such that it will need to pick another input making total of 3 inputs make_outputs: Some(vec![ @@ -100,7 +100,10 @@ fn contract_self_spend_custom_tx_impl(test_dir: &'static str) -> Result<(), libw send_mask, PathBuf::from(test_dir), |api, m| { + let mut current_selection = selection_args.clone(); + current_selection.minimum_confirmations = None; let args = &ContractSetupArgsAPI { + selection_args: current_selection, ..Default::default() }; slate = api.contract_sign(m, &slate, args)?; @@ -196,3 +199,102 @@ fn wallet_contract_self_spend_custom_tx() -> Result<(), libwallet::Error> { clean_output_dir(test_dir); Ok(()) } + +#[test] +fn contract_min_confirmations() -> Result<(), libwallet::Error> { + let test_dir = "test_output/contract_min_confirmations"; + setup(test_dir); + let (wallets, _, stopper, bh) = create_wallets( + vec![ + vec![("default", 7)], + vec![("default", 1)], + vec![("default", 3)], + ], + test_dir, + ) + .unwrap(); + let send_wallet = wallets[0].0.clone(); + let send_mask = wallets[0].1.as_ref(); + let recv_wallet = wallets[1].0.clone(); + let recv_mask = wallets[1].1.as_ref(); + let mut slate = Slate::blank(0, true); + + wallet::controller::owner_single_use( + send_wallet.clone(), + send_mask, + PathBuf::from(test_dir), + |api, m| { + slate = api.contract_new( + m, + &ContractNewArgsAPI { + setup_args: ContractSetupArgsAPI { + net_change: Some(-(consensus::GRIN_BASE as i64)), + selection_args: OutputSelectionArgs { + minimum_confirmations: Some(10), + ..Default::default() + }, + ..Default::default() + }, + }, + )?; + Ok(()) + }, + )?; + assert_eq!(slate.state, SlateState::Standard1); + + wallet::controller::owner_single_use( + recv_wallet, + recv_mask, + PathBuf::from(test_dir), + |api, m| { + let (_, outputs) = api.retrieve_outputs(m, true, false, None)?; + assert_eq!(outputs.len(), 1); + assert!(outputs[0].output.eligible_to_spend(bh, 1)); + assert!(!outputs[0].output.eligible_to_spend(bh, 10)); + slate = api.contract_sign( + m, + &slate, + &ContractSetupArgsAPI { + net_change: Some(consensus::GRIN_BASE as i64), + selection_args: OutputSelectionArgs { + minimum_confirmations: Some(10), + ..Default::default() + }, + ..Default::default() + }, + )?; + Ok(()) + }, + )?; + assert_eq!(slate.state, SlateState::Standard2); + assert_eq!(slate.tx_or_err()?.inputs().len(), 0); + + wallet::controller::owner_single_use( + send_wallet, + send_mask, + PathBuf::from(test_dir), + |api, m| { + let args = |minimum| ContractSetupArgsAPI { + selection_args: OutputSelectionArgs { + minimum_confirmations: minimum, + ..Default::default() + }, + ..Default::default() + }; + let err = api.contract_sign(m, &slate, &args(Some(12))).unwrap_err(); + assert!(matches!( + err, + libwallet::Error::GenericError(ref msg) + if msg == "Can't change minimum confirmations after contract setup. setup:10, current:12" + )); + slate = api.contract_sign(m, &slate, &args(None))?; + Ok(()) + }, + )?; + assert_eq!(slate.state, SlateState::Standard3); + + stopper.store(false, Ordering::Relaxed); + thread::sleep(Duration::from_millis(200)); + clean_output_dir(test_dir); + Ok(()) +} diff --git a/controller/tests/contract_srs.rs b/controller/tests/contract_srs.rs index 10bcb7e..e50b3d5 100644 --- a/controller/tests/contract_srs.rs +++ b/controller/tests/contract_srs.rs @@ -53,6 +53,7 @@ fn contract_srs_tx_impl(test_dir: &'static str) -> Result<(), libwallet::Error> // Send wallet inititates a standard transaction with --send=5 let args = &mut ContractNewArgsAPI { setup_args: ContractSetupArgsAPI { + selection_args: common::contract_selection_args(), net_change: Some(-5_000_000_000), ..Default::default() }, @@ -71,6 +72,7 @@ fn contract_srs_tx_impl(test_dir: &'static str) -> Result<(), libwallet::Error> |api, m| { // Receive wallet calls --receive=5 let args = &mut ContractSetupArgsAPI { + selection_args: common::contract_selection_args(), net_change: Some(5_000_000_000), ..Default::default() }; @@ -252,6 +254,7 @@ fn contract_srs_tx_impl(test_dir: &'static str) -> Result<(), libwallet::Error> PathBuf::from(test_dir), |api, m| { let args = &mut ContractSetupArgsAPI { + selection_args: common::contract_selection_args(), ..Default::default() }; slate = api.contract_sign(m, &slate, args)?; diff --git a/controller/tests/contract_srs_mwmixnet.rs b/controller/tests/contract_srs_mwmixnet.rs index 1f8c5cb..8de3ec6 100644 --- a/controller/tests/contract_srs_mwmixnet.rs +++ b/controller/tests/contract_srs_mwmixnet.rs @@ -53,6 +53,7 @@ fn contract_srs_mwixnet_tx_impl(test_dir: &'static str) -> Result<(), libwallet: // Send wallet inititates a standard transaction with --send=5 let args = &mut ContractNewArgsAPI { setup_args: ContractSetupArgsAPI { + selection_args: common::contract_selection_args(), net_change: Some(-5_000_000_000), ..Default::default() }, @@ -71,6 +72,7 @@ fn contract_srs_mwixnet_tx_impl(test_dir: &'static str) -> Result<(), libwallet: |api, m| { // Receive wallet calls --receive=5 let args = &mut ContractSetupArgsAPI { + selection_args: common::contract_selection_args(), net_change: Some(5_000_000_000), ..Default::default() }; @@ -88,6 +90,7 @@ fn contract_srs_mwixnet_tx_impl(test_dir: &'static str) -> Result<(), libwallet: PathBuf::from(test_dir), |api, m| { let args = &mut ContractSetupArgsAPI { + selection_args: common::contract_selection_args(), ..Default::default() }; args.proof_args.suppress_proof = true; diff --git a/doc/contracts.md b/doc/contracts.md index d7b35ec..079e137 100644 --- a/doc/contracts.md +++ b/doc/contracts.md @@ -33,9 +33,11 @@ the receiver from adding an input and cannot be used together with `--use-inputs Inputs and outputs are normally picked when signing. `--add-outputs` picks and locks them during `new`, but they are still only added to the slate when signing. `--use-inputs` and -`--make-outputs` limit the selection but do not lock anything early. Open contracts do not -reserve funds against each other, so a late-locked contract can fail when another -transaction has spent the available outputs first. +`--make-outputs` limit the selection but do not lock anything early. Each party chooses +`--min_conf` when it first joins and cannot change it later. The same limit is used for the fee +estimate, so any required inputs need enough confirmations at that point. A value of `0` allows +unconfirmed non-coinbase outputs. Open contracts do not reserve funds, so a late-locked contract +can fail if another transaction spends the available outputs first. ## View and revoke diff --git a/libwallet/src/contract/actions/README.md b/libwallet/src/contract/actions/README.md index a928622..b9faa82 100644 --- a/libwallet/src/contract/actions/README.md +++ b/libwallet/src/contract/actions/README.md @@ -24,9 +24,10 @@ return slate; `save_step` writes the context, tx log, outputs and input locks in one LMDB batch. The signed transaction is stored separately outside LMDB and cannot share that batch. -Custom input and output selection is only accepted during setup. `add_outputs` selects and -locks them during setup; otherwise this happens during signing. In either case they are -only added to the slate when signing, so the counterparty does not see them earlier. +Input and output choices are made during setup. The confirmation limit is stored in the local +context because it is also used for the fee estimate. `add_outputs` selects and locks inputs +during setup; otherwise this happens during signing. The inputs and outputs are only added to +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 @@ -36,16 +37,17 @@ Separating side effects until the 'save_step' part would make these functions mu #### TODOs - - Honour `min_input_confirmation`; contract input selection currently uses one confirmation. - Decide whether `--use-inputs` or `--make-outputs` should imply early locking; currently only `--add-outputs` locks during `new`. - Reserve funds across open late-locked contracts, or make the possible signing failure clearer to callers. - Handle the coinbase flows. Coinbase inputs are built into the slate, but nothing else is exercised. + - Keep contracts at two participants until the known multi-party attack is addressed. - For a payjoin, `--use-inputs any` picks an input for us; naming the commitment would be 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. @@ -65,19 +67,17 @@ Separating side effects until the 'save_step' part would make these functions mu - 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. + - Decide whether the foreign JSON-RPC API should expose `new` and `sign`. - Add full API and RPC examples once the contract interface is stable. - Separate side effects out from the computation, as described above. + - Store transactions in LMDB so they can be committed with the wallet state. - Decide whether `contract_accounts_switch.rs` is a legitimate scenario, or whether signing under a different account than setup should error. #### Tests - signing the same slate twice, which `verify_not_signed` guards - - zero-value outputs, and `--make-outputs` beyond what is available - - a receiver with no spendable input, through lack of funds or of confirmations - invalid or inconsistent slates and unexpected states - - the CLI conflict between `--no-payjoin` and `--use-inputs` - - 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 diff --git a/libwallet/src/contract/actions/revoke.rs b/libwallet/src/contract/actions/revoke.rs index ce9cf82..cb39b56 100644 --- a/libwallet/src/contract/actions/revoke.rs +++ b/libwallet/src/contract/actions/revoke.rs @@ -131,6 +131,8 @@ where num_participants: 1, add_outputs: false, selection_args: OutputSelectionArgs { + // Keep revoke at one confirmation + minimum_confirmations: Some(1), use_inputs: Some(String::from(input_commit)), ..Default::default() }, @@ -148,6 +150,7 @@ where num_participants: 1, add_outputs: false, selection_args: OutputSelectionArgs { + minimum_confirmations: Some(1), use_inputs: Some(String::from(input_commit)), ..Default::default() }, diff --git a/libwallet/src/contract/actions/setup.rs b/libwallet/src/contract/actions/setup.rs index d744a16..ddb8213 100644 --- a/libwallet/src/contract/actions/setup.rs +++ b/libwallet/src/contract/actions/setup.rs @@ -65,12 +65,13 @@ where let mut sl = slate.clone(); check_ttl(w, &sl)?; - // Get or create a transaction Context and verify consistency of setup arguments + // 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)?; - contract::utils::verify_setup_args_consistency( - &context.setup_args.as_ref().unwrap(), - &setup_args, - )?; + let context_args = context + .setup_args + .as_ref() + .ok_or_else(|| Error::GenericError("Context carries no contract setup args".to_string()))?; + contract::utils::verify_setup_args_consistency(context_args, setup_args)?; // Add keys and payment proof to slate (both are idempotent operations) contract::slate::add_keys(&mut sl, &w.keychain(keychain_mask)?, &mut context)?; diff --git a/libwallet/src/contract/actions/sign.rs b/libwallet/src/contract/actions/sign.rs index cdc0fb1..fa0f2d3 100644 --- a/libwallet/src/contract/actions/sign.rs +++ b/libwallet/src/contract/actions/sign.rs @@ -44,7 +44,7 @@ where }; let will_add_outputs = existing_context .as_ref() - .map(|context| context.get_inputs().len() + context.get_outputs().len() == 0) + .map(|context| !context.has_inputs_or_outputs()) .unwrap_or(true); // Compute state for 'sign' let (sl, mut context) = compute( diff --git a/libwallet/src/contract/context.rs b/libwallet/src/contract/context.rs index ca1b2cf..e913217 100644 --- a/libwallet/src/contract/context.rs +++ b/libwallet/src/contract/context.rs @@ -150,7 +150,7 @@ where debug!("contract::utils::add_outputs => called"); // Do nothing if we have already contributed our outputs. The assumption is that if this was done, // our output contribution is complete. - if context.output_ids.len() > 0 || context.input_ids.len() > 0 { + if context.has_inputs_or_outputs() { debug!("contract::utils::add_outputs => outputs have already been added, returning."); return Ok(()); } diff --git a/libwallet/src/contract/selection.rs b/libwallet/src/contract/selection.rs index 7003cec..ec9dbee 100644 --- a/libwallet/src/contract/selection.rs +++ b/libwallet/src/contract/selection.rs @@ -36,16 +36,22 @@ where K: Keychain, { // Find available inputs - let mut eligible_inputs = find_eligible(w, parent_key_id, current_height)?; + let mut eligible_inputs = find_eligible( + w, + parent_key_id, + current_height, + setup_args.selection_args.effective_minimum_confirmations(), + )?; // Select which inputs to use to satisfy the equation compute(setup_args, committed_fee, &mut eligible_inputs) } /// Find all inputs eligible to spend pub fn find_eligible( - w: &mut WalletBackend, + w: &WalletBackend, parent_key_id: &Identifier, current_height: u64, + minimum_confirmations: u64, ) -> Result, Error> where C: NodeClient, @@ -54,7 +60,10 @@ where // Find eligible inputs in the wallet let eligible_inputs = w .iter()? - .filter(|out| out.root_key_id == *parent_key_id && out.eligible_to_spend(current_height, 1)) + .filter(|out| { + out.root_key_id == *parent_key_id + && out.eligible_to_spend(current_height, minimum_confirmations) + }) .collect::>(); Ok(eligible_inputs) } @@ -331,16 +340,28 @@ pub fn verify_selection_consistency( ctx_args: &OutputSelectionArgs, cur_args: &OutputSelectionArgs, ) -> Result<(), Error> { - // We can't define a selection strategy if we've already done the setup phase. We only allow to pass either the - // default or exactly the same strategy we defined when doing the setup phase. - if cur_args != ctx_args && cur_args != &OutputSelectionArgs::default() { + // Input and output choices are fixed during setup + let strategy_changed = cur_args.use_inputs != ctx_args.use_inputs + || cur_args.make_outputs != ctx_args.make_outputs; + let defaults = OutputSelectionArgs::default(); + let default_strategy = cur_args.use_inputs == defaults.use_inputs + && cur_args.make_outputs == defaults.make_outputs; + if strategy_changed && !default_strategy { return Err(Error::GenericError(format!( "Can't define selection args now because we've already done the setup phase. ctx_selection_args:{:#?}, cur_selection_args:{:#?}", ctx_args, cur_args ))); } - // NOTE: The logic above isn't perfect. This is because the user could define arguments that are the default. In this case - // we'd simply silently use the arguments provided in the setup phase. This could be confusing for the user. + if let Some(minimum) = cur_args.minimum_confirmations { + if minimum != ctx_args.effective_minimum_confirmations() { + return Err(Error::GenericError(format!( + "Can't change minimum confirmations after contract setup. setup:{}, current:{}", + ctx_args.effective_minimum_confirmations(), + minimum + ))); + } + } + // An explicit default cannot be distinguished from omitted arguments Ok(()) } @@ -430,6 +451,16 @@ mod tests { }; let mut inputs = _create_output_data_for(vec![u64::MAX, 1]); assert!(compute(&setup_args, None, &mut inputs).is_err()); + + let setup_args = ContractSetupArgsAPI { + net_change: Some(0), + selection_args: OutputSelectionArgs { + make_outputs: Some(vec![u64::MAX, 1]), + ..Default::default() + }, + ..Default::default() + }; + assert!(compute(&setup_args, None, &mut vec![]).is_err()); } #[test] @@ -579,6 +610,34 @@ mod tests { ); } + #[test] + fn receiver_without_input() { + let setup_args = ContractSetupArgsAPI { + net_change: Some(3_000_000_000), + ..Default::default() + }; + let expected_fee = my_fee_contribution(0, 1, 1, 2).unwrap(); + let result = compute(&setup_args, None, &mut vec![]).unwrap(); + + assert_eq!( + result, + ( + vec![], + vec![3_000_000_000 - expected_fee.fee()], + expected_fee, + ) + ); + + let setup_args = ContractSetupArgsAPI { + net_change: Some(expected_fee.fee() as i64 - 1), + ..Default::default() + }; + assert!(matches!( + compute(&setup_args, None, &mut vec![]), + Err(Error::NotEnoughFunds { .. }) + )); + } + #[test] fn sender_use_inputs_ok() { let setup_args = ContractSetupArgsAPI { @@ -676,6 +735,50 @@ mod tests { ); } + #[test] + fn custom_output_limits() { + let setup_args = ContractSetupArgsAPI { + net_change: Some(-1_000_000_000), + selection_args: OutputSelectionArgs { + make_outputs: Some(vec![0]), + ..Default::default() + }, + ..Default::default() + }; + let mut inputs = _create_output_data_for(vec![2_000_000_000]); + let (_, outputs, _) = compute(&setup_args, None, &mut inputs).unwrap(); + assert_eq!(outputs[0], 0); + + let setup_args = ContractSetupArgsAPI { + net_change: Some(-1_000_000_000), + selection_args: OutputSelectionArgs { + make_outputs: Some(vec![2_000_000_000]), + ..Default::default() + }, + ..Default::default() + }; + let mut inputs = _create_output_data_for(vec![2_000_000_000]); + assert!(matches!( + compute(&setup_args, None, &mut inputs), + Err(Error::NotEnoughFunds { .. }) + )); + } + + #[test] + fn committed_fee_too_low() { + let setup_args = ContractSetupArgsAPI { + net_change: Some(-3_000_000_000), + ..Default::default() + }; + let committed_fee = my_fee_contribution(1, 1, 1, 2).unwrap(); + let mut inputs = _create_output_data_for(vec![2_000_000_000, 2_000_000_000]); + + assert!(matches!( + compute(&setup_args, Some(committed_fee), &mut inputs), + Err(Error::Fee(_)) + )); + } + #[test] fn selection_args_consistency() { // Selection args agreed at the setup phase (stored in the Context). @@ -693,25 +796,26 @@ mod tests { ..Default::default() }; assert!(verify_selection_consistency(&ctx_args, &changed).is_err()); + + let changed_confirmations = OutputSelectionArgs { + minimum_confirmations: Some(3), + use_inputs: ctx_args.use_inputs.clone(), + ..Default::default() + }; + assert!(verify_selection_consistency(&ctx_args, &changed_confirmations).is_err()); + + let omitted_confirmations = OutputSelectionArgs { + minimum_confirmations: None, + use_inputs: ctx_args.use_inputs.clone(), + ..Default::default() + }; + assert!(verify_selection_consistency(&ctx_args, &omitted_confirmations).is_ok()); + + let same_confirmations = OutputSelectionArgs { + minimum_confirmations: Some(ctx_args.effective_minimum_confirmations()), + use_inputs: ctx_args.use_inputs.clone(), + ..Default::default() + }; + assert!(verify_selection_consistency(&ctx_args, &same_confirmations).is_ok()); } - - /* - - Tests to add: - - compute_receiver_invariant - test that receiving_amount - my_fees >= 0 (to prevent going into negative accidentally) - - compute_sender_invariant - test that -send_amount - my_fees < 0 (do you need this one and is it correct?) - - compute_receiver_payjoin_negative_fee - could the receiver receive a negative amount through fees? but the thing - would go through because they made a payjoin so they could pay for the fees? - - compute_receiver_omit_payjoin - we can't contribute an input, but have enough for other fees - - compute_make_outputs_fee_err - fail due to not enough funds for fees - - compute_make_outputs_sum_err - is this even possible? - - compute_zero_value_outputs_sender - sender uses all 0-value outputs when sending - - compute_zero_value_inputs_receiver - receiver uses 0-value inputs in payjoin - - test_fee_committed_err - we have already committed to a certain fee which we no longer satisfy - - think if we should have sender/receiver separate testing - - sender_use_all_features - - coinbase output cases - - validate --make-outputs has all positive u64 numbers - - */ } diff --git a/libwallet/src/contract/types.rs b/libwallet/src/contract/types.rs index 2b16f12..ee549a1 100644 --- a/libwallet/src/contract/types.rs +++ b/libwallet/src/contract/types.rs @@ -19,11 +19,15 @@ pub use crate::slate::PaymentProofType as ProofType; use crate::slate_versions::ser as dalek_ser; use ed25519_dalek::VerifyingKey as DalekPublicKey; +/// Default confirmations for contract inputs +/// Keep in sync with contract new in grin-wallet.yml +pub const DEFAULT_MINIMUM_CONFIRMATIONS: u64 = 10; + /// Output selection args #[derive(Serialize, Deserialize, Clone, Debug, PartialEq)] pub struct OutputSelectionArgs { - /// Constraint on how many confirmations used inputs must have - pub min_input_confirmation: u64, + /// Required confirmations, or None to use the stored value or default + pub minimum_confirmations: Option, /// Which inputs we want to use - default to payjoin if available with Some("any") pub use_inputs: Option, /// Change output specification: explicit output amounts in nanogrin, not including @@ -32,6 +36,12 @@ pub struct OutputSelectionArgs { } impl OutputSelectionArgs { + /// Number of confirmations required when selecting inputs + pub fn effective_minimum_confirmations(&self) -> u64 { + self.minimum_confirmations + .unwrap_or(DEFAULT_MINIMUM_CONFIRMATIONS) + } + /// We try to make a payjoin if use_inputs has a value (either commitments or Some("any")) pub fn is_payjoin(&self) -> bool { self.use_inputs.is_some() @@ -70,7 +80,7 @@ impl OutputSelectionArgs { impl Default for OutputSelectionArgs { fn default() -> OutputSelectionArgs { OutputSelectionArgs { - min_input_confirmation: 10, + minimum_confirmations: None, use_inputs: Some(String::from("any")), make_outputs: None, } @@ -230,6 +240,28 @@ pub struct ContractRevokeArgsAPI { mod tests { use super::*; + #[test] + fn selection_args_json() { + let explicit: OutputSelectionArgs = serde_json::from_value(serde_json::json!({ + "minimum_confirmations": 3, + "use_inputs": "any", + "make_outputs": null + })) + .unwrap(); + assert_eq!(explicit.minimum_confirmations, Some(3)); + + let omitted: OutputSelectionArgs = serde_json::from_value(serde_json::json!({ + "use_inputs": "any", + "make_outputs": null + })) + .unwrap(); + assert_eq!(omitted.minimum_confirmations, None); + assert_eq!( + omitted.effective_minimum_confirmations(), + DEFAULT_MINIMUM_CONFIRMATIONS + ); + } + #[test] fn own_commitment_status_json() { for (status, value) in [ diff --git a/libwallet/src/types.rs b/libwallet/src/types.rs index ffd90a6..23cf08a 100644 --- a/libwallet/src/types.rs +++ b/libwallet/src/types.rs @@ -494,6 +494,11 @@ impl Context { self.input_ids.clone() } + /// Whether inputs or outputs have already been selected for this context + pub fn has_inputs_or_outputs(&self) -> bool { + !self.input_ids.is_empty() || !self.output_ids.is_empty() + } + /// Returns private key, private nonce pub fn get_private_keys(&self) -> (SecretKey, SecretKey) { (self.sec_key.clone(), self.sec_nonce.clone()) diff --git a/src/bin/grin-wallet.yml b/src/bin/grin-wallet.yml index fc0fe26..cba2312 100644 --- a/src/bin/grin-wallet.yml +++ b/src/bin/grin-wallet.yml @@ -497,6 +497,12 @@ subcommands: short: o long: make-outputs takes_value: true + - minimum_confirmations: + help: Minimum confirmations required for an output; 0 allows unconfirmed outputs + short: c + long: min_conf + default_value: "10" + takes_value: true - sign: about: Sign a contract args: @@ -535,6 +541,11 @@ subcommands: short: o long: make-outputs takes_value: true + - minimum_confirmations: + help: Minimum confirmations when first joining the contract; 0 allows unconfirmed outputs + short: c + long: min_conf + takes_value: true - no-broadcast: help: Don't broadcast the transaction if it was finalized long: no-broadcast diff --git a/src/cmd/wallet_args.rs b/src/cmd/wallet_args.rs index d8fd001..7d7836e 100644 --- a/src/cmd/wallet_args.rs +++ b/src/cmd/wallet_args.rs @@ -1028,6 +1028,10 @@ pub fn parse_contract_new_args( ))); }; let src_acct_name = Some(String::from(account)); + let minimum_confirmations = parse_u64( + parse_required(args, "minimum_confirmations")?, + "minimum_confirmations", + )?; let add_outputs = args.is_present("add-outputs"); let as_json = args.is_present("as-json"); let no_payjoin = args.is_present("no-payjoin"); @@ -1044,7 +1048,7 @@ pub fn parse_contract_new_args( if no_payjoin { None } else { - // Some("any") means pick 1 random input to contribute (payjoin) + // Some("any") means pick an available input to contribute (payjoin) Some(String::from("any")) } } @@ -1079,6 +1083,7 @@ pub fn parse_contract_new_args( add_outputs: add_outputs, use_inputs: use_inputs, make_outputs: make_outputs, + minimum_confirmations, // Not yet wired to CLI flags: fee_rate: None, outfile: None, @@ -1107,6 +1112,10 @@ pub fn parse_contract_setup_args( ))); }; let as_json = args.is_present("as-json"); + let minimum_confirmations = match args.value_of("minimum_confirmations") { + Some(value) => Some(parse_u64(value, "minimum_confirmations")?), + None => None, + }; let no_payjoin = args.is_present("no-payjoin"); let use_inputs = match args.value_of("use-inputs") { Some(v) => { @@ -1121,7 +1130,7 @@ pub fn parse_contract_setup_args( if no_payjoin { None } else { - // Some("any") means pick 1 random input to contribute (payjoin) + // Some("any") means pick an available input to contribute (payjoin) Some(String::from("any")) } } @@ -1148,6 +1157,7 @@ pub fn parse_contract_setup_args( add_outputs: false, use_inputs: use_inputs, make_outputs: make_outputs, + minimum_confirmations, // Not yet wired to CLI flags: fee_rate: None, outfile: None, diff --git a/tests/cmd_line_contract.rs b/tests/cmd_line_contract.rs index 67c5caf..a043c6b 100644 --- a/tests/cmd_line_contract.rs +++ b/tests/cmd_line_contract.rs @@ -112,6 +112,8 @@ fn contract_command_test_impl(test_dir: &str) -> Result<(), grin_wallet_controll "new", "--send", "1", + "--min_conf", + "1", ]; execute_command(&app, test_dir, "wallet1", &client1, arg_vec)?; @@ -165,3 +167,93 @@ fn wallet_contract_command_line() { } clean_output_dir(test_dir); } + +#[test] +fn rejects_conflicting_input_options() { + let yml = load_yaml!("../src/bin/grin-wallet.yml"); + let app = App::from_yaml(yml); + let account = "default".to_string(); + let args = app + .clone() + .get_matches_from_safe(vec![ + "grin-wallet", + "contract", + "new", + "--send", + "1", + "--no-payjoin", + "--use-inputs", + "commitment", + ]) + .unwrap(); + let contract = args.subcommand_matches("contract").unwrap(); + let new_args = contract.subcommand_matches("new").unwrap(); + + assert!(grin_wallet::cmd::wallet_args::parse_contract_new_args(new_args, &account).is_err()); + + let args = app + .get_matches_from_safe(vec![ + "grin-wallet", + "contract", + "sign", + "--no-payjoin", + "--use-inputs", + "commitment", + ]) + .unwrap(); + let contract = args.subcommand_matches("contract").unwrap(); + let sign_args = contract.subcommand_matches("sign").unwrap(); + assert!(grin_wallet::cmd::wallet_args::parse_contract_setup_args(sign_args).is_err()); +} + +#[test] +fn parses_contract_min_confirmations() { + let yml = load_yaml!("../src/bin/grin-wallet.yml"); + let app = App::from_yaml(yml); + let account = "default".to_string(); + let args = app + .clone() + .get_matches_from_safe(vec![ + "grin-wallet", + "contract", + "new", + "--send", + "1", + "--min_conf", + "3", + ]) + .unwrap(); + let contract = args.subcommand_matches("contract").unwrap(); + let new_args = contract.subcommand_matches("new").unwrap(); + let parsed = + grin_wallet::cmd::wallet_args::parse_contract_new_args(new_args, &account).unwrap(); + assert_eq!(parsed.minimum_confirmations, 3); + + let expected = grin_wallet_libwallet::contract::types::DEFAULT_MINIMUM_CONFIRMATIONS; + let args = app + .clone() + .get_matches_from_safe(vec!["grin-wallet", "contract", "new", "--send", "1"]) + .unwrap(); + let contract = args.subcommand_matches("contract").unwrap(); + let new_args = contract.subcommand_matches("new").unwrap(); + let parsed = + grin_wallet::cmd::wallet_args::parse_contract_new_args(new_args, &account).unwrap(); + assert_eq!(parsed.minimum_confirmations, expected); + + let args = app + .clone() + .get_matches_from_safe(vec!["grin-wallet", "contract", "sign"]) + .unwrap(); + let contract = args.subcommand_matches("contract").unwrap(); + let sign_args = contract.subcommand_matches("sign").unwrap(); + let parsed = grin_wallet::cmd::wallet_args::parse_contract_setup_args(sign_args).unwrap(); + assert_eq!(parsed.minimum_confirmations, None); + + let args = app + .get_matches_from_safe(vec!["grin-wallet", "contract", "sign", "--min_conf", "3"]) + .unwrap(); + let contract = args.subcommand_matches("contract").unwrap(); + let sign_args = contract.subcommand_matches("sign").unwrap(); + let parsed = grin_wallet::cmd::wallet_args::parse_contract_setup_args(sign_args).unwrap(); + assert_eq!(parsed.minimum_confirmations, Some(3)); +}