Skip to content

refactor(key-wallet): extracted method for fee calculation #461

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
ZocoLini wants to merge 1 commit into
base: v0.42-dev
Choose a base branch
from
Open

refactor(key-wallet): extracted method for fee calculation #461

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 9 additions & 4 deletions key-wallet/src/wallet/managed_wallet_info/transaction_builder.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -356,6 +356,14 @@ impl TransactionBuilder {
self.build_internal()
}

/// Calculates the transaction fee for an additional output
pub fn calculate_fee(&self) -> u64 {
let fee_rate = self.fee_level.fee_rate();
let estimated_size =
self.estimate_transaction_size(self.inputs.len(), self.outputs.len() + 1);
fee_rate.calculate_fee(estimated_size)
}
Comment on lines +360 to +365
Copy link
Contributor

@coderabbitai coderabbitai bot Feb 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Add a doc comment and document the unconditional +1 change-output assumption.

Two related concerns:

  1. Every other pub fn in this impl block has a /// doc comment — calculate_fee is the only exception.

  2. self.outputs.len() + 1 always treats a change output as present, regardless of whether change_address is Some, and regardless of transaction type (e.g. coinbase has no change). Inside build_internal this is harmless and self-consistent (the same function is used to derive the fee that is deducted). However, as a public method, external callers — e.g. a UI showing the expected fee before build() is called — will always receive an estimate inflated by one extra P2PKH output (~34 bytes × fee_rate) even when no change output will exist.

Consider making the +1 conditional:

✏️ Proposed fix 
+    /// Calculates the estimated fee for this transaction.
+    ///
+    /// The estimate always includes one change output in the size calculation.
+    /// This is consistent with what [`build`] / [`build_internal`] use, but
+    /// callers with no change address set (or special transactions that never
+    /// produce change) will see a slightly inflated estimate (~34 bytes × fee_rate).
     pub fn calculate_fee(&self) -> u64 {
         let fee_rate = self.fee_level.fee_rate();
+        let extra_output = if self.change_address.is_some() { 1 } else { 0 };
         let estimated_size =
-            self.estimate_transaction_size(self.inputs.len(), self.outputs.len() + 1);
+            self.estimate_transaction_size(self.inputs.len(), self.outputs.len() + extra_output);
         fee_rate.calculate_fee(estimated_size)
     }
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@key-wallet/src/wallet/managed_wallet_info/transaction_builder.rs` around
lines 359 - 364, Add a doc comment to calculate_fee explaining it currently
assumes a change output is present and documents the +1 behavior; then change
the unconditional "+1" to be conditional on whether a change output will
actually be created (e.g. use outputs_count = self.outputs.len() + if
self.change_address.is_some() { 1 } else { 0 } or equivalent) and also ensure
you account for transaction types that never have change (e.g. coinbase) by
checking the appropriate flag/method used elsewhere (refer to change_address and
the transaction-type predicate used in build_internal). Keep the call to
fee_rate.calculate_fee(estimated_size) and update
estimate_transaction_size(self.inputs.len(), outputs_count) accordingly.


/// Internal build method that uses the stored special_payload
fn build_internal(mut self) -> Result<Transaction, BuilderError> {
if self.inputs.is_empty() {
Expand Down Expand Up @@ -412,10 +420,7 @@ impl TransactionBuilder {

let mut tx_outputs = self.outputs.clone();

// Calculate fee
let fee_rate = self.fee_level.fee_rate();
let estimated_size = self.estimate_transaction_size(tx_inputs.len(), tx_outputs.len() + 1);
let fee = fee_rate.calculate_fee(estimated_size);
let fee = self.calculate_fee();

let change_amount = total_input.saturating_sub(total_output).saturating_sub(fee);

Expand Down
Loading