Repository navigation
fix: pubky swaps route review follow-ups - #1929
Conversation
Codex Review Summary: ✅ CompletedThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
[Medium impact] The changes affect Pubky Swaps sends, recovery, and wallet-wipe cleanup. The PR appears safe to merge, with two non-blocking cleanup gaps that can leave swap polling running. FindingsSummaryAdds failure and wipe cleanup for the debug-only Pubky Swaps route.
Reviews (1) · Last reviewed commit: "fix: stop swaps on any hold failure, wip..." · Reviewed by Greptile |
Regtest APKDownload bitkit-dev-debug universal APK (expires in 30 days). |
talosmachina
left a comment
There was a problem hiding this comment.
1 finding (1 P1). Closes the four follow-ups from #1923 on the Pubky Swaps route: a closed Send sheet bumps sendSession, wipes cancel running claim waits, each swap's hold hash is persisted so a failed hold drops the swap live or at the next start, and a restore pass skips swaps committed or dropped this session. Reviewed f26be5f, static review (bitkit-android is not built on this side).
The live path and the wipe/restore/closed-sheet paths hold up; the gap is the restart check for a hold paid over Spark.
Merge confidence: 3/5, one P1 (the restart drop misses Spark-paid holds); build-and-test was still running when I read CI.
What I checked, and what I ruled out
Read: the full diff, PubkySwapsRouteRepo.kt around every changed hunk, proceedWithSwapRoutedPayment, finishSwapRoutedPayment, releaseFailedSwapHold, hideSheet, resetSendState, LightningRepo.payInvoice, WipeWalletUseCase.wipeLocal, the new tests.
CI at read time: lint, detekt, build-local green; build-and-test, compose-ui and the local e2e jobs pending.
Ruled out
- Live hold failure no longer dropped after removing
dropFailedSwapfromreleaseFailedSwapHold:watchHoldFailuresstarts fromresumeUnfinishedSwapson every successful node start, before any send can commit, andwatchHoldpersists the hash beforepayInvoice, so the repo collector drops it. - Hold failing before
payInvoicereturns: the hash is in the cache store before the payment goes out, so the collector sees the event regardless ofpendingPaymentRepo.track. sendSession++inhideSheetbreaking the in-sheet flow: the session is read at the start ofproceedWithSwapRoutedPayment, andrestoreSwapContact/releaseFailedSwapHold/showCommittedSwaponly compare it, so a closed sheet just stops handing the contact back, as intended.- Wipe racing a claim wait:
claimWaitsregisters the job beforeawait, andinvokeOnCompletionremoves it, so a cancel on wipe reaches every live wait. - Restore picking up a dropped swap:
known.removeAll { it in committed || it in dropped }runs beforetrack, andadvanceInBackgroundalso refuses dropped ids.
Reviewer model: claude-opus-5-5
Reasoning effort: Not exposed (the session metadata available to this reviewer does not record a reasoning setting)
coreyphillips
left a comment
There was a problem hiding this comment.
Two independent reviews, nothing blocking a merge.
worth doing, does not block
- Send sheet replaced via showSheet still keeps its send session (app/src/main/java/to/bitkit/viewmodels/AppViewModel.kt:8801-8826). The #1925 fix only bumps
sendSessioninhideSheet, butshowSheetcan close an open Send sheet without going through it.showSheetsets_currentSheetto null and then to the new sheet (AppViewModel.kt:8811 and 8826).SheetHostonly callsonDismisswhen the dismissed key equals the current visibility key (SheetHost.kt:171-175), so a programmatic swap never reacheshideSheet. Two callers do this while a Send sheet can be open: -handleChannelClosedshowsSheet.ConnectionClosedon a counterparty close (AppViewModel.kt:2633). - ContentView opens sheet deep links (ContentView.kt:386). A counterparty close while a swap hold HTLC is in flight is a natural way for that hold to fail later. The sequence is: 1. The Pending sheet is replaced andsendSessionstays the same. 2. The hold fails. 3.releaseFailedSwapHold(AppViewModel.kt:7524) orrestoreSwapContact(7462) passes the session check and setsactiveContactPaymentContextwith no Send sheet open, which is the #1925 symptom. I confirmed this by reading the code, not by running it. A test like the newswap route hold failing after its sheet closed leaves no contact behind, withsut.showSheet(Sheet.ConnectionClosed)in place ofsut.hideSheet(), should show it. One fix is to bump the session wherever an openSheet.Sendis replaced, for example in a small helper that bothhideSheetandshowSheetcall. The route is behind the debug-onlyPUBKY_SWAPS_ROUTESflag, so this is non-blocking.
|
@coreyphillips thanks. Confirmed in the code: Out of scope: tracked in #1930, because the route is behind the debug-only Merging this PR now: approved, CI green, no open threads. |
Closes #1925
Closes #1926
Closes #1927
Closes #1928
Follow-up to #1923: the non-blocking points from its last review, all on the Pubky Swaps route behind the debug-only
PUBKY_SWAPS_ROUTESflag.Description
payInvoicereturns. The background loop checks the hold before each advance, in the node's payments and then Spark's sends, so a hold that failed while the app was closed, over Lightning or Spark, stops its swap at the next start (bug: early or post-restart hold failures keep the swap polled #1927).Out of Scope
showSheet(a counterparty close, a sheet deep link) still keeps its send session: tracked in bug: replaced send sheet keeps its swap contact #1930, because the route is behind the debug-only flag and this PR is approved.Design
N/A — no UI changes.
Preview
N/A
QA Notes
Setup
N/A — no device testing required: the swap route is behind the debug-only
PUBKY_SWAPS_ROUTESflag, and these are failure, wipe and restore paths the unit tests drive.Journeys
N/A — not drivable; see Manual Tests.
Manual Tests
N/A
Automated Checks
AppViewModelSendFlowTest.kt— a hold failing after its sheet closed leaves no contact behindPubkySwapsRouteRepoTest.kt— a failed hold payment stops its resumed swap; a hold that failed while the app was closed, over the node or Spark, is dropped at start; a dropped swap's claim wait stops; a wipe stops a running claim wait; a restore leaves a swap dropped this session aloneAppViewModelSendFlowTest.kt— each hold is recorded before it goes outEvidence
just compile,just testandjust lintpass locally on this PR's head. The closed-sheet test failed before its fix.Gaps
Models used
claude-opus-5-5(reasoning:Not exposed)claude-opus-5-5(reasoning:Not exposed)The session records the model but not its reasoning setting.