Repository navigation
feat(lnurl): LNURL send, receive, token rails and a Lightning address page - #1084
Conversation
Deploying wallet-bitcoin with
|
| Latest commit: |
3f0a414
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://d8cae7eb.wallet-bitcoin.pages.dev |
| Branch Preview URL: | https://split-p12-settings-lnaddress.wallet-bitcoin.pages.dev |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe pull request adds LNURL receive and send flows with Lightning, Arkade, on-chain, and token rails. It adds receiver onboarding, token quotes, payment persistence, Lightning address settings, transaction metadata, confirmation status, deployment configuration, and tests. ChangesLNURL payments
Priority: Estimated code review effort: 5 (Critical) | ~90 minutes Change: Feature Sequence Diagram(s)sequenceDiagram
participant Wallet
participant LnurlRail
participant LNURLServer
participant SendRouter
participant PaymentStore
Wallet->>LnurlRail: Resolve receiver and payment rails
LnurlRail->>LNURLServer: Request capabilities and pay data
LNURLServer-->>LnurlRail: Return receivers and payment options
Wallet->>SendRouter: Request LNURL quote
SendRouter->>LNURLServer: Request payment quote
LNURLServer-->>SendRouter: Return quote and settlement metadata
Wallet->>PaymentStore: Record payment and confirmation data
PaymentStore-->>Wallet: Return activity and confirmation state
Possibly related issues
Possibly related PRs
Suggested reviewers: Merge Risk:
 |
| Check name | Status | Explanation |
|---|---|---|
| Title check |  |
The title clearly and concisely summarizes the main changes: LNURL send and receive support, token rails, and the Lightning address settings page. |
| Linked Issues check | |
Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | |
Check skipped because no linked issues were found for this pull request. |
| Description Check | |
Check skipped - CodeRabbit’s high-level summary is enabled. |
Finishing Touches
-
📝 Generate docstrings
-
- Commit to this branch
- Create a new PR
🧪 Generate unit tests (beta)
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands.
Deploying wallet-mutinynet with
|
| Latest commit: |
3f0a414
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://5c49f884.arkade-wallet.pages.dev |
| Branch Preview URL: | https://split-p12-settings-lnaddress.arkade-wallet.pages.dev |
arkana-ai-bot
left a comment
arkana-ai-bot
left a comment
There was a problem hiding this comment.
Arkana review — feat(settings): Lightning address page
No protocol-critical paths touched (no VTXO handling, signing, forfeit, round lifecycle, or unilateral exit).
Correctness
lnurlRailLimits fallback (src/lib/receive/lnurlRail.ts, new +66–70)
min: Math.ceil((o.minSendable ?? payRequest.minSendable) / 1000),
max: Math.floor((o.maxSendable ?? payRequest.maxSendable) / 1000),PaymentOption.minSendable/maxSendable are optional in the server schema (confirmed in lnurl-server/src/payment-options.ts). The fallback to the top-level payRequest fields is correct because LUD-06 requires those to always be present. Logic is sound.
server?.domain in yours TableData (src/screens/Settings/Lnurl.tsx, ~line 86)
[t('settings.lnurlServer'), server?.domain],server is LnurlServer | null (return type of configuredLnurlServer()), so .domain is string | undefined here. The filter in Advanced.tsx guarantees the page is only mounted when server is truthy, so this is safe at runtime — but if TableData is typed as [string, string][], TypeScript will reject it; if it accepts string | undefined, a future path that bypasses the filter would silently render "undefined" in the UI. Consider asserting server!.domain (or narrowing once at the top of the component with an early return) to make the invariant explicit.
handleChange is async without await (src/screens/Settings/Lnurl.tsx, ~line 99)
const handleChange = async () => {
backupAndUpdateConfig({ ... })
}backupAndUpdateConfig is synchronous (its mock type is (config: Config) => void). The async keyword is harmless but dead weight — it silently swallows any future exception if backupAndUpdateConfig is made async later. Either drop async or add await and surface errors (the existing pattern in other settings handlers is sync).
Error handling
payRequest() failure has no UI surface (src/screens/Settings/Lnurl.tsx, ~lines 63–72)
rail.receiver
.payRequest()
.then(...)
.catch(consoleError) // logged, never shownA transient network error on the lnurl server during settings load silently leaves the "What payers can send" section empty, with no indication of failure. rail.status === 'failed' covers registration/init failures but not this secondary fetch. A small error state wired to this catch would match the existing ErrorMessage pattern used for name-claim failures on the same page.
Dependencies
config.receiveViaLnurl and config.receiveViaTokens are used throughout Lnurl.tsx but do not appear in the Config type visible in this PR's src/lib/types.ts diff — they must be added by the dep chain (#1083). TypeScript build will hard-fail if this merges before #1083 lands; the test plan's tsc 0 check confirms the series is expected to be applied in order.
Test coverage
Nine tests cover the happy paths and the one-time token explainer flow well, including the "seen before / not seen" state. No test exercises rail.status === 'failed' rendering or a payRequest() rejection. Low risk for a settings display, but worth a follow-up if the empty-limits silent-failure above is addressed.
Privacy disclosure
Both warning strings are explicit and accurate:
receiveViaLnurlSubtexttells the user the server sees every payment when the toggle is on.tokenExplainernames the third-party deposit holder and is shown exactly once before opt-in.
No concerns there.
Cross-repo
lnurlRailLimits is a new export from lnurlRail.ts but is wallet-internal (not re-exported from an SDK package). No consumers found in ts-sdk, go-sdk, rust-sdk, or dotnet-sdk. No cross-repo breakage.
Summary
Three items worth addressing before merge:
- Assert or narrow
serverbefore buildingyours(defensive?.obscures a real invariant). - Drop
asyncfromhandleChangeor add proper error handling. - Consider surfacing
payRequest()fetch errors in the UI.
All three are minor; none is a blocker if the series is landing together and the CI tsc gate is green.
|
@arkana-ai-bot thanks. All three points are fixed in
On the dependency note: this PR is stacked on #1083, so the config fields are already in its tree. |
|
Thanks @Kukks — verified all three in
Nothing further from me on these items. |
arkana-ai-bot
left a comment
arkana-ai-bot
left a comment
There was a problem hiding this comment.
Follow-up pass on 7508ca2 — 10 commits, 18 files since c7521dd. All 4 issues from the prior review are closed; no new blocking findings.
Prior findings — status
1. server?.domain optional chain in yours TableData (Settings/Lnurl.tsx)
Fixed in a01ce4ef. The spread now conditionally includes the row only when server is truthy and asserts the full [string, string] tuple — the TypeScript gap and the silent-undefined render are both gone.
2. handleChange declared async without await (Settings/Lnurl.tsx)
Fixed in a01ce4ef. async removed.
3. payRequest() rejection not surfaced in the UI (Settings/Lnurl.tsx)
Fixed in a01ce4ef. limitsError state is now set on the catch path and rendered via <ErrorMessage> under the limits section header. The section itself is now shown when either sendable.length or limitsError is truthy — correct. The test at settings/lnurl.test.tsx +104 ('says so when what payers can send cannot be read') locks the behaviour against regression.
4. No test for payRequest() failure path
Fixed — covered by the test noted above and by the payRequestFail option added to fakeLnurlServer.
New findings on the incremental diff
lnurlTokenRails.ts +109 — strict verifiability gate
- const offered = o.available && o.verifiable !== false && o.unit.decimals <= 18 && inBounds
+ const offered = o.available && o.verifiable === true && o.unit.decimals <= 18 && inBoundsSecurity improvement. The previous !== false would silently offer a token option whose verifiable field was absent (undefined), meaning the wallet would display a receive QR for a token it cannot confirm was actually paid. The strict === true is the right gate. Test added (['no verifiable flag', { verifiable: undefined }]) locks it.
lnurlRail.ts — guard own addresses before requesting arkade/onchain legs
const rails: LnurlRailType[] = [
'lightning',
...(allRails && ownArkade ? (['arkade'] as const) : []),
...(allRails && ownOnchain ? (['onchain'] as const) : []),
]Correct fix. Previously, resolveLnurlRails was called with empty strings for own.arkade/own.onchain when the wallet's addresses hadn't resolved yet, causing sameOperator / the onchain equality check to always fail on the first render. Now those legs are deferred until the address is known. The effect's dep array ([receiver, amountSat, allRails, ownArkade, ownOnchain]) re-fires when they arrive, so nothing is lost. The new useLnurlRails integration test pins the sequencing.
lnurlPaymentRepository.ts — db.close() on error paths
Three error handlers previously rejected without closing the connection; all three are fixed. An IndexedDB connection left open on an error path can block a future indexedDB.open() version-upgrade transaction, which would silently stall activity sync. Correct.
QrCode.tsx +588 — Copy disabled when token selected but quote absent
disabled={generating || Boolean(activeToken && !tokenQuote)}generating (tokenRails.quoting) is false once a quote attempt resolves — whether successfully or with an error. In the error case tokenQuote is undefined, so without this extra condition a Copy press would fall through to tokenQuote?.destination ?? qrCodeValue and copy the wrong value (the invoice/address for a different rail). The added guard closes that window. Test in receive-lnurl.test.tsx +583 verifies the button is disabled when the error fires.
swapDisplay.ts — lnurlLabel now takes t: Translate
Pure i18n plumbing; all call-sites updated; Spanish strings present and tested. No issues.
LnurlRail.tsx — hardcoded strings removed
All UI strings now go through t(...). Both en and es keys are wired in i18n.ts. No issues.
Remaining concern (carry-over, unchanged)
The series dependency note from the prior review stands: config.receiveViaLnurl / config.receiveViaTokens must arrive from #1083 before this merges or tsc fails. No code change in this increment addresses or worsens that; it is purely a merge-order enforcement matter already called out in the PR description's test plan.
No protocol-critical paths (VTXO, signing, forfeit, round lifecycle, unilateral exit) are touched in this diff.
arkana-ai-bot
left a comment
arkana-ai-bot
left a comment
There was a problem hiding this comment.
Follow-up pass on 7508ca2 — 10 commits since c7521dd; all 3 prior actionable findings fixed, 1 prior note unchanged.
Prior findings: closure
1. server?.domain undefined in yours TableData — Fixed (commit a01ce4ef)
Replaced with a conditional spread ...(server ? [[t('settings.lnurlServer'), server.domain] as [string, string]] : []). Invariant now expressed in type, not just runtime guard.
2. handleChange spuriously async — Fixed (commit a01ce4ef)
Dropped async. Matches the sync pattern used by every other settings handler.
3. payRequest() failure silent in UI — Fixed (commit a01ce4ef)
limitsError state added, extractError imported, the .catch sets limitsError, and <ErrorMessage> renders it in the limits section. Test 'says so when what payers can send cannot be read' covers the path using payRequestFail on fakeLnurlServer.
4. Merge-order dependency on #1083 — Still open, no change
config.receiveViaLnurl/config.receiveViaTokens on Config still require #1083 in base first. Nothing here changes that.
Incremental findings
src/lib/receive/lnurlTokenRails.ts:109 — verifiable !== false → verifiable === true
An absent verifiable field previously allowed the option to be offered; it is now rejected. For a flow routing incoming payments through a third-party token deposit, requiring an explicit true is the safer default. Test updated to include 'no verifiable flag' as an omit case. No concerns.
src/lib/receive/lnurlRail.ts:135–146 — useLnurlRails deferred-rail guard
allRails && ownArkade / allRails && ownOnchain now gate each rail's inclusion. resolveLnurlRails rejects a resolved address that differs from the wallet's own, so passing an empty string (address not yet loaded) would always fail the foreign-address check. Effect re-runs when addresses load; the stale-flag cleanup is in place. New test useLnurlRails asks for a leg only once its own address is known pins the behavior. Correct fix.
src/lib/lnurlPaymentRepository.ts:40, 57, 73 — IDB db.close() on error paths
Three onerror handlers previously rejected without closing the connection; they now close it first. Prevents an IDB connection leak when a read, write, or clear operation fails. Good fix.
src/lib/swapDisplay.ts:107 — lnurlLabel(tx, t) signature change
lnurlLabel now takes t: Translate rather than producing hardcoded English. All callers updated: TransactionsList.tsx:113, swapDisplay.test.ts, activityHistory.test.ts. Spanish translation test added. This is wallet-internal; no cross-repo API surface affected.
src/screens/Wallet/Receive/QrCode.tsx:590 — Copy disabled when activeToken && !tokenQuote
Copy button is now disabled while a token quote is pending, preventing the user from copying an incomplete payment URI. Correct guard.
src/screens/Wallet/Receive/LnurlRail.tsx — String literals moved to i18n
NameForm and LnurlRailPanel now use t(...) throughout. The lnurl.test.tsx references 'Choose a name' / 'Claim code' as placeholder strings — these are the English values of the respective keys established in prior commits. Tests pass, keys resolve correctly.
No protocol-critical paths touched. No new security concerns. All three raised findings were resolved with test coverage. The merge-order note on #1083 stands unchanged. Ready to land once #1083 is in base.
arkana-ai-bot
left a comment
arkana-ai-bot
left a comment
There was a problem hiding this comment.
Follow-up review — 5 commits since 7508ca2, 4 of 5 prior issues fixed, 1 still open
This push merges three upstream split branches (p06, p09, p11) into p12 and adds a handful of targeted fixes across the receive rail. No protocol-critical paths (VTXO / signing / forfeit / round / unilateral exit) are touched.
Prior-issue closure
Fixed — server?.domain optional access in yours TableData (src/screens/Settings/Lnurl.tsx)
Now uses a guarded spread: ...(server ? [[t('settings.lnurlServer'), server.domain] as [string, string]] : []). The invariant is explicit and TypeScript-sound.
Fixed — async on handleChange without await (src/screens/Settings/Lnurl.tsx)
handleChange is now a plain synchronous arrow function.
Fixed — payRequest() failure silently lost (src/screens/Settings/Lnurl.tsx, ~line 69)
The .catch now calls setLimitsError(extractError(err)), and limitsError is wired to <ErrorMessage> inside the limits section. The gap I flagged is closed.
Fixed — receiveViaLnurl / receiveViaTokens missing from Config type
split/p11-token-rails is now in the branch base; the type is present and tsc passes.
Still open — no test for lnurlRail.status === 'failed' render path
The new tests cover stale-invoice suppression, multi-locale error messages, and generic fallback, but none mount the settings Lnurl page (or the receive QrCode) with status === 'failed' to verify the rail.error text is displayed. Low risk for a display-only path, but the gap persists as noted last pass.
New findings (incremental diff only)
useLayoutEffect for current.current — correct fix (src/lib/receive/lnurlRail.ts, lines 240-242)
Replacing the render-phase assignment with useLayoutEffect(() => { current.current = facade }, [facade]) is the right pattern for concurrent React. Layout effects commit synchronously before any user callbacks fire, so the ref is always consistent by the time run reads it. No concerns.
Empty-group vacuous-truth fix (src/lib/activityHistory.ts, line 274)
Adding activity.txs.length > 0 before the every(...) guard correctly prevents empty sent: groups from satisfying the skip condition vacuously. Clean.
Stale-invoice suppression (src/screens/Wallet/Receive/QrCode.tsx, line 153)
const invoice = (swapRailOn ? recvInfo.invoice : undefined) || lnurlRails.targets.lightning || ''When the LNURL rail is active, a swap invoice that persisted in the flow context from a previous session is intentionally discarded. The loading state is correctly covered by lnurlRails.generating, and the new test at line 218 verifies the behaviour end-to-end. Good.
CLAIM_ERROR_KEYS typing — minor (src/lib/receive/lnurlRail.ts, line 183)
Record<string, string> types index access as string, not string | undefined, so TypeScript won't enforce the if (key) guard unless noUncheckedIndexedAccess is on. The runtime check is still there, so this is harmless, but Partial<Record<'taken' | 'invalid_username' | 'blacklisted' | 'invalid_claim' | 'forbidden_mode' | 'limit_reached', string>> (or a satisfies assertion) would make the exhaustiveness more explicit. Nit only.
Summary
The four substantive issues from the last review are fixed. The one remaining gap (test for the failed rail render) is low risk and can be a follow-up. The incremental changes are clean; the useLayoutEffect refactor and invoice-priority fix are both correct.
|
@arkana-ai-bot thanks.
|
|
Verified both points in Failed rail test —
No further open items on this PR. |
arkana-ai-bot
left a comment
arkana-ai-bot
left a comment
There was a problem hiding this comment.
Follow-up review — 3 commits since c93d211, 1 remaining prior issue fixed, 0 still open
Commits: e7fa58c (URI param-injection fix + Share guard), c8f3753 (merge p11), a55a64e (failed-rail test). No protocol-critical paths (VTXO / signing / forfeit / round / unilateral exit) touched.
Prior-issue closure
Fixed — no test for lnurlRail.status === 'failed' render path
a55a64e adds 'says why the lightning address could not be loaded, and offers no naming' in src/test/screens/settings/lnurl.test.tsx. The capabilitiesFail: true flag makes the fake server return a 500 with { error: 'domain lookup failed' }; the test asserts the error text is visible and the naming form is absent. Closes the gap. ✅
CLAIM_ERROR_KEYS typing nit — still deferred to the next #1081 change as agreed. Not this review's subject.
New findings (incremental diff only)
src/lib/receive/lnurlTokenRails.ts:47–52 — security fix for query-parameter shadowing on payment URIs
const only = (key: string) => {
const values = q.getAll(key)
return values.length === 1 ? values[0] : undefined
}Previously q.get(key) was used for EVM (address, uint256) and Solana (spl-token, amount) validation. URLSearchParams.get() returns the first value when a key is repeated. The new only() helper returns undefined for any multi-valued key, causing paysQuote to return false and the UI to fall back to the raw deposit address. The vulnerability class is query-parameter shadowing / HTTP parameter pollution on payment URIs; the impact class is misdirected token payment. The fix is correct and complete: rejecting ambiguous URIs outright is the right policy. Both new test cases ('a second, conflicting recipient', 'the amount repeated') cover EVM.
Minor: the Solana duplicate-key cases (spl-token, amount repeated) are not exercised by a test, but the code path is identical to the EVM path through only(). Low risk; a follow-up test case covering only('amount') with a duplicated Solana amount param would complete coverage.
src/lib/receive/lnurlTokenRails.ts:67 — decimal(only('amount') ?? null) is consistent
decimal() was already called with a null return from q.get() in the original code; the ?? null coercion maintains that contract. No behavioural change when the key is absent or single-valued.
src/screens/Wallet/Receive/QrCode.tsx:413 — Share disabled guard matches Copy
Boolean(activeToken && !tokenQuote)Symmetry with the existing Copy button guard is correct. The new test in src/test/screens/wallet/receive-lnurl.test.tsx (Share mock + disabled assertion when token error is present) pins the behaviour. ✅
src/test/screens/wallet/receive-lnurl.test.tsx:56–60 — onTestFinished cleanup
share.can = false is reset via onTestFinished rather than afterEach, which is the right scoping (test-local cleanup rather than suite-wide). The mock is not leaked across tests. ✅
Summary
All prior open items are now closed. The only() security fix is the most material change — correct approach, good test coverage for EVM, Solana coverage optional as a follow-up. No blocking issues.
arkana-ai-bot
left a comment
arkana-ai-bot
left a comment
There was a problem hiding this comment.
Arkana follow-up — a55a64e → 0adb038
Status: 3 commits landed since last pass (1 content + 2 merges). Prior issues: 0 fixed, 3 still open, 0 no longer applicable. No new findings in the incremental diff.
What changed
c5f2fd3 — the only substantive commit; two changes:
src/lib/receive/lnurlRail.ts line 177 — CLAIM_ERROR_KEYS promoted from Record<string, string> to Partial<Record<string, string>>.
This is correct. Without noUncheckedIndexedAccess, the non-Partial type told TypeScript any string key was always present, silently handing a non-null string to t() even for unknown error codes. The call site already has if (key) return t(key) (line 191), so the guard was defensive but not compiler-enforced; now it is. No behaviour change, improved type accuracy. ✓
src/test/lib/receive/useLnurlRail.test.ts — new test: "settles back on the same receiver when the boarding address arrives after a session registered."
The test correctly pins that result.current.lnurl is reference-equal across the re-render, not just truthy — which matters here because a changed LNURL would invalidate any QR the user had already shared. Also checks error === '' after re-render. Both assertions are the right ones to have. ✓
Prior-issue status
-
server?.domainwithout assertion (src/screens/Settings/Lnurl.tsx, ~line 86) — Still open. File untouched in this diff. -
handleChangeisasyncwithoutawait(src/screens/Settings/Lnurl.tsx, ~line 99) — Still open. File untouched in this diff. Low risk now, but a futureasync backupAndUpdateConfigwould silently swallow errors. -
payRequest()failure has no UI surface (src/screens/Settings/Lnurl.tsx, ~lines 63–72) — Still open. Therail.status === 'failed'rendering test added ata55a64ecovers the rail itself failing; a transientpayRequest()error (the secondary fetch that populates the "What payers can send" limits) is a distinct path and still surfaces nothing to the user.
No protocol-critical paths touched. Incremental changes are clean; three pre-existing nits in Lnurl.tsx remain but are cosmetic/low-risk for a settings display screen.
|
@coderabbitai review |
✅ Action performedReview finished.
|
arkana-ai-bot
left a comment
arkana-ai-bot
left a comment
There was a problem hiding this comment.
Follow-up pass on 3219ebf — 5 commits, 6 files since 0adb038. All prior actionable findings remain Fixed; 1 standing note unchanged. No new blocking issues.
Prior findings — closure
1. server?.domain — Fixed (a01ce4ef)
2. async handleChange — Fixed (a01ce4ef)
3. payRequest() silent failure — Fixed (a01ce4ef)
4. Merge-order dependency on #1083 — Still open, unchanged.
config.receiveViaLnurl/config.receiveViaTokens still require #1083 in base. Nothing in this increment changes or worsens that; the tsc 0 gate in the test plan is the enforcement mechanism.
Incremental diff findings
src/lib/lnurlPaymentRepository.ts — write-gate and clearLnurlPaymentsForReset
Logic is correct. The key invariant the comment claims — "Refusing and registering happen in one synchronous turn, so no write can slip past closeWrites and then land after writesSettled resolved" — holds. Between the gate check (if (closed) return) and the inFlight.add(write), there is no await; JavaScript's single-threaded event loop guarantees no other code can run in that window. A write that passes the gate is in inFlight before closeWrites() can observe it, and writesSettled() spreads inFlight after closed is already true, so no new writes can enter after the snapshot. ✅
The finally { inFlight.delete(write) } cleanup is safe: Promise.allSettled already holds a reference to the promise, so removing it from inFlight after settlement doesn't affect the outstanding allSettled call. ✅
writesSettled() on rejection path (lnurlPaymentRepository.test.ts +160): the finally block runs and removes the promise from inFlight before writesSettled() is called in the test, so allSettled([]) resolves immediately. Correct. ✅
src/providers/wallet.tsx — ordering change: lnurl clear is now fatal-before-destructive
// before
await clearStorage()
...
await lnurlPaymentRepository.clear().catch(consoleError) // non-fatal, after destructive
// after
await clearLnurlPaymentsForReset() // fatal, before destructive
await clearStorage()
...Moving this before clearStorage() and making it fatal is the right call. These records hold preimages; a completed reset that left them intact would hand the next wallet the previous wallet's payment history. If the clear fails (e.g., IDB locked by a second tab), the reset now aborts before anything is destroyed, and the wallet is left intact. The error path surfaces correctly in the UI (see below). ✅
One edge case worth noting but not a blocker: if clearLnurlPaymentsForReset() succeeds but a later step (e.g., svcWallet.clear()) fails, the records are gone and the reset aborts without a page reload. On the next retry, closeWrites() is idempotent on the singleton, writesSettled() resolves immediately (empty inFlight), and clear() runs on an already-empty store — so retry is safe. ✅
src/screens/Settings/Reset.tsx — error feedback
setError('') at the top of handleReset clears stale errors before each attempt. The fixed resetFailed string deliberately avoids echoing the underlying error, which could name internal storage objects or paths. The Reset.test.tsx test asserts not.toHaveTextContent('indexeddb unavailable') to lock this. setLoading(false) resets only in the catch path; on success location.reload() obviates any state cleanup. ✅
src/lib/i18n.ts — resetFailed string pair
Both the English and Spanish strings name the symptom ("nothing was cleared") and the mitigation ("close other tabs"), without identifying which subsystem failed. Good. ✅
src/lib/lnurlPaymentRepository.ts — readWatermarkMap validation (+170-172)
const parsed: unknown = JSON.parse(value)
return parsed && typeof parsed === 'object' && !Array.isArray(parsed) ? (parsed as Record<string, number>) : {}Defensive against null, [], and primitives in localStorage. The test covers 'null', '[]', and '7' as raw stored values and confirms both that the watermark reads as undefined and that a subsequent write succeeds. ✅
src/test/lib/lnurlPaymentRepository.test.ts — heldWriteStore helper
The tick() helper (setTimeout(resolve, 0)) correctly schedules a macrotask, which allows microtasks (Promise callbacks) to drain first. The test at +145 verifies settled === false before release(), and settled === true after. The sequencing is sound. ✅
src/test/screens/settings/reset.test.tsx — new file
Tests the two paths: failure (error message shown, underlying error not leaked, button remains enabled) and no-failure (no error message). Both are correct and cover the added code. ✅
No protocol-critical paths (VTXO handling, signing, forfeit, round lifecycle, unilateral exit) are touched in this diff. No new security concerns.
|
You're right, @Kukks — that was my error again. I've verified at HEAD ( My previous review at pullrequestreview-5672252582 correctly recorded this as Fixed. I should not have re-listed it as open in the |
…y the address from Receive Claim sheet: on a server that offers claim codes but no `self` mode, a chosen name cannot be claimed without a code. The sheet now asks for the code first, above the name field, instead of hiding it behind the "I have a claim code" link. Where the code is optional, the link still shows and hides the field. Receive: the lightning address under the QR now shows only on the Unified and Lightning views, which are the ones it pays, and tapping it copies it, the way other copyable text in the wallet does.
arkana-ai-bot
left a comment
arkana-ai-bot
left a comment
There was a problem hiding this comment.
Follow-up pass on 27203da — 1 commit since 81f96b5; 0 prior findings fixed (none were open blockers), 2 low-risk carry-overs unchanged.
Prior-finding closure
CLAIM_ERROR_KEYS typed as Record<string, string> (src/lib/receive/lnurlRail.ts) — Still open, not worsened.
Unchanged from last pass. The runtime guard is present; this remains a nit.
No test for lnurlRail.status === 'failed' render path — Still open, not worsened.
Unchanged. Still low risk for a display-only path.
Neither was flagged as a blocker. This commit does not touch either site.
Incremental findings
NameForm — codeFirst logic (src/screens/Wallet/Receive/LnurlRail.tsx)
The three-way condition is correct for all choices combinations:
self |
admin |
codeFirst |
code = |
canClaim condition |
|---|---|---|---|---|
| ✓ | ✓ | false | claimCode if hasCode, else "" |
Boolean(username) (code optional) |
| ✓ | ✗ | false | "" always |
Boolean(username) (no code path) |
| ✗ | ✓ | true | claimCode always |
Boolean(username) && Boolean(code) (both required) |
admin && !codeFirst correctly gates the "I have a claim code" link — it appears only when code is optional (self && admin), and is hidden when code is mandatory (admin && !self). The {hasCode && !codeFirst ? codeInput : null} guard prevents a duplicate field from rendering in the mandatory case. No logic gaps found.
codeInput is defined once and conditionally placed either at the top (mandatory) or below the name (optional, toggled by link). The focus prop on codeInput is appropriate in the mandatory case where it is the first field; in the optional-and-revealed case it still auto-focuses the revealed field, which is reasonable.
LnurlRailPanel — showAddress prop (src/screens/Wallet/Receive/LnurlRail.tsx:14, 26)
showAddress={activeMethod === 'unified' || activeMethod === 'lightning'} is the right predicate: a Lightning address is payable only on the Lightning and Unified rails; showing it under Arkade or Bitcoin would invite misdirected payments. Returning null on other methods (rather than hiding via CSS) is preferable — no node in the DOM for screen-readers to announce. Correct.
<Text centered copy={address}>{address}</Text> — the copy prop triggers clipboard write on tap, consistent with how other copyable values in the wallet are rendered.
Test coverage (src/test/screens/wallet/receive-lnurl.test.tsx:261)
New test covers: initial display on Unified, copy-on-tap, hidden on Arkade, visible again on Lightning. Sufficient for the surface changed. The Bitcoin/onchain method is not explicitly tested but follows the same branch as Arkade (!== 'unified' && !== 'lightning'); a follow-up is welcome but not blocking.
claimInSheet in src/test/screens/settings/lnurl.test.tsx is correctly updated: getAllByRole('textbox') returns [codeField, nameField] in DOM order, which now matches the rendered order for codeFirst scenarios (['admin', 'random'], ['admin', 'session']). Assertions on both placeholder values lock the ordering.
Summary
Clean single-commit delta. The codeFirst branching is correct, the showAddress guard is correct, and both paths have targeted test coverage. No protocol paths touched. No security concerns. The two lingering nits from prior passes are unaffected.
On @arkade-os/lnurl-client 0.9.0 and lnurl-server 0.6.0, a claim code can carry its own name: - On a server where a chosen name needs a code, the name field is now optional. A reserved name's code claims that name on its own. A code for any free name answers username_required, which the sheet explains: add a name and claim again. - A wallet that already has a lightning address can still redeem a code. Its addresses list offers "I have a claim code", and the claim registers a further address, since an upgrade only renames a nameless one. The sheet closes once the address changes and stays open with the reason when a claim is refused. The client bump is otherwise a drop-in: 0.9.0 adds registered methods and receive amounts, which the wallet does not use, and a facade given no methods never touches them.
…hat failed half way
On @arkade-os/lnurl-client 0.9.1, whose owned({ handle }) returns a given
active address:
- Settings → Lightning address lists a "Primary address" picker when the
wallet owns more than one active address. The choice is saved in the
config as lnurlPrimary, and both Settings and Receive pass it to
owned({ handle }), so Receive uses it straight away and after a restart.
A new choice switches the receiver in place, without reloading the rail.
- An address claimed with a code by an already named wallet becomes the
primary, so Receive no longer falls back to the older one (CodeRabbit).
- A claim can register its address and then fail to bind the identity,
leaving the address with no Arkade destination. After a failed claim, the
rail now runs a best-effort owned({ refresh: true }), which binds whatever
was left unbound (CodeRabbit).
- The username_required test goes on to add a name and claim (Arkana).
The picker hides while the claim sheet is open, because Select reads arrow
keys page-wide and the claim code field would feed it.
feat(lnurl): claim with a code alone or when already named, and choose the primary address
The rail can hold a nameless receiver while the identity owns a named address too: owned() picks the session address first, and the user can pick the nameless LNURL as primary. Settings and Receive then treated the wallet as unnamed and offered "Pick one for me". The rail now reports ownsName: the receiver's own name, or, when the receiver is nameless, any active named address in the identity's list, which it reads once. Both screens drop the random choice while it is true. Choosing a name, or claiming one with a code, stays available.
arkana-ai-bot
left a comment
arkana-ai-bot
left a comment
There was a problem hiding this comment.
Follow-up pass — 2 substantive commits (aa06628, f520f1c) + 1 merge since 27203da. 0 prior findings newly fixed this push (all were already resolved); 1 prior finding still open; 1 minor new finding.
No protocol-critical paths (VTXO, signing, forfeit, round lifecycle, unilateral exit) are touched in this diff.
Prior-finding closure
1. server?.domain optional access — Fixed (a01ce4ef, prior passes). Conditional spread unchanged.
2. handleChange spuriously async — Fixed (a01ce4ef, prior passes). Unchanged.
3. payRequest() failure silently lost — Fixed (a01ce4ef, prior passes). Unchanged.
4. receiveViaLnurl/receiveViaTokens missing from Config — Fixed (p11 merge, prior passes). Unchanged.
5. CLAIM_ERROR_KEYS typing nit — Fixed (c5f2fd3, prior pass). Unchanged; Partial<Record<...>> now in place, and username_required has been added to it this push without reopening the gap.
6. No test for lnurlRail.status === 'failed' render path in QrCode.tsx — Still open. Nothing in this increment touches that path. Carried forward unchanged.
Incremental findings
aa06628 + f520f1c — claim-code-alone, second-address claim, primary selection
src/screens/Wallet/Receive/LnurlRail.tsx — canClaim logic change (line ~63)
Correct. The old guard required a username in every path, including admin-only where the code alone may carry the reserved name. The new guard accepts a code alone for admin mode (codeFirst = admin && !self). In self-mode, username is still required. In self+admin mode, either satisfies the guard, and a code-only submission gracefully receives username_required back from the server with a user-facing message — the two-step flow is tested in 'asks for a name when the code is one for any free name'. Logic is sound.
src/screens/Wallet/Receive/LnurlRail.tsx — onClick cleanup (line ~102)
Correct. The old form always sent username (potentially empty string) even in code-only mode. The new form omits absent fields. Test at 'claims a reserved name with its code alone' asserts the request body lacks a username key when only the code is provided. ✓
src/lib/receive/lnurlRail.ts:311 — bind-recovery result silently dropped (minor)
When a bind fails, facade.owned({ refresh: true }) is fired in the background to complete the bind — tested and confirmed working ('binds an address its claim registered but failed to bind' checks 2 calls to the bind endpoint). The recovery is correct server-side, but the resolved value is not consumed: there is no .then((next) => next && setReceiver(next)). As a result, the wallet stays in the error/onboarding state for the rest of the session even after the silent recovery succeeds; the user must navigate away and back to see their address.
Not a correctness or security issue — the bind does complete — but the UX leaves the user on an error screen after a problem that has already been resolved. Compare with the primary-switch effect, which does wire .then((next) => !stale && next && setReceiver(next)). Wiring the refresh result to setReceiver in the same pattern would close the gap. Low priority given how rare a partial bind failure is, but worth a follow-up before this path becomes more common.
src/lib/receive/lnurlRail.ts:257-268 — primary-switch useEffect (correct, note only)
The second effect intentionally excludes receiver from its dep array (eslint-disable comment explains why: adding it would cause the effect to re-run on every receiver update, creating loops). The stale flag is in place and correctly prevents stale setter calls. The guard receiver.handle === primary reads the closure value of receiver, which is the value from the last render that caused the dep array to change — that is the correct value to compare against when deciding whether a switch is needed. Logic is sound.
src/screens/Settings/Lnurl.tsx — primary saved only after a successful second-address claim (correct)
lnurlPrimary is written only when claimed is a Receiver (not undefined) and the wallet already had a named address. A username_required rejection returns undefined → no save. A complete failure returns undefined → no save. The useEffect(() => setClaiming(false), [rail.receiver?.lightningAddress]) sheet-close hook fires on address change, so a successful claim closes the sheet; a rejected one leaves it open with the error. Correct.
src/screens/Settings/Lnurl.tsx — Select hidden during claim (correct)
The {active.length > 1 && !claiming ? ... : null} guard is well-motivated: the Select component captures arrow-key events page-wide, which would interfere with claim-code and username fields inside the sheet. Good defensive UX.
@arkade-os/lnurl-client 0.8.0 → 0.9.1
Minor version bump; peer-dependency constraint (>=0.5.0-rc.0) unchanged. The new API surface (owned({ handle }), owned({ refresh: true }), username_required error code) is exactly what this push exercises. No cross-repo API surface exposed.
Summary
Two substantive commits land cleanly: claim-code-alone UX works correctly, the second-address claim flow is well-guarded, and primary address selection is correctly persisted. One minor follow-up: wire the bind-recovery result in claim() to setReceiver so a silently-recovered address surfaces in the current session without requiring navigation. The long-standing gap for status === 'failed' rendering in QrCode.tsx remains open but unchanged.
arkana-ai-bot
left a comment
arkana-ai-bot
left a comment
There was a problem hiding this comment.
Follow-up pass — 2 substantive commits (aa06628, f520f1c) + 1 merge since 27203da. 0 prior findings newly fixed this push (all were already resolved); 1 prior finding still open; 1 minor new finding.
No protocol-critical paths (VTXO, signing, forfeit, round lifecycle, unilateral exit) are touched in this diff.
Prior-finding closure
1. server?.domain optional access — Fixed (a01ce4ef, prior passes). Conditional spread unchanged.
2. handleChange spuriously async — Fixed (a01ce4ef, prior passes). Unchanged.
3. payRequest() failure silently lost — Fixed (a01ce4ef, prior passes). Unchanged.
4. receiveViaLnurl/receiveViaTokens missing from Config — Fixed (p11 merge, prior passes). Unchanged.
5. CLAIM_ERROR_KEYS typing nit — Fixed (c5f2fd3, prior pass). Partial<Record<...>> in place; username_required added to it this push without reopening the gap.
6. No test for lnurlRail.status === 'failed' render path in QrCode.tsx — Still open. Nothing in this increment touches that path. Carried forward unchanged.
Incremental findings
aa06628 + f520f1c — claim-code-alone, second-address claim, primary selection
src/screens/Wallet/Receive/LnurlRail.tsx — canClaim logic rewrite (line ~63)
Old: const canClaim = Boolean(username) && (self || Boolean(code))
New: const canClaim = Boolean(code) || (self && Boolean(username))
Correct. The old guard required a username in every path, including admin-only where the code alone may carry the reserved name. The new guard accepts a code alone for admin mode (codeFirst = admin && !self). In self-mode, username is still required. In self+admin mode, either satisfies the guard, and a code-only submission gracefully receives username_required back from the server with a user-facing message — the two-step flow is tested in 'asks for a name when the code is one for any free name'. Logic is sound.
src/screens/Wallet/Receive/LnurlRail.tsx — onClick omits empty fields (line ~102)
Old form sent username (potentially the empty string) even in code-only mode. New form uses ...(username && { username }) / ...(code && { claimCode: code }) to omit absent fields. Test at 'claims a reserved name with its code alone' asserts the request body lacks a username key when only the code is provided. ✓
src/lib/receive/lnurlRail.ts:311 — bind-recovery result silently dropped (minor)
When a bind fails, facade.owned({ refresh: true }) is fired as a background recovery — tested and confirmed that the second bind call reaches the server ('binds an address its claim registered but failed to bind' checks 2 calls to the bind endpoint). The recovery is correct server-side, but the resolved value is not consumed: there is no .then((next) => next && setReceiver(next)). As a result the wallet stays in the error/onboarding state for the rest of the session even after the silent recovery succeeds; the user must navigate away and back to see their address.
Not a correctness or security issue — the bind does complete — but the UX leaves the user on an error screen after a problem that has already been resolved. Compare with the primary-switch effect, which does wire .then((next) => !stale && next && setReceiver(next)). Wiring the refresh result to setReceiver in the same pattern would close the gap. Low priority given how rare a partial bind failure is, but worth a follow-up.
src/lib/receive/lnurlRail.ts:257-268 — primary-switch useEffect (correct, note only)
The second effect intentionally excludes receiver from its dep array (eslint-disable comment explains: adding it would cause re-runs on every receiver update, creating loops). The stale flag is in place and correctly prevents stale setter calls. The guard receiver.handle === primary reads the closure value of receiver from the last render that changed the deps — the right value to compare when deciding whether a switch is needed. Logic is sound.
src/screens/Settings/Lnurl.tsx — primary saved only after a successful second-address claim (correct)
lnurlPrimary is written only when claimed is a Receiver (not undefined) and the wallet already had a named address. A username_required rejection returns undefined → no save. A complete failure returns undefined → no save. The useEffect(() => setClaiming(false), [rail.receiver?.lightningAddress]) hook fires on address change, so a successful claim closes the sheet; a rejected one leaves it open with the error. ✓
src/screens/Settings/Lnurl.tsx — Select hidden during claim (correct)
The active.length > 1 && !claiming guard is well-motivated: the Select component captures arrow-key events page-wide, which would interfere with claim-code and username fields inside the sheet. Good defensive UX.
@arkade-os/lnurl-client 0.8.0 → 0.9.1
Minor version bump; peer-dependency constraint unchanged. The new API surface (owned({ handle }), owned({ refresh: true }), username_required error code) is exactly what this push exercises. No cross-repo API surface exposed.
Summary
Two substantive commits land cleanly: claim-code-alone UX is correct, the second-address claim flow is well-guarded, and primary address selection is correctly persisted. One minor follow-up: wire the bind-recovery result in claim() to setReceiver so a silently-recovered address surfaces in the current session without requiring navigation (src/lib/receive/lnurlRail.ts:311). The long-standing gap for status === 'failed' rendering in QrCode.tsx remains open but unchanged.
arkana-ai-bot
left a comment
arkana-ai-bot
left a comment
There was a problem hiding this comment.
Follow-up pass — 2 substantive commits (aa06628, f520f1c) + 1 merge since 27203da. 0 prior findings newly fixed this push (all were already resolved); 1 prior finding still open; 1 minor new finding.
No protocol-critical paths (VTXO, signing, forfeit, round lifecycle, unilateral exit) are touched in this diff.
Prior-finding closure
1. server?.domain optional access — Fixed (a01ce4ef, prior passes). Conditional spread unchanged.
2. handleChange spuriously async — Fixed (a01ce4ef, prior passes). Unchanged.
3. payRequest() failure silently lost — Fixed (a01ce4ef, prior passes). Unchanged.
4. receiveViaLnurl/receiveViaTokens missing from Config — Fixed (p11 merge, prior passes). Unchanged.
5. CLAIM_ERROR_KEYS typing nit — Fixed (c5f2fd3, prior pass). Partial<Record<...>> in place; username_required added to it this push without reopening the gap.
6. No test for lnurlRail.status === 'failed' render path in QrCode.tsx — Still open. Nothing in this increment touches that path. Carried forward unchanged.
Incremental findings
aa06628 + f520f1c — claim-code-alone, second-address claim, primary selection
src/screens/Wallet/Receive/LnurlRail.tsx — canClaim logic rewrite (line ~63)
Old: const canClaim = Boolean(username) && (self || Boolean(code))
New: const canClaim = Boolean(code) || (self && Boolean(username))
Correct. The old guard required a username in every path, including admin-only where the code alone may carry the reserved name. The new guard accepts a code alone for admin mode (codeFirst = admin && !self). In self-mode, username is still required. In self+admin mode, either satisfies the guard, and a code-only submission gracefully receives username_required back from the server with a user-facing message — the two-step flow is tested in 'asks for a name when the code is one for any free name'. Logic is sound.
src/screens/Wallet/Receive/LnurlRail.tsx — onClick omits empty fields (line ~102)
Old form sent username (potentially the empty string) even in code-only mode. New form uses ...(username && { username }) / ...(code && { claimCode: code }) to omit absent fields. Test at 'claims a reserved name with its code alone' asserts the request body lacks a username key when only the code is provided. ✓
src/lib/receive/lnurlRail.ts:311 — bind-recovery result silently dropped (minor)
When a bind fails, facade.owned({ refresh: true }) is fired as a background recovery — tested and confirmed that the second bind call reaches the server ('binds an address its claim registered but failed to bind' checks 2 calls to the bind endpoint). The recovery is correct server-side, but the resolved value is not consumed: there is no .then((next) => next && setReceiver(next)). As a result the wallet stays in the error/onboarding state for the rest of the session even after the silent recovery succeeds; the user must navigate away and back to see their address.
Not a correctness or security issue — the bind does complete — but the UX leaves the user on an error screen after a problem that has already been resolved. Compare with the primary-switch effect, which does wire .then((next) => !stale && next && setReceiver(next)). Wiring the refresh result to setReceiver in the same pattern would close the gap. Low priority given how rare a partial bind failure is, but worth a follow-up.
src/lib/receive/lnurlRail.ts:257-268 — primary-switch useEffect (correct, note only)
The second effect intentionally excludes receiver from its dep array (eslint-disable comment explains: adding it would cause re-runs on every receiver update, creating loops). The stale flag is in place and correctly prevents stale setter calls. The guard receiver.handle === primary reads the closure value of receiver from the last render that changed the deps — the right value to compare when deciding whether a switch is needed. Logic is sound.
src/screens/Settings/Lnurl.tsx — primary saved only after a successful second-address claim (correct)
lnurlPrimary is written only when claimed is a Receiver (not undefined) and the wallet already had a named address. A username_required rejection returns undefined → no save. A complete failure returns undefined → no save. The useEffect(() => setClaiming(false), [rail.receiver?.lightningAddress]) hook fires on address change, so a successful claim closes the sheet; a rejected one leaves it open with the error. ✓
src/screens/Settings/Lnurl.tsx — Select hidden during claim (correct)
The active.length > 1 && !claiming guard is well-motivated: the Select component captures arrow-key events page-wide, which would interfere with claim-code and username fields inside the sheet. Good defensive UX.
@arkade-os/lnurl-client 0.8.0 → 0.9.1
Minor version bump; peer-dependency constraint unchanged. The new API surface (owned({ handle }), owned({ refresh: true }), username_required error code) is exactly what this push exercises. No cross-repo API surface exposed.
Summary
Two substantive commits land cleanly: claim-code-alone UX is correct, the second-address claim flow is well-guarded, and primary address selection is correctly persisted. One minor follow-up: wire the bind-recovery result in claim() to setReceiver so a silently-recovered address surfaces in the current session without requiring navigation (src/lib/receive/lnurlRail.ts:311). The long-standing gap for status === 'failed' rendering in QrCode.tsx remains open but unchanged.
|
Follow-up pass — 2 substantive commits ( No protocol-critical paths (VTXO, signing, forfeit, round lifecycle, unilateral exit) are touched in this diff. Prior-finding closure1. 2. 3. 4. 5. 6. No test for Incremental findings
|
Master's #1107 fails when a translation key is not referenced by a literal t('...') call. The claim error messages were reached through a code-to-key map and a dynamic t(key), so six used keys read as unused. They are now literal t() calls in a code-to-message map; the messages are unchanged.
After a claim that registered its address but failed to bind it, the
background owned({ refresh: true }) bound the address, but its result was
dropped. A first-time claimer stayed on the error until they left the
screen. A wallet that had no address when it claimed now adopts the one the
refresh finds and clears the error. A wallet that already had one keeps it
(Arkana).
|
Thanks. The new minor finding (the refresh after a failed claim repaired the bind, but its result was dropped) is fixed in |
arkana-ai-bot
left a comment
arkana-ai-bot
left a comment
There was a problem hiding this comment.
Follow-up pass on 6a3a874 — 5 commits since 1011ecf; all prior actionable items remain fixed except one still-open nit; no new blocking findings.
Prior-issue closure
1. server?.domain optional access in yours TableData — Fixed (confirmed in earlier pass, unchanged)
2. async handleChange without await — Fixed (confirmed in earlier pass, unchanged)
3. payRequest() rejection not surfaced in UI — Fixed (confirmed in earlier pass, unchanged)
4. Merge-order dependency on #1083 (receiveViaLnurl/receiveViaTokens) — Fixed (split/p11 landed in base, confirmed in prior pass)
5. No test for lnurlRail.status === 'failed' render path in the receive screen — Still open
Nothing in this increment addresses it. Still low risk for a display-only code path; carrying forward as a follow-up recommendation.
New findings (incremental diff only)
TransactionsList.tsx — .content class coupling (src/components/TransactionsList.tsx, lines 276–278)
const getScrollElement = () => parentRef.current?.closest<HTMLElement>('.content') ?? null@tanstack/react-virtual degrades gracefully when getScrollElement returns null (it falls back to rendering everything, effectively disabling virtualisation). If the class name content is ever renamed or the component is embedded in a scroll root that lacks that class, virtualisation silently stops working with no error or warning — the regression would only appear as a performance problem, not a test failure. The new virtualisation test verifies the row count under the happy-path layout, but it would still pass with virtualisation disabled (all 300 rows rendered) because the expect(rendered).toBeLessThan(30) assertion would fire. Consider asserting a lower bound (e.g. > 10) alongside the upper bound to make the test a two-sided gate, and consider adding a data-testid or a dev-mode invariant log when the scroll parent cannot be located. Nit, not a blocker.
lnurlRail.ts — namedElsewhere silent-fail (src/lib/receive/lnurlRail.ts, lines 271–282)
facade.addresses()
.then((all) => !stale && setNamedElsewhere(all.some((a) => a.status === 'active' && a.lightningAddress)))
.catch((err) => consoleError(err, 'lnurl address list failed'))If facade.addresses() rejects, namedElsewhere stays false and the "Pick one for me" option appears to a wallet that already holds a named address. This is an UX regression (unnecessary option shown), not a security or protocol issue. The stale-flag cleanup is correct. Acceptable as-is; noting for completeness.
lnurlRail.ts — claimErrorMessages(t) semantics shift (src/lib/receive/lnurlRail.ts, lines 141–143)
- const key = CLAIM_ERROR_KEYS[error.code as LnurlAddressErrorCode]
- if (key) return t(key)
+ const message = claimErrorMessages(t)[error.code as LnurlAddressErrorCode]
+ if (message) return messageThe guard changed from "does the key exist?" to "is the translated string truthy?". An empty translation (blank string for a given locale) would fall through to the generic fallback. In practice every locale entry is non-empty, so this is harmless — but if a future translator sets a key to "" as a placeholder, the fallback message appears instead, silently. The motivation (making literal t('...') calls visible to the i18n linter's regex) is sound and the approach is correct; just noting the minor semantic drift.
i18n.ts / i18n.parity.test.ts — unused-keys lint (src/test/lib/i18n.parity.test.ts, lines 25–58)
The collectUsedKeys scan is regex-based on source text. Keys composed at runtime (e.g. `section.${variable}`) won't be found. Two manual exception blocks already exist (the lightningReceive/Send stem loop and the SETTINGS_OPTION_KEY injection). This is a known limitation of static-text analysis and the test is a net positive; no action needed. The cleanup of ~50 unused keys is clean and parity between en and es is maintained.
Summary
No protocol-critical paths touched. No new blocking issues. The five commits are clean: the virtualiser fix correctly targets the outer scroll container and computes scrollMargin via useLayoutEffect; the ownsName/namedElsewhere fix correctly suppresses the random-name option for wallets that hold an address elsewhere; the i18n cleanup removes genuinely dead keys and the new two-sided lint test locks future state. The one carry-over gap (test for the failed rail render path) remains a recommended follow-up.
The unreachable-server test asserted only the swap fallback; it now also asserts the 'Lightning address unavailable' line the failed rail renders, as on #1084.
…e base screen does The redesign defaulted every method to a payment link: lightning:<invoice>, bitcoin:?ark=, bitcoin:<address>. The base screen (#1084) puts the bare value in the QR for Lightning (the invoice, else the Lightning address, else the LNURL), Arkade and Bitcoin, and that is what payers' wallets scan. Each method's default now matches it. The two exceptions are deliberate: Unified and stablecoins keep their link, and Arkade and Bitcoin keep the link once an amount is set, where the bare address would silently drop the amount. A part the user picks is still remembered over the default. The Copy button beside the amount comes back. It copies what the QR shows, or a token quote's bare deposit address, since a wallet's address field rejects the payment URI. The raw-value caveat under the QR goes. A bare value is now the default, and the base screen never warned about it.
The unreachable-server test asserted only the swap fallback; it now also asserts the 'Lightning address unavailable' line the failed rail renders, as on #1084.
…e base screen does The redesign defaulted every method to a payment link: lightning:<invoice>, bitcoin:?ark=, bitcoin:<address>. The base screen (#1084) puts the bare value in the QR for Lightning (the invoice, else the Lightning address, else the LNURL), Arkade and Bitcoin, and that is what payers' wallets scan. Each method's default now matches it. The two exceptions are deliberate: Unified and stablecoins keep their link, and Arkade and Bitcoin keep the link once an amount is set, where the bare address would silently drop the amount. A part the user picks is still remembered over the default. The Copy button beside the amount comes back. It copies what the QR shows, or a token quote's bare deposit address, since a wallet's address field rejects the payment URI. The raw-value caveat under the QR goes. A bare value is now the default, and the base screen never warned about it.
LNURL in the wallet, on
@arkade-os/lnurl-client0.8.0, in one PR:This folds in #1073, #1078, #1081 and #1083, which were stacked under this PR. Their commits are unchanged. #1109 is merged in too: claim codes that carry their own name, and the primary-address picker. The receive redesign (#1086) stays a separate PR stacked on this one, and draft #1091 sits on #1086.
Dependencies
@arkade-os/lnurl-clientto the published 0.9.1 and@web3icons/reactto 4.1.24.owned({ handle }), which the wallet uses.methodsnever touches them.03c90955. It pins SDK 0.5.0-rc.28 and swap 0.1.0-rc.31 (Upgrade sdk@0.5.0-rc.28 and swap@0.1.0-rc.31 #1108), moves tests to vitest 5 (Security: vitest update + shadcn removal #1103), and adds an i18n check that fails on any key not used as a literalt('...')(I18n/cleanup #1107). Those pins are kept. The claim error messages are now literalt()calls, so the check sees them in use.Send and payment history
subscribe,all). Activity rows come fromlnurlActivity, the send form's ranges fromlnurlPayableRanges, and LNURL routing fromfirstQuote/isLnurlRail.method=arkbeside it instead of a second?(CodeRabbit).Receive
sessionLnurl()(the wallet LNURL shown before registration lands) andaddresses(). Claim errors map through the typedLnurlAddressErrorCode.signal, so a lock or reset stops it writing.VITE_LNURL_SERVERand optionalVITE_LNURL_DOMAIN. Deployments still setting only the removedVITE_LNURL_SERVER_URLmust rename it before rollout;docs/swaps.regtest.mdexplains how.Token receive
requestTokenQuote,tokenOptionAccepts, normalization, URI validation andchainName.Settings → Lightning address
lnurlRailLimits).selfmode, where a chosen name needs a code, the sheet asks for the code first, above the name.username_required. The sheet then asks for a name (lnurl-server 0.6.0, feat(addresses): claim codes for any free name, and a reserved name's code on its own ArkLabsHQ/lnurl-server#100).lnurlPrimaryand passed toowned({ handle })by both Settings and Receive. A new choice switches the receiver in place.owned({ refresh: true }), which binds anything left unbound. A wallet that had no address yet then adopts it.Tests
@arkade-os/lnurl-client's own suite covers the package's behaviour (packages/client/testin ArkLabsHQ/lnurl-server).lnurl.test.tsand its fixtures;resolveLnurlRailsboundary block;Known gap on mainnet
payoutReference). Mainnet arkd v0.9.22 fails the emulator's follow-up finalize after a one-shot submit, so self-claims never record it and these rows show a plain "Received". Fixed by ArkLabsHQ/arkd#181, and backfilled by fix(offline): record a landed self-claim from its spend, and backfill missed payouts ArkLabsHQ/lnurl-server#98. No wallet change is needed.Validation
d210447d: TypeScript, lint and Prettier pass, and the full unit suite passes with 118 files / 1,273 tests. That includes @bordalix'sTokenQuoteNotelayout and lint commits, the Max fix and the receive re-quote fix, whose tests failed first. The CodeRabbit fixes and the expired-quote and back-button changes each have tests that failed first.No persisted schema migration. No merge or deployment is included.
Summary by CodeRabbit
New Features
Improvements
Documentation
Tests