Skip to content

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

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Search #10

wants to merge 4 commits into from

Conversation

ntheile
Copy link
Contributor

@ntheile ntheile commented Apr 22, 2025

Summary by CodeRabbit

  • New Features
    • Added support for searching invoices and transactions using flexible search terms, such as payer note, memo, or description, in addition to payment hash.
    • Enhanced invoice event subscriptions to allow filtering by search queries.
  • Bug Fixes
    • Improved error handling in invoice event polling to ensure continued operation after encountering errors.
  • Documentation
    • Updated parameter structures and method signatures to reflect new search capabilities.
  • Chores
    • Removed detailed TypeScript interfaces and enums from public exports, simplifying the exposed API surface.
    • Removed export of the InvoiceType symbol from the Node.js bindings.

Copy link

coderabbitai bot commented Apr 22, 2025

Walkthrough

This 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 (LookupInvoiceParams, updated ListTransactionsParams) and propagate these through all relevant methods, trait signatures, and API layers for CLN, LND, and Phoenixd backends. SQL-based searching is implemented for CLN, and all backends now support searching invoices and transactions by description, memo, or payer note. Test suites and error handling are updated to reflect the new interfaces and behaviors. Additionally, polling loops no longer break immediately on error events, allowing continued operation.

Changes

File(s) Change Summary
crates/lni/types.rs Introduced LookupInvoiceParams struct with optional payment_hash and search; added search to ListTransactionsParams; made payment_hash optional and added search to OnInvoiceEventParams; updated LightningNode trait signatures.
crates/lni/cln/api.rs, crates/lni/lnd/api.rs, crates/lni/phoenixd/api.rs Extended lookup_invoice and list_transactions functions to accept optional search parameter; CLN implementation adds SQL-based searching; polling functions updated to pass through search; commented out break on error in polling loops.
crates/lni/cln/lib.rs, crates/lni/lnd/lib.rs, crates/lni/phoenixd/lib.rs Updated LightningNode implementations to use new parameter structs for invoice lookup and transaction listing; updated test suites to use new interfaces and add debug output.
bindings/lni_nodejs/src/cln.rs, bindings/lni_nodejs/src/lnd.rs, bindings/lni_nodejs/src/phoenixd.rs Updated Node.js bindings to accept LookupInvoiceParams for invoice lookup and pass search to transaction listing; updated import statements.
crates/lni/phoenixd/types.rs Changed OutgoingPaymentResponse fields preimage and payment_hash from String to Option<String> to allow optional values.
bindings/lni_nodejs/index.d.ts, bindings/lni_nodejs/index.js Removed all exported TypeScript interfaces and enums except the three node classes; updated lookupInvoice method signatures to accept LookupInvoiceParams; removed export of InvoiceType from JS bindings.

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)
Loading

Possibly related PRs

  • core lightning #4: Adds the ClnNode class and its methods including lookupInvoice and listTransactions with updated parameter interfaces, directly related to the changes in this PR.
  • On Invoice Events #9: Introduces asynchronous polling and callback mechanisms for invoice events; related to invoice handling but focuses on event-driven features rather than parameter and search enhancements.

Poem

In the warren of code where the queries run deep,
Now rabbits can search, not just burrow or leap.
With hashes or words, find invoices galore—
Digging through memos, descriptions, and more!
Structured and nimble, our search is a breeze,
These carrots of progress will surely appease.
🥕✨

Note

⚡️ AI Code Reviews for VS Code, Cursor, Windsurf

CodeRabbit 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.
Learn more here.


Note

⚡️ Faster reviews with caching

CodeRabbit 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 Review - Disable Cache at either the organization or repository level. If you prefer to disable all data retention across your organization, simply turn off the Data Retention setting under your Organization Settings.
Enjoy the performance boost—your workflow just got faster.


📜 Recent review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting

📥 Commits

Reviewing files that changed from the base of the PR and between afab5b1 and b85ab4c.

📒 Files selected for processing (3)
  • bindings/lni_nodejs/index.d.ts (3 hunks)
  • bindings/lni_nodejs/index.js (1 hunks)
  • crates/lni/phoenixd/api.rs (8 hunks)
✅ Files skipped from review due to trivial changes (1)
  • bindings/lni_nodejs/index.js
🔇 Additional comments (6)
bindings/lni_nodejs/index.d.ts (1)

15-15: API signature update to support structured params

The lookupInvoice method signatures in all three node classes have been updated to accept a params parameter of type LookupInvoiceParams instead of a single paymentHash string. This is consistent with the broader refactoring to support more flexible invoice lookups with optional search capabilities.

Note that this is a breaking change that will require client code to be updated. If you're releasing this in a public API, make sure to document this change clearly in release notes.

Also applies to: 31-31, 47-47

crates/lni/phoenixd/api.rs (5)

76-77: Safer access to channel balances

Good improvement! Using first() and map_or instead of direct indexing prevents potential panics if the channels array is empty.


319-341: Improved filtering logic for transactions

The refactored filtering logic properly handles both payment_hash and search parameters. The early continue statements for non-matching transactions improve efficiency.


392-408: Consistent filtering for outgoing transactions

The filtering logic for outgoing transactions matches the approach used for incoming transactions, providing consistent behavior across transaction types.


448-456: Updated polling implementation

The polling implementation now properly constructs a ListTransactionsParams object with all necessary fields, including the new search parameter.


479-479: Changed error handling behavior in polling loop

Commenting out the break statement on error allows the polling loop to continue instead of terminating immediately. This makes the system more resilient to temporary failures.

Make sure that this behavior is desired and consistent with other implementations. While this improves resilience, it could also potentially mask serious errors or lead to prolonged polling when there are persistent issues.

✨ Finishing Touches
  • 📝 Generate Docstrings

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.

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Explain this complex logic.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai explain this code block.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and explain its main purpose.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Support

Need 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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR.
  • @coderabbitai generate sequence diagram to generate a sequence diagram of the changes in this PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link

@coderabbitai coderabbitai bot left a 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 issue

Missing search parameter in list_transactions.

While the lookup_invoice method was updated to pass the search parameter, the list_transactions method still passes None for the search parameter instead of params.search. This is inconsistent with the implementations in other backend files like lnd.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.rs

Length of output: 881


Add the search parameter to the LND list_transactions URL

The list_transactions signature now accepts search: Option<String>, but the URL built in crates/lni/lnd/api.rs (around lines 408–411) never uses it. Update the URL builder so that when search is Some, its value is appended to the query string.

Locations to update:
- crates/lni/lnd/api.rs, lines 408–411

Suggested 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: Remove dbg!/println! noise or gate it behind log / tracing

dbg!(...) and plain println! 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 unreachable

With the break removed on "error", a permanently failing lookup loops until max_polling_sec expires, potentially spamming the backend every polling_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: Pass from/limit explicitly or expose them in LookupInvoiceParams

Hard‑coding from = None, limit = None means clients cannot page through historic invoices via this high‑level call. Either:

  1. expose from and limit in LookupInvoiceParams, or
  2. 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 for payment_hash and search. 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 leaving dbg! 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

📥 Commits

Reviewing files that changed from the base of the PR and between e9e891e and 79f298f.

📒 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 support

The 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 support

The function signature has been updated to replace payment_hash with search parameter, aligning with the refactoring for search functionality across the codebase.


436-442: Updated invoice lookup call with new parameters

The 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 error

The 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 lookup

The 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 ListTransactionsParams

The addition of the search field to ListTransactionsParams enables filtering transactions by payer_note, memo, or description.


293-307: Added new LookupInvoiceParams struct with Default implementation

The new LookupInvoiceParams struct provides a structured way to look up invoices by either payment_hash or search term. The Default implementation appropriately initializes optional fields to None.


402-404: Updated OnInvoiceEventParams for search support

The payment_hash field is now optional, and a new search field has been added, providing more flexibility in how invoices are tracked for events.


407-416: Added Default implementation for OnInvoiceEventParams

The 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 struct

The import has been updated to include the new LookupInvoiceParams struct.


86-94: Updated lookup_invoice implementation for structured parameters

The 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 support

The method now forwards the optional search parameter to the API call, enabling text-based filtering of transactions.


253-254: Updated test case for search parameter

The test has been updated to include the new search field in ListTransactionsParams, maintaining compatibility with the updated method signature.


257-258: Improved debugging in tests

Replaced println! with dbg! 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_invoice

The test now constructs a LookupInvoiceParams with the payment hash wrapped in Some, using the Default implementation for other fields.


299-303: Updated OnInvoiceEventParams usage in test

The test now wraps the payment_hash in Some 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 structs

The imports have been updated to include the new LookupInvoiceParams struct.


81-89: Updated lookup_invoice implementation for structured parameters

The 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 support

The implementation now forwards the search parameter to the API call, enabling text-based filtering of transactions.


307-310: Updated test for lookup_invoice

The test now constructs a LookupInvoiceParams with the payment hash wrapped in Some and uses the Default implementation for other fields.


312-312: Improved debugging in tests

Replaced println! with dbg! 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 parameter

The test has been updated to include the new search field in ListTransactionsParams, maintaining compatibility with the updated method signature.


390-394: Updated OnInvoiceEventParams usage in test

The 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 both payment_hash and search are supplied

pay_hash prefers search over payment_hash; callers providing both may be surprised that the explicit payment_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(),
    });
}

Comment on lines 342 to 348
pub fn lookup_invoice(
config: &LndConfig,
payment_hash: Option<String>,
from: Option<i64>,
limit: Option<i64>,
search: Option<String>,
) -> Result<Transaction, ApiError> {
Copy link

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());
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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(),
+     });
+ }

Comment on lines +531 to +600
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);
}
}
Copy link

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.

Comment on lines +501 to +514
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 {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

⚠️ Potential issue

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant