Onchain LND, CLN, Phoenixd#40
Open
ntheile wants to merge 5 commits into
Open
Conversation
📝 WalkthroughWalkthroughThis PR adds complete on-chain Bitcoin payment support to LND, CLN, and Phoenixd node implementations via a prepare-then-pay flow. Changes span TypeScript client logic, Rust backend APIs, N-API bindings, comprehensive test coverage, and documentation. Each provider's implementation includes fee preference resolution, guardrail enforcement, REST API integration, and PSBT/response parsing. The feature is accessible from both JavaScript and Rust with consistent error handling and validation. ChangesOn-chain payment implementations for LND, CLN, and Phoenixd
Sequence Diagram(s)sequenceDiagram
participant Client as JavaScript Client
participant TSNode as TypeScript Node (CLN/LND/Phoenixd)
participant NAPI as N-API Binding
participant RustLib as Rust Node Lib
participant RustAPI as Rust Backend API
participant Provider as Lightning Provider REST
Client->>TSNode: prepareOnchainTransaction(params)
TSNode->>TSNode: validate amount & fee preference
TSNode->>NAPI: call napi.prepare_onchain_transaction
NAPI->>RustLib: forward call
RustLib->>RustAPI: prepare_onchain_transaction(config, params)
RustAPI->>Provider: REST /txprepare or /fee or build send request
Provider-->>RustAPI: quote / PSBT / response
RustAPI->>RustAPI: parse fee, apply guardrails
RustAPI-->>RustLib: OnchainTransaction
RustLib-->>NAPI: return
NAPI-->>TSNode: OnchainTransaction
TSNode-->>Client: OnchainTransaction
Client->>TSNode: payOnchain(transaction, options)
TSNode->>TSNode: re-validate, enforce guardrails
TSNode->>NAPI: call napi.pay_onchain
NAPI->>RustLib: forward call
RustLib->>RustAPI: pay_onchain_with_options(config, transaction, options)
RustAPI->>Provider: REST /txsend or /transactions or /sendtoaddress
Provider-->>RustAPI: txid / confirmation
RustAPI->>RustAPI: normalize PayOnchainResponse (pending/failed)
RustAPI-->>RustLib: PayOnchainResponse
RustLib-->>NAPI: return
NAPI-->>TSNode: PayOnchainResponse
TSNode-->>Client: PayOnchainResponse
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c0193ebd01
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const feeRequest = resolveLndFeeRequest(fee); | ||
| const estimate = await this.getJsonWithQuery<LndEstimateFeeResponse>('/v1/transactions/fee', { | ||
| [`AddrToAmount[${params.address}]`]: amountSats, | ||
| ...feeRequest, |
There was a problem hiding this comment.
When callers prepare an LND on-chain send with fee: { type: 'satsPerVbyte', ... }, this spread adds sat_per_vbyte to /v1/transactions/fee. I checked the LND EstimateFee REST docs: the request only supports target_conf, min_confs, and spend_unconfirmed query fields; sat_per_vbyte is only a response field and is supported later by SendCoins. As a result this mode either gets rejected by LND or returns a fee quote that is not based on the manual feerate, while payOnchain later broadcasts with that manual feerate, so the guardrail can approve a transaction using the wrong fee estimate.
Useful? React with 👍 / 👎.
Comment on lines
+805
to
+806
| if let Some(sat_per_vbyte) = fee_request.sat_per_vbyte { | ||
| query.push(("sat_per_vbyte".to_string(), sat_per_vbyte.to_string())); |
There was a problem hiding this comment.
When Rust callers prepare an LND on-chain send with OnchainFeePreferenceType::SatsPerVbyte, this adds sat_per_vbyte to /v1/transactions/fee. I checked the LND EstimateFee REST docs: the request query fields are target_conf, min_confs, and spend_unconfirmed; sat_per_vbyte is only returned by EstimateFee and accepted by SendCoins. This means the manual-fee prepare path either fails or records a fee quote unrelated to the fee rate that pay_onchain later broadcasts with, so the fee guardrail can be evaluated against the wrong fee.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (2)
crates/lni/cln/api.rs (1)
162-204: 💤 Low valueInconsistent guardrail logic between CLN and LND implementations.
CLN uses
min(max_fee_sats, max_fee_by_percent)as a single threshold (Line 195), meaning the fee must satisfy both constraints simultaneously. LND (lines 223-236 in lnd/api.rs) checks each constraint independently with separate error messages.This leads to different user experiences:
- CLN: 1000 sat fee with 500 sat max and 10% max on 50000 sats → fails at 500 sat threshold
- LND: Same scenario → fails with "exceeds max_fee_sats 500" message
Consider aligning the guardrail semantics across providers for consistent behavior.
Also applies to: 193-200
🤖 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/cln/api.rs` around lines 162 - 204, The CLN guardrail in assert_onchain_fee_guardrail currently combines max_fee_sats and max_fee_by_percent with min(...) which enforces both as a single threshold; change it to check each constraint independently like the LND implementation by computing max_fee_by_percent as now but then comparing fee_sats separately against max_fee_sats and against max_fee_by_percent and returning distinct ApiError::InvalidInput messages for each violation (e.g., "exceeds max_fee_sats {max_fee_sats}" and "exceeds max_fee_percent {max_fee_by_percent}") so behavior and user-facing errors match lnd/api.rs.crates/lni/lnd/api.rs (1)
187-196: 💤 Low valueRedundant fallback chain for guardrail defaults.
The fallback
guardrail.max_fee_sats.or(default_guardrail.max_fee_sats).unwrap_or(CONSTANT)is overly complex sincedefault_guardrailisDefault::default()which should already contain the constant values.♻️ Simplified version
- let default_guardrail = crate::types::OnchainFeeGuardrail::default(); let guardrail = options.fee_guardrail.unwrap_or_default(); let max_fee_sats = guardrail .max_fee_sats - .or(default_guardrail.max_fee_sats) .unwrap_or(crate::types::DEFAULT_ONCHAIN_MAX_FEE_SATS); let max_fee_percent = guardrail .max_fee_percent - .or(default_guardrail.max_fee_percent) .unwrap_or(crate::types::DEFAULT_ONCHAIN_MAX_FEE_PERCENT);🤖 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/lnd/api.rs` around lines 187 - 196, The current fallback chain is redundant: you create default_guardrail then call guardrail.max_fee_sats.or(default_guardrail.max_fee_sats).unwrap_or(...). Simplify by relying on options.fee_guardrail.unwrap_or_default() (the Default provides the constants) and then use straightforward unwrap_or on the fields; e.g., compute max_fee_sats from guardrail.max_fee_sats.unwrap_or(crate::types::DEFAULT_ONCHAIN_MAX_FEE_SATS) and max_fee_percent from guardrail.max_fee_percent.unwrap_or(crate::types::DEFAULT_ONCHAIN_MAX_FEE_PERCENT), removing the extra default_guardrail.or(...) logic.
🤖 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/phoenixd.rs`:
- Around line 82-95: The defaulting in pay_onchain currently calls
options.unwrap_or_default(), which leaves Phoenixd-prepared transactions (that
omit fee_sats) rejected by backend guardrails; update the defaulting logic in
the pay_onchain function so that when options is None you construct a
PayOnchainOptions with dangerously_disable_fee_guardrail = true (instead of the
plain Default), then pass that to lni::phoenixd::api::pay_onchain_with_options;
reference the pay_onchain function, PayOnchainOptions type, and the call to
lni::phoenixd::api::pay_onchain_with_options to locate where to change the
default.
In `@bindings/typescript/src/__tests__/cln.test.ts`:
- Around line 82-94: The fetch mock in cln.test.ts is too permissive and should
assert the CLN prepare endpoint and call count; inside the fetchMock
implementation (the vi.fn named fetchMock) check that the received request's URL
pathname equals '/v1/txprepare' and that init?.method (or the request method) is
'POST', and after calling prepareOnchainTransaction() add
expect(fetchMock).toHaveBeenCalledTimes(1); update the assertions in the test
surrounding prepareOnchainTransaction and fetchMock to ensure the RPC path, HTTP
method, and a single network call are verified.
In `@bindings/typescript/src/__tests__/integration/cln.real.test.ts`:
- Around line 71-89: The test currently only calls clnPost('/v1/txdiscard', {
txid: transaction.id }) after assertions, so a failed assertion leaves the
prepared CLN transaction reserved; wrap the discard call in a finally block that
always runs after prepareOnchainTransaction — store the result of
node.prepareOnchainTransaction(...) in the variable transaction, then in finally
check transaction.id and call clnPost('/v1/txdiscard', { txid: transaction.id })
if present; ensure the existing shouldBroadcastOnchain flow still returns early
when appropriate and that node.payOnchain is only invoked when broadcasting.
In `@bindings/typescript/src/__tests__/integration/lnd.real.test.ts`:
- Around line 71-85: Remove the full-object console logs that expose sensitive
runtime fields in the LND on-chain test flow: replace or delete the console.log
calls that print the entire transaction and payment objects (see variables
transaction and payment and the call node.payOnchain) so tests only emit
non-sensitive scalar fields or nothing; if logging is needed, log a minimal
whitelist such as transaction.address, transaction.amountSats, and a sanitized
payment.status or payment.id instead of the whole objects, and ensure the
early-return branch that checks runOnchainSend keeps the minimal logging policy.
In `@crates/lni/phoenixd/api.rs`:
- Around line 425-433: The guardrail check in pay_onchain_with_options calls
assert_onchain_fee_guardrail but prepare_onchain_transaction leaves
transaction.fee.fee_sats as None, causing the guardrail to always error unless
dangerously_disable_fee_guardrail is set; update pay_onchain_with_options (or
assert_onchain_fee_guardrail) to skip enforcement when transaction.fee.fee_sats
is None (i.e., treat unknown fees as "cannot enforce" and allow processing), or
alternatively make prepare_onchain_transaction populate fee_sats before calling
assert_onchain_fee_guardrail; reference prepare_onchain_transaction,
pay_onchain_with_options, assert_onchain_fee_guardrail, transaction.fee.fee_sats
and dangerously_disable_fee_guardrail to locate and change the logic so unknown
fees don't cause a hard guardrail failure.
- Around line 89-103: The sats_per_vbyte value in the
OnchainFeePreferenceType::SatsPerVbyte branch is missing a finite check and can
be NaN/Inf before casting; update the validation for sats_per_vbyte (used to
build PhoenixdOnchainFeeRequest with feerate_sat_byte) to reject non-finite
values by checking !sats_per_vbyte.is_finite() || sats_per_vbyte <= 0.0 and
return the same ApiError::InvalidInput when that condition is true so only
finite, positive rates are ceil()-cast to i64.
In `@crates/lni/phoenixd/lib.rs`:
- Around line 103-108: The forwarded pay_onchain convenience method
(pay_onchain) blindly calls crate::phoenixd::api::pay_onchain with default
guardrail options but prepare_onchain_transaction returns transactions with
fee_sats = None, so phoenixd's api rejects them unless guardrails are disabled;
update pay_onchain to detect a prepared OnchainTransaction with fee_sats == None
and either (A) call crate::phoenixd::api::pay_onchain with the guardrail option
explicitly disabled or (B) return a clear error instructing the caller to
re-quote/prepare with a fee; locate pay_onchain and prepare_onchain_transaction
symbols to implement the check and choose one of these two behaviors so the
prepare->pay flow no longer always fails.
---
Nitpick comments:
In `@crates/lni/cln/api.rs`:
- Around line 162-204: The CLN guardrail in assert_onchain_fee_guardrail
currently combines max_fee_sats and max_fee_by_percent with min(...) which
enforces both as a single threshold; change it to check each constraint
independently like the LND implementation by computing max_fee_by_percent as now
but then comparing fee_sats separately against max_fee_sats and against
max_fee_by_percent and returning distinct ApiError::InvalidInput messages for
each violation (e.g., "exceeds max_fee_sats {max_fee_sats}" and "exceeds
max_fee_percent {max_fee_by_percent}") so behavior and user-facing errors match
lnd/api.rs.
In `@crates/lni/lnd/api.rs`:
- Around line 187-196: The current fallback chain is redundant: you create
default_guardrail then call
guardrail.max_fee_sats.or(default_guardrail.max_fee_sats).unwrap_or(...).
Simplify by relying on options.fee_guardrail.unwrap_or_default() (the Default
provides the constants) and then use straightforward unwrap_or on the fields;
e.g., compute max_fee_sats from
guardrail.max_fee_sats.unwrap_or(crate::types::DEFAULT_ONCHAIN_MAX_FEE_SATS) and
max_fee_percent from
guardrail.max_fee_percent.unwrap_or(crate::types::DEFAULT_ONCHAIN_MAX_FEE_PERCENT),
removing the extra default_guardrail.or(...) logic.
🪄 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: c7b75170-1f18-40b0-811f-e4da4bac1538
📒 Files selected for processing (21)
bindings/lni_nodejs/index.d.tsbindings/lni_nodejs/src/cln.rsbindings/lni_nodejs/src/lnd.rsbindings/lni_nodejs/src/phoenixd.rsbindings/typescript/README.mdbindings/typescript/src/__tests__/cln.test.tsbindings/typescript/src/__tests__/integration/cln.real.test.tsbindings/typescript/src/__tests__/integration/lnd.real.test.tsbindings/typescript/src/__tests__/integration/phoenixd.real.test.tsbindings/typescript/src/__tests__/lnd.test.tsbindings/typescript/src/__tests__/phoenixd.test.tsbindings/typescript/src/nodes/cln.tsbindings/typescript/src/nodes/lnd.tsbindings/typescript/src/nodes/phoenixd.tscrates/lni/cln/api.rscrates/lni/cln/lib.rscrates/lni/lnd/api.rscrates/lni/lnd/lib.rscrates/lni/phoenixd/api.rscrates/lni/phoenixd/lib.rsreadme.md
Comment on lines
+82
to
+95
| #[napi] | ||
| pub async fn pay_onchain( | ||
| &self, | ||
| transaction: OnchainTransaction, | ||
| options: Option<PayOnchainOptions>, | ||
| ) -> Result<lni::types::PayOnchainResponse> { | ||
| lni::phoenixd::api::pay_onchain_with_options( | ||
| self.inner.clone(), | ||
| transaction, | ||
| options.unwrap_or_default(), | ||
| ) | ||
| .await | ||
| .map_err(|e| napi::Error::from_reason(e.to_string())) | ||
| } |
There was a problem hiding this comment.
The JS pay_onchain convenience path will fail for Phoenixd by default.
This wrapper defaults PayOnchainOptions, but Phoenixd prepared transactions intentionally omit fee_sats, so the backend guardrail rejects the payment unless dangerously_disable_fee_guardrail is set. That means the new JS-visible pay_onchain(transaction) API does not work for the normal prepare→pay flow.
Suggested fix
pub async fn pay_onchain(
&self,
transaction: OnchainTransaction,
options: Option<PayOnchainOptions>,
) -> Result<lni::types::PayOnchainResponse> {
- lni::phoenixd::api::pay_onchain_with_options(
- self.inner.clone(),
- transaction,
- options.unwrap_or_default(),
- )
+ let options = options.unwrap_or_default();
+ if transaction.fee_sats.is_none() && !options.dangerously_disable_fee_guardrail {
+ return Err(napi::Error::from_reason(
+ "Phoenixd prepared on-chain transactions do not include feeSats; pass { dangerouslyDisableFeeGuardrail: true } explicitly or use an API that returns a fee quote.".to_string(),
+ ));
+ }
+
+ lni::phoenixd::api::pay_onchain_with_options(
+ self.inner.clone(),
+ transaction,
+ options,
+ )
.await
.map_err(|e| napi::Error::from_reason(e.to_string()))
}📝 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
| #[napi] | |
| pub async fn pay_onchain( | |
| &self, | |
| transaction: OnchainTransaction, | |
| options: Option<PayOnchainOptions>, | |
| ) -> Result<lni::types::PayOnchainResponse> { | |
| lni::phoenixd::api::pay_onchain_with_options( | |
| self.inner.clone(), | |
| transaction, | |
| options.unwrap_or_default(), | |
| ) | |
| .await | |
| .map_err(|e| napi::Error::from_reason(e.to_string())) | |
| } | |
| #[napi] | |
| pub async fn pay_onchain( | |
| &self, | |
| transaction: OnchainTransaction, | |
| options: Option<PayOnchainOptions>, | |
| ) -> Result<lni::types::PayOnchainResponse> { | |
| let options = options.unwrap_or_default(); | |
| if transaction.fee_sats.is_none() && !options.dangerously_disable_fee_guardrail { | |
| return Err(napi::Error::from_reason( | |
| "Phoenixd prepared on-chain transactions do not include feeSats; pass { dangerouslyDisableFeeGuardrail: true } explicitly or use an API that returns a fee quote.".to_string(), | |
| )); | |
| } | |
| lni::phoenixd::api::pay_onchain_with_options( | |
| self.inner.clone(), | |
| transaction, | |
| options, | |
| ) | |
| .await | |
| .map_err(|e| napi::Error::from_reason(e.to_string())) | |
| } |
🤖 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/phoenixd.rs` around lines 82 - 95, The defaulting in
pay_onchain currently calls options.unwrap_or_default(), which leaves
Phoenixd-prepared transactions (that omit fee_sats) rejected by backend
guardrails; update the defaulting logic in the pay_onchain function so that when
options is None you construct a PayOnchainOptions with
dangerously_disable_fee_guardrail = true (instead of the plain Default), then
pass that to lni::phoenixd::api::pay_onchain_with_options; reference the
pay_onchain function, PayOnchainOptions type, and the call to
lni::phoenixd::api::pay_onchain_with_options to locate where to change the
default.
Comment on lines
+82
to
+94
| const fetchMock = vi.fn<FetchLike>(async (_input, init) => { | ||
| const body = init?.body ? JSON.parse(String(init.body)) : undefined; | ||
| expect(body).toEqual({ | ||
| outputs: [{ bc1qxy2kgdygjrsqtzq2n0yrf2493p83kkfjhx0wlh: '10000sat' }], | ||
| feerate: 'normal', | ||
| }); | ||
|
|
||
| return jsonResponse({ | ||
| txid: 'txid-1', | ||
| unsigned_tx: '02000000', | ||
| psbt: testPsbtWithFee(1_000), | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Assert the CLN prepare endpoint and request count.
This mock returns success for any fetch call, so prepareOnchainTransaction() would still pass if it hit the wrong RPC path or made extra network calls. Please assert url.pathname === '/v1/txprepare', the HTTP method, and expect(fetchMock).toHaveBeenCalledTimes(1) here.
🤖 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__/cln.test.ts` around lines 82 - 94, The
fetch mock in cln.test.ts is too permissive and should assert the CLN prepare
endpoint and call count; inside the fetchMock implementation (the vi.fn named
fetchMock) check that the received request's URL pathname equals '/v1/txprepare'
and that init?.method (or the request method) is 'POST', and after calling
prepareOnchainTransaction() add expect(fetchMock).toHaveBeenCalledTimes(1);
update the assertions in the test surrounding prepareOnchainTransaction and
fetchMock to ensure the RPC path, HTTP method, and a single network call are
verified.
Comment on lines
+71
to
+89
| const transaction = await node.prepareOnchainTransaction({ | ||
| address: process.env.CLN_ONCHAIN_TEST_ADDRESS!, | ||
| amountSats, | ||
| fee: { type: 'speed', speed: 'normal' }, | ||
| description: testInvoiceLabel('cln onchain'), | ||
| }); | ||
|
|
||
| expect(transaction.id?.length).toBeGreaterThan(0); | ||
| expect(transaction.amountSats).toBe(amountSats); | ||
| expect(transaction.feeSats).toBeGreaterThanOrEqual(0); | ||
|
|
||
| if (!shouldBroadcastOnchain) { | ||
| await clnPost('/v1/txdiscard', { txid: transaction.id }); | ||
| return; | ||
| } | ||
|
|
||
| const payment = await node.payOnchain(transaction); | ||
| expect(payment.txid?.length).toBeGreaterThan(0); | ||
| }, timeout); |
There was a problem hiding this comment.
Always discard the prepared CLN transaction in a finally block.
Right now txdiscard only runs after the assertions pass. If one of those assertions fails, this test leaves the prepared CLN transaction behind, which can reserve wallet state and poison later runs. Move the discard into finally once transaction.id exists.
Suggested fix
- const transaction = await node.prepareOnchainTransaction({
- address: process.env.CLN_ONCHAIN_TEST_ADDRESS!,
- amountSats,
- fee: { type: 'speed', speed: 'normal' },
- description: testInvoiceLabel('cln onchain'),
- });
-
- expect(transaction.id?.length).toBeGreaterThan(0);
- expect(transaction.amountSats).toBe(amountSats);
- expect(transaction.feeSats).toBeGreaterThanOrEqual(0);
-
- if (!shouldBroadcastOnchain) {
- await clnPost('/v1/txdiscard', { txid: transaction.id });
- return;
- }
-
- const payment = await node.payOnchain(transaction);
- expect(payment.txid?.length).toBeGreaterThan(0);
+ let transaction:
+ | Awaited<ReturnType<ClnNode['prepareOnchainTransaction']>>
+ | undefined;
+
+ try {
+ transaction = await node.prepareOnchainTransaction({
+ address: process.env.CLN_ONCHAIN_TEST_ADDRESS!,
+ amountSats,
+ fee: { type: 'speed', speed: 'normal' },
+ description: testInvoiceLabel('cln onchain'),
+ });
+
+ expect(transaction.id?.length).toBeGreaterThan(0);
+ expect(transaction.amountSats).toBe(amountSats);
+ expect(transaction.feeSats).toBeGreaterThanOrEqual(0);
+
+ if (!shouldBroadcastOnchain) {
+ return;
+ }
+
+ const payment = await node.payOnchain(transaction);
+ expect(payment.txid?.length).toBeGreaterThan(0);
+ } finally {
+ if (!shouldBroadcastOnchain && transaction?.id) {
+ await clnPost('/v1/txdiscard', { txid: transaction.id });
+ }
+ }🤖 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
71 - 89, The test currently only calls clnPost('/v1/txdiscard', { txid:
transaction.id }) after assertions, so a failed assertion leaves the prepared
CLN transaction reserved; wrap the discard call in a finally block that always
runs after prepareOnchainTransaction — store the result of
node.prepareOnchainTransaction(...) in the variable transaction, then in finally
check transaction.id and call clnPost('/v1/txdiscard', { txid: transaction.id })
if present; ensure the existing shouldBroadcastOnchain flow still returns early
when appropriate and that node.payOnchain is only invoked when broadcasting.
Comment on lines
+71
to
+85
| console.log('Prepared LND on-chain transaction:', transaction); | ||
|
|
||
| expect(transaction.address).toBe(process.env.LND_ONCHAIN_TEST_ADDRESS); | ||
| expect(transaction.amountSats).toBe(quoteOnlyAmountSats); | ||
| expect(transaction.feePayer).toBe('sender'); | ||
| expect(transaction.feeSats).toBeGreaterThanOrEqual(0); | ||
|
|
||
| if (!runOnchainSend) { | ||
| console.log('Prepared LND on-chain quote; skipping broadcast without explicit confirmation'); | ||
| return; | ||
| } | ||
|
|
||
| const payment = await node.payOnchain(transaction); | ||
|
|
||
| console.log('LND on-chain payment:', payment); |
There was a problem hiding this comment.
Remove the full-object logs from the real LND on-chain flow.
These console.log calls dump live transaction/payment objects into test output. That exposes runtime payment metadata and any backend-specific raw fields carried on these objects. Please remove them or log only narrowly selected non-sensitive fields.
As per coding guidelines, "Do not print or expose sensitive runtime values in tests and logging, including: API keys, macaroons, runes, passwords, NWC URIs, 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
71 - 85, Remove the full-object console logs that expose sensitive runtime
fields in the LND on-chain test flow: replace or delete the console.log calls
that print the entire transaction and payment objects (see variables transaction
and payment and the call node.payOnchain) so tests only emit non-sensitive
scalar fields or nothing; if logging is needed, log a minimal whitelist such as
transaction.address, transaction.amountSats, and a sanitized payment.status or
payment.id instead of the whole objects, and ensure the early-return branch that
checks runOnchainSend keeps the minimal logging policy.
Comment on lines
+89
to
+103
| OnchainFeePreferenceType::SatsPerVbyte => { | ||
| let sats_per_vbyte = fee.sats_per_vbyte.ok_or_else(|| { | ||
| ApiError::InvalidInput( | ||
| "Phoenixd sats_per_vbyte fee preference requires a fee rate".to_string(), | ||
| ) | ||
| })?; | ||
| if sats_per_vbyte <= 0.0 { | ||
| return Err(ApiError::InvalidInput( | ||
| "Phoenixd sats_per_vbyte fee preference requires a positive fee rate".to_string(), | ||
| )); | ||
| } | ||
|
|
||
| Ok(PhoenixdOnchainFeeRequest { | ||
| feerate_sat_byte: sats_per_vbyte.ceil() as i64, | ||
| }) |
There was a problem hiding this comment.
Missing is_finite() check for sats_per_vbyte.
The LND implementation (line 152) checks !sats_per_vbyte.is_finite() || sats_per_vbyte <= 0.0, but Phoenixd only checks <= 0.0. This allows NaN or Infinity to pass validation and be cast to i64, resulting in undefined behavior.
🐛 Proposed fix
if sats_per_vbyte <= 0.0 {
+ if !sats_per_vbyte.is_finite() || sats_per_vbyte <= 0.0 {
return Err(ApiError::InvalidInput(
"Phoenixd sats_per_vbyte fee preference requires a positive fee rate".to_string(),
));
}🤖 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/phoenixd/api.rs` around lines 89 - 103, The sats_per_vbyte value
in the OnchainFeePreferenceType::SatsPerVbyte branch is missing a finite check
and can be NaN/Inf before casting; update the validation for sats_per_vbyte
(used to build PhoenixdOnchainFeeRequest with feerate_sat_byte) to reject
non-finite values by checking !sats_per_vbyte.is_finite() || sats_per_vbyte <=
0.0 and return the same ApiError::InvalidInput when that condition is true so
only finite, positive rates are ceil()-cast to i64.
Comment on lines
+425
to
+433
| pub async fn pay_onchain_with_options( | ||
| config: PhoenixdConfig, | ||
| transaction: OnchainTransaction, | ||
| options: PayOnchainOptions, | ||
| ) -> Result<PayOnchainResponse, ApiError> { | ||
| assert_valid_onchain_amount(transaction.amount_sats)?; | ||
| let _fee_payer = resolve_phoenixd_fee_payer(Some(transaction.fee_payer.clone()))?; | ||
| let fee_request = resolve_phoenixd_fee_request(&transaction.fee)?; | ||
| assert_onchain_fee_guardrail(&transaction, options)?; |
There was a problem hiding this comment.
Phoenixd on-chain payments always fail guardrail check unless explicitly disabled.
prepare_onchain_transaction sets fee_sats: None (line 397), but assert_onchain_fee_guardrail (lines 149-153) requires fee_sats to be Some or returns an error. This means Phoenixd users must always pass dangerously_disable_fee_guardrail: true to use pay_onchain, defeating the safety purpose of the guardrail.
Consider either:
- Skipping guardrail enforcement when
fee_satsisNone(since Phoenixd doesn't support fee estimation) - Documenting this behavior clearly for Phoenixd users
- Having Phoenixd estimate fees differently
♻️ Option 1: Skip guardrail when fee unknown
fn assert_onchain_fee_guardrail(
transaction: &OnchainTransaction,
options: PayOnchainOptions,
) -> Result<(), ApiError> {
if options.dangerously_disable_fee_guardrail {
return Ok(());
}
+
+ // If fee_sats is unknown, we cannot enforce guardrails - caller must explicitly disable
+ let fee_sats = match transaction.fee_sats {
+ Some(fee) => fee,
+ None => return Ok(()), // Cannot enforce without fee information
+ };🤖 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/phoenixd/api.rs` around lines 425 - 433, The guardrail check in
pay_onchain_with_options calls assert_onchain_fee_guardrail but
prepare_onchain_transaction leaves transaction.fee.fee_sats as None, causing the
guardrail to always error unless dangerously_disable_fee_guardrail is set;
update pay_onchain_with_options (or assert_onchain_fee_guardrail) to skip
enforcement when transaction.fee.fee_sats is None (i.e., treat unknown fees as
"cannot enforce" and allow processing), or alternatively make
prepare_onchain_transaction populate fee_sats before calling
assert_onchain_fee_guardrail; reference prepare_onchain_transaction,
pay_onchain_with_options, assert_onchain_fee_guardrail, transaction.fee.fee_sats
and dangerously_disable_fee_guardrail to locate and change the logic so unknown
fees don't cause a hard guardrail failure.
Comment on lines
+103
to
+108
| pub async fn pay_onchain( | ||
| &self, | ||
| transaction: OnchainTransaction, | ||
| ) -> Result<PayOnchainResponse, ApiError> { | ||
| crate::phoenixd::api::pay_onchain(self.config.clone(), transaction).await | ||
| } |
There was a problem hiding this comment.
pay_onchain is broken for prepared Phoenixd transactions.
The new convenience method forwards default guardrail options, but this backend's own prepare flow returns fee_sats = None (Lines 266-270), and crates/lni/phoenixd/api.rs:134-176 rejects payment when fee_sats is unknown unless the caller explicitly disables the guardrail. As added, prepare_onchain_transaction(...) followed by pay_onchain(...) will fail every time for Phoenixd.
Suggested direction
- pub async fn pay_onchain(
- &self,
- transaction: OnchainTransaction,
- ) -> Result<PayOnchainResponse, ApiError> {
- crate::phoenixd::api::pay_onchain(self.config.clone(), transaction).await
- }
+ pub async fn pay_onchain(
+ &self,
+ _transaction: OnchainTransaction,
+ ) -> Result<PayOnchainResponse, ApiError> {
+ Err(ApiError::InvalidInput(
+ "Phoenixd prepared on-chain transactions do not include fee_sats; use pay_onchain_with_options(...) explicitly until prepare_onchain_transaction returns a fee quote.".to_string(),
+ ))
+ }Longer term, the better fix is to either quote fee_sats during prepare or avoid exposing this convenience API for Phoenixd.
📝 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
| pub async fn pay_onchain( | |
| &self, | |
| transaction: OnchainTransaction, | |
| ) -> Result<PayOnchainResponse, ApiError> { | |
| crate::phoenixd::api::pay_onchain(self.config.clone(), transaction).await | |
| } | |
| pub async fn pay_onchain( | |
| &self, | |
| _transaction: OnchainTransaction, | |
| ) -> Result<PayOnchainResponse, ApiError> { | |
| Err(ApiError::InvalidInput( | |
| "Phoenixd prepared on-chain transactions do not include fee_sats; use pay_onchain_with_options(...) explicitly until prepare_onchain_transaction returns a fee quote.".to_string(), | |
| )) | |
| } |
🤖 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/phoenixd/lib.rs` around lines 103 - 108, The forwarded pay_onchain
convenience method (pay_onchain) blindly calls crate::phoenixd::api::pay_onchain
with default guardrail options but prepare_onchain_transaction returns
transactions with fee_sats = None, so phoenixd's api rejects them unless
guardrails are disabled; update pay_onchain to detect a prepared
OnchainTransaction with fee_sats == None and either (A) call
crate::phoenixd::api::pay_onchain with the guardrail option explicitly disabled
or (B) return a clear error instructing the caller to re-quote/prepare with a
fee; locate pay_onchain and prepare_onchain_transaction symbols to implement the
check and choose one of these two behaviors so the prepare->pay flow no longer
always fails.
There was a problem hiding this comment.
🧹 Nitpick comments (3)
bindings/typescript/src/__tests__/lnd.test.ts (2)
172-189: ⚡ Quick winAdd test coverage for guardrail disabled case.
The test name mentions "unless the fee guardrail is disabled" but only validates the blocking behavior. To complete the test matrix, add a complementary test that passes
{ feeGuardrail: false }topayOnchainand verifies that manual sat/vbyte execution succeeds even without fee estimates.🧪 Suggested test case to add
it('allows manual sat/vbyte execution when the fee guardrail is disabled', async () => { const fetchMock = vi.fn<FetchLike>(async (input, init) => { const url = new URL(String(input)); const body = init?.body ? JSON.parse(String(init.body)) : undefined; if (url.pathname === '/v1/transactions') { expect(body).toMatchObject({ addr: 'bc1qxy2kgdygjrsqtzq2n0yrf2493p83kkfjhx0wlh', amount: 10_000, sat_per_vbyte: '5', }); return jsonResponse({ txid: 'txid-manual' }); } return new Response('not found', { status: 404 }); }); const node = new LndNode( { url: 'https://lnd.test', macaroon: '00' }, { fetch: fetchMock }, ); const payment = await node.payOnchain( { address: 'bc1qxy2kgdygjrsqtzq2n0yrf2493p83kkfjhx0wlh', amountSats: 10_000, recipientAmountSats: 10_000, feePayer: 'sender', fee: { type: 'satsPerVbyte', satsPerVbyte: 5 }, }, { feeGuardrail: false }, // Disable guardrail ); expect(payment).toMatchObject({ txid: 'txid-manual', state: 'pending', }); expect(fetchMock).toHaveBeenCalledOnce(); });🤖 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__/lnd.test.ts` around lines 172 - 189, Add a complementary test that verifies payOnchain accepts manual sats-per-vbyte when the fee guardrail is disabled: create a fetchMock that responds to POST /v1/transactions with a successful txid, instantiate LndNode, call node.payOnchain with the same payload used in the failing test but pass the options { feeGuardrail: false }, and assert the returned payment has txid and state 'pending' and that fetchMock was called; reference the LndNode class and its payOnchain method and the existing fetchMock pattern to locate where to add this test.
74-78: ⚡ Quick winConsider explicitly asserting that
raw.estimateis absent.The test validates that
raw.sendRequestandraw.labelare present, but doesn't verify thatraw.estimateis undefined. According to the upstream contract (context snippet 2), the manual sat/vbyte path returns early and never populates theestimatefield.✨ Suggested assertion for completeness
expect(transaction.feeSats).toBeUndefined(); expect(transaction.totalAmountSats).toBeUndefined(); + expect(transaction.raw?.estimate).toBeUndefined(); expect(fetchMock).not.toHaveBeenCalled();🤖 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__/lnd.test.ts` around lines 74 - 78, The test currently asserts that raw.sendRequest and raw.label are present but misses asserting that raw.estimate is absent; update the test in bindings/typescript/src/__tests__/lnd.test.ts to explicitly assert that raw.estimate is undefined (e.g., using expect(raw.estimate).toBeUndefined()) after the existing assertions so the manual sat_per_vbyte code path's contract (which should not populate estimate) is enforced; locate the assertion block where the response object "raw" is inspected and add the explicit absence check for raw.estimate.crates/lni/nwc/lib.rs (1)
167-194: ⚡ Quick winConsider using the existing lazy_static
NODEfor consistency.This test creates a new
NwcNodeinstance and manually loadsNWC_URIwith graceful skip logic, while all other tests in this file use the shared lazy_staticNODE(lines 150-156). This creates inconsistency and duplicates the node setup logic.Additionally, the graceful skip doesn't reliably work: if any other test runs first, the lazy_static initialization will panic if
NWC_URIis not set (line 148), so this test would never get the chance to skip gracefully.For consistency and to reduce duplication, consider using the existing
NODE:♻️ Refactor to use shared lazy_static NODE
#[tokio::test] async fn test_get_permissions() { - dotenv().ok(); - let nwc_uri = match env::var("NWC_URI") { - Ok(nwc_uri) => nwc_uri, - Err(_) => { - eprintln!("Skipping NWC permissions test because NWC_URI is not set"); - return; - } - }; - let node = NwcNode::new(NwcConfig { - nwc_uri, - ..Default::default() - }); - - match node.get_permissions().await { + match NODE.get_permissions().await { Ok(permissions) => { dbg!(&permissions); assert!( !permissions.is_empty(), "NWC permissions should include at least one supported method" ); } Err(e) => { panic!("Failed to get NWC permissions: {:?}", e); } } }Alternatively, if the graceful skip behavior is important, consider restructuring all tests to use the same optional pattern.
🤖 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/lib.rs` around lines 167 - 194, The test creates a new NwcNode and re-implements NWC_URI loading/skipping; instead use the shared lazy_static NODE used by the other tests: remove the dotenv/env block and NwcNode::new call in test_get_permissions and instead call NODE.get_permissions().await (or NODE.as_ref()/clone() as appropriate to your NODE type), then assert on the returned permissions or panic on Err as before; this keeps setup consistent with other tests and avoids duplicating NWC_URI handling.
🤖 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.
Nitpick comments:
In `@bindings/typescript/src/__tests__/lnd.test.ts`:
- Around line 172-189: Add a complementary test that verifies payOnchain accepts
manual sats-per-vbyte when the fee guardrail is disabled: create a fetchMock
that responds to POST /v1/transactions with a successful txid, instantiate
LndNode, call node.payOnchain with the same payload used in the failing test but
pass the options { feeGuardrail: false }, and assert the returned payment has
txid and state 'pending' and that fetchMock was called; reference the LndNode
class and its payOnchain method and the existing fetchMock pattern to locate
where to add this test.
- Around line 74-78: The test currently asserts that raw.sendRequest and
raw.label are present but misses asserting that raw.estimate is absent; update
the test in bindings/typescript/src/__tests__/lnd.test.ts to explicitly assert
that raw.estimate is undefined (e.g., using
expect(raw.estimate).toBeUndefined()) after the existing assertions so the
manual sat_per_vbyte code path's contract (which should not populate estimate)
is enforced; locate the assertion block where the response object "raw" is
inspected and add the explicit absence check for raw.estimate.
In `@crates/lni/nwc/lib.rs`:
- Around line 167-194: The test creates a new NwcNode and re-implements NWC_URI
loading/skipping; instead use the shared lazy_static NODE used by the other
tests: remove the dotenv/env block and NwcNode::new call in test_get_permissions
and instead call NODE.get_permissions().await (or NODE.as_ref()/clone() as
appropriate to your NODE type), then assert on the returned permissions or panic
on Err as before; this keeps setup consistent with other tests and avoids
duplicating NWC_URI handling.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 91bf3350-ef06-46f5-b545-27d2a1a62906
📒 Files selected for processing (6)
bindings/typescript/README.mdbindings/typescript/src/__tests__/lnd.test.tsbindings/typescript/src/nodes/lnd.tscrates/lni/lnd/api.rscrates/lni/nwc/lib.rsreadme.md
🚧 Files skipped from review as they are similar to previous changes (2)
- crates/lni/lnd/api.rs
- bindings/typescript/src/nodes/lnd.ts
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.
Testing
For each test the different fee configs.
Summary by CodeRabbit
New Features
Documentation
Tests