core lightning #4
Conversation
WalkthroughThe changes refactor the Lightning Network integrations by removing several obsolete interfaces and classes while introducing a new CLN implementation. A new class and related modules for Changes
Sequence Diagram(s)sequenceDiagram
participant Main as main()
participant Phoenixd as PhoenixdNode API
participant Cln as ClnNode API
participant LN as Lightning API
Main->>Phoenixd: Call phoenixd() function
Phoenixd->>LN: Fetch node info / process payment offer
LN-->>Phoenixd: Return invoice/payment response
Phoenixd->>Main: Return results
Main->>Cln: Call cln() function
Cln->>LN: Fetch node info / lookup invoice / process pay offer
LN-->>Cln: Return transaction/payment details
Cln->>Main: Return results
Poem
Tip ⚡🧪 Multi-step agentic review comment chat (experimental)
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🔭 Outside diff range comments (2)
crates/lni/phoenixd/api.rs (2)
96-114:⚠️ Potential issueImprove error handling in get_info function.
The function has several calls to
unwrap()which can lead to panics if the network request fails or returns malformed data. Consider proper error handling with the?operator or pattern matching.- let response_text = response.unwrap().text().unwrap(); + let response = response.map_err(|e| ApiError::Network { reason: e.to_string() })?; + let response_text = response.text().map_err(|e| ApiError::Network { reason: e.to_string() })?;This pattern should be applied throughout the file for robust error handling.
220-223: 🛠️ Refactor suggestionImprove error handling in pay_resp deserialization.
The error handling for JSON deserialization returns the entire response text as the error reason, which could expose sensitive information. Consider returning a more generic error message.
- Err(e) => return Err(ApiError::Json { reason: response_text.to_string() }), + Err(e) => return Err(ApiError::Json { reason: format!("Failed to parse payment response: {}", e) }),
🧹 Nitpick comments (14)
bindings/lni_nodejs/src/lib.rs (1)
10-10: Remove redundant explicit importThere's already a wildcard import
pub use lni::types::*;on line 9 that would importTransactionalong with all other types from this module. The additional explicit import is unnecessary and could cause confusion.pub use lni::types::*; -pub use lni::types::{Transaction};crates/lni/cln/types.rs (1)
3-12: Add additional derive traits and documentationThe
InfoResponsestruct could benefit from additional derive traits commonly used for data structures, such asClone,PartialEq, and possiblySerializeif needed. Additionally, consider adding documentation comments to explain the purpose of this struct and its fields.use serde::Deserialize; +use serde::Serialize; -#[derive(Debug, Deserialize)] +/// Response structure for CLN node information +#[derive(Debug, Deserialize, Clone, PartialEq, Serialize)] pub struct InfoResponse { // #[serde(rename = "nodeId")] // Handle JSON field `nodeId` // pub node_id: String, + /// The node's public key pub id: String, + /// The node's alias pub alias: String, + /// The node's color in hex format pub color: String, + /// The blockchain network (e.g., "bitcoin", "testnet") pub network: String, + /// Current block height known to the node pub blockheight: i64, }bindings/lni_uniffi/src/lni.udl (1)
87-101: Consider adding documentation for new fieldsWhile the field additions are good, adding comments to describe the purpose of the new fields would improve clarity, similar to how documentation could be added to other dictionaries in the file.
dictionary Transaction { string type_; string invoice; string description; string description_hash; string preimage; string payment_hash; i64 amount_msats; i64 fees_paid; i64 created_at; i64 expires_at; i64 settled_at; + // Optional note provided by the payer string? payer_note; + // Optional external identifier for integration with other systems string? external_id; };crates/lni/types.rs (1)
175-175: Add the payment_hash field before preimage for consistency.The
payment_hashfield is a logical addition to thePayInvoiceResponsestruct. However, for better organization and consistency, consider placing it before thepreimagefield since the payment hash is typically derived first in the payment flow, and this ordering would match the fields in theTransactionstruct.@@ -174,8 +174,8 @@ #[cfg_attr(feature = "napi_rs", napi(object))] #[derive(Debug, Serialize, Deserialize)] pub struct PayInvoiceResponse { - pub payment_hash: String, pub preimage: String, + pub payment_hash: String, pub fee: i64, }crates/lni/cln/api.rs (1)
24-33: Populate the block_hash field from the response if available.The
block_hashfield is currently hardcoded to an empty string, but it should be populated from the response if available.let node_info = NodeInfo { alias: info.alias, color: info.color, pubkey: info.id, network: info.network, block_height: info.blockheight, - block_hash: "".to_string(), + block_hash: info.block_hash.unwrap_or_default(), }; Ok(node_info)Note: You'll need to add a
block_hashfield to theInfoResponsestruct in the types module.readme.md (3)
118-127: Improve Markdown formatting for environment variables section.The environment variables section has inconsistent formatting compared to other sections.
-### .env -``` +### Environment Variables + +```env PHOENIXD_URL=http://localhost:9740 PHOENIXD_PASSWORD=YOUR_HTTP_PASSWORD PHOENIXD_TEST_PAYMENT_HASH=YOUR_TEST_PAYMENT_HASH TEST_OFFER=lno*** CLN_URL=http://localhost:3010 CLN_RUNE=YOUR_RUNE🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
118-118: Heading levels should only increment by one level at a time
Expected: h2; Actual: h3(MD001, heading-increment)
118-118: Heading style
Expected: setext; Actual: atx(MD003, heading-style)
119-119: Fenced code blocks should have a language specified
null(MD040, fenced-code-language)
138-151: Good addition of native module instructions.The instructions for using native modules in environments like Electron or Vercel are helpful. Consider standardizing the indentation and formatting to match the rest of the document.
- - if you want to use the native node module (maybe for an electron app) you can reference the file `bindings/lni_nodejs/lni_js.${platform}-${arch}.node`. It would look something like in your project: - ```typescript - const path = require("path"); - const os = require("os"); - const platform = os.platform(); - const arch = os.arch(); - const nativeModulePath = path.join( - __dirname, - `../../code/lni/bindings/lni_nodejs/lni_js.${platform}-${arch}.node` - ); - const { PhoenixdNode } = require(nativeModulePath); - ``` + - If you want to use the native Node.js module (maybe for an Electron app), you can reference the file `bindings/lni_nodejs/lni_js.${platform}-${arch}.node`. + + Example usage in your project: + + ```typescript + const path = require("path"); + const os = require("os"); + const platform = os.platform(); + const arch = os.arch(); + const nativeModulePath = path.join( + __dirname, + `../../code/lni/bindings/lni_nodejs/lni_js.${platform}-${arch}.node` + ); + const { PhoenixdNode } = require(nativeModulePath); + ```🧰 Tools
🪛 LanguageTool
[uncategorized] ~138-~138: The official spelling of this programming framework is “Node.js”.
Context: ...rn build- testnode main.mjs` - nodejs - native modules (electron, vercel etc....(NODE_JS)
[typographical] ~138-~138: Two consecutive dots
Context: ...s - native modules (electron, vercel etc..) - if you want to use the native no...(DOUBLE_PUNCTUATION)
🪛 markdownlint-cli2 (0.17.2)
139-139: Unordered list indentation
Expected: 2; Actual: 4(MD007, ul-indent)
225-225: Format the URL properly in the research item.The URL should be properly formatted as a link rather than a bare URL.
-- [ ] Facade REST API? - Use the same api as phoenixd https://phoenix.acinq.co/server/api as a facade in front of any lightning node implementation. +- [ ] Facade REST API? - Use the same API as phoenixd [https://phoenix.acinq.co/server/api](https://phoenix.acinq.co/server/api) as a facade in front of any lightning node implementation.🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
225-225: Bare URL used
null(MD034, no-bare-urls)
crates/lni/cln/lib.rs (3)
29-31: Avoid unnecessary cloning of strings.The
get_infomethod is unnecessarily cloning the URL and rune strings. Since these are just being passed to an API function, consider passing them by reference if the API supports it.- pub async fn get_info(&self) -> Result<NodeInfo, ApiError> { - crate::cln::api::get_info(self.url.clone(), self.rune.clone()) - } + pub async fn get_info(&self) -> Result<NodeInfo, ApiError> { + crate::cln::api::get_info(self.url.as_str(), self.rune.as_str()) + }This would require modifying the signature of the
get_infofunction in theapimodule to accept string slices (&str) instead of owned strings.
59-70: Fix error message in test function.The error message in the test doesn't match the context. It mentions "Failed to get offer" but should be "Failed to get info" since you're testing the
get_infomethod.- panic!("Failed to get offer: {:?}", e); + panic!("Failed to get info: {:?}", e);
34-71: Add more test coverage.The test module only includes a positive test case for
get_info. Consider adding tests for error scenarios, such as invalid URLs or authentication failures, to ensure robust error handling.For example, you could add a test with an intentionally invalid URL:
#[test] async fn test_get_info_invalid_url() { let node = ClnNode::new(ClnConfig { url: "invalid_url".to_string(), rune: RUNE.clone(), }); match node.get_info().await { Ok(_) => { panic!("Expected an error but got a successful response"); } Err(e) => { println!("Got expected error: {:?}", e); // Verify it's the right kind of error } } }crates/lni/phoenixd/api.rs (3)
130-136: Address TODO comments in bolt11_req.There are TODO comments for the
external_idandwebhook_urlfields. Since you've added support for these fields in the struct, consider implementing them properly.- external_id: None, // TODO + external_id: Some("".to_string()), // Initialize with empty string like other placesWould you like me to help implement these TODO items or create an issue to track them?
8-10: Track and address global TODO items.There are TODO comments at the top of the file for
list_channelsandget_balancefunctions that haven't been implemented yet.Would you like me to help implement these functions or create an issue to track them for future implementation?
101-103: Remove debugging println statement.There's a
println!statement that outputs the raw response. In production code, this should be replaced with proper logging or removed entirely.- println!("Raw response: {}", response_text); + // Use a proper logging framework instead of println + log::debug!("Raw response: {}", response_text);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
bindings/lni_nodejs/yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (10)
bindings/lni_nodejs/index.d.ts(2 hunks)bindings/lni_nodejs/src/lib.rs(1 hunks)bindings/lni_uniffi/src/lni.udl(2 hunks)crates/lni/cln/api.rs(1 hunks)crates/lni/cln/lib.rs(1 hunks)crates/lni/cln/types.rs(1 hunks)crates/lni/lib.rs(1 hunks)crates/lni/phoenixd/api.rs(8 hunks)crates/lni/types.rs(2 hunks)readme.md(3 hunks)
🧰 Additional context used
🪛 markdownlint-cli2 (0.17.2)
readme.md
118-118: Heading levels should only increment by one level at a time
Expected: h2; Actual: h3
(MD001, heading-increment)
118-118: Heading style
Expected: setext; Actual: atx
(MD003, heading-style)
119-119: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
135-135: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
136-136: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
139-139: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
225-225: Bare URL used
null
(MD034, no-bare-urls)
🪛 LanguageTool
readme.md
[uncategorized] ~138-~138: The official spelling of this programming framework is “Node.js”.
Context: ...rn build - testnode main.mjs` - nodejs - native modules (electron, vercel etc....
(NODE_JS)
[typographical] ~138-~138: Two consecutive dots
Context: ...s - native modules (electron, vercel etc..) - if you want to use the native no...
(DOUBLE_PUNCTUATION)
🔇 Additional comments (19)
bindings/lni_uniffi/src/lni.udl (2)
99-100: LGTM - Good field additions to TransactionThe addition of optional fields
payer_noteandexternal_idto the Transaction dictionary is consistent with the field naming style in the file and appropriately marked as optional withstring?.
199-199: LGTM - Good field addition to PayInvoiceResponseThe addition of the
payment_hashfield to the PayInvoiceResponse dictionary is consistent with the naming of other fields in the file.bindings/lni_nodejs/index.d.ts (3)
62-63: LGTM - Proper TypeScript property additionsThe added optional properties
payerNoteandexternalIdfollow proper camelCase naming conventions for TypeScript/JavaScript and are correctly marked as optional with the?suffix.
150-150: LGTM - Proper addition of paymentHash propertyThe addition of the
paymentHashproperty to the PayInvoiceResponse interface follows proper camelCase naming convention for TypeScript/JavaScript.
181-181: Ensure API call is updated for the payerNote parameterLine 181 shows the
payOffermethod already accepts an optionalpayerNoteparameter, which aligns with the new field added to the Transaction interface. This is good consistency.crates/lni/lib.rs (1)
29-33: Nice addition of a well-structured CLN module.The new CLN module follows the same organizational pattern as the existing phoenixd module, with a clean separation of concerns into lib, api, and types submodules. This consistency makes the codebase more maintainable and easier to navigate.
crates/lni/types.rs (1)
46-47: Good addition of optional fields with clear documentation.The new optional fields
payer_noteandexternal_idare well-documented with comments explaining their specific use cases for bolt12 and bolt11 respectively. UsingOption<String>is appropriate for these fields that may not always be present.crates/lni/cln/api.rs (1)
1-7: The imports and documentation look good.Good job on including the link to Core Lightning API documentation in the comments. The imports are also well-organized, only importing what's needed.
readme.md (1)
135-135: Updated build command to use release mode.The build command now correctly uses the
--releaseflag, which is appropriate for generating optimized bindings for production use.🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
135-135: Unordered list indentation
Expected: 2; Actual: 4(MD007, ul-indent)
crates/lni/cln/lib.rs (4)
9-19: Good struct definition with proper attributes for Node.js bindings.The structure definitions look good with appropriate attributes for Node.js bindings. The
#[cfg_attr(feature = "napi_rs", napi(object))]attribute ensures these structs are properly exposed to JavaScript when the feature is enabled.
21-27: Constructor implementation looks good.The constructor for
ClnNodeis implemented correctly, creating a new instance from aClnConfigobject.
42-57: Good use of environment variables for test configuration.The code uses
dotenvandlazy_staticto load and cache environment variables for testing, which is a good practice for handling test configuration without hardcoding sensitive information.
1-8:❓ Verification inconclusive
Verify necessary imports and feature use.
The code defines conditional compilation for the
napi_rsfeature, which is common practice for creating bindings to other languages, specifically Node.js in this case. However, ensure that all the imports are being used and are necessary for the functionality.
🏁 Script executed:
#!/bin/bash # Verify all imports are used # Check if there are any unused imports in the file rg -A 1 "use " crates/lni/cln/lib.rs | grep -v "#\[cfg" | grep "use"Length of output: 307
Action Required: Verify All Import Usages
The file currently includes not only the conditional
napi_rsimport and the expected core imports but also additional ones (e.g.,use super::*;,use dotenv::dotenv;,use lazy_static::lazy_static;,use std::env;, anduse tokio::test;). Please ensure that every import declared is actually used or needed in this module. If some of them are only used for testing or were left over from earlier iterations, consider removing them to improve code clarity and reduce potential warnings.crates/lni/phoenixd/api.rs (6)
54-57: Good field additions with helpful comments.The addition of
payer_noteandexternal_idfields to theInvoiceResponsestruct with comments explaining their purpose is well-implemented.
78-81: Consistent field additions across structs.The addition of the same fields to
OutgoingPaymentResponsemaintains consistency across the API structures.
168-169: Properly initializing new fields in transaction creation.The initialization of the new fields with empty strings wrapped in
Someis appropriate. This ensures that the fields are always present in the returnedTransactionstruct.Also applies to: 192-193
225-225: Important field addition to PayInvoiceResponse.Adding the
payment_hashfield to thePayInvoiceResponsestruct is an important improvement for tracking payments.
255-256: Good handling of optional fields in lookup_invoice.Properly extracting the optional fields from
InvoiceResponsewith appropriate fallbacks to empty strings.
377-380:❓ Verification inconclusive
Similar description behavior change for outgoing transactions.
Same issue as above - the
descriptionfield is set to an empty string for outgoing transactions as well. Ensure this change is intentional.- description: "".to_string(), + description: payment.description.unwrap_or_default(),The
OutgoingPaymentResponsestruct doesn't appear to have adescriptionfield, so we need to determine if this is an oversight or if the field should not be populated for outgoing transactions.
Please run the above script to verify if the
OutgoingPaymentResponsestruct contains adescriptionfield. Once we confirm its definition, we can decide whether the empty string assignment for outgoing transactions is intentional or if it should be updated similarly to incoming transactions (i.e., usingpayment.description.unwrap_or_default()).
🏁 Script executed:
#!/bin/bash # Search for the definition of OutgoingPaymentResponse to verify whether it includes a 'description' field. rg "struct OutgoingPaymentResponse" -n .Length of output: 233
Please run this script to verify if the
OutgoingPaymentResponsestruct contains anydescriptionfield. This will help determine if populating adescriptionfield for outgoing transactions is needed or if the empty string assignment is intentional.
🏁 Script executed:
#!/bin/bash # Search for any occurrence of 'description:' in the file to check if it's present in the OutgoingPaymentResponse struct. rg "description:" -n ./crates/lni/phoenixd/api.rsLength of output: 632
Action Required: Confirm
descriptionField Usage in Outgoing Transactions
- The code at lines 377–380 currently sets
descriptionto an empty string.- A diff suggestion proposes updating it to use
payment.description.unwrap_or_default(), mirroring changes made for incoming transactions.- However, verification shows that the
OutgoingPaymentResponsestruct does not include adescriptionfield.- Please verify whether the outgoing transactions should include a description. If so, update the
OutgoingPaymentResponsestruct accordingly; otherwise, remove or repurpose the assignment.
| // #[serde(rename = "nodeId")] // Handle JSON field `nodeId` | ||
| // pub node_id: String, |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Uncomment or remove commented code
There are commented-out lines for a node_id field with a JSON field rename. Either uncomment and implement this field if it's needed, or remove the commented code to improve code cleanliness.
🏁 Script executed:
#!/bin/bash
# Check if node_id is used elsewhere in the codebase
rg "node_id" --type rust -A 3 -B 3 crates/lni/cln/Length of output: 433
Action Required: Remove Unused Commented Code in crates/lni/cln/types.rs
The commented-out lines for the node_id field (with its JSON rename) are not used elsewhere in the codebase. Please remove these lines to keep the code clean. If you intend to use this field in the future, uncomment and fully implement it; otherwise, removal is the best course of action.
- File:
crates/lni/cln/types.rs(lines 5–6)
crates/lni/cln/api.rs
Outdated
| pub fn get_info(base: String, rune: String) -> Result<NodeInfo, ApiError> { | ||
| let req_url = format!("{}/v1/getinfo", base); | ||
| println!("Constructed URL: {} rune {}", req_url, rune); | ||
| let mut headers = reqwest::header::HeaderMap::new(); | ||
| headers.insert("Rune", header::HeaderValue::from_str(&rune).unwrap()); | ||
| let client = reqwest::blocking::ClientBuilder::new() | ||
| .danger_accept_invalid_certs(true) | ||
| .default_headers(headers.clone()) | ||
| .build() | ||
| .unwrap(); | ||
|
|
There was a problem hiding this comment.
Improve error handling and remove unsafe TLS configuration.
The current implementation uses .unwrap() which will panic if errors occur. Additionally, disabling certificate validation is a security risk in production environments.
Consider using proper error handling with the ? operator and making TLS validation configurable:
-pub fn get_info(base: String, rune: String) -> Result<NodeInfo, ApiError> {
+pub fn get_info(base: String, rune: String, skip_cert_verification: bool) -> Result<NodeInfo, ApiError> {
let req_url = format!("{}/v1/getinfo", base);
- println!("Constructed URL: {} rune {}", req_url, rune);
+ log::debug!("Constructed URL: {}", req_url);
let mut headers = reqwest::header::HeaderMap::new();
- headers.insert("Rune", header::HeaderValue::from_str(&rune).unwrap());
+ headers.insert(
+ "Rune",
+ header::HeaderValue::from_str(&rune).map_err(|e| ApiError::Http { reason: e.to_string() })?
+ );
let client = reqwest::blocking::ClientBuilder::new()
- .danger_accept_invalid_certs(true)
+ .danger_accept_invalid_certs(skip_cert_verification)
.default_headers(headers.clone())
- .build()
- .unwrap();
+ .build()
+ .map_err(|e| ApiError::Http { reason: e.to_string() })?;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| pub fn get_info(base: String, rune: String) -> Result<NodeInfo, ApiError> { | |
| let req_url = format!("{}/v1/getinfo", base); | |
| println!("Constructed URL: {} rune {}", req_url, rune); | |
| let mut headers = reqwest::header::HeaderMap::new(); | |
| headers.insert("Rune", header::HeaderValue::from_str(&rune).unwrap()); | |
| let client = reqwest::blocking::ClientBuilder::new() | |
| .danger_accept_invalid_certs(true) | |
| .default_headers(headers.clone()) | |
| .build() | |
| .unwrap(); | |
| pub fn get_info(base: String, rune: String, skip_cert_verification: bool) -> Result<NodeInfo, ApiError> { | |
| let req_url = format!("{}/v1/getinfo", base); | |
| log::debug!("Constructed URL: {}", req_url); | |
| let mut headers = reqwest::header::HeaderMap::new(); | |
| headers.insert( | |
| "Rune", | |
| header::HeaderValue::from_str(&rune).map_err(|e| ApiError::Http { reason: e.to_string() })? | |
| ); | |
| let client = reqwest::blocking::ClientBuilder::new() | |
| .danger_accept_invalid_certs(skip_cert_verification) | |
| .default_headers(headers.clone()) | |
| .build() | |
| .map_err(|e| ApiError::Http { reason: e.to_string() })?; | |
| // ... (remaining implementation) | |
| } |
crates/lni/phoenixd/api.rs
Outdated
| description: "".to_string(), | ||
| description_hash: "".to_string(), | ||
| payer_note: Some(inv.payer_note.unwrap_or("".to_string())), | ||
| external_id: Some(inv.external_id.unwrap_or("".to_string())), |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Changed behavior for description field.
You've changed the behavior of the description field to always set it to an empty string, rather than using the value from the invoice. Make sure this is intentional as it could be a regression.
- description: "".to_string(),
+ description: inv.description.unwrap_or_default(),Let's check how this field is handled elsewhere in the codebase:
🏁 Script executed:
#!/bin/bash
# Check how description is handled in other Transaction creation code
# Look for patterns where Transaction struct is created
rg -A 20 "Transaction \{" --glob "*.rs" | grep -A 1 "description:" | grep -v "description_hash"Length of output: 562
Critical: Inconsistent Handling of the description Field
The current implementation in crates/lni/phoenixd/api.rs (lines 322–325) still hard-codes the description as an empty string, which deviates from the pattern observed elsewhere where the field is set from the invoice using unwrap_or_default(). In other parts of the codebase, we see assignments like:
description: description.unwrap_or_default(),description: inv.description.unwrap_or_default(),
This inconsistency suggests that the hard-coded empty string might be an unintentional regression. Please verify whether the change was purposeful. If not, update the code to consistently use the invoice-provided description. For example:
- description: "".to_string(),
+ description: inv.description.unwrap_or_default(),Ensure that this update aligns with the overall behavior expected for transaction creation.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| description: "".to_string(), | |
| description_hash: "".to_string(), | |
| payer_note: Some(inv.payer_note.unwrap_or("".to_string())), | |
| external_id: Some(inv.external_id.unwrap_or("".to_string())), | |
| description: inv.description.unwrap_or_default(), | |
| description_hash: "".to_string(), | |
| payer_note: Some(inv.payer_note.unwrap_or("".to_string())), | |
| external_id: Some(inv.external_id.unwrap_or("".to_string())), |
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (2)
crates/lni/cln/api.rs (2)
8-30:⚠️ Potential issueUse HTTP GET method and improve error handling for getinfo endpoint.
The current implementation uses a POST request for the
/v1/getinfoendpoint, but according to RESTful principles, it should be a GET request since it's retrieving information. Also, using.unwrap()is dangerous as it will panic if an error occurs.pub fn get_info(url: String, rune: String) -> Result<NodeInfo, ApiError> { let req_url = format!("{}/v1/getinfo", url); - println!("Constructed URL: {} rune {}", req_url, rune); + println!("Constructed URL: {}", req_url); // Don't log sensitive rune values let client = clnrest_client(rune); let response = client - .post(&req_url) + .get(&req_url) .header("Content-Type", "application/json") .send() - .unwrap(); + .map_err(|e| ApiError::Http { reason: e.to_string() })?; let response_text = response.text().unwrap(); println!("Raw response: {}", response_text);
102-110: 🛠️ Refactor suggestionImprove error handling and make TLS validation configurable in client builder.
Using
.unwrap()can lead to panics, and disabling certificate validation is a security risk in production. Make certificate validation configurable and add proper error handling.-fn clnrest_client(rune: String) -> reqwest::blocking::Client { +fn clnrest_client(rune: String, skip_cert_verification: bool) -> Result<reqwest::blocking::Client, ApiError> { let mut headers = reqwest::header::HeaderMap::new(); - headers.insert("Rune", header::HeaderValue::from_str(&rune).unwrap()); + headers.insert( + "Rune", + header::HeaderValue::from_str(&rune) + .map_err(|e| ApiError::Http { reason: e.to_string() })? + ); - reqwest::blocking::ClientBuilder::new() - .danger_accept_invalid_certs(true) + reqwest::blocking::ClientBuilder::new() + .danger_accept_invalid_certs(skip_cert_verification) .default_headers(headers) .build() - .unwrap() + .map_err(|e| ApiError::Http { reason: e.to_string() }) }Then update the function call in
get_info:- let client = clnrest_client(rune); + let client = clnrest_client(rune, true)?; // Consider making this false in production
🧹 Nitpick comments (6)
readme.md (5)
118-118: Fix heading level for the.envsection.Markdown analysis recommends incrementing headings in order and using setext style. Apply this diff:
-### .env +## .env🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
118-118: Heading levels should only increment by one level at a time
Expected: h2; Actual: h3(MD001, heading-increment)
118-118: Heading style
Expected: setext; Actual: atx(MD003, heading-style)
119-124: Specify a language for the fenced code block.It's good practice to label code blocks for proper syntax highlighting and to satisfy markdown lint rules.
-``` +```shell🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
119-119: Fenced code blocks should have a language specified
null(MD040, fenced-code-language)
135-136: Adjust list indentation for markdown compliance.The markdown linter reports an indentation mismatch. You can correct it as follows:
- - `cd bindings/lni_nodejs && cargo build --release && yarn build` + - `cd bindings/lni_nodejs && cargo build --release && yarn build`🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
135-135: Unordered list indentation
Expected: 2; Actual: 4(MD007, ul-indent)
136-136: Unordered list indentation
Expected: 2; Actual: 4(MD007, ul-indent)
138-138: Fix naming to “Node.js” and remove double punctuation.Minor typographical issue with official project naming and extra dots.
- - nodejs - native modules (electron, vercel etc..) + - Node.js - native modules (electron, vercel, etc.)🧰 Tools
🪛 LanguageTool
[uncategorized] ~138-~138: The official spelling of this programming framework is “Node.js”.
Context: ...rn build- testnode main.mjs` - nodejs - native modules (electron, vercel etc....(NODE_JS)
[typographical] ~138-~138: Two consecutive dots
Context: ...s - native modules (electron, vercel etc..) - if you want to use the native no...(DOUBLE_PUNCTUATION)
275-276: Convert the bare link into a proper Markdown hyperlink.Markdown linting discourages bare URLs. A small formatting tweak helps maintain clarity.
-Facade REST API? - Use the same api as phoenixd https://phoenix.acinq.co/server/api +Facade REST API? - Use the same api as [phoenixd](https://phoenix.acinq.co/server/api)🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
276-276: Bare URL used
null(MD034, no-bare-urls)
crates/lni/cln/api.rs (1)
66-70: Remove unnecessary parentheses in condition check.The parentheses around the condition are unnecessary and not idiomatic Rust.
- if (fetch_invoice_resp.invoice.is_empty()) { + if fetch_invoice_resp.invoice.is_empty() { return Err(ApiError::Json { reason: "Missing BOLT 12 invoice".to_string(), }); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
crates/lni/cln/api.rs(1 hunks)crates/lni/cln/lib.rs(1 hunks)crates/lni/cln/types.rs(1 hunks)readme.md(4 hunks)
🧰 Additional context used
🪛 markdownlint-cli2 (0.17.2)
readme.md
118-118: Heading levels should only increment by one level at a time
Expected: h2; Actual: h3
(MD001, heading-increment)
118-118: Heading style
Expected: setext; Actual: atx
(MD003, heading-style)
119-119: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
135-135: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
136-136: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
139-139: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
276-276: Bare URL used
null
(MD034, no-bare-urls)
🪛 LanguageTool
readme.md
[uncategorized] ~138-~138: The official spelling of this programming framework is “Node.js”.
Context: ...rn build - testnode main.mjs` - nodejs - native modules (electron, vercel etc....
(NODE_JS)
[typographical] ~138-~138: Two consecutive dots
Context: ...s - native modules (electron, vercel etc..) - if you want to use the native no...
(DOUBLE_PUNCTUATION)
[grammar] ~252-~252: It appears that a hyphen is missing in the noun “To-do” (= task) or did you mean the verb “to do”?
Context: ...-->>App: PayInvoiceResponse object ``` Todo ==== - [X] make interface - [X] napi-rs...
(TO_DO_HYPHEN)
🔇 Additional comments (8)
crates/lni/cln/types.rs (1)
5-6: Remove or implement the commented-outnode_idfield.These lines remain unused or unimplemented. This issue was previously flagged in a past review and still applies.
readme.md (3)
125-126: Environment variables look good.
139-150: No concerns with this native module snippet.🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
139-139: Unordered list indentation
Expected: 2; Actual: 4(MD007, ul-indent)
202-251: Sequence diagram addition looks great.crates/lni/cln/lib.rs (4)
9-13: Configuration struct looks good.
15-19: Node struct definitions are proper.
21-47: CLN node implementation logic is straightforward.
50-114: Test module is well-structured.
| #[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, | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Confirm the need for signed integers in payment fields.
Payment-related amounts are typically non-negative. Consider verifying if i64 is required or if u64 would be more appropriate to avoid potential confusion or unintended behavior.
🏁 Script executed:
#!/bin/bash
# Description: Check references to PayResponse fields to confirm negative amounts aren’t expected.
rg -A 5 $'PayResponse'Length of output: 1856
Payment Amount Type: Switch to Unsigned
After verifying the usage of the PayResponse struct across the codebase, there’s no indication that negative values are ever expected for payment amounts. Since fields like amount_msat and amount_sent_msat represent inherently non-negative values, it would be more precise and safer to use u64 instead of i64.
- In crates/lni/cln/types.rs, update the fields as follows:
- pub amount_msat: i64, - pub amount_sent_msat: i64, + pub amount_msat: u64, + pub amount_sent_msat: u64,
Please review any external API constraints or downstream logic to ensure that switching to an unsigned type aligns with all expectations.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #[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, | |
| } | |
| #[derive(Debug, Deserialize)] | |
| pub struct PayResponse { | |
| pub destination: String, | |
| pub payment_hash: String, | |
| pub created_at: f64, | |
| pub parts: i32, | |
| pub amount_msat: u64, | |
| pub amount_sent_msat: u64, | |
| pub payment_preimage: String, | |
| pub status: String, | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
bindings/lni_react_native/example/src/App.tsx (1)
7-8: Consider removing commented-out Db codeSince the
Dbclass has been removed from the bindings (as seen in index.js changes), you should consider removing these commented-out lines that reference it, as they're likely no longer relevant.Also applies to: 59-72
bindings/lni_nodejs/main.mjs (1)
90-95: Added main function with call to cln()The new main function currently only calls
cln()withphoenixd()commented out. Consider providing a way to run either or both functions, perhaps based on environment configuration or command-line arguments.async function main() { - // phoenixd(); - cln(); + const nodeType = process.env.NODE_TYPE || 'cln'; + if (nodeType === 'phoenixd' || nodeType === 'all') { + await phoenixd(); + } + if (nodeType === 'cln' || nodeType === 'all') { + await cln(); + } }readme.md (1)
276-276: Fix markdown formatting for URL.The URL should be enclosed in angle brackets to comply with Markdown standards.
-- [ ] Facade REST API? - Use the same api as phoenixd https://phoenix.acinq.co/server/api as a facade in front of any lightning node implementation. +- [ ] Facade REST API? - Use the same api as phoenixd <https://phoenix.acinq.co/server/api> as a facade in front of any lightning node implementation.🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
276-276: Bare URL used
null(MD034, no-bare-urls)
bindings/lni_nodejs/src/cln.rs (2)
42-70: Commented code should be removed or completed.There are several commented-out method implementations that appear to be placeholders or copied from
PhoenixdNode. Consider either implementing these methods forClnNodeor removing the commented code.
90-147: Commented test code should be updated for ClnNode.The test code is commented out and still references
PhoenixdNodeand related structures. Consider either implementing proper tests forClnNodeor removing the commented code.crates/lni/cln/lib.rs (1)
79-90: Improve error message in get_info test.The error message in the
test_get_infopanic mentions "Failed to get offer" which is not accurate for aget_infocall.- panic!("Failed to get offer: {:?}", e); + panic!("Failed to get node info: {:?}", e);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
bindings/lni_nodejs/index.d.ts(1 hunks)bindings/lni_nodejs/index.js(1 hunks)bindings/lni_nodejs/main.mjs(1 hunks)bindings/lni_nodejs/src/cln.rs(1 hunks)bindings/lni_nodejs/src/lib.rs(1 hunks)bindings/lni_react_native/example/src/App.tsx(2 hunks)crates/lni/cln/lib.rs(1 hunks)crates/lni/phoenixd/lib.rs(2 hunks)crates/lni/types.rs(2 hunks)readme.md(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- bindings/lni_nodejs/src/lib.rs
- crates/lni/types.rs
🧰 Additional context used
🪛 markdownlint-cli2 (0.17.2)
readme.md
118-118: Heading levels should only increment by one level at a time
Expected: h2; Actual: h3
(MD001, heading-increment)
118-118: Heading style
Expected: setext; Actual: atx
(MD003, heading-style)
119-119: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
135-135: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
136-136: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
139-139: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
276-276: Bare URL used
null
(MD034, no-bare-urls)
🪛 LanguageTool
readme.md
[uncategorized] ~138-~138: The official spelling of this programming framework is “Node.js”.
Context: ...rn build - testnode main.mjs` - nodejs - native modules (electron, vercel etc....
(NODE_JS)
[typographical] ~138-~138: Two consecutive dots
Context: ...s - native modules (electron, vercel etc..) - if you want to use the native no...
(DOUBLE_PUNCTUATION)
[grammar] ~252-~252: It appears that a hyphen is missing in the noun “To-do” (= task) or did you mean the verb “to do”?
Context: ...-->>App: PayInvoiceResponse object ``` Todo ==== - [X] make interface - [X] napi-rs...
(TO_DO_HYPHEN)
🔇 Additional comments (21)
crates/lni/phoenixd/lib.rs (2)
146-149: Static reference rename:TEST_OFFER->TEST_RECEIVER_OFFERThe rename of both the static reference and the environment variable it's accessing to
TEST_RECEIVER_OFFERimproves code clarity by better specifying the purpose of this offer - it's specifically a receiver offer.
193-193: Updated function call with renamed variableThe function call has been updated to use the renamed
TEST_RECEIVER_OFFERvariable, maintaining consistency with the static reference rename.bindings/lni_nodejs/index.js (2)
313-313: Updated imports to include ClnNodeThe imports from the native binding now include
ClnNodeinstead of previously imported entities likeInvoiceTypeandDb. This change aligns with the core lightning integration.
316-316: Added ClnNode to exportsNow exporting the new
ClnNodeclass, which will allow users of this module to interact with Core Lightning nodes.bindings/lni_react_native/example/src/App.tsx (2)
13-13: Import rename:TEST_OFFER->TEST_RECEIVER_OFFERUpdated the imported environment variable name to be consistent with the changes made in other files.
75-75: Updated variable usage in payOffer methodCorrectly updated the function call to use the renamed
TEST_RECEIVER_OFFERvariable.bindings/lni_nodejs/main.mjs (4)
1-1: Updated import to include ClnNodeThe import statement now includes
ClnNodealongsidePhoenixdNode, reflecting the addition of Core Lightning support.
5-46: Refactored existing code into phoenixd() functionThe original implementation has been cleanly encapsulated within an
async function phoenixd(), making the code more modular.
31-31: Updated environment variable referenceConsistently updated the environment variable name to
TEST_RECEIVER_OFFERto match changes in other files.
48-88: Added new cln() function for Core Lightning integrationNew function to handle interactions with a Core Lightning node via the
ClnNodeclass. The implementation is similar to the phoenixd function but uses different configuration parameters (CLN_URL and CLN_RUNE).Note that several operations are commented out - is this intentional as placeholders for future implementation, or should these be uncommented and tested?
Can you verify which operations are currently supported by the new ClnNode implementation?
readme.md (4)
123-124: Environment variable name change noted.The renaming from
TEST_OFFERtoTEST_RECEIVER_OFFERprovides better clarity about the variable's purpose.
125-126: New Core Lightning environment variables are properly documented.The addition of
CLN_URLandCLN_RUNEenvironment variables is essential for the Core Lightning integration.
135-135: Build command improved for production use.The updated build command now properly includes
cargo cleanand uses the--releaseflag, which is better for production deployments.🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
135-135: Unordered list indentation
Expected: 2; Actual: 4(MD007, ul-indent)
202-251: Excellent sequence diagram addition.The sequence diagram clearly illustrates the interactions between application layers, bindings, LNI core, and lightning nodes. This significantly improves documentation for developers.
bindings/lni_nodejs/index.d.ts (1)
17-24: Well-structured ClnNode class declaration.The new
ClnNodeclass follows the same pattern as the existingPhoenixdNodeclass, maintaining consistency in the API design while supporting the Core Lightning implementation. The method signatures are appropriate for the required functionality.bindings/lni_nodejs/src/cln.rs (2)
1-40: Good implementation of ClnNode structure and core methods.The implementation properly defines the
ClnNodestructure with appropriate getter methods and theget_info()function. Error handling is correctly implemented by converting Rust errors to NAPI errors.
72-88: Proper implementation of pay_offer method.The
pay_offermethod is well-implemented with correct parameter handling and error conversion. It matches the TypeScript declaration and follows the established API pattern.crates/lni/cln/lib.rs (4)
9-19: Well-defined ClnConfig and ClnNode structures.The structures are properly defined with the necessary fields and appropriate feature flags for NAPI integration.
21-48: Clean implementation of ClnNode methods.The methods are well-implemented with proper error handling and consistent parameter passing to the API functions.
58-77: Good test setup with environment variables.The test setup properly uses environment variables with appropriate error messages when they're missing.
92-113:❓ Verification inconclusive
Hardcoded amount in pay_offer test.
The test for
pay_offeruses a hardcoded amount of 3000 millisatoshis. This could fail if the offer requires a specific amount.Consider adding a check to verify the offer's required amount or making this test more flexible:
🏁 Script executed:
#!/bin/bash # Check whether the codebase has any logic to handle offers with specific amount requirements # Search for code that might handle offer amount requirements rg -A 3 -B 3 "amount.*required|required.*amount" --glob "*.rs"Length of output: 64
ACTION: Revisit and update the pay_offer test for amount flexibility
- The test in
crates/lni/cln/lib.rscurrently uses a hardcoded value (3000) for the payment amount.- Our search for any offer-specific amount validation logic yielded no conclusive evidence.
- Please either:
- Retrieve and use the required amount dynamically (if such metadata is available), or
- Clearly document that the fixed amount is valid for the current offer context.
- Manual verification is needed to ensure that the underlying business logic hasn’t changed and that the hardcoded value still meets offer requirements.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (6)
crates/lni/phoenixd/api.rs (2)
240-243:⚠️ Potential issueCritical: Inconsistent Handling of the
descriptionFieldThe current implementation still hard-codes the description as an empty string, which deviates from the pattern observed elsewhere where the field is set from the invoice using
unwrap_or_default().Apply this diff to maintain consistency:
- description: "".to_string(), + description: inv.description.unwrap_or_default(),
295-296:⚠️ Potential issueInconsistent Handling of the
descriptionFieldSimilar to the issue above, the description field is hard-coded as an empty string rather than using the value from the payment.
Apply this diff to maintain consistency:
- description: "".to_string(), + description: payment.description.unwrap_or_default(),crates/lni/cln/api.rs (4)
8-30:⚠️ Potential issueUse HTTP GET method and improve error handling in the get_info function.
The
/v1/getinfoendpoint should use the GET method instead of POST, and there are multiple instances of.unwrap()that can cause runtime panics in production. Additionally, usingprintln!for logging is not suitable for production code.pub fn get_info(url: String, rune: String) -> Result<NodeInfo, ApiError> { let req_url = format!("{}/v1/getinfo", url); - println!("Constructed URL: {} rune {}", req_url, rune); + log::debug!("Constructed URL: {}", req_url); let client = clnrest_client(rune); let response = client - .post(&req_url) + .get(&req_url) .header("Content-Type", "application/json") .send() - .unwrap(); + .map_err(|e| ApiError::Http { reason: e.to_string() })?; - let response_text = response.text().unwrap(); - println!("Raw response: {}", response_text); + let response_text = response.text() + .map_err(|e| ApiError::Http { reason: e.to_string() })?; + log::debug!("Raw response: {}", response_text); let info: InfoResponse = serde_json::from_str(&response_text)?;
32-64:⚠️ Potential issueUse async client for async function and improve error handling in pay_offer.
The function is marked as
asyncbut uses blocking HTTP methods, which defeats the purpose of async programming. Additionally, all.unwrap()calls should be replaced with proper error handling.pub async fn pay_offer( url: String, rune: String, offer: String, amount_msats: i64, payer_note: Option<String>, ) -> Result<PayInvoiceResponse, ApiError> { let fetch_invoice_url = format!("{}/v1/fetchinvoice", url); - let client = clnrest_client(rune); + let client = clnrest_async_client(rune).await?; - let response: reqwest::blocking::Response = client + let response = client .post(&fetch_invoice_url) .header("Content-Type", "application/json") .json(&serde_json::json!({ "offer": offer, "amount_msat": amount_msats, "payer_note": payer_note, "timeout": 60, })) .send() - .unwrap(); + .await.map_err(|e| ApiError::Http { reason: e.to_string() })?; - let response_text = response.text().unwrap(); - let response_text = response_text.as_str(); + let response_text = response.text() + .await + .map_err(|e| ApiError::Http { reason: e.to_string() })?;You'll also need to implement an async version of the client builder:
async fn clnrest_async_client(rune: String) -> Result<reqwest::Client, ApiError> { let mut headers = reqwest::header::HeaderMap::new(); headers.insert( "Rune", header::HeaderValue::from_str(&rune) .map_err(|e| ApiError::Http { reason: e.to_string() })? ); reqwest::ClientBuilder::new() .danger_accept_invalid_certs(true) // Consider making this configurable .default_headers(headers) .build() .map_err(|e| ApiError::Http { reason: e.to_string() }) }
72-93:⚠️ Potential issueUse async methods and proper error handling for payment request.
Similar to the fetch invoice request, this code needs to use async methods and proper error handling to avoid unwraps.
// now pay the bolt 12 invoice lni let pay_url = format!("{}/v1/pay", url); - let pay_response: reqwest::blocking::Response = client + let pay_response = client .post(&pay_url) .header("Content-Type", "application/json") .json(&serde_json::json!({ "bolt11": fetch_invoice_resp.invoice.to_string(), - "maxfeepercent": 1, // TODO read from config + "maxfeepercent": 1, // TODO: Make this configurable "retry_for": 60, })) .send() - .unwrap(); - let pay_response_text = pay_response.text().unwrap(); - let pay_response_text = pay_response_text.as_str(); + .await + .map_err(|e| ApiError::Http { reason: e.to_string() })?; + let pay_response_text = pay_response.text() + .await + .map_err(|e| ApiError::Http { reason: e.to_string() })?; let pay_resp: PayResponse = match serde_json::from_str(&pay_response_text) { Ok(resp) => resp, Err(e) => { + log::error!("Failed to parse pay response: {:?}", e); return Err(ApiError::Json { reason: pay_response_text.to_string(), }) } };
179-187:⚠️ Potential issueImprove error handling and remove unsafe TLS configuration.
The current implementation uses
.unwrap()which will panic if errors occur. Additionally, disabling certificate validation is a security risk in production environments.-fn clnrest_client(rune: String) -> reqwest::blocking::Client { +fn clnrest_client(rune: String) -> Result<reqwest::blocking::Client, ApiError> { let mut headers = reqwest::header::HeaderMap::new(); - headers.insert("Rune", header::HeaderValue::from_str(&rune).unwrap()); + headers.insert( + "Rune", + header::HeaderValue::from_str(&rune).map_err(|e| ApiError::Http { reason: e.to_string() })? + ); - reqwest::blocking::ClientBuilder::new() - .danger_accept_invalid_certs(true) + reqwest::blocking::ClientBuilder::new() + .danger_accept_invalid_certs(false) // Consider making this configurable .default_headers(headers) .build() - .unwrap() + .map_err(|e| ApiError::Http { reason: e.to_string() }) }
🧹 Nitpick comments (14)
crates/lni/phoenixd/api.rs (1)
181-187: Clean Up Commented CodeThe file contains multiple blocks of commented-out code related to parameter handling. This can make the code harder to read and maintain.
Either remove these commented sections entirely if they're no longer needed, or add clear comments explaining why they're preserved for future reference.
Also applies to: 199-205, 256-263
readme.md (3)
118-130: Improve Markdown Formatting ConsistencyThere are several formatting issues in the markdown file that should be addressed:
- The heading hierarchy is inconsistent (jumping from h1 to h3)
- The code block at line 119 is missing a language specifier
Suggested fixes:
-### .env +## Environment Variables+env
TEST_RECEIVER_OFFER=lno**
PHOENIX_MOBILE_OFFER=lno***<details> <summary>🧰 Tools</summary> <details> <summary>🪛 markdownlint-cli2 (0.17.2)</summary> 118-118: Heading levels should only increment by one level at a time Expected: h2; Actual: h3 (MD001, heading-increment) --- 118-118: Heading style Expected: setext; Actual: atx (MD003, heading-style) --- 119-119: Fenced code blocks should have a language specified null (MD040, fenced-code-language) </details> </details> --- `141-153`: **Fix Node.js Naming and Punctuation** Minor language and formatting issues in this section: 1. The official spelling is "Node.js" (with capitalized "N" and dot) 2. Double punctuation in "etc.." ```diff -nodejs - native modules (electron, vercel etc..) +Node.js - native modules (electron, vercel, etc.)🧰 Tools
🪛 LanguageTool
[uncategorized] ~141-~141: The official spelling of this programming framework is “Node.js”.
Context: ...rn build- testnode main.mjs` - nodejs - native modules (electron, vercel etc....(NODE_JS)
[typographical] ~141-~141: Two consecutive dots
Context: ...s - native modules (electron, vercel etc..) - if you want to use the native no...(DOUBLE_PUNCTUATION)
🪛 markdownlint-cli2 (0.17.2)
142-142: Unordered list indentation
Expected: 2; Actual: 4(MD007, ul-indent)
279-279: Format URL as Proper Markdown LinkThe Phoenix API URL should be formatted as a proper markdown link rather than a bare URL.
-- [ ] Facade REST API? - Use the same api as phoenixd https://phoenix.acinq.co/server/api as a facade in front of any lightning node implementation. +- [ ] Facade REST API? - Use the same api as phoenixd [Phoenix API](https://phoenix.acinq.co/server/api) as a facade in front of any lightning node implementation.🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
279-279: Bare URL used
null(MD034, no-bare-urls)
crates/lni/cln/api.rs (3)
66-70: Remove unnecessary parentheses in if condition.The extra parentheses around the condition in the if statement are unnecessary and inconsistent with Rust style.
- if (fetch_invoice_resp.invoice.is_empty()) { + if fetch_invoice_resp.invoice.is_empty() { return Err(ApiError::Json { reason: "Missing BOLT 12 invoice".to_string(), }); }
79-80: Make the maxfeepercent configurable.The maxfeepercent is hardcoded with a TODO comment. Consider making this configurable through a parameter.
- "maxfeepercent": 1, // TODO read from config + "maxfeepercent": config.max_fee_percent.unwrap_or(1), // Default to 1%This would require adding the configuration parameter to the function signature and relevant structs.
167-177: Simplify list_transactions implementation.The current implementation with the match is unnecessarily verbose. This can be simplified.
pub fn list_transactions( url: String, rune: String, from: i64, limit: i64, ) -> Result<Vec<Transaction>, ApiError> { - match lookup_invoice(url, rune, None, Some(from), Some(limit)) { - Ok(transactions) => Ok(transactions), - Err(e) => Err(e), - } + lookup_invoice(url, rune, None, Some(from), Some(limit)) }bindings/lni_nodejs/src/cln.rs (3)
42-59: Remove or implement the commented-out make_invoice method.There's a large block of commented-out code for a
make_invoicemethod. This should either be properly implemented or removed.Either implement the method with the proper parameters and functionality, or remove the commented-out code entirely to keep the codebase clean.
61-72: Improve error handling in lookup_invoice.The code for handling transactions when none are found could be clearer and more specific.
pub async fn lookup_invoice(&self, payment_hash: String) -> napi::Result<lni::Transaction> { let txn = lni::cln::api::lookup_invoice( self.inner.url.clone(), self.inner.rune.clone(), Some(payment_hash), None, None, ) .map_err(|e| napi::Error::from_reason(e.to_string()))?; - Ok(txn.into_iter().next().ok_or_else(|| napi::Error::from_reason("No transaction found"))?) + txn.into_iter().next().ok_or_else(|| napi::Error::from_reason(format!("No transaction found for payment hash: {}", payment_hash))) }
108-145: Consider implementing or removing the test module.There's a large block of commented-out test code. This should either be implemented with the proper CLN test configuration or removed.
If you plan to use this test module, update it for CLN testing with proper URLs and authentication. Otherwise, consider removing it to keep the codebase clean.
crates/lni/cln/lib.rs (4)
64-75: Rename method to match functionality.The
list_transactionsmethod actually callslookup_invoiceinternally. This can be confusing. Consider renaming the method or using the correct API call.pub async fn list_transactions( &self, params: ListTransactionsParams, ) -> Result<Vec<crate::Transaction>, ApiError> { - crate::cln::api::lookup_invoice( + crate::cln::api::list_transactions( self.url.clone(), self.rune.clone(), - None, - Some(params.from), - Some(params.limit), + params.from, + params.limit, ) }
119-120: Fix error message in test_get_info.The error message says "Failed to get offer" but it should be "Failed to get info".
- panic!("Failed to get offer: {:?}", e); + panic!("Failed to get info: {:?}", e);
142-143: Fix error message in test_pay_offer.The error message says "Failed to get offer" but it should be "Failed to pay offer".
- panic!("Failed to get offer: {:?}", e); + panic!("Failed to pay offer: {:?}", e);
173-176: Improve assertion in test_list_transactions.The current assertion
txns.len() >= 0is always true and doesn't provide meaningful validation. Consider a more specific test.- assert!( - txns.len() >= 0, - "Should contain at least zero or one transaction" - ); + // Print the count for debugging + println!("Found {} transactions", txns.len()); + + // Verify transaction structure if any exist + if !txns.is_empty() { + let first_txn = &txns[0]; + assert!(!first_txn.payment_hash.is_empty(), "Transaction should have a payment hash"); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
bindings/lni_nodejs/index.d.ts(1 hunks)bindings/lni_nodejs/main.mjs(1 hunks)bindings/lni_nodejs/src/cln.rs(1 hunks)bindings/lni_nodejs/src/lib.rs(1 hunks)bindings/lni_nodejs/src/phoenixd.rs(1 hunks)bindings/lni_react_native/example/src/App.tsx(3 hunks)bindings/lni_uniffi/src/lib.rs(1 hunks)bindings/lni_uniffi/src/lni.udl(3 hunks)crates/lni/cln/api.rs(1 hunks)crates/lni/cln/lib.rs(1 hunks)crates/lni/cln/types.rs(1 hunks)crates/lni/lib.rs(1 hunks)crates/lni/phoenixd/api.rs(10 hunks)crates/lni/phoenixd/lib.rs(5 hunks)crates/lni/phoenixd/types.rs(1 hunks)crates/lni/types.rs(3 hunks)readme.md(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- crates/lni/phoenixd/lib.rs
- bindings/lni_react_native/example/src/App.tsx
🧰 Additional context used
🪛 markdownlint-cli2 (0.17.2)
readme.md
118-118: Heading levels should only increment by one level at a time
Expected: h2; Actual: h3
(MD001, heading-increment)
118-118: Heading style
Expected: setext; Actual: atx
(MD003, heading-style)
119-119: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
138-138: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
139-139: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
142-142: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
279-279: Bare URL used
null
(MD034, no-bare-urls)
🪛 LanguageTool
readme.md
[uncategorized] ~141-~141: The official spelling of this programming framework is “Node.js”.
Context: ...rn build - testnode main.mjs` - nodejs - native modules (electron, vercel etc....
(NODE_JS)
[typographical] ~141-~141: Two consecutive dots
Context: ...s - native modules (electron, vercel etc..) - if you want to use the native no...
(DOUBLE_PUNCTUATION)
[grammar] ~255-~255: It appears that a hyphen is missing in the noun “To-do” (= task) or did you mean the verb “to do”?
Context: ...-->>App: PayInvoiceResponse object ``` Todo ==== - [X] make interface - [X] napi-rs...
(TO_DO_HYPHEN)
🔇 Additional comments (28)
bindings/lni_nodejs/src/lib.rs (3)
7-7: Improved type exports for better API clarityThe explicit export of specific types from the
lni::typesmodule provides better clarity about what types are available to consumers of this API. This is a good practice that makes the public interface more discoverable.
9-10: Logical module organizationMoving the
phoenixdmodule declaration after the public use declarations maintains good code organization, ensuring that its export (PhoenixdNode) remains accessible while keeping related code grouped together.
12-13: Good addition of Core Lightning supportThe addition of the
clnmodule and export ofClnNodeappropriately implements the Core Lightning functionality mentioned in the PR objectives. This is a well-structured way to add new functionality while keeping it separate from the existingphoenixdimplementation.bindings/lni_uniffi/src/lib.rs (1)
2-4: Simplified import structureReplacing specific imports with wildcard imports simplifies the code and makes it more maintainable. The addition of the new
lni::clnmodule import properly aligns with the Core Lightning integration goal of this PR.bindings/lni_uniffi/src/lni.udl (3)
99-100: Enhanced transaction metadata supportThe addition of
payer_noteandexternal_idfields to theTransactiondictionary provides useful metadata for transactions. Thepayer_notefield will be particularly valuable for bolt12 offers, whileexternal_idcan be used for bolt11 invoice identification.
199-199: Critical payment tracking field addedAdding the
payment_hashfield toPayInvoiceResponseis essential for tracking and identifying payments. This field is a standard identifier in the Lightning Network protocol and enhances the API's compliance with Lightning standards.
223-227: Simplified transaction filteringThe streamlined
ListTransactionsParamsdictionary now focuses on pagination (from,limit) and specific transaction lookup (payment_hash), removing unnecessary fields. This simplification makes the API more focused and easier to use.bindings/lni_nodejs/src/phoenixd.rs (2)
93-93: Updated parameter type for consistencyChanging the parameter type from
lni::phoenixd::lib::ListTransactionsParamstocrate::ListTransactionsParamsensures consistency with the simplified transaction params structure defined at the crate level.
100-100:❓ Verification inconclusive
Verify impact of parameter changes
Passing
Noneas theuntilparameter (and removing other parameters) aligns with the simplifiedListTransactionsParamsstructure. This change simplifies the API but could affect existing functionality.
🏁 Script executed:
#!/bin/bash # Check for any other usages of list_transactions to ensure they're compatible with the new parameter structure grep -r "list_transactions" --include="*.rs" --include="*.ts" . | grep -v "None" || echo "No other implementations found" # Check if there are any tests that might be affected by this change find . -name "*test*.rs" -o -name "*test*.ts" | xargs grep -l "list_transactions" || echo "No tests found for list_transactions"Length of output: 1156
Verify and confirm the new parameter usage across implementations
The change in
bindings/lni_nodejs/src/phoenixd.rs(passingNonefor theuntilparameter) aligns with the updated, simplifiedListTransactionsParamsdesign. However, please verify that:
- Other implementations (e.g., in
bindings/lni_nodejs/src/cln.rs,crates/lni/cln/*, andcrates/lni/phoenixd/*) correctly support this change.- The removal of additional parameters does not lead to unexpected behavior, especially since our search did not locate dedicated tests for
list_transactions.Ensure that downstream code calling this API is adjusted appropriately and consider adding relevant tests to cover this change if needed.
crates/lni/types.rs (3)
45-46: Good field additions for better transaction trackingThe new optional fields (
payer_noteandexternal_id) enhance the Transaction struct by providing support for both bolt12 and bolt11 protocol features. This addition properly maintains backward compatibility by making them optional.
174-174: Essential field addition to PayInvoiceResponseAdding the
payment_hashfield to PayInvoiceResponse is necessary for uniquely identifying payments in the Lightning Network, which is critical for payment tracking and lookups.
203-209: Well-structured pagination and filtering parametersThe new
ListTransactionsParamsstruct provides a clean interface for transaction listing with proper pagination (from,limit) and filtering (payment_hash) capabilities.crates/lni/lib.rs (2)
23-28: Good modular architecture expansionThe addition of the
typesmodule to thephoenixdnamespace and explicit exports of key structs improves the organization and accessibility of the code.
30-34: Excellent addition of Core Lightning supportThe introduction of the
clnmodule with its submodules (lib,api,types) establishes a clear parallel structure to the existingphoenixdmodule, facilitating multiple Lightning implementation support.bindings/lni_nodejs/main.mjs (4)
1-1: Proper import for new node implementationsAdding the
ClnNodeimport alongsidePhoenixdNodeproperly supports the multi-implementation architecture being introduced.
5-42: Well-structured code refactoringEncapsulating the phoenixd functionality into a dedicated function improves code organization and readability.
82-87: Clean main function implementationThe
main()function now properly orchestrates both implementations, allowing for parallel testing and execution.
44-80:✅ Verification successful
Good implementation of Core Lightning functionality
The
cln()function mirrors the phoenixd implementation, providing a consistent interface for interacting with Core Lightning nodes.
🌐 Web query:
Does Core Lightning use InvoiceType.Bolt11 as shown in line 59?💡 Result:
Core Lightning explicitly uses BOLT #11 as its standard for Lightning invoices, with its implementation deeply integrated into the codebase. The
bolt11terminology appears consistently across Core Lightning's JSON-RPC interface and documentation when handling invoices[1][3][12][13].Key evidence from the search results:
- The JSON-RPC
invoicecommand directly returns a"bolt11"field containing the BOLT #11-encoded invoice string[12][13].- Unit tests specifically reference
run-bolt11.cfor validating BOLT #11 functionality[1].- Core Lightning's documentation explicitly states: "returns some internal details, and a standard invoice string called bolt11 (named after the BOLT #11 lightning spec)"[12][13].
- Multiple live coding sessions focus on BOLT #11 implementation details in Core Lightning, including signature handling and bech32 encoding[1][4][7].
While the exact line number reference isn't visible in the provided sources, the pattern matches Core Lightning's convention of using
bolt11identifiers for BOLT #11-related code structures. The implementation follows the BOLT #11 specification from the Lightning Network RFCs[9], with explicit references to BOLT #11 tagged fields liker(routing info) ands(payment secrets) in the code analysis[4][7].Citations:
- 1: https://lnroom.live/2023-12-21-live-0019-core-lightning-implementation-of-bolt-11-invoices-part-3/
- 2: https://lnroom.live/2023-11-23-live-0017-core-lightning-implementation-of-bolt-11-invoices-part-1/
- 3: https://docs.lightning.engineering/the-lightning-network/payment-lifecycle/understanding-lightning-invoices
- 4: https://lnroom.live/2023-12-07-live-0018-core-lightning-implementation-of-bolt-11-invoices-part-2/
- 5: https://community.corelightning.org/c/video-archive/bolt-11-invoices-implementation-part-3
- 6: https://lnroom.live/2024-01-04-live-0020-core-lightning-implementation-of-bolt-11-invoices-part-4/
- 7: https://www.youtube.com/watch?v=3pP9bYQVsZg
- 8: https://www.youtube.com/playlist?list=PLseHpvCI1BjCYA9wd4YauzMueKg2acNi-
- 9: https://github.com/lightning/bolts/blob/master/11-payment-encoding.md
- 10: https://www.whatisbitcoin.com/lightning-network/bolt-11-vs-bolt-12
- 11: https://www.youtube.com/watch?v=FidG8O8TGxo
- 12: https://github.com/ElementsProject/lightning/blob/master/README.md
- 13: https://github.com/ElementsProject/lightning
- 14: https://community.corelightning.org/c/video-archive/bolt-11-invoices-implementation-part-4-4d535d-890269-437de7-b7dcc2
- 15: https://lnroom.live/2024-01-18-live-0021-core-lightning-implementation-of-bolt-11-invoices-part-5/
- 16: https://www.youtube.com/watch?v=Ttdm9j-Dt10
- 17: https://community.start9.com/t/core-lightning-v23-11-0-creating-incompatible-invoices/901
- 18: https://docs.corelightning.org/docs/sending-and-receiving-payments
- 19: https://www.voltage.cloud/blog/what-are-bolts-lightning-network
Verified: Core Lightning Invoice Type Usage Correct
The usage of
InvoiceType.Bolt11in thecln()function is consistent with Core Lightning's implementation of the BOLT #11 standard. The code snippet correctly mirrors the structure seen in related implementations (e.g., phoenixd) and adheres to the established JSON-RPC conventions where BOLT #11 invoice strings are used.
- File:
bindings/lni_nodejs/main.mjs(Lines: 44-80)- Observation: The call to
node.makeInvoicewithinvoiceType: InvoiceType.Bolt11correctly aligns with Core Lightning's specification.No modifications are necessary.
crates/lni/phoenixd/types.rs (5)
1-7: Good basic info response structureThe
InfoResponsestruct properly handles JSON field mapping between camelCase (nodeId) and snake_case (node_id), following Rust conventions.
9-21: Well-designed invoice request structureThe
Bolt11Reqstruct contains all necessary fields for creating Lightning invoices with proper JSON field mapping.
23-49: Comprehensive invoice response structureThe
InvoiceResponsestruct captures all relevant details about an invoice, including payment tracking fields, timestamps, and protocol-specific fields (payer_note for bolt12, external_id for bolt11).
51-73: Complete outgoing payment response structureThe
OutgoingPaymentResponsestruct properly mirrors the invoice response structure but focuses on outgoing payment details, maintaining consistency in the API.
75-85: Concise payment response structureThe
PayResponsestruct provides the essential payment information needed after a successful payment operation.crates/lni/cln/types.rs (2)
5-6: Action Required: Remove Unused Commented CodeThe commented-out lines for the
node_idfield are not being used. Either uncomment and fully implement this field if needed, or remove these lines to keep the code clean.
25-26: Payment Amount Type: Switch to UnsignedPayment amounts are inherently non-negative values. It would be more precise and safer to use
u64instead ofi64for the payment amount fields.- pub amount_msat: i64, - pub amount_sent_msat: i64, + pub amount_msat: u64, + pub amount_sent_msat: u64,readme.md (1)
205-254: Excellent Sequence DiagramThe addition of the sequence diagram is excellent and provides valuable context for understanding the architecture and interactions between components. This significantly improves the documentation quality.
bindings/lni_nodejs/index.d.ts (1)
17-25: Well-Designed ClnNode Class ImplementationThe new
ClnNodeclass follows a consistent interface pattern matching the existingPhoenixdNodeclass. This is an excellent design choice that will make it easy for developers to switch between different node implementations without changing their code significantly.The method signatures are consistent, and the class provides all the necessary functionality for interacting with a Core Lightning node.
crates/lni/cln/lib.rs (1)
95-98: Missing environment variable in README.The test uses
PHOENIX_MOBILE_OFFERenvironment variable, but this isn't documented in the README.#!/bin/bash # Check if PHOENIX_MOBILE_OFFER is documented in the README grep -r "PHOENIX_MOBILE_OFFER" --include="README*" .Please add the
PHOENIX_MOBILE_OFFERenvironment variable to the.envsection in the README to ensure developers can run the tests successfully.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
readme.md (5)
107-112: Fix Typographical and Formatting Errors in Troubleshooting Section
The header`*troubleshooting react-natve`:contains a typo ("react-natve" should be "react-native") and appears to have loose punctuation. Please correct the spelling and consider refining the formatting (and list indentations) for better readability.🧰 Tools
🪛 LanguageTool
[uncategorized] ~107-~107: Loose punctuation mark.
Context: ...tart ```*troubleshooting react-natve: - if you get an error like `uniffiEnsu...(UNLIKELY_OPENING_PUNCTUATION)
🪛 markdownlint-cli2 (0.17.2)
110-110: Unordered list indentation
Expected: 2; Actual: 4(MD007, ul-indent)
111-111: Unordered list indentation
Expected: 2; Actual: 4(MD007, ul-indent)
146-147: Correct Naming and Formatting for Node.js Section
The bullet point reads “- nodejs - native modules (electron, vercel etc..)”. Please replace “nodejs” with the officially styled “Node.js” and remove the extra period (use “etc.” instead). This ensures consistency and clarity.🧰 Tools
🪛 LanguageTool
[uncategorized] ~146-~146: The official spelling of this programming framework is “Node.js”.
Context: ...rn build- testnode main.mjs` - nodejs - native modules (electron, vercel etc....(NODE_JS)
[typographical] ~146-~146: Two consecutive dots
Context: ...s - native modules (electron, vercel etc..) - if you want to use the native no...(DOUBLE_PUNCTUATION)
🪛 markdownlint-cli2 (0.17.2)
147-147: Unordered list indentation
Expected: 2; Actual: 4(MD007, ul-indent)
196-196: Clarify Tor Socks5 Proxy Instructions
The note “Use Tor socks if connecting to a .onion hidden service by passing in socks5 proxy. (TODO WIP)” is useful as a reminder. Consider providing further guidance or an example on how to configure the SOCKS5 proxy, or remove the TODO if it’s not planned for the near term.
268-277: Update Todo List Formatting and Content
The updated Todo list now includes new tasks (such as “Tor Socks5 fetch” and additional lightning node implementations). Please review the indentation and consistency of the list markers, as markdownlint has flagged some unordered list indentation issues. Ensure that the task statuses accurately reflect the current work plan.🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
270-270: Unordered list indentation
Expected: 2; Actual: 4(MD007, ul-indent)
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)
282-285: Refine To Research Section Formatting
In the “To Research” section, bare URLs are used (e.g., in the items regarding napi-rs and the Facade REST API). Consider replacing these with proper markdown links to improve readability and comply with markdown best practices. Also, check the list indentation to adhere to markdownlint guidelines.🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
282-282: Bare URL used
null(MD034, no-bare-urls)
285-285: Bare URL used
null(MD034, no-bare-urls)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
bindings/lni_react_native/yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (7)
bindings/lni_react_native/LniFramework.xcframework/Info.plist(2 hunks)bindings/lni_react_native/build.sh(0 hunks)bindings/lni_react_native/example/src/App.tsx(4 hunks)bindings/lni_react_native/package.json(2 hunks)bindings/lni_react_native/ubrn.config.yaml(1 hunks)crates/lni/lib.rs(1 hunks)readme.md(5 hunks)
💤 Files with no reviewable changes (1)
- bindings/lni_react_native/build.sh
✅ Files skipped from review due to trivial changes (2)
- bindings/lni_react_native/package.json
- bindings/lni_react_native/ubrn.config.yaml
🚧 Files skipped from review as they are similar to previous changes (2)
- bindings/lni_react_native/example/src/App.tsx
- crates/lni/lib.rs
🧰 Additional context used
🪛 LanguageTool
readme.md
[uncategorized] ~107-~107: Loose punctuation mark.
Context: ...tart ``` *troubleshooting react-natve: - if you get an error like `uniffiEnsu...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~146-~146: The official spelling of this programming framework is “Node.js”.
Context: ...rn build - testnode main.mjs` - nodejs - native modules (electron, vercel etc....
(NODE_JS)
[typographical] ~146-~146: Two consecutive dots
Context: ...s - native modules (electron, vercel etc..) - if you want to use the native no...
(DOUBLE_PUNCTUATION)
[grammar] ~260-~260: It appears that a hyphen is missing in the noun “To-do” (= task) or did you mean the verb “to do”?
Context: ...-->>App: PayInvoiceResponse object ``` Todo ==== - [X] make interface - [X] napi-rs...
(TO_DO_HYPHEN)
[uncategorized] ~264-~264: The official spelling of this programming framework is “Node.js”.
Context: ... - [X] make interface - [X] napi-rs for nodejs - [X] uniffi bindings for Android and I...
(NODE_JS)
🪛 markdownlint-cli2 (0.17.2)
readme.md
110-110: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
111-111: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
123-123: Heading levels should only increment by one level at a time
Expected: h2; Actual: h3
(MD001, heading-increment)
123-123: Heading style
Expected: setext; Actual: atx
(MD003, heading-style)
124-124: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
143-143: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
144-144: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
147-147: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
270-270: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
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)
282-282: Bare URL used
null
(MD034, no-bare-urls)
285-285: Bare URL used
null
(MD034, no-bare-urls)
🔇 Additional comments (5)
bindings/lni_react_native/LniFramework.xcframework/Info.plist (1)
11-11: Properly configured platform identifiers for iOS builds.The changes correctly reorganize the framework architecture support by:
- Distinguishing between simulator and device builds for iOS arm64 architecture
- Adding explicit simulator variant identification with
SupportedPlatformVariant- Ensuring appropriate library identifiers for each target platform
This configuration follows Apple's XCFramework best practices and will ensure the correct binary is used when building for simulator versus physical device.
Also applies to: 20-21, 27-27
readme.md (4)
125-134: Confirm Environment Variables Update
New environment variables (TEST_RECEIVER_OFFER,PHOENIX_MOBILE_OFFER,CLN_URL,CLN_RUNE,CLN_TEST_PAYMENT_HASH) have been added. Ensure that these updates are consistent with related changes in code and documentation elsewhere in the project.
143-143: Enhanced Build Command for Node.js Bindings
The updated command (cd bindings/lni_nodejs && cargo clean && cargo build --release && yarn && yarn build) now enforces a release build and cleans the previous builds. This is a welcome improvement; please verify it aligns with your overall build workflow.🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
143-143: Unordered list indentation
Expected: 2; Actual: 4(MD007, ul-indent)
148-158: Review Native Module Usage Example
The code snippet clearly demonstrates how to load a native Node.js module based on the platform and architecture. Just confirm that the constructed file path (../../code/lni/bindings/lni_nodejs/lni_js.${platform}-${arch}.node) is correct relative to the repository root.
210-259: New LNI Sequence Diagram Integration
The added LNI Sequence Diagram effectively visualizes interactions among the Application, Binding, LNI Core, and Lightning Node components. This visual aid is a valuable enhancement for the documentation. Please verify that the Mermaid syntax renders correctly in all intended environments.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
readme.md (6)
107-114: Fix Typo and Improve Troubleshooting Section Formatting
The heading "*troubleshooting react-natve:" contains a typographical error and an informal style. Consider updating it to use a standard markdown heading (e.g., "### Troubleshooting React Native:") and refine the list items to avoid repetitive phrases (like repeatedly saying “for ios”).-`*troubleshooting react-natve`: +### Troubleshooting React Native:🧰 Tools
🪛 LanguageTool
[uncategorized] ~107-~107: Loose punctuation mark.
Context: ...tart ```*troubleshooting react-natve: - if you get an error like `uniffiEnsu...(UNLIKELY_OPENING_PUNCTUATION)
[grammar] ~109-~109: This phrase is duplicated. You should probably use “for ios” only once.
Context: ...swipe away app) - try updating the pods for ioscd example/ios && pod install --repo-update && cd ../- for ios open the xcode app - lni/bindings/lni_r...(PHRASE_REPETITION)
[uncategorized] ~114-~114: The official spelling of this programming framework is “Node.js”.
Context: ...talling the app from the mobile device nodejs ``` cd bindings/lni_nodejs cat main.mj...(NODE_JS)
🪛 markdownlint-cli2 (0.17.2)
111-111: Unordered list indentation
Expected: 2; Actual: 4(MD007, ul-indent)
112-112: Unordered list indentation
Expected: 2; Actual: 4(MD007, ul-indent)
124-136: Enhance the .env Section Formatting
The ".env" section would benefit from a more explicit code block language and consistent heading hierarchy. Specifying a language identifier (e.g.,dotenv) in the fenced code block improves readability and helps with syntax highlighting.-``` +```dotenv🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
124-124: Heading levels should only increment by one level at a time
Expected: h2; Actual: h3(MD001, heading-increment)
124-124: Heading style
Expected: setext; Actual: atx(MD003, heading-style)
125-125: Fenced code blocks should have a language specified
null(MD040, fenced-code-language)
141-146: Correct Unordered List Indentation in the Node.js Build Command
The build command line for the Node.js bindings is over-indented relative to its parent list item. To comply with markdown linting guidelines (MD007), please adjust the indentation (typically 2 spaces for nested items) for improved consistency and readability.- - `cd bindings/lni_nodejs && cargo clean && cargo build --release && yarn && yarn build` + - `cd bindings/lni_nodejs && cargo clean && cargo build --release && yarn && yarn build`🧰 Tools
🪛 LanguageTool
[uncategorized] ~141-~141: The official spelling of this programming framework is “Node.js”.
Context: ...ASH=YOUR_HASH ``` Bindings ======== - nodejs - napi_rs - https://napi.rs/do...(NODE_JS)
🪛 markdownlint-cli2 (0.17.2)
142-142: Unordered list indentation
Expected: 2; Actual: 4(MD007, ul-indent)
143-143: Unordered list indentation
Expected: 2; Actual: 4(MD007, ul-indent)
143-143: Bare URL used
null(MD034, no-bare-urls)
144-144: Unordered list indentation
Expected: 2; Actual: 4(MD007, ul-indent)
145-145: Unordered list indentation
Expected: 2; Actual: 4(MD007, ul-indent)
147-160: Standardize Framework Naming and Punctuation in the Native Modules Section
References to “nodejs” should be updated to “Node.js” (notice the proper capitalization and punctuation). Additionally, adjust punctuation such as “etc..” to “etc.” and consider capitalizing technology names (e.g., Electron, Vercel) for clarity.-- nodejs - native modules (electron, vercel etc..) +- Node.js - native modules (Electron, Vercel, etc.)🧰 Tools
🪛 LanguageTool
[uncategorized] ~147-~147: The official spelling of this programming framework is “Node.js”.
Context: ...rn build- testnode main.mjs` - nodejs - native modules (electron, vercel etc....(NODE_JS)
[typographical] ~147-~147: Two consecutive dots
Context: ...s - native modules (electron, vercel etc..) - if you want to use the native no...(DOUBLE_PUNCTUATION)
🪛 markdownlint-cli2 (0.17.2)
148-148: Unordered list indentation
Expected: 2; Actual: 4(MD007, ul-indent)
269-278: Refine Todo List Indentation and Consistency
The todo items in this section (e.g., "Tor Socks5 fetch" and the sub-items under "implement lightning nodes") have inconsistent indentation according to markdown guidelines. Adjust the nested bullet points (using 2 spaces per level) to improve structure and readability.- - [X] phoenixd - - [X] cln + - [X] phoenixd + - [X] cln🧰 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)
283-287: Format Bare URLs in the "To Research" Section
The “To Research” section includes bare URLs. For better readability and usability, convert these into proper markdown links. For example, replace
https://napi.rs/docs/introduction/simple-package
with something like[napi-rs docs](https://napi.rs/docs/introduction/simple-package).- - [X] napi-rs https://napi.rs/docs/introduction/simple-package + - [X] napi-rs [docs](https://napi.rs/docs/introduction/simple-package)- - [ ] Facade REST API? - Use the same api as phoenixd https://phoenix.acinq.co/server/api as a facade in front of any lightning node implementation. + - [ ] Facade REST API? - Use the same API as Phoenixd ([phoenix.acinq.co/server/api](https://phoenix.acinq.co/server/api)) as a facade in front of any lightning node implementation.🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
283-283: Bare URL used
null(MD034, no-bare-urls)
286-286: Bare URL used
null(MD034, no-bare-urls)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
bindings/lni_react_native/build.sh(1 hunks)bindings/lni_react_native/example/src/App.tsx(4 hunks)readme.md(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- bindings/lni_react_native/build.sh
- bindings/lni_react_native/example/src/App.tsx
🧰 Additional context used
🪛 LanguageTool
readme.md
[uncategorized] ~107-~107: Loose punctuation mark.
Context: ...tart ``` *troubleshooting react-natve: - if you get an error like `uniffiEnsu...
(UNLIKELY_OPENING_PUNCTUATION)
[grammar] ~109-~109: This phrase is duplicated. You should probably use “for ios” only once.
Context: ...swipe away app) - try updating the pods for ios cd example/ios && pod install --repo-update && cd ../ - for ios open the xcode app - lni/bindings/lni_r...
(PHRASE_REPETITION)
[uncategorized] ~147-~147: The official spelling of this programming framework is “Node.js”.
Context: ...rn build - testnode main.mjs` - nodejs - native modules (electron, vercel etc....
(NODE_JS)
[typographical] ~147-~147: Two consecutive dots
Context: ...s - native modules (electron, vercel etc..) - if you want to use the native no...
(DOUBLE_PUNCTUATION)
[grammar] ~261-~261: It appears that a hyphen is missing in the noun “To-do” (= task) or did you mean the verb “to do”?
Context: ...-->>App: PayInvoiceResponse object ``` Todo ==== - [X] make interface - [X] napi-rs...
(TO_DO_HYPHEN)
[uncategorized] ~265-~265: The official spelling of this programming framework is “Node.js”.
Context: ... - [X] make interface - [X] napi-rs for nodejs - [X] uniffi bindings for Android and I...
(NODE_JS)
🪛 markdownlint-cli2 (0.17.2)
readme.md
111-111: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
112-112: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
124-124: Heading levels should only increment by one level at a time
Expected: h2; Actual: h3
(MD001, heading-increment)
124-124: Heading style
Expected: setext; Actual: atx
(MD003, heading-style)
125-125: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
144-144: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
145-145: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
148-148: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
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)
283-283: Bare URL used
null
(MD034, no-bare-urls)
286-286: Bare URL used
null
(MD034, no-bare-urls)
There was a problem hiding this comment.
Actionable comments posted: 10
♻️ Duplicate comments (7)
crates/lni/phoenixd/api.rs (2)
240-243:⚠️ Potential issueCritical: Inconsistent Handling of the
descriptionFieldThe current implementation in
crates/lni/phoenixd/api.rs(lines 240-243) still hard-codes the description as an empty string, which deviates from the pattern observed elsewhere where the field is set from the invoice usingunwrap_or_default().This inconsistency suggests that the hard-coded empty string might be an unintentional regression.
Apply this diff to fix the issue:
- description: "".to_string(), + description: inv.description.unwrap_or_default(),
295-298: 🛠️ Refactor suggestionConsistent handling of optional fields in transactions
The code now correctly handles the optional
payer_noteandexternal_idfields for outgoing transactions, similar to how they're handled for incoming transactions. This ensures consistency in how transaction data is processed.However, note that the
descriptionfield is still set to an empty string instead of using data from the response, which differs from the handling of incoming transactions.- description: "".to_string(), + description: payment.description.unwrap_or_default(),crates/lni/cln/lib.rs (1)
63-78:⚠️ Potential issueFix error handling in lookup_invoice.
The function still uses
.unwrap()at line 74 which can cause runtime panics. This was previously reported and should be fixed with proper error handling.pub async fn lookup_invoice( &self, payment_hash: String, ) -> Result<crate::Transaction, ApiError> { let transactions = crate::cln::api::lookup_invoice( self.url.clone(), self.rune.clone(), Some(payment_hash), None, None, ) - .unwrap(); + .map_err(|e| ApiError::Http { reason: format!("Failed to lookup invoice: {}", e) })?; transactions.into_iter().next().ok_or(ApiError::Json { reason: "No transactions found".to_string(), }) }crates/lni/cln/api.rs (4)
135-167:⚠️ Potential issueUse async client for async function and improve error handling in pay_offer.
The function is marked as
asyncbut uses blocking HTTP methods. Additionally, all.unwrap()calls should be replaced with proper error handling.pub async fn pay_offer( url: String, rune: String, offer: String, amount_msats: i64, payer_note: Option<String>, ) -> Result<PayInvoiceResponse, ApiError> { let fetch_invoice_url = format!("{}/v1/fetchinvoice", url); - let client = clnrest_client(rune); + let client = clnrest_async_client(rune).await?; - let response: reqwest::blocking::Response = client + let response = client .post(&fetch_invoice_url) .header("Content-Type", "application/json") .json(&serde_json::json!({ "offer": offer, "amount_msat": amount_msats, "payer_note": payer_note, "timeout": 60, })) .send() - .unwrap(); - let response_text = response.text().unwrap(); - let response_text = response_text.as_str(); + .await.map_err(|e| ApiError::Http { reason: e.to_string() })?; + let response_text = response.text() + .await + .map_err(|e| ApiError::Http { reason: e.to_string() })?; let fetch_invoice_resp: FetchInvoiceResponse = match serde_json::from_str(&response_text) { Ok(resp) => { - println!("fetch_invoice_resp: {:?}", resp); + log::debug!("fetch_invoice_resp: {:?}", resp); resp }You'll also need to implement an async version of the client builder:
async fn clnrest_async_client(rune: String) -> Result<reqwest::Client, ApiError> { let mut headers = reqwest::header::HeaderMap::new(); headers.insert( "Rune", header::HeaderValue::from_str(&rune) .map_err(|e| ApiError::Http { reason: e.to_string() })? ); reqwest::ClientBuilder::new() .danger_accept_invalid_certs(true) // Consider making this configurable .default_headers(headers) .build() .map_err(|e| ApiError::Http { reason: e.to_string() }) }
175-196:⚠️ Potential issueUse async methods and proper error handling for payment request.
Similar to the fetch invoice request, this code needs to use async methods and proper error handling to avoid unwraps.
// now pay the bolt 12 invoice lni let pay_url = format!("{}/v1/pay", url); -let pay_response: reqwest::blocking::Response = client +let pay_response = client .post(&pay_url) .header("Content-Type", "application/json") .json(&serde_json::json!({ "bolt11": fetch_invoice_resp.invoice.to_string(), - "maxfeepercent": 1, // TODO read from config + "maxfeepercent": 1, // TODO: Make this configurable "retry_for": 60, })) .send() - .unwrap(); -let pay_response_text = pay_response.text().unwrap(); -let pay_response_text = pay_response_text.as_str(); + .await + .map_err(|e| ApiError::Http { reason: e.to_string() })?; +let pay_response_text = pay_response.text() + .await + .map_err(|e| ApiError::Http { reason: e.to_string() })?; let pay_resp: PayResponse = match serde_json::from_str(&pay_response_text) { Ok(resp) => resp, Err(e) => { + log::error!("Failed to parse pay response: {:?}", e); return Err(ApiError::Json { reason: pay_response_text.to_string(), }) } };
230-242:⚠️ Potential issueImprove error handling in HTTP request and remove commented-out code.
The HTTP request uses
.unwrap()which can cause runtime panics. Also, there's commented code that should be removed.// Fetch incoming transactions let response: reqwest::blocking::Response = client .post(&list_invoices_url) .header("Content-Type", "application/json") - //.json(&serde_json::json!(params)) .json(&serde_json::json!(params .into_iter() .filter_map(|(k, v)| v.map(|v| (k, v))) .collect::<serde_json::Value>())) .send() - .unwrap(); -let response_text = response.text().unwrap(); -let response_text = response_text.as_str(); -let incoming_payments: InvoicesResponse = serde_json::from_str(&response_text).unwrap(); + .map_err(|e| ApiError::Http { reason: e.to_string() })?; +let response_text = response.text() + .map_err(|e| ApiError::Http { reason: e.to_string() })?; +let incoming_payments: InvoicesResponse = serde_json::from_str(&response_text) + .map_err(|e| ApiError::Json { reason: e.to_string() })?;
256-257:⚠️ Potential issueImplement the created_at field.
The created_at field is set to 0 with a TODO comment. This should be properly implemented to ensure transactions are sorted correctly.
- created_at: 0, // TODO + created_at: inv.paid_at.unwrap_or_else(|| inv.expires_at - (expiry.unwrap_or(3600) as u64)),The suggested implementation attempts to calculate a creation time based on either the payment time or by subtracting the expiry period from the expiration time.
🧹 Nitpick comments (7)
crates/lni/cln/types.rs (2)
31-36: Use more descriptive field name foroutnumThe field
outnuminPaidOutpointstruct is not immediately clear in its purpose. Consider using a more descriptive name likeoutput_indexorvoutto better reflect that this is a transaction output index.
40-58: Add more specific status type enumeration for invoicesThe comment on line 44 indicates that
statuscan have values like "paid", "unpaid", or "expired". Consider defining an enum for this field rather than using a String to enforce valid status values at compile time.+#[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>, // Other fields remain unchangedreadme.md (1)
107-113: Improved troubleshooting instructions for React NativeThe expanded troubleshooting section provides valuable steps for resolving common React Native integration issues, making it easier for developers to work with the library.
However, there's a typo in "react-natve" that should be corrected to "react-native".
-`*troubleshooting react-natve`: +`*troubleshooting react-native`:🧰 Tools
🪛 LanguageTool
[uncategorized] ~107-~107: Loose punctuation mark.
Context: ...tart ```*troubleshooting react-natve: - if you get an error like `uniffiEnsu...(UNLIKELY_OPENING_PUNCTUATION)
[grammar] ~109-~109: This phrase is duplicated. You should probably use “for ios” only once.
Context: ...swipe away app) - try updating the pods for ioscd example/ios && pod install --repo-update && cd ../- for ios open the xcode app - lni/bindings/lni_r...(PHRASE_REPETITION)
🪛 markdownlint-cli2 (0.17.2)
111-111: Unordered list indentation
Expected: 2; Actual: 4(MD007, ul-indent)
112-112: Unordered list indentation
Expected: 2; Actual: 4(MD007, ul-indent)
bindings/lni_nodejs/src/cln.rs (3)
42-59: Review parameter cloning in create_invoice methodThe
create_invoicemethod clones the URL and rune for each API call. Consider passing references instead of cloning these values, especially if they might be large strings.let txn = lni::cln::api::create_invoice( - self.inner.url.clone(), - self.inner.rune.clone(), + &self.inner.url, + &self.inner.rune, params.invoice_type, params.amount_msats, params.description, params.description_hash, params.expiry, )This would require updating the signature of the underlying API function to accept string references instead of owned strings.
74-90: pay_offer method has inconsistent formattingThere's inconsistent formatting in the
pay_offermethod. The closing parenthesis for thepay_offercall is followed by.awaiton the next line.let offer = lni::cln::api::pay_offer( self.inner.url.clone(), self.inner.rune.clone(), offer, amount_msats, payer_note, -).await +) +.await .map_err(|e| napi::Error::from_reason(e.to_string()))?;
108-145: Remove or update commented test codeThere's a large block of commented-out test code. This should either be updated to test CLN functionality and uncommented, or removed if it's no longer needed. Commented test code can become outdated and confusing.
If this test code was for PhoenixdNode (as indicated by the variable names), it should be updated to test ClnNode functionality:
// #[cfg(test)] // mod tests { // use super::*; // use dotenv::dotenv; // use lazy_static::lazy_static; // use std::env; // use tokio::test; // // lazy_static! { // static ref URL: String = { // dotenv().ok(); // env::var("CLN_URL").expect("CLN_URL must be set") // }; // static ref RUNE: String = { // dotenv().ok(); // env::var("CLN_RUNE").expect("CLN_RUNE must be set") // }; // static ref NODE: ClnNode = { // ClnNode::new(ClnConfig { // url: URL.clone(), // rune: RUNE.clone(), // }) // }; // } // // #[test] // async fn test_get_info() { // match NODE.get_info().await { // Ok(info) => { // println!("info: {:?}", info.pubkey); // assert!(!info.pubkey.is_empty(), "Node pubkey should not be empty"); // } // Err(e) => { // panic!("Failed to get info: {:?}", e); // } // } // } // }crates/lni/cln/lib.rs (1)
137-138: Improve error message in get_info test.The panic message at line 137 says "Failed to get offer" but it should be "Failed to get node info".
- panic!("Failed to get offer: {:?}", e); + panic!("Failed to get node info: {:?}", e);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
bindings/lni_nodejs/index.d.ts(5 hunks)bindings/lni_nodejs/index.js(1 hunks)bindings/lni_nodejs/main.mjs(1 hunks)bindings/lni_nodejs/src/cln.rs(1 hunks)bindings/lni_nodejs/src/phoenixd.rs(3 hunks)bindings/lni_react_native/example/src/App.tsx(5 hunks)bindings/lni_uniffi/src/lni.udl(5 hunks)crates/lni/Cargo.toml(1 hunks)crates/lni/cln/api.rs(1 hunks)crates/lni/cln/lib.rs(1 hunks)crates/lni/cln/types.rs(1 hunks)crates/lni/lib.rs(1 hunks)crates/lni/phoenixd/api.rs(12 hunks)crates/lni/phoenixd/lib.rs(7 hunks)crates/lni/phoenixd/types.rs(1 hunks)crates/lni/types.rs(3 hunks)readme.md(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- bindings/lni_nodejs/index.js
- bindings/lni_react_native/example/src/App.tsx
- bindings/lni_nodejs/index.d.ts
🧰 Additional context used
🪛 LanguageTool
readme.md
[uncategorized] ~107-~107: Loose punctuation mark.
Context: ...tart ``` *troubleshooting react-natve: - if you get an error like `uniffiEnsu...
(UNLIKELY_OPENING_PUNCTUATION)
[grammar] ~109-~109: This phrase is duplicated. You should probably use “for ios” only once.
Context: ...swipe away app) - try updating the pods for ios cd example/ios && pod install --repo-update && cd ../ - for ios open the xcode app - lni/bindings/lni_r...
(PHRASE_REPETITION)
[uncategorized] ~147-~147: The official spelling of this programming framework is “Node.js”.
Context: ...rn build - testnode main.mjs` - nodejs - native modules (electron, vercel etc....
(NODE_JS)
[typographical] ~147-~147: Two consecutive dots
Context: ...s - native modules (electron, vercel etc..) - if you want to use the native no...
(DOUBLE_PUNCTUATION)
[grammar] ~261-~261: It appears that a hyphen is missing in the noun “To-do” (= task) or did you mean the verb “to do”?
Context: ...-->>App: PayInvoiceResponse object ``` Todo ==== - [X] make interface - [X] napi-rs...
(TO_DO_HYPHEN)
[uncategorized] ~265-~265: The official spelling of this programming framework is “Node.js”.
Context: ... - [X] make interface - [X] napi-rs for nodejs - [X] uniffi bindings for Android and I...
(NODE_JS)
🪛 markdownlint-cli2 (0.17.2)
readme.md
111-111: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
112-112: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
124-124: Heading levels should only increment by one level at a time
Expected: h2; Actual: h3
(MD001, heading-increment)
124-124: Heading style
Expected: setext; Actual: atx
(MD003, heading-style)
125-125: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
144-144: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
145-145: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
148-148: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
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)
283-283: Bare URL used
null
(MD034, no-bare-urls)
286-286: Bare URL used
null
(MD034, no-bare-urls)
🔇 Additional comments (41)
crates/lni/Cargo.toml (1)
28-28: Good addition of the rand crate.Adding the rand crate (version 0.8) is appropriate for the implementation of ClnNode, as it will likely be used for generating random values needed in cryptographic operations common in Lightning Network implementations.
crates/lni/lib.rs (2)
26-28: Well-structured module organization.Good refactoring of the phoenixd module to include a dedicated types submodule and cleaner exports.
30-35: Good implementation of the cln module.The structure of the cln module mirrors the phoenixd module, which maintains consistency in the codebase. The exports correctly include ClnConfig and ClnNode from the lib submodule.
bindings/lni_nodejs/main.mjs (2)
5-42: Good refactoring of phoenixd functionality.The phoenixd functionality has been properly encapsulated in its own function with clear steps for configuration, node information retrieval, invoice creation, and transaction listing.
44-87: Well-implemented cln functionality.The cln function follows the same pattern as phoenixd, providing consistent behavior between both implementations. The code correctly utilizes environment variables for configuration.
crates/lni/types.rs (4)
45-46: Good addition of optional fields for transaction details.Adding payer_note and external_id as optional fields enhances the Transaction struct with better support for both bolt12 and bolt11 formats.
174-174: Critical payment_hash field added to PayInvoiceResponse.Adding the payment_hash field to PayInvoiceResponse is essential as it provides a unique identifier for payments in the Lightning Network.
203-209: Well-designed ListTransactionsParams struct.The ListTransactionsParams struct provides a clean interface for pagination and filtering of transactions with sensible default fields.
212-220: Flexible CreateInvoiceParams struct.The CreateInvoiceParams struct is well-designed with appropriate optional fields that provide flexibility in invoice creation for different use cases.
crates/lni/phoenixd/lib.rs (4)
35-48: Function rename:make_invoice→create_invoiceThe function has been renamed from
make_invoicetocreate_invoicewith a parameter type change fromPhoenixdMakeInvoiceParamstoCreateInvoiceParams. This API change appears to be part of a broader standardization across the codebase.When deploying, ensure all calling code has been updated to use the new function name.
74-85: Simplified list_transactions parametersThe
list_transactionsimplementation now passesNonefor the omitted parameters, which simplifies the API. This change aligns with the modified function signature in the API module.
136-177: Test updated to reflect API changesThe test function has been properly updated to use the new
create_invoicemethod name and the newCreateInvoiceParamsstructure, with both Bolt11 and Bolt12 test cases covered.
117-120:✅ Verification successful
Environment variable renamed: TEST_OFFER → TEST_RECEIVER_OFFER
The static reference and environment variable have been renamed from
TEST_OFFERtoTEST_RECEIVER_OFFER. Make sure this environment variable is updated in all deployment environments.
🏁 Script executed:
#!/bin/bash # Check if any references to the old environment variable name still exist rg "TEST_OFFER" --type rustLength of output: 27
I'm generating an additional verification script to search the entire repository for any lingering references to the old environment variable. This will help ensure that "TEST_OFFER" isn’t accidentally present in non‑Rust files (e.g., configuration or deployment scripts).
Please run this script and verify its output. Based on the result, if no references are found, the renaming from
TEST_OFFERtoTEST_RECEIVER_OFFERappears consistent. If any instances are found, further investigation into those files will be needed to complete the renaming process.
🏁 Script executed:
#!/bin/bash # Search the entire repository (all file types) for references to the old environment variable name "TEST_OFFER" rg "TEST_OFFER"Length of output: 15
Environment variable renaming verified – please confirm deployment configs
The code changes (in
crates/lni/phoenixd/lib.rsat lines 117–120) correctly update the static reference to useTEST_RECEIVER_OFFER, and our repository scans show no remaining references to the old name (TEST_OFFER). Please ensure that all deployment and CI/CD configurations are updated to use the new environment variable name.bindings/lni_nodejs/src/phoenixd.rs (3)
1-1: Updated import for new parameter structureThe import has been updated to use
CreateInvoiceParamswhich aligns with the changes in the core Rust implementation.
42-59: Node.js binding updated to match Rust API changesThe Node.js binding function has been renamed from
make_invoicetocreate_invoiceand updated to use the new parameter structure. This ensures consistency between the Rust implementation and the Node.js bindings.
91-100: Simplified list_transactions parameters in Node.js bindingThe
list_transactionsmethod has been updated to usecrate::ListTransactionsParamsand now passesNonefor the omitted parameter, which aligns with the changes in the core Rust implementation.crates/lni/phoenixd/types.rs (1)
1-101: New types module with well-structured data modelsThis new file introduces several well-structured data models with proper serialization/deserialization mappings between Rust and JSON field names. The structs follow a consistent pattern and provide clear field documentation where needed (e.g., which fields are used in Bolt11 vs. Bolt12 scenarios).
Some observations:
- All structs properly derive
Debugfor improved logging- JSON field names are correctly mapped using
serdeattributes- Types that need to be exposed to Node.js are properly annotated with the
napimacroThis is a good refactoring that improves code organization by moving types to their own module.
crates/lni/phoenixd/api.rs (6)
1-4: Updated imports to use new types moduleThe imports now reference the types from the newly created
types.rsmodule, which improves code organization.
32-40: Function rename:make_invoice→create_invoicewith optional amountThe function has been renamed from
make_invoicetocreate_invoiceand theamount_msatsparameter has been changed from a requiredi64to anOption<i64>. This change allows the function to handle cases where the amount is not specified, like for Bolt12 invoices.
48-48: Safe handling of optional amount_msatsThe code now uses
unwrap_or_default()to ensure thatamount_satis set to zero ifamount_msatsisNone. This change makes the code more robust by handling optional values properly.
77-85: Added payer_note and external_id fields to Transaction structNew fields have been added to the Transaction struct to support additional metadata for transactions.
140-144: Added payment_hash to PayInvoiceResponseThe
payment_hashfield has now been included in thePayInvoiceResponsestruct, ensuring that this critical identifier is available for tracking payments.
171-172: Properly unwrapping optional payer_note and external_idThe code now correctly unwraps the optional
payer_noteandexternal_idfields from the invoice response, defaulting to empty strings if they're not present.crates/lni/cln/types.rs (3)
5-6: Remove or implement the commented code fornode_idfieldThe commented-out lines for the
node_idfield should either be implemented if needed or removed to maintain clean code.
19-29: Consider using unsigned integers for payment amountsThe fields
amount_msatandamount_sent_msatare using signed integers (i64), but payment amounts are inherently non-negative values. Consider usingu64instead for better type safety.
75-83:❓ Verification inconclusive
Ensure Bolt12Resp fields have correct types
Several boolean fields in
Bolt12Resp(active,single_use,used,created) may need validation to ensure they are properly deserialized from the API response. Verify the API documentation to confirm these are indeed boolean fields and not strings or integers representing true/false values.
🏁 Script executed:
#!/bin/bash # Check CLN API documentation or code references for Bolt12 response fields rg -A 3 -B 3 "active|single_use|used|created" --type rust crates/lni/cln/Length of output: 3810
Double-check BOlt12Resp API boolean types
The
Bolt12Respstructure incrates/lni/cln/types.rscorrectly declares the fieldsactive,single_use,used, andcreatedas booleans. Our search confirms that there’s no evidence of custom conversion logic or mismatches elsewhere in the codebase. However, please verify using the official CLN API documentation that these fields are indeed expected to be received as booleans rather than alternative representations (e.g., numeric or string values).
- File to review:
crates/lni/cln/types.rs(structBolt12Resp)bindings/lni_uniffi/src/lni.udl (5)
9-10: Method signature updated to use consistent namingThe method was renamed from
make_invoicetocreate_invoiceand now uses the more genericCreateInvoiceParamstype, improving consistency across the codebase.
65-71: Good improvement: Added optional amount parameterMaking the
amount_msatsfield optional inCreateInvoiceParamsis a good improvement as it allows for creating invoices without specifying an amount, which is useful for donation-type invoices.
99-100: Added helpful fields to Transaction dictionaryAdding
payer_noteandexternal_idfields to theTransactiondictionary enhances the data model by providing additional context for transactions, which is useful for user-facing applications.
199-200: Added essential payment_hash field to PayInvoiceResponseAdding the
payment_hashfield toPayInvoiceResponseis crucial as it provides a unique identifier for the payment, enabling proper tracking and reconciliation.
223-227: Simplified ListTransactionsParams with focused filteringThe
ListTransactionsParamsdictionary has been simplified to include onlyfrom,limit, and an optionalpayment_hash, removing less used fields and focusing on the most common filtering needs.readme.md (6)
124-136: Updated environment variables with new CLN configurationThe environment variables section has been correctly updated to replace
TEST_OFFERwith more specific variables likeTEST_RECEIVER_OFFERandPHOENIX_MOBILE_OFFER, and to add new CLN-specific variables. This change improves configuration clarity.🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
124-124: Heading levels should only increment by one level at a time
Expected: h2; Actual: h3(MD001, heading-increment)
124-124: Heading style
Expected: setext; Actual: atx(MD003, heading-style)
125-125: Fenced code blocks should have a language specified
null(MD040, fenced-code-language)
144-144: Improved build command for Node.js bindingsThe updated build command includes cleaning the cargo build cache and building in release mode, which should result in better performance for the final bindings.
🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
144-144: Unordered list indentation
Expected: 2; Actual: 4(MD007, ul-indent)
147-160: Added guidance for native Node.js module integrationThe new section on using native Node.js modules provides valuable information for developers who want to integrate the library with Electron or other native Node.js applications.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~147-~147: The official spelling of this programming framework is “Node.js”.
Context: ...rn build- testnode main.mjs` - nodejs - native modules (electron, vercel etc....(NODE_JS)
[typographical] ~147-~147: Two consecutive dots
Context: ...s - native modules (electron, vercel etc..) - if you want to use the native no...(DOUBLE_PUNCTUATION)
🪛 markdownlint-cli2 (0.17.2)
148-148: Unordered list indentation
Expected: 2; Actual: 4(MD007, ul-indent)
211-260: Added helpful sequence diagramThe sequence diagram is an excellent addition that visualizes the interactions between different components of the system, making it easier for developers to understand the architecture.
269-279: Updated implementation status in todo listThe todo list has been updated to reflect the current implementation status, including marking CLN as completed and adding new potential implementations (lndk, ldk_node, strike, nwc).
🧰 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)
285-286: Added new research item for REST API facadeAdding the research item for a facade REST API that uses the same API as Phoenixd is a good idea for future development, as it would standardize the API across different lightning node implementations.
🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
286-286: Bare URL used
null(MD034, no-bare-urls)
bindings/lni_nodejs/src/cln.rs (3)
1-8: Well-structured CLN Node implementationThe
ClnNodestruct is well-designed with a clean separation of concerns, storing the configuration in an inner field.
17-33: Appropriate accessor methods for configurationThe accessor methods for URL, rune, and full configuration are implemented correctly and provide the necessary functionality for interacting with the node configuration.
35-40: Well-implemented get_info method with proper error handlingThe
get_infomethod correctly handles error cases by mapping API errors to NAPI errors with descriptive messages.crates/lni/cln/lib.rs (1)
113-116: Missing environment variable in README.The test uses
PHOENIX_MOBILE_OFFERenvironment variable, but this isn't documented in the README.Please add the
PHOENIX_MOBILE_OFFERenvironment variable to the.envsection in the README to ensure developers can run the tests successfully.#!/bin/bash # Verify if PHOENIX_MOBILE_OFFER is documented in README.md grep -i "PHOENIX_MOBILE_OFFER" README.md || echo "PHOENIX_MOBILE_OFFER is not documented in README.md"
| // until: i64, | ||
| limit: i64, | ||
| offset: i64, | ||
| unpaid: bool, | ||
| invoice_type: String, // not currently used but included for parity | ||
| payment_hash: Option<String>, | ||
| // offset: i64, | ||
| // unpaid: bool, | ||
| // invoice_type: Option<String>, // not currently used but included for parity | ||
| // search_term: Option<String>, // not currently used but included for parity |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Simplified list_transactions parameters
Several parameters have been commented out in the list_transactions function signature, indicating a simplification of the API. Make sure all callers have been updated to use the new simplified signature.
🏁 Script executed:
#!/bin/bash
# Check for any callers of list_transactions that might be passing the now-removed parameters
rg "list_transactions.*until|offset|unpaid|invoice_type" --type rustLength of output: 1510
Action: Update all call sites for list_transactions to match the simplified signature
The new signature in crates/lni/phoenixd/api.rs now only accepts limit: i64 and payment_hash: Option<String>—the other parameters (until, offset, unpaid, and invoice_type) have been commented out. However, the verification search revealed that several modules are still referencing invoice_type (and related fields), for example:
crates/lni/phoenixd/lib.rs– several call sites still passparams.invoice_type.crates/lni/cln/lib.rs&crates/lni/cln/api.rs– similar references toparams.invoice_typeremain.- NodeJS bindings (
bindings/lni_nodejs/src/phoenixd.rsandbindings/lni_nodejs/src/cln.rs) – these also continue referencingparams.invoice_type.
Please update these callers to align with the simplified signature. Additionally, once all call sites are updated, consider removing the commented-out code from the API definition to avoid confusion.
bindings/lni_nodejs/src/cln.rs
Outdated
| #[napi] | ||
| pub async fn lookup_invoice(&self, payment_hash: String) -> napi::Result<lni::Transaction> { | ||
| let txn = lni::cln::api::lookup_invoice( | ||
| self.inner.url.clone(), | ||
| self.inner.rune.clone(), | ||
| Some(payment_hash), | ||
| None, | ||
| None, | ||
| ) | ||
| .map_err(|e| napi::Error::from_reason(e.to_string()))?; | ||
| Ok(txn.into_iter().next().ok_or_else(|| napi::Error::from_reason("No transaction found"))?) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Potential issue with error handling in lookup_invoice
The error handling in lookup_invoice method could be improved. The current implementation uses .into_iter().next() and then ok_or_else to handle the case when no transaction is found, but this might not provide a clear error message about what went wrong.
Consider making the error message more descriptive by including the payment hash:
-Ok(txn.into_iter().next().ok_or_else(|| napi::Error::from_reason("No transaction found"))?)
+Ok(txn.into_iter().next().ok_or_else(|| napi::Error::from_reason(format!("No transaction found for payment hash: {}", payment_hash)))?)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #[napi] | |
| pub async fn lookup_invoice(&self, payment_hash: String) -> napi::Result<lni::Transaction> { | |
| let txn = lni::cln::api::lookup_invoice( | |
| self.inner.url.clone(), | |
| self.inner.rune.clone(), | |
| Some(payment_hash), | |
| None, | |
| None, | |
| ) | |
| .map_err(|e| napi::Error::from_reason(e.to_string()))?; | |
| Ok(txn.into_iter().next().ok_or_else(|| napi::Error::from_reason("No transaction found"))?) | |
| } | |
| #[napi] | |
| pub async fn lookup_invoice(&self, payment_hash: String) -> napi::Result<lni::Transaction> { | |
| let txn = lni::cln::api::lookup_invoice( | |
| self.inner.url.clone(), | |
| self.inner.rune.clone(), | |
| Some(payment_hash), | |
| None, | |
| None, | |
| ) | |
| .map_err(|e| napi::Error::from_reason(e.to_string()))?; | |
| Ok(txn.into_iter().next().ok_or_else(|| napi::Error::from_reason(format!("No transaction found for payment hash: {}", payment_hash)))?) | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (5)
bindings/lni_nodejs/main.mjs (1)
96-101:⚠️ Potential issueAdd proper promise handling in main function
The main function calls phoenixd and cln without awaiting them, which means their promises won't be handled properly and errors could be silently ignored.
async function main() { - phoenixd(); - cln(); + try { + await phoenixd(); + await cln(); + } catch (error) { + console.error("Error in execution:", error); + process.exit(1); + } }crates/lni/phoenixd/api.rs (1)
258-262:⚠️ Potential issueCritical: Inconsistent Handling of the
descriptionFieldThe current implementation in
list_transactions(line 258) still hard-codes the description as an empty string, which deviates from the pattern observed elsewhere where the field is set from the invoice usingunwrap_or_default().- description: "".to_string(), + description: inv.description.unwrap_or_default(),bindings/lni_nodejs/src/cln.rs (1)
115-128:⚠️ Potential issuePass the optional
payment_hashtolist_transactions.Calls to
lni::cln::api::list_transactionsdo not includeparams.payment_hash, causing potential mismatches with user expectations:let txns = lni::cln::api::list_transactions( self.inner.url.clone(), self.inner.rune.clone(), params.from, params.limit, + params.payment_hash, // Add this to filter transactions if provided )crates/lni/cln/lib.rs (2)
74-85:⚠️ Potential issueAvoid using
.unwrap()for error handling inlookup_invoice.This can cause a runtime panic if the underlying call fails. Consider a safe error transformation:
- .unwrap(); + .map_err(|e| ApiError::Http { + reason: format!("Failed to lookup invoice: {}", e) + })?;
91-102:⚠️ Potential issueUse
list_transactionsinstead oflookup_invoicefor listing.This block calls
crate::cln::api::lookup_invoicebut should calllist_transactionswithparams:- crate::cln::api::lookup_invoice( + crate::cln::api::list_transactions( self.url.clone(), self.rune.clone(), None, Some(params.from), Some(params.limit), )
🧹 Nitpick comments (6)
bindings/lni_uniffi/src/lni.udl (4)
3-6: Consider clarifying usage and scope for PhoenixdConfig.Documenting in-code or in the broader docs how
urlandpasswordshould be used or secured (e.g. handling credentials securely) could help developers integrate it more confidently.
17-18: Check the naming consistency forget_offer.As the parameter name is omitted, consider providing clarity (e.g.
get_offer(search: string?)) in the documentation or comments for future maintainers.
102-105: Optional values inCreateInvoiceParamsseem flexible.Confirm that all downstream consumers (e.g.
create_invoicecalls) properly handleamount_msats == nulloroffer == null.
267-273:PayCodedictionary addition is aligned with BOLT12-based flows.Consider whether an optional
expires_ator additional metadata might be helpful for advanced client handling, if relevant to your application logic.bindings/lni_nodejs/src/cln.rs (1)
98-113: Incorporate the payment hash in “No transaction found” error.Including the user-supplied hash in the error message helps debugging. For example:
- .ok_or_else(|| napi::Error::from_reason("No transaction found"))? + .ok_or_else(|| napi::Error::from_reason( + format!("No transaction found for payment hash: {}", payment_hash) + ))?crates/lni/cln/api.rs (1)
148-166: Consider removingunwrap()usage inget_offer.
Although it’s logically safe due to the preceding empty-check onoffers, callingunwrap()can still be confusing to readers. As a minor improvement, consider using safe Rust idioms (e.g.,offers.first().cloned().unwrap_or_default()) to avoid potential surprise.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
bindings/lni_nodejs/index.d.ts(5 hunks)bindings/lni_nodejs/main.mjs(1 hunks)bindings/lni_nodejs/src/cln.rs(1 hunks)bindings/lni_nodejs/src/database.rs(0 hunks)bindings/lni_nodejs/src/phoenixd.rs(4 hunks)bindings/lni_uniffi/src/lib.rs(1 hunks)bindings/lni_uniffi/src/lni.udl(6 hunks)crates/lni/cln/api.rs(1 hunks)crates/lni/cln/lib.rs(1 hunks)crates/lni/cln/types.rs(1 hunks)crates/lni/phoenixd/api.rs(11 hunks)crates/lni/phoenixd/lib.rs(7 hunks)crates/lni/types.rs(4 hunks)
💤 Files with no reviewable changes (1)
- bindings/lni_nodejs/src/database.rs
🧰 Additional context used
🧬 Code Definitions (4)
bindings/lni_nodejs/src/phoenixd.rs (3)
bindings/lni_nodejs/index.d.ts (4) (4)
CreateInvoiceParams(163-170)Transaction(43-57)PayCode(171-178)ListTransactionsParams(158-162)crates/lni/phoenixd/lib.rs (3) (3)
create_invoice(32-46)get_offer(48-50)list_transactions(75-86)crates/lni/phoenixd/api.rs (3) (3)
create_invoice(34-96)get_offer(107-124)list_transactions(195-324)
crates/lni/types.rs (1)
bindings/lni_nodejs/index.d.ts (3) (3)
ListTransactionsParams(158-162)CreateInvoiceParams(163-170)PayCode(171-178)
crates/lni/cln/types.rs (1)
bindings/lni_nodejs/index.d.ts (2) (2)
PayCode(171-178)Bolt11Resp(14-18)
crates/lni/cln/lib.rs (2)
bindings/lni_nodejs/index.d.ts (8) (8)
NodeInfo(35-42)CreateInvoiceParams(163-170)ListTransactionsParams(158-162)PayCode(171-178)PayInvoiceResponse(142-146)Transaction(43-57)ClnConfig(19-22)ClnNode(23-26)bindings/lni_nodejs/src/cln.rs (9) (9)
new(12-14)get_info(35-39)create_invoice(42-59)get_offer(62-67)list_offers(70-76)pay_offer(79-95)lookup_invoice(98-113)list_transactions(116-128)decode(131-136)
🔇 Additional comments (59)
bindings/lni_uniffi/src/lib.rs (1)
2-4: Library exports updated to include new CLN moduleThe changes to the public use statements correctly expose the CLN module while maintaining the existing phoenixd module. This aligns with the Core Lightning integration that's being implemented.
bindings/lni_nodejs/index.d.ts (7)
19-26: CLN configuration and node interfaces added correctlyThe new interfaces for Core Lightning are properly defined with the required URL and rune properties, which are the standard authentication mechanism for CLN REST API.
55-57: Fields added to Transaction interfaceThe optional
payerNoteandexternalIdfields enhance the Transaction interface to support additional metadata, which is useful for providing context to payments.
158-162: ListTransactionsParams interface simplifiedThe interface has been simplified to focus on pagination parameters and an optional payment hash filter, making it more targeted and easier to use.
163-170: CreateInvoiceParams interface provides flexible invoice creationThis new interface supports both BOLT11 and BOLT12 invoice types with appropriate optional parameters, allowing for future extensibility.
171-178: PayCode interface added for offer supportThe PayCode interface properly models Lightning Network offers (BOLT12), including important metadata like single use status and activation.
193-197: PhoenixdNode API updated to use consistent namingThe method was renamed from
makeInvoicetocreateInvoicefor better API consistency across node implementations.
199-212: ClnNode class implemented with comprehensive APIThe new ClnNode class implements all core Lightning functionality including node info, invoice creation, offer management, and transaction listing. The additional
decodemethod is a nice enhancement for working with Lightning encoded strings.crates/lni/phoenixd/lib.rs (9)
4-6: Imports updated to use shared typesThe import statements have been updated to use the common types from the crate root, which improves code organization by centralizing type definitions.
32-46: Method renamed and updated to use common parameter typeThe
make_invoicemethod has been renamed tocreate_invoiceand updated to use the sharedCreateInvoiceParamstype. The default value handling for amount_msats is also improved.
48-50: Added new method to retrieve Lightning offersThe new
get_offermethod allows retrieving BOLT12 offers, which is a valuable addition for supporting the latest Lightning Network capabilities.
75-86: Updated list_transactions to use simpler parametersThe method now uses the simplified
ListTransactionsParamsstructure and passesNonefor omitted parameters, maintaining backward compatibility while streamlining the API.
118-121: Environment variable reference updatedThe static reference has been renamed from
TEST_OFFERtoTEST_RECEIVER_OFFERto more accurately describe its purpose, and the corresponding environment variable accessed has been updated.
138-161: Test updated to use new parameter structureThe test has been updated to use the new
CreateInvoiceParamsstructure, ensuring that tests remain valid with the API changes.
163-175: New test added for get_offer functionalityA new test has been added to verify the
get_offerfunctionality, which is important for ensuring the new method works correctly.
178-194: Updated test for paying offersThe test has been updated to use the new environment variable name
TEST_RECEIVER_OFFERand includes a note parameter, ensuring comprehensive testing of the payment functionality.
197-213: Test for list_transactions updated to use new parametersThe test has been updated to use the simplified
ListTransactionsParamsstructure, maintaining test coverage for the updated API.bindings/lni_nodejs/main.mjs (2)
1-45: Phoenixd functionality extracted to dedicated functionThe phoenixd-related code has been refactored into a separate async function, which improves code organization. The implementation correctly uses the updated API methods like
createInvoiceand includes the newgetOfferfunctionality.
47-94: New CLN functionality added in dedicated functionThe new CLN implementation is properly structured in its own async function with equivalent functionality to the phoenixd implementation. The commented-out code regarding CLN-to-phoenixd payment issues is appropriately documented with a TODO.
crates/lni/phoenixd/api.rs (10)
34-42: Function renamed frommake_invoicetocreate_invoicewith updated parametersThe function signature has been updated to use a more descriptive name and now accepts
amount_msatsas anOption<i64>instead of a requiredi64, making it more flexible.
48-54: Good handling of optional amount parameterThe code properly handles the optional
amount_msatsparameter usingunwrap_or_default(), which will setamount_satto 0 ifamount_msatsisNone.
79-88: Updated transaction creation with new optional fieldsThe transaction creation now includes initialization of the new
payer_noteandexternal_idfields, which aligns with the updatedTransactionstruct.
90-94: Improved error message for unsupported bolt12 invoicesThe error message is now more specific about what functionality isn't supported.
98-124: New functions for BOLT12 support and offer handlingThe introduction of
decodeandget_offerfunctions adds support for BOLT12 functionality, although thedecodefunction is currently just a placeholder.
149-153: Improved error handling in pay_offer functionThe error handling now properly returns the raw response text in error cases, making debugging easier.
155-159: Added payment_hash to PayInvoiceResponseThe function now includes
payment_hashin the response, which is important for tracking and referencing payments.
189-190: Added support for payer_note and external_id in lookup_invoiceThese fields are now properly extracted from the invoice response and included in the transaction.
199-205: Simplified list_transactions parametersSeveral parameters have been commented out, simplifying the API. Make sure all call sites have been updated to use the new simplified signature.
#!/bin/bash # Check for any callers of list_transactions that might be passing the now-removed parameters rg "list_transactions.*until|offset|unpaid|invoice_type" --type rust
313-316: Consistent handling of payer_note and external_idThe outgoing transactions now properly include the
payer_noteandexternal_idfields, consistent with the handling for incoming transactions.bindings/lni_nodejs/src/phoenixd.rs (5)
1-1: Updated import to use CreateInvoiceParamsThe import now correctly references the new
CreateInvoiceParamstype from the lni crate.
43-59: Function renamed frommake_invoicetocreate_invoiceThe function has been renamed to
create_invoiceto match the API changes, and now correctly uses the newCreateInvoiceParamstype.
61-67: Added get_offer functionNew function added to retrieve BOLT12 offers via the API, aligning with the new functionality in the backend.
93-95: Fixed ordering of await and error mappingThe code now correctly awaits the result before mapping the error, ensuring proper asynchronous error handling.
102-110:✅ Verification successful
Updated list_transactions parameters
The function now correctly uses the simplified parameter type and passes
Nonefor parameters that have been removed from the API. This aligns with the changes in the API function.
🏁 Script executed:
#!/bin/bash # Check if the ListTransactionsParams struct in the codebase matches this usage rg "struct ListTransactionsParams" --type rust -A 10Length of output: 581
Verified: The updated parameters match the API changes.
Thelni::phoenixd::api::list_transactionscall now correctly usesparams.fromandparams.limitwhile explicitly passingNonefor the removed parameter. Although theListTransactionsParamsstruct still containspayment_hash, the omission of its value in the API call is intentional and aligns with the simplified parameter usage.crates/lni/types.rs (4)
3-3: Added Serialize trait to importsThe import now includes both Serialize and Deserialize traits from serde, enabling serialization for the defined types.
45-46: Added new fields to Transaction structThe Transaction struct now includes optional
payer_noteandexternal_idfields, enabling better support for BOLT12 payments and external references.
174-174: Added payment_hash to PayInvoiceResponseThe PayInvoiceResponse struct now includes a required
payment_hashfield, improving payment tracking capabilities.
203-232: Added new parameter and response structuresThree new structs have been added:
ListTransactionsParams- For simplified transaction listing parametersCreateInvoiceParams- For invoice creation with flexible parametersPayCode- For BOLT12 offer representationThese additions support the API changes and provide proper type safety for the new functionality.
crates/lni/cln/types.rs (6)
1-4: New file for Core Lightning type definitionsThis file defines the necessary types for Core Lightning (CLN) integration, properly importing required dependencies.
5-14: Remove unused commented code for node_id fieldThe commented-out lines for the
node_idfield are not used elsewhere in the codebase.- // #[serde(rename = "nodeId")] // Handle JSON field `nodeId` - // pub node_id: String,
21-31: Payment Amount Type: Switch to UnsignedAfter verifying the usage of the
PayResponsestruct across the codebase, there's no indication that negative values are ever expected for payment amounts. Since fields likeamount_msatandamount_sent_msatrepresent inherently non-negative values, it would be more precise and safer to useu64instead ofi64.- pub amount_msat: i64, - pub amount_sent_msat: i64, + pub amount_msat: u64, + pub amount_sent_msat: u64,
34-60: Comprehensive Invoice and Outpoint structuresThe
PaidOutpointandInvoicestructs provide detailed definitions for handling payment data, with appropriate optional fields for handling various payment states.
67-85: Well-structured Bolt11 and Bolt12 response typesThe response types for both BOLT11 and BOLT12 are well-defined, with appropriate fields for each protocol.
87-90: ListOffersResponse using PayCode from common typesThe
ListOffersResponseproperly reuses thePayCodetype defined in the common types, promoting code reuse.bindings/lni_uniffi/src/lni.udl (5)
15-16: Renamingmake_invoicetocreate_invoiceis clear and consistent.
31-34: Keep security in mind forrune.If
runeis a sensitive token, confirm that no logs or error messages expose it. Would you like a script to scan for accidental logs or prints ofruneacross the repository?
36-63: NewClnNodeinterface looks well-structured.
137-138: New optional fieldspayer_noteandexternal_idinTransaction.Ensure these fields are returned and tested in the relevant endpoints or methods to maintain end-to-end consistency.
262-265: Replacinguntil,offset, etc., withpayment_hashstreamlinesListTransactionsParams.crates/lni/cln/api.rs (10)
11-33: Use GET method and replaceunwrap/println!with proper error handling and logging.
This function calls/v1/getinfobut uses a POST request instead of GET, and repeatedly calls.unwrap()andprintln!, which can cause panics and noisy console output in production environments.
35-129: Avoid blocking calls in async function and removeunwrap/println!.
This async function usesreqwest::blocking::Response, which defeats the purpose of async. Also, repeated.unwrap()calls can panic on errors, and printing to stdout viaprintln!should be replaced with structured logging for production code.
131-146: Use async HTTP methods and structured error handling.
Similar to other functions,decodeis declared async but relies on blocking requests and.unwrap(). Consider switching toreqwest::Clientwith async methods and replacingunwrap()with the?operator or a custom error type.
168-195: Switch to async HTTP client and handle errors gracefully.
This function is declared async but usesreqwest::blocking::Responseand multiple.unwrap()calls. For a more robust design, consider using an async client, removing blocking calls, and replacing.unwrap()with structured error handling.
197-246: Remove blocking calls andunwrap()fromcreate_offer.
As an async function,create_offershould either usereqwest::Clientwith async methods or remove the async designation. Multiple.unwrap()calls may cause runtime panics; switch to the?operator for robust error handling.
248-282: Use non-blocking calls infetch_invoice_from_offerand avoid.unwrap().
Using blocking calls in an async context can lead to performance issues, and.unwrap()can cause panics. Consider adopting an async client and proper error propagation for resilience.
284-335: Eliminate blocking calls andunwrap()inpay_offer.
This exact pattern has been flagged previously: an async function should rely on non-blocking HTTP calls, and.unwrap()calls should be replaced with robust error handling to avoid panics.
337-403: Replace.unwrap()with safe error handling inlookup_invoice.
Here, multiple.unwrap()calls on sending requests and reading responses can crash the application if errors occur. Use?or custom error handling to improve stability.
405-415: LGTM!
This function delegates tolookup_invoiceand provides a clean error-forwarding approach with no direct unwrap usage. No issues found here.
417-425: Remove unsafe TLS acceptance and replaceunwrap()with structured error handling inclnrest_client.
Allowing invalid certificates by default is risky for production code. Also, any failure while building headers or the client will panic on.unwrap(). Make these behaviors configurable and use safe error handling.
https://docs.corelightning.org/docs/rest
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores