Repository navigation
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. |
jvsena42
left a comment
There was a problem hiding this comment.
Delta 891630d49..91b4a4eaa is clean: e06ac74cf treats a PaymentSendingFailed on an unexpired invoice as no route (the Android side of the LOW on synonymdev/bitkit-ios#1212), 91b4a4eaa only names arguments, 7d9bd0f4d offers the move only when the minimum still fits after the first fee guess, ac748932e is journey text. The rest comes from the base, reviewed on #1697.
Device gate (partial): 91b4a4eaa — not run yet; transfer/move-to-spark.xml needs a channel and a Spark regtest wallet.
|
Pushed |
pwltr
left a comment
There was a problem hiding this comment.
Reviewed 91b4a4e.
Medium candidate — Retain hashless settlement failures until the payment ID is known. MoveToSparkRepo.kt:133: the settlement listener begins filtering events while paymentId is null; the ID is assigned only after payInvoiceOverLdk returns. A supported PaymentFailed event with a null hash can arrive independently before that return and be rejected despite belonging to the transfer. The SharedFlow does not replay it once the ID becomes known, so the two-minute timeout reports InFlight and skips the failure or smaller-payment recovery flow. Buffer or reconcile early hashless failures when the matching ID becomes available. This is supported by static source ordering and existing event-shape tests; production timing was not reproduced, and native library internals were unavailable, so it remains a candidate.
Review details
General review
Model: GPT-6.1 Sol
Reasoning effort: high
piotr-iohk
left a comment
There was a problem hiding this comment.
QA review
Scope: Full reassessment of the whole pull request at 91b4a4ea, against merge base c85259fe. The comparison base changed and no coverage baseline was supplied, so this pass covers the entire PR.
No new actionable code findings.
The quote keeps the moved amount plus the shown routing fee (the LDK estimate, rounded down, plus 2 sats) within the usable outbound limit, and the channel-only send caps the fee at that amount. The send uses the Spark invoice’s one-hour expiry. While this process still remembers the payment hash, foreground send/receive notices and the background payment notification are suppressed. If the payment has no result after two minutes, the screen offers only OK.
The same quote math, 1,000 sat minimum, fee cap, entry points, and move-to-spark journey match bitkit-ios#1212 at b6049d0a. iOS keeps the swipe in progress until the payment settles; Android shows the in-flight result after two minutes. bitkit-e2e-tests#297 at b004e5ed dismisses MoveToSparkIntroLater, which this sheet exposes. Spark createInvoice(amount, description, expiry) and SparkState.isReady match on bitkit-android#1697 at 85986cde and on this PR’s base.
Unit, lint, and UI checks passed at this revision. Local e2e failed, including onboarding waiting for home on a new wallet, before a channel exists for this sheet. Device testing was not performed. The author’s journey evidence is from earlier revisions and still needs the PR’s local Spark regtest setup.
Suggested additional test cases
- Android, dev build on the PR’s local Spark regtest, with one channel that can send at least 1,000 sats: swipe to move and leave the payment unsettled for more than 2 minutes. The screen shows the in-flight text and only OK. After the payment settles, Spark holds the amount and the channel can still be closed from Lightning Connections.
Earlier findings reconciled at this revision: 5 verified fixed, 1 not substantiated.
Reviewer model: grok-4.7
Reasoning effort: xhigh
jvsena42
left a comment
There was a problem hiding this comment.
Device gate (partial): 91b4a4eaa. Journeys: 0 passed, 0 failed, 1 partial. perf: not relevant, skipped.
transfer/move-to-spark.xml:
- Steps 1–3 passed. The Move To Spark sheet showed, Later closed it, and it did not come back after a relaunch.
- On the first launch the sheet came after Backup.
canMovewas still false at the 2 s timed-sheet check, so the priority-4 Backup sheet won. This is expected ordering, not a defect.
- On the first launch the sheet came after Backup.
- Step 4 did not hold, because of the environment. The local bitcoind was reset mid-run and rolled back 13 blocks. The funding tx lost its confirmations (
Funding transaction was un-confirmed. Locked at 3 confs, now have 0 confs) and LDK closed the channel, socanMovewent false and the Move button was gone. A second channel never confirmed on the reset chain. - Steps 5–8 not run. They need the PR's local spark-sdk
regtest/localstack. Itsghcr.io/breez/spark-operatorimage is private ("unauthorized"), so I could not start it. I used Lightspark's hosted regtest for Spark, plus a 400k Manual Setup channel to the local LND.
Setup: Pixel_9 emulator, E2E=true E2E_BACKEND=local build with local VSS, lnurl-auth and electrs/bitcoind, and Spark on Lightspark's hosted regtest. Fresh wallet. No crashes.
Reviewer model: Claude Opus 5.5 (claude-opus-5-5)
Reasoning effort: medium
@pwltr Fixed in eccf364: while the payment id is still unknown, the listener keeps every |
jvsena42
left a comment
There was a problem hiding this comment.
Delta review 91b4a4eaa..eccf3648a: no findings. A hashless PaymentFailed that arrives before payInvoiceOverLdk returns its id is now queued, and matched to sentId afterwards. A failure for another payment can't match, and the in-flight timeout path is unchanged.
Device gate: still partial (steps 5–8 of move-to-spark.xml need the private spark-operator image), as in my last review.
Reviewer model: Claude Opus 5.5 (claude-opus-5-5)
Reasoning effort: low
ben-kaufman
left a comment
There was a problem hiding this comment.
No new material findings at eccf3648a.
Coverage limits remain: the actual move has no current-head device run, multi-hop routing fees are untested, move/view-model tests are deferred to #1802, and the separate security passes did not complete.
Reviewer model: gpt-6.1-sol
Reasoning effort: xhigh
piotr-iohk
left a comment
There was a problem hiding this comment.
QA review
Scope: Follow-up at eccf3648, re-reviewing the delta since 91b4a4ea against the completed baseline review. Merge base is still c85259fe. This pass inspected MoveToSparkRepo.payAndWait, the channel-only send it calls, and the move screen's failure and in-flight results. Inherited coverage: quote and fee reserve, channel-only send and invoice expiry before an id exists, notice suppression, the intro sheet and Spending/Connections entry, the iOS comparison at b6049d0a, the e2e intro selector at b004e5ed, and Spark createInvoice / isReady on the #1697 base.
No new actionable code findings.
A hashless PaymentFailed that arrives before payInvoiceOverLdk returns is queued and, when its payment id is the id that send returns, ends the move as too large or failed. The two-minute in-flight result remains the path when that id has no matching event. The invoice hash is still recorded before that result, so foreground and background notice suppression still applies to this early failure. The five resolved threads on this PR were checked at this head; the payment-id match, raw route error, live channel state, reported-hash suppression, and 1,000 sat fee reserve are still in the code.
Device testing: not performed in this review. Validation was source inspection only. MoveToSparkRepoTest covers the quote and fee math.
Suggested additional test cases
- Android unit (
MoveToSparkRepoTest): with Spark ready and a channel above the minimum, emitPaymentFailed(paymentHashnull,paymentIdequal to the idpayInvoiceOverLdkwill return, reasonROUTE_NOT_FOUND) before that call returns. Expected:move()fails withMoveToSparkError.TooLarge.
Non-blocking consistency suggestions
- bitkit-ios#1212 at 0a7b433e starts listening in
sendOverChannelsonly aftersendreturns, andpaymentFailedmatches only whenpaymentHashequals that id. The same early hashless failure would leave the iOS move screen unchanged.
Reviewer model: grok-4.7
Reasoning effort: xhigh
jvsena42
left a comment
There was a problem hiding this comment.
Delta eccf3648a..f7dd3ecb7: no findings. The in-flight Done screen now has its own title and illustration, and still offers only OK, so closing the channel stays hidden while the payment may return. in_flight_title and the new in_flight_text are translated in every values-* file.
Device gate: still partial (needs the private spark-operator image).
Reviewer model: Claude Opus 5.5 (claude-opus-5-5)
Reasoning effort: not exposed
|
@jvsena42 The QA Notes Setup now uses the |
… feat/move-to-spark # Conflicts: # app/src/main/java/to/bitkit/repositories/SparkRepo.kt
|
Pushed a6a8432: merged the Spark base |
jvsena42
left a comment
There was a problem hiding this comment.
Delta f7dd3ecb7..a6a84323d: no findings. The moves channel send and its Spark receive share one hash, and the send now owns the row whichever leg arrives first. SparkRepo.recordActivity skips the Spark receive when a SENT row exists, and ActivityService lets a non-failed outbound LDK payment replace a RECEIVED row through the existing update path. A failed channel send cant replace it, and Spark only receives when the send succeeds.
Device gate: pending. It can now run on the bitkit-docker#37 spark profile.
Reviewer model: Claude Opus 5.5 (claude-opus-5-5)
Reasoning effort: not exposed
pwltr
left a comment
There was a problem hiding this comment.
Reviewed a6a8432.
Medium — Completed Spark receipt can leave the send activity pending or failed. SparkRepo.recordActivity discards every receive update when an existing row is a send, including when that send is still pending or failed. CoreService.processBolt11 can also replace a completed receive with a pending send. With a delayed or missing LDK terminal update after timeout or restart, completed settlement is therefore not reflected in activity. Preserve the outbound direction and fee while reconciling terminal receipt evidence.
Same-quote retry idempotency remains an unconfirmed Medium candidate: the LDK send implementation and UI retry caller were outside the frozen scope and their transport was rejected. No duplicate payment is established. This was static source review; runtime validation remains unperformed.
Review details
General review
Model: GPT-6.1 Sol
Reasoning effort: high
Security review
Model: Kimi K3
Reasoning effort: high
A completed Spark receive marks its pending channel send succeeded, and only a succeeded send replaces the Spark receive row.
… feat/move-to-spark
|
jvsena42
left a comment
There was a problem hiding this comment.
Delta a6a84323d..6ca4d3fa2 (c5327fc): no findings. A completed Spark receive marks the move's pending channel send SUCCEEDED, and only a SUCCEEDED send replaces the receive row. A pending or failed update can no longer downgrade a settled row. The send's own later success still updates its fee through the normal path.
Device gate: in progress on the bitkit-docker spark profile.
Reviewer model: Claude Opus 5.5 (claude-opus-5-5)
Reasoning effort: not exposed
… feat/move-to-spark
jvsena42
left a comment
There was a problem hiding this comment.
The device gate passed at a6a8432. It ran on the bitkit-docker spark profile (cdd294a) on a Pixel_9 emulator with a fresh to.bitkit.dev, with Savings funded and a 400k channel opened to the SSP.
- J1 (
move-to-spark.xml): PASS.- With no channel there is no sheet and no button.
- The intro sheet shows once the channel is ready and doesn't come back after Later and a relaunch.
- The quote, the swipe and the Done screen work, as do both closes. Spark ends at 394 930, which is 196 478 + 198 452.
- Activity shows one "Sent · Move to Spark" row per move, with no received row.
- Manual test 1: PASS. I used a local route-limit hook (
limit=250000), since the PR ships none. "Amount too large" offered Move Half at 196 478, the move paid ₿0, and Done offered Move Rest.
The commits since then (3341321, c5327fc) change only how the move's activity row is recorded and settled. I code-reviewed them, and unit tests cover them.
Not exercised, because no QA case covers them: the in-flight screen, "Could not move", and a multi-hop route with a real fee.
Evidence: .ai/gate_spark_1774_1794249927/ (screens, layout dumps, log excerpt).
Reviewer model: Claude Fable 5.1 (claude-fable-5-1), Claude Opus 5.5 (claude-opus-5-5)
Reasoning effort: not exposed
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 readyActivityServiceMoveToSparkTest.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.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.