Warn when not enough funds to cover fee#825
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a6f179452c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Greptile SummaryThis PR introduces fee-aware balance validation in the Bitcoin send flow. When a user's amount would be affordable on its own but the network fee pushes total spend above their available balance, they now see a "Fee Estimate Still Loading" or "Insufficient Funds" warning instead of being silently blocked at finalization.
Confidence Score: 5/5Safe to merge — the fee-aware balance check is logically correct, the coin-control bypass is intentional and properly deferred to finalization, and all edge cases (zero amount, missing fee, coin selection failure) are covered by new tests. The validation logic is sound end-to-end: amount-only failures, amount+fee failures, and missing-fee states each produce distinct, correct outcomes. The fee_from_insufficient_funds_needed helper now returns an error on bad input instead of the previous silent zero. The coin-control bypass is commented, tested, and matched by finalization's total_fee_blocks_send check. No wrong-data paths or broken contracts were found. No files require special attention. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[validate_amount_internal] --> B{amount == 0?}
B -- yes --> Z[return false: ZeroAmount]
B -- no --> C[spendable_balance_limit\nbased on mode]
C --> D[total_fee_sats from\nfee_selection.selected]
D --> E[spendable_balance_check\namount, total_fee_sats, balance]
E -- WithinBalance --> F[return true ✓]
E -- ExceedsBalance --> G{fee_consumes\ncoin_control_balance?}
G -- yes --> H[return true ✓\ndefer to finalization\nFee Too High! alert]
G -- no --> I{unavailable\nbalance alert?}
I -- yes --> J[return false\nLocked / Loading alert]
I -- no --> K{is_max_selected?}
K -- yes --> L[select_max async\nreturn false]
K -- no --> M[return false\nInsufficientFunds alert]
E -- MissingFeeTotal --> N[schedule_fee_rate_update\nreturn false\nFee Estimate Still Loading]
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
A[validate_amount_internal] --> B{amount == 0?}
B -- yes --> Z[return false: ZeroAmount]
B -- no --> C[spendable_balance_limit\nbased on mode]
C --> D[total_fee_sats from\nfee_selection.selected]
D --> E[spendable_balance_check\namount, total_fee_sats, balance]
E -- WithinBalance --> F[return true ✓]
E -- ExceedsBalance --> G{fee_consumes\ncoin_control_balance?}
G -- yes --> H[return true ✓\ndefer to finalization\nFee Too High! alert]
G -- no --> I{unavailable\nbalance alert?}
I -- yes --> J[return false\nLocked / Loading alert]
I -- no --> K{is_max_selected?}
K -- yes --> L[select_max async\nreturn false]
K -- no --> M[return false\nInsufficientFunds alert]
E -- MissingFeeTotal --> N[schedule_fee_rate_update\nreturn false\nFee Estimate Still Loading]
Reviews (2): Last reviewed commit: "Handle missing send fee totals" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
praveenperera
force-pushed
the
warn-on-fee
branch
from
a6f1794 to
a329c5b
Compare
Include the selected network fee when checking send amount balance validation so the send flow surfaces the existing insufficient-funds warning before transaction construction.
Refactors fee estimation in wallet transactions to compute each fee option through a shared helper that still returns a total fee when coin selection reports insufficient funds. Instead of failing, it derives the fee from `needed - amount`, so UI fee options remain populated for amounts that exceed spendable balance only because of fees. Adds an async actor test covering this edge case.
praveenperera
force-pushed
the
warn-on-fee
branch
from
a329c5b to
44e4c8a
Compare
Validate fee-inclusive sends against selected coin-control funds instead of wallet-wide unlocked balance. Keep the fee-specific hard-block path for selected fees that consume the full manual spend set.
Block send validation while selected fee totals are missing so near-max sends cannot pass by treating fees as zero. Also make the insufficient-funds fee estimate reject invalid BDK needed amounts and add a shared constructor for fee options without totals.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@rust/src/manager/send_flow_manager.rs`:
- Around line 346-356: The MissingFeeTotal branch in
send_flow_manager::validate_amount_internal is now conflating “no fee selected”
with “fee selected but missing total,” which breaks the manager_for_validation
fixture used by validate_amount_allows_low_nonzero_amount. Update the validation
flow so the default fixture seeds fee_selection, or introduce a separate
state/path for the no-fee case and keep it out of MissingFeeTotal. Use the
validate_amount_internal match arm and manager_for_validation setup to locate
the change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ff0136b1-92fc-4468-8af0-696ac53cd534
📒 Files selected for processing (7)
rust/crates/cove-types/src/fees.rsrust/src/manager/send_flow_manager.rsrust/src/manager/send_flow_manager/finalize.rsrust/src/manager/send_flow_manager/read_api.rsrust/src/manager/send_flow_manager/validation.rsrust/src/manager/wallet_manager/actor.rsrust/src/manager/wallet_manager/actor/transactions.rs
| validation::SpendableBalanceCheck::MissingFeeTotal => { | ||
| self.schedule_fee_rate_update(); | ||
|
|
||
| let msg = Message::SetAlert(SendFlowError::InsufficientFunds.into()); | ||
| if display_alert { | ||
| sender.queue(msg); | ||
| } else { | ||
| debug!("validate_amount_failed: {msg:?}"); | ||
| let msg = Message::SetAlert(validation::missing_fee_total_alert()); | ||
| if display_alert { | ||
| sender.queue(msg); | ||
| } else { | ||
| debug!("validate_amount_failed: {msg:?}"); | ||
| } | ||
|
|
||
| false |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect the conflicting paths without executing tests.
rg -n -C4 'fn validate_amount_allows_low_nonzero_amount|SpendableBalanceCheck::MissingFeeTotal|set_selected_fee_total' rust/src/manager/send_flow_manager.rsRepository: bitcoinppl/cove
Length of output: 7473
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read the validation logic and the specific test/fixture context.
sed -n '250,370p' rust/src/manager/send_flow_manager.rs
printf '\n--- TEST FIXTURE ---\n'
sed -n '760,920p' rust/src/manager/send_flow_manager.rs
printf '\n--- RELATED TESTS ---\n'
sed -n '920,980p' rust/src/manager/send_flow_manager.rsRepository: bitcoinppl/cove
Length of output: 12826
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the validation helpers that classify missing fee totals.
rg -n -C4 'spendable_balance_check|missing_fee_total_alert|SpendableBalanceCheck::MissingFeeTotal|spendable_balance_for_validation|total_fee_sats' rust/src/manager -g '*.rs'Repository: bitcoinppl/cove
Length of output: 21357
Keep the no-fee case out of MissingFeeTotal validate_amount_internal now fails the default manager_for_validation() fixture used by validate_amount_allows_low_nonzero_amount, because it never sets fee_selection. Seed a fee selection there, or split “no fee selected yet” from “selected fee missing total”.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@rust/src/manager/send_flow_manager.rs` around lines 346 - 356, The
MissingFeeTotal branch in send_flow_manager::validate_amount_internal is now
conflating “no fee selected” with “fee selected but missing total,” which breaks
the manager_for_validation fixture used by
validate_amount_allows_low_nonzero_amount. Update the validation flow so the
default fixture seeds fee_selection, or introduce a separate state/path for the
no-fee case and keep it out of MissingFeeTotal. Use the validate_amount_internal
match arm and manager_for_validation setup to locate the change.
Source: Path instructions
1 participant
Summary by CodeRabbit
New Features
Bug Fixes