Add docs + initial review

src/input.rs

@@ -7,7 +7,7 @@ use miniscript::bitcoin;
 use miniscript::bitcoin::{OutPoint, Transaction, TxOut};
 use miniscript::plan::Plan;
 
-/// Confirmation status of a tx.
+/// Confirmation status of a tx input.
 #[derive(Debug, Clone, Copy)]
 pub struct InputStatus {
     /// Confirmation block height.
@@ -20,10 +20,11 @@ pub struct InputStatus {
 }
 
 impl InputStatus {
-    /// Helper method.
+    /// From consensus `height` and `time`.
     pub fn new(height: u32, time: u64) -> Result<Self, absolute::ConversionError> {
         Ok(Self {
             height: absolute::Height::from_consensus(height)?,
+            // TODO: handle `.try_into::<u32>()`
             time: absolute::Time::from_consensus(time as _)?,
         })
     }
@@ -46,7 +47,7 @@ impl PlanOrPsbtInput {
     /// TODO: Check whether satisfaction_weight calculations are correct.
     /// TODO: Return error type: invalid outpoint, no `witness_utxo` or `non_witness_utxo`, etc.
     ///
-    /// # WHy do we only support finalized psbt inputs?
+    /// # Why do we only support finalized psbt inputs?
     ///
     /// There is no mulit-party tx building protocol that requires choosing from foreign,
     /// non-finalized PSBT inputs.
@@ -139,9 +140,12 @@ pub struct Input {
 }
 
 impl Input {
-    /// Create
+    /// Create [`Input`] from a previous transaction.
     ///
-    /// Returns `None` if `prev_output_index` does not exist in `prev_tx`.
+    /// # Errors
+    ///
+    /// Returns `OutputsIndexError` if the previous txout is not found in `prev_tx`
+    /// at `output_index`.
     pub fn from_prev_tx<T>(
         plan: Plan,
         prev_tx: T,
@@ -163,7 +167,7 @@ impl Input {
         })
     }
 
-    /// Create
+    /// Create [`Input`] from a previous txout and plan.
     pub fn from_prev_txout(
         plan: Plan,
         prev_outpoint: OutPoint,
@@ -181,9 +185,9 @@ impl Input {
         }
     }
 
-    /// Create
+    /// Create [`Input`] from a [`psbt::Input`].
     ///
-    /// TODO: Return error type: out of bounds, etc.
+    // TODO: Return error type: out of bounds, etc.
     pub fn from_psbt_input(
         prev_outpoint: OutPoint,
         sequence: Sequence,
@@ -207,6 +211,8 @@ impl Input {
             prev_tx,
             plan,
             status,
+            // TODO: maybe add `is_coinbase` parameter, or try to infer from the psbt
+            // input whether the input is coinbase.
             is_coinbase: false,
         })
     }
@@ -332,9 +338,10 @@ impl Input {
         self.plan.sequence()
     }
 
-    /// In weight units.
+    /// The weight in witness units needed for satisfying the [`Input`].
     ///
-    /// TODO: Describe what fields are actually included in this calculation.
+    /// The satisfaction weight is the combined size of the fully satisfied input's witness
+    /// and scriptSig expressed in weight units. See <https://en.bitcoin.it/wiki/Weight_units>.
     pub fn satisfaction_weight(&self) -> u64 {
         self.plan
             .satisfaction_weight()

src/input_candidates.rs

@@ -274,6 +274,7 @@ impl<PF: Debug> Display for PolicyFailure<PF> {
         match self {
             PolicyFailure::MissingOutputs(missing_outputs) => Debug::fmt(missing_outputs, f),
             PolicyFailure::PolicyFailure(failure) => {
+                // Review: should not use fmt::Debug on Display
                 write!(f, "policy failure: {:?}", failure)
             }
         }

src/output.rs

@@ -3,12 +3,12 @@ use miniscript::bitcoin;
 
 use crate::DefiniteDescriptor;
 
-/// Can get script pubkey from this.
+/// Source of the output script pubkey
 #[derive(Debug, Clone)]
 pub enum ScriptSource {
-    /// From ScriptBuf.
+    /// bitcoin script
     Script(ScriptBuf),
-    /// From definite descriptor.
+    /// definite descriptor
     Descriptor(DefiniteDescriptor),
 }
 
@@ -38,7 +38,7 @@ impl ScriptSource {
     /// To ScriptBuf
     pub fn script(&self) -> ScriptBuf {
         match self {
-            ScriptSource::Script(script_buf) => script_buf.clone(),
+            ScriptSource::Script(spk) => spk.clone(),
             ScriptSource::Descriptor(descriptor) => descriptor.script_pubkey(),
         }
     }

src/rbf.rs

@@ -91,6 +91,7 @@ impl RbfSet {
                 tx.input
                     .iter()
                     .map(|txin| txin.previous_output)
+                    // Review: needless collect?
                     .collect::<Vec<_>>()
             })
             .collect::<HashSet<OutPoint>>();

src/selection.rs

@@ -2,7 +2,7 @@ use core::fmt::{Debug, Display};
 use std::vec::Vec;
 
 use bdk_coin_select::FeeRate;
-use bitcoin::{absolute, transaction};
+use bitcoin::{absolute, transaction, Sequence};
 use miniscript::bitcoin;
 use miniscript::psbt::PsbtExt;
 
@@ -36,10 +36,13 @@ pub struct PsbtParams {
     /// It is best practive to set this to the latest block height to avoid fee sniping.
     pub fallback_locktime: absolute::LockTime,
 
-    /// Yes.
-    pub fallback_sequence: bitcoin::Sequence,
+    /// [`Sequence`] value to use by default if not provided by the input.
+    pub fallback_sequence: Sequence,
 
-    /// Recommended.
+    /// Whether to require the full tx, aka [`non_witness_utxo`] for segwit v0 inputs,
+    /// default is `true`.
+    ///
+    /// [`non_witness_utxo`]: bitcoin::psbt::Input::non_witness_utxo
     pub mandate_full_tx_for_segwit_v0: bool,
 }
 
@@ -97,7 +100,7 @@ impl std::error::Error for CreatePsbtError {}
 impl Selection {
     /// Returns none if there is a mismatch of units in `locktimes`.
     ///
-    /// As according to BIP-64...
+    // TODO: As according to BIP-64... ?
     fn _accumulate_max_locktime(
         locktimes: impl IntoIterator<Item = absolute::LockTime>,
         fallback: absolute::LockTime,
@@ -138,7 +141,6 @@ impl Selection {
                 .iter()
                 .map(|input| bitcoin::TxIn {
                     previous_output: input.prev_outpoint(),
-                    // TODO: Custom fallback sequence?
                     sequence: input.sequence().unwrap_or(params.fallback_sequence),
                     ..Default::default()
                 })

src/selector.rs

@@ -8,7 +8,7 @@ use miniscript::bitcoin;
 use crate::{cs_feerate, DefiniteDescriptor, InputCandidates, InputGroup, Output, Selection};
 use alloc::vec::Vec;
 
-/// You seeing this?
+/// A coin selector
 #[derive(Debug, Clone)]
 pub struct Selector<'c> {
     candidates: &'c InputCandidates,
@@ -82,7 +82,8 @@ pub struct RbfParams {
     pub incremental_relay_feerate: FeeRate,
 }
 
-/// TODO: Make this more flexible.
+/// Change policy type
+// TODO: Make this more flexible.
 #[derive(Debug, Clone, Copy)]
 pub enum ChangePolicyType {
     /// Avoid creating dust change output.
@@ -95,13 +96,9 @@ pub enum ChangePolicyType {
 }
 
 impl OriginalTxStats {
-    /// Feerate.
-    ///
-    /// TODO: Make sure this is correct with the rounding.
+    /// Return the [`FeeRate`] of the original tx.
     pub fn feerate(&self) -> FeeRate {
-        FeeRate::from_sat_per_vb_unchecked(
-            ((self.fee.to_sat() as f32) / (self.weight.to_vbytes_ceil() as f32)) as _,
-        )
+        self.fee / self.weight
     }
 }
 
@@ -120,11 +117,10 @@ impl RbfParams {
 
     /// To coin select `Replace` params.
     pub fn to_cs_replace(&self) -> Replace {
-        let replace = Replace {
+        Replace {
             fee: self.original_txs.iter().map(|otx| otx.fee.to_sat()).sum(),
             incremental_relay_feerate: cs_feerate(self.incremental_relay_feerate),
-        };
-        replace
+        }
     }
 
     /// Max feerate of all the original txs.
@@ -134,6 +130,7 @@ impl RbfParams {
         self.original_txs
             .iter()
             .map(|otx| otx.feerate())
+            // Review: should this be `max` instead?
             .min()
             .unwrap_or(FeeRate::ZERO)
     }

tests/synopsis.rs

@@ -201,14 +201,16 @@ fn synopsis() -> anyhow::Result<()> {
 
     let addr = wallet.next_address().expect("must derive address");
 
-    env.send(&addr, Amount::ONE_BTC)?;
+    let txid = env.send(&addr, Amount::ONE_BTC)?;
     env.mine_blocks(1, None)?;
     wallet.sync(&env)?;
-    println!("balance: {}", wallet.balance());
+    println!("Received {}", txid);
+    println!("Balance (confirmed): {}", wallet.balance());
 
-    env.send(&addr, Amount::ONE_BTC)?;
+    let txid = env.send(&addr, Amount::ONE_BTC)?;
     wallet.sync(&env)?;
-    println!("balance: {}", wallet.balance());
+    println!("Received {txid}");
+    println!("Balance (pending): {}", wallet.balance());
 
     let (tip_height, tip_time) = wallet.tip_info(env.rpc_client())?;
     let longterm_feerate = FeeRate::from_sat_per_vb_unchecked(1);
@@ -218,7 +220,7 @@ fn synopsis() -> anyhow::Result<()> {
         .get_new_address(None, None)?
         .assume_checked();
 
-    // okay now create tx.
+    // Okay now create tx.
     let selection = wallet
         .all_candidates()
         .regroup(group_by_spk())
@@ -261,7 +263,7 @@ fn synopsis() -> anyhow::Result<()> {
     let txid = env.rpc_client().send_raw_transaction(&tx)?;
     println!("tx broadcasted: {}", txid);
     wallet.sync(&env)?;
-    println!("balance: {}", wallet.balance());
+    println!("Balance (send tx): {}", wallet.balance());
 
     // Try cancel a tx.
     // We follow all the rules as specified by
@@ -345,7 +347,7 @@ fn synopsis() -> anyhow::Result<()> {
         let txid = env.rpc_client().send_raw_transaction(&tx)?;
         println!("tx broadcasted: {}", txid);
         wallet.sync(&env)?;
-        println!("balance: {}", wallet.balance());
+        println!("Balance (RBF): {}", wallet.balance());
     }
 
     Ok(())