fix(receive): handle additional receive liquidity edge cases - #711
Conversation
5aa9515 to
54e6fdf
Compare
Greptile SummaryThe PR centralizes receive-liquidity decisions and updates the receive flow to distinguish normal Lightning invoices, on-chain fallback, and additional CJIT capacity.
Confidence Score: 5/5The PR appears safe to merge, with no concrete blocking or independently actionable non-blocking defects identified. The centralized receive-liquidity policy, navigation changes, invoice gating, and CJIT maximum enforcement are internally consistent with the documented behavior and focused tests.
|
| Filename | Overview |
|---|---|
| Bitkit/Models/ReceiveLiquidityDecision.swift | Introduces a focused, deterministic policy for Lightning invoice eligibility and additional-liquidity routing, with matching unit coverage. |
| Bitkit/ViewModels/BlocktankViewModel.swift | Enforces maximum CJIT channel size and derives the maximum invoice amount through the existing LSP-balance calculation. |
| Bitkit/ViewModels/WalletViewModel.swift | Routes receive invoice generation through the centralized readiness and inbound-capacity decision. |
| Bitkit/Views/Wallets/Receive/ReceiveEdit.swift | Preserves the originating receive tab and limits additional CJIT creation or routing to Spending edits. |
| Bitkit/Views/Wallets/Receive/ReceiveQr.swift | Updates receive-tab availability and onboarding presentation when a Lightning invoice cannot be created. |
| Bitkit/Views/Wallets/Receive/ReceiveCjitAmount.swift | Applies the calculated CJIT ceiling to amount entry and surfaces a dedicated maximum-capacity warning. |
| BitkitTests/ReceiveLiquidityDecisionTests.swift | Covers the principal source-tab, inbound-capacity, geographic, minimum, and maximum decision boundaries. |
| Docs/receive-liquidity.md | Documents the intended receive fallback and additional-liquidity behavior across supported edge cases. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Receive amount edited] --> B{Source tab}
B -->|Savings or Auto| C{Lightning capacity sufficient?}
C -->|Yes| D[Generate unified or Lightning-capable QR]
C -->|No| E[Show on-chain Savings QR]
B -->|Spending| F{Additional inbound liquidity needed?}
F -->|No| G[Generate normal Lightning invoice]
F -->|Yes| H{Geo-blocked?}
H -->|Yes| I[Show geo-block screen]
H -->|No| J{Amount within CJIT limits?}
J -->|Yes| K[Create additional CJIT]
J -->|No or limits unavailable| L[Open CJIT amount entry]
Reviews (1): Last reviewed commit: "fix(receive): handle additional receive ..." | Re-trigger Greptile
54e6fdf to
282da3f
Compare
|
pls sync with master and don't forget to request review(ers) when ready |
|
QA iOS sim, regtest. Existing CJIT channel: inbound 768,097, channel size 777,600, spending 1,067. Invoice at exactly 768,097 creates a normal Lightning invoice. Savings edit above inbound stayed on-chain. Auto edit above inbound fell back to Savings QR. Neither entered extra CJIT. Spending bump to 768,098 opens extra-CJIT amount entry (MINIMUM 2,000). Continue at 2,000 (the minimum), 20,000, and 200,009 all fail with generic App Error. Client max toast does not show — these amounts are under the per-channel max (~694k). Logs: Same error for invoice amounts 20000 and 200009. Extra CJIT is trying to open another 777,600 channel on a node that already has one. Blocktank’s node cap rejects it; the new max handling only covers per-channel Please don’t offer extra CJIT (or map this error) when the node is already at Blocktank’s capacity limit. Recording: Screen.Recording.2026-09-04.at.15.15.24.mov |
jvsena42
left a comment
There was a problem hiding this comment.
Diffed against the Android port (#1222). The core is a faithful port — ReceiveLiquidityDecision and its 12 tests match 1:1, and the routing, CJIT max search and session reset are equivalent. Android ended up ahead in three places, which is where most of these comments come from.
On the funds-risk questions: the fee shown is the fee committed (both read from the IcJitEntry whose invoice is displayed), CJIT entries are immutable once created so the quote cannot drift from the payment, and zero-amount invoices with inbound > 0 behave identically on both platforms.
Also: AGENTS.md wants a changelog fragment for a user-facing fix:, and there is none here. Android #1222 ships changelog.d/next/1220.fixed.md.
|
Conflicts. |
282da3f to
a0382d4
Compare
|
@pwltr can we wrap this up to make release-branch cutting easier? Last thing we did for Asking because Android is now on PS. All good, just a ping, but you're at it already. |
|
@ovitrif it's ready now, did local manual QA including hardware wallet flows. |
ovitrif
left a comment
There was a problem hiding this comment.
I found a receive-entry regression: a previous request amount can route a new receive session into CJIT despite available inbound liquidity.
Coverage
| Scope | Score | Basis |
|---|---|---|
| Journeys | n/a | ReceiveLiquidityDecisionTests covers routing decisions; complete UI journey coverage is not measured. |
| Unit tests | n/a | ReceiveLiquidityDecisionTests, ReceiveSheetSessionTests and WalletViewModelReceiveTests cover policy and identity; CJIT maximum and entry routing remain uncovered. |
| QA | n/a | Manual Tests not run |
| Total | n/a | average of the scored rows |
—
Reviewed by Codex (GPT-6) via gh-pr-review-loop skill
There was a problem hiding this comment.
utACK, left a few inline nits.
Coverage
- Journeys: 0%
- Neither of the two changed user journeys, fresh Spending receive routing and node-capacity warning presentation, is exercised end to end in this delta.
- Unit tests: 20%
- TabBarReceiveTests covers one of five changed production decisions: the Boolean routing helper; the nil-amount wallet query, wrapped error mapping, and two receive warning branches remain untested.
- QA: 0%
- Manual Tests not run
- Total: 7%
—
Reviewed by Codex (gpt-5.6-sol high) via gh-pr-review-loop skill
| if navigation.currentRoute == .spendingWallet && !hasInboundCapacity && !hasPendingTransfersToSpending { | ||
| if Self.shouldOpenSpendingCjitEntry( | ||
| isSpendingWallet: navigation.currentRoute == .spendingWallet, | ||
| canCreateVariableLightningInvoice: wallet.canCreateReceiveLightningInvoice(amountSats: nil), |
There was a problem hiding this comment.
TabBarReceiveTests only calls shouldOpenSpendingCjitEntry with hard-coded booleans, so it still passes if this line goes back to the cached wallet.canCreateReceiveLightningInvoice property. That would restore the stale-session routing bug this change fixes. Could we cover the actual tab-bar decision with a stale invoiceAmountSats value and verify that the fresh variable-amount check wins?
| options: .init(source: defaultSource, discountCode: nil) | ||
| ) | ||
| } catch { | ||
| if isNodeCapacityError(error) { |
There was a problem hiding this comment.
createCjit now classifies a backend message after ServiceQueue has wrapped the core error, and both receive screens depend on the new typed result. No test references cjitNodeCapacityExceeded, so a wrapping or message-classification regression would return to the generic or per-channel-max toast without failing CI. Could we add focused coverage that feeds the wrapped capacity is above our capacity limit rejection through the classifier and asserts the distinct error?
|
@pwltr I confirmed the review fixes on the updated head. |
jvsena42
left a comment
There was a problem hiding this comment.
Two LOW inline, both pre-existing and neither introduced here — plus a refinement on one of my earlier threads. Not blocking.
All six of my inline threads and ovitrif's three are addressed at head, and I checked each for over-correction rather than trusting the "fixed" replies. The re-introduced refreshAndSyncState() + refreshBip21() in the .channelReady handler is harmless — invoice regeneration only.
Checked and clean:
- Fee disclosure (the thing that actually matters here).
createCjit(channelSizeSat: amountSats + lspBalance, invoiceSat: amountSats)->IcJitEntrypassed by value into.cjitConfirm. The confirmation rendersentry.networkFeeSat,entry.serviceFeeSatandreceiveAmountSats - entry.feeSat, and Continue pushes.qr(cjitInvoice: entry.invoice.request)from the same value. The fee is never re-fetched after display and there's no secondcreateCjitbetween confirmation and QR — what the user approves is what the displayed invoice commits to. - Quote expiry isn't read anywhere in the receive flow; it's enforced by the bolt11 itself and by the LSP. A payment attempted after expiry fails at the payer rather than taking a fee for a channel the user doesn't get.
- Address/invoice consistency. I checked every branch for a QR-vs-copy divergence and found none:
QrAreacopiescopyValue ?? uriand regenerates the share image ononChange(of: uri, initial: true); Savings usesstripLightningFromBip21(wallet.bip21)for both, Unifiedwallet.bip21for both, SpendingcjitInvoice ?? wallet.bolt11for both, Trezor the full URI when it has params else the bare address. On editing a normal amount,refreshBip21(forceRefreshBolt11: true)rebuildsbip21andbolt11together before dismissal, and the root.taskre-runs on pop, so the QR is never left with a pre-edit amount. (The one exception is the CJIT case in the second comment below.) - Binary search correctness.
get_default_lsp_balancemakesamount + lspBalance(amount) <= maxmonotone true->false across all three fee regions, somaxCjitAmountSats()returns the true largest fitting amount.0/nil is treated as "unknown" consistently acrossadditionalLiquidityAction,isWithinMaxCjitAmountandupdateInputCap, andmaxAmountOverrideis clamped so an absurd server max can't lift the global input cap. freshMaxChannelSizeSatis now non-throwing and falls back to cached info; with no info at all it returns nil andcreateCjitskips the client max check, butgetDefaultLspBalancestill throws ifrefreshInfo()fails — so creation fails closed rather than sending an unboundedchannelSizeSat. The overflow guard covers the rest.- Node-capacity mapping is checked before the max-channel-size branch and the substring was removed from the latter, so the two no longer overlap. Server text is used only for classification; both toasts are localized keys.
- No server-supplied string reaches the UI.
AppError(error:)stashes the server description indebugMessagewhilemessageis a static key, andcjitNodeCapacityExceeded's EnglisherrorDescriptionis never shown because both screens use the localized toast. - Double-tap guards are set synchronously before the first
await, reset indefer, and passed toCustomButton(isLoading:)— two taps can't create two entries. That closes my earlier thread properly. - Session identity and lifecycle.
presentationIDis alet UUID()created once pershowSheet;ReceiveSheetapplies.id(config.id)and resets path, amount, note and tags inonAppear. Dismissing mid-flow aftercreateCjitleaves an unpaid LSP entry, which is free until paid, picked up byrefreshOrderspolling, and expires server-side — no orphaned charge.TabBarusescanCreateReceiveLightningInvoice(amountSats: nil), so a previous session's amount can't route a fresh sheet to CJIT. - Concurrency.
BlocktankViewModelis@MainActor. The one real interleaving —onChange(of: maxChannelSizeSat)racing.task'srefreshMaxCjitAmount()— writes the same deterministic value from both, andonContinuere-fetches when nil, so a lost update can't move the client cap in the unsafe direction. The server check remains regardless. - Localization format check across all 15 files. The new
wallet__receive_cjit_error_max__*keys exist only inenand fall back cleanly;wallet__receive_cjit_error_node_capacity__*are present in all 15 with no placeholders.wallet__receive_connect_initialhas both fee placeholders everywhere it's defined. Only fr's_additionalis broken — first comment below. Docs/receive-liquidity.mdmatches the head on every bullet I traced. One minor omission, not a misdescription: the Spending tab shows CJIT onboarding for any!canCreateReceiveLightningInvoice, including a fixed amount above inbound, where the doc states only the zero-inbound case.- Tests match the implementation — the 12
ReceiveLiquidityDecisionTestsline up with the pure functions, and no test asserts something the code doesn't guarantee.
| BodyMText( | ||
| t( | ||
| isAdditional ? "wallet__receive_connect_additional" : "wallet__receive_connect_initial", | ||
| isAdditionalFlow ? "wallet__receive_connect_additional" : "wallet__receive_connect_initial", |
There was a problem hiding this comment.
The French copy for this branch has a placeholder the code never fills, so French users see a literal {lspFee} where the fee should be.
fr.lproj/Localizable.strings:1041 reads … A <accent>{lspFee}</accent> frais de fournisseur de services seront déduits …, but t() only substitutes the keys the caller passes — networkFee and serviceFee — so the token is left as-is. The English fallback in getString triggers only when the key is missing from the bundle, never on a placeholder mismatch, so there's no safety net.
It's the only {lspFee} in the tree and it came in via a Transifex "Update translations" commit, so it's a stale translation of an older source string rather than a hand-edit — note the French sentence also names only one fee where English names two.
Pre-existing, and I want to be accurate about the reach: the additional copy was already reachable pre-PR through ReceiveEdit's isAdditional: true, so any French user editing above inbound already hit it. What changed here is that isAdditionalFlow = isAdditional || wallet.hasExistingChannels makes the ReceiveCjitAmount path resolve to it too when a channel exists. Modest widening, not a new defect.
Impact is capped because the gross amount and the "Vous recevrez" net line both still render, so the fee is inferable — hence LOW rather than a fee-disclosure failure. All 12 other locales carrying this key have both placeholders; ar and pt lack the key and fall back to English correctly.
Worth fixing at the translation source rather than in-repo: bitkit-android values-fr/strings.xml:873 carries the identical broken string.
| switch additionalLiquidityAction(maxCjitAmountSats: maxCjitAmountSats) { | ||
| case .none: | ||
| try await wallet.refreshBip21(forceRefreshBolt11: true) | ||
| dismiss() |
There was a problem hiding this comment.
Editing down from a CJIT invoice QR leaves the Spending tab showing the old CJIT invoice.
With a ready channel at 10 000 sats inbound: user edits Spending to 50 000 -> .createCjit -> confirm -> Continue pushes .qr(cjitInvoice: <50k>, tab: .spending). That QR still offers Edit (ReceiveQr.swift:270 skips onboarding when cjitInvoice != nil and falls through to qrContent(.spending) with a non-nil editRoute; also via CopyAddressCard). User edits to 5 000, which is under inbound, so additionalLiquidityAction returns .none, refreshBip21(forceRefreshBolt11: true) mints a fresh 5 000-sat bolt11 — and this dismiss() pops back onto the same .qr route with the stale cjitInvoice still bound as a let.
Since qrConfig(.spending) is cjitInvoice ?? wallet.bolt11 and detailsContent(.spending) prefers cjitInvoice, the QR, copy and share all show the 50 000-sat LSP invoice while wallet.invoiceAmountSats is 5 000. The freshly created bolt11 is never displayed. The user shares a request for a different amount than they just entered, carrying a CJIT fee they no longer need to pay.
I checked whether the .task on ReceiveQr rescues this on reappearance — it doesn't. refreshBip21 only rewrites bolt11/bip21; it can't touch cjitInvoice, so the fresh invoice this branch just created is precisely what gets shadowed.
Pre-existing, and this PR narrowed rather than widened it — merge-base did the same refreshBip21 + dismiss() in the !needsAdditionalCjit() branch, and CJIT-from-edit is now Spending-source only. Android has the same unconditional Edit on its CJIT QR, so it's a shared design gap. Flagging as a follow-up, not something to fix here.
If it is worth fixing: rather than manipulating the nav stack, just pass editRoute: nil on the Spending tab when cjitInvoice != nil. A CJIT quote is immutable — any edit needs a fresh quote, and the amount path already pushes a fresh confirm and QR. Worth mirroring on Android if you take it.
Description
Docs/receive-liquidity.md.Linked Issues/Tasks
Closes #671
Screenshot / Video
Simulator.Screen.Recording.-.iPhone.17.-.2026-09-01.at.19.11.14.mov
Testing
swiftformaton touched Swift files.ReceiveLiquidityDecisionTestspassed, 12/12.