fix: align receive liquidity - #1248
Conversation
884ef2e to
f604acd
Compare
|
jvsena42
left a comment
There was a problem hiding this comment.
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:
validateCjitEntrytwin — closed.CjitQuoteValidator(models/CjitQuoteValidator.kt:17-24) rejectsfeeSat >= invoiceSatbefore subtracting, thenchannelSizeSat < invoiceSat - feeSat, all inULong— the guard makes the subtraction underflow-proof. Placement is what matters and it's right: it runs insideBlocktankRepo.createCjit(:283-287) right after the onlycoreService.blocktank.createCjitcall in the app, and both UI callers go throughBlocktankViewModel.createCjit→ repo.CjitEntryDetails.from(ReceiveConfirmScreen.kt:208-223) is the only production constructor of the confirm model, so the old signed-Longpath at:83can 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 livegetChannels()when the node is running; this PR only swapsisUsable→isChannelReady(:776), andcalculateRemoteBalance()already filtered onisChannelReady, 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.
jvsena42
left a comment
There was a problem hiding this comment.
Finding A is fixed — 79dd4c3bf 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 addressed — updateOnchainBip21Amount keeps the Savings QR on the approved amount. C is addressed — getOrThrow() 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.
| private set | ||
|
|
||
| fun onCjitCreated(entry: CjitEntryDetails) { | ||
| cjitInvoice = null |
There was a problem hiding this comment.
Low, and a both-platforms follow-up rather than something to fix here alone: Confirm→Back after an Edit-driven CJIT still loses the CJIT the user already showed, and now leaves Savings at the declined amount.
A CJIT QR at 20k is shown to a payer → Edit → 100k → Continue → CreateCjit → navigateReceiveConfirm (:384-390) runs updateOnchainBip21Amount(100k), then onCjitCreated nulls cjitInvoice here — both before the user confirms. They see the fee, decline, Back → Back. The QR comes back with cjitInvoice == null: ReceiveRoute.QR is a data object, entryDetails now holds the declined quote, and cjitEntries is only read by ChannelDetailScreen. bip21AmountSats is 100k, above ready inbound, so Spending shows CJIT onboarding and the tab flips to Savings, which encodes 100k. The Amount-screen path (onCjitCreated at :268, reached when creation fails) nulls it the same way.
It's a regression against Android master, where neither call touched cjitInvoice — it was only replaced at Confirm (master :287, :302). The two halves came from different commits: the null from f604acdf2, hidden behind A until 79dd4c3bf removed A; and the Savings-at-100k half from 79dd4c3bf, which followed my own suggestion on :384 to set the bip21 amount before navigating. My last walkthrough also cited this null as the guard keeping the CreateCjit path clean — true for the success case, but I didn't walk the decline.
Why this isn't blocking: iOS does exactly the same thing. For CreateCjit, ReceiveEdit.swift:244-246 calls finishWithRoute(.cjitConfirm) → replaceEditedQrRoute, which removes the stale CJIT QR at creation, not at Confirm — and testReplacingEditedQrRouteCanNavigateToFreshCjitFlow (ReceiveEditTests.swift:40-50) pins that. iOS also sets invoiceAmountSats before branching (:177), so its Savings shows the new amount after a decline too. A was blocking partly because Android diverged from iOS; blocking an alignment PR in order to make it diverge would be backwards. It's also narrower than A — CreateCjit needs ready inbound that's non-zero but below the new amount (ReceiveLiquidityDecision.kt:43-45), so zero-channel first-CJIT users can't reach it — and no sats are at risk, since the old invoice stays payable.
If you want it fixed, do it on both platforms: replace at Confirm, not at creation. Drop cjitInvoice = null here (onCjitConfirmed overwrites it anyway) and move updateOnchainBip21Amount into the ConfirmIncreaseInbound onContinue (:306). Two caveats: onContinue isn't a suspend function, so call onCjitConfirmed and navigate synchronously and launch the bip21 update separately; and ConfirmIncreaseInbound is also reached from the Amount screen, so gate or accept that.
Related, introduced by 79dd4c3bf: wrapping navigateReceiveConfirm in scope.launch means isCreatingCjit = false (EditInvoiceScreen.kt:147) runs before navigation, so a second tap during the DataStore write re-runs createCjit and leaves an orphan LSP quote, overwriting entryDetails. navigateTo is single-top so there's no duplicate screen, and no sats are at risk. Making the navigation synchronous again — which the fix above does — removes it.
|
|
||
| fun setBip21AmountSats(amount: ULong?) = _walletState.update { it.copy(bip21AmountSats = amount) } | ||
|
|
||
| suspend fun updateOnchainBip21Amount(amountSats: ULong?): Result<Unit> = withContext(bgDispatcher) { |
There was a problem hiding this comment.
Low, latent: this moves bip21AmountSats but leaves walletState.bolt11 at the old amount.
updateBip21Invoice clears bolt11 when the amount can't be covered (:755-756); this rebuilds bip21 without lightning= but never clears bolt11. So after a CreateCjit edit the wallet holds bolt11 at 20k and bip21AmountSats at 100k.
It's masked today: CreateCjit only fires when the amount exceeds ready inbound, so canCreateLightningInvoice is false and getInvoiceForTab hides the stale bolt11, and a ChannelReady regenerates it. It would surface only if ready inbound grew without a ChannelReady (say, after an outbound payment) while the QR sits with cjitInvoice == null — i.e. after the Confirm→Back above — at which point Spending would show a payable 20k bolt11 under a 100k Savings request.
A setBolt11("") in here matches updateBip21Invoice's cannot-cover branch, and the existing test's never().createInvoice assert still holds.
| } | ||
|
|
||
| @Test | ||
| fun `receive CJIT session keeps invoice when edit is cancelled`() { |
There was a problem hiding this comment.
These two tests would still pass with the A fix reverted, so the regression isn't actually pinned.
The fix lives in composable lambda wiring (ReceiveSheet.kt:155-164, :365-368) that this unit test can't reach. keeps invoice when edit is cancelled never simulates an edit — it's create → confirm → assert, effectively the same as exposes confirmed fresh invoice (:84-93). clears stale invoice when edit is applied calls clear() directly. Re-adding beginReceiveEdit() and its two call sites would leave both green.
Not blocking — the state class is thin and there's no Compose harness for ReceiveSheet — but worth knowing so nobody reads these as coverage for the Edit→Back behaviour.
Finding A is fixed in 79dd4c3 and nothing blocking remains. Remaining notes are non-blocking — see the latest review.
Description
Follow-up from synonymdev/bitkit-ios#711 (comment)
This PR:
Design
N/A — no design available.
Preview
Screen.Recording.2026-09-10.at.19.48.49.mov
QA Notes
Manual Tests
regression:ready channel with inbound > 0 but peer not yet usable → Receive: normal LN invoice/Auto availability follows ready inbound capacity.Automated Checks
CjitQuoteValidatorTest.ktcovers invalid fee/net quote cases and a valid quote.CjitEntryDetailsTest.ktcovers confirmation-entry mapping and quote rejection before confirmation UI.ReceiveInvoiceEditStateTest.ktcovers clearing stale CJIT invoice state on edit and fresh CJIT creation.ReceiveLiquidityDecisionTest.ktandWalletRepoTest.ktcover ready-channel receive eligibility and ready inbound liquidity.BlocktankRepoTest.ktcovers node capacity error classification separately from per-channel max-size errors../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