Repository navigation
feat: move LN channel balances to spark - #1774
Conversation
Regtest APKDownload bitkit-dev-debug universal APK (expires in 30 days). |
|
Pushed |
|
Pushed
Screenshots at 360 dp are in Preview. |
ab34526 to
ab1b09d
Compare
62526d3 to
967292c
Compare
|
Rebased onto the rebuilt Spark base
|
… feat/move-to-spark
|
Merged the new base head |
|
Reran the failed CI jobs at |
|
Dropped "at the lowest fee" from this PR's changelog line, to match the new title: the line now says what the change does, not how cheaply. |
|
Pushed |
… feat/move-to-spark
|
Merged the new base head |
|
Pushed
The remaining low-severity items are in #1802. |
… feat/move-to-spark # Conflicts: # app/src/main/java/to/bitkit/repositories/SparkRepo.kt # app/src/test/java/to/bitkit/repositories/SparkRepoTest.kt
|
323adc9 merges base 97951b8, which conflicted in |
pwltr
left a comment
There was a problem hiding this comment.
Reviewed 323adc9.
Low candidate — Preserve move notification suppression across process restart.
MoveToSparkRepo.kt stores move-identification hashes only in memory. If the process dies while a move is in flight, late settlement may produce ordinary send and receive notifications for the same self-transfer. The reviewed accounting path still converges to a single activity; this candidate concerns notification accuracy.
Low candidate — Verify fee refresh after receive-first settlement.
SparkRepo.kt marks an existing pending send successful when its matching Spark receive completes, preserving the send record’s fee. If that fee is still zero, the activity could temporarily understate it. The later ActivityRepo.handlePaymentEvent correction path was outside the scoped review, so a persistent defect is unverified. The move screen uses the actual paid fee, and the approved routing-fee cap remains enforced.
Review details
General review
Model: GPT-6.1 Sol
Reasoning effort: high
Security review
Model: Kimi K3 (OpenRouter/Moonshot)
Reasoning effort: high
piotr-iohk
left a comment
There was a problem hiding this comment.
QA review
Scope: Re-reviewed the full PR at 323adc9 after no usable baseline was supplied and the comparison base changed from 99b81bcc to 97951b8. Coverage is the whole diff against that merge base.
No new actionable code findings.
The move creates an amountless Spark invoice and pays it only through payInvoiceOverLdk. The quoted amount plus the shown routing fee (the LDK estimate plus 2 sats) stays within the live outbound HTLC limit, and that shown fee is the payment cap. A missing route offers a smaller amount down to 1,000 sats. The sheet and both Move To Spark buttons stay hidden when the fee reserve leaves less than 1,000 sats. A failure received during the wait stays marked reported, so the screen error stands alone. After the two-minute wait, a later failure still raises the usual failure notice, and the screen changes to Could not move or Moved to Spark only while that same move is still showing.
Leaving the screen, or killing the app, still drops the in-memory outcome and payment hash. That limitation remains synonymdev/bitkit-android#1802, already deferred on this PR. Compared with bitkit-ios#1212 at 0ce947b, the quote, fee cap, and too-large recovery match. iOS keeps an in-flight move when its screen closes. Android drops that wait with the screen. bitkit-e2e-tests#297 at b004e5e was checked only for dismissing the intro when it covers Home. Dependency bitkit-android#1697 is this PR's base 97951b8. The channel-only send uses that base's LDK pay path.
Validation: this review did not run the unit suite or a device. build-and-test passed on this head. The inspected tests cover quote bounds, a late stored outcome, activity replacement, and the late screen update. Device testing: not performed in this review. The author reports a device run at eccf3648a, and a later J1 pass was reported at a6a84323d. Those builds predate the settlement listener and the send-hash commit on this head. Local e2e for send, multi_address, and onchain was still pending, so those jobs are not evidence here.
Reviewer model: grok-4.7
Reasoning effort: high
… feat/move-to-spark # Conflicts: # app/src/main/java/to/bitkit/ui/ContentView.kt
Out of scope: tracked in #1802, which this PR lists under Out of Scope (keeping the move's hash across a restart). A process death mid-move only costs a duplicate notification; the activity still converges to one row.
Confirmed and fixed in 5315c36. bitkit-core stamps |
pwltr
left a comment
There was a problem hiding this comment.
Reviewed 5315c36, the merge of the updated base branch and 4e5dc91. A move's channel send now keeps the node's routing fee even when its Spark receive settled it first: the activity takes the fee when the node reports the succeeded send, and a recorded success is still never undone. The merge keeps the base's shown Spark balance beside the Move to Spark entry on the spending wallet and Lightning connections. The move-to-Spark translations are left to a follow-up; those screens read in English until then. No new findings.
Review details
General review
Model: Claude Opus 5.5
Reasoning effort: high
jvsena42
left a comment
There was a problem hiding this comment.
Re-review of 5315c36, the base merge and 4e5dc91: no findings.
- Routing fee kept:
CoreService.kt:903-914lets a node update through the staleness check when it changes the stored fee. Status downgrades are still blocked earlier (:884-896). The next sync rewrites the same row, and the Spark receive path returns early once the send succeeded, so the two writers don't overwrite each other back and forth. - Merge resolution:
SparkRepo.kt:938-949follows the base's unsaved-activity tracking. - Translations: 4e5dc91 removes the move-to-Spark keys from every locale in full, so no locale is left with a partial set.
MissingTranslationis off, so lint is unaffected. Until the follow-up lands, these screens read in English in other locales.
Reviewer model: claude-fable-5-1 (delta review), claude-opus-5-5 (coordination)
Reasoning effort: not exposed (Fable subagent), default (Opus)
The base branch was changed.
|
Retargeted to master: #1697, the Spark base this PR is stacked on, is merging now, and deleting its branch would close this PR. |
ben-kaufman
left a comment
There was a problem hiding this comment.
No new material findings at c9f4357. The PR's documented follow-ups remain deferred.
Current-head device journeys and local app checks were not run in this review. The author's earlier regtest runs used direct channels. Multi-hop routing with a nonzero fee remains untested.
Reviewer model: gpt-6.1-sol, claude-opus-5-5
Reasoning effort: xhigh (gpt-6.1-sol), high (claude-opus-5-5)
|
Merged: approved by @ben-kaufman at c9f4357, required checks green, no open threads. |
Closes #1768
Twin: synonymdev/bitkit-ios#1212
Dependency: #1697
Companion: synonymdev/bitkit-e2e-tests#297
This PR adds Move to Spark: a one-time sheet and an action that pays the balance of a Lightning channel to the user's own Spark wallet over the channel, then offers to close the emptied channel. Blocktank is closing its channels, and moving the balance over Lightning costs only the routing fee, where a close followed by a deposit costs on-chain and claim fees and 3 confirmations. It stacks on #1697: review it against that base; it merges after #1697.
Description
MoveToSparkRepo, which quotes the move against a new amountless Spark invoice: it starts from what the channels can send and leaves room for the routing fee. The fee shown is LDK's estimate plus a 2 sat margin, and the payment's routing fee is capped at exactly that.LightningRepo.payInvoiceOverLdkandestimateLdkRoutingFeesForAmount, so the move always goes over the channels and never falls back to Spark; the defaultshouldPayWithSparkrouting is unchanged.Out of Scope
move()andMoveToSparkViewModelunit tests, keeping the outcome after leaving the screen, and keeping the move's hash across a restart — chore: move to spark review follow-ups #1802.isExitingcomes with the exit PR (feat: spark emergency exit #1782), not this PR's base. Every Move to Spark check goes throughMoveToSparkMath.isSparkUsableinMoveToSparkRepo.kt, so whichever of the two PRs lands second changes that one line tospark.isReady && !spark.isExiting.Design
N/A — no design available. UI pending design: this is an AI-made first version from existing components; a Figma design and designer review follow.
Preview
Android emulator (Pixel profile) at 896 × 1994, 320 dpi, at 2x speed.
rec-sheet2x.mp4
rec-too-large2x.mp4
rec-move-rest2x.mp4
rec-close2x.mp4
Screens
Android emulator (Pixel profile) at 896 × 1994, 320 dpi, in flow order. Bitkit draws dark only, its system bars included.
QA Notes
Setup
sparkprofile from feat: local spark regtest network bitkit-docker#37 (./spark up), which runs three Spark operators, the SSP with its Lightning node, the LNURL and data-sync servers and a chain API on bitkit-docker's chain, compatible with Spark SDK 0.28. It does not publish the SSP node's P2P port (9735); publish it in a compose override to open a channel from the app.gradlew assembleDevDebug) withSPARK_LOCAL_CONFIG_B64set to./spark config --base64and the Spark chain service pointed athttp://127.0.0.1:23090/api, thenadb reversefor 23535–23537, 23049, 23090 and the SSP node's P2P port, and Electrum (60001). This PR has no switch for it; the app-side settings land in separate PRs../spark send <address> 500000), then open Lightning Connections, Manual Setup, and open a channel to the SSP node; confirm it with./spark mine 6.qa-movefile in the app's files folder (limit=<sats>,fail,slow).Journeys
move-to-spark.xml— sheet once, entry button, move with routing fee, close afterwards, nothing for a wallet without a channelManual Tests
Automated Checks
MoveToSparkRepoTest.kt— quote fits the routing fee under the channel limit, grows back once the real fee is known, keeps a smaller target, offers a move only when 1,000 sats fit beside the fee reserve, maps no route (including a send that finds no route on a live invoice) to too large, refuses with nothing to move or Spark not readyMoveToSparkViewModelTest.kt— a failure or success that arrives after the wait changes the in-flight screen only for the move that still owns itActivityServiceMoveToSparkTest.ktand aSparkRepoTest.ktcase — a move keeps one outgoing Activity row with the routing fee, whichever leg arrives firstNotifyPaymentReceivedHandlerTest.ktandAppViewModelSendFlowTest.kt— no received notice for the move, the new sheet wired inEvidence
Tested at
eccf3648aon the stack and build from Setup, Android emulator (Pixel profile) at 896 × 1994, 320 dpi, one 400,000 sat channel to the SSP; the Preview and Screens above are this run. Later commits (f7dd3ec and 3341321) change only the in-flight screen and Activity rows, which this run does not show.failhook, the screen showed "Could not move" with Try Again.Tested again at
4e5dc9105on the same stack and emulator, with a fresh wallet and one channel to the SSP, and no hook switched on.1-no-channel2x.mp4
2a-sheet2x.mp4
2b-relaunch2x.mp4
3-in-flight-staged2x.mp4
4-activity2x.mp4
4b-activity-late2x.mp4
Gaps
Mainnet tests (run by the author)
These run on real devices with mainnet funds, by the author. Reviewers and review bots don't need to run them, and they don't block review.
Round 1
None yet.
Round 2 (pending)
Models used
claude-opus-5-5(reasoning:medium)claude-opus-5-5(reasoning:medium)claude-opus-5-5(reasoning:Not exposed)Metadata note: the review subagents' run records give the model but not a reasoning setting.