typescript #29
typescript #29
Conversation
📝 WalkthroughWalkthroughAdds a TypeScript frontend binding for LNI: package scaffolding, public types/errors, HTTP/encoding/transform/polling utilities, LNURL resolution, seven node adapters (Phoenixd, CLN, LND, NWC, Strike, Speed, Blink), factory/index exports, environment-gated real integration tests, and supporting docs/agent guidance. Changes
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
No actionable comments were generated in the recent review. 🎉 🧹 Recent nitpick comments
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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 19
🤖 Fix all issues with AI agents
In `@bindings/typescript/src/internal/encoding.ts`:
- Around line 3-6: The encodeBase64 function currently calls globalThis.btoa
which throws on non-Latin-1 input; update encodeBase64 to consistently UTF-8
encode the input first and then base64-encode the resulting bytes across
environments: use Buffer.from(input, 'utf-8').toString('base64') in Node when
Buffer exists, and in browsers use new TextEncoder().encode(input) to get a
Uint8Array then convert those bytes to a base64 string (e.g., binary string
chunking + globalThis.btoa) as the fallback; ensure you reference and replace
the current globalThis.btoa path and handle TextEncoder/Buffer presence within
encodeBase64 so non-ASCII API keys/passwords work consistently.
In `@bindings/typescript/src/internal/http.ts`:
- Around line 118-124: The call to response.text() is currently outside the
try-catch so any failure while reading the body will escape as a raw error; wrap
the await response.text() in the same error handling pattern used earlier:
surround it with a try-catch that throws a LniError('NetworkError', `Network
request failed: ${...}`, { cause: error }) (use the same message construction as
the existing catch) so all network/body-read failures are normalized to
LniError; update the code around response.text() and reuse the same error
variable naming to keep consistency.
- Around line 136-151: The function requestJson currently returns "{} as T" on
empty response which unsafely coerces callers into assuming a full object;
instead modify requestJson to throw an LniError when requestText(...) returns an
empty string (include a descriptive message and body/context), and mention in
docs/comments that callers expecting empty responses should use requestMaybeJson
or requestText; update any callers like listTransactions to handle or use
requestMaybeJson if they legitimately accept empty responses.
In `@bindings/typescript/src/lnurl.ts`:
- Around line 92-97: The parsed JSON is being unsafely cast to LnurlPayResponse
after checking for an LnurlError; add runtime validation to ensure required
fields (callback, minSendable, maxSendable, tag) exist and have the expected
types/values before returning as LnurlPayResponse. In the function that
currently uses maybeError and throws LniError, validate that callback is a
non-empty string, minSendable and maxSendable are numbers (or numeric strings
convertible to numbers) and tag equals the expected value, and throw a
descriptive LniError (or a new validation error) if any check fails; this
prevents downstream failures in requestInvoice and assertAmountRange when those
properties are undefined.
In `@bindings/typescript/src/nodes/blink.ts`:
- Around line 276-284: In payInvoice, the code currently returns empty
paymentHash and preimage after lnInvoicePaymentSend because that mutation
doesn't return those fields; update payInvoice to perform a follow-up payment
lookup (e.g., call Blink's payment lookup/get mutation using the
lnInvoicePaymentSend response or feeProbe identifiers) to retrieve and populate
paymentHash and preimage before returning, falling back to null/undefined if the
lookup still doesn't provide values; alternatively, make the return types
optional and add a brief comment documenting this Blink limitation. Ensure you
update the logic around lnInvoicePaymentSend, the returned object construction
(where paymentHash, preimage, feeMsats are set), and keep existing error
handling via LniError and satsToMsats intact.
In `@bindings/typescript/src/nodes/cln.ts`:
- Around line 66-68: ClnListOffersResponse and listOffers are returning CLN's
raw snake_case objects but typed as Offer (camelCase), so at runtime fields like
offer_id and single_use won't match Offer.offerId/singleUse; define a raw
response type (e.g., RawClnOffer { offer_id: string; single_use: boolean; ... }
and ClnListOffersResponse { offers: RawClnOffer[] }) and in listOffers map
payload.offers to convert snake_case to the Offer shape (offer_id -> offerId,
single_use -> singleUse, etc.) before returning; update any other functions
around lines ~300-306 that similarly return payload.offers to perform the same
mapping.
- Around line 341-363: ClnNode.listTransactions currently only queries
/v1/listinvoices so outgoing payments are omitted; update listTransactions to
also call /v1/listpays (or /v1/listsendpays as appropriate) via postJson, map
those pay entries to the same Transaction shape (add a helper like
payToTransaction or extend invoiceToTransaction logic to handle pay records),
merge the incoming (invoices) and outgoing (pays) transaction arrays, apply the
existing search filter to the combined list, and return the merged result
(optionally sorted by timestamp/created) so both incoming and outgoing
transactions are included; reference ClnNode.listTransactions,
invoiceToTransaction, and create/payToTransaction or similar.
- Around line 323-339: In lookupInvoice, don't fall back to using params.search
as a payment_hash; instead require params.paymentHash and return a clear error
if it's missing—remove the else-if that assigns query.payment_hash =
params.search and throw an LniError('Api', 'paymentHash is required') (or
similar) when params.paymentHash is undefined; keep using
postJson('/v1/listinvoices', query) with query.payment_hash set only from
params.paymentHash and then continue to call invoiceToTransaction on the
returned invoice.
In `@bindings/typescript/src/nodes/lnd.ts`:
- Around line 165-175: The expiresAt field in the createInvoice return uses a
relative duration (params.expiry ?? 86400) but must be an absolute Unix
timestamp; update the expiresAt assignment in the function that returns
emptyTransaction (the createInvoice/invoice construction in nodes/lnd.ts) to
compute Math.floor(Date.now() / 1000) + (params.expiry ?? 86400) so expiresAt is
a current-time + expiry-seconds Unix timestamp (ensure it stays an integer).
- Line 114: mapInvoice and createInvoice are setting expiresAt to a relative
expiry seconds value (parseOptionalNumber(invoice.expiry) and params.expiry)
instead of an absolute timestamp; change both to compute expiresAt as createdAt
+ expiry (convert expiry seconds to the same time unit as createdAt) — in
mapInvoice use the parsed createdAt (e.g.,
parseOptionalNumber(invoice.created_at) or the existing createdAt mapping) plus
parseOptionalNumber(invoice.expiry), and in createInvoice set expiresAt to
(createdAt + (params.expiry ?? 86400) * 1000 or seconds consistent with
Transaction.expiresAt) rather than directly assigning params.expiry; ensure you
use the same time unit everywhere and update references to parseOptionalNumber,
mapInvoice, and createInvoice accordingly.
- Around line 105-121: The mapInvoice function incorrectly sets feesPaid to
invoice.value_msat (the requested amount); change mapInvoice (in
bindings/typescript/src/nodes/lnd.ts) so feesPaid is the actual fees (for
incoming invoices usually 0) by replacing
parseOptionalNumber(invoice.value_msat) with 0 (or
parseOptionalNumber(invoice.fees_msat) if your LND response includes a fees
field), and keep amountMsats mapped from invoice.amt_paid_msat as currently
done; if you need to preserve the invoice requested amount, map
invoice.value_msat to a new field (e.g., requestedAmountMsats) instead of
feesPaid.
- Around line 185-190: The code currently builds a nested fee_limit object using
params.feeLimitPercentage and params.amountMsats; change this to set a flat
fee_limit_msat string (not an object) expected by /v2/router/send and compute it
from the payment amount when only a percentage is provided. Specifically, in the
block that references params.feeLimitPercentage and params.amountMsats, remove
creation of body.fee_limit object and instead set body.fee_limit_msat =
String(Math.floor(Number(params.amountMsats) * (params.feeLimitPercentage /
100))); ensure you use params.amountMsats and params.feeLimitPercentage to
compute the integer msat cap and store it as a string.
In `@bindings/typescript/src/nodes/nwc.ts`:
- Around line 102-115: The createInvoice method currently forces a zero amount
by using params.amountMsats ?? 0 which turns omitted amounts into literal 0;
change the call in createInvoice (the makeInvoice call) to pass
params.amountMsats directly so undefined is propagated for amountless invoices
(allowing payer-chosen amounts), matching behavior used by other backends like
Strike and Phoenixd and leaving the existing error handling and
nwcTransactionToLniTransaction conversion intact.
In `@bindings/typescript/src/nodes/phoenixd.ts`:
- Around line 249-254: In listTransactions, the expression params.from ?
params.from * 1000 : undefined treats 0 as falsy and drops a valid start offset;
change it to an explicit presence check (e.g., params.from !== undefined &&
params.from !== null) so that when params.from is 0 you still set query.from =
params.from * 1000, leaving other fields (limit, all) unchanged; update the
query construction in listTransactions to compute from using that explicit
check.
- Around line 110-154: In createInvoice, expiresAt is currently set to the
expiry duration (params.expiry ?? 3600) instead of an absolute Unix timestamp;
update the code paths that call emptyTransaction (both the InvoiceType.Bolt12
branch and the Bolt11 branch) to compute expiresAt as createdAt + (params.expiry
?? 3600) (use the same createdAt value you pass into emptyTransaction, e.g.,
Math.floor(Date.now()/1000) or the createdAt you already compute) so
Transaction.expiresAt is an absolute Unix timestamp rather than a duration.
In `@bindings/typescript/src/nodes/speed.ts`:
- Around line 150-170: The payInvoice implementation currently returns empty
paymentHash and preimage; update the payInvoice method to extract and return
actual identifiers from the Speed API response (the SpeedSendResponse returned
by postJson). Inspect fields on SpeedSendResponse such as payment_hash,
preimage, hash, speed_payment_hash, or nested objects like
withdrawal.id/withdrawal.payment_hash (or similar names used by the API) and map
them into the PayInvoiceResponse.paymentHash and PayInvoiceResponse.preimage; if
the API truly does not provide those values, return null/undefined in those
fields and update PayInvoiceResponse type (or add a comment) to document that
they may be missing so callers don’t assume non-empty values. Ensure you
reference payInvoice, PayInvoiceResponse, SpeedSendResponse, and postJson when
making the changes.
- Around line 93-107: The sendToTransaction function contains a dead ternary and
a hardcoded empty paymentHash: change the transaction type mapping so the
conditional reflects the real direction (e.g., map send.withdraw_method ===
'lightning' to 'outgoing' and the other branch to 'incoming' or whatever the
Speed API semantics require) instead of returning 'outgoing' for both branches,
and populate paymentHash from the actual Speed API field (check for properties
like send.payment_hash, send.paymentHash, send.pay_hash or similar) so
lookupInvoice can match by hash; if no payment-hash field exists in the API, set
paymentHash to null/undefined and document that hash-based lookup is unsupported
for SpeedNode (refer to sendToTransaction, send.withdraw_method, paymentHash and
lookupInvoice).
- Around line 125-127: The code currently computes sat amounts as
(params.amountMsats ?? 0) / 1000 which can produce fractional sats; update the
conversion in the Speed client to produce an integer sat value (e.g., use
Math.floor or Math.trunc on (params.amountMsats ?? 0) / 1000) before sending to
postJson so the API receives an integer sat amount; apply the same change in
both the createPayment path (where payload is created for '/payments' and typed
as SpeedCreatePaymentResponse) and the payInvoice path referenced in the review.
In `@readme.md`:
- Around line 296-314: The heading "#### typescript (frontend)" should be
demoted to the next level to preserve hierarchy—change that line to "###
typescript (frontend)" in the README so it follows the preceding h2 sections;
update the same heading token in the block that shows the TypeScript
install/build instructions (the markdown header string "#### typescript
(frontend)") to "### typescript (frontend)" to satisfy the static analysis rule.
🧹 Nitpick comments (26)
bindings/typescript/tsconfig.json (1)
13-13: Consider scopingvitest/globalstypes to test files only.Including
"vitest/globals"in the basetsconfig.jsonmakesdescribe,it,expect, etc. available as globals in all source files, not just tests. This can mask missing imports in production code. You could move this to a separatetsconfigfor tests or rely on explicit imports (which the test file already does on Line 1 of the LND test).bindings/typescript/src/__tests__/integration/speed.real.test.ts (1)
5-5:itIfis duplicated across all seven integration test files.Consider extracting this one-liner into
helpers.tsalongside the other shared utilities. Every test file defines the same conditional runner.bindings/typescript/src/__tests__/integration/blink.real.test.ts (1)
31-55: Consider wrapping inrunOrSkipKnownErrorfor consistency with other lookupInvoice tests.The Speed and Strike lookupInvoice tests use
runOrSkipKnownErrorto gracefully handle known API error patterns, but this Blink test does not. If the Blink API can return transient or expected errors (e.g., "not found"), those would cause a test failure here rather than a graceful skip.bindings/typescript/src/errors.ts (1)
21-23: Consider passingcausetosuper()directly.If your
tsconfigtargets ES2022+ (Node ≥20 supports it), you can pass{ cause }to theErrorconstructor and drop the manual assignment + cast.♻️ Suggested simplification
- constructor(code: LniErrorCode, message: string, options?: { status?: number; body?: string; cause?: unknown }) { - super(message); + constructor(code: LniErrorCode, message: string, options?: { status?: number; body?: string; cause?: unknown }) { + super(message, options?.cause !== undefined ? { cause: options.cause } : undefined); this.name = 'LniError'; this.code = code; this.status = options?.status; this.body = options?.body; - - if (options?.cause !== undefined) { - (this as Error & { cause?: unknown }).cause = options.cause; - } }bindings/typescript/package.json (2)
51-58:NODE_TLS_REJECT_UNAUTHORIZED=0disables TLS certificate validation in integration tests.This is common for local integration tests against Lightning nodes with self-signed certificates, but consider adding a comment in the README or contributing guide explaining why this is needed, so future contributors don't copy this pattern into production code.
60-68: Consider updating outdated dependencies.Version verification shows:
@getalby/sdk: ^7.0.0 is current ✓@scure/base: ^1.2.6 is outdated; v2.0.0 is available (note: already present as a transitive dependency via@scure/bip32,@scure/bip39, and nostr-tools in package-lock.json)websocket-polyfill: ^0.0.3 is significantly outdated; v1.0.0 (stable release) is now availableNo security advisories were found.
websocket-polyfillis used only in test helpers and would be a good candidate for updating to v1.0.0. Verify compatibility before updating@scure/baseto v2.0.0 despite it already being present in the dependency tree.bindings/typescript/src/internal/polling.ts (2)
20-35: Finalsleepbefore timeout adds unnecessary latency.When the last iteration finds the invoice still pending, the loop sleeps for
delayMsbefore re-checking the condition and emitting the final'failure'callback. This means the timeout fires up todelayMslate.Move the sleep to the top of the loop body (after the first iteration) or check the elapsed time before sleeping:
♻️ Suggested fix
while (Date.now() - startedAt <= maxDurationMs) { try { const tx = await args.lookup(); if (tx.settledAt > 0) { args.callback('success', tx); return; } args.callback('pending', tx); } catch { args.callback('failure'); } - await sleep(delayMs); + if (Date.now() - startedAt + delayMs <= maxDurationMs) { + await sleep(delayMs); + } else { + break; + } } args.callback('failure');
28-29: Transient errors are silently swallowed with no diagnostic info.When
lookup()throws,callback('failure')is emitted but the error is discarded. TheInvoiceEventCallbacktype doesn't carry an error parameter, so there's no way for consumers to distinguish a transient network blip from a permanent failure. Consider whether the callback type should be extended, or at minimum log/surface the error for debugging.bindings/typescript/src/nodes/nwc.ts (2)
29-30: Unnecessary buffer copy beforecrypto.subtle.digest.
digest()does not mutate its input. The copy on lines 29–30 can be removed:♻️ Simplification
async function sha256Hex(bytes: Uint8Array): Promise<string> { if (!globalThis.crypto?.subtle) { throw new LniError('Api', 'Web Crypto API is required to hash NWC preimages.'); } - const digestInput = new Uint8Array(bytes.length); - digestInput.set(bytes); - const digest = await globalThis.crypto.subtle.digest('SHA-256', digestInput); + const digest = await globalThis.crypto.subtle.digest('SHA-256', bytes); return bytesToHex(new Uint8Array(digest)); }
195-217:filterTransactionsappliespaymentHashas exact match butsearchas substring — document or align.The filtering logic uses exact equality for
paymentHash(line 202) but case-insensitive substring matching forsearch(lines 208-211). This is reasonable but the dual behavior could surprise callers. A brief JSDoc on the method or on theListTransactionsParamsfields would help clarify the contract.bindings/typescript/src/lnurl.ts (3)
50-53:needsResolutionhas a subtle inconsistency withdetectPaymentTypefor@detection.
detectPaymentTypeguards against LNURL strings containing@with!lower.startsWith('lnurl'), butneedsResolutionuses a baredestination.includes('@')without the same guard. If a bech32-encoded LNURL string somehow contains@,needsResolutionwould returntruefor the wrong reason. In practice this is extremely unlikely since bech32 doesn't encode@, but aligning the logic would be more robust:♻️ Suggested alignment
export function needsResolution(destination: string): boolean { const normalized = destination.trim().toLowerCase(); - return destination.includes('@') || normalized.startsWith('lnurl1'); + return (destination.includes('@') && !normalized.startsWith('lnurl')) || normalized.startsWith('lnurl1'); }
76-98:fetchLnurlPayusesrequestText+ manualJSON.parsewhilerequestInvoiceusesrequestJson.This inconsistency in how JSON responses are handled between
fetchLnurlPay(lines 77–90) andrequestInvoice(line 104) makes the code harder to follow. Consider usingrequestJsonin both places, or add a comment explaining whyrequestTextis preferred here (e.g., to inspect raw payload on error).
142-173: LNURL and Lightning Address resolution paths share identical tail logic — consider extracting.Lines 164–166 and 169–172 perform the same
fetchLnurlPay→assertAmountRange→requestInvoicesequence with only the URL derivation differing. A small helper would reduce duplication:♻️ Suggested extraction
+async function resolveViaLnurlPay(url: string, amountMsats: number, fetchFn: FetchLike): Promise<string> { + const lnurlPay = await fetchLnurlPay(url, fetchFn); + assertAmountRange(amountMsats, lnurlPay.minSendable, lnurlPay.maxSendable); + return requestInvoice(lnurlPay.callback, amountMsats, fetchFn); +} + export async function resolveToBolt11(...) { ... if (destinationType === 'lightning_address') { const { user, domain } = parseLightningAddress(destination.trim()); - const lnurlPay = await fetchLnurlPay(lightningAddressToUrl(user, domain), fetchFn); - assertAmountRange(amountMsats, lnurlPay.minSendable, lnurlPay.maxSendable); - return requestInvoice(lnurlPay.callback, amountMsats, fetchFn); + return resolveViaLnurlPay(lightningAddressToUrl(user, domain), amountMsats, fetchFn); } const lnurl = decodeLnurl(destination.trim()); - const lnurlPay = await fetchLnurlPay(lnurl, fetchFn); - assertAmountRange(amountMsats, lnurlPay.minSendable, lnurlPay.maxSendable); - return requestInvoice(lnurlPay.callback, amountMsats, fetchFn); + return resolveViaLnurlPay(lnurl, amountMsats, fetchFn); }bindings/typescript/src/nodes/phoenixd.ts (1)
330-348: Polling withlimit: 2500may be heavy for repeated poll iterations.Each poll iteration fetches up to 2500 transactions via
listTransactions(which itself makes two HTTP calls — incoming + outgoing). For long-running polls with short delays, this could place significant load on the Phoenixd instance.Consider using
lookupInvoicewhenpaymentHashis not set butsearchis, or reduce the limit.bindings/typescript/src/types.ts (1)
27-41: Consider narrowingTransaction.typeto a string literal union.All node implementations consistently use
'incoming'or'outgoing'. A union type would catch typos at compile time and improve DX.Proposed change
export interface Transaction { - type: string; + type: 'incoming' | 'outgoing'; invoice: string;bindings/typescript/src/nodes/blink.ts (4)
123-146:getBtcWalletId()makes a network call on every invocation — consider caching.
getBtcWalletIdis called from bothcreateInvoiceandpayInvoice, each triggering a full GraphQL round-trip. The wallet ID is unlikely to change during the lifetime of aBlinkNodeinstance.Proposed caching
export class BlinkNode implements LightningNode { private readonly fetchFn; private readonly timeoutMs?: number; private readonly baseUrl: string; + private cachedWalletId?: string; // ... private async getBtcWalletId(): Promise<string> { + if (this.cachedWalletId) { + return this.cachedWalletId; + } + const query = `...`; const response = await this.gql<BlinkMeQuery>(query); const wallet = response.me.defaultAccount.wallets.find((item) => item.walletCurrency === 'BTC'); if (!wallet) { throw new LniError('Api', 'No BTC wallet found in Blink account.'); } + this.cachedWalletId = wallet.id; return wallet.id; }
148-173:getInfo()duplicates the wallet query fromgetBtcWalletId().The same GraphQL query and wallet lookup logic exists in both methods.
getInfocould reusegetBtcWalletId(or a shared internal that returns the full wallet data) to reduce duplication.
387-402: Search filtering is inconsistent withmatchesSearchused by other nodes.PhoenixdNode uses the shared
matchesSearchutility (which checkspaymentHash,description,payerNote,invoice). BlinkNode rolls its own filter that checksdescription,paymentHash, andpreimage— but omitsinvoiceandpayerNote. Consider reusingmatchesSearchfor consistency across node implementations.Proposed fix
Import
matchesSearchfrom'../internal/transform.js'(already partially imported) and replace the inline filter:+import { emptyNodeInfo, emptyTransaction, matchesSearch, satsToMsats } from '../internal/transform.js'; const filtered = transactions.filter((tx) => { if (params.paymentHash && tx.paymentHash !== params.paymentHash) { return false; } - - if (!params.search) { - return true; - } - - const search = params.search.toLowerCase(); - return ( - tx.description.toLowerCase().includes(search) || - tx.paymentHash.toLowerCase().includes(search) || - tx.preimage.toLowerCase().includes(search) - ); + return matchesSearch(tx, params.search); });
350-359: Pagination fetches excess data —first: from + limitover-fetches then slices client-side.
firstis set toparams.from + params.limit, fetching all items from the start up to the desired page end. For largefromvalues, this transfers significant unnecessary data. The Blink API supports cursor-based pagination (after/before) which would be more efficient but would require tracking cursors.This is acceptable for now, but worth noting as a scalability limitation.
bindings/typescript/src/nodes/strike.ts (3)
132-171: Msats-to-BTC conversion is the inverse ofbtcToMsatsbut written inline.Lines 142 and 180 use
(params.amountMsats / 100_000_000_000).toFixed(8)to convert msats to BTC. This is the inverse of thebtcToMsatshelper intransform.ts. Consider extracting a sharedmsatsToBtchelper to reduce duplication and ensure the conversion factor stays consistent.
305-318: Search filtering is inconsistent with sharedmatchesSearchutility.Same issue as in
blink.ts: this inline filter checkspaymentHash,description, andinvoicebut notpayerNote, diverging from the sharedmatchesSearchutility used by PhoenixdNode. ReusingmatchesSearchwould ensure consistency across all node implementations.Proposed fix
Import
matchesSearchfrom'../internal/transform.js'and replace the inline filter:+import { btcToMsats, emptyNodeInfo, emptyTransaction, matchesSearch, parseOptionalNumber, toUnixSeconds } from '../internal/transform.js'; const filtered = txs.filter((tx) => { if (params.paymentHash && tx.paymentHash !== params.paymentHash) { return false; } - if (params.search) { - const search = params.search.toLowerCase(); - return ( - tx.paymentHash.toLowerCase().includes(search) || - tx.description.toLowerCase().includes(search) || - tx.invoice.toLowerCase().includes(search) - ); - } - return true; + return matchesSearch(tx, params.search); });
244-260: Swallowing 404 for outgoing payments is reasonable but worth documenting.The try/catch around the outgoing payments fetch (lines 251-260) silently returns an empty list on 404. This handles the case where Strike returns 404 when there are no payments. A brief inline comment explaining this would help future maintainers.
bindings/typescript/src/nodes/cln.ts (1)
222-227: Invoice label usesMath.random()which has non-trivial collision risk.
Math.floor(Math.random() * 1_000_000_000)generates ~30 bits of entropy. By the birthday paradox, collision probability reaches ~50% after ~31k invoices. CLN requires unique labels. Consider usingcrypto.randomUUID()or incorporating a timestamp.Proposed fix
- label: `lni.${Math.floor(Math.random() * 1_000_000_000)}`, + label: `lni.${Date.now()}.${Math.floor(Math.random() * 1_000_000)}`,bindings/typescript/src/nodes/lnd.ts (1)
199-201: Fragile error detection via string matching on response text.Checking
responseText.includes('"error"')can produce false positives if the string"error"appears within field values (e.g., an error description embedded in aresultpayload). Consider parsing the last JSON line first and then checking for anerrorproperty on the parsed object, or at minimum using a more targeted check.bindings/typescript/src/nodes/speed.ts (1)
204-212: Redundant null check after length check.After
if (!txs.length)throws on Line 204,txs[0]is guaranteed to be defined — theif (!tx)check on Line 209 is unreachable dead code.Proposed simplification
if (!txs.length) { throw new LniError('Api', 'No transactions found matching lookup parameters.'); } - const tx = txs[0]; - if (!tx) { - throw new LniError('Api', 'No transactions found matching lookup parameters.'); - } - return tx; + return txs[0]!;bindings/typescript/src/internal/http.ts (1)
57-90: Timeout timer leaks when the request completes before the deadline.
withTimeoutcreates asetTimeoutthat is only cleared when the signal aborts (via external signal or timeout itself). If the request succeeds before the timeout fires, the timer remains live in the event loop until it eventually fires and callscontroller.abort()on a completed request. This is benign in low-throughput scenarios but wasteful for long timeouts.One approach: return both the signal and a cleanup function, and call it in
requestTextafter the fetch completes.Sketch
-function withTimeout(signal: AbortSignal | undefined, timeoutMs: number | undefined): AbortSignal | undefined { +function withTimeout(signal: AbortSignal | undefined, timeoutMs: number | undefined): { signal: AbortSignal | undefined; clear: () => void } { if (!timeoutMs || timeoutMs <= 0) { - return signal; + return { signal, clear: () => {} }; } const controller = new AbortController(); const timeoutId = setTimeout(() => controller.abort(), timeoutMs); + const clear = () => clearTimeout(timeoutId); if (signal) { if (signal.aborted) { clearTimeout(timeoutId); controller.abort(); } else { signal.addEventListener( 'abort', () => { clearTimeout(timeoutId); controller.abort(); }, { once: true }, ); } } - controller.signal.addEventListener( - 'abort', - () => { - clearTimeout(timeoutId); - }, - { once: true }, - ); - - return controller.signal; + return { signal: controller.signal, clear }; }Then in
requestText, callclear()after fetch completes (in afinallyblock).
There was a problem hiding this comment.
Actionable comments posted: 10
🤖 Fix all issues with AI agents
In `@bindings/typescript/src/__tests__/integration/cln.real.test.ts`:
- Line 27: Fix inconsistent indentation for the console.log statement in the
test by replacing the 3-space indent before console.log('CLN Invoice:', invoice)
with the same 4-space indentation used by the surrounding block (align with
other statements in the test). Locate the console.log in the cln.real.test.ts
integration test and adjust its leading whitespace so it matches the block's
indentation style.
In `@bindings/typescript/src/__tests__/integration/lnd.real.test.ts`:
- Line 3: The createInvoice test currently doesn't use the imported helper
runOrSkipKnownError, causing a partially unused import; wrap the createInvoice
test invocation (the test that calls createInvoice) with runOrSkipKnownError the
same way getInfo is wrapped so the test is skipped on known LND error
conditions, and remove the unused import only if you intentionally choose not to
wrap the test (reference the runOrSkipKnownError helper and the createInvoice
test name to find where to apply the wrapper).
- Around line 23-38: Wrap the test body for "createInvoice + lookupInvoice +
listTransactions" in runOrSkipKnownError so network/unreachable LND errors are
caught and the test is skipped instead of failing hard; locate the test that
calls makeNode() and then node.createInvoice, node.lookupInvoice, and
node.listTransactions and call runOrSkipKnownError(async () => { ...existing
test code... }) around that async block (keeping the existing itIf(enabled)(...)
and timeout) so any known network errors thrown by
createInvoice/lookupInvoice/listTransactions are handled gracefully.
In `@bindings/typescript/src/nodes/blink.ts`:
- Around line 162-172: In getInfo() (Blink Node) the code uses the custodial
wallet balance for both sendBalanceMsat and receiveBalanceMsat which is
misleading; update the emptyNodeInfo call in getInfo() (after getBtcWallet() /
satsToMsats(sats)) to use sendBalanceMsat: satsToMsats(sats) but set
receiveBalanceMsat to 0 (or remove the receiveBalanceMsat field entirely) so
consumers aren’t misled about receive capacity for this custodial wallet.
- Around line 349-395: The pagination mixes server fetch size and client offset:
the local variable first (set to params.from + params.limit) is used in the gql
call and then you apply a client-side skip/slice (skip = params.from;
filtered.slice(skip, end)), which breaks when filtering reduces results;
instead, implement proper cursor-based server pagination or fetch only
params.limit items per request and use an after cursor; specifically, stop
computing first = params.from + params.limit and/or stop relying on client skip
on the filtered array—use the GraphQL cursor (after) returned from this.gql and
the transactions edges cursors to request the correct page, or set first =
params.limit and iterate using after cursors when params.from > 0 before
mapping/filtering in the transactions -> filtered -> slice pipeline
(functions/variables to change: first, this.gql call, use of after,
transactions, filtered, skip, and slice).
In `@bindings/typescript/src/nodes/cln.ts`:
- Around line 128-144: In invoiceToTransaction, createdAt is hardcoded to 0;
compute a sensible creation timestamp instead by deriving createdAt from CLN
fields if available: if invoice.expires_at and an expiry duration field exist
(e.g., invoice.expiry or invoice.expiry_seconds), set createdAt = expires_at -
expiry (clamp to >=0); otherwise fall back to invoice.paid_at (invoice.paid_at
|| invoice.settledAt equivalent) or as a last resort use
Math.floor(Date.now()/1000) so consumers don't get epoch-zero; update the
createdAt assignment in invoiceToTransaction accordingly and ensure units are
seconds to match other timestamps.
In `@bindings/typescript/src/nodes/lnd.ts`:
- Around line 271-273: listTransactions currently ignores the incoming
ListTransactionsParams and always fetches all invoices; update the async
listTransactions(_params: ListTransactionsParams) method to apply filtering and
limiting after mapping invoices: use params.paymentHash to filter mapped results
by matching transaction.paymentHash, use params.search to case-insensitively
match against relevant fields (e.g., memo/description or paymentHash) and
enforce params.limit and params.from (offset) when slicing the sorted array;
keep using this.mapInvoice(invoice) for mapping and ensure the final array is
sorted by createdAt before applying from/limit.
In `@bindings/typescript/src/nodes/speed.ts`:
- Around line 211-227: listTransactions currently ignores params.from so
pagination is incorrect; update the post-sort slicing of txs to apply the offset
and limit together (use params.from as the slice start and params.limit as the
length) instead of always slicing from 0, handling cases where params.from is
missing/negative (treat as 0) and where params.limit <= 0 (treat as no limit).
Locate the logic in listTransactions (after mapping rows via sendToTransaction
and sorting) and replace the existing .slice(0, params.limit > 0 ? params.limit
: undefined) with a slice that uses params.from and params.limit (e.g., start =
Math.max(0, params.from || 0), end = params.limit > 0 ? start + params.limit :
undefined) so filtering by paymentHash and other flow remains unchanged; ensure
changes integrate with fetchSendTransactions and sendToTransaction usages.
In `@bindings/typescript/src/nodes/strike.ts`:
- Around line 234-236: The settledAt calculation uses Date.parse(item.completed
?? '') which yields NaN when item.completed is undefined; update the logic in
the strike mapping (field settledAt) to only call toUnixSeconds(Date.parse(...))
when item.completed is a non-empty string (e.g., item.completed ?
toUnixSeconds(Date.parse(item.completed)) : 0) to avoid NaN, and apply the same
guard/pattern to the analogous conversions in listTransactions (the two
occurrences referenced around lines 278 and 296) so all date conversions use a
presence check before parsing.
- Around line 244-314: listTransactions merges separately-paginated receives and
payments so it can return up to 2×params.limit; after filtering and sorting
(variables: txs, filtered) truncate the final result to params.limit before
returning. Modify listTransactions to sort the filtered array and then .slice(0,
params.limit) (using params.limit) so callers receive at most the requested
number of transactions.
🧹 Nitpick comments (6)
bindings/typescript/src/types.ts (1)
118-171: Consider extracting shared config fields into a base type.All seven config interfaces repeat
socks5Proxy,acceptInvalidCerts, andhttpTimeout. Extracting these into a shared base interface would reduce duplication and make it easier to add future shared options.Example
interface BaseNodeConfig { socks5Proxy?: string; acceptInvalidCerts?: boolean; httpTimeout?: number; } export interface PhoenixdConfig extends BaseNodeConfig { url: string; password: string; } // Similarly for ClnConfig, LndConfig, etc.bindings/typescript/src/nodes/nwc.ts (1)
64-82: Silent fallback ingetInfomay mask real errors.When
this.client.getInfo()fails (line 74), the error is silently caught and a defaultNodeInfois returned. This is reasonable for NWC providers that don't supportgetInfo, but consider logging the error for debuggability.bindings/typescript/src/nodes/phoenixd.ts (1)
265-283:listTransactionsbypasses the privaterequestJsonhelper.These two calls use the module-level
requestJson+buildUrlwith manual auth headers, while the rest of the class uses the privatethis.requestJsonhelper (lines 70–79). This inconsistency means changes to auth or timeout defaults must be synchronized in two places.Proposed refactor
- const incoming = await requestJson<PhoenixdInvoiceResponse[]>( - this.fetchFn, - buildUrl(this.config.url, '/payments/incoming', query), - { - method: 'GET', - timeoutMs: this.timeoutMs, - headers: { authorization: this.authHeader() }, - }, - ); - - const outgoing = await requestJson<PhoenixdOutgoingPaymentResponse[]>( - this.fetchFn, - buildUrl(this.config.url, '/payments/outgoing', query), - { - method: 'GET', - timeoutMs: this.timeoutMs, - headers: { authorization: this.authHeader() }, - }, - ); + const incoming = await this.requestJson<PhoenixdInvoiceResponse[]>('/payments/incoming', { + method: 'GET', + query, + }); + + const outgoing = await this.requestJson<PhoenixdOutgoingPaymentResponse[]>('/payments/outgoing', { + method: 'GET', + query, + });Note: This requires the private
requestJsonhelper to support thequeryparameter, which it currently does not since it callsbuildUrlwithout forwarding query params. You'd need to update the helper to accept and forwardqueryfromargs:private async requestJson<T>(path: string, args: Parameters<typeof requestJson<T>>[2]): Promise<T> { - return requestJson<T>(this.fetchFn, buildUrl(this.config.url, path), { + return requestJson<T>(this.fetchFn, buildUrl(this.config.url, path, args?.query), { ...args,bindings/typescript/src/__tests__/integration/cln.real.test.ts (1)
14-37: NorunOrSkipKnownErrorwrapper — inconsistent with Phoenixd tests.The CLN integration tests don't use
runOrSkipKnownErrorto handle transient network failures, unlike the Phoenixd tests. If the CLN node is temporarily unreachable while env vars are set, these tests will fail instead of being gracefully skipped. Consider wrapping the test bodies for consistency, or importing only the helpers you actually use (line 3 importsrunOrSkipKnownErrorfrom helpers but it's not used in the Phoenixd test — actually it's not imported here at all, which is fine).bindings/typescript/src/factory.ts (1)
50-67: Add adefaultbranch to guard against unhandledkindvalues.If
BackendNodeConfigis ever extended with a new kind but this switch isn't updated, the function silently returnsundefinedat runtime despite theLightningNodereturn type. Adefaultbranch with an exhaustiveness check prevents this.♻️ Proposed fix
case 'blink': return new BlinkNode(input.config, options); + default: { + const _exhaustive: never = input; + throw new Error(`Unknown node kind: ${(_exhaustive as any).kind}`); + } } }bindings/typescript/src/nodes/strike.ts (1)
173-196:preimageis always empty inpayInvoiceresponse.If the Strike API does not return the preimage, this is acceptable, but consumers relying on the
PayInvoiceResponse.preimagefield for proof-of-payment will get an empty string with no indication it's unavailable.
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 Fix all issues with AI agents
In `@bindings/typescript/src/nodes/blink.ts`:
- Around line 307-318: lookupInvoice currently searches only the first 100
transactions (limit: 100) and can miss older matches; update lookupInvoice to
page through listTransactions using its cursor-based pagination (call
listTransactions repeatedly with the returned nextCursor/after token until a
transaction with paymentHash is found or no more pages) rather than a fixed
small limit, and ensure onInvoiceEvents (which delegates to lookupInvoice)
continues to work with the updated behavior; reference the lookupInvoice method
and the listTransactions call to implement a loop that checks each page's
results for params.paymentHash and returns the match or throws the LniError if
exhausted.
- Around line 361-369: The loop uses limit = params.limit > 0 ? params.limit :
Number.POSITIVE_INFINITY which allows an unbounded fetch when params.limit ≤ 0;
change this to apply a sensible max cap (e.g. DEFAULT_MAX_FETCH) instead of
POSITIVE_INFINITY. Update the initialization of limit and possibly pageSize so
when params.limit ≤ 0 you set limit = Math.min(DEFAULT_MAX_FETCH, someFallback)
and keep pageSize computed from that bounded limit; modify the while loop that
checks transactions.length < limit to rely on the bounded limit. Use a clearly
named constant (e.g. DEFAULT_MAX_FETCH or MAX_TRANSACTION_FETCH) and adjust any
related logic around pageSize/after/skipped in the same function (in blink.ts)
to ensure the loop terminates within the cap.
- Around line 201-207: The code is passing amount as a string to Blink's GraphQL
API; change the amount field in the this.gql call (where
BlinkInvoiceCreateResponse is used) to pass a numeric Sat amount instead of
String(Math.floor((params.amountMsats ?? 0) / 1000)). Remove the String(...)
wrapper so amount is Math.floor((params.amountMsats ?? 0) / 1000) (a number) and
leave the rest (walletId, memo from params.description) intact to satisfy
LnInvoiceCreateInput.amount being a numeric SatAmount.
In `@bindings/typescript/src/nodes/cln.ts`:
- Around line 314-322: getOffer currently returns an empty partial Offer when
listOffers yields no results; change it to throw an LniError (matching
lookupInvoice's behavior) instead of returning a bogus object so callers can
detect "not found". Locate the getOffer function and, after awaiting
this.listOffers(search), if offers.length === 0 throw new LniError('Offer not
found', { search }) (or similar contextual message/metadata), otherwise return
offers[0]; preserve the Promise<Offer> signature and import/use the existing
LniError type used by lookupInvoice.
- Around line 246-263: The returned Transaction sets expiresAt to the expiry
duration instead of an absolute epoch time; update the code paths that call
postJson (both the Bolt11 branch using postJson('/v1/invoice') and the Bolt12
branch) so expiresAt is computed as current Unix time plus the expiry duration
(e.g. Math.floor(Date.now()/1000) + (params.expiry ?? 3600)) before calling
emptyTransaction; adjust any places using params.expiry to treat it as a
duration and ensure emptyTransaction receives an absolute timestamp for
expiresAt.
In `@bindings/typescript/src/nodes/speed.ts`:
- Around line 217-230: The pagination is applied before filtering by
params.paymentHash, causing matching transactions outside the sliced window to
be dropped; change the sequence so you apply the paymentHash filter prior to
slicing (and after mapping to transaction objects) — e.g., map rows via
this.sendToTransaction, then if params.paymentHash is present filter the mapped
txs by tx.paymentHash === params.paymentHash, then sort by createdAt and finally
apply the slice using params.from and params.limit so the returned txs respect
both the filter and pagination.
In `@bindings/typescript/src/nodes/strike.ts`:
- Around line 173-196: The payInvoice flow can lose the payment result if the
final getJson call fails after
patchJson(`/payment-quotes/${quote.paymentQuoteId}/execute`) succeeds; modify
payInvoice (which calls postJson, patchJson, getJson) to wrap the final fetch of
the payment (the getJson(`/payments/${execution.paymentId}`) call) in a
try/catch and, on failure, surface execution.paymentId (e.g., return a response
containing paymentId and best-effort fee/payment fields or throw an error that
includes execution.paymentId) so callers can independently recover the payment
state by calling getJson with execution.paymentId.
- Around line 244-255: In listTransactions, the payments call uses non-OData
params (skip/top) so pagination will fail; update the getJson call that fetches
StrikePaymentsResponse from '/payments' to pass OData-style parameters ('$skip'
and '$top') matching how receives uses '$skip'/'$top' and how the API expects
pagination; keep the same param sources (params.from and params.limit) when
replacing skip/top with '$skip'/'$top'.
🧹 Nitpick comments (4)
bindings/typescript/src/nodes/cln.ts (1)
332-345: Misleading variable name:bolt11holds a BOLT12 invoice.
fetchInvoiceFromOfferreturns a BOLT12 invoice string, but it's stored in a variable namedbolt11and passed as thebolt11key. While the CLNpayAPI accepts either format via thebolt11parameter, the local variable name is confusing.Suggested rename
- const bolt11 = await this.fetchInvoiceFromOffer(offer, amountMsats, payerNote); + const invoice = await this.fetchInvoiceFromOffer(offer, amountMsats, payerNote); const payload = await this.postJson<ClnPayResponse>('/v1/pay', { - bolt11, + bolt11: invoice, maxfeepercent: 1, retry_for: 60, });bindings/typescript/src/nodes/blink.ts (1)
393-405:settledAtusescreatedAtas a stand-in, which is inaccurate.Line 400 sets
settledAttonode.createdAtfor successful transactions. Settlement can occur well after creation (e.g., multi-hop payments). If Blink's API provides a separate settlement timestamp, prefer that. Otherwise, a brief comment noting the approximation would help future maintainers.bindings/typescript/src/nodes/strike.ts (2)
317-319:decodeis a no-op — is this intentional?Returning the input string unchanged means callers get no decoded invoice information (e.g., amount, destination, expiry). Other node adapters may provide richer decoding. If Strike doesn't offer a server-side decode endpoint, consider using a client-side bolt11 decoder so the interface contract is meaningful.
83-89:content-type: application/jsonheader sent on GET requests.
headers()unconditionally setscontent-type: application/json, which is unnecessary (and technically incorrect per HTTP semantics) for GET requests that have no body. While most servers ignore it, some proxies or strict servers may reject the request.
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
1 participant
Summary by CodeRabbit