-
Notifications
You must be signed in to change notification settings - Fork 0
Search #10
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
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
Note ⚡️ AI Code Reviews for VS Code, Cursor, WindsurfCodeRabbit now has a plugin for VS Code, Cursor and Windsurf. This brings AI code reviews directly in the code editor. Each commit is reviewed immediately, finding bugs before the PR is raised. Seamless context handoff to your AI code agent ensures that you can easily incorporate review feedback. Note ⚡️ Faster reviews with cachingCodeRabbit now supports caching for code and dependencies, helping speed up reviews. This means quicker feedback, reduced wait times, and a smoother review experience overall. Cached data is encrypted and stored securely. This feature will be automatically enabled for all accounts on May 16th. To opt out, configure 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (1)
🔇 Additional comments (6)
✨ 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 (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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_invoice
method was updated to pass the search parameter, thelist_transactions
method still passesNone
for 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
search
parameter to the LNDlist_transactions
URLThe
list_transactions
signature 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 whensearch
isSome
, 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
search
argument 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
break
removed on"error"
, a permanently failing lookup loops untilmax_polling_sec
expires, potentially spamming the backend everypolling_delay_sec
. Consider:
- counting consecutive errors and aborting early after N failures, or
- exponentially backing‑off, or
- reinstating the
break
when the HTTP client itself errors (network down).This saves bandwidth and avoids tying up threads.
crates/lni/cln/lib.rs (3)
86-94
: Passfrom
/limit
explicitly or expose them inLookupInvoiceParams
Hard‑coding
from = None, limit = None
means clients cannot page through historic invoices via this high‑level call. Either:
- expose
from
andlimit
inLookupInvoiceParams
, or- document that
lookup_invoice
always queries “latest / single” records.Minor, but clarifying prevents future misuse.
305-335
: Unit test duplicates same assertion; consider table‑driven style
test_lookup_invoice
repeats almost identical blocks forpayment_hash
andsearch
. 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
LookupInvoiceParams
struct, which is needed for the updated method signatures.
70-79
: Method signature updated to support search functionality.The
lookup_invoice
method 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
LookupInvoiceParams
struct, which is needed for the updated method signatures.
85-89
: Method signature updated to support search functionality.The
lookup_invoice
method 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_transactions
method 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
LookupInvoiceParams
struct, which is needed for the updated method signatures.
87-91
: Method signature updated to support search functionality.The
lookup_invoice
method 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_transactions
method 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
break
statement 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_hash
withsearch
parameter, aligning with the refactoring for search functionality across the codebase.
436-442
: Updated invoice lookup call with new parametersThe call to
lookup_invoice
has been updated to pass the new optional parameters, supporting the search capability in invoice polling.
459-461
: Changed polling behavior to continue on errorThe
break
statement 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_invoice
method 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
search
field toListTransactionsParams
enables filtering transactions by payer_note, memo, or description.
293-307
: Added new LookupInvoiceParams struct with Default implementationThe new
LookupInvoiceParams
struct provides a structured way to look up invoices by eitherpayment_hash
orsearch
term. The Default implementation appropriately initializes optional fields toNone
.
402-404
: Updated OnInvoiceEventParams for search supportThe
payment_hash
field is now optional, and a newsearch
field 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
LookupInvoiceParams
struct.
86-94
: Updated lookup_invoice implementation for structured parametersThe
lookup_invoice
implementation 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
search
parameter 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
search
field 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
LookupInvoiceParams
with the payment hash wrapped inSome
, using the Default implementation for other fields.
299-303
: Updated OnInvoiceEventParams usage in testThe test now wraps the
payment_hash
inSome
and 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
LookupInvoiceParams
struct.
81-89
: Updated lookup_invoice implementation for structured parametersThe method implementation now accepts
LookupInvoiceParams
and 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
search
parameter to the API call, enabling text-based filtering of transactions.
307-310
: Updated test for lookup_invoiceThe test now constructs a
LookupInvoiceParams
with the payment hash wrapped inSome
and 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
search
field 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_hash
and default values, aligning with the broader refactoring for search support.crates/lni/cln/api.rs (1)
612-618
: Ambiguous precedence when bothpayment_hash
andsearch
are supplied
pay_hash
preferssearch
overpayment_hash
; callers providing both may be surprised that the explicitpayment_hash
is 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 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=rust
Length 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ 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.
Summary by CodeRabbit
InvoiceType
symbol from the Node.js bindings.