Repository navigation
Conversation
|
Pushed |
Regtest APKDownload bitkit-dev-debug universal APK (expires in 30 days). |
|
Added to the 3.2.0 milestone: Spark via Breez SDK is the 3.2.0 highlight, and this PR stacks on #1697. |
ab34526 to
ab1b09d
Compare
Rebuilt on the split Spark head; carries the Pubky Swaps route (#1732) as it was merged into this branch.
c08023a to
f00fb41
Compare
# Conflicts: # gradle/libs.versions.toml
jvsena42
left a comment
There was a problem hiding this comment.
Delta review a7bef98e9..b22c2496c. My allowance-ledger MEDIUM is fixed in 590f925, and I confirmed it on the thread. b22c249 is clean.
One new MEDIUM, inline: the Android side is missing the fix iOS #1191 made in 2dcd5389c. It is reproduced by a unit test that fails at this head.
The base commits 09eac5b and 9f44c5b are reviewed on #1697.
Reviewer model: Claude Fable 5.1 (claude-fable-5-1), delta review; Claude Opus 5.5 (claude-opus-5-5), verification
Reasoning effort: not exposed
pwltr
left a comment
There was a problem hiding this comment.
Reviewed b22c249. Independent static General review identified 3 confirmed findings. Recommended outcome: Request changes.
High — Discard routing results when the active payment changes. app/src/main/java/to/bitkit/viewmodels/AppViewModel.kt:4693-4725. Continue a Spark-funded payment to contact A while prepareSend is delayed, dismiss the sheet, and open contact B before A's quote returns. routePaykitPayment retains A's endpoints across the suspension and then copies A's route/service leg into the current SendUiState, which now belongs to B. PaykitRouteRepo.route suspends while quoting Spark endpoints. ContentView.kt:566-603 permits dismissal during ordinary route loading; hideSheet increments sendStateGeneration and clears the contact context without cancelling observeSendEvents. openContactPayment runs through launchScan independently of the send-event collector and can initialize B while A's routing call remains suspended. The routing result checks neither sendStateGeneration nor the original contact/sheet before updating the current state. Later routing uses the overwritten A endpoints; proceedWithServiceRoutedPayment sends to leg.endpoint.value while using B's current contact context. SendConfirmScreen displays the current contact profile as TO. A stale asynchronous quote can bind A's destination to B's displayed contact and send B's payment to A. Capture and revalidate payment generation/context before applying the result or emitting confirmation navigation. Counterexamples checked: Normal completion without dismissal is safe. The serialized send-event collector does not prevent sheet dismissal or a separate launchScan from changing the active payment.
Medium — Connect uncertain contact withdrawals to their eventual Spark outcome. app/src/main/java/to/bitkit/viewmodels/AppViewModel.kt:6962-6968. An ordinary contact payment without an incoming payment request reaches Spark, but send throws after dispatch. Spark either already lists the withdrawal or lists it on a subsequent event. handleServiceLegFailure treats both Sent and Unknown as pending and calls showUnresolvedOnchainSend with a null txid and null request. showUnresolvedOnchainSend creates an on-chain pending route with an empty payment identifier and observeResolution=false. SendPendingViewModel.initOnchain cannot observe the Spark journal: it only tracks an LDK OnchainSendAttempt, and returns without finding activity when txid is null. SparkRepo.recordWithdrawal can save the successful activity and clear the journal, but emits no pending-screen resolution. The Paykit proof resolution observer cannot help an ordinary contact payment because startSparkOnchainProof creates no request proof for it. The screen remains Payment Pending indefinitely even after Spark knows the payment succeeded, with no transaction details target. Handle Sent immediately and provide an outcome observer for ordinary uncertain Spark withdrawals. Counterexamples checked: Incoming requests can resolve through their payment proof, and sends returning normally use onSendSuccess; neither safeguard covers this ordinary-contact failure path.
Medium — Reconcile expired withdrawal journals on subsequent successful syncs. app/src/main/java/to/bitkit/repositories/SparkRepo.kt:911-914. A withdrawal send fails without creating a payment. The immediate successful sync runs during the ten-minute grace period and correctly retains its journal. The user then keeps the app in the foreground past that period. syncActivities at lines 895-905 is the only path that drops an unlisted journal after its grace period. Its callers are start and the one immediate sync following a failed send. Subsequent SdkEvent.Synced handling only calls refresh; refresh and refreshState neither settle nor expire the journal. withdraw rejects every new withdrawal while pendingSparkWithdrawal is non-null. withdrawalOutcome continues reporting Unknown while the journal remains. An attempt that never sent blocks all later Paykit Spark withdrawals beyond the advertised grace period until another Spark start occurs, such as an app restart or lifecycle restart. Settle the journal after successful SDK sync events as well. Counterexamples checked: The added grace-period test explicitly calls start after advancing the journal age, so it verifies restart recovery but does not exercise continuous foreground operation.
Security review and runtime validation remain incomplete and held for fixes. No builds, tests, devices, services or installations ran. Native bitkit-core/Breez SDK internals and immutable counterpart compatibility were not independently verified; prior Security reuse was not established.
Review details
General review
Model: GPT-6.1 Sol
Reasoning effort: high
pwltr
left a comment
There was a problem hiding this comment.
Reviewed 20ac89e.
Medium — Unselected LNURL constraints block on-chain Spark withdrawals. In app/src/main/java/to/bitkit/viewmodels/AppViewModel.kt:4744-4748, selecting an on-chain Spark service leg leaves LNURL metadata attached. Amount validation still applies that unselected endpoint's minimum, and execution may fetch an LNURL invoice before the service withdrawal. A fundable on-chain payment can therefore fail because of another endpoint's limits or callback availability. Apply amount and invoice checks only to the selected route.
High candidate — Positive withdrawal evidence can be lost from recovery classification. In app/src/main/java/to/bitkit/repositories/SparkRepo.kt:1010-1016, a pending/successful SDK event saves activity and clears the withdrawal journal. If the send response is then lost and the payment list temporarily lacks the withdrawal, outcome classification can return Unsent without consulting the saved activity, releasing the request proof for retry. This could permit a duplicate payment. The triggering SDK event-versus-list consistency contract was not inspected or reproduced, so this remains a candidate. Preserve durable positive evidence until recovery can classify the payment safely.
Review details
General review
Model: GPT-6.1 Sol
Reasoning effort: high
… HEAD # Conflicts: # app/src/main/java/to/bitkit/repositories/SparkRepo.kt # app/src/test/java/to/bitkit/repositories/SparkRepoTest.kt
|
@ovitrif this conflicts with |
pwltr
left a comment
There was a problem hiding this comment.
Rechecked commit b905364. A Paykit route that returns after its payment was dismissed or replaced is now dropped, an uncertain Spark send to a contact now settles from Spark's withdrawal journal, and an expired journal is settled on every Spark sync. These address the earlier High and two Mediums.
Medium: a late Spark withdrawal settlement can take over an unrelated Send sheet (app/src/main/java/to/bitkit/viewmodels/AppViewModel.kt line 1183). The uncertain withdrawal recorded for the pending screen is not cleared when the sheet is closed; hideSheet clears the uncertain request ID but not this. If the user dismisses the pending screen and starts a different payment, the settlement from Spark's next sync only checks that some Send sheet is open, then shows the earlier withdrawal's success screen (clearing the current flow) or its failure, although the new payment was never sent. Clearing it in clearActiveContactPaymentContext, or applying the settlement only while that pending screen is shown, would fix it.
Review details
General review
Model: Claude Opus 5.5
Reasoning effort: high
|
@pwltr re #1754 (review):
|
|
@pwltr re #1754 (review):
|
jvsena42
left a comment
There was a problem hiding this comment.
Delta review of the seven commits since b22c2496c. The head has since only merged the base branch; AppViewModel.kt is unchanged at 370653f.
- My MEDIUM is fixed by 20ac89e. A withdrawal that Spark returns as FAILED now throws
WithdrawalNotSent. The proof is released, the allowance attempt fails, and nothing is reported as sent. A FAILED result that only shows up later through the journal or sync path reads asUnsentonce the journal closes. The failed txid also can't settle a retry. - One LOW is inline (4439a24), reproduced with a unit test.
- Checked and clean:
- 03f22e6: the pending screen moves to error only for its own request.
- d79cced: a stale route is dropped before it is applied.
- 5584e40: expiry is settled on each sync.
- 0c74a5b: an LNURL's limits stay off a Spark leg.
- 4bd0732: the journal is kept until Spark lists the send, so a list that lags behind never reads as
Unsent.
The contact-payment device gate passed earlier (gate_spark_1754).
Reviewer model: Claude Fable 5.1 (claude-fable-5-1), Claude Opus 5.5 (claude-opus-5-5)
Reasoning effort: not exposed
pwltr
left a comment
There was a problem hiding this comment.
Rechecked commit 370653f, a merge of the updated base branch. Its conflict in Spark's activity sync is resolved correctly: the base's re-read before releasing failed sends runs first, and the journal settlement still runs after it.
Medium (still open from b905364): a late Spark withdrawal settlement can take over an unrelated Send sheet (app/src/main/java/to/bitkit/viewmodels/AppViewModel.kt line 1183). The uncertain withdrawal recorded for the pending screen is still not cleared when the sheet closes, so after starting a different payment, the settlement replaces that payment's flow with the earlier withdrawal's success or failure. Clearing it in clearActiveContactPaymentContext, or applying the settlement only while that pending screen is shown, would fix it.
Review details
General review
Model: Claude Opus 5.5
Reasoning effort: high
|
@pwltr re #1754 (review) (also #1754 (review)): fixed in 55e4991. Closing the send sheet now clears the uncertain withdrawal the pending screen waits on, so a late settlement no longer takes over a Send sheet opened afterwards. Test: @jvsena42 your LOW at #1754 (comment) is fixed in 31298c4; reply in the thread. This push makes the approval stale. A re-approval is needed only for these two fixes (55e4991, 31298c4) plus the earlier base merge at 665641e (cac9d12). |
jvsena42
left a comment
There was a problem hiding this comment.
Delta review of 55e4991 and 31298c4: no findings.
- 31298c4 closes my LOW; thread resolved.
- 55e4991 clears
uncertainSparkWithdrawalinclearActiveContactPaymentContext, sohideSheetdrops a late settlement before a new Send sheet can take it. That closes pwltr's thread. None of the clearing paths runs while the pending screen is still shown.
Reviewer model: Claude Fable 5.1 (claude-fable-5-1), Claude Opus 5.5 (claude-opus-5-5)
Reasoning effort: not exposed
Closes #1807
Twin: synonymdev/bitkit-ios#1191
Dependency:
This PR routes Paykit contact payments through bitkit-core's Paykit router, so a wallet whose spending balance sits on Spark pays contacts directly over Lightning, or through a Spark withdrawal to a contact who only accepts on-chain bitcoin.
Description
paykitRoutePayment) how to pay: on amount Continue, when a payment request opens its confirm screen directly, and when the source is switched on a request's confirm screen. The router gets the contact's full endpoint list and what the selected source can spend:Directpays the endpoint it names: Lightning (bolt11 first, then LNURL) or the on-chain address.Serviceopens the confirm screen withRoute: Sparkand the leg's fee in the fee row. The swipe withdraws exactly the amount to the contact's address, and the contact receives exactly the amount entered.Noneshows "Insufficient balance" when the amount exceeds what the source can spend, else "Can't pay this contact"; the router's reason goes to the log.SparkRepo.withdrawalRouteLeg).0.8.0-spark.3, a prerelease of feat: pay contacts via spark bitkit-core#193.What it needs to ship
Out of Scope
Design
N/A — no design available.
Preview
A wallet whose spending balance is held on Spark pays a contact who accepts only on-chain bitcoin. The router picks a Spark withdrawal, the confirm screen shows
Route: Spark, and the contact receives exactly 20,000 sats on Spark's regtest chain (2x speed).android-spark-j12x.mp4
QA Notes
Setup
bitkit://contact?pubky=<pubky>)../lsp POST /regtest/chain/deposit(cases 10, 11).mainwith the marketplace and payment-requests profiles, itsfixture-issueras the contact, and the app built withE2E=true E2E_BACKEND=local E2E_LOCAL_HOST=127.0.0.1 E2E_HOMESERVER_PUBKY=$(./pubky-marketplace info | jq -r .homeserver_z32)andadb reverse6286, 6287, 6288, 15411, 15412, 60001, 23735, 23000 and 23010.Journeys
paykit-spark-route.xml— a wallet whose spending balance is on Spark pays an on-chain-only contact through a Spark withdrawal, and the contact receives exactly 20,000 satsManual Tests
Two contacts as above, both Spark only (no channel); either one can play payer or payee.
./lsp POST /regtest/chain/deposit): Spending is the default source; switching to Savings pays the contact's on-chain endpoint, which is the payee's Savings address, and the payee's receive is linked to the payer.Route: Sparkwith the withdrawal's fee, the withdrawal pays exactly the amount to the payee's address, and the payee's request shows "Proof submitted".Evidence
Each case ran with dev debug builds on two regtest Android emulators (API 35), with Spark on Lightspark's hosted regtest and no channel. Both wallets were Spark only, except in cases 10 and 11, where the payer's Savings was funded too.
1cbb4b6c9: 300 sats paid; the payer showed Sent 301 (fee 1) to the contact with its preimage, and the payee showed Received 300 from the contact. Again through the router, Direct bolt11: 120 sats atbd755b839, and 110 sats at25b40e0fdon bitkit-core0.8.0-spark.3.case 1
case 1 on spark.3
1cbb4b6c9: after the payee's relaunch the payer opened list version 5 after 0 and paid 150 sats.f11-rotate2x.mp4
1cbb4b6c9: the force-stopped payee received 250 sats; on relaunch Home showed Received 250 from the contact and no duplicate rows.case 3 payer
case 3 payee
1cbb4b6c9: a 200-sat request was paid, and the payee showed "Proof submitted".case 4 payee
case 4 payer
bd755b839: both sides showed Rejected.f5-reject-payer2x.mp4
bd755b839: the 300-sat request was paid automatically ("Paid automatically $0.23"); a 2,000-sat request was kept manual ("the allowance limit is reached"); after End Allowance, a 200-sat request was presented for manual payment.case 6 payer
case 6 payee
1cbb4b6c9: with 4,473 sendable, 6,000 and 44,731 could not be entered; 4,473 was paid, quoted at ± ₿ 12.f9-above2x.mp4
bd755b839: two kills before Spark dispatched left the balance unchanged and the request payable; a kill after dispatch could not be timed, since Spark sends finish in under a second.case 8
case 8 relaunch
25b40e0fd: the first 100-sat period was paid from Spark and its proof queued; after a force-stop and relaunch the payer listed the subscription as Active, renewing the next day. Later periods not observed yet.25b40e0fd: from Savings, the router paid the payee's on-chain endpoint 15,000 sats at Normal (₿ 141); the payee showed Received 15,000 from the contact in Savings.f4-onchain2x.mp4
47a1484a8's fee fix: switching to Spending showedRoute: Spark; Spark's withdrawal365dc1df…paid exactly 2,000 sats to the payee's address and confirmed on Lightspark's chain (the payee's node runs on staging's chain, so its Savings doesn't show it); the proof was delivered and the payee showed "Proof submitted"; the payer's activity showed the contact. The confirm screen quoted ± ₿ 750 while Spark charged 2,190, and the activity read as a transfer: both fixed in47a1484a8.f5-onchain-payer2x.mp4
47a1484a8: the confirm screen opened from Spending at Instant (± ₿ 1); 200 sats sent and received.f5-router-request22x.mp4
47a1484a8: Spark refused the send before it left ("Spark could not prepare the payment"), the payment-failed screen showed, and after reconnecting no payment had been made.f9-offline2x.mp4
28a63f8on bitkit-core0.8.0-swaps.3, before the move to the router: Spark's withdrawal56263be8…paid exactly 20,000 sats to the contact. Not rerun since.Paykit flows with a Spark balance
Flows from the v0.1 specs at pubky/paykit-rs
cb8cba5. State a is Spark only, with no channel. State b is a channel too small for the amount, with Spark covering the rest. The manual case numbers above give the evidence.LightningRepoTest,PaykitAllowanceRepoTest).0.8.0-spark.3; ⏭️ public endpoint, because both test wallets hold a private link, which is paid first.28a63f8before the router.Automated Checks
AppViewModelSendFlowTestcases: router Direct Lightning, Direct on-chain from Savings, Service leg, None with a short balance and None with an unpayable contact; the Spark leg runs only from Spending; a request's confirm screen pays the endpoint the router picks; an on-chain request paid through the Spark leg starts and completes its proof, and a failed leg releases an unsent proof; a source switch on confirmation reroutes; Spark leg fees count in the warnings; the confirm tags reach the withdrawal; an on-chain request Savings can't cover goes to SparkPaykitPaymentStateBackupTestcase: a Spark withdrawal proof keeps its rail through backupPaykitPaymentProofRepoTestcases: an on-chain proof completed with a Spark withdrawal's txid, one settled from Spark's payment list on reconcile, and one released when its withdrawal never sentSparkRepoTestcases: a withdrawal quoted as a route leg, no leg the balance cannot cover, a withdrawal recorded with its contact and fee, a second withdrawal refused while one is unsettled, a journaled withdrawal settled or dropped on start, a withdrawal refused right before it sends or above the confirmed debit, the journal kept when the activity fails to save, an earlier withdrawal never settling a new attempt, tags on a recovered withdrawal, a contact withdrawal not marked as a transfer, and kept off the Spark screen's transfersjust compile,just test,just lintand the androidTest compile on bitkit-core0.8.0-spark.3: passMainnet 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: the flows above ran on regtest.
Round 2 (pending)
On the final heads, two mainnet wallets that are each other's Paykit contacts, small amounts.
Models used
claude-opus-5-5(reasoning:medium),claude-sonnet-5-5(reasoning:high)claude-opus-5-5(reasoning:medium)Taken from the run logs of every session and subagent that planned or wrote this PR, which record the model and reasoning effort per turn. The
claude-sonnet-5-5planning runs were research and spec-mapping subagents. No independent agent review was run on this PR.