Skip to content

Commit

Permalink
Merge #1426: feat: add further bitcoin::Amount usage on public APIs
Browse files Browse the repository at this point in the history
a03949a feat: use `Amount` on `calculate_fee`, `fee_absolute`, `fee_amount` and others (Leonardo Lima)

Pull request description:

  builds on top of #1411, further improves #823

  <!-- You can erase any parts of this template not applicable to your Pull Request. -->

  ### Description

  It further updates and adds the usage of `bitcoin::Amount` instead of `u64`.

  <!-- Describe the purpose of this PR, what's being adding and/or fixed -->

  ### Notes to the reviewers

  Open for comments and discussions.

  <!-- In this section you can include notes directed to the reviewers, like explaining why some parts
  of the PR were done in a specific way -->

  ### Changelog notice
  - Updated `CreateTxError::FeeTooLow` to use `bitcoin::Amount`.
  - Updated `Wallet::calculate_fee()`. to use `bitcoin::Amount`
  - Updated `TxBuilder::fee_absolute()`. to use `bitcoin::Amount`.
  - Updated `CalculateFeeError::NegativeFee` to use `bitcoin::SignedAmount`.
  - Updated `TxGraph::calculate_fee()`. to use `bitcoin::Amount`.
  - Updated `PsbUtils::fee_amount()` to use `bitcoin::Amount`.

  <!-- Notice the release manager should include in the release tag message changelog -->
  <!-- See https://keepachangelog.com/en/1.0.0/ for examples -->

  ### Checklists

  #### All Submissions:

  * [x] I've signed all my commits
  * [x] I followed the [contribution guidelines](https://github.com/bitcoindevkit/bdk/blob/master/CONTRIBUTING.md)
  * [x] I ran `cargo fmt` and `cargo clippy` before committing

  #### New Features:

  * [x] I've added tests for the new feature
  * [x] I've added docs for the new feature

ACKs for top commit:
  storopoli:
    ACK a03949a

Tree-SHA512: abfbf7d48ec004eb448ed0a928e7e64b5f82a2ab51ec278fede4ddbff4eaba16469a7403f78dfeba1aba693b0132a528bc7c4ef072983cbbcc2592993230e40f
  • Loading branch information
notmandatory committed Jun 4, 2024
2 parents 4a8452f + a03949a commit 26586fa
Show file tree
Hide file tree
Showing 10 changed files with 113 additions and 118 deletions.
27 changes: 12 additions & 15 deletions crates/chain/src/tx_graph.rs
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,7 @@ use crate::{
use alloc::collections::vec_deque::VecDeque;
use alloc::sync::Arc;
use alloc::vec::Vec;
use bitcoin::{Amount, OutPoint, Script, Transaction, TxOut, Txid};
use bitcoin::{Amount, OutPoint, Script, SignedAmount, Transaction, TxOut, Txid};
use core::fmt::{self, Formatter};
use core::{
convert::Infallible,
Expand Down Expand Up @@ -182,7 +182,7 @@ pub enum CalculateFeeError {
/// Missing `TxOut` for one or more of the inputs of the tx
MissingTxOut(Vec<OutPoint>),
/// When the transaction is invalid according to the graph it has a negative fee
NegativeFee(i64),
NegativeFee(SignedAmount),
}

impl fmt::Display for CalculateFeeError {
Expand All @@ -196,7 +196,7 @@ impl fmt::Display for CalculateFeeError {
CalculateFeeError::NegativeFee(fee) => write!(
f,
"transaction is invalid according to the graph and has negative fee: {}",
fee
fee.display_dynamic()
),
}
}
Expand Down Expand Up @@ -307,7 +307,7 @@ impl<A> TxGraph<A> {
})
}

/// Calculates the fee of a given transaction. Returns 0 if `tx` is a coinbase transaction.
/// Calculates the fee of a given transaction. Returns [`Amount::ZERO`] if `tx` is a coinbase transaction.
/// Returns `OK(_)` if we have all the [`TxOut`]s being spent by `tx` in the graph (either as
/// the full transactions or individual txouts).
///
Expand All @@ -318,20 +318,20 @@ impl<A> TxGraph<A> {
/// Note `tx` does not have to be in the graph for this to work.
///
/// [`insert_txout`]: Self::insert_txout
pub fn calculate_fee(&self, tx: &Transaction) -> Result<u64, CalculateFeeError> {
pub fn calculate_fee(&self, tx: &Transaction) -> Result<Amount, CalculateFeeError> {
if tx.is_coinbase() {
return Ok(0);
return Ok(Amount::ZERO);
}

let (inputs_sum, missing_outputs) = tx.input.iter().fold(
(0_i64, Vec::new()),
(SignedAmount::ZERO, Vec::new()),
|(mut sum, mut missing_outpoints), txin| match self.get_txout(txin.previous_output) {
None => {
missing_outpoints.push(txin.previous_output);
(sum, missing_outpoints)
}
Some(txout) => {
sum += txout.value.to_sat() as i64;
sum += txout.value.to_signed().expect("valid `SignedAmount`");
(sum, missing_outpoints)
}
},
Expand All @@ -343,15 +343,12 @@ impl<A> TxGraph<A> {
let outputs_sum = tx
.output
.iter()
.map(|txout| txout.value.to_sat() as i64)
.sum::<i64>();
.map(|txout| txout.value.to_signed().expect("valid `SignedAmount`"))
.sum::<SignedAmount>();

let fee = inputs_sum - outputs_sum;
if fee < 0 {
Err(CalculateFeeError::NegativeFee(fee))
} else {
Ok(fee as u64)
}
fee.to_unsigned()
.map_err(|_| CalculateFeeError::NegativeFee(fee))
}

/// The transactions spending from this output.
Expand Down
10 changes: 5 additions & 5 deletions crates/chain/tests/test_tx_graph.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,8 @@ use bdk_chain::{
Anchor, Append, BlockId, ChainOracle, ChainPosition, ConfirmationHeightAnchor,
};
use bitcoin::{
absolute, hashes::Hash, transaction, Amount, BlockHash, OutPoint, ScriptBuf, Transaction, TxIn,
TxOut, Txid,
absolute, hashes::Hash, transaction, Amount, BlockHash, OutPoint, ScriptBuf, SignedAmount,
Transaction, TxIn, TxOut, Txid,
};
use common::*;
use core::iter;
Expand Down Expand Up @@ -466,14 +466,14 @@ fn test_calculate_fee() {
}],
};

assert_eq!(graph.calculate_fee(&tx), Ok(100));
assert_eq!(graph.calculate_fee(&tx), Ok(Amount::from_sat(100)));

tx.input.remove(2);

// fee would be negative, should return CalculateFeeError::NegativeFee
assert_eq!(
graph.calculate_fee(&tx),
Err(CalculateFeeError::NegativeFee(-200))
Err(CalculateFeeError::NegativeFee(SignedAmount::from_sat(-200)))
);

// If we have an unknown outpoint, fee should return CalculateFeeError::MissingTxOut.
Expand Down Expand Up @@ -505,7 +505,7 @@ fn test_calculate_fee_on_coinbase() {

let graph = TxGraph::<()>::default();

assert_eq!(graph.calculate_fee(&tx), Ok(0));
assert_eq!(graph.calculate_fee(&tx), Ok(Amount::ZERO));
}

// `test_walk_ancestors` uses the following transaction structure:
Expand Down
3 changes: 2 additions & 1 deletion crates/electrum/tests/test_electrum.rs
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,8 @@ fn scan_detects_confirmed_tx() -> anyhow::Result<()> {
.fee
.expect("Fee must exist")
.abs()
.to_sat() as u64;
.to_unsigned()
.expect("valid `SignedAmount`");

// Check that the calculated fee matches the fee from the transaction data.
assert_eq!(fee, tx_fee);
Expand Down
3 changes: 2 additions & 1 deletion crates/esplora/tests/async_ext.rs
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,8 @@ pub async fn test_update_tx_graph_without_keychain() -> anyhow::Result<()> {
.fee
.expect("Fee must exist")
.abs()
.to_sat() as u64;
.to_unsigned()
.expect("valid `Amount`");

// Check that the calculated fee matches the fee from the transaction data.
assert_eq!(fee, tx_fee);
Expand Down
3 changes: 2 additions & 1 deletion crates/esplora/tests/blocking_ext.rs
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,8 @@ pub fn test_update_tx_graph_without_keychain() -> anyhow::Result<()> {
.fee
.expect("Fee must exist")
.abs()
.to_sat() as u64;
.to_unsigned()
.expect("valid `Amount`");

// Check that the calculated fee matches the fee from the transaction data.
assert_eq!(fee, tx_fee);
Expand Down
15 changes: 5 additions & 10 deletions crates/wallet/src/psbt/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ pub trait PsbtUtils {

/// The total transaction fee amount, sum of input amounts minus sum of output amounts, in sats.
/// If the PSBT is missing a TxOut for an input returns None.
fn fee_amount(&self) -> Option<u64>;
fn fee_amount(&self) -> Option<Amount>;

/// The transaction's fee rate. This value will only be accurate if calculated AFTER the
/// `Psbt` is finalized and all witness/signature data is added to the
Expand All @@ -49,18 +49,13 @@ impl PsbtUtils for Psbt {
}
}

fn fee_amount(&self) -> Option<u64> {
fn fee_amount(&self) -> Option<Amount> {
let tx = &self.unsigned_tx;
let utxos: Option<Vec<TxOut>> = (0..tx.input.len()).map(|i| self.get_utxo_for(i)).collect();

utxos.map(|inputs| {
let input_amount: u64 = inputs.iter().map(|i| i.value.to_sat()).sum();
let output_amount: u64 = self
.unsigned_tx
.output
.iter()
.map(|o| o.value.to_sat())
.sum();
let input_amount: Amount = inputs.iter().map(|i| i.value).sum();
let output_amount: Amount = self.unsigned_tx.output.iter().map(|o| o.value).sum();
input_amount
.checked_sub(output_amount)
.expect("input amount must be greater than output amount")
Expand All @@ -70,6 +65,6 @@ impl PsbtUtils for Psbt {
fn fee_rate(&self) -> Option<FeeRate> {
let fee_amount = self.fee_amount();
let weight = self.clone().extract_tx().ok()?.weight();
fee_amount.map(|fee| Amount::from_sat(fee) / weight)
fee_amount.map(|fee| fee / weight)
}
}
8 changes: 4 additions & 4 deletions crates/wallet/src/wallet/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ use crate::descriptor::DescriptorError;
use crate::wallet::coin_selection;
use crate::{descriptor, KeychainKind};
use alloc::string::String;
use bitcoin::{absolute, psbt, OutPoint, Sequence, Txid};
use bitcoin::{absolute, psbt, Amount, OutPoint, Sequence, Txid};
use core::fmt;

/// Errors returned by miniscript when updating inconsistent PSBTs
Expand Down Expand Up @@ -78,8 +78,8 @@ pub enum CreateTxError {
},
/// When bumping a tx the absolute fee requested is lower than replaced tx absolute fee
FeeTooLow {
/// Required fee absolute value (satoshi)
required: u64,
/// Required fee absolute value [`Amount`]
required: Amount,
},
/// When bumping a tx the fee rate requested is lower than required
FeeRateTooLow {
Expand Down Expand Up @@ -160,7 +160,7 @@ impl fmt::Display for CreateTxError {
)
}
CreateTxError::FeeTooLow { required } => {
write!(f, "Fee to low: required {} sat", required)
write!(f, "Fee to low: required {}", required.display_dynamic())
}
CreateTxError::FeeRateTooLow { required } => {
write!(
Expand Down
24 changes: 11 additions & 13 deletions crates/wallet/src/wallet/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -977,7 +977,7 @@ impl Wallet {
self.persist.stage(ChangeSet::from(additions));
}

/// Calculates the fee of a given transaction. Returns 0 if `tx` is a coinbase transaction.
/// Calculates the fee of a given transaction. Returns [`Amount::ZERO`] if `tx` is a coinbase transaction.
///
/// To calculate the fee for a [`Transaction`] with inputs not owned by this wallet you must
/// manually insert the TxOut(s) into the tx graph using the [`insert_txout`] function.
Expand All @@ -1004,7 +1004,7 @@ impl Wallet {
/// let fee = wallet.calculate_fee(tx).expect("fee");
/// ```
/// [`insert_txout`]: Self::insert_txout
pub fn calculate_fee(&self, tx: &Transaction) -> Result<u64, CalculateFeeError> {
pub fn calculate_fee(&self, tx: &Transaction) -> Result<Amount, CalculateFeeError> {
self.indexed_graph.graph().calculate_fee(tx)
}

Expand Down Expand Up @@ -1036,8 +1036,7 @@ impl Wallet {
/// ```
/// [`insert_txout`]: Self::insert_txout
pub fn calculate_fee_rate(&self, tx: &Transaction) -> Result<FeeRate, CalculateFeeError> {
self.calculate_fee(tx)
.map(|fee| Amount::from_sat(fee) / tx.weight())
self.calculate_fee(tx).map(|fee| fee / tx.weight())
}

/// Compute the `tx`'s sent and received [`Amount`]s.
Expand Down Expand Up @@ -1465,7 +1464,7 @@ impl Wallet {
if let Some(previous_fee) = params.bumping_fee {
if fee < previous_fee.absolute {
return Err(CreateTxError::FeeTooLow {
required: previous_fee.absolute,
required: Amount::from_sat(previous_fee.absolute),
});
}
}
Expand Down Expand Up @@ -1498,9 +1497,8 @@ impl Wallet {
return Err(CreateTxError::NoUtxosSelected);
}

// we keep it as a float while we accumulate it, and only round it at the end
let mut outgoing: u64 = 0;
let mut received: u64 = 0;
let mut outgoing = Amount::ZERO;
let mut received = Amount::ZERO;

let recipients = params.recipients.iter().map(|(r, v)| (r, *v));

Expand All @@ -1513,7 +1511,7 @@ impl Wallet {
}

if self.is_mine(script_pubkey) {
received += value;
received += Amount::from_sat(value);
}

let new_out = TxOut {
Expand All @@ -1523,7 +1521,7 @@ impl Wallet {

tx.output.push(new_out);

outgoing += value;
outgoing += Amount::from_sat(value);
}

fee_amount += (fee_rate * tx.weight()).to_sat();
Expand Down Expand Up @@ -1565,7 +1563,7 @@ impl Wallet {
required_utxos,
optional_utxos,
fee_rate,
outgoing + fee_amount,
outgoing.to_sat() + fee_amount,
&drain_script,
)?;
fee_amount += coin_selection.fee_amount;
Expand Down Expand Up @@ -1613,7 +1611,7 @@ impl Wallet {
} => fee_amount += remaining_amount,
Change { amount, fee } => {
if self.is_mine(&drain_script) {
received += amount;
received += Amount::from_sat(*amount);
}
fee_amount += fee;

Expand Down Expand Up @@ -1797,7 +1795,7 @@ impl Wallet {
.collect(),
utxos: original_utxos,
bumping_fee: Some(tx_builder::PreviousFee {
absolute: fee,
absolute: fee.to_sat(),
rate: fee_rate,
}),
..Default::default()
Expand Down
8 changes: 4 additions & 4 deletions crates/wallet/src/wallet/tx_builder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -186,10 +186,10 @@ impl<'a, Cs> TxBuilder<'a, Cs> {
}

/// Set an absolute fee
/// The fee_absolute method refers to the absolute transaction fee in satoshis (sats).
/// If anyone sets both the fee_absolute method and the fee_rate method,
/// the FeePolicy enum will be set by whichever method was called last,
/// as the FeeRate and FeeAmount are mutually exclusive.
/// The fee_absolute method refers to the absolute transaction fee in [`Amount`].
/// If anyone sets both the `fee_absolute` method and the `fee_rate` method,
/// the `FeePolicy` enum will be set by whichever method was called last,
/// as the [`FeeRate`] and `FeeAmount` are mutually exclusive.
///
/// Note that this is really a minimum absolute fee -- it's possible to
/// overshoot it slightly since adding a change output to drain the remaining
Expand Down
Loading

0 comments on commit 26586fa

Please sign in to comment.