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

[ZocoLini]

Summary by CodeRabbit

• Refactor
  • Improved consistency in transaction fee calculations to ensure more reliable fee estimation across wallet operations.

[coderabbitai]

📝 Walkthrough

Walkthrough

A new public calculate_fee() method is added to TransactionBuilder that encapsulates fee computation logic by obtaining the current fee rate and estimating transaction size. The existing build_internal method is refactored to invoke this new method instead of performing inline fee calculations.

Changes

Cohort / File(s)	Summary
Transaction Fee Calculation key-wallet/src/wallet/managed_wallet_info/transaction_builder.rs	New public method calculate_fee(&self) -> u64 extracts fee computation logic. Refactored build_internal to call this method instead of duplicating inline calculations, centralizing fee rate application and transaction size estimation.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Poem

🐰 A fee to compute, now clean and bright,
Extracted to its own calculate_fee height,
No more inline math scattered about,
One method to rule them—no doubt!
The builder grows wise with each refactor's might.

🚥 Pre-merge checks | ✅ 3

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately describes the main change: extracting a fee calculation method from inline code into a reusable helper method in TransactionBuilder.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

• 📝 Generate docstrings (stacked PR)
• 📝 Generate docstrings (commit on current branch)

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch refactor/extract-fee-calculation

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (1)

key-wallet/src/wallet/managed_wallet_info/transaction_builder.rs (1)

422-422: Note: select_inputs fee path is still independent of calculate_fee.

The delegation to self.calculate_fee() here is correct and consistent. However, select_inputs (Line 119) derives the fee separately through self.fee_level.fee_rate() passed directly to CoinSelector::select_coins_with_size, and does not call the new calculate_fee(). For transactions with a special payload, the two fee estimates can diverge — the coin-selection path uses calculate_base_size() (which excludes per-input size) while calculate_fee() uses estimate_transaction_size() (which includes per-input size). This is a pre-existing inconsistency worth tracking.

🤖 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` at line
422, select_inputs currently computes fee via self.fee_level.fee_rate() passed
into CoinSelector::select_coins_with_size while other paths use
self.calculate_fee(), causing inconsistent estimates for payload-bearing
transactions; fix by making select_inputs derive its fee using the same
calculate_fee() path (or refactor calculate_fee to accept parameters and call
calculate_base_size/estimate_transaction_size consistently) so both
coin-selection and final fee use the same size/fee logic — update the call sites
in select_inputs and the use of CoinSelector::select_coins_with_size to accept
the unified fee value from calculate_fee() (or the refactored helper) and ensure
calculate_base_size() vs estimate_transaction_size() logic is reconciled inside
calculate_fee()/the new helper.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@key-wallet/src/wallet/managed_wallet_info/transaction_builder.rs`:
- Around line 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.

---

Nitpick comments:
In `@key-wallet/src/wallet/managed_wallet_info/transaction_builder.rs`:
- Line 422: select_inputs currently computes fee via self.fee_level.fee_rate()
passed into CoinSelector::select_coins_with_size while other paths use
self.calculate_fee(), causing inconsistent estimates for payload-bearing
transactions; fix by making select_inputs derive its fee using the same
calculate_fee() path (or refactor calculate_fee to accept parameters and call
calculate_base_size/estimate_transaction_size consistently) so both
coin-selection and final fee use the same size/fee logic — update the call sites
in select_inputs and the use of CoinSelector::select_coins_with_size to accept
the unified fee value from calculate_fee() (or the refactored helper) and ensure
calculate_base_size() vs estimate_transaction_size() logic is reconciled inside
calculate_fee()/the new helper.

[coderabbitai]

⚠️ 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.