Skip to content

lnd #5

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

Merged
merged 5 commits into from
Mar 24, 2025
Merged

lnd #5

merged 5 commits into from
Mar 24, 2025

Conversation

ntheile
Copy link
Contributor

@ntheile ntheile commented Mar 21, 2025

Summary by CodeRabbit

  • New Features

    • Introduced native LND Node integration to enable advanced Lightning Network functionalities like node information retrieval, invoice creation, offer management, and transaction listing.
    • Added a new method pay_invoice to facilitate invoice payments across various nodes.
    • Extended invoice options now support additional flags for more flexible payment processing.
    • Introduced a new response structure for payment invoices, enhancing the handling of payment details.
  • Documentation

    • Updated configuration references and documentation details to reflect the new LND features.
  • Enhancements

    • Streamlined invoice lookup and error handling for improved reliability and performance.

Copy link

coderabbitai bot commented Mar 21, 2025

Caution

Review failed

The pull request is closed.

Walkthrough

The update integrates native support for LND nodes across multiple layers. New interfaces and classes for LND configuration and operations (such as getting node info, creating invoices, handling offers, and decoding invoices) have been introduced in TypeScript, JavaScript, and Rust. The changes streamline existing CLN node functions and update configuration, dependencies, and documentation. Additionally, bindings for React Native and UniFFI now expose the LND APIs, reinforcing a consistent interface for Lightning node interactions.

Changes

Files Change Summary
bindings/lni_nodejs/(index.d.ts, index.js, main.mjs) Added new interfaces (LndConfig, LndNode), updated CreateInvoiceParams, exported LndNode, and introduced a new lnd() function integrating LND functionality.
bindings/lni_nodejs/src/(lnd.rs, lib.rs) Introduced a new LND module; defined the LndNode struct with methods for LND operations.
bindings/lni_nodejs/src/cln.rs Simplified the lookup_invoice method by removing iterator-based error handling.
bindings/lni_uniffi/(src/lib.rs, src/lni.udl) Exported the LND module and defined corresponding dictionaries/interfaces for LND configuration and operations.
crates/lni/(lib.rs, lnd/api.rs, lnd/lib.rs, lnd/types.rs) Added a new LND module with sub-modules for API functions, core implementation, and associated type definitions and exported LndConfig and LndNode.
crates/lni/(cln/api.rs, cln/lib.rs, phoenixd/lib.rs, types.rs) Modified CLN node functions for invoice lookup and transaction listing; updated CreateInvoiceParams with additional optional fields and added the Clone trait to Transaction.
Config & Docs (bindings/lni_react_native/ubrn.config.yaml, readme.md, Cargo.toml) Updated the configuration branch from cln to lnd, revised documentation to reflect completed LND implementation, and added dependencies (hex, sha2).

Sequence Diagram(s)

sequenceDiagram
    participant App as Application (main)
    participant LN as LndNode
    participant API as LND API (Rust)

    App->>LN: Instantiate LndNode(config)
    LN->>API: get_info()
    API-->>LN: Return node info
    LN-->>App: Deliver node info

    App->>LN: create_invoice(params)
    LN->>API: create_invoice(params)
    API-->>LN: Invoice created
    LN-->>App: Deliver invoice

    App->>LN: lookup_invoice(payment_hash)
    LN->>API: lookup_invoice(payment_hash)
    API-->>LN: Return transaction
    LN-->>App: Deliver transaction
Loading

Poem

I'm a little rabbit with coding flair,
Hopping through bytes in electric air.
LND and CLN now dance as one,
New invoices and offers, oh what fun!
I nibble on changes with a twitch of my nose,
Celebrating fresh code where the lightning flows!
🐇✨


📜 Recent review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 2385f5f and 32ccbf4.

📒 Files selected for processing (3)
  • crates/lni/cln/api.rs (4 hunks)
  • crates/lni/phoenixd/api.rs (3 hunks)
  • crates/lni/utils.rs (1 hunks)
✨ Finishing Touches
  • 📝 Generate Docstrings

🪧 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.
    • Generate unit testing code for this file.
    • 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 generate unit testing code for this file.
    • @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 generate unit testing code.
    • @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.

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 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: 9

🧹 Nitpick comments (34)
readme.md (2)

269-269: Fix the markdown linting issue with the bare URL.

The bare URL for Tor Socks5 fetch doesn't follow proper markdown formatting guidelines.

-- [ ] Tor Socks5 fetch https://tpo.pages.torproject.net/core/arti/guides/starting-arti
+- [ ] Tor Socks5 fetch [Arti Guide](https://tpo.pages.torproject.net/core/arti/guides/starting-arti)
🧰 Tools
🪛 markdownlint-cli2 (0.17.2)

269-269: Bare URL used
null

(MD034, no-bare-urls)


271-274: Fix indentation for list items.

The list items are indented with 4 spaces, but the markdown linter expects 2 spaces for proper nesting.

 - [ ] implement lightning nodes
-    - [X] phoenixd
-    - [X] cln
-    - [X] lnd
+  - [X] phoenixd
+  - [X] cln
+  - [X] lnd

Great job completing the LND implementation! This matches perfectly with the code changes in the other files.

🧰 Tools
🪛 markdownlint-cli2 (0.17.2)

271-271: Unordered list indentation
Expected: 2; Actual: 4

(MD007, ul-indent)


272-272: Unordered list indentation
Expected: 2; Actual: 4

(MD007, ul-indent)


273-273: Unordered list indentation
Expected: 2; Actual: 4

(MD007, ul-indent)


274-274: Unordered list indentation
Expected: 2; Actual: 4

(MD007, ul-indent)

bindings/lni_nodejs/index.d.ts (4)

27-30: Consider secure handling of LND macaroon.
Storing macaroons as plain strings can be risky if they are sensitive credentials. Consider ensuring that these fields are handled securely (e.g., encrypted at rest, masked in logs) to prevent inadvertent leaks.


31-34: Avoid repeating configuration fields in the LndNode interface.
Since LndConfig already has url and macaroon, including them again in LndNode may introduce duplication. Consider relying upon an internal config object or using a getter to avoid scattered references.


178-182: Add doc comments for the new optional invoice parameters.
It would be helpful to document what rPreimage, isBlinded, isKeysend, isAmp, and isPrivate do for improved clarity and maintainability.


226-239: Unify node classes under a common baseline interface.
PhoenixdNode, ClnNode, and LndNode largely share similar methods. Consider extracting a shared interface or abstract class to streamline code maintenance.

crates/lni/cln/api.rs (1)

350-356: Consider a single parameter struct for paging fields.
payment_hash, from, and limit are commonly passed around together. You could streamline the code by encapsulating these into a single struct (e.g., LookupParams) to reduce repetition and improve clarity.

bindings/lni_uniffi/src/lni.udl (3)

65-68: Handle the LND macaroon securely.
Similar to the TypeScript interface, take care to handle sensitive macaroons safely (avoid logging them, consider encryption in storage, etc.).


70-97: Consider a shared approach for repeating node interfaces.
LndNode appears structurally similar to PhoenixdNode and ClnNode. Extracting a common interface or base type might reduce duplication across node implementations.


143-147: Add descriptive comments for new CreateInvoiceParams fields.
r_preimage, is_blinded, is_keysend, is_amp, and is_private would benefit from short descriptive statements on their purpose.

crates/lni/lnd/types.rs (6)

1-3: Consider deriving Serialize as well
Currently, you only derive Deserialize. If you plan to send these structures back to clients or log them in JSON, deriving Serialize can be beneficial.


4-8: Use a typed enum for the network if possible
The Chain struct stores chain and network as plain strings. If the set of valid networks is known (e.g., mainnet, testnet, regtest), consider using a typed enum to reduce errors and improve clarity.


34-37: FetchInvoiceResponse rename consideration
invoice could be renamed to a more descriptive field, like encoded_invoice or bolt11, to distinguish it from other invoice fields in the codebase.


39-49: Check created_at type in PayResponse
created_at is a f64, which might be prone to floating-point inaccuracies when handling timestamps. Consider using an integer type (e.g., i64) or a specialized time library if precision is important.


83-99: Clarify meaning of created in Bolt12Resp
With fields like active, single_use, and used, the boolean created could be confusing. Consider a more descriptive name like is_newly_created or adding doc comments.


101-149: Ensure proper numeric parsing in ListInvoiceResponse
Many fields (e.g., value, value_msat, creation_date, settle_date, etc.) are Option<String>. Make sure any conversions to numeric types (e.g., i64) are sufficiently validated. This helps avoid runtime panics from invalid strings.

Do you want me to provide a helper or code snippet for safely parsing these fields using a library like serde_with or custom deserializers?

crates/lni/lnd/lib.rs (5)

9-19: Consider doc comments for LndConfig
Adding doc comments to fields url and macaroon clarifies their intended usage and valid formats. This is especially helpful for new team members or external consumers unfamiliar with LND.


21-27: Validate LndNode::new parameters
If LndConfig allows empty or invalid url and macaroon, consider validating or returning an error if they don't match expected patterns.


33-38: Review concurrency approach in create_invoice
Using .await ensures the call is asynchronous. Verify that concurrent invoice creation is safe on the LND side, especially if multiple invoices are created rapidly.


75-85: Paginated results in list_transactions
from and limit are used for pagination. Validate that subsequent or large queries won’t silently omit transactions if LND returns partial data or changes the indexing rules.

Can provide a verification script if you want to confirm the offset logic with various test scenarios.


92-316: Improve test coverage
Your test module covers get_info, create_invoice, lookup_invoice, list_transactions, and decode. Consider also testing error scenarios (e.g., invalid macaroon, unreachable URL) to ensure robust error handling.

bindings/lni_nodejs/src/lnd.rs (7)

4-7: Add doc comments to the LndNode struct
Because this struct is exposed to Node.js developers via N-API, clarifying usage and constraints in doc comments helps consumers use it more effectively.


11-14: Constructor input validation
new(config: LndConfig) trusts that config is valid. If you anticipate malicious or invalid input from external calls, consider validating config.url and config.macaroon.


16-19: Consider customizing get_url
get_url trivially returns the stored URL. If there's any requirement to sanitize or mask sensitive parts, do it here before exposing it to consumers.


41-55: create_invoice Node API usage
This function is asynchronous and returns a napi::Result. Confirm that Node.js consumers handle potential rejections appropriately, especially if the system cannot reach the LND or if invoice creation fails.


65-72: list_offers usage
Like get_offer, this might return partial or unimplemented data from LND if Bolt12 is incomplete. Decide whether you prefer to gate these calls behind a feature flag for clarity.


104-117: list_transactions filtering
This function does not accept payment_hash here (unlike the Rust method signature in the crate). If you need payment-hash filtering, ensure the TypeScript definitions match.


119-125: Clarify usage of decode
Method name is generic. If it’s intended only for BOLT 11 or BOLT 12 strings, consider naming it decode_payment_request for clarity.

crates/lni/lnd/api.rs (6)

30-47: Prevent potential runtime panics by handling errors explicitly.

Calls to send().unwrap() and text().unwrap() will panic on network or parsing errors. Return an appropriate ApiError instead.

- let response = client.get(&req_url).send().unwrap();
- let response_text = response.text().unwrap();
+ let response = client.get(&req_url).send().map_err(|e| ApiError::Network {
+     reason: e.to_string(),
+ })?;
+ let response_text = response.text().map_err(|e| ApiError::Network {
+     reason: e.to_string(),
+ })?;

76-79: Avoid printing debug statements to stdout in production code.

Use a structured logger (e.g., log crate) or remove these println! calls to prevent cluttering output logs.

- println!("Status: {}", response.status());
- println!("Bolt11 {}", &invoice_str.to_string());
+ // log::debug!("Status: {}", response.status());
+ // log::debug!("Bolt11 {}", &invoice_str);

109-118: Use an async client or handle blocking appropriately.

Similar to create_invoice(), this function is async but uses blocking reqwest calls. Consider using the async client or removing async.


120-129: Implement or remove stubbed Bolt12 functions.

All these functions currently return an error stating Bolt12 is not implemented. If you plan to support Bolt12, consider outlining a roadmap or removing them for now to avoid confusion.

Do you want help prototyping these Bolt12 methods?

Also applies to: 131-139, 141-151, 153-163, 165-175


177-233: Handle potential parsing or network failures without panicking.

Calls such as send().unwrap(), text().unwrap(), and serde_json::from_str(...).unwrap() can panic. Map these errors to the ApiError instead of unwrapping.


235-298: Refactor to safely handle JSON deserialization failures.

Parsing invoice fields using .unwrap_or_default() on parsed integers can mask errors. Consider logging or returning relevant error information to troubleshoot malformed responses or unexpected data.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 6fa252d and 14a18e6.

📒 Files selected for processing (19)
  • bindings/lni_nodejs/index.d.ts (3 hunks)
  • bindings/lni_nodejs/index.js (1 hunks)
  • bindings/lni_nodejs/main.mjs (2 hunks)
  • bindings/lni_nodejs/src/cln.rs (1 hunks)
  • bindings/lni_nodejs/src/lib.rs (1 hunks)
  • bindings/lni_nodejs/src/lnd.rs (1 hunks)
  • bindings/lni_react_native/ubrn.config.yaml (1 hunks)
  • bindings/lni_uniffi/src/lib.rs (1 hunks)
  • bindings/lni_uniffi/src/lni.udl (2 hunks)
  • crates/lni/Cargo.toml (1 hunks)
  • crates/lni/cln/api.rs (2 hunks)
  • crates/lni/cln/lib.rs (2 hunks)
  • crates/lni/lib.rs (1 hunks)
  • crates/lni/lnd/api.rs (1 hunks)
  • crates/lni/lnd/lib.rs (1 hunks)
  • crates/lni/lnd/types.rs (1 hunks)
  • crates/lni/phoenixd/lib.rs (1 hunks)
  • crates/lni/types.rs (3 hunks)
  • readme.md (1 hunks)
🧰 Additional context used
🧬 Code Definitions (8)
bindings/lni_nodejs/index.js (1)
bindings/lni_nodejs/index.d.ts (1) (1)
  • LndNode (31-34)
crates/lni/phoenixd/lib.rs (1)
crates/lni/types.rs (1) (1)
  • default (228-242)
bindings/lni_nodejs/src/lib.rs (1)
bindings/lni_nodejs/index.d.ts (1) (1)
  • LndNode (31-34)
crates/lni/lib.rs (1)
bindings/lni_nodejs/index.d.ts (2) (2)
  • LndConfig (27-30)
  • LndNode (31-34)
crates/lni/lnd/types.rs (1)
bindings/lni_nodejs/index.d.ts (2) (2)
  • PayCode (184-191)
  • Bolt11Resp (14-18)
crates/lni/lnd/api.rs (1)
bindings/lni_nodejs/index.d.ts (5) (5)
  • Bolt11Resp (14-18)
  • NodeInfo (43-50)
  • CreateInvoiceParams (171-183)
  • PayCode (184-191)
  • PayInvoiceResponse (150-154)
crates/lni/cln/lib.rs (3)
crates/lni/cln/api.rs (2) (2)
  • lookup_invoice (337-348)
  • list_transactions (418-428)
crates/lni/phoenixd/api.rs (2) (2)
  • lookup_invoice (165-193)
  • list_transactions (195-324)
bindings/lni_nodejs/index.d.ts (2) (2)
  • ListTransactionsParams (166-170)
  • Transaction (51-65)
crates/lni/lnd/lib.rs (3)
bindings/lni_nodejs/index.d.ts (5) (5)
  • NodeInfo (43-50)
  • CreateInvoiceParams (171-183)
  • ListTransactionsParams (166-170)
  • PayCode (184-191)
  • Transaction (51-65)
bindings/lni_nodejs/src/lnd.rs (6) (6)
  • get_info (35-39)
  • create_invoice (42-55)
  • get_offer (58-63)
  • lookup_invoice (94-102)
  • list_transactions (105-117)
  • decode (120-125)
crates/lni/types.rs (1) (1)
  • default (228-242)
🪛 markdownlint-cli2 (0.17.2)
readme.md

269-269: Bare URL used
null

(MD034, no-bare-urls)


271-271: Unordered list indentation
Expected: 2; Actual: 4

(MD007, ul-indent)


272-272: Unordered list indentation
Expected: 2; Actual: 4

(MD007, ul-indent)


273-273: Unordered list indentation
Expected: 2; Actual: 4

(MD007, ul-indent)

🔇 Additional comments (30)
bindings/lni_react_native/ubrn.config.yaml (1)

4-4: Branch update aligns with LND integration focus

The change from cln to lnd branch correctly updates the configuration to target the Lightning Network Daemon implementation branch instead of Core Lightning.

crates/lni/Cargo.toml (1)

29-30: Appropriate cryptographic dependencies added for LND integration

The addition of hex and sha2 crates is necessary for handling cryptographic operations commonly used in Lightning Network implementations, such as encoding/decoding hexadecimal data and computing SHA-2 hashes for payment verification.

bindings/lni_nodejs/index.js (2)

313-313: LndNode class correctly added to native binding imports

The LndNode class is now properly extracted from the native binding, consistent with the other node types (PhoenixdNode, ClnNode).


318-318: LndNode properly exported from module

The LndNode class is now correctly exported, making it available for use by consumers of this module.

bindings/lni_uniffi/src/lib.rs (1)

4-4: LND module properly exposed through UniFFI

The addition of pub use lni::lnd::*; correctly exposes all public items from the LND module, following the same pattern used for other Lightning implementations (phoenixd, cln).

bindings/lni_nodejs/src/lib.rs (1)

14-16: Successfully added LND module.

The Lightning Network Daemon (LND) node implementation has been correctly added to the library exports, following the same pattern as the existing PhoenixdNode and ClnNode.

crates/lni/phoenixd/lib.rs (1)

150-150: Good practice using Default trait.

Using ..Default::default() is a clean way to handle the additional parameters that were added to the CreateInvoiceParams struct. This ensures that the test code won't break when new fields are added to the struct in the future.

crates/lni/lib.rs (1)

37-42: Correctly implemented LND module structure.

The LND module follows the same structure as the existing modules (phoenixd, cln) with appropriate submodules for lib, api, and types. This consistent architecture makes the codebase more maintainable and easier to understand.

bindings/lni_nodejs/main.mjs (2)

1-1: Clean import update to include LndNode.

The import has been updated to include LndNode along with the existing node types, maintaining a consistent pattern.


132-132: Main function properly updated to include the LND node.

The LND integration is correctly enabled in the main function, ensuring it runs alongside the existing node implementations.

bindings/lni_nodejs/src/cln.rs (1)

107-107:

✅ Verification successful

Code simplification in lookup_invoice.

The implementation has been simplified to directly return the transaction result from the API call. This eliminates unnecessary processing and aligns with the pattern seen in the LND implementation.

Let's verify that error handling is still robust by checking if any error cases are still properly handled:


🏁 Script executed:

#!/bin/bash
# Check if the error handling has been moved to the API level
grep -A 10 "lookup_invoice" crates/lni/cln/api.rs | grep -A 5 "Result<"

Length of output: 479


Error handling verification in lookup_invoice is robust

The simplified implementation now directly returns the transaction result from the API call, and error conditions are appropriately handled at the API level. The verification confirms that any errors from lookup_invoices are correctly propagated using Err(e), which safeguards against unhandled error cases.

crates/lni/types.rs (3)

32-32: Added Clone trait to Transaction struct.

Adding the Clone trait to the Transaction struct allows creating copies of transactions, which is necessary for operations like returning transactions from lookup functions.


220-225: Added LND-specific invoice parameters.

The additional fields support LND-specific features:

  • r_preimage: Optional payment preimage for verification
  • is_blinded: For blinded payments hiding routing information
  • is_keysend: For keysend payments (without requiring invoice)
  • is_amp: For Atomic Multi-path Payments
  • is_private: For private invoices not broadcasted to the network

These provide the necessary flexibility for working with LND while maintaining compatibility with other implementations.


236-240: Default values for new invoice parameters.

The default implementation correctly initializes the new fields with sensible defaults:

  • r_preimage is None since it's typically generated
  • Boolean flags are Some(false) rather than None, making the intent explicit
crates/lni/cln/lib.rs (3)

91-96: Updated list_transactions to use correct API.

The implementation now correctly calls list_transactions API with direct parameters rather than wrapping them in Some(). This is consistent with the API definition shown in the relevant code snippets.


213-213: Updated Default usage in tests.

The test correctly uses ..Default::default() to leverage the default values for the new invoice parameters, ensuring the tests remain compatible with the updated struct.


78-84:

✅ Verification successful

Simplified lookup_invoice implementation.

The lookup_invoice method has been simplified to directly return the API result. This aligns with the pattern seen in the LND implementation and reduces complexity.

Looking at the relevant code snippets, the error handling appears to have been moved to the API layer. Let's verify:


🏁 Script executed:

#!/bin/bash
# Look for error handling in the updated lookup_invoice API function
grep -A 10 "pub fn lookup_invoice" crates/lni/cln/api.rs

Length of output: 403


Simplified lookup_invoice Implementation Verified

  • The lookup_invoice function in crates/lni/cln/lib.rs (lines 78-84) now directly returns the result from the API call.
  • Error handling has been effectively moved into the API layer. As seen in crates/lni/cln/api.rs, the function properly matches on the result of lookup_invoices, returning an error when necessary.
  • This approach aligns well with patterns used in the LND implementation and reduces code complexity.
crates/lni/cln/api.rs (1)

424-424:

❓ Verification inconclusive

Verify behavior for negative from or limit parameters.
Since from and limit are i64, ensure the service can handle negative values safely or add validation to fail early.

Run this script to locate parameter references and confirm appropriate validation or usage patterns:


🏁 Script executed:

#!/bin/bash
# Searches for references of 'list_transactions' to confirm negative param handling.
rg 'list_transactions'

Length of output: 1753


🏁 Script executed:

#!/bin/bash
# Searching for the definition of lookup_invoices to inspect its handling of parameters.
rg "fn lookup_invoices" -n

Length of output: 74


Validation Check for Negative from and limit Parameters
Please verify that negative values for from and limit are either safely handled within lookup_invoices (defined in crates/lni/cln/api.rs around line 351) or trigger an early validation error. Cross-check this behavior against similar endpoints (like list_transactions) to ensure consistency and avoid unintended behavior with pagination or related queries.

crates/lni/lnd/types.rs (2)

10-32: Confirm that all fields in GetInfoResponse exist across LND versions
Some older or customized LND versions may not provide fields such as require_htlc_interceptor or store_final_htlc_resolutions. If using varied LND backends, consider making these fields optional to gracefully handle missing fields.


51-55: Validate TXID endianness in PaidOutpoint
LND typically returns TXIDs in big-endian hexadecimal. Confirm that the txid field is interpreted correctly, as different APIs can invert endianness.

crates/lni/lnd/lib.rs (5)

1-8: Check feature gating
You conditionally compile with #[cfg(feature = "napi_rs")] in some places. Verify whether you need consistent gating for other items (e.g., use napi_derive::napi). Inconsistent gating can cause build issues if features differ.


29-31: Check potential network errors in get_info
Make sure you handle potential timeouts, connection issues, or 4xx/5xx HTTP responses when calling the LND. Currently, the call is forwarded to the underlying API.


40-46: get_offer and list_offers disclaimers
Given that Bolt12 is not fully implemented in many LND versions, confirm whether these calls will always return “not implemented” or partial data.


64-73: lookup_invoice error handling
Invoices not found raise an error with "not found". Confirm that the presence of an invoice with partial data or unusual statuses (e.g., “canceled”) is also handled.


87-89: Confirm decode usage
decode is often used for BOLT 11 payment requests. If used with BOLT 12 or custom strings, confirm that an appropriate error or fallback is provided.

bindings/lni_nodejs/src/lnd.rs (4)

1-3: Validate cross-crate references
You import LndConfig from lni::lnd::lib::LndConfig. Confirm that version mismatches or reorganization won't break these references, particularly when the crates evolve.


34-39: Check concurrency for get_info
While .map_err(...) wraps errors, ensure that concurrency is safe if multiple Node.js threads use this method simultaneously.


57-63: Confirm partial Bolt12 support
get_offer references Bolt12, which many LND builds might not fully support. The error path returns “Bolt12 not implemented” in some placeholders. Ensure docs note that limitation.

Likely an incorrect or invalid review comment.


93-102: lookup_invoice return validation
Ensure that the resulting Transaction object is consistent with the Node.js type definitions. Check for mismatches in field naming or type conversions.

crates/lni/lnd/api.rs (1)

85-99: Check for discrepancies in the Bolt11 response structure.

According to the related TypeScript snippet, Bolt11Resp may differ from the fields (payment_request and r_hash) used here. Ensure the local Bolt11Resp struct is up to date.

Likely an incorrect or invalid review comment.

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: 9

🔭 Outside diff range comments (1)
crates/lni/lnd/types.rs (1)

1-223: 🛠️ Refactor suggestion

Add documentation for structs and fields

The file lacks documentation comments for structs and their fields. Adding documentation would improve code readability and maintainability, especially since this is a public API.

Consider adding:

  1. Doc comments (///) for each struct explaining its purpose
  2. Doc comments for fields, especially those with special formats or constraints
  3. Examples where appropriate

Example:

+/// Represents a blockchain with chain name and network information
 #[derive(Debug, Deserialize)]
 pub struct Chain {
+    /// The name of the blockchain (e.g., "bitcoin")
     pub chain: String,
+    /// The network type (e.g., "mainnet", "testnet")
     pub network: String,
 }
🧹 Nitpick comments (21)
readme.md (4)

269-269: Add proper formatting for the URL.

The bare URL should be formatted using Markdown link syntax for better readability and to follow Markdown best practices.

-Tor Socks5 fetch https://tpo.pages.torproject.org/core/arti/guides/starting-arti
+Tor Socks5 fetch [https://tpo.pages.torproject.org/core/arti/guides/starting-arti](https://tpo.pages.torproject.org/core/arti/guides/starting-arti)
🧰 Tools
🪛 markdownlint-cli2 (0.17.2)

269-269: Bare URL used
null

(MD034, no-bare-urls)


271-279: Fix unordered list indentation.

The markdown linter has flagged inconsistent indentation in the unordered list. The expected indentation is 2 spaces, but 4 spaces are used.

- [ ] implement lightning nodes
-    - [X] phoenixd
-    - [X] cln
-    - [X] lnd
-    - [ ] lndk
-    - [ ] ldk_node
-    - [ ] eclair
-    - [ ] strike?? (BOLT 12 support, BOLT 11 blinded path support?)
-    - [ ] nwc?? (AlbyHub - blinded path support?) https://github.com/rust-nostr/nostr/blob/master/crates/nwc/examples/nwc.rs
-    - [ ] LNURL?
+ [ ] implement lightning nodes
+  - [X] phoenixd
+  - [X] cln
+  - [X] lnd
+  - [ ] lndk
+  - [ ] ldk_node
+  - [ ] eclair
+  - [ ] strike?? (BOLT 12 support, BOLT 11 blinded path support?)
+  - [ ] nwc?? (AlbyHub - blinded path support?) [https://github.com/rust-nostr/nostr/blob/master/crates/nwc/examples/nwc.rs](https://github.com/rust-nostr/nostr/blob/master/crates/nwc/examples/nwc.rs)
+  - [ ] LNURL?
🧰 Tools
🪛 markdownlint-cli2 (0.17.2)

271-271: Unordered list indentation
Expected: 2; Actual: 4

(MD007, ul-indent)


272-272: Unordered list indentation
Expected: 2; Actual: 4

(MD007, ul-indent)


273-273: Unordered list indentation
Expected: 2; Actual: 4

(MD007, ul-indent)


274-274: Unordered list indentation
Expected: 2; Actual: 4

(MD007, ul-indent)


275-275: Unordered list indentation
Expected: 2; Actual: 4

(MD007, ul-indent)


276-276: Unordered list indentation
Expected: 2; Actual: 4

(MD007, ul-indent)


277-277: Unordered list indentation
Expected: 2; Actual: 4

(MD007, ul-indent)


278-278: Unordered list indentation
Expected: 2; Actual: 4

(MD007, ul-indent)


278-278: Bare URL used
null

(MD034, no-bare-urls)


279-279: Unordered list indentation
Expected: 2; Actual: 4

(MD007, ul-indent)


278-278: Add proper formatting for the URL.

Similar to the previous URL, this bare URL should be formatted using Markdown link syntax.

-nwc?? (AlbyHub - blinded path support?) https://github.com/rust-nostr/nostr/blob/master/crates/nwc/examples/nwc.rs
+nwc?? (AlbyHub - blinded path support?) [https://github.com/rust-nostr/nostr/blob/master/crates/nwc/examples/nwc.rs](https://github.com/rust-nostr/nostr/blob/master/crates/nwc/examples/nwc.rs)
🧰 Tools
🪛 markdownlint-cli2 (0.17.2)

278-278: Unordered list indentation
Expected: 2; Actual: 4

(MD007, ul-indent)


278-278: Bare URL used
null

(MD034, no-bare-urls)


279-279: Consider adding a URL for LNURL.

For consistency with the other items in the list, consider adding a reference URL for LNURL as well.

-LNURL?
+LNURL? [https://github.com/fiatjaf/lnurl-rfc](https://github.com/fiatjaf/lnurl-rfc)
🧰 Tools
🪛 markdownlint-cli2 (0.17.2)

279-279: Unordered list indentation
Expected: 2; Actual: 4

(MD007, ul-indent)

crates/lni/utils.rs (2)

14-18: Consider removing debug println statements.

These debug print statements are useful during development but should typically be replaced with proper logging before merging to production code.

-    println!("invoice: {:?}", invoice);
-
-    // Get the amount from the invoice
-    let invoice_amount_msats = invoice.amount_milli_satoshis().unwrap_or(0);
-    println!("invoice amount: {:?}", invoice_amount_msats);
+    // Get the amount from the invoice
+    let invoice_amount_msats = invoice.amount_milli_satoshis().unwrap_or(0);

30-33: Consider removing debug println statements.

Similar to the previous comment, these debug print statements should be replaced with proper logging.

-    println!(
-        "calculated fee_limit_msat {} from percentage, {} for the total amount {}",
-        fee_msats, fee_percentage, amount_msats
-    );
bindings/lni_nodejs/main.mjs (1)

46-50: Commented out code for payInvoice is a good placeholder for future implementation.

The commented code hints at how to implement the payInvoice functionality in the phoenixd function, but isn't actively used yet.

Consider adding a TODO comment explaining when this code will be uncommented and used.

bindings/lni_nodejs/src/lnd.rs (3)

1-3: Consider avoiding wildcard imports if possible.

Replacing:

use lni::{lnd::lib::LndConfig, CreateInvoiceParams, PayInvoiceParams};

with more selective imports or distinct namespaces can improve clarity and help maintain a structured import hierarchy.


9-14: Constructor security check.

When storing the entire macaroon, you may want to confirm that you actually need persisted plaintext credentials. Consider storing sensitive data more securely or adding usage disclaimers.


128-135: Rename “str” parameter to avoid naming confusion.

Using str as a parameter name may overshadow the str type reference in Rust. Consider something like encoded_str or payload_str:

-pub async fn decode(&self, str: String) -> Result<String> {
-    ...
+pub async fn decode(&self, encoded_str: String) -> Result<String> {
+    ...
bindings/lni_nodejs/index.d.ts (3)

19-25: Validate naming consistency with other pay responses.

PhoenixPayInvoiceResp aligns with existing naming patterns. If consistent with the rest of your codebase, all good. Otherwise, consider a more generic name like PayInvoiceResp for reuse.


34-37: Consider flagging macaroon as a secret.

You might mark macaroon with doc comments warning developers it’s sensitive. This helps communicate the potential security considerations when handling this field.


247-261: LndNode class interface.

This interface reflects the Rust definitions. Double-check that returning the entire macaroon (via getMacaroon) is truly intended.

crates/lni/lnd/api.rs (2)

232-242: Partial support for decode.

Currently, decoding only handles Bolt11. Document or handle Bolt12 decode if you plan to expand. Errors are straightforward, but call out that partial coverage is by design.


358-421: Incoming transaction listing sorts only invoices.

list_transactions is restricted to invoices. If you require outgoing payments, you may need a separate call or unify them. Confirm it aligns with your design for a complete transaction list.

crates/lni/lnd/types.rs (6)

39-49: Use enums for payment status

The status field in PayResponse is using a string type. Similar to the invoice status, consider using an enum for better type safety and improved code readability.

+#[derive(Debug, Deserialize)]
+#[serde(rename_all = "lowercase")]
+pub enum PaymentStatus {
+    Succeeded,
+    Failed,
+    InFlight,
+    // Add other possible statuses
+}
+
 #[derive(Debug, Deserialize)]
 pub struct PayResponse {
     pub destination: String,
     pub payment_hash: String,
     pub created_at: f64,
     pub parts: i32,
     pub amount_msat: i64,
     pub amount_sent_msat: i64,
     pub payment_preimage: String,
-    pub status: String,
+    pub status: PaymentStatus,
 }

150-168: Consider using enums and dedicated types for payment responses

The LndPayInvoiceResponse structure contains several fields that could benefit from stronger typing:

  1. status field should be an enum instead of a string
  2. Using more semantic types for values, timestamps, and identifiers would improve code clarity
  3. failure_reason field could be an enum of possible failure types
+#[derive(Debug, Deserialize)]
+#[serde(rename_all = "lowercase")]
+pub enum PaymentStatus {
+    Succeeded,
+    Failed,
+    InFlight,
+    // Add other possible statuses
+}
+
+#[derive(Debug, Deserialize)]
+#[serde(rename_all = "lowercase")]
+pub enum FailureReason {
+    None,
+    Timeout,
+    NoRoute,
+    Error,
+    IncorrectPaymentDetails,
+    InsufficientBalance,
+    // Add other failure reasons
+}
+
 #[derive(Debug, Deserialize)]
 pub struct LndPayInvoiceResponse {
     pub payment_hash: String,
     pub value: String,
     pub creation_date: String,
     pub fee: String,
     pub payment_preimage: String,
     pub value_sat: String,
     pub value_msat: String,
     pub payment_request: String,
-    pub status: String,
+    pub status: PaymentStatus,
     pub fee_sat: String,
     pub fee_msat: String,
     pub creation_time_ns: String,
     pub htlcs: Option<Vec<Htlc>>,
     pub payment_index: String,
-    pub failure_reason: String,
+    pub failure_reason: FailureReason,
     pub first_hop_custom_records: Option<serde_json::Value>,
 }

213-222: Use enums for HTLC status

The status field in the Htlc struct is a string. Similar to other status fields, consider using an enum for better type safety.

+#[derive(Debug, Deserialize)]
+#[serde(rename_all = "lowercase")]
+pub enum HtlcStatus {
+    InFlight,
+    Succeeded,
+    Failed,
+    // Add other possible statuses
+}
+
 #[derive(Debug, Deserialize)]
 pub struct Htlc {
     pub attempt_id: String,
-    pub status: String,
+    pub status: HtlcStatus,
     pub route: Route,
     pub attempt_time_ns: String,
     pub resolve_time_ns: String,
     pub failure: Option<serde_json::Value>,
     pub preimage: String,
 }

1-223: Consider using stronger types for numeric values and timestamps

Many fields in these structs use primitive types like String, i64, and f64 when more semantic types would be clearer:

  1. Use specialized types for amounts (satoshis/millisatoshis)
  2. Use chrono types for timestamps
  3. Use appropriate types for cryptographic hashes and keys

This would make the code more self-documenting and add type safety.

Example implementation:

use chrono::{DateTime, Utc};
use std::fmt;

pub struct SatAmount(pub u64);
pub struct MsatAmount(pub u64);

// For cryptographic hashes
pub struct Hash(pub [u8; 32]);
impl fmt::Debug for Hash { /* implementation */ }
impl<'de> Deserialize<'de> for Hash { /* implementation */ }

// Then in your structs:
// pub amount_msat: MsatAmount,
// pub creation_date: DateTime<Utc>,
// pub payment_hash: Hash,

131-147: Consider using more specific types instead of serde_json::Value

Several fields use serde_json::Value for complex JSON structures. While this works, it leaves the specific structure of these values undocumented and bypasses type checking. If the structure of these fields is known, consider defining proper types.

For example:

+#[derive(Debug, Deserialize)]
+pub struct RouteHint {
+    // Fields for route hint structure
+}
+
+#[derive(Debug, Deserialize)]
+pub struct Feature {
+    // Fields for feature structure
+}
+
 #[derive(Debug, Deserialize)]
 pub struct ListInvoiceResponse {
     // ...existing fields
-    pub route_hints: Option<serde_json::Value>,
+    pub route_hints: Option<Vec<RouteHint>>,
     pub private: Option<bool>,
     pub add_index: Option<String>,
     pub settle_index: Option<String>,
     pub amt_paid: Option<String>,
     pub amt_paid_sat: Option<String>,
     pub amt_paid_msat: Option<String>,
     pub state: Option<String>,
-    pub htlcs: Option<serde_json::Value>,
-    pub features: Option<serde_json::Value>,
+    pub htlcs: Option<Vec<Htlc>>,
+    pub features: Option<HashMap<String, Feature>>,
     // ...remaining fields

114-148: Consider grouping related fields in ListInvoiceResponse

The ListInvoiceResponse struct has many fields that could be organized into logical groups by creating nested structs. This would improve readability and make the structure more maintainable.

For example:

+#[derive(Debug, Deserialize)]
+pub struct InvoiceAmount {
+    pub value: Option<String>,
+    pub value_msat: Option<String>,
+    pub amt_paid: Option<String>,
+    pub amt_paid_sat: Option<String>,
+    pub amt_paid_msat: Option<String>,
+}
+
+#[derive(Debug, Deserialize)]
+pub struct InvoiceDescription {
+    pub memo: Option<String>,
+    pub description: Option<String>,
+    pub description_hash_hex: Option<String>,
+    pub description_hash_b64: Option<String>,
+    pub description_hash: Option<String>,
+}
+
+#[derive(Debug, Deserialize)]
+pub struct InvoiceTiming {
+    pub creation_date: Option<String>,
+    pub settle_date: Option<String>,
+    pub expiry: Option<String>,
+    pub cltv_expiry: Option<String>,
+}
+
 #[derive(Debug, Deserialize)]
 pub struct ListInvoiceResponse {
-    pub memo: Option<String>,
     pub r_preimage: Option<String>,
     pub r_hash: Option<String>,
-    pub value: Option<String>,
-    pub value_msat: Option<String>,
+    pub amount: InvoiceAmount,
+    pub description: InvoiceDescription,
+    pub timing: InvoiceTiming,
     pub settled: Option<bool>,
-    pub creation_date: Option<String>,
-    pub settle_date: Option<String>,
     pub payment_request: Option<String>,
-    pub description: Option<String>,
-    pub description_hash_hex: Option<String>,
-    pub description_hash_b64: Option<String>,
-    pub description_hash: Option<String>,
-    pub expiry: Option<String>,
     pub fallback_addr: Option<String>,
-    pub cltv_expiry: Option<String>,
     // ... remaining fields
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 14a18e6 and 2385f5f.

📒 Files selected for processing (20)
  • bindings/lni_nodejs/index.d.ts (7 hunks)
  • bindings/lni_nodejs/main.mjs (3 hunks)
  • bindings/lni_nodejs/src/cln.rs (3 hunks)
  • bindings/lni_nodejs/src/lib.rs (1 hunks)
  • bindings/lni_nodejs/src/lnd.rs (1 hunks)
  • bindings/lni_nodejs/src/phoenixd.rs (2 hunks)
  • bindings/lni_uniffi/src/lni.udl (5 hunks)
  • crates/lni/Cargo.toml (1 hunks)
  • crates/lni/cln/api.rs (4 hunks)
  • crates/lni/cln/lib.rs (5 hunks)
  • crates/lni/lib.rs (1 hunks)
  • crates/lni/lnd/api.rs (1 hunks)
  • crates/lni/lnd/lib.rs (1 hunks)
  • crates/lni/lnd/types.rs (1 hunks)
  • crates/lni/phoenixd/api.rs (3 hunks)
  • crates/lni/phoenixd/lib.rs (4 hunks)
  • crates/lni/phoenixd/types.rs (1 hunks)
  • crates/lni/types.rs (5 hunks)
  • crates/lni/utils.rs (1 hunks)
  • readme.md (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
  • bindings/lni_nodejs/src/lib.rs
  • crates/lni/Cargo.toml
  • crates/lni/types.rs
  • bindings/lni_nodejs/src/cln.rs
  • crates/lni/cln/lib.rs
🧰 Additional context used
🧬 Code Definitions (6)
crates/lni/phoenixd/types.rs (1)
bindings/lni_nodejs/index.d.ts (1)
  • PhoenixPayInvoiceResp (19-25)
crates/lni/lib.rs (1)
bindings/lni_nodejs/index.d.ts (2)
  • LndConfig (34-37)
  • LndNode (38-41)
crates/lni/cln/api.rs (6)
crates/lni/utils.rs (1)
  • calculate_fee_msats (6-36)
bindings/lni_nodejs/index.d.ts (3)
  • PayInvoiceParams (199-210)
  • PayInvoiceResponse (157-161)
  • Transaction (58-72)
crates/lni/cln/lib.rs (2)
  • pay_invoice (51-56)
  • lookup_invoice (82-93)
crates/lni/lnd/api.rs (4)
  • pay_invoice (112-230)
  • client (15-31)
  • params (188-191)
  • lookup_invoice (300-356)
crates/lni/lnd/lib.rs (2)
  • pay_invoice (41-46)
  • lookup_invoice (72-77)
crates/lni/phoenixd/api.rs (2)
  • pay_invoice (100-129)
  • lookup_invoice (198-226)
crates/lni/lnd/lib.rs (3)
bindings/lni_nodejs/index.d.ts (8)
  • NodeInfo (50-57)
  • CreateInvoiceParams (178-190)
  • ListTransactionsParams (173-177)
  • PayCode (191-198)
  • PayInvoiceParams (199-210)
  • PayInvoiceResponse (157-161)
  • Transaction (58-72)
  • LndConfig (34-37)
bindings/lni_nodejs/src/lnd.rs (8)
  • new (12-14)
  • get_info (35-39)
  • create_invoice (42-51)
  • pay_invoice (54-63)
  • get_offer (66-72)
  • lookup_invoice (103-111)
  • list_transactions (114-126)
  • decode (129-134)
crates/lni/types.rs (2)
  • default (228-242)
  • default (273-287)
bindings/lni_nodejs/src/lnd.rs (2)
bindings/lni_nodejs/index.d.ts (8)
  • LndConfig (34-37)
  • CreateInvoiceParams (178-190)
  • PayInvoiceParams (199-210)
  • NodeInfo (50-57)
  • Transaction (58-72)
  • PayInvoiceResponse (157-161)
  • PayCode (191-198)
  • ListTransactionsParams (173-177)
crates/lni/lnd/lib.rs (10)
  • new (23-28)
  • get_info (30-32)
  • create_invoice (34-39)
  • pay_invoice (41-46)
  • get_offer (48-50)
  • list_offers (52-54)
  • pay_offer (56-70)
  • lookup_invoice (72-77)
  • list_transactions (79-89)
  • decode (91-93)
crates/lni/lnd/api.rs (5)
bindings/lni_nodejs/index.d.ts (6)
  • Bolt11Resp (14-18)
  • NodeInfo (50-57)
  • CreateInvoiceParams (178-190)
  • PayInvoiceParams (199-210)
  • PayInvoiceResponse (157-161)
  • Transaction (58-72)
crates/lni/utils.rs (1)
  • calculate_fee_msats (6-36)
crates/lni/lnd/lib.rs (7)
  • new (23-28)
  • get_info (30-32)
  • create_invoice (34-39)
  • pay_invoice (41-46)
  • decode (91-93)
  • lookup_invoice (72-77)
  • list_transactions (79-89)
crates/lni/phoenixd/api.rs (6)
  • get_info (16-34)
  • create_invoice (36-98)
  • pay_invoice (100-129)
  • decode (133-135)
  • lookup_invoice (198-226)
  • list_transactions (228-357)
crates/lni/lib.rs (1)
  • from (16-20)
🪛 markdownlint-cli2 (0.17.2)
readme.md

269-269: Bare URL used
null

(MD034, no-bare-urls)


271-271: Unordered list indentation
Expected: 2; Actual: 4

(MD007, ul-indent)


272-272: Unordered list indentation
Expected: 2; Actual: 4

(MD007, ul-indent)


273-273: Unordered list indentation
Expected: 2; Actual: 4

(MD007, ul-indent)


274-274: Unordered list indentation
Expected: 2; Actual: 4

(MD007, ul-indent)


275-275: Unordered list indentation
Expected: 2; Actual: 4

(MD007, ul-indent)


276-276: Unordered list indentation
Expected: 2; Actual: 4

(MD007, ul-indent)


277-277: Unordered list indentation
Expected: 2; Actual: 4

(MD007, ul-indent)


278-278: Unordered list indentation
Expected: 2; Actual: 4

(MD007, ul-indent)


278-278: Bare URL used
null

(MD034, no-bare-urls)


279-279: Unordered list indentation
Expected: 2; Actual: 4

(MD007, ul-indent)

🔇 Additional comments (59)
readme.md (1)

273-273: Great job implementing LND support!

I see you've completed the LND implementation task which aligns with the PR objectives. The implementation appears to be consistent with the other Lightning node implementations.

🧰 Tools
🪛 markdownlint-cli2 (0.17.2)

273-273: Unordered list indentation
Expected: 2; Actual: 4

(MD007, ul-indent)

crates/lni/lib.rs (2)

37-42: LGTM - LND module structure is consistent with existing patterns.

The new lnd module follows the same structure as the existing phoenixd and cln modules, with appropriate sub-modules and exports. This maintains consistency in the codebase.


47-48: LGTM - New utils module exported.

The introduction of a utils module is a good addition for shared functionality like fee calculations that can be used across different node implementations.

crates/lni/phoenixd/types.rs (1)

105-118: LGTM - Response struct is consistently structured with other types.

The new PhoenixPayInvoiceResp struct follows the same pattern as other response types in the file, with appropriate annotations for serialization/deserialization and field renaming. The field naming is consistent with the existing codebase style.

crates/lni/utils.rs (1)

6-36: LGTM - Utility function for fee calculation.

The calculate_fee_msats function provides a useful utility for calculating fees based on invoice amounts and specified percentage rates. This will ensure consistent fee calculation logic across different node implementations.

crates/lni/phoenixd/lib.rs (3)

51-56: The new pay_invoice method is well-structured and follows existing patterns.

The implementation correctly passes the required parameters to the API function and maintains consistency with the other methods in the struct.


160-161: Good use of Default trait.

Using ..Default::default() for the remaining fields is a clean approach that makes the code more maintainable as new fields are added to the struct.


4-7: Import changes correctly support the new functionality.

The updated imports properly include the new types needed for the pay invoice functionality.

bindings/lni_nodejs/main.mjs (2)

1-1: Import statement correctly updated to include LndNode.

The import statement has been properly updated to include the new LndNode class.


98-132: LND integration follows the consistent pattern established by other node types.

The implementation of the lnd() function follows the same structure as the existing phoenixd() and cln() functions, providing a consistent interface for users.

However, there is a concern regarding the environment variable used in line 123:

#!/bin/bash
# Check if the LND_TEST_PAYMENT_HASH environment variable is defined
if grep -q "LND_TEST_PAYMENT_HASH" .env 2>/dev/null; then
  echo "LND_TEST_PAYMENT_HASH is defined in .env file"
else
  echo "Warning: LND_TEST_PAYMENT_HASH not found in .env file"
  echo "This will cause lookupInvoice() to fail in the lnd() function"
fi
crates/lni/phoenixd/api.rs (2)

1-7: Import statements properly updated to include new types.

The imports are correctly updated to include PhoenixPayInvoiceResp, PayInvoiceParams, and PayInvoiceResponse, which are needed for the new functionality.


191-192: Variable rename improves code clarity.

Changing fee to fee_msats makes the code clearer by explicitly indicating the unit of measurement, which is especially important given the conversion from satoshis to millisatoshis.

bindings/lni_nodejs/src/phoenixd.rs (3)

1-1: Import statement correctly updated to include PayInvoiceParams.

The import has been properly updated to include the PayInvoiceParams type needed for the new pay_invoice method.


61-71: Well-implemented pay_invoice method.

The method follows the established pattern of other methods in the struct, correctly passing the node's URL and password along with the provided parameters to the API function and handling errors appropriately.


74-79: Improved code formatting enhances readability.

The reformatting of the get_offer method improves code consistency with other methods in the file.

crates/lni/cln/api.rs (6)

6-9: Imports look good.


418-418: Validate that fee calculation cannot be negative.

The expression pay_resp.amount_sent_msat - pay_resp.amount_msat could yield a negative fee in edge cases. Please confirm that real-world scenarios never allow amount_sent_msat to be less than amount_msat.


422-434: Prevent potential panic when no invoices are found.

This repeats a prior review concern: .first().unwrap() will panic if the transactions vector is empty. Handle the empty case gracefully:

 match lookup_invoices(url, rune, payment_hash, from, limit) {
     Ok(transactions) => {
-        Ok(transactions.first().unwrap().clone())
+        if let Some(t) = transactions.first() {
+            Ok(t.clone())
+        } else {
+            Err(ApiError::Api {
+                reason: "No matching invoice found".to_string(),
+            })
+        }
     }
     Err(e) => Err(e),
 }

435-442: No issues found.


503-503: No concerns here.


509-509: Looks good.

bindings/lni_uniffi/src/lni.udl (9)

17-19: New pay_invoice method for PhoenixdNode looks fine.


48-50: New pay_invoice method for ClnNode is consistent with the rest of the interface.


51-68: Additional ClnNode methods are coherent and align with existing interface patterns.


69-69: Interface block termination is fine.


71-74: LndConfig dictionary fields look straightforward.


76-87: LndNode interface additions are consistent and no issues.


244-244: Dictionary end bracket is fine.


316-322: PayCode dictionary updates look good.


324-335: PayInvoiceParams dictionary is well-defined and matches the code usage.

crates/lni/lnd/lib.rs (15)

1-3: Conditional N-API imports look fine.


4-9: Imports are correct and consistent with usage.


10-14: LndConfig struct is straightforward.


16-20: LndNode struct definitions look good.


22-28: Constructor properly assigns config fields.


30-32: get_info method delegates logic appropriately.


34-39: create_invoice method usage looks correct.


41-46: pay_invoice method aligns with desired usage.


48-50: get_offer method is consistent with the LND API.


52-54: list_offers method is acceptable.


56-70: pay_offer implementation delegates to the API correctly.


72-77: lookup_invoice is straightforward.


79-89: list_transactions usage is consistent with the rest of the code.


91-93: decode method usage is standard.


96-341: Test coverage is impressive.

Tests rely on environment variables, which can affect reproducibility, but overall the coverage for invoice creation, payment, lookups, and decoding is thorough.

bindings/lni_nodejs/src/lnd.rs (6)

5-7: Structure is straightforward.

Encapsulating the config in LndNode is clean. However, ensure there's no accidental exposure of internal fields in external modules unless intentional.


21-24: Returning the entire macaroon might be a security concern.

A similar point was raised in previous reviews. Directly exposing the macaroon can be risky if logs or UI inadvertently disclose it. Consider obfuscating or restricting direct access.


26-32: Returning the full config includes sensitive macaroon.

As previously discussed (lines 26-32 in past reviews), returning sensitive fields (like macaroon) may not be ideal. Verify you truly want to expose them.


34-40: Synchronous blocking in an async method.

This method uses lni::lnd::api::get_info which in turn creates a reqwest::blocking::Client. Past reviews recommended using async clients or making the function synchronous to avoid blocking the runtime.


41-51: Blocking calls in an async function.

create_invoice calls an underlying blocking reqwest::Client. Reiterating the suggestion to switch to reqwest’s async API or remove the async signature to avoid potential runtime blocking.


83-100: Check partial success in offer payment.

If LND enters a partial payment state or partial success is returned, ensure you handle it properly. Right now, any error triggers a direct rejection. This is correct for fully failing states—but confirm partial states are also surfaced.

Do you want me to generate a script to scan for partial success scenarios or discuss them with upstream docs?

bindings/lni_nodejs/index.d.ts (5)

38-41: Minimal LndNode interface.

While it matches the Rust struct shape, ensure you expose any additional fields or methods that the code uses internally (if needed by consumers). Otherwise, looks good.


185-189: New invoice params look valid.

rPreimage, isBlinded, isKeysend, isAmp, and isPrivate usage lines up with the Rust code. Make sure each new param is tested or documented if it’s not fully supported.


199-210: Check optional fee fields.

Having both feeLimitMsat and feeLimitPercentage optional is good. You already handle conflicts in Rust code by returning errors if both are set. Good consistency.


226-226: Ensure PhoenixdNode payInvoice coverage.

No immediate issues found. Confirm it behaves consistently with the newly introduced PayInvoiceParams in the Rust backend.


239-246: CLN methods are consistent.

Adding payInvoice, getOffer, listOffers, payOffer, lookupInvoice, listTransactions, and decode broadens parity across nodes. Looks aligned with your LND/CLN bridging.

crates/lni/lnd/api.rs (2)

15-31: Invalid cert acceptance in production.

As previously noted, using .danger_accept_invalid_certs(true) is risky. Consider removing or gating it behind a dev/test config to maintain secure TLS in production.


52-110: Async function with blocking HTTP client.

Past reviewers recommended using reqwest’s async API or removing async to prevent blocking the async executor. The same pattern appears in create_invoice.

crates/lni/lnd/types.rs (1)

57-76: Use enums for invoice status

The status field is a string that can only be "paid", "unpaid", or "expired". Consider using a Rust enum to ensure type safety, reduce runtime string checks, and prevent accidental typos.

+#[derive(Debug, Deserialize)]
+#[serde(rename_all = "lowercase")]
+pub enum InvoiceStatus {
+    Paid,
+    Unpaid,
+    Expired,
+}
+
 #[derive(Debug, Deserialize)]
 pub struct Invoice {
     pub label: String,
     pub bolt11: Option<String>,
     pub bolt12: Option<String>,
     pub payment_hash: String,
-    pub status: String, // "paid" "unpaid" "expired"
+    pub status: InvoiceStatus,
     pub pay_index: Option<i32>,
     // ...rest of the fields remain the same

@ntheile ntheile merged commit 794dae2 into master Mar 24, 2025
1 check was pending
@coderabbitai coderabbitai bot mentioned this pull request Apr 7, 2025
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