From 6465f070df591f7cfd50e199fa1d8c7e4e11999c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ad=C3=A1n=20SDPC?= Date: Tue, 11 Jul 2023 18:11:12 +0200 Subject: [PATCH 1/3] fix(wallet): allow generating transactions without persisting change addresses --- .../actors/app/handlers/create_data_req.rs | 8 ++++- wallet/src/actors/app/handlers/create_vtt.rs | 3 ++ wallet/src/actors/worker/methods.rs | 2 +- wallet/src/repository/wallet/mod.rs | 32 +++++++++++++++---- wallet/src/types.rs | 2 ++ 5 files changed, 38 insertions(+), 9 deletions(-) diff --git a/wallet/src/actors/app/handlers/create_data_req.rs b/wallet/src/actors/app/handlers/create_data_req.rs index 77581f838..6b71bcd0f 100644 --- a/wallet/src/actors/app/handlers/create_data_req.rs +++ b/wallet/src/actors/app/handlers/create_data_req.rs @@ -33,6 +33,8 @@ pub struct CreateDataReqRequest { #[serde(deserialize_with = "deserialize_fee_backwards_compatible")] fee: Fee, fee_type: Option, + #[serde(default)] + preview: bool, } #[derive(Debug, Serialize, Deserialize)] @@ -81,7 +83,11 @@ impl Handler for app::App { let fee = fee_compat(msg.fee, msg.fee_type); let f = fut::result(validated).and_then(move |request, slf: &mut Self, _ctx| { - let params = types::DataReqParams { request, fee }; + let params = types::DataReqParams { + request, + fee, + preview: msg.preview, + }; slf.create_data_req(&msg.session_id, &msg.wallet_id, params) .map_ok( diff --git a/wallet/src/actors/app/handlers/create_vtt.rs b/wallet/src/actors/app/handlers/create_vtt.rs index 3b9d673c6..1c87ccf61 100644 --- a/wallet/src/actors/app/handlers/create_vtt.rs +++ b/wallet/src/actors/app/handlers/create_vtt.rs @@ -65,6 +65,8 @@ pub struct CreateVttRequest { utxo_strategy: UtxoSelectionStrategy, #[serde(default)] selected_utxos: HashSet, + #[serde(default)] + preview: bool, } /// Part of CreateVttResponse struct, containing additional data to be displayed in clients @@ -124,6 +126,7 @@ impl Handler for app::App { outputs, utxo_strategy: msg.utxo_strategy.clone(), selected_utxos: msg.selected_utxos.iter().map(|x| x.into()).collect(), + preview: msg.preview, }; act.create_vtt(&msg.session_id, &msg.wallet_id, params) diff --git a/wallet/src/actors/worker/methods.rs b/wallet/src/actors/worker/methods.rs index 15193d76c..23b5e9a22 100644 --- a/wallet/src/actors/worker/methods.rs +++ b/wallet/src/actors/worker/methods.rs @@ -352,7 +352,7 @@ impl Worker { let address = if external { wallet.gen_external_address(label)? } else { - wallet.gen_internal_address(label)? + wallet.gen_internal_address(label, false)? }; Ok(address) diff --git a/wallet/src/repository/wallet/mod.rs b/wallet/src/repository/wallet/mod.rs index e9862ef3e..fe77d7dc2 100644 --- a/wallet/src/repository/wallet/mod.rs +++ b/wallet/src/repository/wallet/mod.rs @@ -501,10 +501,14 @@ where } /// Generate an address in the internal keychain (WIP-0001). - pub fn gen_internal_address(&self, label: Option) -> Result> { + pub fn gen_internal_address( + &self, + label: Option, + preview: bool, + ) -> Result> { let mut state = self.state.write()?; - self._gen_internal_address(&mut state, label) + self._gen_internal_address(&mut state, label, preview) } /// Return a list of the generated external addresses that. @@ -1082,6 +1086,7 @@ where outputs, utxo_strategy, selected_utxos, + preview, }: types::VttParams, ) -> Result<(model::ExtendedTransaction, AbsoluteFee)> { let mut state = self.state.write()?; @@ -1095,6 +1100,7 @@ where fee, &utxo_strategy, selected_utxos, + preview, )?; let pointers_as_inputs = inputs @@ -1118,14 +1124,18 @@ where pub fn create_data_req( &self, - types::DataReqParams { fee, request }: types::DataReqParams, + types::DataReqParams { + fee, + request, + preview, + }: types::DataReqParams, ) -> Result<(model::ExtendedTransaction, AbsoluteFee)> { let mut state = self.state.write()?; let TransactionComponents { fee, inputs, outputs, - } = self.create_dr_transaction_components(&mut state, request.clone(), fee)?; + } = self.create_dr_transaction_components(&mut state, request.clone(), fee, preview)?; let pointers_as_inputs = inputs .pointers @@ -1190,6 +1200,7 @@ where fee: Fee, utxo_strategy: &UtxoSelectionStrategy, selected_utxos: HashSet, + preview: bool, ) -> Result { let timestamp = u64::try_from(get_timestamp()).unwrap(); @@ -1203,6 +1214,7 @@ where utxo_strategy, self.params.max_vt_weight, selected_utxos, + preview, ) } @@ -1211,6 +1223,7 @@ where state: &mut State, request: DataRequestOutput, fee: Fee, + preview: bool, ) -> Result { let utxo_strategy = UtxoSelectionStrategy::Random { from: None }; let timestamp = u64::try_from(get_timestamp()).unwrap(); @@ -1225,6 +1238,7 @@ where &utxo_strategy, self.params.max_dr_weight, HashSet::default(), + preview, ) } @@ -1233,6 +1247,7 @@ where &self, state: &mut State, force_input: Option, + preview: bool, ) -> Result { let pkh = if let Some(input) = force_input { let forced = input.output_pointer(); @@ -1240,7 +1255,7 @@ where key_balance.pkh } else { - self._gen_internal_address(state, None)?.pkh + self._gen_internal_address(state, None, preview)?.pkh }; Ok(pkh) @@ -1263,6 +1278,7 @@ where utxo_strategy: &UtxoSelectionStrategy, max_weight: u32, selected_utxos: HashSet, + preview: bool, ) -> Result { let empty_hashset = HashSet::default(); let unconfirmed_transactions = if self.params.use_unconfirmed_utxos { @@ -1298,6 +1314,7 @@ where let change_pkh = self.calculate_change_address( state, dr_output.and_then(|_| inputs.pointers.get(0).cloned().map(Input::new)), + preview, )?; let mut outputs = outputs; @@ -1318,6 +1335,7 @@ where &self, state: &mut State, label: Option, + preview: bool, ) -> Result> { let keychain = constants::INTERNAL_KEYCHAIN; let account = state.account; @@ -1325,7 +1343,7 @@ where let parent_key = &state.keychains[keychain as usize]; let (address, next_index) = - self.derive_and_persist_address(label, parent_key, account, keychain, index, true)?; + self.derive_and_persist_address(label, parent_key, account, keychain, index, !preview)?; state.next_internal_index = next_index; @@ -1569,7 +1587,7 @@ where state.transient_external_addresses.remove(&addr.pkh); } for _ in state.next_internal_index..new_internal_index { - let addr = self._gen_internal_address(&mut state, None)?; + let addr = self._gen_internal_address(&mut state, None, false)?; state.transient_internal_addresses.remove(&addr.pkh); } diff --git a/wallet/src/types.rs b/wallet/src/types.rs index d3917c17d..84c9a9513 100644 --- a/wallet/src/types.rs +++ b/wallet/src/types.rs @@ -149,11 +149,13 @@ pub struct VttParams { pub outputs: Vec, pub utxo_strategy: UtxoSelectionStrategy, pub selected_utxos: HashSet, + pub preview: bool, } pub struct DataReqParams { pub fee: Fee, pub request: DataRequestOutput, + pub preview: bool, } #[derive(Debug, PartialEq, Eq)] From 0443509d088e7c5e23694280bc12d4cf35597b33 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ad=C3=A1n=20SDPC?= Date: Wed, 12 Jul 2023 12:19:23 +0200 Subject: [PATCH 2/3] tests(wallet): adapt tests to preview mode --- wallet/src/repository/wallet/tests/mod.rs | 81 +++++++++++++++++------ 1 file changed, 59 insertions(+), 22 deletions(-) diff --git a/wallet/src/repository/wallet/tests/mod.rs b/wallet/src/repository/wallet/tests/mod.rs index fab35d896..dd8ea949c 100644 --- a/wallet/src/repository/wallet/tests/mod.rs +++ b/wallet/src/repository/wallet/tests/mod.rs @@ -132,9 +132,9 @@ fn test_gen_external_address_associates_pkh_to_account_in_db() { fn test_list_internal_addresses() { let (wallet, _db) = factories::wallet(None); - let address1 = (*wallet.gen_internal_address(None).unwrap()).clone(); - let address2 = (*wallet.gen_internal_address(None).unwrap()).clone(); - let address3 = (*wallet.gen_internal_address(None).unwrap()).clone(); + let address1 = (*wallet.gen_internal_address(None, false).unwrap()).clone(); + let address2 = (*wallet.gen_internal_address(None, false).unwrap()).clone(); + let address3 = (*wallet.gen_internal_address(None, false).unwrap()).clone(); let offset = 0; let limit = 10; @@ -150,9 +150,9 @@ fn test_list_internal_addresses() { fn test_list_internal_addresses_paginated() { let (wallet, _db) = factories::wallet(None); - let _ = wallet.gen_internal_address(None).unwrap(); - let address = (*wallet.gen_internal_address(None).unwrap()).clone(); - let _ = wallet.gen_internal_address(None).unwrap(); + let _ = wallet.gen_internal_address(None, false).unwrap(); + let address = (*wallet.gen_internal_address(None, false).unwrap()).clone(); + let _ = wallet.gen_internal_address(None, false).unwrap(); let offset = 1; let limit = 1; @@ -185,13 +185,15 @@ fn test_get_address() { fn test_gen_internal_address() { let (wallet, _db) = factories::wallet(None); let label = "address label".to_string(); - let address = wallet.gen_internal_address(Some(label.clone())).unwrap(); + let address = wallet + .gen_internal_address(Some(label.clone()), false) + .unwrap(); assert!(address.address.starts_with("wit")); assert_eq!("m/3'/4919'/0'/1/0", &address.path); assert_eq!(Some(label), address.info.label); - let address_no_label = wallet.gen_internal_address(None).unwrap(); + let address_no_label = wallet.gen_internal_address(None, false).unwrap(); assert_eq!(None, address_no_label.info.label); } @@ -199,12 +201,12 @@ fn test_gen_internal_address() { #[test] fn test_gen_internal_address_creates_different_addresses() { let (wallet, _db) = factories::wallet(None); - let address = wallet.gen_internal_address(None).unwrap(); + let address = wallet.gen_internal_address(None, false).unwrap(); assert_eq!("m/3'/4919'/0'/1/0", &address.path); assert_eq!(0, address.index); - let new_address = wallet.gen_internal_address(None).unwrap(); + let new_address = wallet.gen_internal_address(None, false).unwrap(); assert_eq!("m/3'/4919'/0'/1/1", &new_address.path); assert_eq!(1, new_address.index); @@ -215,7 +217,7 @@ fn test_gen_internal_address_stores_next_address_index_in_db() { let (wallet, db) = factories::wallet(None); let account = 0; let keychain = constants::INTERNAL_KEYCHAIN; - wallet.gen_internal_address(None).unwrap(); + wallet.gen_internal_address(None, false).unwrap(); assert_eq!( 1, @@ -223,7 +225,7 @@ fn test_gen_internal_address_stores_next_address_index_in_db() { .unwrap() ); - wallet.gen_internal_address(None).unwrap(); + wallet.gen_internal_address(None, false).unwrap(); assert_eq!( 2, @@ -239,7 +241,9 @@ fn test_gen_internal_address_saves_details_in_db() { let keychain = constants::INTERNAL_KEYCHAIN; let index = 0; let label = "address label".to_string(); - let address = wallet.gen_internal_address(Some(label.clone())).unwrap(); + let address = wallet + .gen_internal_address(Some(label.clone()), false) + .unwrap(); assert_eq!( address.address, @@ -269,7 +273,7 @@ fn test_gen_internal_address_associates_pkh_to_account_in_db() { let (wallet, db) = factories::wallet(None); let account = 0; let keychain = constants::INTERNAL_KEYCHAIN; - let address = wallet.gen_internal_address(None).unwrap(); + let address = wallet.gen_internal_address(None, false).unwrap(); let pkh = &address.pkh; let path: model::Path = db.get(&keys::pkh(pkh)).unwrap(); @@ -342,6 +346,7 @@ fn test_create_transaction_components_when_wallet_have_no_utxos() { fee, &utxo_strategy, HashSet::default(), + false, ) .unwrap_err(); @@ -398,6 +403,7 @@ fn test_create_transaction_components_without_a_change_address() { fee, &utxo_strategy, HashSet::default(), + false, ) .unwrap(); @@ -454,6 +460,7 @@ fn test_create_transaction_components_with_a_change_address() { fee, &utxo_strategy, HashSet::default(), + false, ) .unwrap(); @@ -525,6 +532,7 @@ fn test_create_transaction_components_which_value_overflows() { fee, &utxo_strategy, HashSet::default(), + false, ) .unwrap_err(); @@ -589,6 +597,7 @@ fn test_create_vtt_does_not_spend_utxos() { }], utxo_strategy, selected_utxos: HashSet::default(), + preview: false, }) .unwrap(); @@ -674,7 +683,11 @@ fn test_create_data_request_does_not_spend_utxos() { let fee = Fee::absolute_from_nanowits(123); let (extended, _) = wallet - .create_data_req(types::DataReqParams { fee, request }) + .create_data_req(types::DataReqParams { + fee, + request, + preview: false, + }) .unwrap(); // the extended transaction should actually contain a data request transaction @@ -1015,6 +1028,7 @@ fn test_index_transaction_vtt_created_by_wallet() { }], utxo_strategy: UtxoSelectionStrategy::Random { from: None }, selected_utxos: HashSet::default(), + preview: false, }) .unwrap(); @@ -1138,6 +1152,7 @@ fn test_get_transaction() { }], utxo_strategy: UtxoSelectionStrategy::Random { from: None }, selected_utxos: HashSet::default(), + preview: false, }) .unwrap(); @@ -1220,6 +1235,7 @@ fn test_get_transactions() { }], utxo_strategy: UtxoSelectionStrategy::Random { from: None }, selected_utxos: HashSet::default(), + preview: false, }) .unwrap(); @@ -1301,6 +1317,7 @@ fn test_create_vtt_with_locked_balance() { }], utxo_strategy: UtxoSelectionStrategy::Random { from: None }, selected_utxos: HashSet::default(), + preview: false, }) .unwrap_err(); @@ -1364,6 +1381,7 @@ fn test_create_vtt_with_multiple_outputs() { ], utxo_strategy: UtxoSelectionStrategy::Random { from: None }, selected_utxos: HashSet::default(), + preview: false, }) .unwrap(); @@ -1461,6 +1479,7 @@ fn test_create_vt_components_weighted_fee() { fee, &utxo_strategy, HashSet::default(), + false, ) .unwrap(); @@ -1533,6 +1552,7 @@ fn test_create_vt_components_weighted_fee_2() { fee, &utxo_strategy, HashSet::default(), + false, ) .unwrap(); @@ -1600,6 +1620,7 @@ fn test_create_vt_components_weighted_fee_3() { fee, &utxo_strategy, HashSet::default(), + false, ) .unwrap_err(); @@ -1692,6 +1713,7 @@ fn test_create_vt_components_weighted_fee_4() { fee, &utxo_strategy, HashSet::default(), + false, ) .unwrap(); @@ -1781,6 +1803,7 @@ fn test_create_vt_components_weighted_fee_5() { fee, &utxo_strategy, HashSet::default(), + false, ) .unwrap(); @@ -1876,6 +1899,7 @@ fn test_create_vt_components_weighted_fee_6() { fee, &utxo_strategy, HashSet::default(), + false, ) .unwrap(); @@ -1920,6 +1944,7 @@ fn test_create_vt_components_weighted_fee_without_outputs() { fee, &utxo_strategy, HashSet::default(), + false, ) .unwrap_err(); @@ -1977,6 +2002,7 @@ fn test_create_vt_components_weighted_fee_with_too_large_fee() { fee, &utxo_strategy, HashSet::default(), + false, ) .unwrap_err(); @@ -2039,6 +2065,7 @@ fn test_create_vt_weight_too_large() { fee, &utxo_strategy, HashSet::default(), + false, ) .unwrap_err(); @@ -2090,7 +2117,7 @@ fn test_create_dr_components_weighted_fee_1() { let mut state = wallet.state.write().unwrap(); let fee = Fee::relative_from_float(1.0); let TransactionComponents { inputs, .. } = wallet - .create_dr_transaction_components(&mut state, request, fee) + .create_dr_transaction_components(&mut state, request, fee, false) .unwrap(); assert_eq!(inputs.pointers.len(), 1); @@ -2137,7 +2164,7 @@ fn test_create_dr_components_weighted_fee_2_not_enough_funds() { let mut state = wallet.state.write().unwrap(); let fee = Fee::relative_from_float(1.0); let err = wallet - .create_dr_transaction_components(&mut state, request, fee) + .create_dr_transaction_components(&mut state, request, fee, false) .unwrap_err(); assert!( @@ -2188,7 +2215,7 @@ fn test_create_dr_components_weighted_fee_3_funds_splitted() { let fee = Fee::relative_from_float(1.0); let TransactionComponents { inputs, .. } = wallet - .create_dr_transaction_components(&mut state, request.clone(), fee) + .create_dr_transaction_components(&mut state, request.clone(), fee, false) .unwrap(); let weight = calculate_weight(inputs.pointers.len(), 1, Some(&request), u32::MAX).unwrap(); @@ -2245,7 +2272,7 @@ fn test_create_dr_components_weighted_fee_3_funds_splitted() { let mut state_2 = wallet_2.state.write().unwrap(); let TransactionComponents { inputs, .. } = wallet_2 - .create_dr_transaction_components(&mut state_2, request, fee) + .create_dr_transaction_components(&mut state_2, request, fee, false) .unwrap(); assert_eq!(inputs.pointers.len(), 3); @@ -2280,7 +2307,7 @@ fn test_create_dr_components_weighted_fee_without_outputs() { let fee = Fee::relative_from_float(1.0); let err = wallet - .create_dr_transaction_components(&mut state, request, fee) + .create_dr_transaction_components(&mut state, request, fee, false) .unwrap_err(); assert!( @@ -2333,7 +2360,7 @@ fn test_create_dr_components_weighted_fee_weight_too_large() { }; let fee = Fee::relative_from_float(0); let err = wallet - .create_dr_transaction_components(&mut state, request.clone(), fee) + .create_dr_transaction_components(&mut state, request.clone(), fee, false) .unwrap_err(); assert_eq!( @@ -2387,7 +2414,7 @@ fn test_create_dr_components_weighted_fee_fee_too_large() { let fee = Fee::relative_from_float(f64::MAX / 2.0); let err = wallet - .create_dr_transaction_components(&mut state, request, fee) + .create_dr_transaction_components(&mut state, request, fee, false) .unwrap_err(); assert_eq!( @@ -2461,6 +2488,7 @@ fn test_create_transaction_components_filter_from_address() { fee, &utxo_strategy, HashSet::default(), + false, ) .unwrap(); @@ -2533,6 +2561,7 @@ fn test_create_transaction_components_filter_from_address_2() { fee, &utxo_strategy, HashSet::default(), + false, ) .unwrap(); @@ -2605,6 +2634,7 @@ fn test_create_transaction_components_filter_from_address_3() { fee, &utxo_strategy, HashSet::default(), + false, ) .unwrap_err(); @@ -2683,6 +2713,7 @@ fn test_create_transaction_components_does_not_use_unconfirmed_utxos() { fee, &utxo_strategy, HashSet::default(), + false, ) .unwrap_err(); @@ -2708,6 +2739,7 @@ fn test_create_transaction_components_does_not_use_unconfirmed_utxos() { fee, &utxo_strategy, HashSet::default(), + false, ) .unwrap(); @@ -2786,6 +2818,7 @@ fn test_create_transaction_components_uses_unconfirmed_utxos() { fee, &utxo_strategy, HashSet::default(), + false, ) .unwrap(); @@ -2856,6 +2889,7 @@ fn test_create_vtt_selecting_utxos() { fee, &utxo_strategy, vec![out_pointer_0].into_iter().collect(), + false, ) .unwrap_err(); @@ -2875,6 +2909,7 @@ fn test_create_vtt_selecting_utxos() { fee, &utxo_strategy, vec![out_pointer_1].into_iter().collect(), + false, ) .unwrap(); @@ -2893,6 +2928,7 @@ fn test_create_vtt_selecting_utxos() { fee, &utxo_strategy, HashSet::default(), + false, ) .unwrap(); @@ -2956,6 +2992,7 @@ fn test_create_transaction_components_does_not_use_unconfirmed_utxos_and_selecti fee, &utxo_strategy, vec![pending_outptr].into_iter().collect(), + false, ) .unwrap_err(); From bd6c1302c057065e70089848132185cfd2466cd0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ad=C3=A1n=20SDPC?= Date: Thu, 27 Jul 2023 11:14:40 +0200 Subject: [PATCH 3/3] fix(wallet): advance internal keychain when needed i.e. when processing a transaction that uses the next internal address --- wallet/src/actors/app/routes.rs | 2 +- wallet/src/repository/wallet/mod.rs | 19 ++++++++++++++++++- 2 files changed, 19 insertions(+), 2 deletions(-) diff --git a/wallet/src/actors/app/routes.rs b/wallet/src/actors/app/routes.rs index fb211e81f..f902c0e77 100644 --- a/wallet/src/actors/app/routes.rs +++ b/wallet/src/actors/app/routes.rs @@ -15,7 +15,7 @@ macro_rules! routes { { let api_addr = $api.clone(); $io.add_method($method_jsonrpc, move |params: Params| { - log::debug!("Handling request for method: {}", $method_jsonrpc); + log::debug!("Handling request for method {}: {:?}", $method_jsonrpc, params); let addr = api_addr.clone(); // Try to parse the request params into the actor message let fut03 = future::ready(params.parse::<$actor_msg>()) diff --git a/wallet/src/repository/wallet/mod.rs b/wallet/src/repository/wallet/mod.rs index fe77d7dc2..6e401c586 100644 --- a/wallet/src/repository/wallet/mod.rs +++ b/wallet/src/repository/wallet/mod.rs @@ -1342,10 +1342,15 @@ where let index = state.next_internal_index; let parent_key = &state.keychains[keychain as usize]; + // `preview` is negated because it turns into `persist_db` let (address, next_index) = self.derive_and_persist_address(label, parent_key, account, keychain, index, !preview)?; - state.next_internal_index = next_index; + // Don't advance the internal index if we are simply previewing + if !preview { + state.next_internal_index = next_index; + log::debug!("Internal keychain advanced to index #{next_index}"); + } Ok(address) } @@ -1461,6 +1466,18 @@ where // - Cannot borrow `state` as mutable because it is also borrowed as immutable let state = &mut *state; + // Move internal keychain forward if we used a new change address + let next = self._gen_internal_address(state, None, true)?; + if match &txn.transaction { + Transaction::ValueTransfer(vtt) => { + vtt.body.outputs.iter().any(|vto| vto.pkh == next.pkh) + } + Transaction::DataRequest(dr) => dr.body.outputs.iter().any(|vto| vto.pkh == next.pkh), + _ => false, + } { + let _ = self._gen_internal_address(state, None, false); + } + // Mark UTXOs as used so we don't double spend // Save the timestamp to after which the UTXO can be spent again let tx_pending_timeout = self.params.pending_transactions_timeout_seconds;