Add cancellable Payjoin polling countdown - #869
Sandipmandal25 wants to merge 7 commits into
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 Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughPayjoin polling now reports an absolute deadline, exposes cancellation, and propagates waiting state through Rust, Android, and iOS. Both mobile send flows display countdown controls and handle cancellation, broadcast completion, and errors during polling. ChangesPayjoin polling lifecycle
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Payjoin as Rust payjoin flow
participant WalletActor
participant WalletManager
participant SendFlow
participant ConfirmScreen
Payjoin->>WalletActor: report polling deadline
WalletActor->>WalletManager: publish PayjoinPollingStarted
WalletManager->>SendFlow: update payjoinDeadlineSecs
SendFlow->>ConfirmScreen: render PayjoinWaiting
ConfirmScreen->>WalletManager: request cancelPayjoin
WalletManager->>WalletActor: cancel active payjoin
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 SummaryThe PR adds a cancellable Payjoin polling countdown across the Rust core and both mobile clients.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains. Important Files Changed
Sequence DiagramsequenceDiagram
participant UI as Mobile Send UI
participant WM as WalletManager
participant PA as PayjoinActor
participant Receiver as Payjoin Receiver
participant Node as Bitcoin Node
PA->>Receiver: Poll for proposal
PA->>WM: PayjoinPollingStarted(deadline)
WM->>UI: Show countdown and cancel action
alt Receiver supplies proposal
PA->>Node: Broadcast Payjoin transaction
else User cancels or deadline expires
UI->>WM: cancelPayjoin()
WM->>PA: cancel_and_fallback()
PA->>Node: Broadcast fallback transaction
end
Node-->>WM: PayjoinTxBroadcast or SendFlowError
WM-->>UI: Clear deadline and show result
Reviews (5): Last reviewed commit: "Add cancellable Payjoin polling countdow..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
04948cd to
d84bdba
Compare
|
@greptileai review |
|
@greptileai re review |
|
@coderabbitai review |
✅ Action performedReview finished.
|
![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
🧹 Nitpick comments (1)
ios/Cove/Flows/SendFlow/SendFlowConfirmScreen.swift (1)
128-132: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRoute Payjoin cancellation through
WalletManager.Line 130 calls
RustWalletManager.cancelPayjoin()directly from the view. AddWalletManager.cancelPayjoin()and call that method fromSendFlowConfirmScreen. Keep generated Rust access inside the platform manager boundary.🤖 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 `@ios/Cove/Flows/SendFlow/SendFlowConfirmScreen.swift` around lines 128 - 132, Update SendFlowConfirmScreen.cancelPayjoin() to call a new WalletManager.cancelPayjoin() method instead of accessing manager.rust directly. Implement WalletManager.cancelPayjoin() to delegate to the generated Rust manager, keeping Rust access within the platform manager boundary and preserving the existing asynchronous cancellation behavior.Source: Coding guidelines
🤖 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/wallet_manager/payjoin.rs`:
- Around line 582-586: The Payjoin fallback-persistence failure in
complete_with_fallback must terminate the mobile waiting state instead of
leaving payjoinDeadlineSecs set. Route the set_pending_fallback error through
the existing send-flow error channel or add a typed terminal Payjoin reconcile
event, then update both mobile wallet managers to clear the deadline and
transition the send flow out of PayjoinWaiting when handling it.
---
Nitpick comments:
In `@ios/Cove/Flows/SendFlow/SendFlowConfirmScreen.swift`:
- Around line 128-132: Update SendFlowConfirmScreen.cancelPayjoin() to call a
new WalletManager.cancelPayjoin() method instead of accessing manager.rust
directly. Implement WalletManager.cancelPayjoin() to delegate to the generated
Rust manager, keeping Rust access within the platform manager boundary and
preserving the existing asynchronous cancellation behavior.
🪄 Autofix
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 Plus
Run ID: 7d962894-7adc-4f05-a124-ba2bd9f8da60
⛔ Files ignored due to path filters (2)
android/app/src/main/java/org/bitcoinppl/cove_core/cove.ktis excluded by!android/app/src/main/java/org/bitcoinppl/cove_core/**ios/CoveCore/Sources/CoveCore/generated/cove.swiftis excluded by!**/generated/**,!ios/CoveCore/Sources/CoveCore/generated/**
📒 Files selected for processing (9)
android/app/src/main/java/org/bitcoinppl/cove/WalletManager.ktandroid/app/src/main/java/org/bitcoinppl/cove/flows/SendFlow/ConfirmScreen/SendFlowConfirmScreen.ktandroid/app/src/main/java/org/bitcoinppl/cove/flows/SendFlow/SendFlowContainer.ktios/Cove/Flows/SendFlow/ConfirmScreen/SwipeToSendView.swiftios/Cove/Flows/SendFlow/SendFlowConfirmScreen.swiftios/Cove/WalletManager.swiftrust/src/manager/wallet_manager.rsrust/src/manager/wallet_manager/actor.rsrust/src/manager/wallet_manager/payjoin.rs
88a8c72 to
1ce99d3
Compare
1ce99d3 to
b399614
Compare
|
@praveenperera rebased this pr! |
b399614 to
33fab44
Compare
|
@greptileai review |
33fab44 to
ee6f552
Compare
ee6f552 to
b7a0f5a
Compare
| private func cancelPayjoin() { | ||
| sendState = .sending | ||
| Task { | ||
| try? await manager.rust.cancelPayjoin() |
There was a problem hiding this comment.
The iOS build currently fails because WalletManager.rust is a private optional property, so this screen cannot call cancelPayjoin() directly. Could you add a WalletManager.cancelPayjoin() wrapper that uses withRustAsync, and call that wrapper from the screen? This also keeps the code aligned with ARCHITECTURE.md:177.
|
|
||
| pub async fn cancel_payjoin(&mut self) -> ActorResult<Result<(), Error>> { | ||
| if let Some(actor) = self.payjoin_actor.take() { | ||
| send!(actor.cancel_and_fallback()); |
There was a problem hiding this comment.
When an active session is cancelled, PayjoinActor::cancel_and_fallback returns Some(fallback_tx) after persisting the fallback marker and closing the child. The current fire-and-forget call drops that transaction, so the fallback is not broadcast and no PayjoinTxBroadcast is emitted. Both screens can remain in Sending until the app restarts. Could the child result be awaited and passed to handle_payjoin_fallback? ARCHITECTURE.md:58 also requires the manager to apply child results.
| #[uniffi::method] | ||
| pub async fn cancel_payjoin(&self) -> Result<(), Error> { | ||
| call!(self.actor.cancel_payjoin()).await.unwrap()?; | ||
| self.force_wallet_scan().await; |
There was a problem hiding this comment.
force_wallet_scan() returns a Result, but this statement ignores it. Since Rust CI runs Clippy with -D warnings (.github/workflows/ci.yml:210), unused_must_use will fail the build. Please either propagate the result or explicitly handle the failure if this is intended to be best effort.
|
|
||
| // restore or transition to waiting UI from manager-owned deadline; | ||
| // survives Compose recreation on config changes like rotation | ||
| LaunchedEffect(walletManager.payjoinDeadlineSecs) { |
There was a problem hiding this comment.
If a persisted Payjoin resumes before navigation, or the user leaves its confirmation and opens another one, the app-owned manager still has the old deadline. This effect can then mark any new confirmation for that wallet as PayjoinWaiting, even when its endpoint and details belong to a different payment. The banner shows one payment while Cancel acts on the old session. Could the session or transaction identity be carried through, so the deadline is restored only for the matching confirmation? This is required by AGENTS.md:12.
There was a problem hiding this comment.
Fixed with a saved local guard so only the same screen that started Payjoin can show the waiting state.
| } | ||
|
|
||
| private func payjoinPollingStarted(_: UInt64?, _ deadline: UInt64?) { | ||
| guard let deadline, case .sending = sendState else { return } |
There was a problem hiding this comment.
When payjoinDeadlineSecs is already set before this view appears, for example after a persisted polling session resumes, onChange does not run on the first render by default. The new view starts in .idle, so the guard rejects the existing deadline. The iOS countdown and cancel action then never appear for the resumed session. Could the initial matching deadline be applied separately from the change handler?
There was a problem hiding this comment.
only showing waiting after this screen has already started sending
| case .idle: | ||
| EmptyView() | ||
| case .sending: | ||
| case .sending, .payjoinWaiting: |
There was a problem hiding this comment.
While the state is .payjoinWaiting, SwipeToSendHandle still has its drag gesture, and dragEnded calls onConfirm() without checking the state. A user can swipe again during the ten-minute wait, which starts initiatePayment again and replaces the waiting UI with the active-session error while the original Payjoin continues. Could the gesture be enabled only when the state is .idle, as it already is on Android?
There was a problem hiding this comment.
users cannot swipe again while Payjoin is waiting
b280f68 to
0cc159f
Compare
4c90a96 to
876081a
Compare
876081a to
b0ec0a4
Compare
Give each Payjoin attempt a stable session id and typed intent so confirm screens only react to their own payment, cancel cannot hit the wrong session, and the endpoint is no longer a raw string on the route.
Preserve the terminal failure state after alert dismissal so users cannot start a second send while recovery remains active.
Cancel in-flight relay work at the session deadline so fallback broadcast is not delayed and late proposals are not accepted.
Keep the session lifecycle, broadcast helpers, and ActivePayjoin state together so actor.rs and transactions.rs stay focused on their primary concerns.
Replace broadcast-request count polling in wallet actor test support with `tokio::sync::Notify` signaling. The payjoin shutdown tests now wait for an explicit broadcast request notification instead of checking atomic counters, and pending-broadcast test node state was simplified accordingly. This makes request detection more direct and less timing-sensitive.
2 participants
Summary
Why
PayJoin polling can take time, but the send screen previously showed only a generic loading state with no indication of what was happening or how long remained.
This gives users clearer feedback and lets them fall back to a normal send without waiting for the session to expire.
Checklist
Summary by CodeRabbit