Conversation
📝 WalkthroughWalkthroughAdds a Lexe-backed LightningNode with wallet initialization, invoice and offer operations, transaction lookup, event polling, public factory wiring, tests, and README usage documentation. ChangesLexe backend integration
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant LexeNode
participant LexeAPI
participant LexeWallet
Client->>LexeNode: create_lexe_node(config)
LexeNode->>LexeAPI: create_invoice(params)
LexeAPI->>LexeWallet: create invoice
LexeWallet-->>LexeAPI: invoice payment data
LexeAPI-->>LexeNode: Transaction
LexeNode-->>Client: LightningNode result
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 83f81bd130
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/lni/lexe/api.rs`:
- Around line 256-276: Bound the caller-controlled limit in matching_payments
and the surrounding API parameter validation to the public API’s maximum page
size, rejecting params.limit values above that maximum. Avoid
Vec::with_capacity(limit) based on the untrusted request; use a bounded capacity
or no preallocation while preserving pagination and result limits.
- Around line 402-408: Update the timeout handling in the payment flow around
wallet.pay_invoice to reconcile the remote payment state using the invoice
payment hash before returning. If reconciliation cannot determine whether
payment completed, return an explicit indeterminate result that requires lookup
before retrying instead of the generic NetworkError; preserve the existing
successful payment path.
- Around line 519-543: Update on_invoice_events to reject requests where both
params.payment_hash and params.search are empty, using the same selector
validation behavior as lookup_invoice before entering the polling loop. Preserve
the existing callback.failure(None) response and avoid calling matching_payments
without at least one event selector.
- Around line 99-104: Update the routing-options validation condition to reject
only enabled unsupported options, allowing optional boolean fields set to
Some(false) while continuing to reject Some(true). Preserve the existing
handling for max_parts and hop public keys, and adjust the is_amp and
allow_self_payment checks in the surrounding validation logic.
In `@crates/lni/lexe/lib.rs`:
- Around line 306-307: Remove the payment preimage output from the affected
diagnostic paths, including the println! and corresponding dbg! calls around
payment handling. Ensure the locations also covered at the referenced lines do
not log payment.preimage, while retaining only non-sensitive payment identifiers
if diagnostics are still required.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0aa055ad-db06-4084-a514-2233e5cb96f8
📒 Files selected for processing (6)
crates/lni/Cargo.tomlcrates/lni/lexe/api.rscrates/lni/lexe/lib.rscrates/lni/lib.rscrates/lni/types.rsreadme.md
Comment on lines
+99
to
+104
| if params.max_parts.is_some() | ||
| || params.first_hop_pubkey.is_some() | ||
| || params.last_hop_pubkey.is_some() | ||
| || params.allow_self_payment.is_some() | ||
| || params.is_amp.is_some() | ||
| { |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Accept explicitly disabled routing options.
Some(false) requests no unsupported behavior, but this rejects it merely because the field is present. This can break clients that serialize optional booleans explicitly.
Proposed fix
if params.max_parts.is_some()
|| params.first_hop_pubkey.is_some()
|| params.last_hop_pubkey.is_some()
- || params.allow_self_payment.is_some()
- || params.is_amp.is_some()
+ || params.allow_self_payment.unwrap_or(false)
+ || params.is_amp.unwrap_or(false)📝 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.
Suggested change
| if params.max_parts.is_some() | |
| || params.first_hop_pubkey.is_some() | |
| || params.last_hop_pubkey.is_some() | |
| || params.allow_self_payment.is_some() | |
| || params.is_amp.is_some() | |
| { | |
| if params.max_parts.is_some() | |
| || params.first_hop_pubkey.is_some() | |
| || params.last_hop_pubkey.is_some() | |
| || params.allow_self_payment.unwrap_or(false) | |
| || params.is_amp.unwrap_or(false) | |
| { |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/lni/lexe/api.rs` around lines 99 - 104, Update the routing-options
validation condition to reject only enabled unsupported options, allowing
optional boolean fields set to Some(false) while continuing to reject
Some(true). Preserve the existing handling for max_parts and hop public keys,
and adjust the is_amp and allow_self_payment checks in the surrounding
validation logic.
Comment on lines
+256
to
+276
| async fn matching_payments( | ||
| wallet: &LexeWallet, | ||
| payment_hash: Option<&str>, | ||
| search: Option<&str>, | ||
| created_after: Option<i64>, | ||
| created_before: Option<i64>, | ||
| skip: usize, | ||
| limit: usize, | ||
| ) -> Result<Vec<Payment>, ApiError> { | ||
| if limit == 0 { | ||
| return Ok(Vec::new()); | ||
| } | ||
|
|
||
| wallet | ||
| .sync_payments() | ||
| .await | ||
| .map_err(|error| api_error("Failed to sync Lexe payments", error))?; | ||
|
|
||
| let mut after = None; | ||
| let mut skipped = 0usize; | ||
| let mut matches = Vec::with_capacity(limit); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Bound the caller-controlled transaction limit.
An i64::MAX limit is accepted and passed to Vec::with_capacity(limit), which can panic or abort on allocation; large values also trigger unbounded pagination. Enforce a reasonable maximum and avoid preallocating the full requested size.
Proposed defensive allocation
- let mut matches = Vec::with_capacity(limit);
+ let mut matches = Vec::with_capacity(limit.min(PAYMENT_PAGE_SIZE));Also reject params.limit above the public API’s maximum page size.
Also applies to: 466-474
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/lni/lexe/api.rs` around lines 256 - 276, Bound the caller-controlled
limit in matching_payments and the surrounding API parameter validation to the
public API’s maximum page size, rejecting params.limit values above that
maximum. Avoid Vec::with_capacity(limit) based on the untrusted request; use a
bounded capacity or no preallocation while preserving pagination and result
limits.
Comment on lines
+402
to
+408
| let payment = if let Some(timeout_secs) = params.timeout_seconds { | ||
| tokio::time::timeout( | ||
| Duration::from_secs(timeout_secs as u64), | ||
| wallet.pay_invoice(request), | ||
| ) | ||
| .await | ||
| .map_err(|_| ApiError::NetworkError("Lexe payment timed out".to_owned()))? |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Reconcile payment state after a timeout.
The timeout only stops the local future; the remote payment may still complete. Returning a generic network error can cause callers to retry an indeterminate payment. Reconcile using the invoice payment hash, or return an explicit indeterminate result requiring lookup before retry.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/lni/lexe/api.rs` around lines 402 - 408, Update the timeout handling
in the payment flow around wallet.pay_invoice to reconcile the remote payment
state using the invoice payment hash before returning. If reconciliation cannot
determine whether payment completed, return an explicit indeterminate result
that requires lookup before retrying instead of the generic NetworkError;
preserve the existing successful payment path.
Comment on lines
+519
to
+543
| pub async fn on_invoice_events( | ||
| wallet: Arc<LexeWallet>, | ||
| params: OnInvoiceEventParams, | ||
| callback: Arc<dyn OnInvoiceEventCallback>, | ||
| ) { | ||
| if params.polling_delay_sec <= 0 || params.max_polling_sec <= 0 { | ||
| callback.failure(None); | ||
| return; | ||
| } | ||
|
|
||
| let started = tokio::time::Instant::now(); | ||
| let max_polling = Duration::from_secs(params.max_polling_sec as u64); | ||
| let polling_delay = Duration::from_secs(params.polling_delay_sec as u64); | ||
| let mut last_transaction = None; | ||
|
|
||
| while started.elapsed() < max_polling { | ||
| match matching_payments( | ||
| &wallet, | ||
| params.payment_hash.as_deref(), | ||
| params.search.as_deref(), | ||
| None, | ||
| None, | ||
| 0, | ||
| 1, | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Require an event selector before polling.
With both payment_hash and search empty, matching_payments matches the newest arbitrary wallet payment. The callback can therefore report an unrelated transaction immediately. Apply the same selector validation used by lookup_invoice.
Proposed fix
) {
+ if params.payment_hash.as_deref().is_none_or(str::is_empty)
+ && params.search.as_deref().is_none_or(str::is_empty)
+ {
+ callback.failure(None);
+ return;
+ }
+
if params.polling_delay_sec <= 0 || params.max_polling_sec <= 0 {📝 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.
Suggested change
| pub async fn on_invoice_events( | |
| wallet: Arc<LexeWallet>, | |
| params: OnInvoiceEventParams, | |
| callback: Arc<dyn OnInvoiceEventCallback>, | |
| ) { | |
| if params.polling_delay_sec <= 0 || params.max_polling_sec <= 0 { | |
| callback.failure(None); | |
| return; | |
| } | |
| let started = tokio::time::Instant::now(); | |
| let max_polling = Duration::from_secs(params.max_polling_sec as u64); | |
| let polling_delay = Duration::from_secs(params.polling_delay_sec as u64); | |
| let mut last_transaction = None; | |
| while started.elapsed() < max_polling { | |
| match matching_payments( | |
| &wallet, | |
| params.payment_hash.as_deref(), | |
| params.search.as_deref(), | |
| None, | |
| None, | |
| 0, | |
| 1, | |
| ) | |
| pub async fn on_invoice_events( | |
| wallet: Arc<LexeWallet>, | |
| params: OnInvoiceEventParams, | |
| callback: Arc<dyn OnInvoiceEventCallback>, | |
| ) { | |
| if params.payment_hash.as_deref().is_none_or(str::is_empty) | |
| && params.search.as_deref().is_none_or(str::is_empty) | |
| { | |
| callback.failure(None); | |
| return; | |
| } | |
| if params.polling_delay_sec <= 0 || params.max_polling_sec <= 0 { | |
| callback.failure(None); | |
| return; | |
| } | |
| let started = tokio::time::Instant::now(); | |
| let max_polling = Duration::from_secs(params.max_polling_sec as u64); | |
| let polling_delay = Duration::from_secs(params.polling_delay_sec as u64); | |
| let mut last_transaction = None; | |
| while started.elapsed() < max_polling { | |
| match matching_payments( | |
| &wallet, | |
| params.payment_hash.as_deref(), | |
| params.search.as_deref(), | |
| None, | |
| None, | |
| 0, | |
| 1, | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/lni/lexe/api.rs` around lines 519 - 543, Update on_invoice_events to
reject requests where both params.payment_hash and params.search are empty,
using the same selector validation behavior as lookup_invoice before entering
the polling loop. Preserve the existing callback.failure(None) response and
avoid calling matching_payments without at least one event selector.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit