From 64a90192d9d49c3df22d2b5f489ba20222fd941a Mon Sep 17 00:00:00 2001 From: vmammal Date: Sat, 28 Oct 2023 22:32:32 -0400 Subject: [PATCH 1/6] refactor: remove old clippy allow attributes These lints either resolved themselves, or the code has changed such that they no longer apply, hence they can be removed with no further changes. `clippy::derivable_impls` `clippy::needless_collect` `clippy::almost_swapped` --- crates/bdk/src/wallet/mod.rs | 3 --- crates/bdk/src/wallet/signer.rs | 1 - example-crates/example_cli/src/lib.rs | 2 -- 3 files changed, 6 deletions(-) diff --git a/crates/bdk/src/wallet/mod.rs b/crates/bdk/src/wallet/mod.rs index deb7d626e..0e4270876 100644 --- a/crates/bdk/src/wallet/mod.rs +++ b/crates/bdk/src/wallet/mod.rs @@ -2290,9 +2290,6 @@ impl Wallet { ) -> Result<(), MiniscriptPsbtError> { // We need to borrow `psbt` mutably within the loops, so we have to allocate a vec for all // the input utxos and outputs - // - // Clippy complains that the collect is not required, but that's wrong - #[allow(clippy::needless_collect)] let utxos = (0..psbt.inputs.len()) .filter_map(|i| psbt.get_utxo_for(i).map(|utxo| (true, i, utxo))) .chain( diff --git a/crates/bdk/src/wallet/signer.rs b/crates/bdk/src/wallet/signer.rs index 2fe5804ce..63784a3fd 100644 --- a/crates/bdk/src/wallet/signer.rs +++ b/crates/bdk/src/wallet/signer.rs @@ -820,7 +820,6 @@ pub enum TapLeavesOptions { None, } -#[allow(clippy::derivable_impls)] impl Default for SignOptions { fn default() -> Self { SignOptions { diff --git a/example-crates/example_cli/src/lib.rs b/example-crates/example_cli/src/lib.rs index 2d4f0d701..5d460fef2 100644 --- a/example-crates/example_cli/src/lib.rs +++ b/example-crates/example_cli/src/lib.rs @@ -53,7 +53,6 @@ pub struct Args { pub command: Commands, } -#[allow(clippy::almost_swapped)] #[derive(Subcommand, Debug, Clone)] pub enum Commands { #[clap(flatten)] @@ -137,7 +136,6 @@ impl core::fmt::Display for CoinSelectionAlgo { } } -#[allow(clippy::almost_swapped)] #[derive(Subcommand, Debug, Clone)] pub enum AddressCmd { /// Get the next unused address. From 4679ca1df7209d2b356fbb8cbd675f07756d1301 Mon Sep 17 00:00:00 2001 From: vmammal Date: Sat, 28 Oct 2023 22:53:37 -0400 Subject: [PATCH 2/6] ref(example_cli): add typedefs to reduce type complexity - Add typedefs to model the result of functions `planned_utxos` and `init` - Add new struct `CreateTxChange` to hold any change info resulting from `create_tx` These changes help resolve clippy::type_complexity --- example-crates/example_cli/src/lib.rs | 78 +++++++++++++++------------ 1 file changed, 45 insertions(+), 33 deletions(-) diff --git a/example-crates/example_cli/src/lib.rs b/example-crates/example_cli/src/lib.rs index 5d460fef2..4b3ece51f 100644 --- a/example-crates/example_cli/src/lib.rs +++ b/example-crates/example_cli/src/lib.rs @@ -188,7 +188,12 @@ impl core::fmt::Display for Keychain { } } -#[allow(clippy::type_complexity)] +pub struct CreateTxChange { + pub index_changeset: keychain::ChangeSet, + pub change_keychain: Keychain, + pub index: u32, +} + pub fn create_tx( graph: &mut KeychainTxGraph, chain: &O, @@ -196,10 +201,7 @@ pub fn create_tx( cs_algorithm: CoinSelectionAlgo, address: Address, value: u64, -) -> anyhow::Result<( - Transaction, - Option<(keychain::ChangeSet, (Keychain, u32))>, -)> +) -> anyhow::Result<(Transaction, Option)> where O::Error: std::error::Error + Send + Sync + 'static, { @@ -391,7 +393,11 @@ where } let change_info = if selection_meta.drain_value.is_some() { - Some((changeset, (internal_keychain, change_index))) + Some(CreateTxChange { + index_changeset: changeset, + change_keychain: internal_keychain, + index: change_index, + }) } else { None }; @@ -399,35 +405,34 @@ where Ok((transaction, change_info)) } -#[allow(clippy::type_complexity)] +// Alias the elements of `Result` of `planned_utxos` +pub type PlannedUtxo = (bdk_tmp_plan::Plan, FullTxOut); + pub fn planned_utxos( graph: &KeychainTxGraph, chain: &O, assets: &bdk_tmp_plan::Assets, -) -> Result, FullTxOut)>, O::Error> { +) -> Result>, O::Error> { let chain_tip = chain.get_chain_tip()?; let outpoints = graph.index.outpoints().iter().cloned(); graph .graph() .try_filter_chain_unspents(chain, chain_tip, outpoints) - .filter_map( - #[allow(clippy::type_complexity)] - |r| -> Option, FullTxOut), _>> { - let (k, i, full_txo) = match r { - Err(err) => return Some(Err(err)), - Ok(((k, i), full_txo)) => (k, i, full_txo), - }; - let desc = graph - .index - .keychains() - .get(&k) - .expect("keychain must exist") - .at_derivation_index(i) - .expect("i can't be hardened"); - let plan = bdk_tmp_plan::plan_satisfaction(&desc, assets)?; - Some(Ok((plan, full_txo))) - }, - ) + .filter_map(|r| -> Option, _>> { + let (k, i, full_txo) = match r { + Err(err) => return Some(Err(err)), + Ok(((k, i), full_txo)) => (k, i, full_txo), + }; + let desc = graph + .index + .keychains() + .get(&k) + .expect("keychain must exist") + .at_derivation_index(i) + .expect("i can't be hardened"); + let plan = bdk_tmp_plan::plan_satisfaction(&desc, assets)?; + Some(Ok((plan, full_txo))) + }) .collect() } @@ -597,7 +602,12 @@ where let (tx, change_info) = create_tx(graph, chain, keymap, coin_select, address, value)?; - if let Some((index_changeset, (change_keychain, index))) = change_info { + if let Some(CreateTxChange { + index_changeset, + change_keychain, + index, + }) = change_info + { // We must first persist to disk the fact that we've got a new address from the // change keychain so future scans will find the tx we're about to broadcast. // If we're unable to persist this, then we don't want to broadcast. @@ -646,17 +656,19 @@ where } } -#[allow(clippy::type_complexity)] -pub fn init( - db_magic: &[u8], - db_default_path: &str, -) -> anyhow::Result<( +// Alias the `Result` of `init` +pub type InitialState = ( Args, KeyMap, KeychainTxOutIndex, Mutex>, C, -)> +); + +pub fn init( + db_magic: &[u8], + db_default_path: &str, +) -> anyhow::Result> where C: Default + Append + Serialize + DeserializeOwned, { From f11d663b7efb98dd72fed903ade8c5e7af0b5a3a Mon Sep 17 00:00:00 2001 From: vmammal Date: Sat, 28 Oct 2023 23:12:22 -0400 Subject: [PATCH 3/6] ref(psbt): refactor body of `get_utxo_for` to address `clippy::manual_map` --- crates/bdk/src/psbt/mod.rs | 22 +++++++--------------- 1 file changed, 7 insertions(+), 15 deletions(-) diff --git a/crates/bdk/src/psbt/mod.rs b/crates/bdk/src/psbt/mod.rs index bc6ce8589..796b8618b 100644 --- a/crates/bdk/src/psbt/mod.rs +++ b/crates/bdk/src/psbt/mod.rs @@ -35,24 +35,16 @@ pub trait PsbtUtils { } impl PsbtUtils for Psbt { - #[allow(clippy::all)] // We want to allow `manual_map` but it is too new. fn get_utxo_for(&self, input_index: usize) -> Option { let tx = &self.unsigned_tx; + let input = self.inputs.get(input_index)?; - if input_index >= tx.input.len() { - return None; - } - - if let Some(input) = self.inputs.get(input_index) { - if let Some(wit_utxo) = &input.witness_utxo { - Some(wit_utxo.clone()) - } else if let Some(in_tx) = &input.non_witness_utxo { - Some(in_tx.output[tx.input[input_index].previous_output.vout as usize].clone()) - } else { - None - } - } else { - None + match (&input.witness_utxo, &input.non_witness_utxo) { + (Some(_), _) => input.witness_utxo.clone(), + (_, Some(_)) => input.non_witness_utxo.as_ref().map(|in_tx| { + in_tx.output[tx.input[input_index].previous_output.vout as usize].clone() + }), + _ => None, } } From 097d818d4c7fff12d72ce0ef10ceb9b575c154e1 Mon Sep 17 00:00:00 2001 From: vmammal Date: Fri, 10 Nov 2023 10:54:56 -0500 Subject: [PATCH 4/6] ref(wallet): `Wallet::preselect_utxos` now accepts a `&TxParams` to reduce the number of required function args in order to satisfy `clippy::too_many_arguments` --- crates/bdk/src/wallet/mod.rs | 38 +++++++++++++++++++----------------- 1 file changed, 20 insertions(+), 18 deletions(-) diff --git a/crates/bdk/src/wallet/mod.rs b/crates/bdk/src/wallet/mod.rs index 0e4270876..c6c922d55 100644 --- a/crates/bdk/src/wallet/mod.rs +++ b/crates/bdk/src/wallet/mod.rs @@ -12,7 +12,7 @@ //! Wallet //! //! This module defines the [`Wallet`]. -use crate::collections::{BTreeMap, HashMap, HashSet}; +use crate::collections::{BTreeMap, HashMap}; use alloc::{ boxed::Box, string::{String, ToString}, @@ -1518,15 +1518,8 @@ impl Wallet { return Err(CreateTxError::ChangePolicyDescriptor); } - let (required_utxos, optional_utxos) = self.preselect_utxos( - params.change_policy, - ¶ms.unspendable, - params.utxos.clone(), - params.drain_wallet, - params.manually_selected_only, - params.bumping_fee.is_some(), // we mandate confirmed transactions if we're bumping the fee - Some(current_height.to_consensus_u32()), - ); + let (required_utxos, optional_utxos) = + self.preselect_utxos(¶ms, Some(current_height.to_consensus_u32())); // get drain script let drain_script = match params.drain_to { @@ -2063,17 +2056,26 @@ impl Wallet { /// Given the options returns the list of utxos that must be used to form the /// transaction and any further that may be used if needed. - #[allow(clippy::too_many_arguments)] fn preselect_utxos( &self, - change_policy: tx_builder::ChangeSpendPolicy, - unspendable: &HashSet, - manually_selected: Vec, - must_use_all_available: bool, - manual_only: bool, - must_only_use_confirmed_tx: bool, + params: &TxParams, current_height: Option, ) -> (Vec, Vec) { + let TxParams { + change_policy, + unspendable, + utxos, + drain_wallet, + manually_selected_only, + bumping_fee, + .. + } = params; + + let manually_selected = utxos.clone(); + // we mandate confirmed transactions if we're bumping the fee + let must_only_use_confirmed_tx = bumping_fee.is_some(); + let must_use_all_available = *drain_wallet; + let chain_tip = self.chain.tip().block_id(); // must_spend <- manually selected utxos // may_spend <- all other available utxos @@ -2088,7 +2090,7 @@ impl Wallet { // NOTE: we are intentionally ignoring `unspendable` here. i.e manual // selection overrides unspendable. - if manual_only { + if *manually_selected_only { return (must_spend, vec![]); } From 89a7ddca7f6ed8c65ed0124774f3a868d08faf68 Mon Sep 17 00:00:00 2001 From: vmammal Date: Sun, 29 Oct 2023 01:02:31 -0400 Subject: [PATCH 5/6] ref(esplora): `Box` a large `esplora_client::Error` to address `clippy::result_large_err`. Clippy's default large-error- threshold is 128. `esplora_client::Error` currently has size 272. --- crates/esplora/src/async_ext.rs | 8 ++++---- crates/esplora/src/blocking_ext.rs | 15 ++++++++++----- 2 files changed, 14 insertions(+), 9 deletions(-) diff --git a/crates/esplora/src/async_ext.rs b/crates/esplora/src/async_ext.rs index 8e697a2a9..fddedf3e2 100644 --- a/crates/esplora/src/async_ext.rs +++ b/crates/esplora/src/async_ext.rs @@ -6,11 +6,14 @@ use bdk_chain::{ local_chain::{self, CheckPoint}, BlockId, ConfirmationTimeHeightAnchor, TxGraph, }; -use esplora_client::{Error, TxStatus}; +use esplora_client::TxStatus; use futures::{stream::FuturesOrdered, TryStreamExt}; use crate::anchor_from_status; +/// [`esplora_client::Error`] +type Error = Box; + /// Trait to extend the functionality of [`esplora_client::AsyncClient`]. /// /// Refer to [crate-level documentation] for more. @@ -35,7 +38,6 @@ pub trait EsploraAsyncExt { /// [`LocalChain`]: bdk_chain::local_chain::LocalChain /// [`LocalChain::tip`]: bdk_chain::local_chain::LocalChain::tip /// [`LocalChain::apply_update`]: bdk_chain::local_chain::LocalChain::apply_update - #[allow(clippy::result_large_err)] async fn update_local_chain( &self, local_tip: CheckPoint, @@ -50,7 +52,6 @@ pub trait EsploraAsyncExt { /// The full scan for each keychain stops after a gap of `stop_gap` script pubkeys with no associated /// transactions. `parallel_requests` specifies the max number of HTTP requests to make in /// parallel. - #[allow(clippy::result_large_err)] async fn full_scan( &self, keychain_spks: BTreeMap< @@ -73,7 +74,6 @@ pub trait EsploraAsyncExt { /// may include scripts that have been used, use [`full_scan`] with the keychain. /// /// [`full_scan`]: EsploraAsyncExt::full_scan - #[allow(clippy::result_large_err)] async fn sync( &self, misc_spks: impl IntoIterator + Send> + Send, diff --git a/crates/esplora/src/blocking_ext.rs b/crates/esplora/src/blocking_ext.rs index a7ee290b0..8e53ce218 100644 --- a/crates/esplora/src/blocking_ext.rs +++ b/crates/esplora/src/blocking_ext.rs @@ -7,10 +7,13 @@ use bdk_chain::{ local_chain::{self, CheckPoint}, BlockId, ConfirmationTimeHeightAnchor, TxGraph, }; -use esplora_client::{Error, TxStatus}; +use esplora_client::TxStatus; use crate::anchor_from_status; +/// [`esplora_client::Error`] +type Error = Box; + /// Trait to extend the functionality of [`esplora_client::BlockingClient`]. /// /// Refer to [crate-level documentation] for more. @@ -33,7 +36,6 @@ pub trait EsploraExt { /// [`LocalChain`]: bdk_chain::local_chain::LocalChain /// [`LocalChain::tip`]: bdk_chain::local_chain::LocalChain::tip /// [`LocalChain::apply_update`]: bdk_chain::local_chain::LocalChain::apply_update - #[allow(clippy::result_large_err)] fn update_local_chain( &self, local_tip: CheckPoint, @@ -48,7 +50,6 @@ pub trait EsploraExt { /// The full scan for each keychain stops after a gap of `stop_gap` script pubkeys with no associated /// transactions. `parallel_requests` specifies the max number of HTTP requests to make in /// parallel. - #[allow(clippy::result_large_err)] fn full_scan( &self, keychain_spks: BTreeMap>, @@ -68,7 +69,6 @@ pub trait EsploraExt { /// may include scripts that have been used, use [`full_scan`] with the keychain. /// /// [`full_scan`]: EsploraExt::full_scan - #[allow(clippy::result_large_err)] fn sync( &self, misc_spks: impl IntoIterator, @@ -247,7 +247,12 @@ impl EsploraExt for esplora_client::BlockingClient { .map(|txid| { std::thread::spawn({ let client = self.clone(); - move || client.get_tx_status(&txid).map(|s| (txid, s)) + move || { + client + .get_tx_status(&txid) + .map_err(Box::new) + .map(|s| (txid, s)) + } }) }) .collect::>>>(); From 1c15cb2f9169bb08a4128c183eeeca80fff97ab7 Mon Sep 17 00:00:00 2001 From: vmammal Date: Tue, 30 Jan 2024 17:56:51 -0500 Subject: [PATCH 6/6] ref(example_cli): Add new struct Init for holding the items returned from `example_cli::init` --- .../example_bitcoind_rpc_polling/src/main.rs | 10 ++++-- example-crates/example_cli/src/lib.rs | 31 ++++++++++++------- example-crates/example_electrum/src/main.rs | 11 +++++-- example-crates/example_esplora/src/main.rs | 9 ++++-- 4 files changed, 42 insertions(+), 19 deletions(-) diff --git a/example-crates/example_bitcoind_rpc_polling/src/main.rs b/example-crates/example_bitcoind_rpc_polling/src/main.rs index 49fce4d9e..88b83067b 100644 --- a/example-crates/example_bitcoind_rpc_polling/src/main.rs +++ b/example-crates/example_bitcoind_rpc_polling/src/main.rs @@ -110,9 +110,13 @@ enum RpcCommands { fn main() -> anyhow::Result<()> { let start = Instant::now(); - - let (args, keymap, index, db, init_changeset) = - example_cli::init::(DB_MAGIC, DB_PATH)?; + let example_cli::Init { + args, + keymap, + index, + db, + init_changeset, + } = example_cli::init::(DB_MAGIC, DB_PATH)?; println!( "[{:>10}s] loaded initial changeset from db", start.elapsed().as_secs_f32() diff --git a/example-crates/example_cli/src/lib.rs b/example-crates/example_cli/src/lib.rs index 4b3ece51f..4989c08c6 100644 --- a/example-crates/example_cli/src/lib.rs +++ b/example-crates/example_cli/src/lib.rs @@ -656,19 +656,26 @@ where } } -// Alias the `Result` of `init` -pub type InitialState = ( - Args, - KeyMap, - KeychainTxOutIndex, - Mutex>, - C, -); +/// The initial state returned by [`init`]. +pub struct Init { + /// Arguments parsed by the cli. + pub args: Args, + /// Descriptor keymap. + pub keymap: KeyMap, + /// Keychain-txout index. + pub index: KeychainTxOutIndex, + /// Persistence backend. + pub db: Mutex>, + /// Initial changeset. + pub init_changeset: C, +} +/// Parses command line arguments and initializes all components, creating +/// a file store with the given parameters, or loading one if it exists. pub fn init( db_magic: &[u8], db_default_path: &str, -) -> anyhow::Result> +) -> anyhow::Result> where C: Default + Append + Serialize + DeserializeOwned, { @@ -702,11 +709,11 @@ where let init_changeset = db_backend.load_from_persistence()?.unwrap_or_default(); - Ok(( + Ok(Init { args, keymap, index, - Mutex::new(Database::new(db_backend)), + db: Mutex::new(Database::new(db_backend)), init_changeset, - )) + }) } diff --git a/example-crates/example_electrum/src/main.rs b/example-crates/example_electrum/src/main.rs index ccd17a704..df34795bd 100644 --- a/example-crates/example_electrum/src/main.rs +++ b/example-crates/example_electrum/src/main.rs @@ -103,8 +103,15 @@ type ChangeSet = ( ); fn main() -> anyhow::Result<()> { - let (args, keymap, index, db, (disk_local_chain, disk_tx_graph)) = - example_cli::init::(DB_MAGIC, DB_PATH)?; + let example_cli::Init { + args, + keymap, + index, + db, + init_changeset, + } = example_cli::init::(DB_MAGIC, DB_PATH)?; + + let (disk_local_chain, disk_tx_graph) = init_changeset; let graph = Mutex::new({ let mut graph = IndexedTxGraph::new(index); diff --git a/example-crates/example_esplora/src/main.rs b/example-crates/example_esplora/src/main.rs index b4dabea7a..e92205706 100644 --- a/example-crates/example_esplora/src/main.rs +++ b/example-crates/example_esplora/src/main.rs @@ -99,8 +99,13 @@ pub struct ScanOptions { } fn main() -> anyhow::Result<()> { - let (args, keymap, index, db, init_changeset) = - example_cli::init::(DB_MAGIC, DB_PATH)?; + let example_cli::Init { + args, + keymap, + index, + db, + init_changeset, + } = example_cli::init::(DB_MAGIC, DB_PATH)?; let genesis_hash = genesis_block(args.network).block_hash();