On Invoice Events #9
Conversation
WalkthroughThis pull request updates dependency versions and integrates an invoice event polling mechanism across both Node.js and Rust components. New interfaces and methods are added to handle invoice event parameters and callbacks. The changes cover TypeScript definitions, new asynchronous calls in JavaScript, and multiple Rust functions that implement a polling loop with callback handling. Additionally, configuration files and documentation are updated to reference the new event polling feature. Changes
Sequence Diagram(s)sequenceDiagram
participant App as Application
participant Node as Node Instance
participant API as Polling API
participant CB as Callback Handler
App->>Node: onInvoiceEvents(params, callback)
Node->>API: poll_invoice_events(config, params, callback)
loop Polling Loop
API->>API: lookup_invoice(paymentHash)
alt Invoice Settled
API->>CB: success(transaction)
else Invoice Pending
API->>CB: pending(transaction)
else Timeout/Error
API->>CB: failure()
end
end
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 (6)
bindings/lni_nodejs/src/lnd.rs (1)
108-118: Good implementation of Node.js binding for invoice events.The method implementation correctly:
- Takes the necessary parameters for invoice event polling
- Passes them to the underlying Rust API function
- Converts callback errors into appropriate NAPI errors
- Uses proper type constraints for the callback function
One suggestion: consider adding error handling for the callback invocation. The current implementation discards any errors returned by the callback.
- callback(status.clone(), tx.clone()).map_err(|err| napi::Error::from_reason(err.to_string())); + if let Err(err) = callback(status.clone(), tx.clone()) { + eprintln!("Error in invoice event callback: {}", err); + }readme.md (2)
136-136: Fix hyphenation in "cross platform".The term "cross platform" should be hyphenated as "cross-platform" according to standard English conventions.
-Polling is used instead of a heavier grpc/pubsub (for now) to make sure the lib runs cross platform and stays lightweight. +Polling is used instead of a heavier grpc/pubsub (for now) to make sure the lib runs cross-platform and stays lightweight.🧰 Tools
🪛 LanguageTool
[misspelling] ~136-~136: This word is normally spelled with a hyphen.
Context: ...sub (for now) to make sure the lib runs cross platform and stays lightweight. Typescript for ...(EN_COMPOUNDS_CROSS_PLATFORM)
164-179: Correct framework name from "nodejs" to "Node.js".The official spelling of the JavaScript runtime is "Node.js" with the capitalization and period.
-Typescript for nodejs +TypeScript for Node.js🧰 Tools
🪛 LanguageTool
[uncategorized] ~164-~164: The official spelling of this programming framework is “Node.js”.
Context: ... }, } );Typescript for nodejstypescript await node.onInvoiceEvent...(NODE_JS)
crates/lni/lnd/lib.rs (2)
140-140: Commented out socks5_proxy configuration.The commented line is likely used for testing with Tor but isn't currently active. Consider adding a comment explaining why it's commented out or remove it if it's not needed.
- //socks5_proxy: Some("socks5h://127.0.0.1:9150".to_string()), // Tor socks5 proxy using arti + // Uncomment to test with Tor + // socks5_proxy: Some("socks5h://127.0.0.1:9150".to_string()), // Tor socks5 proxy using arti
348-395: Robust test implementation for invoice event polling.The test appropriately implements the
OnInvoiceEventCallbacktrait and uses thread-safe data structures to collect events. However, there are a few aspects that could be improved:
- The test uses a fixed sleep duration which could lead to flaky tests in CI environments
- There's no validation of the actual event content
Consider enhancing the test to make it more robust:
#[test] fn test_on_invoice_events() { struct OnInvoiceEventCallback { events: Arc<Mutex<Vec<String>>>, + done_signal: Arc<Mutex<bool>>, } impl crate::types::OnInvoiceEventCallback for OnInvoiceEventCallback { fn success(&self, transaction: Option<Transaction>) { let mut events = self.events.lock().unwrap(); events.push(format!("{} - {:?}", "success", transaction)); + let mut done = self.done_signal.lock().unwrap(); + *done = true; } fn pending(&self, transaction: Option<Transaction>) { let mut events = self.events.lock().unwrap(); events.push(format!("{} - {:?}", "pending", transaction)); } fn failure(&self, transaction: Option<Transaction>) { let mut events = self.events.lock().unwrap(); events.push(format!("{} - {:?}", "failure", transaction)); + let mut done = self.done_signal.lock().unwrap(); + *done = true; } } let events = Arc::new(Mutex::new(Vec::new())); + let done_signal = Arc::new(Mutex::new(false)); let callback = OnInvoiceEventCallback { events: events.clone(), + done_signal: done_signal.clone(), }; let params= crate::types::OnInvoiceEventParams { payment_hash: TEST_PAYMENT_HASH.to_string(), polling_delay_sec: 1, // Use shorter delay for faster tests max_polling_sec: 15, }; // Start the event listener in a separate thread thread::spawn(move || { NODE.on_invoice_events(params, Box::new(callback)); }); - // Wait for some time to allow events to be collected - thread::sleep(Duration::from_secs(10)); + // Wait for completion signal or timeout + let start_time = std::time::Instant::now(); + let timeout = Duration::from_secs(12); + while !(*done_signal.lock().unwrap()) { + thread::sleep(Duration::from_millis(100)); + if start_time.elapsed() > timeout { + break; + } + } // Check if events were received let received_events = events.lock().unwrap(); println!("Received events: {:?}", *received_events); assert!( !received_events.is_empty(), "Expected to receive at least one invoice event" ); + + // Validate event content if events were received + if !received_events.is_empty() { + let first_event = &received_events[0]; + assert!( + first_event.contains("success") || first_event.contains("pending") || first_event.contains("failure"), + "Event should contain a valid status" + ); + } }crates/lni/lnd/api.rs (1)
463-504: Well-implemented polling mechanism with appropriate timeout handling.The
poll_invoice_eventsfunction implements a clean polling mechanism that:
- Properly handles timeouts using
max_polling_sec- Correctly maps invoice states to status strings
- Efficiently uses
thread::sleepfor intervalsHowever, there are a few potential improvements:
- The error case doesn't provide detailed error information, which could help with debugging
- There's no early exit if the connection to the LND node fails repeatedly
Consider enhancing error reporting and adding retry limits:
pub fn poll_invoice_events<F>(config: &LndConfig, params: OnInvoiceEventParams, mut callback: F) where F: FnMut(String, Option<Transaction>), { let mut start_time = std::time::Instant::now(); + let mut consecutive_errors = 0; loop { if start_time.elapsed() > Duration::from_secs(params.max_polling_sec as u64) { // timeout - callback("failure".to_string(), None); + callback("timeout".to_string(), None); break; } let (status, transaction) = match lookup_invoice(config, Some(params.payment_hash.clone())) { Ok(transaction) => { + consecutive_errors = 0; if transaction.settled_at > 0 { ("settled".to_string(), Some(transaction)) } else { ("pending".to_string(), Some(transaction)) } } - Err(_) => ("error".to_string(), None), + Err(e) => { + consecutive_errors += 1; + println!("Error looking up invoice: {:?}", e); + + // After 3 consecutive errors, abort polling to avoid wasting resources + if consecutive_errors >= 3 { + ("connection_failed".to_string(), None) + } else { + ("error".to_string(), None) + } + }, }; match status.as_str() { "settled" => { callback("success".to_string(), transaction); break; } - "error" => { + "error" | "connection_failed" => { callback("failure".to_string(), transaction); - break; + if status == "connection_failed" { + break; + } } _ => { callback("pending".to_string(), transaction); } } thread::sleep(Duration::from_secs(params.polling_delay_sec as u64)); } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
bindings/lni_nodejs/Cargo.toml(1 hunks)bindings/lni_nodejs/index.d.ts(2 hunks)bindings/lni_nodejs/main.mjs(1 hunks)bindings/lni_nodejs/src/lnd.rs(1 hunks)bindings/lni_react_native/cpp/lni-react-native.cpp(0 hunks)bindings/lni_react_native/cpp/lni-react-native.h(0 hunks)bindings/lni_react_native/example/src/App.tsx(2 hunks)bindings/lni_react_native/src/NativeLniReactNative.ts(0 hunks)bindings/lni_react_native/src/index.tsx(0 hunks)bindings/lni_react_native/ubrn.config.yaml(1 hunks)crates/lni/Cargo.toml(1 hunks)crates/lni/lnd/api.rs(4 hunks)crates/lni/lnd/lib.rs(4 hunks)crates/lni/types.rs(2 hunks)readme.md(2 hunks)
💤 Files with no reviewable changes (4)
- bindings/lni_react_native/cpp/lni-react-native.cpp
- bindings/lni_react_native/src/index.tsx
- bindings/lni_react_native/src/NativeLniReactNative.ts
- bindings/lni_react_native/cpp/lni-react-native.h
🧰 Additional context used
🧬 Code Definitions (3)
crates/lni/types.rs (2)
crates/lni/lnd/lib.rs (3)
success(355-358)pending(359-362)failure(363-366)bindings/lni_nodejs/index.d.ts (2)
Transaction(71-85)OnInvoiceEventParams(224-228)
bindings/lni_react_native/example/src/App.tsx (1)
bindings/lni_nodejs/index.d.ts (1)
Transaction(71-85)
bindings/lni_nodejs/src/lnd.rs (2)
crates/lni/lnd/api.rs (3)
on_invoice_events(506-517)params(242-245)poll_invoice_events(464-504)bindings/lni_nodejs/index.d.ts (2)
Transaction(71-85)OnInvoiceEventParams(224-228)
🪛 LanguageTool
readme.md
[misspelling] ~136-~136: This word is normally spelled with a hyphen.
Context: ...sub (for now) to make sure the lib runs cross platform and stays lightweight. Typescript for ...
(EN_COMPOUNDS_CROSS_PLATFORM)
[uncategorized] ~164-~164: The official spelling of this programming framework is “Node.js”.
Context: ... }, } ); Typescript for nodejstypescript await node.onInvoiceEvent...
(NODE_JS)
🔇 Additional comments (17)
bindings/lni_react_native/ubrn.config.yaml (1)
4-4: Branch name update looks goodThe branch name has been updated from 'fix-react-native' to 'on-invoice-events', which correctly aligns with the focus of this PR.
bindings/lni_nodejs/Cargo.toml (1)
20-21: Dependency updates look goodUpgrading napi from 2.12.2 to 2.16.17 and napi-derive from 2.12.2 to 2.16.13 while maintaining the same features configuration. These updates are likely needed to support the new invoice event functionality.
crates/lni/Cargo.toml (1)
23-24: Dependency updates match Node.js bindingsThe napi and napi-derive versions have been updated to match those in the Node.js bindings (napi to 2.16.17 and napi-derive to 2.16.13). This maintains consistency across the project components.
bindings/lni_nodejs/main.mjs (2)
144-144: Proxy configuration commented outThe socks5Proxy configuration has been commented out. Ensure this won't affect connectivity in production environments if this test code is used as a reference.
149-159: Successfully implemented invoice event handlingThe implementation of
onInvoiceEventsadds support for polling invoice events with appropriate parameters (paymentHash, polling delay, maximum polling time) and a callback function for handling events. This aligns well with the PR objective of introducing comprehensive invoice event handling.crates/lni/types.rs (3)
32-33: Formatting improvement for better readability.The alignment of comments for these fields enhances code readability.
341-347: Good trait definition for invoice event callbacks.The
OnInvoiceEventCallbacktrait provides a clean interface for handling invoice events with appropriate methods for different states (success, pending, failure). Each method can receive an optional transaction, making it flexible enough to handle various scenarios.
349-355: Well-structured parameter object for invoice event polling.The
OnInvoiceEventParamsstruct encapsulates all necessary parameters for invoice event polling:
payment_hashto identify the specific invoicepolling_delay_secto control the frequency of pollingmax_polling_secto set an upper limit on polling durationThis design provides a clean API and makes it easy to extend with additional parameters if needed in the future.
bindings/lni_nodejs/index.d.ts (2)
224-228: Clean TypeScript interface for invoice event parameters.The TypeScript interface properly mirrors the Rust struct with appropriate types. The naming conventions follow JavaScript/TypeScript standards (camelCase) rather than Rust standards (snake_case), which is good for consistency in the TypeScript codebase.
279-279: Method signature correctly exposes the invoice event functionality.The
onInvoiceEventsmethod signature correctly:
- Takes the parameters object matching the Rust struct
- Provides a callback with appropriate argument types
- Returns void, matching the Rust implementation that returns
Result<()>This ensures proper type safety when using this method from TypeScript.
bindings/lni_react_native/example/src/App.tsx (1)
3-10: Clean import of necessary types.The import statement now correctly includes
OnInvoiceEventCallbackandTransactiontypes from thelni_react_nativemodule, which are needed for the new invoice event functionality.readme.md (4)
135-136: Explain why HTTP polling was chosen over other methods.The documentation clearly explains the trade-off of using HTTP polling instead of gRPC/pubsub, which helps users understand the design decision.
🧰 Tools
🪛 LanguageTool
[misspelling] ~136-~136: This word is normally spelled with a hyphen.
Context: ...sub (for now) to make sure the lib runs cross platform and stays lightweight. Typescript for ...(EN_COMPOUNDS_CROSS_PLATFORM)
138-162: Well-structured TypeScript example for React Native.The example clearly shows both polling parameters and the callback structure, making it easy for React Native developers to understand how to use the API.
182-202: Comprehensive Rust example with clear callback implementation.The example properly demonstrates the trait implementation pattern necessary for Rust callbacks and clearly shows how to configure the polling parameters.
416-416: Accurately updated Todo list.Marking "Simple event polling" as completed aligns with the newly implemented feature.
crates/lni/lnd/lib.rs (1)
92-98: Excellent addition of the invoice events method.The
on_invoice_eventsimplementation follows the established pattern of delegating to the API module and maintains a clean interface for consumers. The method signature is clear and takes appropriate parameters.crates/lni/lnd/api.rs (1)
506-517: Clean wrapper function for invoice event callbacks.The
on_invoice_eventsfunction provides a good abstraction by mapping status strings to the appropriate callback methods. It maintains the thread-safety of the callback by using a Box.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
readme.md (1)
134-202: Documentation for Event Polling is comprehensive and well-structured.The readme thoroughly documents the new polling functionality with:
- Clear explanation of the rationale for using HTTP polling
- Complete examples in TypeScript for both React Native and Node.js
- Corresponding Rust implementation example
- Proper parameter documentation
Minor typos:
- Line 136: "cross platform" should be hyphenated as "cross-platform"
- Line 164: "nodejs" should be written as "Node.js" (the official spelling)
-LNI does some simple event polling over http to get some basic invoice status events. -Polling is used instead of a heavier grpc/pubsub (for now) to make sure the lib runs cross platform and stays lightweight. +LNI does some simple event polling over http to get some basic invoice status events. +Polling is used instead of a heavier grpc/pubsub (for now) to make sure the lib runs cross-platform and stays lightweight. -Typescript for nodejs +Typescript for Node.js🧰 Tools
🪛 LanguageTool
[misspelling] ~136-~136: This word is normally spelled with a hyphen.
Context: ...sub (for now) to make sure the lib runs cross platform and stays lightweight. Typescript for ...(EN_COMPOUNDS_CROSS_PLATFORM)
[uncategorized] ~164-~164: The official spelling of this programming framework is “Node.js”.
Context: ... }, } );Typescript for nodejstypescript await node.onInvoiceEvent...(NODE_JS)
crates/lni/cln/lib.rs (1)
337-359: Test implementation could be improved with assertionsThe test creates the necessary structures and calls the method, but doesn't include any assertions to verify the behavior programmatically. Currently, it only prints to console which makes automated testing difficult.
Consider adding a way to capture and assert the callback invocations:
#[test] fn test_on_invoice_events() { + use std::sync::{Arc, Mutex}; + use std::time::Duration; + + // Capture callback invocations + let success_called = Arc::new(Mutex::new(false)); + let pending_called = Arc::new(Mutex::new(false)); + let success_called_clone = success_called.clone(); + let pending_called_clone = pending_called.clone(); + struct OnInvoiceEventCallback {} impl crate::types::OnInvoiceEventCallback for OnInvoiceEventCallback { fn success(&self, transaction: Option<Transaction>) { println!("success"); + let mut success = success_called_clone.lock().unwrap(); + *success = true; } fn pending(&self, transaction: Option<Transaction>) { println!("pending"); + let mut pending = pending_called_clone.lock().unwrap(); + *pending = true; } fn failure(&self, transaction: Option<Transaction>) { println!("epic fail"); } } let params = crate::types::OnInvoiceEventParams { payment_hash: TEST_PAYMENT_HASH.to_string(), polling_delay_sec: 3, max_polling_sec: 60, }; let callback = OnInvoiceEventCallback {}; NODE.on_invoice_events(params, Box::new(callback)); + + // Wait briefly to allow callbacks to be invoked + std::thread::sleep(Duration::from_millis(100)); + + // Assert that at least one callback method was called + assert!(*pending_called.lock().unwrap() || *success_called.lock().unwrap(), + "Neither success nor pending callback was called"); }crates/lni/lnd/api.rs (1)
506-517: Add documentation to the on_invoice_events functionThe function is missing documentation explaining its purpose and usage.
+/// Handles invoice events by polling for status changes +/// +/// This function wraps poll_invoice_events to work with the OnInvoiceEventCallback trait. +/// It maps status strings to specific callback methods. +/// +/// * `config` - The LND configuration +/// * `params` - Parameters for polling (payment hash, delay, max time) +/// * `callback` - Trait implementation for handling different event states pub fn on_invoice_events( config: LndConfig, params: OnInvoiceEventParams, callback: Box<dyn OnInvoiceEventCallback>, ) { poll_invoice_events(&config, params, move |status, tx| match status.as_str() { "success" => callback.success(tx), "pending" => callback.pending(tx), "failure" => callback.failure(tx), _ => {} }); }crates/lni/cln/api.rs (1)
568-608: New function for polling invoice eventsThis function implements a robust polling mechanism for invoice events with:
- Proper timeout handling
- Status detection based on transaction settlement
- Clear callback pattern for different invoice states
- Appropriate sleep between polling attempts
The implementation looks correct but contains a minor inefficiency.
- let mut start_time = std::time::Instant::now(); + let start_time = std::time::Instant::now();The
start_timevariable is marked as mutable but is never modified after initialization.crates/lni/phoenixd/api.rs (2)
415-456: Implementation of invoice polling mechanismThe polling mechanism implementation is well-structured with:
- Proper timeout handling
- Transaction status detection
- Appropriate callback invocation
- Clear exit conditions
Similar to the CLN implementation, there's a minor inefficiency with the mutable variable.
- let mut start_time = std::time::Instant::now(); + let start_time = std::time::Instant::now();The
start_timevariable is marked as mutable but is never modified after initialization.
415-469: Consider reducing code duplication across implementationsThe polling implementations are nearly identical across the CLN and Phoenixd modules. This could be a candidate for refactoring into a shared utility function.
You could consider creating a generic polling utility that accepts a lookup function and config type as parameters, reducing duplication while preserving type safety:
// In a shared utility module pub fn generic_poll_invoice_events<C, F, L>( config: &C, params: OnInvoiceEventParams, lookup_fn: L, mut callback: F, ) where F: FnMut(String, Option<Transaction>), L: Fn(&C, String) -> Result<Transaction, ApiError>, { let start_time = std::time::Instant::now(); loop { // Common polling logic here // ... } }Then implementers would only need to provide their specific lookup function.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
bindings/lni_nodejs/index.d.ts(4 hunks)bindings/lni_nodejs/main.mjs(1 hunks)bindings/lni_nodejs/src/cln.rs(1 hunks)bindings/lni_nodejs/src/phoenixd.rs(1 hunks)crates/lni/cln/api.rs(2 hunks)crates/lni/cln/lib.rs(2 hunks)crates/lni/lnd/api.rs(4 hunks)crates/lni/phoenixd/api.rs(3 hunks)crates/lni/phoenixd/lib.rs(2 hunks)readme.md(2 hunks)
🧰 Additional context used
🧬 Code Definitions (3)
bindings/lni_nodejs/src/cln.rs (7)
bindings/lni_nodejs/src/phoenixd.rs (1)
on_invoice_events(97-106)crates/lni/cln/lib.rs (1)
on_invoice_events(97-103)crates/lni/phoenixd/api.rs (2)
on_invoice_events(458-469)poll_invoice_events(416-456)crates/lni/phoenixd/lib.rs (1)
on_invoice_events(89-95)crates/lni/cln/api.rs (3)
on_invoice_events(610-621)params(249-252)poll_invoice_events(569-608)crates/lni/lnd/lib.rs (1)
on_invoice_events(92-98)bindings/lni_nodejs/src/lnd.rs (1)
on_invoice_events(109-118)
crates/lni/phoenixd/api.rs (8)
crates/lni/lnd/api.rs (14)
inv(367-370)inv(372-375)inv(377-380)inv(382-385)inv(387-390)inv(425-428)inv(430-433)inv(435-438)inv(440-443)inv(445-448)poll_invoice_events(464-504)params(242-245)lookup_invoice(342-397)on_invoice_events(506-517)crates/lni/cln/api.rs (4)
poll_invoice_events(569-608)params(249-252)lookup_invoice(469-487)on_invoice_events(610-621)bindings/lni_nodejs/src/phoenixd.rs (2)
lookup_invoice(68-72)on_invoice_events(97-106)bindings/lni_nodejs/src/cln.rs (2)
lookup_invoice(87-91)on_invoice_events(111-120)crates/lni/cln/lib.rs (2)
lookup_invoice(82-84)on_invoice_events(97-103)crates/lni/phoenixd/lib.rs (2)
lookup_invoice(78-80)on_invoice_events(89-95)crates/lni/lnd/lib.rs (2)
lookup_invoice(77-79)on_invoice_events(92-98)bindings/lni_nodejs/src/lnd.rs (2)
lookup_invoice(85-89)on_invoice_events(109-118)
crates/lni/lnd/api.rs (6)
bindings/lni_nodejs/src/cln.rs (2)
lookup_invoice(87-91)on_invoice_events(111-120)crates/lni/phoenixd/api.rs (3)
poll_invoice_events(416-456)lookup_invoice(246-277)on_invoice_events(458-469)crates/lni/lnd/lib.rs (2)
lookup_invoice(77-79)on_invoice_events(92-98)bindings/lni_nodejs/src/lnd.rs (2)
lookup_invoice(85-89)on_invoice_events(109-118)bindings/lni_nodejs/src/phoenixd.rs (2)
lookup_invoice(68-72)on_invoice_events(97-106)crates/lni/phoenixd/lib.rs (2)
lookup_invoice(78-80)on_invoice_events(89-95)
🪛 LanguageTool
readme.md
[misspelling] ~136-~136: This word is normally spelled with a hyphen.
Context: ...sub (for now) to make sure the lib runs cross platform and stays lightweight. Typescript for ...
(EN_COMPOUNDS_CROSS_PLATFORM)
[uncategorized] ~164-~164: The official spelling of this programming framework is “Node.js”.
Context: ... }, } ); Typescript for nodejstypescript await node.onInvoiceEvent...
(NODE_JS)
🔇 Additional comments (18)
bindings/lni_nodejs/main.mjs (2)
135-148: Implementation of PhoenixdNode configuration looks good!The code correctly configures the PhoenixdNode with environment variables and initializes it, replacing the previous LndNode implementation.
152-161: New onInvoiceEvents polling integration properly implemented.The implementation adds invoice event polling with:
- Appropriate payment hash from configuration
- Reasonable polling delay (4 seconds) and maximum polling time (60 seconds)
- Callback that logs event status and transaction details
This aligns well with the invoice event handling patterns described in the documentation.
bindings/lni_nodejs/src/cln.rs (1)
110-120: Good implementation of on_invoice_events for CLN Node.The method correctly:
- Takes OnInvoiceEventParams and a callback function
- Forwards the call to poll_invoice_events with appropriate parameters
- Handles error conversion from Rust to Node.js context
- Maintains consistent implementation pattern with other node types
This implementation aligns perfectly with the corresponding methods in LndNode and PhoenixdNode.
bindings/lni_nodejs/src/phoenixd.rs (1)
96-106: Well-implemented on_invoice_events for PhoenixdNode.The method correctly:
- Takes OnInvoiceEventParams and a callback function
- Forwards the call to poll_invoice_events with appropriate parameters
- Properly handles error conversion between Rust and Node.js
- Maintains a consistent implementation pattern with other node types
The implementation effectively bridges the Rust core library with the Node.js bindings for the invoice event polling functionality.
readme.md (1)
416-416: Todo list correctly updated.The "Simple event polling" task is now marked as completed, which accurately reflects the implementation of this feature in the codebase.
crates/lni/cln/lib.rs (1)
96-103: Function implementation looks goodThe implementation correctly delegates to the API function and maintains consistency with the other node implementations.
crates/lni/phoenixd/lib.rs (2)
88-95: Implementation aligns with other node typesThe implementation correctly delegates to the API function and maintains consistency with the ClnNode and LndNode implementations.
255-277: Test implementation has short max polling timeThe test uses a max polling time of 5 seconds, while the ClnNode test uses 60 seconds. This might be intentional, but could cause inconsistent test behavior if the invoice being checked takes longer than 5 seconds to settle.
Also, similar to the ClnNode test, this test lacks assertions to verify the callbacks are invoked. Consider modifying this test similarly to capture and verify callback invocations.
bindings/lni_nodejs/index.d.ts (2)
224-228: Interface looks goodThe OnInvoiceEventParams interface properly defines the required parameters for invoice event handling.
249-249: Method signatures are consistent across node typesThe onInvoiceEvents method is consistently added to all three node types with identical signatures, which is good API design.
Also applies to: 265-265, 281-281
crates/lni/lnd/api.rs (1)
1-3: Appropriate imports addedThe required imports for thread and Duration are correctly added to support the polling functionality.
Also applies to: 12-12
crates/lni/cln/api.rs (3)
7-9: New imports for invoice event polling functionalityThe imports of
Transaction, OnInvoiceEventCallback, OnInvoiceEventParamstypes look good and match the new functionality being added.
11-13: Additional imports for polling mechanismThe imports of
std::threadandstd::time::Durationare necessary for the new polling functionality.
610-621: Callback wrapper for invoice eventsThis function provides a cleaner interface to the polling mechanism by wrapping the raw callback with the
OnInvoiceEventCallbacktrait, making it more developer-friendly to implement event handlers.crates/lni/phoenixd/api.rs (4)
7-9: New imports for invoice event functionalityThese imports of
OnInvoiceEventCallbackandOnInvoiceEventParamsproperly align with the new event handling capabilities being added.
11-12: Thread and Duration imports for pollingThe imports of
std::threadandstd::time::Durationare necessary for implementing the polling mechanism.
270-270: Fix for transaction settled_at fieldGood improvement - setting
settled_attoinv.completed_atprovides accurate settlement data instead of using a hardcoded value.
458-469: Callback wrapper for invoice eventsThis wrapper function appropriately maps the generic status callbacks to the specific
OnInvoiceEventCallbacktrait methods, providing a clean interface for event handling.
Summary by CodeRabbit
New Features
Documentation
Chores