Skip to content

fix(receive): handle additional receive liquidity edge cases - #711

Open
pwltr wants to merge 3 commits into
masterfrom
fix/additional-cjit
Open

fix(receive): handle additional receive liquidity edge cases#711
pwltr wants to merge 3 commits into
masterfrom
fix/additional-cjit

Conversation

@pwltr

@pwltr pwltr commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

Description

  • Gate additional CJIT routing to receive edits that originate from the Spending tab.
  • Keep Savings and Auto edits on the normal receive QR flow, falling back to on-chain when Lightning cannot receive the entered amount.
  • Prevent normal Lightning invoices when there is no ready channel, zero inbound liquidity, or insufficient inbound liquidity for a fixed amount.
  • Extract receive liquidity decisions into a focused, unit-tested helper.
  • Enforce the real CJIT maximum using invoice amount plus default LSP balance against Blocktank max channel size.
  • Improve the CJIT max amount error toast instead of surfacing the generic App Error.
  • Use additional-liquidity copy when the wallet already has channels.
  • Document the receive liquidity behavior in 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

  • Ran swiftformat on touched Swift files.
  • Ran focused unit tests: ReceiveLiquidityDecisionTests passed, 12/12.
  • Manually validated:
    • Savings/Auto edit over inbound falls back to on-chain without CJIT.
    • Spending edit over inbound but below CJIT minimum routes to CJIT amount entry.
    • Spending edit with valid CJIT amount shows confirmation.
    • CJIT amount over max shows the improved max amount toast.

@pwltr
pwltr force-pushed the fix/additional-cjit branch from 5aa9515 to 54e6fdf Compare September 1, 2026 17:00
@pwltr
pwltr marked this pull request as ready for review September 1, 2026 17:12
@greptile-apps

greptile-apps Bot commented Sep 1, 2026

Copy link
Copy Markdown

Greptile Summary

The PR centralizes receive-liquidity decisions and updates the receive flow to distinguish normal Lightning invoices, on-chain fallback, and additional CJIT capacity.

  • Gates fixed and variable Lightning invoices using channel readiness and inbound capacity.
  • Restricts additional CJIT routing to edits originating from the Spending tab.
  • Calculates the CJIT amount ceiling from Blocktank’s maximum channel size and default LSP balance.
  • Adds focused decision tests, improved capacity messaging, source-tab-aware navigation, and receive-liquidity documentation.

Confidence Score: 5/5

The 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.

Important Files Changed

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]
Loading

Reviews (1): Last reviewed commit: "fix(receive): handle additional receive ..." | Re-trigger Greptile

@ovitrif

ovitrif commented Sep 4, 2026

Copy link
Copy Markdown
Collaborator

pls sync with master and don't forget to request review(ers) when ready

@piotr-iohk

piotr-iohk commented Sep 4, 2026

Copy link
Copy Markdown
Collaborator

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:

Creating CJIT invoice with channel size: 777600 and invoice amount: 2000
Failed to create CJIT entry: … Node capacity is above our capacity limit

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 maxChannelSizeSat, so the user still gets App Error.

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

Logs:
bitkit_logs_2026-09-04_13-16-32.zip

@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.

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.

Comment thread Bitkit/ViewModels/WalletViewModel.swift Outdated
Comment thread Bitkit/Views/Wallets/Receive/ReceiveCjitAmount.swift Outdated
Comment thread Bitkit/Views/Wallets/Receive/ReceiveQr.swift
Comment thread Bitkit/Views/Wallets/Receive/ReceiveEdit.swift
Comment thread Bitkit/ViewModels/BlocktankViewModel.swift Outdated
Comment thread Bitkit/Views/Wallets/Receive/ReceiveCjitAmount.swift
Comment thread Bitkit/ViewModels/BlocktankViewModel.swift Outdated
@piotr-iohk

Copy link
Copy Markdown
Collaborator

Conflicts.

@pwltr
pwltr force-pushed the fix/additional-cjit branch from 282da3f to a0382d4 Compare September 8, 2026 11:37
@ovitrif

ovitrif commented Sep 8, 2026

Copy link
Copy Markdown
Collaborator

@pwltr can we wrap this up to make release-branch cutting easier?

Last thing we did for 2.4.1 won't work that well with any automated system (including sec qa -- there is no tag matching the release sources on the main branch correctly)

Asking because Android is now on master, ideally we have iOS matching.

PS. All good, just a ping, but you're at it already.

@pwltr

pwltr commented Sep 8, 2026

Copy link
Copy Markdown
Contributor Author

@ovitrif it's ready now, did local manual QA including hardware wallet flows.

ovitrif
ovitrif previously requested changes Sep 8, 2026

@ovitrif ovitrif left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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

Comment thread Bitkit/Components/TabBar/TabBar.swift Outdated
Comment thread Bitkit/ViewModels/BlocktankViewModel.swift Outdated
Comment thread Bitkit/ViewModels/BlocktankViewModel.swift Outdated

@ovi-reviewer ovi-reviewer Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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),

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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?

@ovi-reviewer

ovi-reviewer Bot commented Sep 8, 2026

Copy link
Copy Markdown

@pwltr I confirmed the review fixes on the updated head.

@ovitrif
ovitrif dismissed their stale review September 8, 2026 18:08

addressed - reaudit confirmed

@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.

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) -> IcJitEntry passed by value into .cjitConfirm. The confirmation renders entry.networkFeeSat, entry.serviceFeeSat and receiveAmountSats - 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 second createCjit between 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: QrArea copies copyValue ?? uri and regenerates the share image on onChange(of: uri, initial: true); Savings uses stripLightningFromBip21(wallet.bip21) for both, Unified wallet.bip21 for both, Spending cjitInvoice ?? wallet.bolt11 for both, Trezor the full URI when it has params else the bare address. On editing a normal amount, refreshBip21(forceRefreshBolt11: true) rebuilds bip21 and bolt11 together before dismissal, and the root .task re-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_balance makes amount + lspBalance(amount) <= max monotone true->false across all three fee regions, so maxCjitAmountSats() returns the true largest fitting amount. 0/nil is treated as "unknown" consistently across additionalLiquidityAction, isWithinMaxCjitAmount and updateInputCap, and maxAmountOverride is clamped so an absurd server max can't lift the global input cap.
  • freshMaxChannelSizeSat is now non-throwing and falls back to cached info; with no info at all it returns nil and createCjit skips the client max check, but getDefaultLspBalance still throws if refreshInfo() fails — so creation fails closed rather than sending an unbounded channelSizeSat. 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 in debugMessage while message is a static key, and cjitNodeCapacityExceeded's English errorDescription is never shown because both screens use the localized toast.
  • Double-tap guards are set synchronously before the first await, reset in defer, and passed to CustomButton(isLoading:) — two taps can't create two entries. That closes my earlier thread properly.
  • Session identity and lifecycle. presentationID is a let UUID() created once per showSheet; ReceiveSheet applies .id(config.id) and resets path, amount, note and tags in onAppear. Dismissing mid-flow after createCjit leaves an unpaid LSP entry, which is free until paid, picked up by refreshOrders polling, and expires server-side — no orphaned charge. TabBar uses canCreateReceiveLightningInvoice(amountSats: nil), so a previous session's amount can't route a fresh sheet to CJIT.
  • Concurrency. BlocktankViewModel is @MainActor. The one real interleaving — onChange(of: maxChannelSizeSat) racing .task's refreshMaxCjitAmount() — writes the same deterministic value from both, and onContinue re-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 in en and fall back cleanly; wallet__receive_cjit_error_node_capacity__* are present in all 15 with no placeholders. wallet__receive_connect_initial has both fee placeholders everywhere it's defined. Only fr's _additional is broken — first comment below.
  • Docs/receive-liquidity.md matches 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 ReceiveLiquidityDecisionTests line 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",

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.

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()

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.

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.

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.

[Bug]: Receive liquidity flow edge cases

4 participants