Skip to content

fix: align receive liquidity - #1248

Open
pwltr wants to merge 6 commits into
masterfrom
fix/receive-liquidity-parity
Open

pwltr wants to merge 6 commits into
masterfrom
fix/receive-liquidity-parity

Conversation

@pwltr

@pwltr pwltr commented Sep 10, 2026

Copy link
Copy Markdown
Contributor

Description

Follow-up from synonymdev/bitkit-ios#711 (comment)

This PR:

  1. Aligns Receive Lightning invoice eligibility with the shared receive-liquidity spec by using ready-channel inbound capacity for normal invoice creation, Auto availability, and Spending CJIT routing.
  2. Clears receive-session CJIT invoice/details state when editing or starting a fresh Receive session so immutable CJIT invoices are not reused after amount changes, including CJIT QR edits that can return to a normal BOLT11 invoice when the new amount fits inbound liquidity.
  3. Validates CJIT quotes before confirmation UI and maps node-wide Blocktank capacity failures to a separate user-facing error instead of the per-channel maximum message.
  4. Updates the platform-agnostic receive-liquidity spec and focused unit coverage for the receive decision matrix, CJIT quote validation, CJIT session state, and Blocktank error classification.

Design

Mapped frames in docs/screens-map.md:

  • EditInvoiceScreen.kt: Send (Contact) (Lightning) › Edit invoice populated
  • ReceiveAmountScreen.kt: Receive CJIT Onboarding › CJIT Enter amount
  • ReceiveConfirmScreen.kt: Send (Enter Manually) › CJIT Fee to connect
  • ReceiveQrScreen.kt: Receive Lightning Active › Receive Auto (Unified QR)

Preview

Screen.Recording.2026-09-10.at.19.48.49.mov

QA Notes

Manual Tests

  • 1. Receive → Spending CJIT QR → Edit Invoice with an amount that fits ready inbound liquidity → QR: returns to Spending, shows the newly generated normal LN invoice, and does not show the previous CJIT invoice.
  • 2. Receive → Spending CJIT QR → Edit Invoice with an amount that still needs additional liquidity → Confirm → QR: shows a fresh Spending-only CJIT invoice and does not expose the old CJIT invoice.
  • 3. Receive → create or view CJIT QR → close sheet → reopen Receive: starts from fresh Receive state with no CJIT invoice/details preserved.
  • 4. regression: ready channel with inbound > 0 but peer not yet usable → Receive: normal LN invoice/Auto availability follows ready inbound capacity.

Automated Checks

  • Unit tests added: CjitQuoteValidatorTest.kt covers invalid fee/net quote cases and a valid quote.
  • Unit tests added: CjitEntryDetailsTest.kt covers confirmation-entry mapping and quote rejection before confirmation UI.
  • Unit tests updated: ReceiveInvoiceEditStateTest.kt covers clearing stale CJIT invoice state on edit and fresh CJIT creation.
  • Unit tests updated: ReceiveLiquidityDecisionTest.kt and WalletRepoTest.kt cover ready-channel receive eligibility and ready inbound liquidity.
  • Unit tests updated: BlocktankRepoTest.kt covers node capacity error classification separately from per-channel max-size errors.
  • Local checks:
    • ./gradlew testDevDebugUnitTest --tests to.bitkit.models.ReceiveLiquidityDecisionTest --tests to.bitkit.models.CjitQuoteValidatorTest --tests to.bitkit.ui.screens.wallets.receive.ReceiveInvoiceEditStateTest --tests to.bitkit.ui.screens.wallets.receive.CjitEntryDetailsTest --tests to.bitkit.repositories.WalletRepoTest --tests to.bitkit.repositories.BlocktankRepoTest
    • ./gradlew compileDevDebugKotlin

@pwltr
pwltr force-pushed the fix/receive-liquidity-parity branch from 884ef2e to f604acd Compare September 10, 2026 17:36
@pwltr
pwltr marked this pull request as ready for review September 10, 2026 17:46
@greptile-apps

greptile-apps Bot commented Sep 10, 2026

Copy link
Copy Markdown

RetriggerConfidence Score: 5/5

The PR appears safe to merge, with the receive-liquidity, CJIT lifecycle, validation, and error-presentation changes remaining internally consistent.

Summary

  • Uses ready channels consistently for normal Lightning invoice eligibility, Auto availability, and additional-liquidity decisions.
  • Clears old CJIT invoice and quote details when editing or beginning a fresh receive session.
  • Rejects CJIT quotes that cannot produce a valid positive net receive amount.
  • Adds dedicated presentation for node-wide capacity failures and focused tests for the updated decision paths.

Diagram

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[Open Receive] --> B[Clear receive-session CJIT state]
    B --> C{Ready inbound capacity covers amount?}
    C -->|Yes| D[Create normal Lightning invoice]
    D --> E[Show Auto or Spending QR]
    C -->|No| F{Spending receive needs CJIT?}
    F -->|No| G[Show Savings/on-chain request]
    F -->|Yes| H[Request CJIT quote]
    H --> I{Quote valid?}
    I -->|No| J[Show quote or capacity error]
    I -->|Yes| K[Show CJIT confirmation]
    K --> L[Confirm fresh CJIT invoice]
    L --> M[Show Spending-only CJIT QR]
    M -->|Edit| N[Clear immutable CJIT state]
    N --> C
Loading

Reviews (1) · Last reviewed commit: "fix: align receive liquidity with spec"

@jvsena42 jvsena42 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Funds-focused review. No sats are at risk anywhere in this PR, and three of the four iOS-parity gaps I'd recorded for Android are closed:

  • validateCjitEntry twin — closed. CjitQuoteValidator (models/CjitQuoteValidator.kt:17-24) rejects feeSat >= invoiceSat before subtracting, then channelSizeSat < invoiceSat - feeSat, all in ULong — the guard makes the subtraction underflow-proof. Placement is what matters and it's right: it runs inside BlocktankRepo.createCjit (:283-287) right after the only coreService.blocktank.createCjit call in the app, and both UI callers go through BlocktankViewModel.createCjit → repo. CjitEntryDetails.from (ReceiveConfirmScreen.kt:208-223) is the only production constructor of the confirm model, so the old signed-Long path at :83 can no longer render a negative "you will receive".
  • Stale CJIT after editing, ready-channel half — closed, by a different mechanism than iOS: clearing eagerly rather than replacing the route on completion. That eagerness is the problem inline.
  • Stale channel cache in canCreateLightningInvoice — closed, not reintroduced. currentChannels() (WalletRepo.kt:771-773) reads live getChannels() when the node is running; this PR only swaps isUsableisChannelReady (:776), and calculateRemoteBalance() already filtered on isChannelReady, so the liquidity number is consistent.

Also checked clean: the hardware address is only emitted under ReceiveTab.TREZOR and CJIT only under SPENDING (ReceiveInvoiceUtils.kt:40-52), with AUTO hidden while a CJIT invoice exists — no wrong-destination display. All new arithmetic is ULong behind the underflow guard. Session reset per presentation is guaranteed by key(receiveSheetPresentationKey(sheet)) plus LaunchedEffect(startRoute) { clear() }.

Still open at LOW — zero-inbound first-channel half. With no channels, Edit on the initial CJIT QR → Continue returns None via (inboundCapacitySats ?: 0uL) == 0uL (ReceiveLiquidityDecision.kt:45), so the CJIT is cleared, the typed amount only reaches the bip21, and ReceiveQrScreen.kt:211-217 flips to Savings. "Receive on Spending" then opens ReceiveAmountScreen empty — it uses a fresh hiltViewModel() AmountInputViewModel whose only setSats calls are the max clamp and the min button. Same ~3-line fix as I suggested on iOS: prefill from walletState.bip21AmountSats in a LaunchedEffect, since both the None and ChooseAmount branches already persist the typed amount there. (Not in the diff, so noting it here.)

Requesting changes for one regression — inline. It's in core receive rather than Paykit, so it isn't gated.

Comment thread app/src/main/java/to/bitkit/ui/screens/wallets/receive/ReceiveSheet.kt Outdated
Comment thread app/src/main/java/to/bitkit/ui/screens/wallets/receive/EditInvoiceScreen.kt Outdated
@pwltr
pwltr requested a review from jvsena42 September 11, 2026 13:19

@jvsena42 jvsena42 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Finding A is fixed79dd4c3bf drops the start-of-edit clear (beginReceiveEdit() is gone from :155-164 and deleted) and clears on completion in updateInvoice (:365-368). Lifting my REQUEST_CHANGES.

I walked every path in EditInvoiceScreen.kt:116-154 to check the stale-invoice bug this PR originally fixed stays closed:

Path Edit applied? CJIT
None yes cleared via updateInvoice → the "edit down with a ready channel" bug stays closed
ChooseAmount yes cleared, then Amount
CreateCjit success on Confirm onCjitConfirmed sets the new invoice
CreateCjit failure / GeoBlocked no old, still-valid CJIT kept — correct
Trezor on-chain edit on-chain only kept — correct
Back from Edit without applying no kept — the bug is fixed

Per-presentation scoping still holds: cjitSessionState is still remembered inside ReceiveSheet (:81) under key(receiveSheetPresentationKey(sheet)), with a fresh UUID per Sheet.Receive, and no CJIT state moved into WalletRepo/WalletViewModel. The only new repo writes are bip21AmountSats/bip21, which every sheet open resets via refreshReceiveState(), so nothing leaks into the next presentation.

B is addressedupdateOnchainBip21Amount keeps the Savings QR on the approved amount. C is addressedgetOrThrow() now sits inside runSuspendCatching (EditInvoiceScreen.kt:133-135). The zero-inbound prefill is still open, but I'd marked it optional.

Three LOW notes inline, none blocking.

Comment thread app/src/main/java/to/bitkit/ui/screens/wallets/receive/ReceiveSheet.kt Outdated
Comment thread app/src/main/java/to/bitkit/repositories/WalletRepo.kt
@jvsena42
jvsena42 dismissed their stale review September 11, 2026 16:44

Finding A is fixed in 79dd4c3 and nothing blocking remains. Remaining notes are non-blocking — see the latest review.

@pwltr
pwltr requested a review from jvsena42 September 14, 2026 16:33
@github-actions

github-actions Bot commented Sep 14, 2026

Copy link
Copy Markdown
Contributor

Regtest APK

Built from 69a076b (run).

Download bitkit-dev-debug universal APK (expires in 30 days).

@jvsena42 jvsena42 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-reviewed at dff7f789b, and I re-derived the whole path table from the code rather than leaning on my previous sign-off — the commit changes exactly the mechanism I signed off on last time, and the skill I work from is explicit that a safety net a prior review praised is an unverified claim until stressed again.

The regression I flagged is fixed. Confirm -> Back now leaves everything alone: onCjitCreated (:447-449) no longer nulls cjitInvoice, navigateReceiveConfirm (:396-397) is synchronous again and no longer touches bip21, and the bip21 write moved into ConfirmIncreaseInbound.onContinue (:317). So declining a replacement quote leaves the old CJIT intact and the QR recomposes onto Spending showing it.

Re-derived table at head:

Path Edit applied Existing CJIT Payer-visible QR stale?
None yes cleared no — re-derived from bip21 at the new amount
ChooseAmount yes cleared, then Amount no
CreateCjit -> Confirm at Confirm only kept until :318, then replaced no — fee, "you will receive" and entry.invoice all come from the same entryDetails object
CreateCjit -> Confirm -> Back no kept no — the fix
CreateCjit failure no kept no
GeoBlocked no kept no
Trezor on-chain edit on-chain only kept no
Back from Edit unapplied no kept no — resetEditInvoiceAmount() only writes to the edit view model, never to wallet state
Sheet dismissed at any point no lost with the presentation by design; and dismissing at Confirm now leaves bip21AmountSats/bolt11 untouched
System Back at Confirm no kept no
Config change mid-flow n/a lost see the second inline note

No completing or cancelling path replaces or destroys a displayed CJIT before the user confirms the replacement. Nothing moved CJIT state into WalletRepo/WalletViewModelremember at :80 under key(receiveSheetPresentationKey(sheet)) still scopes it per presentation, and the one new repo write (setBolt11("")) is regenerated by refreshBip21() on every sheet open.

Two findings inline, both reproduced on a Pixel 9 against the PR-head CI build rather than argued from code. One is MEDIUM and is why I am requesting changes; the other is LOW and optional. Both are pre-existing symptoms rather than regressions — I have said so explicitly on each, and on the MEDIUM I have offered to drop the block if you would rather take it separately.

Also checked and clean:

  • Approve-time display vs encoded invoice. ReceiveConfirmScreen.kt:82-93 computes fees and "you will receive" from entry, and :110 passes entry.invoice; :316-322 reads receiveAmountSats off the same let-bound object, so the screen and the lambda recompose together and cannot disagree.
  • Savings vs Spending after an additional CJIT. :317 sets Savings to the exact amount the quote was priced for and WalletRepo.kt:636 clears the stale-amount bolt11; getInvoiceForTab prefers cjitInvoice and AUTO is hidden while one exists, so no tab can show the old-amount bolt11.
  • Arithmetic. No new signed-Long subtraction. CjitEntryDetails.from is still the only production constructor and still runs CjitQuoteValidator.validate before the Long fields exist.
  • Partial failure. CreateCjit failure touches neither cjitInvoice nor entryDetails.
  • Double-tap. Navigation is synchronous again, which closes the window I flagged last round. _isLoading is set on Main.immediate before the first suspension, PrimaryButton is enabled && !isLoading with a 500 ms debounce. The residual — a tap during the 300 ms exit transition starting a second createCjit — has its LaunchedEffect cancelled when Edit leaves composition, so nothing reaches onCjitCreated; worst case is an orphan quote at the LSP.
  • Fire-and-forget updateOnchainBip21Amount. WalletViewModel.kt:522 is now viewModelScope.launch. The only concurrent writer is refreshBip21() on a new presentation, which needs dismiss-and-reopen inside the DataStore write window; SCREEN_TRANSITION_DELAY in showSheet widens that further. Not reachable by a human.
  • setBolt11("") vs sheet auto-close. observeReceiveSheetInvoice (AppViewModel.kt:550) returns early on an empty bolt11, so receiveSheetContext keeps the previous value and auto-close behaviour is unchanged.
  • Conventions. runSuspendCatching kept at WalletRepo.kt:634; Logger.error(..., context = ...); no runBlocking, no !!, no System.currentTimeMillis().
  • ReceiveInvoiceEditStateTest.kt:70-86 now pins keep-until-confirm at the state level.

Gating: core receive, no feature flag, so severity is assigned without any gating discount.

@pwltr
pwltr requested a review from jvsena42 September 15, 2026 10:35

@jvsena42 jvsena42 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-reviewed at a2a1edf74. The two new commits are f1f088ad (the updateOnchainBip21Amount call I asked for on :303) and a2a1edf74 (the unchanged-edit guard). f1f088ad is correct. The guard in a2a1edf74 has a bug inside it — one MEDIUM below, plus a LOW on the same branch.

Neither finding reverses anything: keep-until-confirm (dff7f789b) and the :303 call stay exactly as they are. The MEDIUM is a defect in the new guard, not an argument against having one.

Checked and clean (exits walked at head, and what keeps each correct):

Exit Result
QR → Edit → system/gesture Back onBack = popBackStack (:376); resetEditInvoiceAmount() writes only to the edit VM. CJIT and bip21 untouched
Edit → CreateCjit → Confirm → Back onCjitCreated (:451) only sets entryDetails; navigateReceiveConfirm touches no wallet state. Displayed CJIT and Savings amount unchanged
Edit → CreateCjit failure / GeoBlocked navigateCjitAmount / navigateGeoBlock; nothing cleared; both Backs are plain pops
Confirm → Continue (both routes) fee, net and entry.invoice all read off the same let-bound entryDetails; popUpTo(QR) inclusive makes a double-tap inert
Liquidity / LiquidityAdditional → Back or Continue both popBackStack; entryDetails retained
Sheet swipe-dismiss → reopen fresh presentationIdkey(...) recreates the sheet → fresh ReceiveCjitSessionState + LaunchedEffect(startRoute) { clear() } + clearBip21State. No CJIT state lives in WalletRepo/WalletViewModel
Backgrounding without death a ChannelReady regenerates bolt11, but Spending still prefers cjitInvoice and AUTO stays hidden while one exists

Also verified: CjitQuoteValidator runs twice (in createCjit and again in CjitEntryDetails.from) before any Long conversion, and net receive is guarded by feeSat < invoiceSat; ReceiveCjitErrorPresenter renders only string resources, no server text; refreshCjitEntries still has the correct withTimeout + explicit CancellationException/TimeoutCancellationException guard while createCjit uses runSuspendCatching. BlocktankRepo, WalletRepo, the validator and the decision table are unchanged since dff7f789b and still look right.

Process death mid-session is still out of scope — you deferred it earlier and I'm not reopening it.


Non-blocking, separate from the findings: ### Design says N/A — no design available., but all four screens this PR touches are mapped to real frames in docs/screens-map.mdEditInvoiceScreen.kt, ReceiveAmountScreen.kt, ReceiveConfirmScreen.kt, ReceiveQrScreen.kt (lines 192-197), none of them todo or n/a. Link the relevant frames and I'll diff the new error states against them next pass. Doesn't block anything.

Comment thread app/src/main/java/to/bitkit/ui/screens/wallets/receive/ReceiveSheet.kt Outdated

@jvsena42 jvsena42 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

69a076b47 fixes the MEDIUM and the LOW. Clearing my REQUEST_CHANGES — details on the two threads.

The repro is closed at the guard itself: confirmedAmountSats is snapshotted in onCjitConfirmed from the same entryDetails both Confirm routes write their bip21 amount from, and clear() nulls it alongside cjitInvoice. A declined replacement quote now leaves entryDetails desynced harmlessly, because the guard no longer reads it.

I spent most of this pass on the mirror bug rather than the one I reported, since a guard fixed in response to review is exactly where the next defect lands — that is how the original got in. hasConfirmedInvoiceForAmount returning false for the genuinely confirmed amount would destroy a CJIT invoice the user has already shown a payer, so I traced every writer of bip21AmountSats that could make the Edit prefill diverge from the snapshot: setSats stores sats verbatim and switchUnit only rewrites display text (no fiat rounding drift on a prefilled untouched amount), and the only in-sheet trigger of refreshBip21/clearBip21State is the notification switch on Confirm/Liquidity, which fires before Confirm's own amount write. Inbound liquidity growing mid-session is no longer relevant either, now that the guard is pure amount equality. Nothing diverges.

Exits re-walked at the new head, since the control flow changed shape: Edit toolbar/system/gesture Back; Confirm → Back after a declined replacement quote, and the mirror where the confirmed amount is re-applied afterwards; an Amount-created quote declined twice back to QR; Liquidity and LiquidityAdditional Back and Continue; CreateCjit failure and GeoBlocked; system Back on the QR root; swipe and scrim dismiss then reopen (fresh presentationId → fresh session state → refreshBip21 reset); config change (all three fields are remembered so they die together — no partial state, and the CJIT loss there is the limitation you explicitly deferred, not reopening it); process death; backgrounding without death.

The test is load-bearing: reverting ReceiveSheet.kt to a2a1edf74 while keeping the new test fails receive CJIT session matches confirmed amount when pending quote differs; at head 10/10 pass. Throwaway worktree outside the repo, nothing committed.

Docs line matches the code. One note for whoever merges: #1231 renames ReceiveTab.TREZOR on lines immediately adjacent to the ones this PR rewrites in ReceiveQrScreen.kt and ReceiveSheet.kt, so expect a textual conflict — no semantic conflict, and ReceiveInvoiceEditStateTest.kt hunks are disjoint.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants