Tor Socks Proxy #7
Conversation
WalkthroughThe changes update the Changes
Sequence Diagram(s)sequenceDiagram
participant Env as Environment Variables
participant Test as Test Function (main.mjs)
participant Node as LndNode
participant API as LND API
Note over Test: Read LND config from environment
Test->>Env: Retrieve URL, macaroon, socks5Proxy, acceptInvalidCerts
Test->>Node: Instantiate with LndConfig
Node->>API: Call async get_info() with config
API-->>Node: Return node info
Node-->>Test: Log node info
sequenceDiagram
participant LndNode as LndNode Methods
participant LndAPI as LND API Functions
Note over LndNode: API call with consolidated config
LndNode->>LndAPI: Call async method (e.g., create_invoice) with LndConfig
LndAPI-->>LndNode: Return response
Possibly related PRs
Poem
✨ Finishing Touches
🪧 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: 2
🧹 Nitpick comments (9)
bindings/lni_nodejs/main.mjs (1)
135-148: Updated test function demonstrates Tor proxy functionalityThe test function now properly demonstrates how to use the LndNode with Tor SOCKS proxy settings, which is valuable for showing users real-world usage patterns.
Consider adding an environment variable for the SOCKS proxy URL rather than hardcoding it, which would make the example more flexible:
- socks5Proxy: "socks5h://127.0.0.1:9150", + socks5Proxy: process.env.TOR_SOCKS_PROXY || "socks5h://127.0.0.1:9150",crates/lni/lnd/lib.rs (1)
127-133: Test setup includes Tor configurationThe test NODE setup now includes the new SOCKS proxy and certificate validation settings, which is good for testing the full functionality. However, this could cause tests to fail if the Tor proxy isn't running.
Consider making the test more robust:
LndNode::new(LndConfig { url: URL.clone(), macaroon: macaroon.clone(), - socks5_proxy: Some("socks5h://127.0.0.1:9150".to_string()), // Tor socks5 proxy using arti + socks5_proxy: std::env::var("TOR_SOCKS_PROXY").ok(), // Use env var if available accept_invalid_certs: Some(true), ..Default::default() })crates/lni/lnd/api.rs (7)
33-37: Handle potential HTTP failures gracefully.Currently,
.await.unwrap()on the HTTP response can panic if the request fails or the body is invalid. Consider returning anApiErroror using?to propagate the error instead of unwrapping.
45-47: Return a proper error instead of panicking.Similar to the previous comment, replace
.await.unwrap()with a safer approach that returns a meaningful error if the request or parsing fails.
100-100: Replace unwrapping HTTP results with error propagation.When creating invoices, several
.unwrap()calls can panic. Refactor to propagate errors using?or returnApiError. This aligns with Rust’s error-handling best practices.Also applies to: 103-103, 109-110, 123-123, 127-127
160-160: Enhance error handling on payment request.Multiple
.unwrap()calls within.awaitcan crash the service if payment fails or the response is not as expected. Return structured errors instead.Also applies to: 163-163, 242-248, 251-251
283-286: Avoid unwrapping on decode request failures.In
decode, calling.await.unwrap()for the response and its body can panic. Refactor to propagate errors gracefully.Also applies to: 288-288
342-343: Propagate lookup failures instead of panicking.Replacing
.await.unwrap()with an error-based approach will prevent unexpected panics in production if invoice lookups fail.Also applies to: 347-347, 349-349, 351-351, 359-359
399-400: Graceful error handling in listing transactions.Again,
.unwrap()calls on network operations can lead to panics. Consider returning a descriptive error if requests or JSON parsing fails.Also applies to: 406-408, 411-412
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
bindings/lni_nodejs/Cargo.toml(1 hunks)bindings/lni_nodejs/index.d.ts(1 hunks)bindings/lni_nodejs/main.mjs(1 hunks)bindings/lni_nodejs/src/lnd.rs(5 hunks)bindings/lni_uniffi/Cargo.toml(1 hunks)bindings/lni_uniffi/src/lni.udl(1 hunks)crates/lni/Cargo.toml(1 hunks)crates/lni/lnd/api.rs(12 hunks)crates/lni/lnd/lib.rs(4 hunks)readme.md(1 hunks)
🔇 Additional comments (25)
crates/lni/Cargo.toml (1)
11-15: Excellent addition of SOCKS proxy supportThe update to reqwest v0.12 with the new "socks" feature is well-aligned with the PR objective of adding Tor SOCKS proxy support. This will enable routing HTTP requests through SOCKS proxies like Tor.
bindings/lni_uniffi/src/lni.udl (1)
74-75: Good enhancement to LndConfig with proxy supportThe addition of
socks5_proxyandaccept_invalid_certsfields provides the necessary configuration options for Tor connectivity. The nullable types are appropriate for these optional settings.These options will enable users to:
- Connect to LND nodes through Tor using the SOCKS5 proxy
- Accept self-signed certificates that are common with Tor hidden services
bindings/lni_nodejs/Cargo.toml (1)
18-18: Consistent dependency update for Node.js bindingsThe reqwest update to v0.12 with "socks" feature maintains consistency with the changes in the main crate, ensuring SOCKS proxy support is available in the Node.js bindings as well.
bindings/lni_uniffi/Cargo.toml (1)
19-19: Consistent dependency update for UniFfi bindingsThe reqwest update to v0.12 with "socks" feature ensures that the UniFfi bindings also have SOCKS proxy support, maintaining consistency across all packages in the project.
readme.md (1)
197-208: Well-documented Tor SOCKS proxy functionality!The documentation clearly explains how to use Tor for connecting to
.onionservices, including:
- The correct format for the SOCKS proxy URL (
socks5h://)- A practical example with all required parameters
- Reference to Arti for setting up a Tor proxy
This will be very helpful for users who need to connect to Lightning nodes behind Tor hidden services.
bindings/lni_nodejs/index.d.ts (2)
37-38: Good addition of proxy support optionsAdding these optional properties enhances flexibility when connecting to LND nodes through Tor or other proxies, while maintaining backward compatibility.
41-41: Improved configuration management approachRefactoring the LndNode interface to use a consolidated config object instead of individual properties is a good architectural improvement that:
- Makes the API more consistent
- Simplifies parameter passing
- Allows for easier addition of future configuration options
This change follows the configuration object pattern, which is a best practice for extensible APIs.
bindings/lni_nodejs/main.mjs (1)
153-155: Test prioritization updated appropriatelyUncommented the
test()function to verify the new Tor SOCKS proxy functionality, while commenting out thecln()test that isn't relevant to the current changes.crates/lni/lnd/lib.rs (5)
11-17: Well-structured LndConfig with Tor supportThe enhanced LndConfig struct correctly implements SOCKS5 proxy support and certificate validation options, and includes:
- Proper use of Option types for optional fields
- Good documentation comments for the proxy URL format
- Appropriate derive macros for Debug and Clone
This implementation enables connecting to Tor hidden services securely.
18-27: Good default implementation for LndConfigProviding a Default implementation with sensible values improves API ergonomics and simplifies usage when not all fields are required.
31-31: Consolidated configuration via structReplacing individual fields with a single config field follows good API design principles and makes the implementation more maintainable.
35-37: Updated constructor for better configuration managementThe constructor now accepts the full LndConfig struct, simplifying parameter passing and making the API more consistent.
39-41: Refactored API methods to use config objectAll API methods have been consistently updated to use the new config field, improving code maintainability.
crates/lni/lnd/api.rs (2)
5-5: Reference LndConfig import looks good.No issues found with adding the
use super::LndConfig;statement.
294-294: Skipping review.Function is not implemented and directly returns an error.
bindings/lni_nodejs/src/lnd.rs (10)
31-32: Cloning new fields in config is correct.Adding
socks5_proxyandaccept_invalid_certstoget_configaligns with the updatedLndConfigstructure.
38-40: Asynchronous get_info integration looks valid.No issues found; errors are nicely wrapped with
map_err().
49-51: Invoice creation call is well-integrated.Propagation of errors to the caller ensures good error handling here.
60-62: Good usage of async for pay_invoice.Parameter passing and error handling via
map_err()are correctly handled.
68-70: Offer retrieval logic is clean.No concerns; it follows the same async pattern with proper error propagation.
76-78: list_offers integration is appropriate.Follows the consolidated config usage.
89-91: pay_offer uses standard asynchronous error handling.Implementation looks consistent with the rest of the code.
97-99: lookup_invoice properly wraps errors.Implementation is coherent and consistent.
108-110: list_transactions leverages new consolidated config well.Error handling approach remains consistent.
116-116: decode method is well integrated.No issues with passing
&self.inner.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (37)
crates/lni/phoenixd/lib.rs (1)
16-27: Reconsider defaultingaccept_invalid_certstoSome(true).Allowing invalid certs by default can pose security risks in production. Consider changing the default to
Some(false)to ensure secure defaults, letting users explicitly opt in to accepting invalid certificates only if required.impl Default for PhoenixdConfig { fn default() -> Self { Self { url: "https://127.0.0.1:8080".to_string(), password: "".to_string(), socks5_proxy: None, - accept_invalid_certs: Some(true), + accept_invalid_certs: Some(false), } } }crates/lni/cln/lib.rs (2)
11-17: ExtendClnConfigwith SOCKS5 and invalid cert options.Consolidating these fields into a single struct is a solid approach, but be mindful of the security implications of allowing invalid certs to be accepted. You might consider defaulting it to
Some(false)for production safety.
18-26: Default implementation note.The default constructor sets
accept_invalid_certstoSome(true). Similar to Phoenixd, consider making this more secure by default (e.g.,Some(false)) to reduce unintentional exposure to insecure connections.crates/lni/phoenixd/api.rs (5)
30-72:get_infomethod goes async with config-based calls.Transitioning to async eliminates blocking I/O. The function logic is solid, but watch for
.expect(...)calls that can panic. Consider gracefully handling errors if they're user-facing.- .expect("Failed to get node info"); + .map_err(|e| ApiError::Network { + reason: e.to_string(), + })?
74-136:create_invoiceupdated for async usage and config.This refactoring aggregates all invoice creation logic in one place. The function is clear, although the presence of multiple
.expect(...)calls might cause panics in production if errors are not handled gracefully.
202-236:pay_offerimplements asynchronous requests.Consistent with other calls, but again note the unconditional
expect("Failed to pay offer"). If the UI or caller is user-facing, more graceful error propagation might be desirable.
241-276:lookup_invoicerefactor.Using the config-based client pattern improves code consistency. Logging more detailed errors instead of
expect()might be helpful for debugging.
278-416:list_transactionsnow async and config-driven.The approach merges incoming and outgoing payments in a single function. The logic is clear, but consider guarding large .unwrap() calls to avoid runtime panics. Additionally, sorting in descending order is appropriate for quick visibility, though might benefit from an optional user-specified sort.
crates/lni/cln/api.rs (28)
15-27: Constructing HTTP client with optional proxy and untrusted certificates.
Building a customreqwest::Clientusingsocks5_proxyanddanger_accept_invalid_certsprovides flexibility. However, allowing invalid certificates is a significant security risk:
- Consider logging or warning when
danger_accept_invalid_certsis enabled.- Handle potential errors more gracefully than using
unwrap().
32-32: Client creation on every request.
Repeatedly callingclnrest_client(config)can add overhead if done frequently. Consider reusing a shared client to optimize performance, unless there's a specific need for rebuilds per request.
37-37: Potential runtime panics withunwrap().
Using.await.unwrap()can crash the program if the response fails. Consider robust error handling here.
39-39: Potential runtime panics withunwrap().
Likewise,.text().await.unwrap()can panic. Consider returning an appropriateApiErroror logging gracefully.
44-44: Use consistent logging.
Here we print the constructed URL above but only rely onprintln!. For consistency and flexible log levels, consider using a structured logger likelogortracing.
49-49: Repetitive usage of.await.unwrap().
Again, consider more robust error handling for network requests to avoid unexpected panics.
51-51: Potential data parsing error.
funds_response_textmay cause a panic if it isn't valid JSON. Recommend graceful error handling and returning a domain-specificApiError.
136-136: Further.await.unwrap()usage.
Consider systematically handling request failures using the project’sApiErrorfor consistent error reporting.
140-140: Debug prints in production code.
Printing invoice data might expose sensitive information (like invoice strings). Consider removing or obfuscating this print for production.
201-202: Client creation repeated.
ReusingClientis often more efficient. If no runtime-specific reason to recreate it for each call, consider a shared client approach.
259-259: Repeated.await.unwrap()
Again, consider turning these into structured error responses.
261-261: Potential user confusion in error feedback.
pay_response_textis returned unchanged if parsing fails. Confirm it doesn’t leak sensitive data.
281-281: Recreate HTTP client once again.
As with other methods, consider a shared or cached client for better efficiency, especially if decoding is frequent.
290-290: Missing error handling.
Awaiting.text()with.unwrap()can cause panics on unexpected errors. Suggest a match or?operator for safe unwrapping.
293-293: Variable naming.
decodedis descriptive enough, but consider clarifying truncated or partial data if large responses are expected.
299-299: Parameter chaining withsearch.clone().
Cloning thesearchstring is fine, though it might be more efficient to handle references if search is large or frequently invoked.
333-333: Offer text handling.
Since the text could be large or contain sensitive data, be mindful of logging or printing.
348-349: Refactored client creation for new route.
Same practice as before: a new call toclnrest_client(config). Consider reusing or documenting the rationale for repeated creation.
368-368: Lack of robust error flow.
Again,.await.unwrap()can produce runtime panics. Suggest returningApiError::Networkor similar.
370-370: Potential sensitive data logging.
offer_strmight expose user data. Evaluate whether logging is truly necessary.
400-401: Repeated creation of new request client & subsequentunwrap().
Use consistent error handling for potential network or parse failures. Reusing aClientwith a robust error path can help.
411-411: Again,.await.unwrap().
Consider returning a more descriptive error or logging the cause.
413-413: Exposing raw response text on parse errors.
Make sure we do not leak private info or tokens. Logging raw text is sometimes risky.
447-448: Same repeating usage pattern.
Similar topay_invoice, we build another request. Double-check that multiple calls for the same offer are intentionally repeated.
457-457: Potential runtime panic.
.await.unwrap()once again. Consistent structured error handling recommended.
459-459: Response logging.
Be sure the success or error path doesn’t log secrets inpay_response_text.
505-505: Redundant or repeated client building.
Creating the client in each invoice listing may degrade performance at scale. Consider a consolidated approach.
530-530: Network error handling.
.awaitplus.unwrap()can fail on transient network errors. ReturningApiError::Networkwould be more consistent.bindings/lni_nodejs/src/phoenixd.rs (1)
29-29: Cloning PhoenixdConfig.
This is consistent with the rest of the design for retrieving config. If performance or memory is a concern, consider returning a reference.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
bindings/lni_nodejs/index.d.ts(2 hunks)bindings/lni_nodejs/src/cln.rs(5 hunks)bindings/lni_nodejs/src/phoenixd.rs(6 hunks)bindings/lni_uniffi/src/lni.udl(3 hunks)crates/lni/cln/api.rs(21 hunks)crates/lni/cln/lib.rs(4 hunks)crates/lni/phoenixd/api.rs(13 hunks)crates/lni/phoenixd/lib.rs(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- bindings/lni_uniffi/src/lni.udl
🧰 Additional context used
🧬 Code Definitions (5)
crates/lni/phoenixd/lib.rs (2)
bindings/lni_nodejs/index.d.ts (4)
PhoenixdConfig(6-11)PhoenixdNode(12-14)NodeInfo(53-67)Transaction(68-82)crates/lni/phoenixd/api.rs (3)
get_info(30-72)create_invoice(74-136)pay_invoice(138-171)
crates/lni/cln/api.rs (7)
bindings/lni_nodejs/index.d.ts (2)
ClnConfig(27-32)NodeInfo(53-67)bindings/lni_nodejs/src/cln.rs (1)
get_info(32-37)bindings/lni_nodejs/src/phoenixd.rs (1)
get_info(33-38)crates/lni/cln/lib.rs (1)
get_info(39-41)bindings/lni_nodejs/src/lnd.rs (1)
get_info(37-42)crates/lni/phoenixd/api.rs (2)
client(18-28)get_info(30-72)crates/lni/lnd/api.rs (2)
client(16-31)get_info(33-97)
bindings/lni_nodejs/src/phoenixd.rs (2)
crates/lni/phoenixd/api.rs (6)
get_info(30-72)pay_invoice(138-171)get_offer(182-200)lookup_invoice(241-276)pay_offer(202-236)list_transactions(278-416)bindings/lni_nodejs/index.d.ts (3)
NodeInfo(53-67)PayCode(201-208)Transaction(68-82)
bindings/lni_nodejs/src/cln.rs (2)
crates/lni/cln/api.rs (9)
get_info(29-100)pay_invoice(197-277)params(246-249)get_offer(298-311)list_offers(313-340)pay_offer(429-475)lookup_invoice(477-495)list_transactions(565-574)decode(280-295)bindings/lni_nodejs/index.d.ts (3)
NodeInfo(53-67)PayCode(201-208)Transaction(68-82)
crates/lni/phoenixd/api.rs (1)
crates/lni/phoenixd/lib.rs (5)
new(36-38)get_info(40-42)get_offer(66-68)lookup_invoice(79-84)list_transactions(86-91)
🔇 Additional comments (70)
crates/lni/phoenixd/lib.rs (10)
12-12: Add derived traits.Good use of
#[derive(Debug, Clone)]for the configuration struct, ensuring easy debugging and making the config clonable across async contexts.
32-32: Config field centralization approved.Replacing individual fields with a single
configfield ensures frictionless management of new or optional parameters (e.g., socks5 proxy), improving future maintainability.
37-37: Constructor looks good.Straightforward initialization of
self.configpromotes clarity and consistency across node types.
41-41: Configuration usage forget_info.Passing the entire config ref is consistent with the new structure and keeps the codebase DRY.
49-49: Refactored call tocreate_invoice.This change centralizes all invoice-related config in one place, improving readability and maintainability.
63-63: Unified payment config usage.Pay invoice now depends on a single
PhoenixdConfigstruct, reducing parameter sprawl.
67-67:get_offermethod streamlined.The updated approach for retrieving offers with just one config struct prevents future signature bloat.
76-76:pay_offerintegration.Passing
&self.configensures any future proxy or cert settings modifications apply automatically here, enhancing flexibility.
83-83: Look up invoice with minimal signature.Adopting the new config-based approach helps keep the function signature concise and avoids repeated parameters.
90-90:list_transactionsnow consistent.Refactoring to pass
self.configmaintains uniformity across parallel methods for Phoenixd interactions.crates/lni/cln/lib.rs (11)
31-31: Consolidated config field approved.Replacing individual fields with one
configobject simplifies expansions, keeps the node representation neat, and avoids scattered references.
36-36: Constructor alignment.Accepting a
ClnConfigdirectly is consistent with the Phoenixd approach, promoting consistent constructor patterns across node types.
40-40: Usage of config inget_info.This refactoring ensures new config fields (like proxy settings) are readily available without adding parameters.
48-48: Refactoring forcreate_invoice.Extracting shared data from
configclarifies the function signature, demonstrating good separation of concerns.
63-63:pay_invoicealignment.Passing the config struct in one piece fosters simpler, uniform function calls and better maintainability.
67-67: Consistent approach toget_offer.Removing individual parameters is consistent with the other modifications and improves the method’s extensibility.
71-71:list_offerssynergy.Using the same pattern for listing offers helps keep the code uniform across invoice and offer endpoints.
80-80: Unifiedpay_offersignature.Keeps the code pattern consistent and leverages the new config object approach for any future changes without method churn.
87-87: Revisedlookup_invoice.Pulling the configuration from a single source (the struct) further standardizes the codebase for dev consistency.
94-94: List transactions clean-up.Similar changes ensure consistent retrieval of data using the new config-driven pattern.
98-98:decodeupdated with minimal fuss.Just note that if CLN eventually supports more advanced decode logic, this approach continues to scale well by referencing the config just once.
bindings/lni_nodejs/index.d.ts (5)
9-10: Added socks5Proxy and acceptInvalidCerts to PhoenixdConfig.Exposing these fields in the Node.js bindings aligns them with the Rust-layer changes. Verify that your TypeScript consumers handle them safely.
13-13: Single config property in PhoenixdNode.Switching to a unified
configproperty ensures consistent usage inside TypeScript definitions, matching the Rust refactor.
30-31: Expanded ClnConfig for socks5Proxy and acceptInvalidCerts.Maintaining parity with the Rust side is vital. Confirm that default behaviors on the TypeScript side match your intended usage.
39-40: Expose proxy and certificate flexibility for LndConfig.LND config now includes optional proxy and cert validation toggles. This fosters alignment across node types while cautioning about security for invalid cert acceptance.
43-43: Centralized LndNode configuration.Mirroring the pattern for PhoenixdNode and ClnNode,
configproperty clarifies the code interface in TypeScript.crates/lni/phoenixd/api.rs (4)
5-8: Imports expanded forPhoenixdConfigand typed responses.Ensures the code references the new configuration struct and typed responses from
types. Good approach for type safety.
18-28: Newclientfunction with optional SOCKS5 and invalid cert acceptance.Centralizing client creation is a solid approach. However, confirm that enabling
danger_accept_invalid_certs(true)for production is necessary, as it poses security risks by default.
138-171:pay_invoiceuses async and single config.The newly introduced
pay_invoiceapproach is coherent. As with other methods, consider more robust error handling instead ofexpect.
182-200: Addedget_offerwith new client logic.Straightforward retrieval of the offer, verifying that the correct proxy or cert flags are used from the config if needed.
crates/lni/cln/api.rs (22)
5-5: Consolidated configuration reference.
IntroducingClnConfighere helps centralize configuration fields, improving readability and maintainability.
29-31: Clarity in function signature.
Using&ClnConfiginget_infois a clean approach for passing configuration around.
71-77: Expanded pending/awaiting states in channel logic.
Including"CHANNELD_AWAITING_LOCKIN"and"DUALOPEND_*"states plus"OPENINGD"is correct for broader channel states coverage. Ensure testing covers these new states to avoid missing edge cases.
104-104: Refined function parameter.
Replacing separate parameters with&ClnConfigkeeps the interface consistent. This simplifies passing configuration to invoice creation.
127-127: Association with correct endpoint.
This updated URL path (/v1/invoice) aligns with the CLN invoice creation route. Ensure that any older references are removed from the codebase.
198-198: Parameter alignment.
Usingconfig: &ClnConfigmatches the rest of the file’s refactor, enforcing consistency in all pay-related functions.
254-254: Assigning response to typed variable.
Returningreqwest::Responseis valid, though type-checking at each stage helps. Just ensure later usage checks HTTP success.
280-280: Consistent async method naming.
decodeis properly an async function. The name is concise and matches its purpose.
282-282: Explicit route construction.
/v1/decodelooks correct. Ensure references in documentation or older code are updated to avoid stale references.
283-283: Consistent request pattern.
Using.post().header().json().send()is consistent with other calls. This maintains a uniform style.
298-298: New get_offer function.
Accepting a search parameter for an offer from CLN. This is a straightforward approach for partial matches.
331-331: Awaiting post request.
Using.awaitis correct for asynchronous request flow. Just confirm the response always yields valid JSON.
343-343: Introduced config parameter.
create_offernow uses theClnConfig, aligning with the established pattern.
394-394: Fetching invoice with config.
Acceptingconfig: &ClnConfigensures consistent usage for all invoice retrieval.
430-430: Uniform config usage.
Ensuringpay_offerreferences&ClnConfigmaintains the new standard across this file.
435-439: Invoice fetching re-invoked.
- Invoking
fetch_invoice_from_offeragain re-creates the client.- Consider caching or a single call if possible.
477-478: Refined ability to filter invoices by payment hash.
The new function signature withpub async fn lookup_invoice(config: &ClnConfig, payment_hash: Option<String>, ...)is flexible.
499-499: Revised scope for invoice lookups.
Using a single function for multiple conditions (payment_hash, from, limit) helps unify query logic.
521-521: Filtered parameters for request.
Using.filter_map(...)is concise. Just ensure no critical parameter is accidentally filtered out.
532-532: Check for partial data.
If partial data is returned, ensure the code gracefully handles incomplete JSON.
565-566: Consistent approach in listing transactions.
Exposingconfig: &ClnConfigparam keeps a unified design.
570-570: Single reference tolookup_invoices.
This is a clean, DRY approach: reusinglookup_invoicesfromlist_transactions.bindings/lni_nodejs/src/cln.rs (10)
28-28: Cloning config.
Returningself.inner.clone()is straightforward, especially ifClnConfigisClone. This is fine for small structs.
33-34: Asynchronous info retrieval.
Switching toget_info(&self.inner).awaitis consistent with the async pattern. Good usage of.map_err(...)for error handling.
45-45: Concise parameter passing.
Passing only&self.innertocreate_invoicereduces duplication.
63-65: Paying invoices with config.
Lines 63–65 unify configuration usage. Consider verifying all relevantClnConfigfields are utilized properly.
71-71:get_offeruniform call style.
Usinglni::cln::api::get_offer(&self.inner, search)is consistent with the new config-based pattern.
79-81: Listing offers improvement.
The shift tolist_offers(&self.inner, search).awaitexemplifies a well-structured approach.
92-94:pay_offerwith config.
Again, good consistency. Validate that the newpay_offerhandles proxy or invalid certs appropriately if set in the config.
100-102: Flexible lookup.
By passingSome(payment_hash)with optional range arguments,lookup_invoicecan handle varied use cases.
111-113: Transaction listing.
Forwardingparams.fromandparams.limitdirectly tolist_transactionsclarifies data flow.
118-118: Unified decode approach.
Delegating tolni::cln::api::decode(&self.inner, str)simplifies the node’s decode logic.bindings/lni_nodejs/src/phoenixd.rs (8)
34-35: Asynchronous info retrieval.
Good usage of.awaitto retrieve node info and error out gracefully with.map_err(...).
46-46: Leverage unified config approach.
Passing&self.innerdirectly to new invoice creation is consistent with the improved config-based architecture.
63-65: Pay invoices with single config parameter.
Consistent with the rest of the refactoring across the codebase. Continue verifying that all relevant fields inPhoenixdConfigare set.
71-73:get_offerrefactoring.
Lines 71–73 unify calls tophoenixd::api::get_offer(&self.inner). Make sure to handle any possible missing fields inPhoenixdConfig.
79-81:lookup_invoicesupports partial lookups.
phoenixd::api::lookup_invoice(&self.inner, payment_hash)is simpler than manual URL param building.
92-94: Consistentpay_offer.
Using the same config reference ensures correct usage of proxy or certificate settings across all phoenixd API calls.
103-105: Listing transactions with partial parameters.
Ensuringphoenixd::api::list_transactionsis called with(params.from, params.limit, None)is a good pattern.
131-131: Default configuration usage.
Relying on..Default::default()is a neat approach if the fields are truly optional and well-handled.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (7)
readme.md (7)
12-14: Review Heading Levels and Styles.
The updated headings at line 12 (### Interface API Examples) and line 14 (#### Rust) use atx-style headers with an increment that may be inconsistent with the rest of the document. Additionally, markdownlint reports that heading levels should only increment by one level at a time. Consider unifying either the heading level progression or switching to a consistent style (for example, setext style) throughout the file.🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
12-12: Heading levels should only increment by one level at a time
Expected: h2; Actual: h3(MD001, heading-increment)
12-12: Heading style
Expected: setext; Actual: atx(MD003, heading-style)
14-14: Heading style
Expected: setext; Actual: atx(MD003, heading-style)
61-61: Standardize the TypeScript Section Heading.
The "#### Typescript" heading on line 61 is written in atx style, whereas other sections (for example, the "Language Bindings" section later on) use setext style. For a cohesive look, please consider unifying the heading style across the document.🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
61-61: Heading style
Expected: setext; Actual: atx(MD003, heading-style)
91-95: Correct the Value forpayment_hashin TypeScript Examples.
In the TypeScript example block (line 94),payment_hashis assigned the valueNone, which is Python syntax. In TypeScript, usenull(orundefined) to indicate the absence of a value.
140-142: Hyphenate ‘cross-platform’ for Clarity.
In line 141, “cross platform” should be hyphenated as “cross-platform” to conform to standard compound adjective usage.🧰 Tools
🪛 LanguageTool
[misspelling] ~142-~142: This word is normally spelled with a hyphen.
Context: ... event system to make sure the lib runs cross platform and stays lightweight. NOT YET IMPLEMEN...(EN_COMPOUNDS_CROSS_PLATFORM)
267-268: Convert Bare URL to Markdown Link in the Tor Section.
The Tor section on line 267 contains a bare URL (https://tpo.pages.torproject.net/core/arti/). To improve readability and address markdownlint MD034 warnings, consider formatting this as a Markdown link (e.g.,[Arti documentation](https://tpo.pages.torproject.net/core/arti/)).🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
267-267: Bare URL used
null(MD034, no-bare-urls)
350-350: Format Bare URL in Checklist Item.
The checklist item on line 350 includes a bare URL (https://tpo.pages.torproject.net/core/arti/guides/starting-arti), which triggers markdownlint MD034. Converting it to a Markdown link will improve the appearance and consistency of the documentation.🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
350-350: Bare URL used
null(MD034, no-bare-urls)
208-209: Ensure Consistent Heading Style in the ‘Language Bindings’ Section.
The "Language Bindings" section on line 208 is written using setext style, which contrasts with the atx-style headings used in other parts of the document. For a cohesive documentation style, consider standardizing the heading style throughout the file.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
readme.md(5 hunks)
🧰 Additional context used
🪛 markdownlint-cli2 (0.17.2)
readme.md
12-12: Heading levels should only increment by one level at a time
Expected: h2; Actual: h3
(MD001, heading-increment)
12-12: Heading style
Expected: setext; Actual: atx
(MD003, heading-style)
14-14: Heading style
Expected: setext; Actual: atx
(MD003, heading-style)
61-61: Heading style
Expected: setext; Actual: atx
(MD003, heading-style)
102-102: Heading style
Expected: setext; Actual: atx
(MD003, heading-style)
119-119: Heading style
Expected: setext; Actual: atx
(MD003, heading-style)
124-124: Heading style
Expected: setext; Actual: atx
(MD003, heading-style)
130-130: Heading style
Expected: setext; Actual: atx
(MD003, heading-style)
267-267: Bare URL used
null
(MD034, no-bare-urls)
350-350: Bare URL used
null
(MD034, no-bare-urls)
🪛 LanguageTool
readme.md
[misspelling] ~142-~142: This word is normally spelled with a hyphen.
Context: ... event system to make sure the lib runs cross platform and stays lightweight. NOT YET IMPLEMEN...
(EN_COMPOUNDS_CROSS_PLATFORM)
🔇 Additional comments (1)
readme.md (1)
270-278: Validate the Tor Proxy Configuration Example.
The Rust code example for setting upLndNodewith a Tor Socks5 proxy is clear. Please ensure that the placeholder values (such as"YOUR_LND_ONION_ADDRESS.onion"and"YOUR_MACAROON") are documented elsewhere as examples and that this configuration is in sync with the updatedreqwestdependency featuring SOCKS support.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
readme.md (5)
6-8: Updated Feature List
The bullet list now highlights support for major nodes, protocols, and Tor. Consider standardizing terminology (e.g. “REST API” instead of “REST apis”) and ensuring consistent punctuation.Also applies to: 10-10
15-15: Markdown Heading Style Consistency
Several headings (such as “### Interface API Examples”, “#### Rust”, “#### Typescript”, etc.) use ATX style. The static analysis suggests a setext style might be preferred per project guidelines. Please review and update the heading style for consistency with the markdown lint rules.Also applies to: 17-17, 64-64, 105-105, 122-122, 127-127, 133-133
🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
15-15: Heading levels should only increment by one level at a time
Expected: h2; Actual: h3(MD001, heading-increment)
15-15: Heading style
Expected: setext; Actual: atx(MD003, heading-style)
127-131: Channel Management Placeholder
The “Channel Management” example correctly notes that the feature is “Not implemented.” Consider adding a brief note that this area is under active development for clarity to end users.🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
127-127: Heading style
Expected: setext; Actual: atx(MD003, heading-style)
270-271: Tor Proxy Documentation Clarity
The Tor section now clearly explains the SOCKS5 proxy settings and emphasizes the need for “socks5h://” when using onion addresses. To improve readability and prevent bare URL issues, consider converting the URL into a markdown hyperlink.🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
270-270: Bare URL used
null(MD034, no-bare-urls)
353-353: Bare URL in TODO List
The TODO list item includes a bare URL for starting up a Tor SOCKS5 proxy. It is recommended to replace the bare URL with a markdown link to enhance documentation consistency.🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
353-353: Bare URL used
null(MD034, no-bare-urls)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
readme.md(5 hunks)
🧰 Additional context used
🪛 LanguageTool
readme.md
[uncategorized] ~9-~9: The official spelling of this programming framework is “Node.js”.
Context: ...upport for kotlin, swift, react-native, nodejs (typescript, javaScript). No support fo...
(NODE_JS)
[misspelling] ~145-~145: This word is normally spelled with a hyphen.
Context: ... event system to make sure the lib runs cross platform and stays lightweight. NOT YET IMPLEMEN...
(EN_COMPOUNDS_CROSS_PLATFORM)
🪛 markdownlint-cli2 (0.17.2)
readme.md
15-15: Heading levels should only increment by one level at a time
Expected: h2; Actual: h3
(MD001, heading-increment)
15-15: Heading style
Expected: setext; Actual: atx
(MD003, heading-style)
17-17: Heading style
Expected: setext; Actual: atx
(MD003, heading-style)
64-64: Heading style
Expected: setext; Actual: atx
(MD003, heading-style)
105-105: Heading style
Expected: setext; Actual: atx
(MD003, heading-style)
122-122: Heading style
Expected: setext; Actual: atx
(MD003, heading-style)
127-127: Heading style
Expected: setext; Actual: atx
(MD003, heading-style)
133-133: Heading style
Expected: setext; Actual: atx
(MD003, heading-style)
270-270: Bare URL used
null
(MD034, no-bare-urls)
353-353: Bare URL used
null
(MD034, no-bare-urls)
🔇 Additional comments (6)
readme.md (6)
1-2: Title Update: “LNI Remote - Lightning Node Interface Remote”
The new title clearly reflects the expanded scope of the project. Ensure that all internal references and marketing materials are updated accordingly.
4-4: Overview Enhancement
The revised introductory sentence emphasizes remote connectivity and standardization across node implementations. It sets a clear context for users.
19-32: Rust API Usage Example
The Rust code snippet clearly demonstrates node instantiation and API usage with the new configuration objects. It effectively shows asynchronous operations (e.g., using.await). Ensure that any error handling in real-world examples is documented elsewhere.
107-120: Updated Payment API Signatures
The “Payments” section now lists updated method signatures that align with the refactored API. This is clear and informative.
122-125: Node Management Example
The example fornode.get_info()is succinct and now reflects the new return type. Everything looks consistent.🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
122-122: Heading style
Expected: setext; Actual: atx(MD003, heading-style)
274-280: TOR Proxy Example Verification
The provided Rust example for configuringLndConfigwithsocks5_proxy(andaccept_invalid_certs) is precise and in line with the new API updates.
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
bindings/lni_nodejs/index.d.ts(2 hunks)bindings/lni_nodejs/src/lnd.rs(5 hunks)bindings/lni_uniffi/src/lni.udl(3 hunks)crates/lni/cln/api.rs(20 hunks)crates/lni/cln/lib.rs(4 hunks)crates/lni/lnd/api.rs(12 hunks)crates/lni/lnd/lib.rs(4 hunks)crates/lni/phoenixd/api.rs(13 hunks)crates/lni/phoenixd/lib.rs(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- bindings/lni_uniffi/src/lni.udl
- crates/lni/lnd/lib.rs
- bindings/lni_nodejs/index.d.ts
- bindings/lni_nodejs/src/lnd.rs
- crates/lni/lnd/api.rs
🧰 Additional context used
🧬 Code Definitions (4)
crates/lni/cln/lib.rs (2)
bindings/lni_nodejs/index.d.ts (5)
ClnConfig(28-34)ClnNode(35-37)NodeInfo(56-70)Transaction(71-85)PayCode(204-211)crates/lni/cln/api.rs (6)
get_info(34-103)create_invoice(106-197)params(248-251)pay_invoice(199-278)get_offer(298-311)list_transactions(560-569)
crates/lni/phoenixd/lib.rs (2)
bindings/lni_nodejs/index.d.ts (5)
PhoenixdConfig(6-12)PhoenixdNode(13-15)NodeInfo(56-70)Transaction(71-85)PayCode(204-211)crates/lni/phoenixd/api.rs (7)
get_info(35-74)create_invoice(76-137)pay_invoice(139-171)get_offer(182-199)pay_offer(201-234)lookup_invoice(239-270)list_transactions(272-406)
crates/lni/phoenixd/api.rs (1)
crates/lni/phoenixd/lib.rs (5)
new(38-40)get_info(42-44)get_offer(68-70)lookup_invoice(81-86)list_transactions(88-93)
crates/lni/cln/api.rs (4)
bindings/lni_nodejs/index.d.ts (2)
ClnConfig(28-34)NodeInfo(56-70)crates/lni/cln/lib.rs (2)
new(37-39)get_info(41-43)bindings/lni_nodejs/src/cln.rs (2)
new(12-14)get_info(32-37)bindings/lni_nodejs/src/phoenixd.rs (2)
new(13-15)get_info(33-38)
🔇 Additional comments (16)
crates/lni/phoenixd/lib.rs (4)
16-18: Add Tor SOCKS proxy and security configuration supportThese new configuration options allow connecting to Phoenixd nodes through Tor for enhanced privacy and security. The SOCKS5 proxy support enables Tor connections, while the accept_invalid_certs option assists with self-signed certificates commonly used in Lightning nodes.
20-29: Good implementation of sensible defaultsThe Default implementation provides reasonable defaults with:
- A typical localhost URL
- Security-focused defaults (accept_invalid_certs defaults to true for self-signed certs)
- A reasonable HTTP timeout of 60 seconds for Lightning operations
33-34: Enhances configuration management with unified config objectRefactoring to use a single config object instead of separate url and password fields improves maintainability and follows best practices for configuration management.
119-121: Clear commented testing configuration for SOCKS5The comments provide useful examples of how to configure a SOCKS5 proxy for testing, making it easier for developers to experiment with Tor connections.
crates/lni/cln/lib.rs (4)
15-17: Add Tor SOCKS proxy support for CLN nodesThese new configuration options enable connecting to Core Lightning nodes through Tor for enhanced privacy and security. This matches the approach used in the PhoenixdConfig, maintaining consistency across implementations.
19-28: Well-structured default implementationThe Default implementation provides reasonable defaults with:
- A sensible localhost URL
- Secure defaults (accept_invalid_certs = true for self-signed certificates)
- A reasonable HTTP timeout (60 seconds) appropriate for Lightning Network operations
33-34: Improves configuration management with unified config objectRefactoring to use a single config object instead of separate url and rune fields improves code organization and maintainability. This is consistent with the approach used in PhoenixdConfig.
135-137: Provides clear testing configuration example for SOCKS5The commented code shows how to configure a SOCKS5 proxy for testing, making it easier for developers to experiment with Tor connections while using sensible defaults for other configuration options.
crates/lni/phoenixd/api.rs (3)
18-33: Robust client configuration for Tor proxy supportThe client function properly configures the reqwest HTTP client with:
- SOCKS5 proxy support when specified
- Certificate validation options
- Configurable timeout settings
This implementation handles all configuration options in a clean and maintainable way.
21-22: Verify proxy initialization error handlingThe unwrap() call on proxy creation could fail if an invalid proxy URL is provided, which might lead to a panic at runtime.
Consider adding error handling to gracefully manage invalid proxy configurations:
- let proxy = reqwest::Proxy::all(&config.socks5_proxy.clone().unwrap_or_default()).unwrap(); + let proxy = match reqwest::Proxy::all(&config.socks5_proxy.clone().unwrap_or_default()) { + Ok(proxy) => proxy, + Err(e) => { + eprintln!("Invalid proxy configuration: {}", e); + return reqwest::blocking::ClientBuilder::new().build().unwrap_or_default(); + } + };
35-45: Improved API design with consolidated configurationThe updated function signature accepting a single config parameter rather than separate URL and password parameters improves code maintainability and follows best practices for API design.
crates/lni/cln/api.rs (5)
15-32: Well-implemented client configuration for Tor supportThe clnrest_client function properly sets up an HTTP client with:
- Authentication headers
- SOCKS5 proxy support
- SSL certificate validation options
- Configurable timeout settings
This implementation follows best practices for HTTP client configuration.
34-38: Improved function signature with configuration objectThe updated get_info function signature using a single config parameter follows good API design practices and is consistent with other functions in the codebase.
433-436: Proper async pattern implementationThe code correctly uses async/await for the fetch_invoice_from_offer call, ensuring that async operations are properly chained. This is a good implementation of async patterns.
560-569: Clean implementation of list_transactions as a wrapperThis implementation correctly reuses the lookup_invoices function, converting the parameters appropriately and maintaining the DRY (Don't Repeat Yourself) principle.
74-80:❓ Verification inconclusive
Ensure compatibility with changed CHANNELD states
Line 79 adds a new CHANNELD state "OPENINGD" for pending open channels classification. This seems correct but should be verified against CLN documentation.
🌐 Web query:
What channel states exist in Core Lightning (CLN)?💡 Result:
Core Lightning (CLN) channels transition through several distinct states during their lifecycle, reflecting their operational status and interactions with the Bitcoin blockchain. Here's a breakdown of key channel states:
Channel Lifecycle States
1. Opening Phase
- CHANNELD_AWAITING_LOCKIN: Initial state after channel creation, waiting for the funding transaction to receive sufficient blockchain confirmations (3 for mainnet, 1 for testnet)[4][15].
- CHANNELD_NORMAL: Active state after confirmation, ready to route payments[4][19].
2. Operational States
- Public/Private Channels:
- Public channels are broadcast to the network for routing[2][11].
- Private channels remain unannounced, requiring direct routing hints for payments[2][7].
3. Closing States
- CLOSINGD_SIGEXCHANGE: Cooperative closure initiated by mutual agreement[19].
- ONCHAIN: Unilateral closure process where channel balances are resolved via blockchain transactions[1][11].
4. Recovery & Dispute States
- AWAITING_REVOCATION: Temporary state during channel updates while waiting for counterparty's revocation[1].
- FUNDING_SPEND_SEEN: On-chain detection of channel closure attempt, triggering dispute resolution[1].
Special Channel Types
Channel Type Key Characteristics Anchor Channels Enable dynamic fee adjustment for commitment transactions[2][7] Turbo (Zero-Conf) Instant usage without blockchain confirmation (requires trust)[2][11] Zombie Channels Inactive channels binding capital, needing manual closure[2] Spliced Channels Resizable capacity through on-chain modifications (experimental)[11] Channel state management is critical for node operators, as improper handling can lead to capital lockups or fund loss[1][5]. The
listpeerscommand provides real-time channel status monitoring, while backups only preserve channel relationships rather than exact balances due to Lightning's punishment system for revoked states[1][17].Citations:
- 1: https://docs.start9.com/0.3.4.x/support/faq/faq-lightning.html
- 2: https://docs.lightning.engineering/the-lightning-network/payment-channels/etymology
- 3: https://raspibolt.org/guide/bonus/lightning/cln.html
- 4: https://github.com/ElementsProject/lightning
- 5: https://cdecker-lightning.readthedocs.io/FAQ.html
- 6: https://www.youtube.com/watch?v=boaaqjInqhw
- 7: https://blog.blockstream.com/core-lightning-v24-08-steel-backed-up-channels/
- 8: https://www.youtube.com/watch?v=Lojku8Tstyg
- 9: https://community.start9.com/t/cln-core-lightning-how-can-i-backup-the-wallet-seed-on-star-9-i-rtfm/2377
- 10: http://corelightning.org
- 11: https://www.fidelitydigitalassets.com/research-and-insights/introduction-channel-splicing-bitcoins-lightning-network
- 12: https://www.voltage.cloud/blog/what-are-the-differences-between-lnd-and-cln
- 13: https://docs.corelightning.org/docs/faq
- 14: https://www.gijsvandam.nl/post/all-types-of-multi-part-payments-in-lightning-network-explained/
- 15: https://docs.corelightning.org/docs/opening-channels
- 16: https://www.voltage.cloud/blog/lightning-channel-strategy-guide-and-faq
- 17: https://docs.corelightning.org/docs/backup
- 18: https://docs.corelightning.org/reference/lightningd-config
- 19: https://docs.corelightning.org/reference/lightningd
- 20: https://docs.corelightning.org/docs/contribute-to-core-lightning
Verify CLN Channel State Compatibility
The updated condition now includes the "OPENINGD" state along with the various DUALOPEND states. However, current Core Lightning documentation does not list "OPENINGD" among the standard channel states (which include "CHANNELD_AWAITING_LOCKIN" and "CHANNELD_NORMAL" among others). Please verify whether "OPENINGD" is an intentional addition based on a recent CLN change or experimental behavior. If "OPENINGD" is valid, consider adding a comment or documentation reference clarifying its role; if not, adjust the condition accordingly.
- File: crates/lni/cln/api.rs, Lines 74-80
| .expect("Failed to get node info"); | ||
| let response_text = response.text().unwrap(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider improving error handling in HTTP requests
The code uses expect() to handle HTTP request failures, which will panic the application if a request fails. In a production environment, this could lead to service disruptions.
Consider returning proper errors rather than using expect():
- let response = client
- .get(&info_url)
- .basic_auth("", Some(config.password.clone()))
- .send()
- .expect("Failed to get node info");
- let response_text = response.text().unwrap();
+ let response = match client
+ .get(&info_url)
+ .basic_auth("", Some(config.password.clone()))
+ .send() {
+ Ok(resp) => resp,
+ Err(e) => return Err(ApiError::Http { reason: format!("Failed to get node info: {}", e) }),
+ };
+ let response_text = match response.text() {
+ Ok(text) => text,
+ Err(e) => return Err(ApiError::Http { reason: format!("Failed to parse response: {}", e) }),
+ };📝 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.
| .expect("Failed to get node info"); | |
| let response_text = response.text().unwrap(); | |
| let response = match client | |
| .get(&info_url) | |
| .basic_auth("", Some(config.password.clone())) | |
| .send() { | |
| Ok(resp) => resp, | |
| Err(e) => return Err(ApiError::Http { reason: format!("Failed to get node info: {}", e) }), | |
| }; | |
| let response_text = match response.text() { | |
| Ok(text) => text, | |
| Err(e) => return Err(ApiError::Http { reason: format!("Failed to parse response: {}", e) }), | |
| }; |
| client.build().unwrap() | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Potential panic point on client builder
The unwrap() call on client.build() could fail and cause a panic, for example with invalid header values.
Consider handling potential errors:
- client.build().unwrap()
+ client.build().unwrap_or_else(|e| {
+ eprintln!("Error building HTTP client: {}", e);
+ reqwest::blocking::Client::new()
+ })📝 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.
| client.build().unwrap() | |
| } | |
| client.build().unwrap_or_else(|e| { | |
| eprintln!("Error building HTTP client: {}", e); | |
| reqwest::blocking::Client::new() | |
| }) | |
| } |
| let proxy = reqwest::Proxy::all(&config.socks5_proxy.clone().unwrap_or_default()).unwrap(); | ||
| client = client.proxy(proxy); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Error handling needed for proxy configuration
Similar to the phoenixd/api.rs file, the unwrap() call for proxy creation could fail if an invalid proxy URL is provided, potentially causing runtime panics.
Consider adding error handling for invalid proxy configurations:
- let proxy = reqwest::Proxy::all(&config.socks5_proxy.clone().unwrap_or_default()).unwrap();
+ let proxy = match reqwest::Proxy::all(&config.socks5_proxy.clone().unwrap_or_default()) {
+ Ok(proxy) => proxy,
+ Err(e) => {
+ eprintln!("Invalid proxy configuration: {}", e);
+ return reqwest::blocking::ClientBuilder::new().default_headers(headers).build().unwrap_or_default();
+ }
+ };📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let proxy = reqwest::Proxy::all(&config.socks5_proxy.clone().unwrap_or_default()).unwrap(); | |
| client = client.proxy(proxy); | |
| let proxy = match reqwest::Proxy::all(&config.socks5_proxy.clone().unwrap_or_default()) { | |
| Ok(proxy) => proxy, | |
| Err(e) => { | |
| eprintln!("Invalid proxy configuration: {}", e); | |
| return reqwest::blocking::ClientBuilder::new().default_headers(headers).build().unwrap_or_default(); | |
| } | |
| }; | |
| client = client.proxy(proxy); |
Summary by CodeRabbit
New Features
Refactor
Chores
Documentation