Search #10
Search #10
Conversation
WalkthroughThis update refactors the invoice lookup and transaction listing APIs across the codebase to accept structured parameter objects that include both a payment hash and an optional search term. The changes introduce new parameter structs ( Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant NodeJS_Binding
participant Rust_Node
participant API_Module
participant Backend
User->>NodeJS_Binding: lookupInvoice({payment_hash, search})
NodeJS_Binding->>Rust_Node: lookup_invoice(LookupInvoiceParams)
Rust_Node->>API_Module: lookup_invoice(payment_hash, ..., search)
API_Module->>Backend: Query invoice(s) by hash or search term
Backend-->>API_Module: Invoice(s) data
API_Module-->>Rust_Node: Transaction(s)
Rust_Node-->>NodeJS_Binding: Transaction(s)
NodeJS_Binding-->>User: Transaction(s)
Possibly related PRs
Poem
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🔭 Outside diff range comments (2)
bindings/lni_nodejs/src/phoenixd.rs (1)
94-102:⚠️ Potential issueMissing search parameter in list_transactions.
While the
lookup_invoicemethod was updated to pass the search parameter, thelist_transactionsmethod still passesNonefor the search parameter instead ofparams.search. This is inconsistent with the implementations in other backend files likelnd.rs.-let txns = lni::phoenixd::api::list_transactions(&self.inner, params.from, params.limit, None) +let txns = lni::phoenixd::api::list_transactions(&self.inner, params.from, params.limit, params.search)crates/lni/lnd/api.rs (1)
402-407: 💡 Verification agent🧩 Analysis chain
Added search parameter to list_transactions.
The function now accepts an optional search parameter, aligning with the common interface across backends.
However, similar to
lookup_invoice, the implementation doesn't appear to use this new parameter for querying. The function still constructs the URL using only the pagination parameters.
🏁 Script executed:
#!/bin/bash # Check if there are any implementations that actually use the search parameter # Look for SQL queries that might use the search parameter rg -A 10 "list_transactions.*search" --type=rust # Check the CLN implementation which might have SQL-based searching ast-grep --pattern 'fn list_transactions($config, $from, $limit, $search) { $$$'Length of output: 2844
🏁 Script executed:
#!/bin/bash # Inspect the LND API implementation of list_transactions rg -n "pub fn list_transactions" -A 20 crates/lni/lnd/api.rsLength of output: 881
Add the
searchparameter to the LNDlist_transactionsURLThe
list_transactionssignature now acceptssearch: Option<String>, but the URL built incrates/lni/lnd/api.rs(around lines 408–411) never uses it. Update the URL builder so that whensearchisSome, its value is appended to the query string.Locations to update:
- crates/lni/lnd/api.rs, lines 408–411Suggested patch:
- let list_txns_url = format!( - "{}/v1/invoices?index_offest={}&num_max_invoices={}", - config.url, from, limit - ); + let mut list_txns_url = format!( + "{}/v1/invoices?index_offset={}&num_max_invoices={}", + config.url, from, limit + ); + if let Some(ref term) = search { + list_txns_url.push_str(&format!("&search={}", term)); + }This ensures the new
searchargument is actually applied to the API request.
🧹 Nitpick comments (5)
crates/lni/cln/api.rs (2)
518-530: Removedbg!/println!noise or gate it behindlog/tracing
dbg!(...)and plainprintln!statements will leak SQL queries and possibly sensitive data into STDOUT in production. Prefer a structured logger with an appropriate log‑level check:tracing::debug!(?sql, "Executing invoice search");Or remove them once debugging is done.
692-717: Infinite‑poll risk when node is unreachableWith the
breakremoved on"error", a permanently failing lookup loops untilmax_polling_secexpires, potentially spamming the backend everypolling_delay_sec. Consider:
- counting consecutive errors and aborting early after N failures, or
- exponentially backing‑off, or
- reinstating the
breakwhen the HTTP client itself errors (network down).This saves bandwidth and avoids tying up threads.
crates/lni/cln/lib.rs (3)
86-94: Passfrom/limitexplicitly or expose them inLookupInvoiceParamsHard‑coding
from = None, limit = Nonemeans clients cannot page through historic invoices via this high‑level call. Either:
- expose
fromandlimitinLookupInvoiceParams, or- document that
lookup_invoicealways queries “latest / single” records.Minor, but clarifying prevents future misuse.
305-335: Unit test duplicates same assertion; consider table‑driven style
test_lookup_invoicerepeats almost identical blocks forpayment_hashandsearch. A small table‑driven loop will cut duplication and make it easier to add more cases:for params in [ LookupInvoiceParams { payment_hash: Some(TEST_PAYMENT_HASH.clone()), ..Default::default() }, LookupInvoiceParams { search: Some(TEST_PAYMENT_HASH.clone()), ..Default::default() }, ] { let txn = NODE.lookup_invoice(params).expect("lookup failed"); assert!(txn.amount_msats >= 0); }
375-377: Avoid leavingdbg!in shipped tests
dbg!(transaction);prints to STDERR every test run and is easy to forget. Replace with an assertion or remove after debugging.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
bindings/lni_nodejs/src/cln.rs(3 hunks)bindings/lni_nodejs/src/lnd.rs(3 hunks)bindings/lni_nodejs/src/phoenixd.rs(2 hunks)crates/lni/cln/api.rs(8 hunks)crates/lni/cln/lib.rs(6 hunks)crates/lni/lnd/api.rs(6 hunks)crates/lni/lnd/lib.rs(6 hunks)crates/lni/phoenixd/api.rs(4 hunks)crates/lni/phoenixd/lib.rs(6 hunks)crates/lni/types.rs(3 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (4)
crates/lni/lnd/api.rs (9)
bindings/lni_nodejs/src/cln.rs (1)
lookup_invoice(87-92)bindings/lni_nodejs/src/lnd.rs (1)
lookup_invoice(85-90)bindings/lni_nodejs/src/phoenixd.rs (1)
lookup_invoice(70-80)crates/lni/cln/api.rs (2)
lookup_invoice(471-490)params(250-253)crates/lni/cln/lib.rs (1)
lookup_invoice(86-94)crates/lni/lnd/lib.rs (1)
lookup_invoice(81-89)crates/lni/phoenixd/lib.rs (1)
lookup_invoice(86-94)crates/lni/phoenixd/api.rs (1)
lookup_invoice(248-282)crates/lni/types.rs (1)
lookup_invoice(25-25)
crates/lni/lnd/lib.rs (5)
bindings/lni_nodejs/src/lnd.rs (2)
lookup_invoice(85-90)list_transactions(93-101)crates/lni/cln/lib.rs (3)
lookup_invoice(86-94)list_transactions(96-101)default(21-29)crates/lni/lnd/api.rs (3)
lookup_invoice(342-400)params(242-245)list_transactions(402-465)crates/lni/phoenixd/api.rs (2)
lookup_invoice(248-282)list_transactions(284-418)crates/lni/types.rs (7)
lookup_invoice(25-25)list_transactions(26-29)default(74-90)default(301-306)default(327-341)default(374-388)default(408-415)
bindings/lni_nodejs/src/cln.rs (6)
bindings/lni_nodejs/index.d.ts (2)
ClnConfig(28-34)Transaction(71-85)bindings/lni_nodejs/src/lnd.rs (2)
lookup_invoice(85-90)list_transactions(93-101)bindings/lni_nodejs/src/phoenixd.rs (2)
lookup_invoice(70-80)list_transactions(95-102)crates/lni/cln/api.rs (3)
lookup_invoice(471-490)params(250-253)list_transactions(667-677)crates/lni/cln/lib.rs (2)
lookup_invoice(86-94)list_transactions(96-101)crates/lni/types.rs (2)
lookup_invoice(25-25)list_transactions(26-29)
crates/lni/types.rs (10)
bindings/lni_nodejs/src/cln.rs (1)
lookup_invoice(87-92)bindings/lni_nodejs/src/lnd.rs (1)
lookup_invoice(85-90)bindings/lni_nodejs/src/phoenixd.rs (1)
lookup_invoice(70-80)crates/lni/cln/api.rs (2)
lookup_invoice(471-490)params(250-253)crates/lni/cln/lib.rs (2)
lookup_invoice(86-94)default(21-29)crates/lni/lnd/lib.rs (2)
lookup_invoice(81-89)default(24-32)crates/lni/lnd/api.rs (2)
lookup_invoice(342-400)params(242-245)crates/lni/phoenixd/lib.rs (2)
lookup_invoice(86-94)default(22-30)crates/lni/phoenixd/api.rs (1)
lookup_invoice(248-282)bindings/lni_nodejs/index.d.ts (1)
Transaction(71-85)
🔇 Additional comments (35)
bindings/lni_nodejs/src/phoenixd.rs (2)
1-3: Addition of LookupInvoiceParams import.The import statement now includes the
LookupInvoiceParamsstruct, which is needed for the updated method signatures.
70-79: Method signature updated to support search functionality.The
lookup_invoicemethod now accepts a structured parameter object instead of just a payment hash, which aligns with the updated trait definition in the underlying API.bindings/lni_nodejs/src/lnd.rs (3)
1-1: Addition of LookupInvoiceParams import.The import statement now includes the
LookupInvoiceParamsstruct, which is needed for the updated method signatures.
85-89: Method signature updated to support search functionality.The
lookup_invoicemethod now accepts a structured parameter object instead of just a payment hash, which aligns with the updated trait definition in the underlying API.
97-99: Updated list_transactions to support search.The
list_transactionsmethod now correctly passes the search parameter to the API.bindings/lni_nodejs/src/cln.rs (3)
1-1: Addition of LookupInvoiceParams import.The import statement now includes the
LookupInvoiceParamsstruct, which is needed for the updated method signatures.
87-91: Method signature updated to support search functionality.The
lookup_invoicemethod now accepts a structured parameter object instead of just a payment hash, which aligns with the updated trait definition in the underlying API.
99-101: Updated list_transactions to support search.The
list_transactionsmethod now correctly passes the search parameter to the API.crates/lni/lnd/api.rs (3)
11-13: Imports reordered.The import statement has been reorganized, with no functional changes.
480-486: Updated poll_invoice_events to pass search parameter.The function now forwards the search parameter from the event params to the lookup_invoice call, ensuring search functionality works during invoice polling.
504-504: Error handling behavior change in poll_invoice_events.The
breakstatement has been commented out, which changes the behavior to continue polling even after an error occurs. This could lead to continued polling despite persistent errors.Is this change intentional? If so, consider adding a comment explaining why the function should continue polling after errors, and perhaps add a counter to limit the number of consecutive errors before breaking.
-// break; +// Don't break on error, allow retry on transient failures +error_count += 1; +if error_count > MAX_ERROR_RETRIES { + break; +}crates/lni/phoenixd/api.rs (4)
248-254: Updated function signature for search supportThe function signature has been updated to accept optional parameters, aligning with the refactoring for search functionality across the codebase.
284-290: Updated function signature for search supportThe function signature has been updated to replace
payment_hashwithsearchparameter, aligning with the refactoring for search functionality across the codebase.
436-442: Updated invoice lookup call with new parametersThe call to
lookup_invoicehas been updated to pass the new optional parameters, supporting the search capability in invoice polling.
459-461: Changed polling behavior to continue on errorThe
breakstatement in the error handling branch has been commented out, which changes the behavior to continue polling even after an error occurs. This behavior change is consistent with the changes in other node implementations.Consider whether this change was intentional. If the intention is to be resilient to temporary errors, this is a good change. However, it might lead to continued polling when the error is permanent, which could result in unnecessary resource consumption.
crates/lni/types.rs (5)
25-25: Updated LightningNode trait for flexible invoice lookupThe
lookup_invoicemethod signature has been changed to accept a structured params object, enabling more flexible invoice lookup by payment hash or search term.
290-291: Added search capability to ListTransactionsParamsThe addition of the
searchfield toListTransactionsParamsenables filtering transactions by payer_note, memo, or description.
293-307: Added new LookupInvoiceParams struct with Default implementationThe new
LookupInvoiceParamsstruct provides a structured way to look up invoices by eitherpayment_hashorsearchterm. The Default implementation appropriately initializes optional fields toNone.
402-404: Updated OnInvoiceEventParams for search supportThe
payment_hashfield is now optional, and a newsearchfield has been added, providing more flexibility in how invoices are tracked for events.
407-416: Added Default implementation for OnInvoiceEventParamsThe Default implementation establishes reasonable defaults for polling parameters and initializes optional fields to
None.crates/lni/phoenixd/lib.rs (7)
9-9: Updated imports for new parameter structThe import has been updated to include the new
LookupInvoiceParamsstruct.
86-94: Updated lookup_invoice implementation for structured parametersThe
lookup_invoiceimplementation has been updated to use the new parameter struct, forwarding optional fields to the API call.
100-105: Updated list_transactions implementation for search supportThe method now forwards the optional
searchparameter to the API call, enabling text-based filtering of transactions.
253-254: Updated test case for search parameterThe test has been updated to include the new
searchfield inListTransactionsParams, maintaining compatibility with the updated method signature.
257-258: Improved debugging in testsReplaced
println!withdbg!for better debug output when running tests, which provides variable names and line numbers in addition to values.Also applies to: 274-275
269-272: Updated test for lookup_invoiceThe test now constructs a
LookupInvoiceParamswith the payment hash wrapped inSome, using the Default implementation for other fields.
299-303: Updated OnInvoiceEventParams usage in testThe test now wraps the
payment_hashinSomeand uses default values for other fields, consistent with the updated struct definition.crates/lni/lnd/lib.rs (7)
6-7: Updated imports for new parameter structsThe imports have been updated to include the new
LookupInvoiceParamsstruct.
81-89: Updated lookup_invoice implementation for structured parametersThe method implementation now accepts
LookupInvoiceParamsand correctly forwards the optional fields to the API call, enabling flexible invoice lookup by payment hash or search term.
95-95: Updated list_transactions implementation for search supportThe implementation now forwards the
searchparameter to the API call, enabling text-based filtering of transactions.
307-310: Updated test for lookup_invoiceThe test now constructs a
LookupInvoiceParamswith the payment hash wrapped inSomeand uses the Default implementation for other fields.
312-312: Improved debugging in testsReplaced
println!withdbg!for better debug output when running tests, providing variable names and line numbers in addition to values.Also applies to: 338-338, 370-370
334-335: Updated test case for search parameterThe test has been updated to include the new
searchfield inListTransactionsParams, maintaining compatibility with the updated method signature.
390-394: Updated OnInvoiceEventParams usage in testThe test now uses the updated parameter structure with optional
payment_hashand default values, aligning with the broader refactoring for search support.crates/lni/cln/api.rs (1)
612-618: Ambiguous precedence when bothpayment_hashandsearchare supplied
pay_hashpreferssearchoverpayment_hash; callers providing both may be surprised that the explicitpayment_hashis ignored. Consider making the precedence explicit in the public API (docs or type system) or, ideally, reject the call as invalid:if payment_hash.is_some() && search.is_some() { return Err(ApiError::Json { reason: "Specify either payment_hash OR search, not both".into(), }); }
| pub fn lookup_invoice( | ||
| config: &LndConfig, | ||
| payment_hash: Option<String>, | ||
| from: Option<i64>, | ||
| limit: Option<i64>, | ||
| search: Option<String>, | ||
| ) -> Result<Transaction, ApiError> { |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Expanded lookup_invoice signature to support search.
The function now accepts optional pagination (from, limit) and search parameters, aligning with the common interface across backends.
However, I notice that while the function signature has been updated, the implementation doesn't appear to use these new parameters for querying. The function still constructs the URL using only the payment hash.
🏁 Script executed:
#!/bin/bash
# Verify if these parameters are used elsewhere in the implementation
# Check if there are any SQL queries that might use these parameters
rg -A 10 -B 5 "lookup_invoices.*search" --type=rust
# Look for any other implementations that might use these parameters
rg -A 5 "search: Option<String>" --type=rustLength of output: 10744
LND lookup_invoice ignores new from, limit, and search parameters
The signature was expanded for pagination and full‑text search, but the implementation never uses these values in its HTTP requests. Please update both methods in crates/lni/lnd/api.rs:
• In lookup_invoice (around the list_invoices_url construction), only payment_hash is used. Append from, limit, and search as query parameters.
• In lookup_invoices (around the list_txns_url construction), you already include index_offest (typo?) and num_max_invoices, but omit search—and should correct index_offest to index_offset.
For example, you might change:
- let list_invoices_url = format!("{}/v1/invoice/{}", config.url, payment_hash_str);
+ let list_invoices_url = format!(
+ "{}/v1/invoice/{}?index_offset={}&num_max_invoices={}&search={}",
+ config.url,
+ payment_hash_str,
+ from.unwrap_or_default(),
+ limit.unwrap_or_default(),
+ search.clone().unwrap_or_default()
+ );And similarly include &search={} in lookup_invoices.
Committable suggestion skipped: line range outside the PR's diff.
| ) -> Result<Transaction, ApiError> { | ||
| let url = format!("{}/payments/incoming/{}", config.url, payment_hash); | ||
| let url = format!("{}/payments/incoming/{}", config.url, payment_hash.unwrap()); |
There was a problem hiding this comment.
Potential panic when payment_hash is None
The code unconditionally unwraps the payment_hash option which will panic if it is None. This needs to be handled properly since payment_hash is now optional.
Consider implementing one of these solutions:
- let url = format!("{}/payments/incoming/{}", config.url, payment_hash.unwrap());
+ let payment_hash_str = payment_hash.ok_or_else(|| ApiError::Api {
+ reason: "Payment hash is required for lookup_invoice".to_string(),
+ })?;
+ let url = format!("{}/payments/incoming/{}", config.url, payment_hash_str);Or if search by payment hash is required:
- let url = format!("{}/payments/incoming/{}", config.url, payment_hash.unwrap());
+ if let Some(hash) = payment_hash {
+ let url = format!("{}/payments/incoming/{}", config.url, hash);
+ // rest of the function
+ } else {
+ return Err(ApiError::Api {
+ reason: "Payment hash is required for lookup_invoice".to_string(),
+ });
+ }| if response_text.len() > 25 { // i.e not blank resp like "[rows: []]" | ||
| // Parse the SQL response into InvoicesResponse | ||
| #[derive(serde::Deserialize)] | ||
| struct SqlResponse { | ||
| rows: Vec<Vec<serde_json::Value>>, | ||
| } | ||
| // Map SQL row indices to InvoicesResponse fields | ||
| let sql_resp: SqlResponse = serde_json::from_str(response_text).unwrap(); | ||
|
|
||
| let mut invoices = Vec::new(); | ||
| for row in sql_resp.rows { | ||
| invoices.push(Invoice { | ||
| label: row | ||
| .get(0) | ||
| .and_then(|v| v.as_str()) | ||
| .unwrap_or_default() | ||
| .to_string(), | ||
| bolt11: row.get(1).and_then(|v| v.as_str()).map(|s| s.to_string()), | ||
| bolt12: row.get(2).and_then(|v| v.as_str()).map(|s| s.to_string()), | ||
| payment_hash: row | ||
| .get(3) | ||
| .and_then(|v| v.as_str()) | ||
| .unwrap_or_default() | ||
| .to_string(), | ||
| amount_msat: Some(row.get(4).and_then(|v| v.as_i64()).unwrap_or(0)), | ||
| status: row | ||
| .get(5) | ||
| .and_then(|v| v.as_str()) | ||
| .unwrap_or_default() | ||
| .to_string(), | ||
| amount_received_msat: row.get(6).and_then(|v| v.as_i64()), | ||
| paid_at: row.get(7).and_then(|v| v.as_i64()), | ||
| payment_preimage: row.get(8).and_then(|v| v.as_str()).map(|s| s.to_string()), | ||
| description: row.get(9).and_then(|v| v.as_str()).map(|s| s.to_string()), | ||
| local_offer_id: row.get(10).and_then(|v| v.as_str()).map(|s| s.to_string()), | ||
| invreq_payer_note: row.get(11).and_then(|v| v.as_str()).map(|s| s.to_string()), | ||
| expires_at: row.get(12).and_then(|v| v.as_i64()).unwrap_or(0), | ||
| pay_index: None, | ||
| created_index: 0, | ||
| updated_index: None, | ||
| paid_outpoint: None, | ||
| }); | ||
| } | ||
| let incoming_payments = InvoicesResponse { invoices }; | ||
| let mut transactions: Vec<Transaction> = incoming_payments | ||
| .invoices | ||
| .into_iter() | ||
| .map(|inv| Transaction { | ||
| type_: "incoming".to_string(), | ||
| invoice: inv | ||
| .bolt11 | ||
| .clone() | ||
| .unwrap_or_else(|| inv.bolt12.clone().unwrap_or_default()), | ||
| preimage: inv.payment_preimage.unwrap_or_default(), | ||
| payment_hash: inv.payment_hash, | ||
| amount_msats: inv.amount_received_msat.unwrap_or(0), | ||
| fees_paid: 0, | ||
| created_at: 0, // TODO: parse if available | ||
| expires_at: inv.expires_at, | ||
| settled_at: inv.paid_at.unwrap_or(0), | ||
| description: inv.description.unwrap_or_default(), | ||
| description_hash: "".to_string(), | ||
| payer_note: Some(inv.invreq_payer_note.unwrap_or_default()), | ||
| external_id: Some(inv.label), | ||
| }) | ||
| .collect(); | ||
| transactions.sort_by(|a, b| b.created_at.cmp(&a.created_at)); | ||
| return Ok(transactions); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
if response_text.len() > 25 is a brittle success check
Using string length to detect an empty result will break as soon as the backend response format changes (e.g. whitespace, extra metadata). Instead, always parse the JSON and check rows.is_empty():
let sql_resp: SqlResponse = serde_json::from_str(response_text)?;
if sql_resp.rows.is_empty() {
return Ok(Vec::new());
}This eliminates the magic number 25 and avoids false negatives.
| if search.is_some() { | ||
| let list_invoices_url = format!("{}/v1/sql", config.url); | ||
| let sql = format!( | ||
| "SELECT label, bolt11, bolt12, payment_hash, amount_msat, status, amount_received_msat, paid_at, payment_preimage, description, local_offer_id, invreq_payer_note, expires_at FROM invoices" | ||
| ); | ||
| let where_clause = if search.is_some() { | ||
| format!( | ||
| "WHERE description = '{}' or invreq_payer_note ='{}' or payment_hash = '{}' ORDER BY created_index DESC LIMIT {}", | ||
| search.clone().unwrap(), | ||
| search.clone().unwrap(), | ||
| search.clone().unwrap(), | ||
| limit.unwrap_or(150), | ||
| ) | ||
| } else { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Potential SQL‑injection: build the query with placeholders, not string concatenation
search is interpolated directly into the WHERE clause without escaping or parameter binding, so a crafted string containing ' ; DROP TABLE invoices; -- (or similar) would be executed verbatim by /v1/sql. Even if the CLN REST middleware sanitises input today, relying on that is brittle.
- let sql = format!(
- "SELECT label, bolt11, bolt12, payment_hash, amount_msat, status, \
- amount_received_msat, paid_at, payment_preimage, description, \
- local_offer_id, invreq_payer_note, expires_at FROM invoices"
- );
- let where_clause = if search.is_some() {
- format!(
- "WHERE description = '{}' or invreq_payer_note ='{}' or payment_hash = '{}' \
- ORDER BY created_index DESC LIMIT {}",
- search.clone().unwrap(),
- search.clone().unwrap(),
- search.clone().unwrap(),
- limit.unwrap_or(150),
- )
- } else {
- format!("ORDER BY created_index DESC LIMIT {}", limit.unwrap_or(150))
- };
+ // Safer: parameterised query – CLN supports SQLite‐style placeholders.
+ let sql = "SELECT label, bolt11, bolt12, payment_hash, amount_msat, status, \
+ amount_received_msat, paid_at, payment_preimage, description, \
+ local_offer_id, invreq_payer_note, expires_at \
+ FROM invoices \
+ WHERE (description = ?1 OR invreq_payer_note = ?1 OR payment_hash = ?1) \
+ ORDER BY created_index DESC LIMIT ?2";
+ let query_json = serde_json::json!({
+ "query": sql,
+ "params": [search.clone().unwrap_or_default(), limit.unwrap_or(150)]
+ });(You would then POST query_json instead of the current string.)
This removes the injection vector and simplifies the code by dropping the ad‑hoc sql/where_clause split.
Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
crates/lni/phoenixd/api.rs (1)
339-349:⚠️ Potential issue
serde_json::from_str(...).unwrap()may crash the node on malformed API outputNetwork calls are fallible; propagate the error instead of panicking.
- let incoming_payments: Vec<InvoiceResponse> = serde_json::from_str(&incoming_text).unwrap(); + let incoming_payments: Vec<InvoiceResponse> = serde_json::from_str(&incoming_text).map_err( + |e| ApiError::Json { + reason: format!("invalid incoming payments response: {e} – body: {incoming_text}"), + }, + )?;Apply the same guard to the outgoing branch below (lines 410-417).
♻️ Duplicate comments (1)
crates/lni/phoenixd/api.rs (1)
284-290:payment_hash.unwrap()can still panic – previous feedback not addressed
This is the exact issue highlighted in the earlier review:lookup_invoicemust gracefully handleNoneinstead of unwrapping.
🧹 Nitpick comments (3)
crates/lni/phoenixd/api.rs (3)
70-78: Aggregate balances across all channels instead of using.first()Using only the first element silently drops liquidity information from the remaining channels and will surface misleading node balances to callers.
- send_balance_msat: info.channels.first().map_or(0, |c| c.balance_sat * 1000), - receive_balance_msat: info.channels.first().map_or(0, |c| c.inbound_liquidity_sat * 1000), + send_balance_msat: info + .channels + .iter() + .map(|c| c.balance_sat * 1000) + .sum(), + receive_balance_msat: info + .channels + .iter() + .map(|c| c.inbound_liquidity_sat * 1000) + .sum(),
352-367: Search filter performs strict equality – consider partial / case-insensitive matchesUsers typically expect
searchto match substrings (e.g. part of memo) regardless of case.- let note_match = inc_payment - .payer_note - .as_ref() - .map_or(false, |note| note == search); + let note_match = inc_payment + .payer_note + .as_ref() + .map_or(false, |note| note.to_lowercase().contains(&search.to_lowercase()));Replicate the same logic for the outgoing branch (lines 426-435).
426-435: Duplicate filtering logic – extract helper to avoid divergenceIncoming & outgoing branches share near-identical filtering code. A small helper will reduce duplication and future bugs.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
crates/lni/phoenixd/api.rs(9 hunks)crates/lni/phoenixd/types.rs(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/lni/phoenixd/types.rs
crates/lni/phoenixd/api.rs
Outdated
| let req_url = format!("{}/createoffer", config.url); | ||
|
|
||
| let bolt12_req = Bolt12Req { | ||
| description: description.clone(), | ||
| amount_sat: amount_msats.unwrap_or_default() / 1000, | ||
| }; | ||
|
|
||
| let response = client | ||
| .post(&req_url) | ||
| .basic_auth("", Some(config.password.clone())) | ||
| .form(&bolt12_req) | ||
| .send() | ||
| .expect("Failed to create invoice"); | ||
|
|
||
| println!("Status: {}", response.status()); | ||
|
|
||
| let invoice_str = response.text().expect("Failed to parse get invoice"); | ||
| let invoice_str = invoice_str.as_str(); | ||
| println!("Bolt12 {}", &invoice_str.to_string()); | ||
|
|
||
|
|
||
| Ok(Transaction { | ||
| type_: "incoming".to_string(), | ||
| invoice: invoice_str.to_string(), | ||
| preimage: "".to_string(), | ||
| payment_hash: "".to_string(), | ||
| amount_msats: amount_msats.unwrap_or(0), | ||
| fees_paid: 0, | ||
| created_at: 0, | ||
| expires_at: expiry.unwrap_or(3600), | ||
| settled_at: 0, | ||
| description: description.unwrap_or_default(), | ||
| description_hash: description_hash.unwrap_or_default(), | ||
| payer_note: Some("".to_string()), | ||
| external_id: Some("".to_string()), | ||
| }) |
There was a problem hiding this comment.
create_invoice (BOLT-12) returns stub data – callers lose critical fields
payment_hash, preimage, and created_at are left blank, but downstream code (e.g. polling & UI layers) relies on them. The Phoenixd /createoffer endpoint returns a JSON payload—parse it instead of discarding it.
- let invoice_str = response.text().expect("Failed to parse get invoice");
- let invoice_str = invoice_str.as_str();
- println!("Bolt12 {}", &invoice_str.to_string());
-
- Ok(Transaction {
- type_: "incoming".to_string(),
- invoice: invoice_str.to_string(),
- preimage: "".to_string(),
- payment_hash: "".to_string(),
- ...
- })
+ let body = response.text().expect("Failed to parse createoffer response");
+ let offer_resp: serde_json::Value = serde_json::from_str(&body).map_err(|e| {
+ ApiError::Json {
+ reason: format!("invalid createoffer response: {e} – body: {body}"),
+ }
+ })?;
+
+ Ok(Transaction {
+ type_: "incoming".to_string(),
+ invoice: offer_resp["serialized"].as_str().unwrap_or_default().to_string(),
+ preimage: offer_resp["preimage"].as_str().unwrap_or_default().to_string(),
+ payment_hash: offer_resp["payment_hash"].as_str().unwrap_or_default().to_string(),
+ created_at: offer_resp["created_at"].as_i64().unwrap_or_default() / 1000,
+ ...
+ })📝 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.
| let req_url = format!("{}/createoffer", config.url); | |
| let bolt12_req = Bolt12Req { | |
| description: description.clone(), | |
| amount_sat: amount_msats.unwrap_or_default() / 1000, | |
| }; | |
| let response = client | |
| .post(&req_url) | |
| .basic_auth("", Some(config.password.clone())) | |
| .form(&bolt12_req) | |
| .send() | |
| .expect("Failed to create invoice"); | |
| println!("Status: {}", response.status()); | |
| let invoice_str = response.text().expect("Failed to parse get invoice"); | |
| let invoice_str = invoice_str.as_str(); | |
| println!("Bolt12 {}", &invoice_str.to_string()); | |
| Ok(Transaction { | |
| type_: "incoming".to_string(), | |
| invoice: invoice_str.to_string(), | |
| preimage: "".to_string(), | |
| payment_hash: "".to_string(), | |
| amount_msats: amount_msats.unwrap_or(0), | |
| fees_paid: 0, | |
| created_at: 0, | |
| expires_at: expiry.unwrap_or(3600), | |
| settled_at: 0, | |
| description: description.unwrap_or_default(), | |
| description_hash: description_hash.unwrap_or_default(), | |
| payer_note: Some("".to_string()), | |
| external_id: Some("".to_string()), | |
| }) | |
| let req_url = format!("{}/createoffer", config.url); | |
| let bolt12_req = Bolt12Req { | |
| description: description.clone(), | |
| amount_sat: amount_msats.unwrap_or_default() / 1000, | |
| }; | |
| let response = client | |
| .post(&req_url) | |
| .basic_auth("", Some(config.password.clone())) | |
| .form(&bolt12_req) | |
| .send() | |
| .expect("Failed to create invoice"); | |
| println!("Status: {}", response.status()); | |
| - let invoice_str = response.text().expect("Failed to parse get invoice"); | |
| - let invoice_str = invoice_str.as_str(); | |
| - println!("Bolt12 {}", &invoice_str.to_string()); | |
| - | |
| - Ok(Transaction { | |
| - type_: "incoming".to_string(), | |
| - invoice: invoice_str.to_string(), | |
| - preimage: "".to_string(), | |
| - payment_hash: "".to_string(), | |
| - amount_msats: amount_msats.unwrap_or(0), | |
| - fees_paid: 0, | |
| - created_at: 0, | |
| - expires_at: expiry.unwrap_or(3600), | |
| - settled_at: 0, | |
| - description: description.unwrap_or_default(), | |
| - description_hash: description_hash.unwrap_or_default(), | |
| - payer_note: Some("".to_string()), | |
| - external_id: Some("".to_string()), | |
| - }) | |
| + let body = response | |
| + .text() | |
| + .expect("Failed to parse createoffer response"); | |
| + let offer_resp: serde_json::Value = serde_json::from_str(&body).map_err(|e| { | |
| + ApiError::Json { | |
| + reason: format!("invalid createoffer response: {e} – body: {body}"), | |
| + } | |
| + })?; | |
| + | |
| + Ok(Transaction { | |
| + type_: "incoming".to_string(), | |
| + invoice: offer_resp["serialized"] | |
| + .as_str() | |
| + .unwrap_or_default() | |
| + .to_string(), | |
| + preimage: offer_resp["preimage"] | |
| + .as_str() | |
| + .unwrap_or_default() | |
| + .to_string(), | |
| + payment_hash: offer_resp["payment_hash"] | |
| + .as_str() | |
| + .unwrap_or_default() | |
| + .to_string(), | |
| + amount_msats: amount_msats.unwrap_or(0), | |
| + fees_paid: 0, | |
| + created_at: offer_resp["created_at"] | |
| + .as_i64() | |
| + .unwrap_or_default() | |
| + / 1000, | |
| + expires_at: expiry.unwrap_or(3600), | |
| + settled_at: 0, | |
| + description: description.unwrap_or_default(), | |
| + description_hash: description_hash.unwrap_or_default(), | |
| + payer_note: Some("".to_string()), | |
| + external_id: Some("".to_string()), | |
| + }) |
🤖 Prompt for AI Agents
In crates/lni/phoenixd/api.rs between lines 140 and 175, the create_invoice
function currently ignores the JSON response from the /createoffer endpoint and
returns stub values for payment_hash, preimage, and created_at, which are
required by downstream code. To fix this, parse the JSON response body into a
struct that includes these fields and populate the Transaction struct with the
actual values from the parsed response instead of using empty or default
placeholders.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
crates/lni/phoenixd/api.rs (2)
143-186:create_invoice(BOLT-12) still returns stub fields – callers lose critical dataThis exact issue was flagged in a previous review and remains unresolved:
preimage,payment_hash, andcreated_atare all hard-coded to empty / zero values even though the/createofferendpoint supplies them in JSON. Down-stream code relying on these fields will break.
295-301: Uncheckedunwrap()onpayment_hashcan panic; other params ignored
payment_hashis optional but immediately unwrapped – a nil input will crash the node. The same code path also discards the newfrom,limit, andsearchparameters.This has been pointed out before and still needs to be addressed.
🧹 Nitpick comments (2)
crates/lni/phoenixd/api.rs (2)
76-80: Balance only reflects the first channel – sum across all channels insteadUsing
channels.first()silently ignores additional channels and misreports total balances for nodes with >1 channel. Aggregating over the full vector is safer and just as cheap.- send_balance_msat: info.channels.first().map_or(0, |c| c.balance_sat * 1000), - receive_balance_msat: info - .channels - .first() - .map_or(0, |c| c.inbound_liquidity_sat * 1000), + send_balance_msat: info + .channels + .iter() + .map(|c| c.balance_sat as i64) + .sum::<i64>() * 1000, + receive_balance_msat: info + .channels + .iter() + .map(|c| c.inbound_liquidity_sat as i64) + .sum::<i64>() * 1000,
337-344: Sentinel value0forfrom/limitis brittle – make themOptionin the param structTreating
0as “not provided” leaks an implementation detail into the API surface and prevents legitimate queries that actually need the zero value. PreferOption<i64>inListTransactionsParamsand push each query param only whenSome.
| "error" => { | ||
| callback("failure".to_string(), transaction); | ||
| break; | ||
| // break; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Polling loop now spins forever on repeated errors
With the break commented out, any transient or permanent error will cause the loop to hammer the endpoint indefinitely, potentially spamming logs and exhausting resources. Consider:
- limiting the retry count / duration after successive failures
- adding exponential back-off
- reinstating a circuit-breaker once a terminal error is detected
- "error" => {
- callback("failure".to_string(), transaction);
- // break;
- }
+ "error" => {
+ callback("failure".to_string(), transaction);
+ error_count += 1;
+ if error_count >= MAX_RETRIES {
+ break;
+ }
+ thread::sleep(backoff.next_delay());
+ }Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In crates/lni/phoenixd/api.rs around lines 521 to 524, the polling loop
currently continues indefinitely on errors because the break statement is
commented out, causing potential resource exhaustion and log spamming. To fix
this, reinstate the break statement to stop the loop on terminal errors, and
implement a retry mechanism with a limited retry count or duration, add
exponential back-off between retries, and consider adding a circuit-breaker to
halt retries after detecting a terminal error.
Summary by CodeRabbit
InvoiceTypesymbol from the Node.js bindings.