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. |
Codex Review SummaryThis 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. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d8b7d32c21
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
[Critical impact] The PR adds an explicit path for moving users’ Lightning funds into Spark. The PR appears safe to merge, with a non-blocking duplicate-error notice to fix. Findings
SummaryAdds a flow to move channel funds to the user’s Spark wallet over Lightning.
Acknowledged by Diagram%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Open Move to Spark] --> B[Create Spark invoice]
B --> C[Quote amount and routing fee]
C --> D[User swipes to move]
D --> E[Pay through LDK with fee cap]
E --> F{Payment result}
F -->|Succeeded| G[Refresh wallet]
G --> H{Funds remain?}
H -->|Yes| I[Offer Move Rest]
H -->|No| J[Offer channel close]
F -->|No route| K[Offer Move Half]
F -->|Other failure| L[Offer Try Again]
F -->|Wait timed out| M[Show in flight without close]
Reviews (1) · Last reviewed commit: "chore: format move to spark repo" · Reviewed by Greptile |
talosmachina
left a comment
There was a problem hiding this comment.
No findings. Adds Move to Spark: quotes and pays a Spark invoice over the LDK channels only (payInvoiceOverLdk, fee capped at the quoted fee), waits up to 2 min for settlement, mutes the payment's own send/receive notices, and surfaces it from a one-time timed sheet, Spending and Lightning Connections. Reviewed d8b7d32, full tier, against feat/breez-spark-lightning.
What I checked, and 7 candidates I ruled out
Read in full: MoveToSparkRepo.kt, MoveToSparkViewModel.kt, MoveToSparkTimedSheet.kt, the MoveToSparkScreen.kt Done branch, the LightningRepo, AppViewModel and NotifyPaymentReceivedHandler hunks.
Call sites traced: payInvoiceWithLdk to LightningService.send (sendUsingAmount with routeParametersWithFeeCap), estimateRoutingFeesForAmount, nodeEvents (LDK and onSparkEvent both emit into it), NotifyPaymentReceived.Command.from from both AppViewModel and LightningNodeService.
Not built here: auto reviews of this repo are static; CI is the gate.
CI at review time: build-and-test, lint, detekt, build-local green; the build gate red only because its run was cancelled (BUILD_RESULT: cancelled); e2e shards and compose-ui still running.
Ruled out
- Settlement event missed: the
settledcollector startsUNDISPATCHEDbefore the send, so a fastPaymentSuccessfulis not lost. - Fee over what the user saw: the cap is
feeSats * 1000msat andfeeSatsis the quoted fee, so LDK refuses a route above it and that reads asTooLarge, never as an overpay. - Amount + fee over the outbound limit: a quote is kept only when
amountSats + feeSats <= limitSats; later rounds fall back to the last fitting quote. - Failure toast doubling the screen's error:
isWatchedMovePaymentmutes it while the screen waits; after a timeout the app announces a late failure, matching theInFlightcopy. - Received sheet for the Spark side: skipped in
shouldShowLightning, which both the foreground and the background service go through. - Close offered while funds may still return: the
InFlightbranch shows OK only. - Spark fallback paying from Spark to Spark:
payInvoiceOverLdkandestimateLdkRoutingFeesForAmountbypass the rail selection entirely.
Merge confidence: 4/5, move() / payAndWait (the path that sends funds) has no test, only quote and the math do, and e2e was still running.
Reviewer model: claude-opus-5-5
Reasoning effort: Unknown (the runtime does not report it)
|
Pushed
#1802 now holds only the missing unit tests. |
|
Triage: stays in 3.2.0. This PR closes #1768, which is already in the 3.2.0 milestone; now that this PR is ready for review, the issue moved to 👀 Review on the project. Twin: synonymdev/bitkit-ios#1212. |
|
Pushed |
… feat/move-to-spark
This reverts commit 591d3c0.
|
Pushed |
|
Cancelled and re-ran the failed e2e jobs of https://github.com/synonymdev/bitkit-android/actions/runs/42273241535 so they check out synonymdev/bitkit-e2e-tests#297 at |
… feat/move-to-spark
… feat/move-to-spark
|
e2e on |
… feat/move-to-spark
… feat/move-to-spark
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 existingshouldPayWithSparkrouting is unchanged.Out of Scope
move()andMoveToSparkViewModelunit tests — 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.
Screens
Pixel 6 emulator at 360 dp width, in flow order. Bitkit draws dark only, its system bars included, and no screen here shows a keyboard or system sheet.
Preview
The recordings below predate the small-screen pass, so they show earlier labels and headlines; the Screens section shows the current ones.
Sheet, too large and a partial move on the local Spark regtest (2x):
f-a1-sheet.mp4
f-a1b-once.mp4
f-a4-too-large.mp4
Moving the rest, closing, and a wallet without a channel (2x):
f-a2-move.mp4
f-a3-close.mp4
f-a5-no-channel.mp4
QA Notes
Setup
regtest/local(docker compose -f regtest/local/docker-compose.yml up), which runs the operators, the SSP with its Lightning node, an LNURL server, data sync, electrs and a chain API.SparkService, the stack's Spark config (from the stack'sspark-config.json), its chain service, LNURL domain and sync URL, and sets Electrum to the stack's electrs. Missing prerequisite for reviewers: that patch, or a shared way to point dev builds at a local Spark stack.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, maps no route to too large, refuses with nothing to move or Spark not readyAppViewModelSendFlowTest.kt— wires the new sheet and repositoryEvidence
Tested at
927472a4e(this PR's feature commit; later commits change only the journey text), dev regtest build with the local-only patch from Setup, Pixel 6 API 35 emulator, one 400,000 sat channel to the SSP:J1 sheet: on a cold launch the sheet shows on Home; Later closes it, and after a relaunch it does not show again.
1: with a local 150,000 sat route-limit hook, ₿ 392,993 and ₿ 196,496 showed "Amount too large"; ₿ 98,248 moved with a ₿ 0 routing fee, and Done offered Move The Rest.
J1 move: without the hook, Move To Spark in Spending moved the rest, ₿ 296,719, with a ₿ 0 routing fee (direct channel to the SSP); Done offered Close Connection.
J1 close: Close Connection, then Close, returned to Lightning Connections with no open connection, and Spark held ₿ 394,967, the sum of both moves.
J1 no channel: a new wallet showed no Move To Spark button and no sheet.
The Lightning Connections entry button was driven on an earlier build of this change: it moved ₿ 77,967 and closed.
just compile,just lintandjust testpass.Small-screen pass at
62526d3bf: the sheet, too large, confirm, partial and full Done screens captured at 360 dp width; every button label stays on one line and the buttons sit at the bottom.just compile,just lintandjust testpass.Rebased onto the rebuilt Spark base
ab1b09d14(without the emergency exit) at967292c09:just compile,just lintandjust testpass. The device runs above ran before this rebase; the Move to Spark code is the same apart from checkingisReadywhere it checkedisUsable, which included the removed exit state.Independent review fixes at
d8b7d32c2, merged with the Spark base2f483bd14(route-only too large, fee cap at the shown fee, no Close while in flight, background notice gate, move guard):just compile,just lintandjust testpass.Bot review fixes at
f715f9da2(a move failure matched by payment id, raw route errors, the Connections button reading live channels, no second failure notice):just compile,just lintandjust testpass.Gaps
The review fixes were not driven on a device; J1 and 1 ran on earlier heads, as listed above.
Only direct channels to the SSP were tested; a multi-hop route with a real routing fee was not set up locally.
Activity shows the move as a single received payment (see Out of Scope).
Mainnet runs on phones come at polish time.
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.