Add RBF fee bump chain: guard, actor, wallet manager, and send flow#673
Open
DhruvArvindSingh wants to merge 7 commits intobitcoinppl:masterfrom
Open
Add RBF fee bump chain: guard, actor, wallet manager, and send flow#673DhruvArvindSingh wants to merge 7 commits intobitcoinppl:masterfrom
DhruvArvindSingh wants to merge 7 commits intobitcoinppl:masterfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds an end-to-end Replace-By-Fee (RBF) fee-bump flow: actor can build fee-bump PSBTs, wallet manager exposes Changes
Sequence Diagram(s)sequenceDiagram
participant UI as Reconciler/UI
participant SFM as SendFlowManager
participant WM as WalletManager
participant WA as WalletActor
participant BDK as BDK
UI->>SFM: dispatch ConfirmFeeBump(txid, fee_rate)
SFM->>WM: confirm_fee_bump_txn(txid, fee_rate)
WM->>WA: build_fee_bump_tx(txid, fee_rate)
WA->>BDK: build fee-bump builder for txid
BDK-->>WA: fee-bump builder
WA->>WA: apply fee_rate and finish() -> Psbt
BDK-->>WA: Psbt (or Error)
WA-->>WM: Psbt (or Error)
WM->>WA: get_confirm_details(psbt, fee_rate)
WA-->>WM: ConfirmDetails
WM-->>SFM: ConfirmDetails
SFM->>UI: reconcile FeeBumpConfirmDetails(ConfirmDetails) + route next step
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Greptile SummaryThis PR wires up an end-to-end RBF fee-bump chain: a
Confidence Score: 4/5Safe to merge for Hot wallets; Cold/XpubOnly fee-bump is broken until the missing save_unsigned_transaction call is added. One P1 defect: Cold and XpubOnly wallets performing a fee bump will route to hardware export without persisting the unsigned PSBT, making recovery impossible if the app is backgrounded. The rest of the chain (guard logic, actor method, wallet-manager plumbing, tests) is correct and consistent with existing patterns. rust/src/manager/send_flow_manager.rs — the confirm_fee_bump function is missing save_unsigned_transaction for Cold/XpubOnly wallets. Important Files Changed
Reviews (1): Last reviewed commit: "Add RBF fee bump chain: guard, actor, wa..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+1686
to
+1688
| WalletType::Cold | WalletType::XpubOnly => { | ||
| RouteFactory::new().send_hardware_export(wallet_id, details) | ||
| } |
There was a problem hiding this comment.
save_unsigned_transaction for Cold/XpubOnly wallets
The fee bump path routes Cold and XpubOnly wallets to send_hardware_export without first persisting the unsigned PSBT. The regular send flow (finalize_and_go_to_next_screen, lines 1632–1638) does this before every hardware export — omitting it here means a Cold/XpubOnly user performing an RBF bump will have no recoverable PSBT in the database if they background the app during signing.
Suggested change
| WalletType::Cold | WalletType::XpubOnly => { | |
| RouteFactory::new().send_hardware_export(wallet_id, details) | |
| } | |
| WalletType::Cold | WalletType::XpubOnly => { | |
| if let Err(e) = manager.save_unsigned_transaction(details.clone()) { | |
| let error = SendFlowError::UnableToSaveUnsignedTransaction(e.to_string()); | |
| return me.send_alert_async(error).await; | |
| } | |
| RouteFactory::new().send_hardware_export(wallet_id, details) | |
| } |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
rust/src/manager/send_flow_manager.rs (1)
1659-1699: Consider extracting the shared "build details → save (if cold) → route" flow.This function largely duplicates the post-
confirm_detailstail offinalize_and_go_to_next_screen(wallet-type-based routing, alert onWatchOnly, deferredPushRoute). Once the missingsave_unsigned_transactionis added, the two diverge only in howConfirmDetailsis produced and whetherFeeBumpConfirmDetailsis reconciled. A small helper takingArc<ConfirmDetails>would keep the routing/save logic in one place and reduce drift risk.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/src/manager/send_flow_manager.rs` around lines 1659 - 1699, The post-confirmation routing and save logic in confirm_fee_bump largely duplicates finalize_and_go_to_next_screen; extract a helper on RustSendFlowManager (e.g., handle_confirmed_details(Arc<ConfirmDetails>, Option<bool> is_fee_bump)) that: accepts Arc<ConfirmDetails>, sends the reconciler message when needed (FeeBumpConfirmDetails for fee-bump callers), performs wallet-type dispatch via RouteFactory (use the same branches as in confirm_fee_bump and finalize_and_go_to_next_screen), calls save_unsigned_transaction for Cold/XpubOnly before routing, returns/send an alert via send_alert_async for WatchOnly, and queues DeferredDispatch::new().queue(AppAction::PushRoute(next_route)); then call this helper from confirm_fee_bump (and from finalize_and_go_to_next_screen) to remove the duplicated tail.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@rust/src/manager/send_flow_manager.rs`:
- Around line 1684-1693: The fee-bump branch for Cold/XpubOnly (the arm that
returns RouteFactory::new().send_hardware_export(wallet_id, details)) does not
persist the unsigned PSBT, so the downstream send_hardware_export/import flow
and the unsigned transactions list won't find it; before returning the
send_hardware_export route in send_flow_manager.rs (within
finalize_and_go_to_next_screen/context where next_route is chosen), call the
same persistence used elsewhere—invoke
manager.save_unsigned_transaction(details.clone()) (or the equivalent method on
the current manager/self) for Cold and XpubOnly wallets so the unsigned
transaction is saved prior to routing to send_hardware_export.
In `@rust/src/manager/wallet_manager/actor.rs`:
- Around line 292-295: Replace the two .map_err(|e|
Error::BuildTxError(e.to_string())) usages in the fee-bump flow with the
project's ResultExt::map_err_str style: on the call to
self.wallet.bdk.build_fee_bump(txid) and on builder.finish(), use
map_err_str(Error::BuildTxError) instead to match other builders (see
build_fee_bump, builder.finish and Error::BuildTxError).
---
Nitpick comments:
In `@rust/src/manager/send_flow_manager.rs`:
- Around line 1659-1699: The post-confirmation routing and save logic in
confirm_fee_bump largely duplicates finalize_and_go_to_next_screen; extract a
helper on RustSendFlowManager (e.g.,
handle_confirmed_details(Arc<ConfirmDetails>, Option<bool> is_fee_bump)) that:
accepts Arc<ConfirmDetails>, sends the reconciler message when needed
(FeeBumpConfirmDetails for fee-bump callers), performs wallet-type dispatch via
RouteFactory (use the same branches as in confirm_fee_bump and
finalize_and_go_to_next_screen), calls save_unsigned_transaction for
Cold/XpubOnly before routing, returns/send an alert via send_alert_async for
WatchOnly, and queues
DeferredDispatch::new().queue(AppAction::PushRoute(next_route)); then call this
helper from confirm_fee_bump (and from finalize_and_go_to_next_screen) to remove
the duplicated tail.
🪄 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: ec0cb16d-18d1-4b8b-a36c-ab2fc9b1f3f5
📒 Files selected for processing (4)
rust/src/manager/send_flow_manager.rsrust/src/manager/wallet_manager.rsrust/src/manager/wallet_manager/actor.rsrust/src/transaction/transaction_details.rs
praveenperera
force-pushed
the
feat/rbf-fee-bump
branch
from
cb09314 to
fabe25f
Compare
praveenperera
requested changes
Apr 17, 2026
Contributor
praveenperera
left a comment
praveenperera
left a comment
There was a problem hiding this comment.
regen bindings and make sure ci passes
DhruvArvindSingh
force-pushed
the
feat/rbf-fee-bump
branch
2 times, most recently
from
26c6250 to
b2da4dd
Compare
Author
|
Hey @praveenperera , All reviews are addressed.... the PR should be ready for a final CI run. |
praveenperera
force-pushed
the
feat/rbf-fee-bump
branch
from
b2da4dd to
07ae287
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@rust/src/manager/send_flow_manager.rs`:
- Around line 1661-1705: The code sends FeeBumpConfirmDetails before checking
for WalletType::WatchOnly, allowing the UI to receive confirm details for a flow
that is immediately rejected; modify confirm_fee_bump so it rejects WatchOnly
(calling send_alert_async with SendFlowError::UnableToBuildTxn("watch only"))
before calling me.reconciler.send_async(Message::FeeBumpConfirmDetails(...)),
i.e., move the WalletType match/WatchOnly check (and the unsigned-tx save logic
for Cold/XpubOnly) to occur prior to emitting FeeBumpConfirmDetails so only
valid wallet types produce that message.
🪄 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: 13e85706-93be-4da8-a1bf-904a1da1b016
⛔ Files ignored due to path filters (1)
ios/CoveCore/Sources/CoveCore/generated/cove.swiftis excluded by!**/generated/**
📒 Files selected for processing (5)
android/app/src/main/java/org/bitcoinppl/cove_core/cove.ktrust/src/manager/send_flow_manager.rsrust/src/manager/wallet_manager.rsrust/src/manager/wallet_manager/actor.rsrust/src/transaction/transaction_details.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- rust/src/transaction/transaction_details.rs
- android/app/src/main/java/org/bitcoinppl/cove_core/cove.kt
praveenperera
force-pushed
the
feat/rbf-fee-bump
branch
from
07ae287 to
d9e9a04
Compare
DhruvArvindSingh
force-pushed
the
feat/rbf-fee-bump
branch
from
d9e9a04 to
71b6796
Compare
Author
|
Hey @praveenperera addressed the remaining review points:
All CI checks should pass now. PR is ready for a final review whenever you get a chance. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
rust/src/manager/send_flow_manager.rs (1)
1661-1709:⚠️ Potential issue | 🟡 MinorReject watch-only wallets before building the replacement PSBT.
The current check happens after
confirm_fee_bump_txn(), so a watch-only fee-bump dispatch can still do wallet work and return a generic build error before reaching the intended"watch only"alert.🛡️ Proposed fix
fn confirm_fee_bump(self: &Arc<Self>, txid: TxId, fee_rate: FeeRate) { let me = self.clone(); let manager = self.wallet_manager.clone(); let (wallet_type, wallet_id) = { let state = self.state.lock(); (state.metadata.wallet_type, state.metadata.id.clone()) }; + if matches!(wallet_type, WalletType::WatchOnly) { + return self.send_alert(SendFlowError::UnableToBuildTxn("watch only".to_string())); + } + cove_tokio::task::spawn(async move { let raw_txid = *txid; let confirm_details = manager.confirm_fee_bump_txn(raw_txid, fee_rate).await; @@ - // reject watch-only before touching UI state - if matches!(wallet_type, WalletType::WatchOnly) { - let error = SendFlowError::UnableToBuildTxn("watch only".to_string()); - return me.send_alert_async(error).await; - } - // save unsigned transaction for hardware wallets before routing if matches!(wallet_type, WalletType::Cold | WalletType::XpubOnly) && let Err(e) = manager.save_unsigned_transaction(details.clone()) @@ WalletType::Cold | WalletType::XpubOnly => { RouteFactory::new().send_hardware_export(wallet_id, details) } - WalletType::WatchOnly => unreachable!("rejected above"), + WalletType::WatchOnly => unreachable!("watch-only wallets are rejected before spawning"), };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/src/manager/send_flow_manager.rs` around lines 1661 - 1709, The code currently calls manager.confirm_fee_bump_txn(...) before checking for WalletType::WatchOnly, allowing watch-only wallets to perform work and surface generic build errors; move the watch-only rejection to occur before calling manager.confirm_fee_bump_txn by checking wallet_type (the value extracted in confirm_fee_bump) and returning me.send_alert_async(SendFlowError::UnableToBuildTxn("watch only".to_string())).await if it matches WalletType::WatchOnly, so no wallet work or confirm_fee_bump_txn invocation happens for watch-only wallets.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@rust/src/transaction/transaction_details.rs`:
- Around line 353-360: The can_rbf_bump() method currently returns true for any
unconfirmed transaction that signals RBF, which incorrectly enables "Speed Up"
for inbound transactions; update can_rbf_bump() to also require the transaction
be outgoing (e.g., add an ownership/direction check like self.is_outgoing or
self.direction == TransactionDirection::Outgoing) so it only returns true for
unconfirmed, RBF-signaling, outgoing transactions: replace the body with a
conjunction that includes the outgoing check (self.is_outgoing &&
self.is_rbf_signaling && !self.is_confirmed()). Apply the same change to the
other identical method occurrence referenced in the comment.
---
Duplicate comments:
In `@rust/src/manager/send_flow_manager.rs`:
- Around line 1661-1709: The code currently calls
manager.confirm_fee_bump_txn(...) before checking for WalletType::WatchOnly,
allowing watch-only wallets to perform work and surface generic build errors;
move the watch-only rejection to occur before calling
manager.confirm_fee_bump_txn by checking wallet_type (the value extracted in
confirm_fee_bump) and returning
me.send_alert_async(SendFlowError::UnableToBuildTxn("watch
only".to_string())).await if it matches WalletType::WatchOnly, so no wallet work
or confirm_fee_bump_txn invocation happens for watch-only wallets.
🪄 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: 0394f3f5-d0d5-4648-b997-ecbce5054a47
⛔ Files ignored due to path filters (1)
ios/CoveCore/Sources/CoveCore/generated/cove.swiftis excluded by!**/generated/**
📒 Files selected for processing (7)
android/app/src/main/java/org/bitcoinppl/cove/flows/SendFlow/SendFlowManager.ktandroid/app/src/main/java/org/bitcoinppl/cove_core/cove.ktios/Cove/Flows/SendFlow/SendFlowManager.swiftrust/src/manager/send_flow_manager.rsrust/src/manager/wallet_manager.rsrust/src/manager/wallet_manager/actor.rsrust/src/transaction/transaction_details.rs
DhruvArvindSingh
force-pushed
the
feat/rbf-fee-bump
branch
from
71b6796 to
20517d8
Compare
DhruvArvindSingh
force-pushed
the
feat/rbf-fee-bump
branch
from
38c3501 to
8c2ecb2
Compare
Contributor
|
ci not passing |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@rust/src/manager/send_flow_manager.rs`:
- Around line 1686-1694: The two flows diverge: finalize_and_go_to_next_screen
calls manager.save_unsigned_transaction and on Err sends an alert but continues
to send_hardware_export, whereas confirm_fee_bump returns early after
me.send_alert_async and skips reconciling FeeBumpConfirmDetails and routing;
make behavior intentional and consistent by either (A) changing confirm_fee_bump
to follow finalize_and_go_to_next_screen’s approach (log/send the same alert via
SendFlowError::UnableToSaveUnsignedTransaction but continue to
me.reconciler.send_async(Message::FeeBumpConfirmDetails(details.clone())) and
proceed to send_hardware_export), or (B) keep confirm_fee_bump’s stricter early
return but add a concise comment in confirm_fee_bump explaining why failing to
persist the unsigned transaction must block FeeBumpConfirmDetails/re-routing;
update the code paths around save_unsigned_transaction,
SendFlowError::UnableToSaveUnsignedTransaction, me.send_alert_async,
me.reconciler.send_async, and send_hardware_export accordingly to reflect the
chosen behavior.
🪄 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: 5c3cd836-ec1b-44b1-952b-e0abff5a76ef
📒 Files selected for processing (5)
android/app/src/main/java/org/bitcoinppl/cove/flows/SendFlow/SendFlowManager.ktios/Cove/Flows/SendFlow/SendFlowManager.swiftrust/src/lib.rsrust/src/manager/send_flow_manager.rsrust/src/transaction/transaction_details.rs
✅ Files skipped from review due to trivial changes (1)
- rust/src/lib.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- ios/Cove/Flows/SendFlow/SendFlowManager.swift
Author
|
@praveenperera i regenerated the bindings, i am sure that the CI will run this time |
Author
|
Hey @praveenperera i ran the CI in my fork locally and it passed all the tests , CI run |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Builds the RBF fee bump action chain on top of #663 (
is_rbf_signaling).Problem: Cove can detect RBF-signaling transactions but has no way to
actually replace them with a higher fee. A stuck pending transaction has
no "Speed Up" path.
What this adds:
can_rbf_bump()onTransactionDetailscombined guard(
is_pending && is_rbf_signaling) for the mobile UI to gate a"Speed Up" action. Prevents showing fee bump on confirmed transactions.
preview_pending_rbf()constructor lets iOS/Android devs buildSwiftUI/Compose previews for the Speed Up UI without a live wallet.
build_fee_bump_tx(txid, fee_rate)onWalletActorwraps BDK'sbuild_fee_bump, which validates the tx is unconfirmed andRBF-signaling, then returns a replacement PSBT at the new fee rate.
confirm_fee_bump_txn(txid, fee_rate)onRustWalletManagermirrors
confirm_txn: builds the bump PSBT and returnsConfirmDetails, reusing the existing confirm screen unchanged.ConfirmFeeBump { txid, fee_rate }action + handler inSendFlowManagerdispatched by iOS/Android when the user confirmsa fee bump. Routes to
send_confirm(hot wallet) orsend_hardware_export(cold wallet), same as a normal send.FeeBumpConfirmDetailsreconcile message notifies the frontendwhen confirm details are ready.
Testing
cargo test transaction_details
10 tests pass, including 3 new ones covering all
can_rbf_bumpstates:truefalsefalsecargo check
No warnings or errors. Rust-only changes UniFFI bindings regenerate on
next
just build-ios/just build-android.Platform Coverage
Checklist
Summary by CodeRabbit
New Features
Tests