fix: extract LNURL session into app-level provider with reusable sessions#559
fix: extract LNURL session into app-level provider with reusable sessions#559Kukks wants to merge 8 commits into
Conversation
Move the SSE connection lifecycle from the useLnurlSession hook into a new LnurlProvider so the session survives component unmount. When the user navigates away from the receive screen, the LNURL remains active and can still serve invoice requests from payers.
WalkthroughLNURL session handling was moved out of a removed ChangesLNURL session migration
Build & Submodule
Sequence DiagramsequenceDiagram
participant QR as QrCode Component
participant Context as LnurlProvider/Context
participant Server as LNURL Server
participant Swaps as Swaps Service
QR->>Context: read { lnurl, active, error }
Context->>Server: POST /lnurl/session (optional credentials/token)
Server-->>Context: SSE -> session_created (sessionId, lnurl, token?)
Context->>Context: set session state (lnurl, active, token)
Server-->>Context: SSE -> invoice_request { amountMsat,... }
Context->>Swaps: createReverseSwap(amount)
Swaps-->>Context: returns invoice (pr)
Context->>Server: POST /lnurl/session/:sessionId/invoice { pr } or { error }
Server-->>Context: 200 OK
QR->>Context: unmount/disable -> provider-managed cleanup
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
Deploying wallet-bitcoin with
|
| Latest commit: |
7739d52
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://1d2ed1c2.wallet-bitcoin.pages.dev |
| Branch Preview URL: | https://fix-lnurl-lifecycle.wallet-bitcoin.pages.dev |
Deploying wallet-mutinynet with
|
| Latest commit: |
7739d52
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://96d5fd79.arkade-wallet.pages.dev |
| Branch Preview URL: | https://fix-lnurl-lifecycle.arkade-wallet.pages.dev |
Deploying tmp-boltz-upstream-mainnet-arkade-wallet with
|
| Latest commit: |
7739d52
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://cc2e41d8.tmp-boltz-upstream-mainnet-arkade-wallet.pages.dev |
| Branch Preview URL: | https://fix-lnurl-lifecycle.tmp-boltz-upstream-mainnet-arkade-wallet.pages.dev |
|
@pietro909 maybe we should consider moving this code to the swaps package and also offering within service worker? |
Merge master and extend LNURL provider to support deterministic sessions. When a wallet address is available, derive sessionId and token via SHA-256 so the same wallet always gets the same LNURL. The server accepts optional client-provided credentials in POST /lnurl/session body.
Kukks
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@package.json`:
- Around line 45-46: The package.json scripts are inconsistent: "lint" uses
double quotes for the glob while "lint:fix" uses single quotes which breaks on
Windows; update the "lint:fix" script to use the same double-quoted glob pattern
as "lint" so both scripts use eslint "src/**/*.{ts,tsx,js,jsx}" and behave
consistently on Windows (modify the "lint:fix" script entry in package.json
accordingly).
In `@src/lib/lnurl.ts`:
- Around line 107-114: deriveLnurlCredentials currently derives both sessionId
and token deterministically from walletAddress (in deriveLnurlCredentials),
which makes the bearer token guessable by anyone who knows the public receive
address; change this so that sessionId can remain a deterministic/predictable
identifier but the token is generated unpredictably and issued/stored by the
server (or locally persisted securely), e.g., generate a cryptographically
random token (or HMAC with a server-only secret) for token instead of
sha256(walletAddress), return the stable sessionId if needed but replace the
token generation in deriveLnurlCredentials with a call/flow that obtains a
server-issued random token or creates a secure random token and persists it so
Authorization: Bearer cannot be recomputed from a public address; update any
callers (e.g., providers/lnurl.tsx and QrCode.tsx) to use the new
token-fetch/issue flow.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 58e8e490-ac66-4550-86fe-c124ab04d5c4
📒 Files selected for processing (7)
package.jsonregtestsrc/hooks/useLnurlSession.tssrc/index.tsxsrc/lib/lnurl.tssrc/providers/lnurl.tsxsrc/screens/Wallet/Receive/QrCode.tsx
sessionId is public (embedded in the LNURL QR), so deriving the token from the sessionId would let anyone who scans the QR compute the auth token. Instead, derive token from wallet address, then sessionId from token — the hash is one-way so the public sessionId doesn't leak the token.
![arkanaai[bot]](https://avatars.githubusercontent.com/u/170820085?s=60&v=4){kind=small}
There was a problem hiding this comment.
🔴 Arkana Code Review — wallet#559
CRITICAL: Session credentials derived from public data — enables payment theft
src/lib/lnurl.ts:106-111 — deriveLnurlCredentials() derives the session token and sessionId solely from the wallet's offchain address (walletAddress), which is public information (displayed as a QR code, embedded in BIP21 URIs, shared with payers).
const token = hex.encode(sha256(encoder.encode('lnurl-session:' + walletAddress)))
const sessionId = hex.encode(sha256(encoder.encode(token))).slice(0, 32)Attack scenario:
- Attacker obtains the victim's public Ark address (from a payment request, LNURL callback URL, etc.)
- Attacker computes
token = SHA-256("lnurl-session:" + arkAddress)andsessionId = SHA-256(token)[0:32] - Attacker calls
POST /lnurl/sessionwith{sessionId, token} - Per the companion PR (ArkLabsHQ/lnurl-server#2), the server destroys the existing session and creates a new one for the attacker's SSE stream
- Attacker now receives all
invoice_requestevents and can post their own bolt11 invoices - Payer's funds are redirected to the attacker
Even without the session-takeover vector, the token is computable by anyone who knows the address, so Bearer auth provides zero protection.
Fix: Derive credentials from a wallet secret (private key, seed, HMAC key), not the public address. E.g. HMAC-SHA256(walletPrivateKey, "lnurl-session-credentials"). This ensures only the wallet owner can compute valid credentials.
HIGH: PR description does not match implementation
The PR description claims:
sessionId= first 32 hex chars ofSHA-256("lnurl-session-id:" + walletAddress)token= full hex ofSHA-256("lnurl-session-token:" + walletAddress)
Actual code:
token = SHA-256("lnurl-session:" + walletAddress) // single prefix, not "lnurl-session-token:"
sessionId = SHA-256(token).slice(0, 32) // hash of token, not "lnurl-session-id:" + addressThe described "domain-separated inputs" don't exist. The sessionId is derived from the token (chained hash), not independently keyed. Update the description to match the code, or (better) fix the code to use the described domain separation.
MEDIUM: Missing React effect dependencies in useLnurlSession
src/hooks/useLnurlSession.ts:38-45
useEffect(() => {
if (enabled && !startedRef.current && !active) {
startedRef.current = true
startSession(onInvoiceRequest, credentials)
} else if (!enabled && startedRef.current) {
startedRef.current = false
stopSession()
}
}, [enabled]) // ← missing: credentials, active, startSession, stopSession- If
walletAddresschanges (e.g. wallet switch),credentialschanges but the effect won't re-run → stale session with old credentials. activeis read but not in deps → could use stale value from closure.startSession/stopSessionare stable (useCallback), but should still be listed per exhaustive-deps.
At minimum, add credentials and active to the dependency array.
LOW: lint:fix script not updated
package.json:45 — The lint script was changed from single to double quotes for Windows compatibility, but lint:fix still uses single quotes:
"lint:fix": "eslint 'src/**/*.{ts,tsx,js,jsx}' --fix",Should be consistent.
NOTE: No SSE reconnection on disconnect
Not a regression (old code had the same issue), but worth noting: if the SSE connection drops (network blip, server restart), the session dies silently. With deterministic credentials, auto-reconnect would be trivial and expected by users. Consider adding it.
Summary
The credential derivation from public data is a blocking security issue that enables payment theft. Everything else is secondary. Please derive credentials from a secret the wallet controls before merging.
Sign the message "lnurl-session" with the wallet's private key (Schnorr, deterministic via RFC 6979) and use the signature as the token. The sessionId is hash(token). This makes the token a true secret — knowing the public address is no longer enough to compute it.
There was a problem hiding this comment.
🔴 Arkana Code Review — wallet#559 (follow-up on new commits)
Previous review: credentials derived from public address → payment theft. The new commit (refactor: derive LNURL credentials from identity signature) addresses this by using identity.signMessage. The credentials now require the private key. Security issue resolved. ✅
However, the fix introduces a new functional bug:
HIGH: Schnorr signatures are non-deterministic — "reusable LNURL" feature is broken
src/lib/lnurl.ts (new lines 106-111)
const sig = await identity.signMessage(
new TextEncoder().encode('lnurl-session'), 'schnorr'
)
const token = hex.encode(sig)
const sessionId = hex.encode(sha256(new TextEncoder().encode(token))).slice(0, 32)identity.signMessage calls schnorr.signAsync(message, this.key) (in SingleKey), which defaults auxRand = randomBytes(32) per BIP340. This means:
- Same key + same message → different signature every time
- Every call to
deriveLnurlCredentialsproduces differenttoken/sessionId - On app restart, page refresh, or React remount, new credentials are derived
- The old LNURL session on the server is orphaned; a new one is created
- Payers cannot bookmark or reuse the LNURL — contradicts the stated goal
The Identity interface doesn't expose an auxRand parameter, so you can't just pass new Uint8Array(32) to make it deterministic.
Fix options (in preference order):
- HMAC-SHA256 with the private key —
deriveLnurlCredentialsshould accept the raw key material and useHMAC-SHA256(privkey, "lnurl-session-token"). Deterministic, doesn't leak the key, and doesn't require interface changes. You already have@noble/hashesin the dep tree. - Add a
signDeterministicoption to theIdentityinterface that passesauxRand = new Uint8Array(32). This couples the SDK to an implementation detail of BIP340 — less clean. - Persist the credentials to storage on first derivation and reuse them. Works around non-determinism but adds statefulness.
MEDIUM: Missing effect dependencies (partially fixed)
src/hooks/useLnurlSession.ts:37-44 — credentials was added to deps (good), but active, startSession, stopSession are still missing:
useEffect(() => {
if (enabled && !startedRef.current && !active && (credentials || !identity)) {
startedRef.current = true
startSession(onInvoiceRequest, credentials)
} else if (!enabled && startedRef.current) {
startedRef.current = false
stopSession()
}
}, [enabled, credentials]) // ← active, startSession, stopSession missingPractical impact: if the user switches wallets while a session is active, identity changes → credentials changes → effect re-runs, but startedRef.current is already true so the branch is skipped. The session continues with old credentials. Add logic to restart on credential change:
useEffect(() => {
if (enabled && credentials) {
startedRef.current = true
startSession(onInvoiceRequest, credentials) // startSession already calls stopSession internally
} else if (!enabled && startedRef.current) {
startedRef.current = false
stopSession()
}
}, [enabled, credentials, startSession, stopSession])LOW: lint:fix still uses single quotes (from previous review, not addressed)
package.json:45 — lint was fixed for Windows but lint:fix wasn't.
Summary
The security issue from the previous review is properly fixed — credentials now require the private key. However, the Schnorr non-determinism means the "reusable/bookmarkable LNURL" feature doesn't actually work. Every app restart generates a new LNURL. This is the blocking issue.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/hooks/useLnurlSession.ts (1)
26-32: 💤 Low valueRace on rapid
identitychanges.
deriveLnurlCredentials(identity).then(setCredentials)has no cancellation, so ifidentitychanges from A → B before A's promise resolves (e.g., wallet unlock/relock, fast HMR), A's credentials can land after B's and clobber the newer value. In the current wallet flowidentityis effectively stable, so this is mostly a future-proofing nit, but a simplecancelledflag in the effect cleanup would close it.Suggested guard
useEffect(() => { if (!identity) { setCredentials(undefined) return } - deriveLnurlCredentials(identity).then(setCredentials) + let cancelled = false + deriveLnurlCredentials(identity).then((c) => { + if (!cancelled) setCredentials(c) + }) + return () => { + cancelled = true + } }, [identity])🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/hooks/useLnurlSession.ts` around lines 26 - 32, The effect in useLnurlSession can set stale credentials when identity changes rapidly; modify the useEffect that calls deriveLnurlCredentials(identity).then(setCredentials) to use a local "cancelled" (or "active") flag captured by the effect, set it to true in the cleanup, and only call setCredentials if the flag indicates the result is still active; also ensure you still call setCredentials(undefined) immediately when identity is falsy. Reference the useEffect, deriveLnurlCredentials, setCredentials and identity identifiers when locating and implementing this guard.src/lib/lnurl.ts (1)
109-111: 💤 Low valueMinor: hash the raw signature bytes, not the hex-encoded token.
sessionIdcurrently hashes the hex string of the signature, which doubles the SHA-256 input size for no security benefit. Hashingsigdirectly is equivalent in collision resistance and avoids the redundant encode→encode round-trip. While here, you can also reuse a singleTextEncoder.Proposed simplification
export const deriveLnurlCredentials = async (identity: Identity): Promise<LnurlSessionCredentials> => { - const sig = await identity.signMessage(new TextEncoder().encode('lnurl-session'), 'schnorr') + const sig = await identity.signMessage(new TextEncoder().encode('lnurl-session'), 'schnorr') const token = hex.encode(sig) - const sessionId = hex.encode(sha256(new TextEncoder().encode(token))).slice(0, 32) + const sessionId = hex.encode(sha256(sig)).slice(0, 32) return { sessionId, token } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/lnurl.ts` around lines 109 - 111, sessionId is computed by hashing the hex-encoded signature string which is redundant and expands the input; instead hash the raw signature bytes and reuse a single TextEncoder for any text encoding needs. Update the block where sig, token and sessionId are created so that you still produce token = hex.encode(sig) if needed, but compute sessionId = hex.encode(sha256(sig)).slice(0, 32) (and create one TextEncoder instance if other text encoding is required elsewhere) to avoid the extra encode→encode round-trip; adjust references to sig, token and sessionId accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/hooks/useLnurlSession.ts`:
- Around line 38-46: The effect that manages the session (the useEffect using
startedRef, enabled, credentials, identity, active, onInvoiceRequest,
startSession, stopSession) must include all referenced values in its dependency
array and change its gating so it only starts when valid credentials are
available if an identity exists; update the logic to (a) list [enabled,
credentials, identity, active, onInvoiceRequest, startSession, stopSession] as
deps (or wrap callbacks in useCallback/memoize them), (b) when enabled is true
and either identity is null (anonymous) or credentials exist, startSession; but
if a session is already started with different credentials detect that (compare
stored/previous credentials) and call stopSession then startSession to rotate,
and (c) when enabled becomes false or credentials are removed, stopSession and
clear startedRef; ensure startedRef is reset on stop to allow restart when
credentials later arrive.
In `@src/lib/lnurl.ts`:
- Around line 108-113: deriveLnurlCredentials currently calls
identity.signMessage(..., 'schnorr') which may produce non-deterministic
signatures; either make the signature deterministic by passing a zeroed aux_rand
(if the SDK/identity.signMessage supports providing auxiliary randomness) when
calling identity.signMessage in deriveLnurlCredentials, or change
deriveLnurlCredentials to persist and reuse the first computed {sessionId,
token} (e.g., in localStorage/sessionStorage) so subsequent calls return the
same credentials for the same identity; update the deriveLnurlCredentials
function and any related initialization logic (useLnurlSession) to read/write
that stored credential instead of re-signing every time.
---
Nitpick comments:
In `@src/hooks/useLnurlSession.ts`:
- Around line 26-32: The effect in useLnurlSession can set stale credentials
when identity changes rapidly; modify the useEffect that calls
deriveLnurlCredentials(identity).then(setCredentials) to use a local "cancelled"
(or "active") flag captured by the effect, set it to true in the cleanup, and
only call setCredentials if the flag indicates the result is still active; also
ensure you still call setCredentials(undefined) immediately when identity is
falsy. Reference the useEffect, deriveLnurlCredentials, setCredentials and
identity identifiers when locating and implementing this guard.
In `@src/lib/lnurl.ts`:
- Around line 109-111: sessionId is computed by hashing the hex-encoded
signature string which is redundant and expands the input; instead hash the raw
signature bytes and reuse a single TextEncoder for any text encoding needs.
Update the block where sig, token and sessionId are created so that you still
produce token = hex.encode(sig) if needed, but compute sessionId =
hex.encode(sha256(sig)).slice(0, 32) (and create one TextEncoder instance if
other text encoding is required elsewhere) to avoid the extra encode→encode
round-trip; adjust references to sig, token and sessionId accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 374e731a-5138-4a59-bc2b-0a50a7b8992b
📒 Files selected for processing (3)
src/hooks/useLnurlSession.tssrc/lib/lnurl.tssrc/screens/Wallet/Receive/QrCode.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- src/screens/Wallet/Receive/QrCode.tsx
LnurlProvider now self-manages the session lifecycle — it starts when the wallet identity and Boltz swaps are ready, and stops when they aren't. The receive screen just reads lnurl/active/error from context. Invoice requests are handled entirely in the provider: create reverse swap, return the invoice, claim in background, notify on completion. The receive screen's existing VTXO_UPDATE listener handles navigation. Removes useLnurlSession hook (no longer needed).
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/providers/lnurl.tsx`:
- Around line 227-241: The effect can start a session after cleanup because the
in-flight deriveLnurlCredentials(identity) resolution only checks
abortRef.current (which stopSession sets to null), so replace that check with a
local cancelled flag captured by the effect: declare let cancelled = false at
start of the useEffect, set cancelled = true in the cleanup (which should also
call stopSession()), and after awaiting deriveLnurlCredentials(identity) check
if (cancelled) return before calling startSession(creds); also remove the
unreachable else-if branch that checks abortRef.current (or change its logic) so
stopSession() is invoked only from cleanup; key symbols: useEffect,
deriveLnurlCredentials, abortRef, startSession, stopSession,
svcWallet?.identity, ready.
- Around line 96-108: Change handleInvoiceRequest to accept an optional
AbortSignal parameter (e.g. handleInvoiceRequest(amountMsat: number, signal?:
AbortSignal)), pass that signal through where appropriate, and after awaiting
createReverseSwapRef.current(sats) immediately check signal?.aborted and bail
out (throw or return) before starting swapsRef.current.waitAndClaim or calling
notifyRef; also ensure any long-running calls (like waitAndClaim) are either
cancelled via the signal if they support it or not started when aborted so you
don't leave orphaned swaps. Reference: handleInvoiceRequest,
createReverseSwapRef.current, swapsRef.current.waitAndClaim, notifyRef.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 12ba5009-e065-4318-a070-6de238963a56
📒 Files selected for processing (3)
src/hooks/useLnurlSession.tssrc/providers/lnurl.tsxsrc/screens/Wallet/Receive/QrCode.tsx
💤 Files with no reviewable changes (1)
- src/hooks/useLnurlSession.ts
| const handleInvoiceRequest = useCallback(async (amountMsat: number) => { | ||
| const sats = Math.floor(amountMsat / 1000) | ||
| const pendingSwap = await createReverseSwapRef.current(sats) | ||
| if (!pendingSwap) throw new Error('Failed to create reverse swap') | ||
| const swaps = swapsRef.current | ||
| if (swaps) { | ||
| swaps | ||
| .waitAndClaim(pendingSwap) | ||
| .then(() => notifyRef.current(pendingSwap.response.onchainAmount ?? 0)) | ||
| .catch((err) => consoleError(err, 'Error claiming LNURL reverse swap')) | ||
| } | ||
| return pendingSwap.response.invoice | ||
| }, []) |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
TypeScript strict mode behavior for excess arguments to a function literal typed via parameter list inference (e.g., useCallback)
💡 Result:
In TypeScript strict mode, when a function literal (arrow function) is passed to a context where its type is inferred from the expected parameter list (e.g., as a callback to useCallback without explicit type annotation, or to a higher-order function), TypeScript infers the exact parameter types and arity from the context. Calling this inferred function with excess arguments (more than expected) will produce a type error like "Expected N arguments, but got N+1". This is standard function call checking under strictFunctionTypes, which is enabled in strict mode.[1][2][3][4] This behavior occurs because the function type is contravariant in parameters: a function expecting specific parameters cannot safely accept calls with incompatible arguments.[1][5] Functions with fewer parameters are assignable to those expecting more (extra args ignored at runtime), but not vice versa.[3][4][5] For useCallback specifically: const cb = useCallback( => {}, []); infers => void. Calling cb(arg) errors: "Expected 0 arguments, but got 1".[6][7][8] Excess property checks apply to object literals, not function arguments directly.[9][10] No special strict mode exception for function literals vs declarations; inference makes the type exact for calls.[11] Example: const callMe: (x: string) => void = (() => console.log('no arg'))(); // Error: fewer params OK for assign, but if reverse... // But inferred: const inferred = (x: string) => console.log(x); inferred('a', 1); // Error: excess arg No changes noted up to TS 7.0 beta.[12]
Citations:
- 1: https://www.typescriptlang.org/tsconfig/strictFunctionTypes.html
- 2: https://typescriptlang.org/tsconfig/strictFunctionTypes.html
- 3: Reject functions with not enough parameters on strict mode microsoft/TypeScript#17868
- 4: https://stackoverflow.com/questions/68813404/passed-function-without-arguments-doesnt-throw-an-error
- 5: https://stackoverflow.com/questions/74295408/does-allowing-extra-parameters-break-type-safety-in-typescript
- 6: https://stackoverflow.com/questions/62061218/react-typescript-and-usecallback
- 7: [@types/react] type for useCallback breaks --noImplicitAny guarantees DefinitelyTyped/DefinitelyTyped#52873
- 8: https://react.dev/reference/react/useCallback
- 9: https://stackoverflow.com/questions/68561033/object-assignment-with-excess-keys-seems-broken
- 10: https://www.codefixeshub.com/typescript/typing-objects-with-exact-properties-in-typescript
- 11: https://www.typescriptlang.org/docs/handbook/2/functions.html
- 12: https://dev.to/jtorchia/typescript-70-beta-i-ran-it-against-my-real-codebase-heres-what-changed-and-what-didnt-1mba
🏁 Script executed:
# First, let's check if the file exists and get its size
wc -l src/providers/lnurl.tsxRepository: arkade-os/wallet
Length of output: 88
🏁 Script executed:
# Read the relevant sections of the file
echo "=== Lines 96-108 ==="
sed -n '96,108p' src/providers/lnurl.tsx
echo ""
echo "=== Lines 190-198 (around line 194) ==="
sed -n '190,198p' src/providers/lnurl.tsxRepository: arkade-os/wallet
Length of output: 1160
🏁 Script executed:
# Search for all invocations of handleInvoiceRequest
rg "handleInvoiceRequest" src/providers/lnurl.tsx -n -A 2 -B 2Repository: arkade-os/wallet
Length of output: 635
🏁 Script executed:
# Check for createReverseSwapRef definition and usage
rg "createReverseSwapRef" src/providers/lnurl.tsx -n -B 3 -A 3Repository: arkade-os/wallet
Length of output: 879
🏁 Script executed:
# Search for other invocations of handleInvoiceRequest outside line 194
rg "handleInvoiceRequest\(" src/providers/lnurl.tsx -n | grep -v "const handleInvoiceRequest"Repository: arkade-os/wallet
Length of output: 148
🏁 Script executed:
# Check TypeScript configuration for strict mode
cat tsconfig.json 2>/dev/null || cat tsconfig.*.json 2>/dev/null || echo "No tsconfig found"Repository: arkade-os/wallet
Length of output: 598
🏁 Script executed:
# Find where createReverseSwap is imported or defined
rg "createReverseSwap" src/providers/lnurl.tsx -n -B 2 -A 2 | head -30Repository: arkade-os/wallet
Length of output: 1069
🏁 Script executed:
# Search across the entire codebase for createReverseSwap definition
rg "function createReverseSwap|const createReverseSwap|export.*createReverseSwap" --type ts --type tsx -A 5Repository: arkade-os/wallet
Length of output: 88
🏁 Script executed:
# Fix the rg command - use correct type flags
rg "function createReverseSwap|const createReverseSwap|export.*createReverseSwap" -t ts -t typescript -A 5Repository: arkade-os/wallet
Length of output: 95
🏁 Script executed:
# Search for SwapsContext definition
rg "SwapsContext" src/ -n -B 2 -A 10 | head -50Repository: arkade-os/wallet
Length of output: 4332
🏁 Script executed:
# Look for createReverseSwap in all files
fd . src/ -type f | xargs grep -l "createReverseSwap" | head -5Repository: arkade-os/wallet
Length of output: 231
🏁 Script executed:
# Find the SwapsContext provider file
find src -name "*swaps*" -type fRepository: arkade-os/wallet
Length of output: 84
🏁 Script executed:
# Search for SwapsContext provider definition
rg "SwapsContext.*createContext|export.*SwapsContext" -n -A 15Repository: arkade-os/wallet
Length of output: 2001
🏁 Script executed:
# Look at the swaps provider file
head -100 src/providers/swaps.tsx 2>/dev/null || head -100 src/providers/swaps.ts 2>/dev/nullRepository: arkade-os/wallet
Length of output: 3785
handleInvoiceRequest signature mismatch — abort.signal is silently dropped.
handleInvoiceRequest is declared as (amountMsat: number) => Promise<string> at line 96, but it is invoked with two arguments at line 194: await handleInvoiceRequest(amountMsat, abort.signal). This is an excess-argument error under TypeScript strict mode, and at runtime the signal is ignored entirely — neither the background waitAndClaim nor createReverseSwap observe abort. If the session is torn down mid-request, the reverse swap is created and then postInvoice aborts, leaving an orphaned swap on the server with no invoice delivered to the LNURL caller.
Accept the signal in the callback and bail out before doing additional work after createReverseSwap resolves (before kicking off waitAndClaim).
🛠️ Proposed fix
- const handleInvoiceRequest = useCallback(async (amountMsat: number) => {
+ const handleInvoiceRequest = useCallback(async (amountMsat: number, signal?: AbortSignal) => {
const sats = Math.floor(amountMsat / 1000)
const pendingSwap = await createReverseSwapRef.current(sats)
if (!pendingSwap) throw new Error('Failed to create reverse swap')
+ if (signal?.aborted) throw new DOMException('Aborted', 'AbortError')
const swaps = swapsRef.current
if (swaps) {
swaps
.waitAndClaim(pendingSwap)
.then(() => notifyRef.current(pendingSwap.response.onchainAmount ?? 0))
.catch((err) => consoleError(err, 'Error claiming LNURL reverse swap'))
}
return pendingSwap.response.invoice
}, [])📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const handleInvoiceRequest = useCallback(async (amountMsat: number) => { | |
| const sats = Math.floor(amountMsat / 1000) | |
| const pendingSwap = await createReverseSwapRef.current(sats) | |
| if (!pendingSwap) throw new Error('Failed to create reverse swap') | |
| const swaps = swapsRef.current | |
| if (swaps) { | |
| swaps | |
| .waitAndClaim(pendingSwap) | |
| .then(() => notifyRef.current(pendingSwap.response.onchainAmount ?? 0)) | |
| .catch((err) => consoleError(err, 'Error claiming LNURL reverse swap')) | |
| } | |
| return pendingSwap.response.invoice | |
| }, []) | |
| const handleInvoiceRequest = useCallback(async (amountMsat: number, signal?: AbortSignal) => { | |
| const sats = Math.floor(amountMsat / 1000) | |
| const pendingSwap = await createReverseSwapRef.current(sats) | |
| if (!pendingSwap) throw new Error('Failed to create reverse swap') | |
| if (signal?.aborted) throw new DOMException('Aborted', 'AbortError') | |
| const swaps = swapsRef.current | |
| if (swaps) { | |
| swaps | |
| .waitAndClaim(pendingSwap) | |
| .then(() => notifyRef.current(pendingSwap.response.onchainAmount ?? 0)) | |
| .catch((err) => consoleError(err, 'Error claiming LNURL reverse swap')) | |
| } | |
| return pendingSwap.response.invoice | |
| }, []) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/providers/lnurl.tsx` around lines 96 - 108, Change handleInvoiceRequest
to accept an optional AbortSignal parameter (e.g.
handleInvoiceRequest(amountMsat: number, signal?: AbortSignal)), pass that
signal through where appropriate, and after awaiting
createReverseSwapRef.current(sats) immediately check signal?.aborted and bail
out (throw or return) before starting swapsRef.current.waitAndClaim or calling
notifyRef; also ensure any long-running calls (like waitAndClaim) are either
cancelled via the signal if they support it or not started when aborted so you
don't leave orphaned swaps. Reference: handleInvoiceRequest,
createReverseSwapRef.current, swapsRef.current.waitAndClaim, notifyRef.
Schnorr signatures (BIP340) use random auxRand by default, making derived credentials non-deterministic across sessions. Switch to HMAC-SHA256(privateKey, "lnurl-session") which is deterministic by construction. Also fixes lint:fix script quoting for Windows.
Server now computes sessionId = SHA-256(token).slice(0, 32), so the
wallet only needs to send the token. Simplifies LnurlSessionCredentials
to { token } and removes redundant sessionId from derivation.
There was a problem hiding this comment.
✅ Arkana Code Review — wallet#559 (3rd pass, new commits only)
Reviewing commits since my last review (455e7549):
046d7513— refactor: start LNURL session at wallet level, not receive screen121fbbcf— fix: use deterministic HMAC-SHA256 for LNURL credential derivation7739d521— refactor: send only token to lnurl-server, let server derive sessionId
Previously flagged issues — resolved ✅
- CRITICAL: Credentials derived from public data → Fixed. Now uses
HMAC-SHA256(privateKey, "lnurl-session"). Requires private key, deterministic, one-way. Cryptographically sound. - HIGH: Non-deterministic Schnorr signatures → Fixed. HMAC is deterministic by construction. Same wallet → same token → same LNURL across sessions.
- Simplified credential model — server now derives
sessionId = SHA-256(token)[0:32], wallet only sendstoken. Clean separation.
New findings
MEDIUM: No SSE reconnection logic — src/providers/lnurl.tsx:137-195
If the SSE stream to lnurl-server drops (server restart, network blip during mobile sleep/wake), the finally block (line ~205) clears state and sets active=false, but nothing triggers a reconnect. The useEffect (line ~228) depends on [credentials, connected, !!arkadeSwaps, swapsInitError] — none of these change when only the LNURL SSE drops.
Partial mitigation: a network outage that kills the SSE stream will likely also disconnect the Boltz WebSocket, toggling connected false→true and re-triggering the effect. But if only the lnurl-server goes down while Boltz stays up, the session dies silently.
Suggestion: add a reconnect-with-backoff in the finally block (only if abort.signal wasn't explicitly aborted), or set a disconnected state that the effect can react to. Not a blocker for merge — the deterministic credentials mean a reconnect will recover the same LNURL — but should be a fast follow-up.
LOW: Duplicate browser notifications — src/providers/lnurl.tsx:103 + src/screens/Wallet/Receive/QrCode.tsx:307
When the user IS on the receive screen and an LNURL payment completes:
- Provider's
waitAndClaimcallback callsnotifyRef.current(amount)→ browser notification - QrCode.tsx's
VTXO_UPDATEservice worker listener callsnotifyPaymentReceived(sats)→ second browser notification
Result: two native notifications for one payment. Fix: either skip the notification in the provider (let the component handle it) or add a dedup guard. Low priority — cosmetic only.
NITPICK: Missing deps in useEffect — src/providers/lnurl.tsx:228-240
startSession and stopSession are not in the dependency array. Both are useCallback with stable deps so this is functionally safe, but it will trigger the react-hooks/exhaustive-deps ESLint rule. Consider adding them, or adding an // eslint-disable comment with a note.
Security summary
- ✅ Private key used only as HMAC key — HMAC is a PRF, doesn't leak key material
- ✅ Token (64 hex chars) is the HMAC output — not invertible to private key
- ✅ SessionId derived by server via
SHA-256(token)— public sessionId in QR doesn't reveal token - ✅ Bearer auth header uses token — only wallet owner can authenticate to their session
- ✅ No protocol-critical code paths changed (VTXO handling, signing, forfeit, exit all untouched)
Verdict
Approving. The critical security issue from my first review is properly resolved. The HMAC-SHA256 credential scheme is clean and correct. The architectural move to a provider-level session is sound. The reconnection gap is worth a follow-up PR but not a blocker.
🤖 Reviewed by Arkana
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/providers/lnurl.tsx (1)
95-107:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAbort race still allows side effects after session teardown.
At Line 95,
handleInvoiceRequeststill has no abort gating, so a teardown can occur after reverse-swap creation but before invoice post, whilewaitAndClaimstill starts. This can leave in-flight/orphaned work during session shutdown.Suggested minimal fix
- const handleInvoiceRequest = useCallback(async (amountMsat: number) => { + const handleInvoiceRequest = useCallback(async (amountMsat: number, signal?: AbortSignal) => { const sats = Math.floor(amountMsat / 1000) const pendingSwap = await createReverseSwapRef.current(sats) if (!pendingSwap) throw new Error('Failed to create reverse swap') + if (signal?.aborted) throw new DOMException('Aborted', 'AbortError') const swaps = swapsRef.current if (swaps) { swaps .waitAndClaim(pendingSwap) .then(() => notifyRef.current(pendingSwap.response.onchainAmount ?? 0)) .catch((err) => consoleError(err, 'Error claiming LNURL reverse swap')) } return pendingSwap.response.invoice }, [])- const pr = await handleInvoiceRequest(amountMsat) + const pr = await handleInvoiceRequest(amountMsat, abort.signal)Also applies to: 190-191
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/providers/lnurl.tsx` around lines 95 - 107, handleInvoiceRequest can start side-effects after session teardown; after creating pendingSwap, re-check an "active session" guard (e.g., a boolean like sessionActiveRef.current or an AbortSignal) before starting swapsRef.current.waitAndClaim and before calling notifyRef.current; if the session is torn down, skip starting waitAndClaim and return the invoice without launching background work. Apply the same pattern for the other handler referenced (around lines 190-191), and if waitAndClaim accepts a cancellation token/signal prefer passing that so the operation can be aborted instead of left running; also ensure errors still use consoleError.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@src/providers/lnurl.tsx`:
- Around line 95-107: handleInvoiceRequest can start side-effects after session
teardown; after creating pendingSwap, re-check an "active session" guard (e.g.,
a boolean like sessionActiveRef.current or an AbortSignal) before starting
swapsRef.current.waitAndClaim and before calling notifyRef.current; if the
session is torn down, skip starting waitAndClaim and return the invoice without
launching background work. Apply the same pattern for the other handler
referenced (around lines 190-191), and if waitAndClaim accepts a cancellation
token/signal prefer passing that so the operation can be aborted instead of left
running; also ensure errors still use consoleError.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8d0f3d6e-b1dd-4376-b4fb-2f7820f778e4
📒 Files selected for processing (3)
package.jsonsrc/lib/lnurl.tssrc/providers/lnurl.tsx
1 participant
Summary
LnurlProviderthat manages the LNURL session lifecycle at the wallet level — the session starts automatically when the wallet and Boltz swaps are readyProblem
Three issues fixed:
Session lifecycle: The LNURL session was managed inside a
useEffecton the receive screen. When the user navigated away, the SSE connection was killed and the payer's LNURL became invalid.Non-deterministic LNURL: Every session got a random ID, so the LNURL changed every time. Payers couldn't reuse a wallet's LNURL across sessions.
Session only active on receive screen: The LNURL session only started when the user was on the amountless receive screen. Now it's always active when the wallet is open.
Architecture
Credential derivation
deriveLnurlCredentials(privateKeyHex)insrc/lib/lnurl.ts:token= hex-encodedHMAC-SHA256(privateKey, "lnurl-session")— deterministic by construction{ token }to the serversessionId = SHA-256(tokenBytes).slice(0, 32)— eliminates mismatched pairs and makes session hijack impossible by construction@noble/hashes(hmac + sha256), synchronousCompanion PR
Test plan
Summary by CodeRabbit
Refactor
Chores