Conversation
📝 WalkthroughWalkthroughThis PR is largely a repository-wide code formatting pass, introducing Prettier configuration, formatting shell scripts, and a pre-commit git hook, then reformatting existing Rust crates and Node.js/TypeScript bindings into consistent multi-line style. It also refactors the NWC crate's request handling and adds a lightning-address helper. ChangesFormatting Tooling and Configuration
Rust Crate and Node.js Binding Changes
TypeScript Bindings Reformatting
Estimated code review effort: 2 (Simple) | ~15 minutes Sequence Diagram(s)sequenceDiagram
participant NwcNode
participant lightning_address_from_nwc_uri
participant execute_nwc_request
participant NWCRelay
NwcNode->>lightning_address_from_nwc_uri: parse nwc_uri for lud16
lightning_address_from_nwc_uri-->>NwcNode: validated Lightning Address
NwcNode->>execute_nwc_request: get_info / get_balance
execute_nwc_request->>NWCRelay: NIP-47 request
NWCRelay-->>execute_nwc_request: response or timeout
execute_nwc_request-->>NwcNode: NodeInfo / ApiError
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/README.md (1)
131-132: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRemove the maintainer-specific path from the notes.
This bakes one checkout path into the docs and will look wrong for everyone else. The earlier repository-root note is enough.
💡 Suggested fix
-Run from `/Users/nick/code/lni` or another checkout root. +Run from the repository root or any checkout root.🤖 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 `@scripts/README.md` around lines 131 - 132, Remove the maintainer-specific checkout path from the notes in the README and keep the generic repository-root guidance only. Update the text near the runtime/version note so it no longer references the hardcoded path, and preserve the Node 20.19+/22.12+ recommendation as-is.
🧹 Nitpick comments (1)
scripts/format-staged.sh (1)
51-51: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
cargo fmt --allformats the entire workspace, not just staged files.If a non-staged Rust file has formatting issues,
cargo fmt --allwill modify it, but lines 58–62 only re-stage files fromrust_files. This leaves unstaged formatting changes in the working tree, which could be confusing for the developer.This is a common tradeoff since
cargo fmtdoesn't support per-file formatting with--all. Consider documenting this behavior in the script or README so developers know to expect possible unstaged changes after commit.🤖 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 `@scripts/format-staged.sh` at line 51, `cargo fmt --all` in `format-staged.sh` formats the whole workspace, which can leave unstaged Rust changes behind after only `rust_files` are re-staged. Update the script behavior or accompanying developer docs/README to explicitly document that `cargo fmt --all --manifest-path "$ROOT/Cargo.toml"` may modify non-staged files, and that only formatted staged Rust files are re-added by the script; reference `format-staged.sh` and the `rust_files` re-stage logic so it’s easy to find.
🤖 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 `@bindings/lni_nodejs/src/lib.rs`:
- Around line 77-90: The HTTP client setup in the request-building logic should
not automatically disable TLS verification when socks5_proxy is present. Update
the client construction in the helper around reqwest::Client::builder and
reqwest::Proxy::all so that danger_accept_invalid_certs(true) is removed from
the default proxy path, and only allow invalid certs behind an explicit opt-in
flag or configuration. Keep the existing proxy fallback behavior intact while
ensuring the default client created by the function still validates
certificates.
In `@bindings/lni_nodejs/src/spark.rs`:
- Around line 287-309: Clamp the polling values in the invoice polling loop
before converting them to durations: in `OnInvoiceEventParams` handling inside
`spark.rs`, ensure `polling_delay_sec` and `max_polling_sec` are validated to
non-negative bounds before the `as u64` casts used by the `while
start_time.elapsed()` condition and `tokio::time::sleep`. Update the
`lookup_invoice` polling logic so negative JS inputs cannot wrap into huge
`Duration` values, keeping the behavior bounded and predictable.
In `@bindings/typescript/src/__tests__/integration/blink.real.test.ts`:
- Around line 24-137: Remove the sensitive console logging from the Blink real
integration tests so CI never prints full invoice, payment, or transaction
payloads. In the test cases that use makeNode, createInvoice,
prepareOnchainTransaction, and payOnchain, delete the console.log calls that
dump the returned objects or replace them with non-sensitive status messages
only. Keep the assertions and flow intact, but ensure no full
invoice/payment/preimage or on-chain transaction details are emitted.
In `@bindings/typescript/src/__tests__/integration/cln.real.test.ts`:
- Around line 14-59: The test in the createInvoice + lookupInvoice +
listTransactions case is logging the full invoice payload, which can leak
sensitive data into CI output. Remove the console.log from the CLN integration
test and keep the existing assertions on invoice.invoice, invoice.paymentHash,
lookupInvoice, and listTransactions in place without exposing the invoice
object.
In `@bindings/typescript/src/__tests__/integration/lnd.real.test.ts`:
- Around line 14-60: Remove the invoice logging from the LND integration test by
deleting the console output in the createInvoice + lookupInvoice +
listTransactions case; update the test around node.createInvoice so it no longer
prints the full invoice payload to CI logs, while keeping the existing
assertions on invoice.invoice and invoice.paymentHash intact.
In `@bindings/typescript/src/__tests__/integration/phoenixd.real.test.ts`:
- Around line 27-52: Remove the console logging of the full invoice object in
the phoenixd integration test. In the createInvoice + lookupInvoice +
listTransactions case, delete the console.log call that prints the invoice
payload and keep the assertions/use of invoice.invoice and invoice.paymentHash
only. This change should be made in the test body around makeNode() and
createInvoice() so CI logs do not expose sensitive invoice data.
In `@bindings/typescript/src/__tests__/integration/speed.real.test.ts`:
- Around line 22-40: Remove the invoice logging from the speed test to avoid
exposing sensitive payment data in CI logs. Update the test in the getInfo +
createInvoice + listTransactions block by deleting the console.log tied to the
invoice variable, while keeping the existing assertions and flow unchanged.
In `@bindings/typescript/src/__tests__/nwc.test.ts`:
- Around line 215-225: The payInvoice test no longer asserts the wrapped
NwcError payload, so it only checks the thrown type and weakens NIP-47 error
coverage. Update the test around makeNode().payInvoice and NwcError to keep the
stronger assertion by verifying the preserved error details from the rejected
promise, not just the instance type, alongside the existing LniError rewrap
case.
In `@crates/lni/lib.rs`:
- Around line 305-316: The SOCKS5 client setup in the reqwest builder is
disabling TLS verification globally via the client_builder path, which should
not be the default. Remove the danger_accept_invalid_certs(true) usage from the
proxy/client construction and keep certificate validation enabled in the normal
flow. If special handling for self-signed endpoints is needed, gate it behind an
explicit opt-in in the same client-building logic around
reqwest::Client::builder(), reqwest::Proxy::all, and the build fallback path.
In `@crates/lni/nwc/api.rs`:
- Around line 97-131: Extract the duplicated pubkey parsing in NodeInfo
construction into a dedicated helper, and use it in both the get_info success
path and the Err fallback. Update the logic in the api.rs NWC handling code to
normalize all NWC URI scheme variants the same way as
lightning_address_from_nwc_uri does, so pubkey extraction works for
nostr+walletconnect://, nostrwalletconnect://, nostrwalletconnect:, and
nostr+walletconnect:. Keep the existing NodeInfo and config.nwc_uri call sites,
but route them through the new helper so the fallback no longer silently returns
an empty pubkey.
---
Outside diff comments:
In `@scripts/README.md`:
- Around line 131-132: Remove the maintainer-specific checkout path from the
notes in the README and keep the generic repository-root guidance only. Update
the text near the runtime/version note so it no longer references the hardcoded
path, and preserve the Node 20.19+/22.12+ recommendation as-is.
---
Nitpick comments:
In `@scripts/format-staged.sh`:
- Line 51: `cargo fmt --all` in `format-staged.sh` formats the whole workspace,
which can leave unstaged Rust changes behind after only `rust_files` are
re-staged. Update the script behavior or accompanying developer docs/README to
explicitly document that `cargo fmt --all --manifest-path "$ROOT/Cargo.toml"`
may modify non-staged files, and that only formatted staged Rust files are
re-added by the script; reference `format-staged.sh` and the `rust_files`
re-stage logic so it’s easy to find.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 355663b8-af64-4e8b-b35a-8efee565172c
📒 Files selected for processing (93)
.githooks/pre-commit.prettierignore.prettierrc.jsonbindings/lni_nodejs/src/blink.rsbindings/lni_nodejs/src/cln.rsbindings/lni_nodejs/src/lib.rsbindings/lni_nodejs/src/lnd.rsbindings/lni_nodejs/src/lnurl.rsbindings/lni_nodejs/src/nwc.rsbindings/lni_nodejs/src/phoenixd.rsbindings/lni_nodejs/src/spark.rsbindings/lni_nodejs/src/speed.rsbindings/typescript-arkade/README.mdbindings/typescript-arkade/src/__tests__/arkade-boltz-node.test.tsbindings/typescript-arkade/src/__tests__/integration/arkade-boltz.real.test.tsbindings/typescript-arkade/src/__tests__/integration/helpers.tsbindings/typescript-arkade/src/nodes/arkade-boltz.tsbindings/typescript-arkade/src/types.tsbindings/typescript-arkade/tsconfig.build.jsonbindings/typescript-spark/README.mdbindings/typescript-spark/examples/spark-expo-go/App.tsxbindings/typescript-spark/scripts/build-spark-vendor.mjsbindings/typescript-spark/scripts/spark-shims/bare-crypto.jsbindings/typescript-spark/scripts/spark-shims/bare-fetch.jsbindings/typescript-spark/scripts/spark-shims/node-events.cjsbindings/typescript-spark/scripts/spark-shims/vitest.jsbindings/typescript-spark/src/__tests__/integration/helpers.tsbindings/typescript-spark/src/__tests__/integration/spark.real.test.tsbindings/typescript-spark/src/__tests__/spark-node.test.tsbindings/typescript-spark/src/__tests__/spark-runtime.test.tsbindings/typescript-spark/src/nodes/spark.tsbindings/typescript-spark/src/spark-runtime.tsbindings/typescript-spark/tsconfig.build.jsonbindings/typescript/README.mdbindings/typescript/src/__tests__/blink.test.tsbindings/typescript/src/__tests__/decode.test.tsbindings/typescript/src/__tests__/expo-polyfills.test.tsbindings/typescript/src/__tests__/integration/blink.real.test.tsbindings/typescript/src/__tests__/integration/cln.real.test.tsbindings/typescript/src/__tests__/integration/helpers.tsbindings/typescript/src/__tests__/integration/lnd.real.test.tsbindings/typescript/src/__tests__/integration/lnurl.real.test.tsbindings/typescript/src/__tests__/integration/nwc.real.test.tsbindings/typescript/src/__tests__/integration/phoenixd.real.test.tsbindings/typescript/src/__tests__/integration/speed.real.test.tsbindings/typescript/src/__tests__/integration/strike.real.test.tsbindings/typescript/src/__tests__/lnurl.test.tsbindings/typescript/src/__tests__/nwc.test.tsbindings/typescript/src/__tests__/permissions.test.tsbindings/typescript/src/__tests__/provider-error-normalization.test.tsbindings/typescript/src/__tests__/strike.test.tsbindings/typescript/src/decode.tsbindings/typescript/src/errors.tsbindings/typescript/src/factory.tsbindings/typescript/src/internal/encoding.tsbindings/typescript/src/internal/error-normalization.tsbindings/typescript/src/internal/http.tsbindings/typescript/src/internal/permissions.tsbindings/typescript/src/internal/sha256.tsbindings/typescript/src/lnurl.tsbindings/typescript/src/nodes/blink.tsbindings/typescript/src/nodes/cln.tsbindings/typescript/src/nodes/lnd.tsbindings/typescript/src/nodes/nwc.tsbindings/typescript/src/nodes/phoenixd.tsbindings/typescript/src/nodes/speed.tsbindings/typescript/src/nodes/strike.tsbindings/typescript/src/types.tscrates/lni/blink/lib.rscrates/lni/blink/types.rscrates/lni/cln/lib.rscrates/lni/cln/types.rscrates/lni/database.rscrates/lni/lib.rscrates/lni/lnd/lib.rscrates/lni/lnd/types.rscrates/lni/nwc/api.rscrates/lni/nwc/lib.rscrates/lni/nwc/types.rscrates/lni/permissions.rscrates/lni/phoenixd/lib.rscrates/lni/phoenixd/types.rscrates/lni/spark/api.rscrates/lni/spark/lib.rscrates/lni/speed/lib.rscrates/lni/speed/types.rscrates/lni/utils.rsscripts/README.mdscripts/format-check.shscripts/format-staged.shscripts/format-typescript.shscripts/format.shscripts/install-hooks.sh
💤 Files with no reviewable changes (1)
- crates/lni/lnd/types.rs
Comment on lines
+77
to
90
| // Create HTTP client with optional SOCKS5 proxy | ||
| let client = if let Some(proxy_url) = socks5_proxy { | ||
| // Ignore certificate errors when using SOCKS5 proxy | ||
| let client_builder = reqwest::Client::builder().danger_accept_invalid_certs(true); | ||
|
|
||
| match reqwest::Proxy::all(&proxy_url) { | ||
| Ok(proxy) => { | ||
| match client_builder.proxy(proxy).build() { | ||
| Ok(client) => client, | ||
| Err(_) => reqwest::Client::new(), // Fallback to default client on error | ||
| } | ||
| } else { | ||
| reqwest::Client::builder().build().unwrap_or_else(|_| reqwest::Client::new()) | ||
| }; | ||
|
|
||
| // Create request with optional header | ||
| let mut request = client.get(&url); | ||
|
|
||
| if let (Some(key), Some(value)) = (header_key, header_value) { | ||
| request = request.header(&key, &value); | ||
| } | ||
| Err(_) => reqwest::Client::new(), // Fallback to default client on error | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n "danger_accept_invalid_certs|say_after_with_tokio" bindings/lni_nodejs/src crates/lni -g '*.rs'Repository: lightning-node-interface/lni
Length of output: 1886
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- bindings/lni_nodejs/src/lib.rs ---'
sed -n '1,160p' bindings/lni_nodejs/src/lib.rs
echo
echo '--- crates/lni/lib.rs (say_after_with_tokio) ---'
sed -n '280,340p' crates/lni/lib.rs
echo
echo '--- crates/lni/lnd/api.rs ---'
sed -n '1,120p' crates/lni/lnd/api.rs
echo
echo '--- crates/lni/strike/api.rs ---'
sed -n '1,110p' crates/lni/strike/api.rsRepository: lightning-node-interface/lni
Length of output: 14923
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- occurrences of proxy/self-signed/local testing guidance ---'
rg -n "self-signed|certificate|invalid cert|proxy|local testing|danger_accept_invalid_certs|accept_invalid_certs" crates/lni bindings/lni_nodejs -g '*.rs' -g '*.md' -g '*.toml'
echo
echo '--- client construction around bindings/lni_nodejs/src/lib.rs ---'
sed -n '60,110p' bindings/lni_nodejs/src/lib.rsRepository: lightning-node-interface/lni
Length of output: 20391
Don't disable TLS validation just because a SOCKS5 proxy is set.
danger_accept_invalid_certs(true) makes every HTTPS request through this helper trust invalid certificates, so a proxy can MITM arbitrary URLs. Make this an explicit opt-in instead of enabling it automatically.
🧰 Tools
🪛 ast-grep (0.44.1)
[warning] 79-79: Dangerously accepting invalid TLS
Context: reqwest::Client::builder().danger_accept_invalid_certs(true)
Note: [CWE-295]: Improper Certificate
(reqwest-accept-invalid-rust)
🤖 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 `@bindings/lni_nodejs/src/lib.rs` around lines 77 - 90, The HTTP client setup
in the request-building logic should not automatically disable TLS verification
when socks5_proxy is present. Update the client construction in the helper
around reqwest::Client::builder and reqwest::Proxy::all so that
danger_accept_invalid_certs(true) is removed from the default proxy path, and
only allow invalid certs behind an explicit opt-in flag or configuration. Keep
the existing proxy fallback behavior intact while ensuring the default client
created by the function still validates certificates.
Source: Linters/SAST tools
Comment on lines
+287
to
+309
| while start_time.elapsed().as_secs() < params.max_polling_sec as u64 { | ||
| let lookup_params = lni::LookupInvoiceParams { | ||
| payment_hash: params.payment_hash.clone(), | ||
| search: params.search.clone(), | ||
| }; | ||
|
|
||
| match node.lookup_invoice(lookup_params).await { | ||
| Ok(transaction) if transaction.settled_at > 0 => { | ||
| let _ = callback("success".to_string(), Some(transaction)); | ||
| return; | ||
| } | ||
| Ok(transaction) => { | ||
| let _ = callback("pending".to_string(), Some(transaction)); | ||
| } | ||
| Err(_) => { | ||
| let _ = callback("pending".to_string(), None); | ||
| } | ||
| } | ||
|
|
||
| let _ = callback("failure".to_string(), None); | ||
| }); | ||
| tokio::time::sleep(tokio::time::Duration::from_secs( | ||
| params.polling_delay_sec as u64, | ||
| )) | ||
| .await; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n "OnInvoiceEventParams|max_polling_sec|polling_delay_sec" bindings/lni_nodejs/src crates/lni -g '*.rs' -g '*.ts'Repository: lightning-node-interface/lni
Length of output: 7367
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the Node.js binding and the shared params type.
sed -n '1,220p' bindings/lni_nodejs/src/spark.rs
printf '\n--- types ---\n'
sed -n '640,690p' crates/lni/types.rs
printf '\n--- node spark binding ---\n'
sed -n '240,340p' bindings/lni_nodejs/src/spark.rsRepository: lightning-node-interface/lni
Length of output: 9779
Clamp polling_delay_sec and max_polling_sec before casting to u64. OnInvoiceEventParams accepts signed values from the JS binding, so a negative input wraps to a huge duration and can block this sync polling loop for far longer than intended.
🤖 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 `@bindings/lni_nodejs/src/spark.rs` around lines 287 - 309, Clamp the polling
values in the invoice polling loop before converting them to durations: in
`OnInvoiceEventParams` handling inside `spark.rs`, ensure `polling_delay_sec`
and `max_polling_sec` are validated to non-negative bounds before the `as u64`
casts used by the `while start_time.elapsed()` condition and
`tokio::time::sleep`. Update the `lookup_invoice` polling logic so negative JS
inputs cannot wrap into huge `Duration` values, keeping the behavior bounded and
predictable.
Comment on lines
+24
to
+137
| enabled && | ||
| process.env.BLINK_RUN_ONCHAIN_SEND === 'true' && | ||
| process.env.BLINK_ONCHAIN_SEND_CONFIRM === ONCHAIN_SEND_CONFIRMATION && | ||
| hasEnv('BLINK_ONCHAIN_TEST_ADDRESS', 'BLINK_ONCHAIN_AMOUNT_SATS') && | ||
| Number.isSafeInteger(onchainSendAmountSats) && | ||
| onchainSendAmountSats > 0; | ||
|
|
||
| const makeNode = () => | ||
| new BlinkNode({ | ||
| apiKey: process.env.BLINK_API_KEY!, | ||
| baseUrl: nonEmpty(process.env.BLINK_BASE_URL), | ||
| }); | ||
|
|
||
| itIf(enabled)('getInfo + createInvoice + listTransactions', async () => { | ||
| const node = makeNode(); | ||
| const info = await node.getInfo(); | ||
| expect(typeof info.alias).toBe('string'); | ||
| itIf(enabled)( | ||
| 'getInfo + createInvoice + listTransactions', | ||
| async () => { | ||
| const node = makeNode(); | ||
| const info = await node.getInfo(); | ||
| expect(typeof info.alias).toBe('string'); | ||
|
|
||
| const invoice = await node.createInvoice({ | ||
| amountMsats: 5_000, | ||
| description: testInvoiceLabel('blink'), | ||
| }); | ||
| console.log('Blink Invoice:', invoice); | ||
| expect(invoice.invoice.length).toBeGreaterThan(0); | ||
| const invoice = await node.createInvoice({ | ||
| amountMsats: 5_000, | ||
| description: testInvoiceLabel('blink'), | ||
| }); | ||
| console.log('Blink Invoice:', invoice); | ||
| expect(invoice.invoice.length).toBeGreaterThan(0); | ||
|
|
||
| const txs = await node.listTransactions({ from: 0, limit: 25 }); | ||
| expect(Array.isArray(txs)).toBe(true); | ||
| }, | ||
| timeout | ||
| ); | ||
|
|
||
| itIf(enabled)( | ||
| 'lookupInvoice (best effort from env or recent tx)', | ||
| async () => { | ||
| await runOrSkipKnownError(async () => { | ||
| const node = makeNode(); | ||
| const txs = await node.listTransactions({ from: 0, limit: 50 }); | ||
| const candidateHash = txs.find((tx) => tx.paymentHash.length > 0)?.paymentHash; | ||
| const hashes = uniqueValues([process.env.BLINK_TEST_PAYMENT_HASH, candidateHash]); | ||
|
|
||
| if (!hashes.length) { | ||
| return; | ||
| } | ||
|
|
||
| const txs = await node.listTransactions({ from: 0, limit: 25 }); | ||
| expect(Array.isArray(txs)).toBe(true); | ||
| }, timeout); | ||
| let lastError: unknown; | ||
| for (const paymentHash of hashes) { | ||
| try { | ||
| const tx = await node.lookupInvoice({ paymentHash }); | ||
| expect(tx.paymentHash.length).toBeGreaterThan(0); | ||
| return; | ||
| } catch (error) { | ||
| lastError = error; | ||
| } | ||
| } | ||
|
|
||
| itIf(enabled)('lookupInvoice (best effort from env or recent tx)', async () => { | ||
| await runOrSkipKnownError(async () => { | ||
| if (lastError) { | ||
| throw lastError; | ||
| } | ||
| }, ['transaction not found', 'http 404']); | ||
| }, | ||
| timeout | ||
| ); | ||
|
|
||
| itIf(enabled && hasEnv('BLINK_ONCHAIN_TEST_ADDRESS'))( | ||
| 'prepareOnchainTransaction + optionally payOnchain', | ||
| async () => { | ||
| const node = makeNode(); | ||
| const txs = await node.listTransactions({ from: 0, limit: 50 }); | ||
| const candidateHash = txs.find((tx) => tx.paymentHash.length > 0)?.paymentHash; | ||
| const hashes = uniqueValues([process.env.BLINK_TEST_PAYMENT_HASH, candidateHash]); | ||
|
|
||
| if (!hashes.length) { | ||
| let transaction: Awaited<ReturnType<BlinkNode['prepareOnchainTransaction']>> | undefined; | ||
|
|
||
| await runOrSkipKnownError(async () => { | ||
| transaction = await node.prepareOnchainTransaction({ | ||
| address: process.env.BLINK_ONCHAIN_TEST_ADDRESS!, | ||
| amountSats: quoteOnlyAmountSats, | ||
| fee: { type: 'speed', speed: runOnchainSend ? 'fast' : 'normal' }, | ||
| feePayer: 'sender', | ||
| description: testInvoiceLabel( | ||
| runOnchainSend ? 'blink onchain e2e' : 'blink onchain quote' | ||
| ), | ||
| }); | ||
|
|
||
| expect(transaction.address).toBe(process.env.BLINK_ONCHAIN_TEST_ADDRESS); | ||
| expect(transaction.amountSats).toBe(quoteOnlyAmountSats); | ||
| expect(transaction.feePayer).toBe('sender'); | ||
| expect(transaction.feeSats).toBeGreaterThanOrEqual(0); | ||
| }, ['insufficient balance', 'invalid address', 'amount']); | ||
|
|
||
| console.log('Prepared Blink on-chain transaction:', transaction); | ||
| if (!transaction) { | ||
| return; | ||
| } | ||
|
|
||
| let lastError: unknown; | ||
| for (const paymentHash of hashes) { | ||
| try { | ||
| const tx = await node.lookupInvoice({ paymentHash }); | ||
| expect(tx.paymentHash.length).toBeGreaterThan(0); | ||
| return; | ||
| } catch (error) { | ||
| lastError = error; | ||
| } | ||
| if (!runOnchainSend) { | ||
| console.log( | ||
| 'Prepared Blink on-chain quote; skipping broadcast without explicit confirmation' | ||
| ); | ||
| return; | ||
| } | ||
|
|
||
| if (lastError) { | ||
| throw lastError; | ||
| } | ||
| }, ['transaction not found', 'http 404']); | ||
| }, timeout); | ||
|
|
||
| itIf(enabled && hasEnv('BLINK_ONCHAIN_TEST_ADDRESS'))('prepareOnchainTransaction + optionally payOnchain', async () => { | ||
| const node = makeNode(); | ||
| let transaction: Awaited<ReturnType<BlinkNode['prepareOnchainTransaction']>> | undefined; | ||
|
|
||
| await runOrSkipKnownError(async () => { | ||
| transaction = await node.prepareOnchainTransaction({ | ||
| address: process.env.BLINK_ONCHAIN_TEST_ADDRESS!, | ||
| amountSats: quoteOnlyAmountSats, | ||
| fee: { type: 'speed', speed: runOnchainSend ? 'fast' : 'normal' }, | ||
| feePayer: 'sender', | ||
| description: testInvoiceLabel(runOnchainSend ? 'blink onchain e2e' : 'blink onchain quote'), | ||
| // Blink can quote high miner/provider fees for small test sends; keep the absolute guardrail | ||
| // default, but allow this real test up to 52% before broadcasting. | ||
| const payment = await node.payOnchain(transaction, { | ||
| feeGuardrail: { | ||
| maxFeePercent: 52, | ||
| }, | ||
| }); | ||
| console.log('Blink on-chain payment result:', payment); | ||
|
|
||
| expect(transaction.address).toBe(process.env.BLINK_ONCHAIN_TEST_ADDRESS); | ||
| expect(transaction.amountSats).toBe(quoteOnlyAmountSats); | ||
| expect(transaction.feePayer).toBe('sender'); | ||
| expect(transaction.feeSats).toBeGreaterThanOrEqual(0); | ||
| }, ['insufficient balance', 'invalid address', 'amount']); | ||
|
|
||
| console.log('Prepared Blink on-chain transaction:', transaction); | ||
| if (!transaction) { | ||
| return; | ||
| } | ||
|
|
||
| if (!runOnchainSend) { | ||
| console.log('Prepared Blink on-chain quote; skipping broadcast without explicit confirmation'); | ||
| return; | ||
| } | ||
|
|
||
| // Blink can quote high miner/provider fees for small test sends; keep the absolute guardrail | ||
| // default, but allow this real test up to 52% before broadcasting. | ||
| const payment = await node.payOnchain(transaction, { | ||
| feeGuardrail: { | ||
| maxFeePercent: 52, | ||
| }, | ||
| }); | ||
| console.log('Blink on-chain payment result:', payment); | ||
|
|
||
| expect(['pending', 'completed']).toContain(payment.state); | ||
| expect(payment.address).toBe(process.env.BLINK_ONCHAIN_TEST_ADDRESS); | ||
| }, timeout); | ||
| expect(['pending', 'completed']).toContain(payment.state); | ||
| expect(payment.address).toBe(process.env.BLINK_ONCHAIN_TEST_ADDRESS); | ||
| }, | ||
| timeout | ||
| ); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Remove sensitive logging from this test.
These console.log(...) calls print full invoice/payment payloads, and the on-chain path also emits transaction details into CI logs. As per coding guidelines, tests/logging must not expose full invoices or preimages.
🤖 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 `@bindings/typescript/src/__tests__/integration/blink.real.test.ts` around
lines 24 - 137, Remove the sensitive console logging from the Blink real
integration tests so CI never prints full invoice, payment, or transaction
payloads. In the test cases that use makeNode, createInvoice,
prepareOnchainTransaction, and payOnchain, delete the console.log calls that
dump the returned objects or replace them with non-sensitive status messages
only. Keep the assertions and flow intact, but ensure no full
invoice/payment/preimage or on-chain transaction details are emitted.
Source: Coding guidelines
Comment on lines
+14
to
+59
| itIf(enabled)( | ||
| 'getInfo', | ||
| async () => { | ||
| const node = makeNode(); | ||
| const info = await node.getInfo(); | ||
| expect(typeof info.pubkey).toBe('string'); | ||
| expect(info.pubkey.length).toBeGreaterThan(0); | ||
| }, | ||
| timeout | ||
| ); | ||
|
|
||
| itIf(enabled)('createInvoice + lookupInvoice + listTransactions', async () => { | ||
| const node = makeNode(); | ||
| const invoice = await node.createInvoice({ | ||
| amountMsats: 2_000, | ||
| description: testInvoiceLabel('cln'), | ||
| }); | ||
| console.log('CLN Invoice:', invoice); | ||
| expect(invoice.invoice.length).toBeGreaterThan(0); | ||
| expect(invoice.paymentHash.length).toBeGreaterThan(0); | ||
| itIf(enabled)( | ||
| 'createInvoice + lookupInvoice + listTransactions', | ||
| async () => { | ||
| const node = makeNode(); | ||
| const invoice = await node.createInvoice({ | ||
| amountMsats: 2_000, | ||
| description: testInvoiceLabel('cln'), | ||
| }); | ||
| console.log('CLN Invoice:', invoice); | ||
| expect(invoice.invoice.length).toBeGreaterThan(0); | ||
| expect(invoice.paymentHash.length).toBeGreaterThan(0); | ||
|
|
||
| const lookedUp = await node.lookupInvoice({ paymentHash: invoice.paymentHash }); | ||
| expect(lookedUp.paymentHash).toBe(invoice.paymentHash); | ||
| const lookedUp = await node.lookupInvoice({ paymentHash: invoice.paymentHash }); | ||
| expect(lookedUp.paymentHash).toBe(invoice.paymentHash); | ||
|
|
||
| const txs = await node.listTransactions({ from: 0, limit: 25, paymentHash: invoice.paymentHash }); | ||
| expect(Array.isArray(txs)).toBe(true); | ||
| expect(txs.some((tx) => tx.paymentHash === invoice.paymentHash)).toBe(true); | ||
| }, timeout); | ||
| const txs = await node.listTransactions({ | ||
| from: 0, | ||
| limit: 25, | ||
| paymentHash: invoice.paymentHash, | ||
| }); | ||
| expect(Array.isArray(txs)).toBe(true); | ||
| expect(txs.some((tx) => tx.paymentHash === invoice.paymentHash)).toBe(true); | ||
| }, | ||
| timeout | ||
| ); | ||
|
|
||
| itIf(enabled && hasEnv('CLN_TEST_PAYMENT_REQUEST'))('decode', async () => { | ||
| const node = makeNode(); | ||
| const decoded = await node.decode(process.env.CLN_TEST_PAYMENT_REQUEST!); | ||
| expect(decoded.length).toBeGreaterThan(0); | ||
| }, timeout); | ||
| itIf(enabled && hasEnv('CLN_TEST_PAYMENT_REQUEST'))( | ||
| 'decode', | ||
| async () => { | ||
| const node = makeNode(); | ||
| const decoded = await node.decode(process.env.CLN_TEST_PAYMENT_REQUEST!); | ||
| expect(decoded.length).toBeGreaterThan(0); | ||
| }, | ||
| timeout | ||
| ); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Remove the invoice log from this test.
console.log('CLN Invoice:', invoice) can leak the full invoice payload into CI logs. As per coding guidelines, tests/logging must not expose full invoices or preimages.
🤖 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 `@bindings/typescript/src/__tests__/integration/cln.real.test.ts` around lines
14 - 59, The test in the createInvoice + lookupInvoice + listTransactions case
is logging the full invoice payload, which can leak sensitive data into CI
output. Remove the console.log from the CLN integration test and keep the
existing assertions on invoice.invoice, invoice.paymentHash, lookupInvoice, and
listTransactions in place without exposing the invoice object.
Source: Coding guidelines
Comment on lines
+14
to
+60
| itIf(enabled)( | ||
| 'getInfo', | ||
| async () => { | ||
| await runOrSkipKnownError(async () => { | ||
| const node = makeNode(); | ||
| const info = await node.getInfo(); | ||
| expect(typeof info.pubkey).toBe('string'); | ||
| expect(info.pubkey.length).toBeGreaterThan(0); | ||
| }, ['permission denied']); | ||
| }, | ||
| timeout | ||
| ); | ||
|
|
||
| itIf(enabled)('createInvoice + lookupInvoice + listTransactions', async () => { | ||
| const node = makeNode(); | ||
| const invoice = await node.createInvoice({ | ||
| amountMsats: 3_000, | ||
| description: testInvoiceLabel('lnd'), | ||
| }); | ||
| console.log('LND Invoice:', invoice); | ||
| expect(invoice.invoice.length).toBeGreaterThan(0); | ||
| expect(invoice.paymentHash.length).toBeGreaterThan(0); | ||
| itIf(enabled)( | ||
| 'createInvoice + lookupInvoice + listTransactions', | ||
| async () => { | ||
| const node = makeNode(); | ||
| const invoice = await node.createInvoice({ | ||
| amountMsats: 3_000, | ||
| description: testInvoiceLabel('lnd'), | ||
| }); | ||
| console.log('LND Invoice:', invoice); | ||
| expect(invoice.invoice.length).toBeGreaterThan(0); | ||
| expect(invoice.paymentHash.length).toBeGreaterThan(0); | ||
|
|
||
| const lookedUp = await node.lookupInvoice({ paymentHash: invoice.paymentHash }); | ||
| expect(lookedUp.paymentHash.length).toBeGreaterThan(0); | ||
| const lookedUp = await node.lookupInvoice({ paymentHash: invoice.paymentHash }); | ||
| expect(lookedUp.paymentHash.length).toBeGreaterThan(0); | ||
|
|
||
| const txs = await node.listTransactions({ from: 0, limit: 25, paymentHash: invoice.paymentHash }); | ||
| expect(Array.isArray(txs)).toBe(true); | ||
| }, timeout); | ||
| const txs = await node.listTransactions({ | ||
| from: 0, | ||
| limit: 25, | ||
| paymentHash: invoice.paymentHash, | ||
| }); | ||
| expect(Array.isArray(txs)).toBe(true); | ||
| }, | ||
| timeout | ||
| ); | ||
|
|
||
| itIf(enabled && hasEnv('LND_TEST_PAYMENT_REQUEST'))('decode', async () => { | ||
| const node = makeNode(); | ||
| const decoded = await node.decode(process.env.LND_TEST_PAYMENT_REQUEST!); | ||
| expect(decoded.length).toBeGreaterThan(0); | ||
| }, timeout); | ||
| itIf(enabled && hasEnv('LND_TEST_PAYMENT_REQUEST'))( | ||
| 'decode', | ||
| async () => { | ||
| const node = makeNode(); | ||
| const decoded = await node.decode(process.env.LND_TEST_PAYMENT_REQUEST!); | ||
| expect(decoded.length).toBeGreaterThan(0); | ||
| }, | ||
| timeout | ||
| ); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Remove the invoice log from this test.
console.log('LND Invoice:', invoice) can leak the full invoice payload into CI logs. As per coding guidelines, tests/logging must not expose full invoices or preimages.
🤖 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 `@bindings/typescript/src/__tests__/integration/lnd.real.test.ts` around lines
14 - 60, Remove the invoice logging from the LND integration test by deleting
the console output in the createInvoice + lookupInvoice + listTransactions case;
update the test around node.createInvoice so it no longer prints the full
invoice payload to CI logs, while keeping the existing assertions on
invoice.invoice and invoice.paymentHash intact.
Source: Coding guidelines
Comment on lines
+27
to
+52
| itIf(enabled)( | ||
| 'createInvoice + lookupInvoice + listTransactions', | ||
| async () => { | ||
| await runOrSkipKnownError(async () => { | ||
| const node = makeNode(); | ||
| const invoice = await node.createInvoice({ | ||
| amountMsats: 2_000, | ||
| description: testInvoiceLabel('phoenixd'), | ||
| }); | ||
| console.log('Phoenixd Invoice:', invoice); | ||
| expect(invoice.invoice.length).toBeGreaterThan(0); | ||
| expect(invoice.paymentHash.length).toBeGreaterThan(0); | ||
|
|
||
| const lookedUp = await node.lookupInvoice({ paymentHash: invoice.paymentHash }); | ||
| expect(lookedUp.paymentHash).toBe(invoice.paymentHash); | ||
| const lookedUp = await node.lookupInvoice({ paymentHash: invoice.paymentHash }); | ||
| expect(lookedUp.paymentHash).toBe(invoice.paymentHash); | ||
|
|
||
| const txs = await node.listTransactions({ from: 0, limit: 25, paymentHash: invoice.paymentHash }); | ||
| expect(Array.isArray(txs)).toBe(true); | ||
| }, ['fetch failed', 'econnrefused', 'enotfound', 'timed out']); | ||
| }, timeout); | ||
| const txs = await node.listTransactions({ | ||
| from: 0, | ||
| limit: 25, | ||
| paymentHash: invoice.paymentHash, | ||
| }); | ||
| expect(Array.isArray(txs)).toBe(true); | ||
| }, ['fetch failed', 'econnrefused', 'enotfound', 'timed out']); | ||
| }, | ||
| timeout | ||
| ); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Remove the invoice log from this test.
console.log('Phoenixd Invoice:', invoice) can leak the full invoice payload into CI logs. As per coding guidelines, tests/logging must not expose full invoices or preimages.
🤖 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 `@bindings/typescript/src/__tests__/integration/phoenixd.real.test.ts` around
lines 27 - 52, Remove the console logging of the full invoice object in the
phoenixd integration test. In the createInvoice + lookupInvoice +
listTransactions case, delete the console.log call that prints the invoice
payload and keep the assertions/use of invoice.invoice and invoice.paymentHash
only. This change should be made in the test body around makeNode() and
createInvoice() so CI logs do not expose sensitive invoice data.
Source: Coding guidelines
Comment on lines
+22
to
+40
| itIf(enabled)( | ||
| 'getInfo + createInvoice + listTransactions', | ||
| async () => { | ||
| const node = makeNode(); | ||
| const info = await node.getInfo(); | ||
| expect(typeof info.alias).toBe('string'); | ||
|
|
||
| const invoice = await node.createInvoice({ | ||
| amountMsats: 5_000, | ||
| description: testInvoiceLabel('speed'), | ||
| }); | ||
| console.log('Speed Invoice:', invoice); | ||
| expect(invoice.invoice.length).toBeGreaterThan(0); | ||
| const invoice = await node.createInvoice({ | ||
| amountMsats: 5_000, | ||
| description: testInvoiceLabel('speed'), | ||
| }); | ||
| console.log('Speed Invoice:', invoice); | ||
| expect(invoice.invoice.length).toBeGreaterThan(0); | ||
|
|
||
| const txs = await node.listTransactions({ from: 0, limit: 25 }); | ||
| expect(Array.isArray(txs)).toBe(true); | ||
| }, timeout); | ||
| const txs = await node.listTransactions({ from: 0, limit: 25 }); | ||
| expect(Array.isArray(txs)).toBe(true); | ||
| }, | ||
| timeout | ||
| ); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Remove the invoice log from this test.
console.log('Speed Invoice:', invoice) can leak the full invoice payload into CI logs. As per coding guidelines, tests/logging must not expose full invoices or preimages.
🤖 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 `@bindings/typescript/src/__tests__/integration/speed.real.test.ts` around
lines 22 - 40, Remove the invoice logging from the speed test to avoid exposing
sensitive payment data in CI logs. Update the test in the getInfo +
createInvoice + listTransactions block by deleting the console.log tied to the
invoice variable, while keeping the existing assertions and flow unchanged.
Source: Coding guidelines
Comment on lines
+215
to
+225
| await expect(makeNode().payInvoice({ invoice: BOLT11_INVOICE })).rejects.toBeInstanceOf( | ||
| NwcError | ||
| ); | ||
| }); | ||
|
|
||
| it('preserves LniError status and body when rewrapping request failures', async () => { | ||
| nwcMocks.payInvoice.mockRejectedValue( | ||
| new LniError('Http', 'gateway timeout', { | ||
| status: 504, | ||
| body: '{"error":"timeout"}', | ||
| }), | ||
| }) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Keep the stronger error assertion.
This hunk now only checks the thrown type, so it no longer verifies the preserved NwcError payload. That weakens coverage for NIP-47 error wrapping.
🤖 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 `@bindings/typescript/src/__tests__/nwc.test.ts` around lines 215 - 225, The
payInvoice test no longer asserts the wrapped NwcError payload, so it only
checks the thrown type and weakens NIP-47 error coverage. Update the test around
makeNode().payInvoice and NwcError to keep the stronger assertion by verifying
the preserved error details from the rejected promise, not just the instance
type, alongside the existing LniError rewrap case.
Comment on lines
305
to
316
| // Ignore certificate errors when using SOCKS5 proxy | ||
| let client_builder = reqwest::Client::builder().danger_accept_invalid_certs(true); | ||
|
|
||
| match reqwest::Proxy::all(&proxy_url) { | ||
| Ok(proxy) => { | ||
| match client_builder.proxy(proxy).build() { | ||
| Ok(client) => client, | ||
| Err(_) => reqwest::Client::new() // Fallback to default client on error | ||
| Err(_) => reqwest::Client::new(), // Fallback to default client on error | ||
| } | ||
| } | ||
| Err(_) => reqwest::Client::new() // Fallback to default client on error | ||
| Err(_) => reqwest::Client::new(), // Fallback to default client on error | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the file and the relevant lines
git ls-files crates/lni/lib.rs
wc -l crates/lni/lib.rs
sed -n '260,340p' crates/lni/lib.rs
# Search for related reqwest client configuration in the repo
rg -n "danger_accept_invalid_certs|Proxy::all|Client::builder\(" crates -SRepository: lightning-node-interface/lni
Length of output: 5475
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show surrounding function context and call sites
ast-grep outline crates/lni/lib.rs --view expanded
rg -n "say_after_with_tokio|proxy_url|socks5" crates/lni/lib.rs -n -SRepository: lightning-node-interface/lni
Length of output: 2513
Don't disable certificate validation on the SOCKS5 path.
danger_accept_invalid_certs(true) disables TLS verification for every HTTPS request made with this client, which exposes requests to proxy/MITM interception. Keep verification on by default and make any relaxation an explicit opt-in for self-signed endpoints.
🧰 Tools
🪛 ast-grep (0.44.1)
[warning] 305-305: Dangerously accepting invalid TLS
Context: reqwest::Client::builder().danger_accept_invalid_certs(true)
Note: [CWE-295]: Improper Certificate
(reqwest-accept-invalid-rust)
🤖 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 `@crates/lni/lib.rs` around lines 305 - 316, The SOCKS5 client setup in the
reqwest builder is disabling TLS verification globally via the client_builder
path, which should not be the default. Remove the
danger_accept_invalid_certs(true) usage from the proxy/client construction and
keep certificate validation enabled in the normal flow. If special handling for
self-signed endpoints is needed, gate it behind an explicit opt-in in the same
client-building logic around reqwest::Client::builder(), reqwest::Proxy::all,
and the build fallback path.
Source: Linters/SAST tools
Comment on lines
+97
to
+131
| Ok(nwc_info) => { | ||
| Ok(NodeInfo { | ||
| alias: nwc_info.alias.unwrap_or_else(|| "NWC Node".to_string()), | ||
| color: nwc_info.color.unwrap_or_default(), | ||
| pubkey: nwc_info.pubkey.map(|pk| pk.to_string()).unwrap_or_else(|| { | ||
| // If no pubkey in get_info, try to extract from URI | ||
| config | ||
| .nwc_uri | ||
| .split("?") | ||
| .next() | ||
| .and_then(|part| part.strip_prefix("nostr+walletconnect://")) | ||
| .unwrap_or_default() | ||
| .to_string() | ||
| }), | ||
| network: nwc_info.network.unwrap_or_else(|| "mainnet".to_string()), | ||
| block_height: nwc_info.block_height.unwrap_or(0) as i64, | ||
| block_hash: nwc_info.block_hash.unwrap_or_default(), | ||
| send_balance_msat: balance as i64, | ||
| receive_balance_msat: 0, // NWC doesn't provide separate receive balance | ||
| fee_credit_balance_msat: 0, | ||
| unsettled_send_balance_msat: 0, | ||
| unsettled_receive_balance_msat: 0, | ||
| pending_open_send_balance: 0, | ||
| pending_open_receive_balance: 0, | ||
| }) | ||
| } | ||
| Err(_) => { | ||
| // Fallback: extract pubkey from NWC URI if get_info is not available | ||
| let pubkey = config | ||
| .nwc_uri | ||
| .split("?") | ||
| .next() | ||
| .and_then(|part| part.strip_prefix("nostr+walletconnect://")) | ||
| .unwrap_or_default() | ||
| .to_string(); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Extract pubkey-from-URI logic into a helper and handle all NWC scheme variants.
The pubkey extraction logic is duplicated between the Ok branch (lines 101–110) and the Err fallback (lines 125–131). Both inline the same split("?") → strip_prefix("nostr+walletconnect://") chain, but this only handles one of the four NWC URI scheme variants. If the URI uses nostrwalletconnect://, nostrwalletconnect:, or nostr+walletconnect:, the pubkey will silently default to an empty string, producing an incorrect NodeInfo.
The new lightning_address_from_nwc_uri helper (lines 166–171) already normalizes all four variants — the same approach should be applied here.
♻️ Proposed refactor: extract a `pubkey_from_nwc_uri` helper
+/// Extracts the pubkey from an NWC URI string, handling all known scheme variants.
+fn pubkey_from_nwc_uri(nwc_uri: &str) -> String {
+ nwc_uri
+ .split("?")
+ .next()
+ .and_then(|part| {
+ part.strip_prefix("nostr+walletconnect://")
+ .or_else(|| part.strip_prefix("nostrwalletconnect://"))
+ .or_else(|| part.strip_prefix("nostr+walletconnect:"))
+ .or_else(|| part.strip_prefix("nostrwalletconnect:"))
+ })
+ .unwrap_or_default()
+ .to_string()
+}
+
pub async fn get_info(config: NwcConfig) -> Result<NodeInfo, ApiError> {
// ...
match info_result {
Ok(nwc_info) => {
Ok(NodeInfo {
// ...
- pubkey: nwc_info.pubkey.map(|pk| pk.to_string()).unwrap_or_else(|| {
- // If no pubkey in get_info, try to extract from URI
- config
- .nwc_uri
- .split("?")
- .next()
- .and_then(|part| part.strip_prefix("nostr+walletconnect://"))
- .unwrap_or_default()
- .to_string()
- }),
+ pubkey: nwc_info
+ .pubkey
+ .map(|pk| pk.to_string())
+ .unwrap_or_else(|| pubkey_from_nwc_uri(&config.nwc_uri)),
// ...
})
}
Err(_) => {
- let pubkey = config
- .nwc_uri
- .split("?")
- .next()
- .and_then(|part| part.strip_prefix("nostr+walletconnect://"))
- .unwrap_or_default()
- .to_string();
+ let pubkey = pubkey_from_nwc_uri(&config.nwc_uri);
// ...
}
}
}📝 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.
Suggested change
| Ok(nwc_info) => { | |
| Ok(NodeInfo { | |
| alias: nwc_info.alias.unwrap_or_else(|| "NWC Node".to_string()), | |
| color: nwc_info.color.unwrap_or_default(), | |
| pubkey: nwc_info.pubkey.map(|pk| pk.to_string()).unwrap_or_else(|| { | |
| // If no pubkey in get_info, try to extract from URI | |
| config | |
| .nwc_uri | |
| .split("?") | |
| .next() | |
| .and_then(|part| part.strip_prefix("nostr+walletconnect://")) | |
| .unwrap_or_default() | |
| .to_string() | |
| }), | |
| network: nwc_info.network.unwrap_or_else(|| "mainnet".to_string()), | |
| block_height: nwc_info.block_height.unwrap_or(0) as i64, | |
| block_hash: nwc_info.block_hash.unwrap_or_default(), | |
| send_balance_msat: balance as i64, | |
| receive_balance_msat: 0, // NWC doesn't provide separate receive balance | |
| fee_credit_balance_msat: 0, | |
| unsettled_send_balance_msat: 0, | |
| unsettled_receive_balance_msat: 0, | |
| pending_open_send_balance: 0, | |
| pending_open_receive_balance: 0, | |
| }) | |
| } | |
| Err(_) => { | |
| // Fallback: extract pubkey from NWC URI if get_info is not available | |
| let pubkey = config | |
| .nwc_uri | |
| .split("?") | |
| .next() | |
| .and_then(|part| part.strip_prefix("nostr+walletconnect://")) | |
| .unwrap_or_default() | |
| .to_string(); | |
| /// Extracts the pubkey from an NWC URI string, handling all known scheme variants. | |
| fn pubkey_from_nwc_uri(nwc_uri: &str) -> String { | |
| nwc_uri | |
| .split("?") | |
| .next() | |
| .and_then(|part| { | |
| part.strip_prefix("nostr+walletconnect://") | |
| .or_else(|| part.strip_prefix("nostrwalletconnect://")) | |
| .or_else(|| part.strip_prefix("nostr+walletconnect:")) | |
| .or_else(|| part.strip_prefix("nostrwalletconnect:")) | |
| }) | |
| .unwrap_or_default() | |
| .to_string() | |
| } | |
| Ok(nwc_info) => { | |
| Ok(NodeInfo { | |
| alias: nwc_info.alias.unwrap_or_else(|| "NWC Node".to_string()), | |
| color: nwc_info.color.unwrap_or_default(), | |
| pubkey: nwc_info | |
| .pubkey | |
| .map(|pk| pk.to_string()) | |
| .unwrap_or_else(|| pubkey_from_nwc_uri(&config.nwc_uri)), | |
| network: nwc_info.network.unwrap_or_else(|| "mainnet".to_string()), | |
| block_height: nwc_info.block_height.unwrap_or(0) as i64, | |
| block_hash: nwc_info.block_hash.unwrap_or_default(), | |
| send_balance_msat: balance as i64, | |
| receive_balance_msat: 0, // NWC doesn't provide separate receive balance | |
| fee_credit_balance_msat: 0, | |
| unsettled_send_balance_msat: 0, | |
| unsettled_receive_balance_msat: 0, | |
| pending_open_send_balance: 0, | |
| pending_open_receive_balance: 0, | |
| }) | |
| } | |
| Err(_) => { | |
| // Fallback: extract pubkey from NWC URI if get_info is not available | |
| let pubkey = pubkey_from_nwc_uri(&config.nwc_uri); |
🤖 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 `@crates/lni/nwc/api.rs` around lines 97 - 131, Extract the duplicated pubkey
parsing in NodeInfo construction into a dedicated helper, and use it in both the
get_info success path and the Err fallback. Update the logic in the api.rs NWC
handling code to normalize all NWC URI scheme variants the same way as
lightning_address_from_nwc_uri does, so pubkey extraction works for
nostr+walletconnect://, nostrwalletconnect://, nostrwalletconnect:, and
nostr+walletconnect:. Keep the existing NodeInfo and config.nwc_uri call sites,
but route them through the new helper so the fallback no longer silently returns
an empty pubkey.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Documentation
Chores