Review: Add currency conversion support for BOLT 12 offers#10
Review: Add currency conversion support for BOLT 12 offers#10
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughIntroduce a CurrencyConversion trait and thread a currency-conversion dependency (generic CC / &CC) across offers, invoice-request/invoice builders, offers flow, ChannelManager wiring, outbound payments, tests, fuzz harnesses, and tooling so amounts are resolved and validated via currency conversion at build/handle time. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant OffersFlow as OffersFlow
participant Conv as CurrencyConversion
participant Invoice as InvoiceBuilder
participant ChanMgr as ChannelManager
Client->>OffersFlow: create/request invoice (offer, amount/quantity)
OffersFlow->>Conv: msats_per_minor_unit(iso4217_code)
Conv-->>OffersFlow: (rate,tolerance) or Err
OffersFlow->>Invoice: build invoice/request (pass &conversion)
Invoice->>Conv: amount_msats(invoice_request, &conversion)
Conv-->>Invoice: resolved msats or Err
Invoice-->>OffersFlow: invoice/request (msat-resolved or Err)
OffersFlow->>ChanMgr: hand off invoice/payment (stores/uses conversion)
ChanMgr-->>Client: emit events / continue payment flow
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ast-grep (0.42.0)lightning/src/ln/channelmanager.rs[] 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
lightning/src/ln/offers_tests.rs (1)
870-872: Update test comment to clarify manual handling.The test comment is identical to the previous test (
creates_and_pays_for_offer_using_one_hop_blinded_path), which may cause confusion. Consider updating it to explicitly mention that this test exercises manual invoice response handling via flow events, which is the key difference from the previous test.lightning/src/offers/invoice.rs (1)
245-266: Centralizing offer-based amount computation viaamount_msatslooks soundMaking
for_offer/for_offer_using_keysgeneric overCC: Deref<Target = CurrencyConversion>and routing both through the sharedpub(crate) fn amount_msats<CC: Deref>( invoice_request: &InvoiceRequest, currency_conversion: CC, ) -> Result<u64, Bolt12SemanticError>is a good consolidation.
The 4-way match over
(invoice_request.amount_msats(), min_amount)behaves as expected:
(Some(ir), Some(min))withir >= min⇒ use the explicit request amount.(Some(_), Some(_))withir < min⇒InsufficientAmount, correctly rejecting under-paying requests.(Some(ir), None)⇒ donation offers with a request-specified amount are accepted as-is.(None, Some(min))⇒ fall back to the offer-implied min (including currency conversion + quantity).(None, None)⇒MissingAmount, preventing amount-less invoices.Because
min_invoice_request_amountalready applies the providedcurrency_conversion(including overflow/validation), this keeps offer-vs-request semantics and currency handling in one place. You might consider adding a small, focused unit test (matrix over these four cases, including a currency-based offer) aroundInvoiceBuilder::amount_msatsto protect this logic explicitly, but the structure looks correct.Also applies to: 321-343, 405-438
lightning/src/offers/flow.rs (3)
1027-1044: Inconsistent type parameter usage foramount_msatscall.On lines 1040-1043 and 1106-1109, the code calls
InvoiceBuilder::<DerivedSigningPubkey>::amount_msats()even increate_invoice_builder_from_invoice_request_without_keyswhich deals withExplicitSigningPubkey. While this may work ifamount_msatsis a static/associated function that doesn't depend on the type parameter, it's potentially confusing. Consider using a type-agnostic path or documenting whyDerivedSigningPubkeyis used in both cases.Also applies to: 1093-1109
1269-1287: Consider whetherenqueue_invoice_errorshould returnResult.The
enqueue_invoice_errormethod returnsResult<(), Bolt12SemanticError>but the function body cannot fail - it always succeeds. Consider returning()instead for API clarity, or document under what conditions this could theoretically fail in the future.🔎 Proposed simplification
- pub fn enqueue_invoice_error( - &self, invoice_error: InvoiceError, path: BlindedMessagePath, - ) -> Result<(), Bolt12SemanticError> { + pub fn enqueue_invoice_error( + &self, invoice_error: InvoiceError, path: BlindedMessagePath, + ) { let mut pending_offers_messages = self.pending_offers_messages.lock().unwrap(); let instructions = MessageSendInstructions::WithoutReplyPath { destination: Destination::BlindedPath(path), }; let message = OffersMessage::InvoiceError(invoice_error); pending_offers_messages.push((message, instructions)); - - Ok(()) }
465-467: Add documentation for the new variant.The
AsynchronouslyHandleResponsevariant lacks documentation, unlike the other variants in the enum. Consider adding a brief doc comment explaining when this variant is returned.🔎 Proposed documentation
+ /// We are recipient of this payment, and should handle the response asynchronously. + /// + /// This variant is returned when [`OffersMessageFlow::enable_events`] is set, allowing + /// the caller to process the invoice request via [`OfferMessageFlowEvent::InvoiceRequestReceived`]. AsynchronouslyHandleResponse,
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
fuzz/src/invoice_request_deser.rslightning/src/ln/channelmanager.rslightning/src/ln/offers_tests.rslightning/src/ln/outbound_payment.rslightning/src/offers/flow.rslightning/src/offers/invoice.rslightning/src/offers/invoice_request.rslightning/src/offers/offer.rs
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-11-28T12:47:06.857Z
Learnt from: shaavan
Repo: shaavan/rust-lightning PR: 8
File: lightning/src/ln/channelmanager.rs:15468-15486
Timestamp: 2025-11-28T12:47:06.857Z
Learning: In `lightning/src/ln/channelmanager.rs`, limit checks for invoice request recurrence fields are handled during the parsing phase, so they don't need to be performed again in the recurrence logic handling code.
Applied to files:
lightning/src/ln/outbound_payment.rslightning/src/ln/channelmanager.rslightning/src/offers/offer.rslightning/src/offers/invoice.rsfuzz/src/invoice_request_deser.rslightning/src/offers/invoice_request.rs
📚 Learning: 2025-11-28T12:48:22.008Z
Learnt from: shaavan
Repo: shaavan/rust-lightning PR: 8
File: lightning/src/ln/channelmanager.rs:15488-15508
Timestamp: 2025-11-28T12:48:22.008Z
Learning: In `lightning/src/ln/channelmanager.rs`, limit checks for invoice request recurrence fields are already handled during the parsing phase, so they don't need to be re-validated in the recurrence handling code.
Applied to files:
lightning/src/ln/outbound_payment.rslightning/src/ln/channelmanager.rslightning/src/offers/offer.rslightning/src/offers/invoice.rsfuzz/src/invoice_request_deser.rslightning/src/offers/invoice_request.rs
🧬 Code graph analysis (7)
lightning/src/ln/outbound_payment.rs (1)
lightning/src/offers/test_utils.rs (3)
payment_paths(75-110)payment_hash(112-114)now(116-120)
lightning/src/ln/channelmanager.rs (2)
lightning/src/offers/invoice.rs (1)
amount_msats(1284-1286)lightning/src/offers/invoice_request.rs (2)
amount_msats(1229-1240)amount_msats(1283-1285)
lightning/src/offers/offer.rs (1)
lightning/src/offers/invoice_request.rs (2)
amount_msats(1229-1240)amount_msats(1283-1285)
lightning/src/offers/invoice.rs (1)
lightning/src/offers/invoice_request.rs (3)
amount_msats(1229-1240)amount_msats(1283-1285)min_invoice_request_amount(1049-1064)
lightning/src/offers/flow.rs (2)
lightning/src/blinded_path/message.rs (1)
new(68-85)lightning/src/offers/offer.rs (1)
paths(931-933)
fuzz/src/invoice_request_deser.rs (1)
lightning/src/offers/invoice_request.rs (2)
fiat_to_msats(595-595)fiat_to_msats(602-604)
lightning/src/offers/invoice_request.rs (1)
fuzz/src/invoice_request_deser.rs (1)
fiat_to_msats(86-88)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: build (windows-latest, stable)
- GitHub Check: build-tx-sync (ubuntu-latest, beta)
- GitHub Check: build-tx-sync (ubuntu-latest, stable)
- GitHub Check: build-tx-sync (ubuntu-latest, 1.75.0)
- GitHub Check: benchmark
- GitHub Check: check_release
- GitHub Check: semver-checks
- GitHub Check: build (windows-latest, stable)
- GitHub Check: build-tx-sync (macos-latest, stable)
- GitHub Check: build (macos-latest, 1.75.0)
- GitHub Check: build (macos-latest, stable)
- GitHub Check: build-tx-sync (ubuntu-latest, 1.75.0)
- GitHub Check: build-tx-sync (macos-latest, beta)
- GitHub Check: build-tx-sync (macos-latest, 1.75.0)
- GitHub Check: build-tx-sync (ubuntu-latest, stable)
- GitHub Check: build-tx-sync (ubuntu-latest, beta)
- GitHub Check: benchmark
- GitHub Check: check_release
- GitHub Check: semver-checks
- GitHub Check: check_docs
🔇 Additional comments (33)
fuzz/src/invoice_request_deser.rs (2)
20-23: LGTM! Fuzzing implementation is appropriate.The
FuzzCurrencyConversionstruct correctly implements theCurrencyConversiontrait for fuzzing purposes. Theunreachable!()panic is intentional—it will surface any unexpected code paths during fuzzing that attempt currency conversion from deserialized invoice requests.Also applies to: 83-89
157-157: LGTM! Currency conversion parameter correctly threaded through.The call to
respond_withnow properly includes theFuzzCurrencyConversionparameter, aligning with the currency-conversion aware API introduced across the codebase.lightning/src/offers/offer.rs (2)
85-85: LGTM! Required imports for currency conversion.The imports are necessary to support the new
to_msatsmethod on theAmountenum.Also applies to: 103-103
1130-1146: LGTM! Currency conversion logic is well-structured.The
to_msatsmethod correctly handles both Bitcoin and fiat currency amounts:
- Bitcoin amounts return the amount directly
- Currency amounts convert via exchange rate with proper overflow checking
- Error handling appropriately maps to
UnsupportedCurrencyandInvalidAmountlightning/src/ln/offers_tests.rs (2)
54-54: LGTM! Required imports for flow events and currency conversion.The imports support the new test that exercises manual invoice response handling via flow events and the currency-conversion aware API.
Also applies to: 61-61
2448-2448: LGTM! Currency conversion parameter correctly added.The call to
respond_using_derived_keys_no_stdnow properly includes&DefaultCurrencyConversion, aligning with the updated API signature across the codebase.lightning/src/ln/outbound_payment.rs (1)
3186-3235: Tests updated torespond_with_no_conversionalign with the new APIThe BOLT‑12 invoice tests here now consistently use
respond_with_no_conversion(...)when constructing invoices from offers, which matches the currency‑conversion refactor (the actual conversion still comes viaDefaultCurrencyConversioninside invoice APIs). The behavioural expectations in the tests (expiry, route not found, duplicate/unexpected invoice cases) remain unchanged and still read correctly against the updated outbound payment flow.Also applies to: 3241-3299, 3303-3397
lightning/src/offers/invoice.rs (2)
27-61: Docs and imports correctly reflect the new currency-conversion APIImporting
DefaultCurrencyConversionandCurrencyConversion, pluscore::ops::Deref, and updating the module‑level example to use:
.respond_with(&DefaultCurrencyConversion, ...)underfeature = "std", and.respond_with_no_std(&DefaultCurrencyConversion, ...)undernot(feature = "std")keeps the public docs aligned with the new conversion‑aware respond APIs. This also matches how tests exercise these paths, so the example should stay compilable and accurate across both std and no‑std builds assuming
DefaultCurrencyConversionis available in both.Also applies to: 128-132, 161-162
1849-1853: Test updates forDefaultCurrencyConversion/respond_with*_no_conversionare consistentThe tests now:
- Import
DefaultCurrencyConversionwhere needed.- Call
.respond_with(&DefaultCurrencyConversion, ...)for std offer flows.- Use
.respond_with_no_conversion(...)and.respond_with_no_std_using_signing_pubkey(&DefaultCurrencyConversion, ...)helpers in the various offer‑based invoice builder tests.- Keep refund-related tests on the original, conversion‑free respond APIs.
This cleanly exercises both conversion-aware and “no conversion” convenience paths without changing the underlying behavioural assertions (amounts, expiry, quantity scaling, signature verification). The changes look internally consistent with the new builder signatures.
Also applies to: 2181-2201, 2268-2283, 2926-2933, 3078-3116, 3406-3437
lightning/src/offers/invoice_request.rs (6)
579-605: Well-designed currency conversion abstraction.The
CurrencyConversiontrait with its documentation about minor units (ISO-4217 exponent) is clear and helpful for implementers. TheDefaultCurrencyConversionno-op implementation provides a sensible fallback for BTC-only use cases.
798-809: LGTM - Currency conversion properly threaded through response methods.The
respond_withandrespond_with_no_stdmethods correctly accept a genericCC: Derefparameter with theCurrencyConversiontrait bound, and pass it through tofor_offer. The pattern is consistent with Rust's zero-cost abstraction idiom.Also applies to: 836-853
857-861: Test helper correctly uses DefaultCurrencyConversion.The
respond_with_no_conversiontest helper appropriately wrapsrespond_with_no_stdwithDefaultCurrencyConversion, providing a convenient API for tests that don't need currency conversion.
1078-1089: Derived keys response methods correctly updated.Both
respond_using_derived_keysandrespond_using_derived_keys_no_stdconsistently thread thecurrency_conversionparameter through to the underlying builder methods.Also applies to: 1098-1119
1815-1821: Test correctly updated to use new API.The test properly uses
respond_with_no_conversioninstead of the previous method name.
2399-2403: Test correctly updated for new API.Consistent use of
respond_with_no_conversionin the test.lightning/src/offers/flow.rs (8)
74-98: Well-documented event enum with clear usage guidance.The
OfferMessageFlowEventenum with theInvoiceRequestReceivedvariant is well-documented with clear instructions on how to respond. The documentation correctly references the appropriate builder methods for eachInvoiceRequestVerifiedFromOffervariant.
122-123: Consider visibility ofenable_eventsfield.The
enable_eventsfield is markedpub(crate), which allows internal modification but theenable_events()method on line 205 sets it totrue. However, the struct only has an immutable&selfreference inenable_events()on line 205, but line 206 attempts to assign toself.enable_events. This should require&mut self.Wait, looking again at line 205-206:
pub fn enable_events(&mut self) { self.enable_events = true; }This is correct - it does take
&mut self. The field visibility aspub(crate)is fine for internal testing/debugging access.Also applies to: 137-138
542-553: Event flow correctly handles async vs sync paths.The conditional logic correctly pushes an
InvoiceRequestReceivedevent whenenable_eventsis true and returnsAsynchronouslyHandleResponse, otherwise returns the verified invoice request directly viaSendInvoice. This provides a clean separation between synchronous and asynchronous handling modes.
1204-1237: LGTM - Clear separation of enqueue methods by destination type.The
enqueue_invoice_using_node_idmethod correctly creates reply paths and sends the invoice to a directPublicKeydestination. The loop structure is appropriate for sending to multiple reply paths.
1239-1267: LGTM - Blinded path variant correctly uses helper function.The
enqueue_invoice_using_reply_pathscorrectly leveragesenqueue_onion_message_with_reply_pathsfor sending to blinded paths, maintaining consistency with other similar operations in the codebase.
1438-1458: LGTM - Flow event methods follow established patterns.The
enqueue_flow_eventandrelease_pending_flow_eventsmethods correctly mirror the existing patterns used forpending_offers_messagesandpending_async_payments_messages, usingcore::mem::takefor efficient draining of the mutex-protected vector.
148-183: Constructor correctly initializes new fields.The
newconstructor properly acceptsenable_eventsas a parameter and initializes bothenable_eventsandpending_flow_eventsfields. The default empty vector for pending events is appropriate.
506-554: TheResponder::into_blinded_path()method is properly defined and accessible. It exists atlightning/src/onion_message/messenger.rs:439with signaturepub(crate) fn into_blinded_path(self) -> BlindedMessagePath. TheRespondertype is correctly imported inflow.rsat line 57. The code at line 546 is valid.Likely an incorrect or invalid review comment.
lightning/src/ln/channelmanager.rs (10)
96-100: LGTM!The import additions for
DefaultCurrencyConversionandAmountare necessary to support the currency-based offer validation and conversion threading introduced in this PR.
2669-2672: LGTM!Standard pattern for exposing
routerandnode_signerfields to tests via conditional compilation.Also applies to: 2898-2901
5675-5681: LGTM!Threading
DefaultCurrencyConversionthroughstatic_invoice_receivedis consistent with the currency conversion abstraction pattern used throughout the PR.
12951-12962: LGTM!The documentation clearly explains the amount handling requirements, especially that callers must provide
amount_msatsfor currency-denominated offers and that the recipient may reject insufficient amounts post-conversion.
13100-13106: LGTM!The validation correctly enforces that
amount_msatsmust be provided when the offer specifies a currency amount. This aligns with the documented contract and the fact thatInvoiceRequest::amount_msats()returnsNonefor currency-based offers (as seen in the relevant code snippet).
13189-13201: LGTM!The conditional logic correctly handles invoice delivery based on whether the refund has reply paths available, falling back to node ID-based routing when paths are empty.
13425-13428: LGTM!Visibility upgrade to
pub(crate)is appropriate to allow other modules to obtain peer information for blinded path construction.
15338-15348: LGTM!The new
AsynchronouslyHandleResponsevariant enables deferred invoice handling via the event-driven flow. ReturningNoneis correct since the response will be generated asynchronously through theOfferMessageFlowEventmechanism mentioned in the PR objectives.
15361-15364: LGTM!
DefaultCurrencyConversionis consistently threaded through both the derived-keys and explicit-keys invoice builder paths, maintaining uniformity in the currency conversion abstraction.Also applies to: 15386-15389
3956-3957: > Likely an incorrect or invalid review comment.
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
shaavan
changed the title
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lightning/src/ln/outbound_payment.rs (1)
1307-1317:⚠️ Potential issue | 🟠 MajorUse a failure reason consistent with the returned error.
This branch returns
SendingFailed(OnionPacketSizeExceeded)but records the queuedPaymentFailedevent asRouteNotFound. That will misclassify the failure for any caller consumingPaymentFailureReason.Suggested fix
if let Err(()) = onion_utils::set_max_path_length( &mut route_params, &RecipientOnionFields::spontaneous_empty(amount_msat), Some(keysend_preimage), Some(invreq), best_block_height, ) { - abandon_with_entry!(entry, PaymentFailureReason::RouteNotFound); + abandon_with_entry!(entry, PaymentFailureReason::UnexpectedError); return Err(Bolt12PaymentError::SendingFailed( RetryableSendFailure::OnionPacketSizeExceeded, )); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lightning/src/ln/outbound_payment.rs` around lines 1307 - 1317, The branch handling onion_utils::set_max_path_length currently records abandon_with_entry!(entry, PaymentFailureReason::RouteNotFound) but returns Bolt12PaymentError::SendingFailed(RetryableSendFailure::OnionPacketSizeExceeded); change the recorded PaymentFailureReason to match the returned error (e.g., a variant representing OnionPacketSizeExceeded or a new matching variant) so the queued PaymentFailed event and the returned SendingFailed share the same failure reason; update the abandon_with_entry! call to use the correct PaymentFailureReason variant and ensure any callers consuming PaymentFailureReason see the consistent classification.
🧹 Nitpick comments (3)
fuzz/src/invoice_request_deser.rs (1)
65-75:FuzzCurrencyConversionis defined but appears unused.The
FuzzCurrencyConversionstruct is defined but never instantiated or used anywhere in this file. If this is intended for future use, consider adding a#[allow(dead_code)]annotation with a comment explaining the intent, or remove it if not needed.Additionally, the
unreachable!()implementations will cause a panic if ever called during fuzzing. If currency conversion code paths can be reached through fuzzed invoice requests (e.g., when a Currency-denominated offer lacks an explicit amount), this would terminate the fuzzer rather than gracefully handling the case.Consider whether this should return
Err(())instead to allow the fuzzer to continue exploring other paths:Suggested alternative if this becomes used
impl CurrencyConversion for FuzzCurrencyConversion { fn msats_per_minor_unit(&self, _iso4217_code: CurrencyCode) -> Result<u64, ()> { - unreachable!() + // Return error to gracefully handle unsupported currencies during fuzzing + Err(()) } fn tolerance_percent(&self) -> u8 { - unreachable!() + 0 } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fuzz/src/invoice_request_deser.rs` around lines 65 - 75, FuzzCurrencyConversion is declared but never used and its impl uses unreachable!() which will panic if exercised; either mark it intentionally unused with #[allow(dead_code)] plus a brief comment explaining it's for future fuzz stubs, or remove the struct entirely, and if you expect currency paths to be reachable during fuzzing replace the panicking impls on CurrencyConversion (methods msats_per_minor_unit and tolerance_percent) with non-panicking behavior: have msats_per_minor_unit return Err(()) and have tolerance_percent return a safe default (e.g., 0) so the fuzzer can continue exploring paths instead of aborting.lightning/src/ln/offers_tests.rs (1)
942-969: Keep the fiat-path test decoupled fromChannelManagerinternals.This test already depends on
TestCurrencyConversion, so reaching intoalice.node.flow.currency_conversionmakes it brittle to unrelatedChannelManagerrefactors. Using a local conversion value here would keep the fixture explicit and consistent with the other updated tests.♻️ Suggested cleanup
+ let conversion = TestCurrencyConversion; let offer = alice.node .create_offer_builder().unwrap() - .amount(amount, &alice.node.flow.currency_conversion).unwrap() + .amount(amount, &conversion).unwrap() .build().unwrap(); @@ - assert_eq!(invoice_request.amount_msats(&alice.node.flow.currency_conversion), Ok(1_000_000)); + assert_eq!(invoice_request.amount_msats(&conversion), Ok(1_000_000));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lightning/src/ln/offers_tests.rs` around lines 942 - 969, The test currently reads the conversion via alice.node.flow.currency_conversion which couples the test to ChannelManager internals; instead, use the TestCurrencyConversion fixture directly (or a local conversion instance) when calling create_offer_builder().amount(...) so the amount check uses an explicit conversion value matching the test setup; update the amount(...) call in the create_offer_builder/use of InvoiceRequest.amount_msats to pass or compare against the local TestCurrencyConversion value and remove the reference to alice.node.flow.currency_conversion to keep the test decoupled and stable.lightning/src/ln/functional_test_utils.rs (1)
903-919: Use the node's configured converter in the round-trip smoke test.Line 918 hardcodes a fresh
TestCurrencyConversion, so this check won't exercise any non-default/stateful converter a test may have attached to the node. Reusingself.currency_conversionwould keep the serialization smoke test aligned with the manager under test.♻️ Suggested change
- currency_conversion: &test_utils::TestCurrencyConversion, + currency_conversion: self.currency_conversion,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lightning/src/ln/functional_test_utils.rs` around lines 903 - 919, The ChannelManagerReadArgs construction currently hardcodes a fresh TestCurrencyConversion which prevents exercising any custom converter attached to the node; replace the hardcoded &test_utils::TestCurrencyConversion with a reference to the node's configured converter (use self.currency_conversion) so the round-trip smoke test uses the same currency conversion instance as the ChannelManager under test (update the field in ChannelManagerReadArgs where TestCurrencyConversion is passed).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@fuzz/src/full_stack.rs`:
- Around line 190-196: The fuzz harness is missing CurrencyCode import and
mis-wires ChannelManager::new plus leaves FuzzCurrencyConversion panicking; add
use lightning::offers::offer::CurrencyCode, implement
FuzzCurrencyConversion::msats_per_minor_unit to return Err(()) and
tolerance_percent to return 0 (replace unreachable!()), instantiate a
FuzzCurrencyConversion in do_test, pass ¤cy_conversion as the 6th argument
to ChannelManager::new so subsequent args align, and include
best_block_timestamp (the current_timestamp u32) as the final (13th) parameter
when calling ChannelManager::new.
In `@fuzz/src/offer_deser.rs`:
- Around line 52-58: The match arm is calling a non-existent to_msats; replace
the call with the correct owning method Amount::into_msats and pass the
conversion: when matching Some(amount) use amount.into_msats(&conversion)? so
the owning Amount is moved into into_msats; keep the rest of the builder call
(builder.amount_msats(...)?). This touches the symbols
DefaultCurrencyConversion, offer.request_invoice, offer.amount(),
builder.amount_msats, and Amount::into_msats.
In `@lightning/src/ln/channelmanager.rs`:
- Around line 8377-8396: The Err branch of verified_invreq.amount_msats(...)
currently only calls debug_assert!(false) and falls through, which can allow
underpaid payments; instead, treat this as a hard failure and call
fail_htlc!(claimable_htlc, payment_hash) (and optionally log/debug the
resolution error) inside the Err arm so the HTLC is failed immediately when
amount resolution fails for verified_invreq; update the Err arm in the match
around amount_msats to mirror the Ok->underpayment failure path rather than
falling through.
In `@lightning/src/offers/flow.rs`:
- Around line 834-847: The local type annotation for InvoiceRequestBuilder in
create_invoice_request_builder is missing the two lifetime parameters; remove
the explicit concrete type annotation so the compiler can infer the correct
InvoiceRequestBuilder<'a, 'b, T, CC> (i.e., replace the annotated let builder:
InvoiceRequestBuilder<secp256k1::All, CC> = ... with an unannotated let builder
= offer.request_invoice(...)? .into();), then keep the subsequent match that
calls offer.resolve_offer_amount(...) and builder.amount_msats(...)? as-is so
the inferred type carries the needed lifetimes.
---
Outside diff comments:
In `@lightning/src/ln/outbound_payment.rs`:
- Around line 1307-1317: The branch handling onion_utils::set_max_path_length
currently records abandon_with_entry!(entry,
PaymentFailureReason::RouteNotFound) but returns
Bolt12PaymentError::SendingFailed(RetryableSendFailure::OnionPacketSizeExceeded);
change the recorded PaymentFailureReason to match the returned error (e.g., a
variant representing OnionPacketSizeExceeded or a new matching variant) so the
queued PaymentFailed event and the returned SendingFailed share the same failure
reason; update the abandon_with_entry! call to use the correct
PaymentFailureReason variant and ensure any callers consuming
PaymentFailureReason see the consistent classification.
---
Nitpick comments:
In `@fuzz/src/invoice_request_deser.rs`:
- Around line 65-75: FuzzCurrencyConversion is declared but never used and its
impl uses unreachable!() which will panic if exercised; either mark it
intentionally unused with #[allow(dead_code)] plus a brief comment explaining
it's for future fuzz stubs, or remove the struct entirely, and if you expect
currency paths to be reachable during fuzzing replace the panicking impls on
CurrencyConversion (methods msats_per_minor_unit and tolerance_percent) with
non-panicking behavior: have msats_per_minor_unit return Err(()) and have
tolerance_percent return a safe default (e.g., 0) so the fuzzer can continue
exploring paths instead of aborting.
In `@lightning/src/ln/functional_test_utils.rs`:
- Around line 903-919: The ChannelManagerReadArgs construction currently
hardcodes a fresh TestCurrencyConversion which prevents exercising any custom
converter attached to the node; replace the hardcoded
&test_utils::TestCurrencyConversion with a reference to the node's configured
converter (use self.currency_conversion) so the round-trip smoke test uses the
same currency conversion instance as the ChannelManager under test (update the
field in ChannelManagerReadArgs where TestCurrencyConversion is passed).
In `@lightning/src/ln/offers_tests.rs`:
- Around line 942-969: The test currently reads the conversion via
alice.node.flow.currency_conversion which couples the test to ChannelManager
internals; instead, use the TestCurrencyConversion fixture directly (or a local
conversion instance) when calling create_offer_builder().amount(...) so the
amount check uses an explicit conversion value matching the test setup; update
the amount(...) call in the create_offer_builder/use of
InvoiceRequest.amount_msats to pass or compare against the local
TestCurrencyConversion value and remove the reference to
alice.node.flow.currency_conversion to keep the test decoupled and stable.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e643375f-233b-4ab5-ae38-952644116a82
📒 Files selected for processing (21)
fuzz/src/full_stack.rsfuzz/src/invoice_request_deser.rsfuzz/src/offer_deser.rslightning-background-processor/src/lib.rslightning-block-sync/src/init.rslightning/src/ln/async_payments_tests.rslightning/src/ln/channelmanager.rslightning/src/ln/functional_test_utils.rslightning/src/ln/functional_tests.rslightning/src/ln/offers_tests.rslightning/src/ln/outbound_payment.rslightning/src/ln/reload_tests.rslightning/src/offers/currency.rslightning/src/offers/flow.rslightning/src/offers/invoice.rslightning/src/offers/invoice_request.rslightning/src/offers/merkle.rslightning/src/offers/mod.rslightning/src/offers/offer.rslightning/src/offers/refund.rslightning/src/util/test_utils.rs
✅ Files skipped from review due to trivial changes (1)
- lightning/src/ln/functional_tests.rs
| match offer.check_amount_msats_for_quantity(&DefaultCurrencyConversion, amount, quantity) { | ||
| // If the offer amount is currency-denominated, we intentionally skip the | ||
| // amount check here, as currency conversion is not available at this stage. | ||
| // The corresponding validation is performed when handling the Invoice Request, | ||
| // i.e., during InvoiceBuilder creation. | ||
| Ok(()) | Err(Bolt12SemanticError::UnsupportedCurrency) => (), | ||
| Err(err) => return Err(err), |
There was a problem hiding this comment.
This weakens the post-parse amount invariant without updating consumers.
After this change, InvoiceRequest::try_from can succeed for currency-denominated requests whose effective amount still needs conversion-time validation. But lightning/src/ln/outbound_payment.rs Line 1289 still treats InvoiceBuilder::amount_msats(..) failure as unreachable after parsing, so these requests can now fall into the wrong later error path (UnknownRequiredFeatures / UnexpectedError). Either keep rejecting them here, or audit the downstream amount_msats callers before landing this relaxation.
There was a problem hiding this comment.
@coderabbitai help me understand this issue clearly
There was a problem hiding this comment.
@shaavan Sure! Here's a clear breakdown of the issue:
🔍 Background: The Old Parse-Time Invariant
Before this change, InvoiceRequest::try_from (the parse path) called:
offer.check_amount_msats_for_quantity(amount, quantity)?;This enforced a strong invariant: if parsing succeeds, the amount in the request is already validated. For currency-denominated offers, since no conversion was available at parse time, this call would return an error — and the parse would fail. In other words, currency-denominated invoice requests simply couldn't be parsed successfully.
🔄 What the Change Does
The new code:
match offer.check_amount_msats_for_quantity(&DefaultCurrencyConversion, amount, quantity) {
Ok(()) | Err(Bolt12SemanticError::UnsupportedCurrency) => (),
Err(err) => return Err(err),
}Now intentionally allows currency-denominated requests to pass parsing by silently swallowing UnsupportedCurrency errors. The idea is: defer the amount validation to later (when building the Bolt12Invoice, where a proper CurrencyConversion is available).
This is necessary to support currency-based offers — parsing must succeed for the flow to continue.
⚠️ Where the Risk Was
The old invariant was implicitly relied upon downstream. Specifically, in outbound_payment.rs, there was code that called InvoiceBuilder::amount_msats(...) after a successful parse and treated failures from it as unreachable!(), reasoning: "parsing already validated the amount, so this can't fail."
After this change, a currency-denominated request now gets through parsing. If that request then reaches a downstream amount_msats() call that lacks proper CurrencyConversion, the call would fail — but the code would hit the unreachable!() branch instead of returning a clean user-facing error. This manifests as a wrong error type (UnknownRequiredFeatures or UnexpectedError) or even a panic.
✅ What the Fix Required
Auditing all downstream callers of amount_msats that run after a parsed InvoiceRequest to ensure:
- They all receive a proper
CurrencyConversion(notDefaultCurrencyConversion), and - Any failures from
amount_msatsare handled as real errors — not treated asunreachable!().
Based on the AI summary, the PR threads currency_conversion through static_invoice_received, InvoiceBuilder, OffersMessageFlow, and related paths — which is precisely the fix needed to make this relaxation safe.
🧠 Learnings used
Learnt from: shaavan
Repo: shaavan/rust-lightning PR: 8
File: lightning/src/ln/channelmanager.rs:15488-15508
Timestamp: 2025-11-28T12:48:22.008Z
Learning: In `lightning/src/ln/channelmanager.rs`, limit checks for invoice request recurrence fields are already handled during the parsing phase, so they don't need to be re-validated in the recurrence handling code.
Learnt from: shaavan
Repo: shaavan/rust-lightning PR: 8
File: lightning/src/ln/channelmanager.rs:15468-15486
Timestamp: 2025-11-28T12:47:06.857Z
Learning: In `lightning/src/ln/channelmanager.rs`, limit checks for invoice request recurrence fields are handled during the parsing phase, so they don't need to be performed again in the recurrence logic handling code.
There was a problem hiding this comment.
@coderabbitai analyse the code, and tell me what needs to change in the PR, to add this fix
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
echo "=== Searching for unreachable! in outbound_payment.rs ==="
rg -n "unreachable" lightning/src/ln/outbound_payment.rs
echo ""
echo "=== Searching for amount_msats calls in outbound_payment.rs ==="
rg -n "amount_msats" lightning/src/ln/outbound_payment.rs -A3 -B3
echo ""
echo "=== Searching for debug_assert in outbound_payment.rs ==="
rg -n "debug_assert" lightning/src/ln/outbound_payment.rs -A2 -B2
echo ""
echo "=== static_invoice_received full function context ==="
rg -n "static_invoice_received" lightning/src/ln/outbound_payment.rs -A 80
echo ""
echo "=== Searching for InvoiceBuilder::amount_msats or amount_msats usage in flow.rs ==="
rg -n "amount_msats" lightning/src/offers/flow.rs -A3 -B3
echo ""
echo "=== Check for UnsupportedCurrency handling downstream ==="
rg -n "UnsupportedCurrency" lightning/src/ln/outbound_payment.rs
rg -n "UnsupportedCurrency" lightning/src/offers/flow.rs
rg -n "UnsupportedCurrency" lightning/src/ln/channelmanager.rs -A2 -B2
echo ""
echo "=== Check InvoiceBuilder amount_msats method in invoice.rs ==="
rg -n "fn amount_msats" lightning/src/offers/invoice.rs -A 20
Length of output: 23885
@shaavan Here is a precise breakdown of exactly what needs to change.
Root of the Problem
After the diff lands, InvoiceRequest::try_from silently passes for currency-denominated offers (swallowing UnsupportedCurrency). This is intentional. But in outbound_payment.rs, the static_invoice_received function (lines 1283–1294) still has a catch-all error arm that incorrectly treats UnsupportedCurrency as a programming bug:
// outbound_payment.rs ~line 1283
let amount_msat = match InvoiceBuilder::<DerivedSigningPubkey>::amount_msats(
invreq,
currency_conversion,
) {
Ok(amt) => amt,
Err(_) => {
// ⚠️ THIS debug_assert fires for UnsupportedCurrency too — wrong!
debug_assert!(false, "LDK requires an msat amount in either the invreq or the invreq's underlying offer");
abandon_with_entry!(entry, PaymentFailureReason::UnexpectedError);
return Err(Bolt12PaymentError::UnknownRequiredFeatures); // Wrong error type
},
};The comment ("We check this during invoice request parsing") is also now stale — currency-denominated offers bypass that check at parse time.
What Needs to Change
File: lightning/src/ln/outbound_payment.rs (inside static_invoice_received)
Split the catch-all Err(_) into two distinct arms:
let amount_msat = match InvoiceBuilder::<DerivedSigningPubkey>::amount_msats(
invreq,
currency_conversion,
) {
Ok(amt) => amt,
Err(Bolt12SemanticError::UnsupportedCurrency) => {
// The offer is currency-denominated and the provided CurrencyConversion
// does not support the currency. This is a legitimate runtime failure
// (not a programming error), so no debug_assert here.
abandon_with_entry!(entry, PaymentFailureReason::UnexpectedError);
return Err(Bolt12PaymentError::UnknownRequiredFeatures);
},
Err(_) => {
// This branch should be unreachable: parsing ensures either the invreq
// or its underlying offer carries an explicit msat amount for non-currency offers.
debug_assert!(false, "LDK requires an msat amount in either the invreq or the invreq's underlying offer");
abandon_with_entry!(entry, PaymentFailureReason::UnexpectedError);
return Err(Bolt12PaymentError::UnknownRequiredFeatures);
},
};Key changes:
- New arm for
UnsupportedCurrency— treated as a real, expected runtime failure. Nodebug_assert!. Err(_)arm retained — still a logic error for non-currency offers (where parse guarantees an amount). Thedebug_assert!(false)stays here.- Comment updated — the old stale comment claiming "We check this during invoice request parsing" should be removed or replaced, since the parse-time check is now intentionally skipped for currency offers.
Optional (but recommended): Better Error Variant
Ideally, Bolt12PaymentError would have a dedicated UnsupportedCurrency variant instead of reusing UnknownRequiredFeatures (which is semantically wrong for a currency conversion failure). That would be a clean follow-up, but the above fix is the minimum needed to prevent the spurious debug_assert!(false).
Other Call Sites
The other amount_msats call sites in flow.rs (lines ~1000 and ~1063) already use ? to propagate errors, so UnsupportedCurrency would naturally bubble up as a Bolt12SemanticError — those are fine and don't need changes.
🧠 Learnings used
Learnt from: shaavan
Repo: shaavan/rust-lightning PR: 10
File: lightning/src/offers/invoice.rs:1790-1815
Timestamp: 2026-03-09T13:39:43.011Z
Learning: In `lightning/src/offers/invoice.rs`, within `TryFrom<PartialInvoiceTlvStream> for InvoiceContents`, it is safe and correct to use `DefaultCurrencyConversion` when calling `invoice_request.amount_msats()` during invoice parsing. The `amount_msats` method checks for an explicitly encoded amount on the invoice request first; the fiat-resolution fallback is only reached when no explicit amount is present and the offer is currency-denominated. Because the invoice request builder always resolves and encodes a concrete msat amount for fiat offers at construction time, the `DefaultCurrencyConversion` fallback is unreachable in valid flows. The `debug_assert!(false)` + `InvalidAmount` error path is intentional defensive programming, not a real conversion dependency.
Learnt from: shaavan
Repo: shaavan/rust-lightning PR: 10
File: lightning/src/offers/invoice.rs:401-426
Timestamp: 2026-03-09T13:35:05.055Z
Learning: In the offers module, the CurrencyConversion trait should be implemented as synchronous and cache-backed. Exchange rates must remain consistent across multiple consecutive calls within a single payment flow (e.g., within a single invoice build or resolution path). Ensure that repeated calls to currency_conversion in the same context are safe and rely on a refreshable cache. This guideline applies to all Rust files in the offers directory (e.g., lightning/src/offers/*.rs) to maintain consistent currency behavior across related components.
Learnt from: shaavan
Repo: shaavan/rust-lightning PR: 8
File: lightning/src/ln/channelmanager.rs:15488-15508
Timestamp: 2025-11-28T12:48:22.008Z
Learning: In `lightning/src/ln/channelmanager.rs`, limit checks for invoice request recurrence fields are already handled during the parsing phase, so they don't need to be re-validated in the recurrence handling code.
Learnt from: shaavan
Repo: shaavan/rust-lightning PR: 8
File: lightning/src/ln/channelmanager.rs:15468-15486
Timestamp: 2025-11-28T12:47:06.857Z
Learning: In `lightning/src/ln/channelmanager.rs`, limit checks for invoice request recurrence fields are handled during the parsing phase, so they don't need to be performed again in the recurrence logic handling code.
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (6)
fuzz/src/offer_deser.rs (1)
56-58:⚠️ Potential issue | 🔴 CriticalUse
into_msatshere;to_msatswon't compile.Line 58 is calling a non-existent
Amount::to_msats. This should moveamountintointo_msatsbefore passing the result toamount_msats.Suggested fix
builder = match offer.amount() { None => builder.amount_msats(1000).unwrap(), - Some(amount) => builder.amount_msats(amount.to_msats(&conversion)?)?, + Some(amount) => builder.amount_msats(amount.into_msats(&conversion)?)?, };#!/bin/bash rg -n "fn (to_msats|into_msats)" lightning/src/offers/offer.rs -A4 -B2 sed -n '54,60p' fuzz/src/offer_deser.rs🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fuzz/src/offer_deser.rs` around lines 56 - 58, The match arm calling a non-existent Amount::to_msats should use the consuming method Amount::into_msats; in the Some(amount) branch pass the moved amount into into_msats (using the same &conversion) and feed its result to builder.amount_msats, preserving the existing ? error handling and unwraps. Update the Some(amount) => branch to call amount.into_msats(&conversion)? and then builder.amount_msats(...) so the code compiles.fuzz/src/full_stack.rs (2)
625-638:⚠️ Potential issue | 🔴 CriticalPass the new
currency_conversiondependency intoChannelManager::new.This constructor call is still using the pre-
CCparameter list. TheChannelMan<'a>alias now includes&'a FuzzCurrencyConversion, so the provider needs to be instantiated locally and inserted before the logger argument.Suggested fix
let fee_est = Arc::new(FuzzEstimator { input: input.clone() }); let router = FuzzRouter {}; + let currency_conversion = FuzzCurrencyConversion; @@ let channelmanager = Arc::new(ChannelManager::new( fee_est.clone(), monitor.clone(), broadcast.clone(), &router, &router, + ¤cy_conversion, Arc::clone(&logger), keys_manager.clone(), keys_manager.clone(), keys_manager.clone(),#!/bin/bash sed -n '236,257p' fuzz/src/full_stack.rs sed -n '622,639p' fuzz/src/full_stack.rs rg -n -A20 "pub fn new\(" lightning/src/ln/channelmanager.rs | head -n 40 rg -n "currency_conversion" lightning/src/ln/channelmanager.rs | head -n 20🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fuzz/src/full_stack.rs` around lines 625 - 638, ChannelManager::new is being called with the old parameter list and missing the new currency_conversion dependency; create a local FuzzCurrencyConversion instance (e.g., let currency_conversion = Arc::new(FuzzCurrencyConversion::new(...)) or similar) and pass it into ChannelManager::new before the logger argument to match the updated ChannelMan<'a> signature that expects &FuzzCurrencyConversion; update the call site inside fuzz/src/full_stack.rs to insert currency_conversion (or a reference/Arc as used elsewhere) immediately prior to Arc::clone(&logger).
54-55:⚠️ Potential issue | 🔴 CriticalDon't panic on unsupported currencies in the fuzz harness.
Line 191 also needs
CurrencyCodein scope, and Lines 192/196 should not useunreachable!(). Unsupported fiat inputs should returnErr(())/0so they surface as semantic errors instead of crashing the target.Suggested fix
-use lightning::offers::currency::CurrencyConversion; +use lightning::offers::currency::CurrencyConversion; +use lightning::offers::offer::CurrencyCode; @@ impl CurrencyConversion for FuzzCurrencyConversion { fn msats_per_minor_unit(&self, _iso4217_code: CurrencyCode) -> Result<u64, ()> { - unreachable!() + Err(()) } fn tolerance_percent(&self) -> u8 { - unreachable!() + 0 } }#!/bin/bash sed -n '50,60p' fuzz/src/full_stack.rs sed -n '188,198p' fuzz/src/full_stack.rs rg -n "pub trait CurrencyConversion|into_msats" lightning/src/offers -g '*.rs'Also applies to: 188-198
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fuzz/src/full_stack.rs` around lines 54 - 55, Add CurrencyCode to the imports and stop panicking on unsupported fiat currencies in the fuzz harness: bring CurrencyCode into scope alongside CurrencyConversion and UnsignedBolt12Invoice, and replace any unreachable!() calls used when fiat currency conversion fails (e.g., in the code paths that call into_msats / CurrencyConversion::into_msats) with returning a semantic error (Err(())) or a zero-value (0) as appropriate so unsupported fiat inputs surface as errors instead of crashing the target; ensure functions/branches that previously called unreachable!() now propagate Err(()) or return 0 where the signature expects Result<(), ()> or u64 respectively.lightning/src/ln/outbound_payment.rs (1)
1283-1294:⚠️ Potential issue | 🟠 MajorStill unresolved:
amount_msatscan fail legitimately here.Now that this call depends on
currency_conversion, theErr(_)arm is reachable for fiat/conversion failures. It still abandons the payment asUnexpectedErrorbut returnsUnknownRequiredFeatures, so the immediate error is misreported and the pending payment is dropped on any conversion miss.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lightning/src/ln/outbound_payment.rs` around lines 1283 - 1294, The amount_msat lookup can legitimately fail due to currency_conversion, so don't treat all errors as an LDK-required-features bug; in the Err(_) arm of InvoiceBuilder::<DerivedSigningPubkey>::amount_msats handle the error explicitly by capturing the original error (e), map or wrap it into a meaningful Bolt12PaymentError variant (or add one like CurrencyConversionFailed) instead of returning UnknownRequiredFeatures, and call abandon_with_entry!(entry, PaymentFailureReason::InvalidInvoice) or a new PaymentFailureReason::CurrencyConversionFailed so the pending payment is abandoned with the correct reason rather than misreported as UnexpectedError.lightning/src/offers/invoice_request.rs (1)
1172-1188:⚠️ Potential issue | 🔴 CriticalRevalidate explicit amounts before returning them for fiat-denominated offers.
InvoiceRequest::try_fromnow defers currency-based amount checks when onlyDefaultCurrencyConversionis available, but thisSome(msats)branch returns the raw requested amount unchanged. A parsed request can therefore understate a fiat-denominated offer minimum, or carry an explicit amount aboveMAX_VALUE_MSAT, and still flow into invoice creation.🛠️ Suggested fix
pub(super) fn amount_msats<CC: CurrencyConversion>( &self, currency_conversion: &CC, ) -> Result<u64, Bolt12SemanticError> { - match self.inner.amount_msats() { - Some(msats) => Ok(msats), + let requested_msats = self.inner.amount_msats(); + self.inner.offer.check_amount_msats_for_quantity( + currency_conversion, + requested_msats, + self.quantity(), + )?; + + match requested_msats { + Some(msats) => Ok(msats), None => { let unit_msats = self .inner .offer .resolve_offer_amount(currency_conversion)?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lightning/src/offers/invoice_request.rs` around lines 1172 - 1188, The Some(msats) branch in InvoiceRequest::amount_msats currently returns the raw explicit msats without revalidating against fiat-denominated offer constraints; update amount_msats so that when inner.amount_msats() is Some(msats) you still call self.inner.offer.resolve_offer_amount(currency_conversion)? to see if a fiat-derived unit minimum exists and then validate the explicit msats against that minimum and against the allowed range (e.g., MAX_VALUE_MSAT), returning Bolt12SemanticError::InvalidAmount (or MissingAmount if appropriate) on violation; use the existing symbols amount_msats, inner.amount_msats(), inner.offer.resolve_offer_amount, and Bolt12SemanticError variants to locate and implement the checks.lightning/src/ln/channelmanager.rs (1)
8377-8395:⚠️ Potential issue | 🟠 MajorFail the HTLC when invoice-request amount resolution fails.
The
Err(_)arm still onlydebug_assert!s and then returnsverified_invreq, so release builds can continue without enforcing the minimum-amount check.Proposed fix
Err(_) => { // `amount_msats()` can only fail if the invoice request does not specify an amount // and the underlying offer's amount cannot be resolved. // // This invoice request corresponds to an offer we constructed, and we only allow // creating offers with currency amounts that the node explicitly supports. // // Therefore, amount resolution must succeed here. Reaching this branch indicates // an internal logic error. debug_assert!(false); + fail_htlc!(claimable_htlc, payment_hash); },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lightning/src/ln/channelmanager.rs` around lines 8377 - 8395, The Err arm of verified_invreq.amount_msats(...) currently only debug_asserts which allows release builds to skip enforcing the minimum amount; instead, treat this as an internal failure and fail the HTLC the same way as when the invoice amount is less than payment_data.total_msat: replace the debug_assert! in the Err(_) branch with the same fail_htlc!(claimable_htlc, payment_hash) call (or equivalent failure path used elsewhere) so any amount resolution error unconditionally rejects the HTLC; update references to verified_invreq, amount_msats, claimable_htlc, payment_hash and reuse the fail_htlc! macro to locate where to make the change.
🧹 Nitpick comments (5)
fuzz/src/invoice_request_deser.rs (1)
19-22: Imports added forFuzzCurrencyConversion.These imports (
CurrencyConversion,CurrencyCode) support the new struct. IfFuzzCurrencyConversionremains unused, these imports will also be dead code and may trigger compiler warnings.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fuzz/src/invoice_request_deser.rs` around lines 19 - 22, The new imports CurrencyConversion and CurrencyCode were added to support FuzzCurrencyConversion but are currently unused and may trigger compiler warnings; either remove these imports if FuzzCurrencyConversion is not used, or ensure FuzzCurrencyConversion is referenced (or the imports are conditionally compiled) so they are consumed. Locate the symbols CurrencyConversion and CurrencyCode in the top of invoice_request_deser.rs and either delete those use lines or integrate FuzzCurrencyConversion into the deserialization tests/path that requires CurrencyConversion, or annotate the imports/struct with appropriate cfg/allow attributes to suppress dead-code warnings.lightning/src/offers/currency.rs (1)
27-38: Don't freezeCurrencyConversionwider than the behavior implemented today.
tolerance_percent()is part of the new public trait, but the current amount validation path inlightning/src/offers/offer.rs, Lines 949-995, still does strict comparisons and never consults it. Combined withResult<u64, ()>, this exposes a wider public contract than the implementation actually uses. I'd either wire those semantics in now or keep the trait narrower until the first real caller needs them.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lightning/src/offers/currency.rs` around lines 27 - 38, The trait CurrencyConversion is exposing tolerance_percent() even though the offer validation path (see the amount validation in offer.rs that uses msats_per_minor_unit) doesn't use it; either remove tolerance_percent() from CurrencyConversion to keep the trait minimal, or update the offer validation logic to call CurrencyConversion::tolerance_percent() when computing acceptable msat ranges and incorporate that tolerance into the range comparison (also ensure msats_per_minor_unit() error handling is respected); prefer removing tolerance_percent() from the public trait until you wire its semantics in offer validation to avoid widening the public contract prematurely.lightning/src/ln/offers_tests.rs (1)
924-990: Please add the unsupported-currency path next to this happy path.This only exercises the shared-converter success case. A companion case with a currency code that's unsupported by the configured converter would lock down the failure behavior when invoice-request or invoice amount resolution can't convert the offer amount.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lightning/src/ln/offers_tests.rs` around lines 924 - 990, Add a companion test that mirrors creates_and_pays_for_offer_with_fiat_amount_using_one_hop_blinded_path but uses a CurrencyCode the node's alice.node.flow.currency_conversion does not support and asserts the failure path: construct Amount::Currency with an unsupported CurrencyCode, call alice.node.create_offer_builder().amount(..., &alice.node.flow.currency_conversion) and assert it returns an Err (or fails to build the offer), or if the builder allows creating the offer, attempt bob.node.pay_for_offer(...) and assert it returns an Err/awaits a failure state; if the failure happens later, exercise invoice_request/invoice resolution by extracting the invoice_request via extract_invoice_request and assert invoice_request.amount_msats(...) (or invoice.amount_msats()) returns an Err, confirming the unsupported-currency conversion path is handled. Ensure you reuse the same helpers (create_offer_builder, amount, pay_for_offer, extract_invoice_request, invoice_request.amount_msats, extract_invoice, invoice.amount_msats) and mirror the happy-path setup so the only change is the unsupported CurrencyCode and assertions for errors.lightning/src/ln/outbound_payment.rs (1)
3312-3318: Add a true currency-offer regression test for this path.All of these updated fixtures still use
.amount_msats(1000), so they only cover the new argument plumbing. They do not exercisestatic_invoice_receivedwith a fiat-denominated offer or a failingCurrencyConversion, which is where the new behavior lives.Also applies to: 3362-3368, 3428-3434, 3518-3521
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lightning/src/ln/outbound_payment.rs` around lines 3312 - 3318, The fixtures only exercise msat-denominated offers; add a regression test that constructs a fiat-denominated offer and a failing CurrencyConversion to exercise the static_invoice_received code path: create an OfferBuilder invocation that sets a fiat currency (instead of .amount_msats(1000)) or uses the builder method that specifies currency/amount in fiat, call request_invoice(...) with a CurrencyConversion instance that simulates failure, then run the same chain (build/request_invoice/build_and_sign/respond_with_no_std/build) and assert that static_invoice_received behaves as expected (e.g., returns the error/handling path). Reference OfferBuilder, request_invoice, CurrencyConversion, static_invoice_received, payment_paths/payment_hash/created_at to locate where to change or add the new test and ensure similar tests are added for the other referenced ranges (3362-3368, 3428-3434, 3518-3521).lightning/src/ln/channelmanager.rs (1)
3505-3508: Add upgrade guidance for the newCurrencyConversionrequirement.
ChannelManager::newandChannelManagerReadArgs::newnow require aCurrencyConversion, which is a source-compatible break for downstream integrators. A short migration note pointing callers toDefaultCurrencyConversionversus a custom implementation would make this change much easier to adopt.Also applies to: 18274-18279
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lightning/src/ln/channelmanager.rs` around lines 3505 - 3508, ChannelManager::new and ChannelManagerReadArgs::new now require a CurrencyConversion parameter which is a source-compatible breaking change for downstream callers; update the public docs/comments for both constructors to include a short migration note instructing integrators to pass DefaultCurrencyConversion when they don't need custom behavior (showing how to construct it) or implement the CurrencyConversion trait for custom logic, and add a brief example call signature using DefaultCurrencyConversion so callers can quickly adapt their code (reference ChannelManager::new, ChannelManagerReadArgs::new, CurrencyConversion, and DefaultCurrencyConversion).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@fuzz/src/invoice_request_deser.rs`:
- Around line 65-75: FuzzCurrencyConversion is declared but unused and its
methods call unreachable!(), which will panic if exercised; update it so do_test
or build_response can use a non-panicking conversion and make
msats_per_minor_unit return Err(()) (matching DefaultCurrencyConversion) and
tolerance_percent return a sensible default instead of unreachable!(), then wire
an instance of FuzzCurrencyConversion into the fuzz path (e.g., pass into
do_test or build_response where CurrencyConversion is expected) so
currency-denominated invoice requests are handled gracefully during fuzzing.
In `@lightning/src/offers/flow.rs`:
- Around line 839-847: Resolve currency-denominated amounts before building
static/blinded-invoice paths: call
offer.resolve_offer_amount(&self.currency_conversion)? and, if
Some(amount_msats), pass it into the InvoiceRequestBuilder via
.amount_msats(amount_msats)? (same pattern as used when creating `builder` from
offer.request_invoice), ensuring this is applied in the static-invoice creation
code paths (the block that currently discards `Amount::Currency`) and the
similar blocks around the other ranges mentioned; reference
`self.currency_conversion`, `offer.resolve_offer_amount`,
`offer.request_invoice` / `create_invoice_request_builder`, and
`InvoiceRequestBuilder::amount_msats` when making the change.
In `@lightning/src/offers/invoice_request.rs`:
- Around line 54-58: The doctest uses the wrong generic arity for
InvoiceRequestBuilder; update the hidden line so InvoiceRequestBuilder has four
generic parameters ('a, 'b, T, CC) to match the current struct signature. Locate
the doctest block using InvoiceRequestBuilder in invoice_request.rs and change
the hidden construction line (the one starting with #
InvoiceRequestBuilder<...>) to include 'a, 'b, T, CC as the generics for
InvoiceRequestBuilder so the example compiles against the current type
definition.
In `@lightning/src/offers/invoice.rs`:
- Around line 1790-1815: The code in Bolt12Invoice::try_from revalidates amounts
using DefaultCurrencyConversion by calling
invoice_request.amount_msats(&DefaultCurrencyConversion), which makes parsing of
arbitrary remote invoices depend on a local conversion choice; instead, remove
the DefaultCurrencyConversion-based check and validate against a conversion-free
value present in the request TLVs (e.g., the explicit amount field encoded on
the invoice_request) or another invariant that does not require a local currency
converter. Concretely, stop calling
invoice_request.amount_msats(&DefaultCurrencyConversion) to compute
requested_amount_msats and replace the comparison with amount_msats by comparing
amount_msats to the explicit request amount TLV (or presence/absence semantics)
available on invoice_request so parsing no longer depends on
DefaultCurrencyConversion.
- Around line 401-426: In amount_msats, avoid calling into currency_conversion
twice (invoice_request.amount_msats(currency_conversion) and
invoice_request.resolve_offer_amount(currency_conversion)) because a
stateful/rate-sensitive CC can change between calls; instead capture a single
snapshot of the conversion and reuse it for both lookups (e.g., create a local
let cc_snapshot = currency_conversion.clone() or currency_conversion.snapshot()
and pass &cc_snapshot to invoice_request.amount_msats and
invoice_request.resolve_offer_amount), updating the CC generic bounds (e.g.,
require CC: Clone or provide a snapshot method) if necessary so both conversions
use the same consistent data.
In `@lightning/src/offers/offer.rs`:
- Around line 348-350: The setter amount_msats currently panics via expect when
Amount::Bitcoin { amount_msats } can produce InvalidAmount for values >
MAX_VALUE_MSAT; change amount_msats to be fallible instead of calling expect:
have amount_msats return a Result<$return_type, InvalidAmount> (or the builder's
error type) and propagate the error from self.amount(...) so invalid msat values
surface to the caller, or alternatively remove validation here and perform the
MAX_VALUE_MSAT check during build() and return an error there; update all
callers of amount_msats (and tests) to handle the Result accordingly.
---
Duplicate comments:
In `@fuzz/src/full_stack.rs`:
- Around line 625-638: ChannelManager::new is being called with the old
parameter list and missing the new currency_conversion dependency; create a
local FuzzCurrencyConversion instance (e.g., let currency_conversion =
Arc::new(FuzzCurrencyConversion::new(...)) or similar) and pass it into
ChannelManager::new before the logger argument to match the updated
ChannelMan<'a> signature that expects &FuzzCurrencyConversion; update the call
site inside fuzz/src/full_stack.rs to insert currency_conversion (or a
reference/Arc as used elsewhere) immediately prior to Arc::clone(&logger).
- Around line 54-55: Add CurrencyCode to the imports and stop panicking on
unsupported fiat currencies in the fuzz harness: bring CurrencyCode into scope
alongside CurrencyConversion and UnsignedBolt12Invoice, and replace any
unreachable!() calls used when fiat currency conversion fails (e.g., in the code
paths that call into_msats / CurrencyConversion::into_msats) with returning a
semantic error (Err(())) or a zero-value (0) as appropriate so unsupported fiat
inputs surface as errors instead of crashing the target; ensure
functions/branches that previously called unreachable!() now propagate Err(())
or return 0 where the signature expects Result<(), ()> or u64 respectively.
In `@fuzz/src/offer_deser.rs`:
- Around line 56-58: The match arm calling a non-existent Amount::to_msats
should use the consuming method Amount::into_msats; in the Some(amount) branch
pass the moved amount into into_msats (using the same &conversion) and feed its
result to builder.amount_msats, preserving the existing ? error handling and
unwraps. Update the Some(amount) => branch to call
amount.into_msats(&conversion)? and then builder.amount_msats(...) so the code
compiles.
In `@lightning/src/ln/channelmanager.rs`:
- Around line 8377-8395: The Err arm of verified_invreq.amount_msats(...)
currently only debug_asserts which allows release builds to skip enforcing the
minimum amount; instead, treat this as an internal failure and fail the HTLC the
same way as when the invoice amount is less than payment_data.total_msat:
replace the debug_assert! in the Err(_) branch with the same
fail_htlc!(claimable_htlc, payment_hash) call (or equivalent failure path used
elsewhere) so any amount resolution error unconditionally rejects the HTLC;
update references to verified_invreq, amount_msats, claimable_htlc, payment_hash
and reuse the fail_htlc! macro to locate where to make the change.
In `@lightning/src/ln/outbound_payment.rs`:
- Around line 1283-1294: The amount_msat lookup can legitimately fail due to
currency_conversion, so don't treat all errors as an LDK-required-features bug;
in the Err(_) arm of InvoiceBuilder::<DerivedSigningPubkey>::amount_msats handle
the error explicitly by capturing the original error (e), map or wrap it into a
meaningful Bolt12PaymentError variant (or add one like CurrencyConversionFailed)
instead of returning UnknownRequiredFeatures, and call
abandon_with_entry!(entry, PaymentFailureReason::InvalidInvoice) or a new
PaymentFailureReason::CurrencyConversionFailed so the pending payment is
abandoned with the correct reason rather than misreported as UnexpectedError.
In `@lightning/src/offers/invoice_request.rs`:
- Around line 1172-1188: The Some(msats) branch in InvoiceRequest::amount_msats
currently returns the raw explicit msats without revalidating against
fiat-denominated offer constraints; update amount_msats so that when
inner.amount_msats() is Some(msats) you still call
self.inner.offer.resolve_offer_amount(currency_conversion)? to see if a
fiat-derived unit minimum exists and then validate the explicit msats against
that minimum and against the allowed range (e.g., MAX_VALUE_MSAT), returning
Bolt12SemanticError::InvalidAmount (or MissingAmount if appropriate) on
violation; use the existing symbols amount_msats, inner.amount_msats(),
inner.offer.resolve_offer_amount, and Bolt12SemanticError variants to locate and
implement the checks.
---
Nitpick comments:
In `@fuzz/src/invoice_request_deser.rs`:
- Around line 19-22: The new imports CurrencyConversion and CurrencyCode were
added to support FuzzCurrencyConversion but are currently unused and may trigger
compiler warnings; either remove these imports if FuzzCurrencyConversion is not
used, or ensure FuzzCurrencyConversion is referenced (or the imports are
conditionally compiled) so they are consumed. Locate the symbols
CurrencyConversion and CurrencyCode in the top of invoice_request_deser.rs and
either delete those use lines or integrate FuzzCurrencyConversion into the
deserialization tests/path that requires CurrencyConversion, or annotate the
imports/struct with appropriate cfg/allow attributes to suppress dead-code
warnings.
In `@lightning/src/ln/channelmanager.rs`:
- Around line 3505-3508: ChannelManager::new and ChannelManagerReadArgs::new now
require a CurrencyConversion parameter which is a source-compatible breaking
change for downstream callers; update the public docs/comments for both
constructors to include a short migration note instructing integrators to pass
DefaultCurrencyConversion when they don't need custom behavior (showing how to
construct it) or implement the CurrencyConversion trait for custom logic, and
add a brief example call signature using DefaultCurrencyConversion so callers
can quickly adapt their code (reference ChannelManager::new,
ChannelManagerReadArgs::new, CurrencyConversion, and DefaultCurrencyConversion).
In `@lightning/src/ln/offers_tests.rs`:
- Around line 924-990: Add a companion test that mirrors
creates_and_pays_for_offer_with_fiat_amount_using_one_hop_blinded_path but uses
a CurrencyCode the node's alice.node.flow.currency_conversion does not support
and asserts the failure path: construct Amount::Currency with an unsupported
CurrencyCode, call alice.node.create_offer_builder().amount(...,
&alice.node.flow.currency_conversion) and assert it returns an Err (or fails to
build the offer), or if the builder allows creating the offer, attempt
bob.node.pay_for_offer(...) and assert it returns an Err/awaits a failure state;
if the failure happens later, exercise invoice_request/invoice resolution by
extracting the invoice_request via extract_invoice_request and assert
invoice_request.amount_msats(...) (or invoice.amount_msats()) returns an Err,
confirming the unsupported-currency conversion path is handled. Ensure you reuse
the same helpers (create_offer_builder, amount, pay_for_offer,
extract_invoice_request, invoice_request.amount_msats, extract_invoice,
invoice.amount_msats) and mirror the happy-path setup so the only change is the
unsupported CurrencyCode and assertions for errors.
In `@lightning/src/ln/outbound_payment.rs`:
- Around line 3312-3318: The fixtures only exercise msat-denominated offers; add
a regression test that constructs a fiat-denominated offer and a failing
CurrencyConversion to exercise the static_invoice_received code path: create an
OfferBuilder invocation that sets a fiat currency (instead of
.amount_msats(1000)) or uses the builder method that specifies currency/amount
in fiat, call request_invoice(...) with a CurrencyConversion instance that
simulates failure, then run the same chain
(build/request_invoice/build_and_sign/respond_with_no_std/build) and assert that
static_invoice_received behaves as expected (e.g., returns the error/handling
path). Reference OfferBuilder, request_invoice, CurrencyConversion,
static_invoice_received, payment_paths/payment_hash/created_at to locate where
to change or add the new test and ensure similar tests are added for the other
referenced ranges (3362-3368, 3428-3434, 3518-3521).
In `@lightning/src/offers/currency.rs`:
- Around line 27-38: The trait CurrencyConversion is exposing
tolerance_percent() even though the offer validation path (see the amount
validation in offer.rs that uses msats_per_minor_unit) doesn't use it; either
remove tolerance_percent() from CurrencyConversion to keep the trait minimal, or
update the offer validation logic to call
CurrencyConversion::tolerance_percent() when computing acceptable msat ranges
and incorporate that tolerance into the range comparison (also ensure
msats_per_minor_unit() error handling is respected); prefer removing
tolerance_percent() from the public trait until you wire its semantics in offer
validation to avoid widening the public contract prematurely.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d9198173-a0f4-4a77-8796-4c796df0eec4
📒 Files selected for processing (21)
fuzz/src/full_stack.rsfuzz/src/invoice_request_deser.rsfuzz/src/offer_deser.rslightning-background-processor/src/lib.rslightning-block-sync/src/init.rslightning/src/ln/async_payments_tests.rslightning/src/ln/channelmanager.rslightning/src/ln/functional_test_utils.rslightning/src/ln/functional_tests.rslightning/src/ln/offers_tests.rslightning/src/ln/outbound_payment.rslightning/src/ln/reload_tests.rslightning/src/offers/currency.rslightning/src/offers/flow.rslightning/src/offers/invoice.rslightning/src/offers/invoice_request.rslightning/src/offers/merkle.rslightning/src/offers/mod.rslightning/src/offers/offer.rslightning/src/offers/refund.rslightning/src/util/test_utils.rs
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
lightning/src/ln/outbound_payment.rs (1)
3301-3318: Add one fiat-denominated regression case.These tests now pass
TestCurrencyConversion, but every shown offer still uses.amount_msats(1000), so the new conversion-dependent path never runs. One currency-denominated async/static-invoice case would keep this wiring from regressing silently.Also applies to: 3359-3368, 3425-3434, 3516-3522
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lightning/src/ln/outbound_payment.rs` around lines 3301 - 3318, The tests currently always call OfferBuilder::amount_msats(1000) so the TestCurrencyConversion path is never exercised; replace one of the test cases (e.g., the block that builds invoice using OfferBuilder, request_invoice, respond_with_no_std) to create a fiat/currency-denominated offer instead of amount_msats — use the builder's currency-denominated method (e.g., amount_currency / amount_fiat or the project-specific equivalent) so TestCurrencyConversion is invoked; make the same change for the other instances noted (around the blocks at 3359-3368, 3425-3434, 3516-3522) to add at least one currency-denominated regression case.lightning/src/ln/functional_test_utils.rs (1)
898-919: Reuse the node’s configured converter in the round-trip reload check.Line 918 is the only new reload path here that hardcodes a fresh
TestCurrencyConversioninstead of reusingself.currency_conversion. Using the injected instance keeps this smoke test aligned with the node’s actual wiring and avoids drift if the test converter ever gains state or per-node expectations.♻️ Suggested tweak
- currency_conversion: &test_utils::TestCurrencyConversion, + currency_conversion: self.currency_conversion,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lightning/src/ln/functional_test_utils.rs` around lines 898 - 919, The reload path in the ChannelManager deserialization uses a fresh TestCurrencyConversion instead of the node's configured converter; update the ChannelManagerReadArgs construction (the call building ChannelManagerReadArgs passed to <Type>::read) to pass self.currency_conversion instead of &test_utils::TestCurrencyConversion so the round-trip reload uses the injected per-node converter (replace the reference to TestCurrencyConversion with self.currency_conversion where currency_conversion is set in ChannelManagerReadArgs).fuzz/src/offer_deser.rs (1)
15-15:DefaultCurrencyConversiondrops fiat offers out of this fuzz path.On Lines 52-58,
amount.into_msats(&conversion)?only succeeds forAmount::Bitcoin;DefaultCurrencyConversionrejects every currency amount. That means fiat-denominated offers now exitbuild_requestbeforeInvoiceRequestserialization, which regresses fuzz coverage for the new BOLT 12 currency flow.🧪 Suggested fuzz-only converter
-use lightning::offers::currency::DefaultCurrencyConversion; +use lightning::offers::currency::{CurrencyCode, CurrencyConversion}; +struct FuzzCurrencyConversion; + +impl CurrencyConversion for FuzzCurrencyConversion { + fn msats_per_minor_unit(&self, _iso4217_code: CurrencyCode) -> Result<u64, ()> { + Ok(1_000) + } + + fn tolerance_percent(&self) -> u8 { + 0 + } +} + fn build_request(offer: &Offer) -> Result<InvoiceRequest, Bolt12SemanticError> { let expanded_key = ExpandedKey::new([42; 32]); let entropy = FixedEntropy {}; let nonce = Nonce::from_entropy_source(&entropy); let secp_ctx = Secp256k1::new(); let payment_id = PaymentId([1; 32]); - let conversion = DefaultCurrencyConversion; + let conversion = FuzzCurrencyConversion;Also applies to: 52-58
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fuzz/src/offer_deser.rs` at line 15, The fuzz path currently uses DefaultCurrencyConversion which rejects non-bitcoin amounts and causes build_request (specifically the amount.into_msats(&conversion) call) to drop fiat offers; replace DefaultCurrencyConversion in the fuzz harness with a small fuzz-only converter (e.g., FuzzCurrencyConversion) that implements the same CurrencyConversion trait used by lightning::offers::currency and returns a deterministic msat value for fiat and bitcoin Amount variants so amount.into_msats(&conversion) succeeds for all currencies; update the instantiation passed into build_request (and any test helpers that construct the conversion) to use this fuzz converter so InvoiceRequest serialization continues to be exercised for fiat offers.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lightning/src/offers/offer.rs`:
- Around line 355-359: The amount setter currently converts any Amount
(including Amount::Currency) using the provided CurrencyConversion and rejects
it if it exceeds MAX_VALUE_MSAT, which causes builder-time rejection of fiat
offers; change pub fn amount (the amount method) to only enforce MAX_VALUE_MSAT
for native Bitcoin amounts (satoshis/msats) and skip conversion/fiat limit
checks for Amount::Currency so fiat-denominated amounts are accepted during
offer construction; ensure the conversion-and-MAX_VALUE_MSAT validation is
performed later when the offer is resolved into an invoice or invoice request
(where Offer::try_from or the invoice-resolution path converts Currency via
CurrencyConversion and enforces MAX_VALUE_MSAT).
- Around line 343-350: The module example uses Offer::amount_msats which now
returns Result, so update the doctest to handle the fallible API: either append
`?` to the amount_msats call and make the example function return a compatible
Result (e.g., -> Result<(), Box<dyn std::error::Error>>), or call `.unwrap()` on
amount_msats; modify the example surrounding code accordingly so the doctest
compiles (refer to Offer::amount_msats).
---
Nitpick comments:
In `@fuzz/src/offer_deser.rs`:
- Line 15: The fuzz path currently uses DefaultCurrencyConversion which rejects
non-bitcoin amounts and causes build_request (specifically the
amount.into_msats(&conversion) call) to drop fiat offers; replace
DefaultCurrencyConversion in the fuzz harness with a small fuzz-only converter
(e.g., FuzzCurrencyConversion) that implements the same CurrencyConversion trait
used by lightning::offers::currency and returns a deterministic msat value for
fiat and bitcoin Amount variants so amount.into_msats(&conversion) succeeds for
all currencies; update the instantiation passed into build_request (and any test
helpers that construct the conversion) to use this fuzz converter so
InvoiceRequest serialization continues to be exercised for fiat offers.
In `@lightning/src/ln/functional_test_utils.rs`:
- Around line 898-919: The reload path in the ChannelManager deserialization
uses a fresh TestCurrencyConversion instead of the node's configured converter;
update the ChannelManagerReadArgs construction (the call building
ChannelManagerReadArgs passed to <Type>::read) to pass self.currency_conversion
instead of &test_utils::TestCurrencyConversion so the round-trip reload uses the
injected per-node converter (replace the reference to TestCurrencyConversion
with self.currency_conversion where currency_conversion is set in
ChannelManagerReadArgs).
In `@lightning/src/ln/outbound_payment.rs`:
- Around line 3301-3318: The tests currently always call
OfferBuilder::amount_msats(1000) so the TestCurrencyConversion path is never
exercised; replace one of the test cases (e.g., the block that builds invoice
using OfferBuilder, request_invoice, respond_with_no_std) to create a
fiat/currency-denominated offer instead of amount_msats — use the builder's
currency-denominated method (e.g., amount_currency / amount_fiat or the
project-specific equivalent) so TestCurrencyConversion is invoked; make the same
change for the other instances noted (around the blocks at 3359-3368, 3425-3434,
3516-3522) to add at least one currency-denominated regression case.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ab4f7c84-da44-42b6-bb67-a53aa44c0453
📒 Files selected for processing (18)
fuzz/src/full_stack.rsfuzz/src/offer_deser.rslightning-background-processor/src/lib.rslightning-block-sync/src/init.rslightning/src/ln/async_payments_tests.rslightning/src/ln/channelmanager.rslightning/src/ln/functional_test_utils.rslightning/src/ln/functional_tests.rslightning/src/ln/offers_tests.rslightning/src/ln/outbound_payment.rslightning/src/ln/reload_tests.rslightning/src/offers/flow.rslightning/src/offers/invoice.rslightning/src/offers/invoice_request.rslightning/src/offers/merkle.rslightning/src/offers/offer.rslightning/src/offers/refund.rslightning/src/util/test_utils.rs
🚧 Files skipped from review as they are similar to previous changes (4)
- lightning/src/ln/offers_tests.rs
- lightning-block-sync/src/init.rs
- lightning/src/offers/merkle.rs
- lightning/src/ln/functional_tests.rs
shaavan
force-pushed
the
currency-review
branch
2 times, most recently
from
3951302 to
051fd9d
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
lightning/src/offers/invoice_request.rs (1)
53-58:⚠️ Potential issue | 🟡 MinorFix the doctest’s
InvoiceRequestBuildergeneric arity.The hidden example still instantiates
InvoiceRequestBuilderwith two generic arguments even though the type now has four ('a,'b,T,CC), so this doctest no longer matches the current Rust type definition.🛠️ Suggested fix
-//! # <InvoiceRequestBuilder<_, _>>::from( +//! # <InvoiceRequestBuilder<'_, '_, _, _>>::from(Run this to confirm the mismatch:
#!/bin/bash set -euo pipefail echo "=== InvoiceRequestBuilder definition ===" rg -n "pub struct InvoiceRequestBuilder<'a, 'b, T: secp256k1::Signing, CC: CurrencyConversion>" lightning/src/offers/invoice_request.rs echo echo "=== Doctest constructor usage ===" rg -n -C2 '<InvoiceRequestBuilder<_, _>>::from' lightning/src/offers/invoice_request.rs🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lightning/src/offers/invoice_request.rs` around lines 53 - 58, The doctest uses the wrong generic arity for InvoiceRequestBuilder: update the hidden constructor invocation that currently uses <InvoiceRequestBuilder<_, _>>::from(...) to match the struct definition with four generics ('a, 'b, T, CC) by replacing the two-parameter placeholder with four placeholders (e.g., <_, _, _, _>) so the doctest compiles against the current pub struct InvoiceRequestBuilder<'a, 'b, T: secp256k1::Signing, CC: CurrencyConversion>; locate the hidden example around the InvoiceRequestBuilder usage and adjust the generic placeholders accordingly.lightning/src/ln/outbound_payment.rs (1)
1283-1294:⚠️ Potential issue | 🟡 MinorDon’t collapse conversion/amount failures into
UnknownRequiredFeatures.Line 1293 still turns any
InvoiceBuilder::amount_msatserror intoBolt12PaymentError::UnknownRequiredFeatures, but that call now propagates currency-resolution failures as well. For currency-denominated static invoices, an unsupported currency or invalid converted amount will be reported as a feature-negotiation problem, which is misleading for callers and logs. Please surface a dedicated amount/conversion failure here instead of mapping everything toUnknownRequiredFeatures.
🧹 Nitpick comments (5)
lightning/src/offers/invoice.rs (2)
1929-1950: Add a currency-denominated round-trip test in this file.These updates thread
TestCurrencyConversionthrough the API, but this test still exercises only the bitcoin-denominated.amount_msats(1000)path. That leaves the new converter-dependent branches inInvoiceBuilder::amount_msatsand the parse-time request-amount validation untested locally. A fiat-denominated happy path plus one failure case would make this change much harder to regress.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lightning/src/offers/invoice.rs` around lines 1929 - 1950, The existing test builds_invoice_for_offer_with_defaults only exercises bitcoin-denominated flow; add two new tests that thread TestCurrencyConversion through the same flow but use a fiat-denominated amount to exercise InvoiceBuilder::amount_msats and the parse-time request-amount validation: (1) a fiat-happy-path that constructs an OfferBuilder with a fiat amount (using the test conversion to convert fiat to msats), then call request_invoice(...).build_and_sign().respond_with_no_std(...) and assert the invoice amounts/currency fields match expected fiat-derived values; and (2) a fiat-failure case that supplies an inconsistent/invalid request amount (e.g., mismatch between declared fiat and converted msats) and assert parsing/request validation returns an error. Reuse TestCurrencyConversion, OfferBuilder, InvoiceBuilder::amount_msats, request_invoice, build_and_sign, and respond_with_no_std to locate where to insert the tests.
1790-1812: Clarify the invariant in this parse-path note.The code is fine, but the explanation leans on “the invoice request we created,” which is narrower than the real guarantee here. The safety comes from
amount_msats()reading an explicitly encoded request amount first and only reaching fiat resolution when that field is absent, so I’d phrase the comment in those terms to avoid misleading future changes in this public parser.Based on learnings:
amount_msats()checks the explicit request amount first, and theDefaultCurrencyConversionfallback is unreachable in valid flows.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lightning/src/offers/invoice.rs` around lines 1790 - 1812, Clarify the comment explaining why invoice_request.amount_msats(&DefaultCurrencyConversion) cannot fail: state that amount_msats() first reads an explicitly encoded request amount (and only consults currency conversion if that explicit field is absent), and in this parse path we always encode an explicit request amount when the offer is currency-denominated, so the DefaultCurrencyConversion fallback is unreachable; update the note near the match on invoice_request.amount_msats(&DefaultCurrencyConversion) (referencing invoice_request.amount_msats and DefaultCurrencyConversion) to reflect this stronger invariant and remove the narrower phrasing “the invoice request we created.”lightning/src/offers/invoice_request.rs (1)
2145-2162: Add one checked fiatInvoiceRequestBuildertest path.These new currency cases only use
build_unchecked/build_unchecked_and_sign, so they bypass thebuild_with_checkspath this PR changed. A regression in the new checkedcurrency_conversionplumbing would still pass this suite.Also applies to: 2532-2553
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lightning/src/offers/invoice_request.rs` around lines 2145 - 2162, The test currently uses unchecked paths (build_unchecked / build_unchecked_and_sign) so it never exercises the new checked validation; update the test to exercise the checked builder path by replacing build_unchecked() with the checked builder API (e.g., build_with_checks() or the OfferBuilder method that performs validation) and replace build_unchecked_and_sign() with the corresponding checked/signing method (e.g., build_and_sign() or the checked invoice request sign method) so that OfferBuilder::...request_invoice(...) and the InvoiceRequest builder perform validation; apply the same change to the similar test at the other location covering lines 2532-2553.lightning/src/ln/offers_tests.rs (1)
937-989: Derive the expected fiat conversion result instead of hard-coding it.This test currently bakes in
TestCurrencyConversion’s USD fixture rate via1_000_000. Computingexpected_msatsonce from the built offer would keep the test validating the fiat flow without coupling it to a specific mock-rate constant.Based on learnings: repeated calls to `currency_conversion` within a single payment flow are intentionally safe and expected to return consistent rates.♻️ Suggested cleanup
let offer = alice.node .create_offer_builder().unwrap() .amount(amount, &alice.node.flow.currency_conversion).unwrap() .build(); + let expected_msats = offer.amount_msats(&alice.node.flow.currency_conversion).unwrap(); assert_ne!(offer.issuer_signing_pubkey(), Some(alice_id)); assert!(!offer.paths().is_empty()); for path in offer.paths() { assert!(check_compact_path_introduction_node(&path, bob, alice_id)); } @@ - assert_eq!(invoice_request.amount_msats(&alice.node.flow.currency_conversion), Ok(1_000_000)); + assert_eq!(invoice_request.amount_msats(&alice.node.flow.currency_conversion), Ok(expected_msats)); @@ - assert_eq!(invoice.amount_msats(), 1_000_000); + assert_eq!(invoice.amount_msats(), expected_msats);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lightning/src/ln/offers_tests.rs` around lines 937 - 989, The test hard-codes 1_000_000 msats; instead compute expected_msats from the built offer and use it in the assertions. After building offer (symbol: offer) call offer.amount_msats(&alice.node.flow.currency_conversion).unwrap() (or the equivalent Result unwrapping used elsewhere) into let expected_msats and replace the two assert_eq! checks that compare invoice_request.amount_msats(...) and invoice.amount_msats() to 1_000_000 with comparisons to expected_msats; keep the rest of the flow unchanged.lightning/src/ln/outbound_payment.rs (1)
3301-3318: Please add one fiat/static-invoice regression test.These updates thread
TestCurrencyConversionthrough the existing helpers, but the changed tests still build only.amount_msats(...)offers. A single currency-denominated offer that reachesstatic_invoice_receivedwould give this PR coverage for the new path instead of only keeping the old msat-only cases compiling.Also applies to: 3359-3368, 3425-3434, 3516-3521
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lightning/src/ln/outbound_payment.rs` around lines 3301 - 3318, The tests currently only exercise msat-denominated offers; add a single regression test that constructs an OfferBuilder with a fiat/currency-denominated amount and threads TestCurrencyConversion through the invoice flow so the code path that calls static_invoice_received is exercised. Concretely, in the same test block that creates conversion = TestCurrencyConversion and calls outbound_payments.add_new_awaiting_invoice(...) replace (or add a parallel case to) the OfferBuilder usage that calls .amount_msats(...) with the API that sets a currency-denominated amount (e.g., .amount_currency(...) or .amount_fiat(...) depending on the helper) and then continue to call .request_invoice(...), .build_and_sign(), .respond_with_no_std(...), and .build() so that the static invoice handling path is hit; ensure the created_at, nonce, expanded_key, payment_id and conversion variables are reused so the test compiles and specifically validates the static_invoice_received behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lightning/src/offers/flow.rs`:
- Around line 869-870: The code resolves a concrete amount into amount_msat via
offer.resolve_offer_amount(&self.currency_conversion) but
create_static_invoice_for_server still calls
inbound_payment::create_for_spontaneous_payment with None, causing the
static-invoice payment-secret constraints to diverge; update
create_static_invoice_for_server to pass Some(amount_msat) (or the resolved
AmountMsat variable returned by resolve_offer_amount) into
inbound_payment::create_for_spontaneous_payment so the payment-secret is created
with the same amount as the advertised static invoice, ensuring the async
static-invoice payment secret stays in sync with resolve_offer_amount.
---
Duplicate comments:
In `@lightning/src/offers/invoice_request.rs`:
- Around line 53-58: The doctest uses the wrong generic arity for
InvoiceRequestBuilder: update the hidden constructor invocation that currently
uses <InvoiceRequestBuilder<_, _>>::from(...) to match the struct definition
with four generics ('a, 'b, T, CC) by replacing the two-parameter placeholder
with four placeholders (e.g., <_, _, _, _>) so the doctest compiles against the
current pub struct InvoiceRequestBuilder<'a, 'b, T: secp256k1::Signing, CC:
CurrencyConversion>; locate the hidden example around the InvoiceRequestBuilder
usage and adjust the generic placeholders accordingly.
---
Nitpick comments:
In `@lightning/src/ln/offers_tests.rs`:
- Around line 937-989: The test hard-codes 1_000_000 msats; instead compute
expected_msats from the built offer and use it in the assertions. After building
offer (symbol: offer) call
offer.amount_msats(&alice.node.flow.currency_conversion).unwrap() (or the
equivalent Result unwrapping used elsewhere) into let expected_msats and replace
the two assert_eq! checks that compare invoice_request.amount_msats(...) and
invoice.amount_msats() to 1_000_000 with comparisons to expected_msats; keep the
rest of the flow unchanged.
In `@lightning/src/ln/outbound_payment.rs`:
- Around line 3301-3318: The tests currently only exercise msat-denominated
offers; add a single regression test that constructs an OfferBuilder with a
fiat/currency-denominated amount and threads TestCurrencyConversion through the
invoice flow so the code path that calls static_invoice_received is exercised.
Concretely, in the same test block that creates conversion =
TestCurrencyConversion and calls outbound_payments.add_new_awaiting_invoice(...)
replace (or add a parallel case to) the OfferBuilder usage that calls
.amount_msats(...) with the API that sets a currency-denominated amount (e.g.,
.amount_currency(...) or .amount_fiat(...) depending on the helper) and then
continue to call .request_invoice(...), .build_and_sign(),
.respond_with_no_std(...), and .build() so that the static invoice handling path
is hit; ensure the created_at, nonce, expanded_key, payment_id and conversion
variables are reused so the test compiles and specifically validates the
static_invoice_received behavior.
In `@lightning/src/offers/invoice_request.rs`:
- Around line 2145-2162: The test currently uses unchecked paths
(build_unchecked / build_unchecked_and_sign) so it never exercises the new
checked validation; update the test to exercise the checked builder path by
replacing build_unchecked() with the checked builder API (e.g.,
build_with_checks() or the OfferBuilder method that performs validation) and
replace build_unchecked_and_sign() with the corresponding checked/signing method
(e.g., build_and_sign() or the checked invoice request sign method) so that
OfferBuilder::...request_invoice(...) and the InvoiceRequest builder perform
validation; apply the same change to the similar test at the other location
covering lines 2532-2553.
In `@lightning/src/offers/invoice.rs`:
- Around line 1929-1950: The existing test
builds_invoice_for_offer_with_defaults only exercises bitcoin-denominated flow;
add two new tests that thread TestCurrencyConversion through the same flow but
use a fiat-denominated amount to exercise InvoiceBuilder::amount_msats and the
parse-time request-amount validation: (1) a fiat-happy-path that constructs an
OfferBuilder with a fiat amount (using the test conversion to convert fiat to
msats), then call request_invoice(...).build_and_sign().respond_with_no_std(...)
and assert the invoice amounts/currency fields match expected fiat-derived
values; and (2) a fiat-failure case that supplies an inconsistent/invalid
request amount (e.g., mismatch between declared fiat and converted msats) and
assert parsing/request validation returns an error. Reuse
TestCurrencyConversion, OfferBuilder, InvoiceBuilder::amount_msats,
request_invoice, build_and_sign, and respond_with_no_std to locate where to
insert the tests.
- Around line 1790-1812: Clarify the comment explaining why
invoice_request.amount_msats(&DefaultCurrencyConversion) cannot fail: state that
amount_msats() first reads an explicitly encoded request amount (and only
consults currency conversion if that explicit field is absent), and in this
parse path we always encode an explicit request amount when the offer is
currency-denominated, so the DefaultCurrencyConversion fallback is unreachable;
update the note near the match on
invoice_request.amount_msats(&DefaultCurrencyConversion) (referencing
invoice_request.amount_msats and DefaultCurrencyConversion) to reflect this
stronger invariant and remove the narrower phrasing “the invoice request we
created.”
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d52ed47f-858f-4172-bfc7-e29443cdd91b
📒 Files selected for processing (22)
fuzz/src/full_stack.rsfuzz/src/offer_deser.rslightning-background-processor/src/lib.rslightning-block-sync/src/init.rslightning-dns-resolver/src/lib.rslightning/src/ln/async_payments_tests.rslightning/src/ln/channelmanager.rslightning/src/ln/functional_test_utils.rslightning/src/ln/functional_tests.rslightning/src/ln/max_payment_path_len_tests.rslightning/src/ln/offers_tests.rslightning/src/ln/outbound_payment.rslightning/src/ln/reload_tests.rslightning/src/offers/flow.rslightning/src/offers/invoice.rslightning/src/offers/invoice_request.rslightning/src/offers/merkle.rslightning/src/offers/offer.rslightning/src/offers/refund.rslightning/src/offers/static_invoice.rslightning/src/offers/test_utils.rslightning/src/util/test_utils.rs
🚧 Files skipped from review as they are similar to previous changes (4)
- lightning-block-sync/src/init.rs
- lightning/src/offers/refund.rs
- lightning/src/ln/functional_tests.rs
- lightning/src/util/test_utils.rs
shaavan
force-pushed
the
currency-review
branch
from
051fd9d to
0b9310c
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
lightning/src/ln/outbound_payment.rs (1)
1283-1293:⚠️ Potential issue | 🟡 MinorAlign the outward error with the abandoned payment reason.
This branch records
PaymentFailureReason::UnexpectedErrorinternally, but returnsBolt12PaymentError::UnknownRequiredFeaturesto the caller. That makes the same failure look like two different problems. Either map both layers to the same failure, or make this path explicitly unreachable ifamount_msatstruly cannot fail here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lightning/src/ln/outbound_payment.rs` around lines 1283 - 1293, In the Err(_) branch of InvoiceBuilder::<DerivedSigningPubkey>::amount_msats(...) make the internal and external errors consistent: update the abandon_with_entry! call and the returned Bolt12PaymentError so they represent the same failure (e.g., keep abandon_with_entry!(entry, PaymentFailureReason::UnexpectedError) and change the return to Err(Bolt12PaymentError::UnexpectedError), or alternatively change the abandon_with_entry! to PaymentFailureReason::UnknownRequiredFeatures and keep the current return); apply the change inside the same match arm so the logged/recorded PaymentFailureReason and the Bolt12PaymentError returned to the caller are identical.lightning/src/offers/invoice_request.rs (1)
53-57:⚠️ Potential issue | 🟡 MinorFix the doctest's
InvoiceRequestBuildergeneric arity.Line 54 still instantiates
InvoiceRequestBuilderwith two generic arguments, but the type now takes'a,'b,T, andCC, so this example no longer matches the public API.📝 Minimal doc fix
-//! # <InvoiceRequestBuilder<_, _>>::from( +//! # <InvoiceRequestBuilder<'_, '_, _, _>>::from(Verify by comparing the builder definition with the doctest helper line. The first command should show four generic parameters on the struct, and the second should highlight the stale two-parameter doctest:
#!/bin/bash set -euo pipefail rg -n "pub struct InvoiceRequestBuilder<'a, 'b, T: secp256k1::Signing, CC: CurrencyConversion>" lightning/src/offers/invoice_request.rs rg -n -C1 "InvoiceRequestBuilder<_, _>|InvoiceRequestBuilder<'_, '_, _, _>" lightning/src/offers/invoice_request.rs🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lightning/src/offers/invoice_request.rs` around lines 53 - 57, The doctest instantiates InvoiceRequestBuilder with only two generics but the struct now declares four (<'a, 'b, T, CC>); update the doctest example to use the correct arity — e.g., change InvoiceRequestBuilder<_, _> to InvoiceRequestBuilder<'_, '_, _, _> (or explicit lifetimes/types matching the surrounding example) so the helper line matches the pub struct InvoiceRequestBuilder<'a, 'b, T: secp256k1::Signing, CC: CurrencyConversion> declaration and the doctest compiles.lightning/src/offers/offer.rs (1)
360-367:⚠️ Potential issue | 🟠 MajorDon't validate fiat offers against today's conversion rate in the setter.
This now converts
Amount::Currencyimmediately and can fail withUnsupportedCurrencyorInvalidAmountbased on the caller's current rate cache, whileOffer::try_fromstill accepts the same wire offer without any conversion. That makes local fiat-offer creation rate-dependent when the converted limit should only be enforced once the offer is actually resolved into an invoice request or invoice.♻️ Safer direction
pub fn amount<CC: CurrencyConversion>($($self_mut)* $self: $self_type, amount: Amount, currency_conversion: &CC) -> Result<$return_type, Bolt12SemanticError> { - if amount.into_msats(currency_conversion)? > MAX_VALUE_MSAT { - return Err(Bolt12SemanticError::InvalidAmount); + let _ = currency_conversion; + if let Amount::Bitcoin { amount_msats } = amount { + if amount_msats > MAX_VALUE_MSAT { + return Err(Bolt12SemanticError::InvalidAmount); + } } $self.offer.amount = Some(amount); Ok($return_value) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lightning/src/offers/offer.rs` around lines 360 - 367, The amount setter (pub fn amount<CC: CurrencyConversion>(...)) currently converts Amount::Currency using currency_conversion and enforces MAX_VALUE_MSAT, making setting fiat rate-dependent; remove the conversion and MAX_VALUE_MSAT check from this method so it only stores self.offer.amount = Some(amount) and returns as before, and instead perform currency-to-msat conversion and MAX_VALUE_MSAT validation at offer resolution time (e.g., in Offer::try_from or the invoice-request/invoice creation code paths) so wire-parsed offers remain accepted regardless of the local rate cache.lightning/src/offers/flow.rs (1)
1701-1707:⚠️ Potential issue | 🟠 MajorPass the resolved static-invoice amount into the payment secret.
Line 1703 still hard-codes
None, so the payment-secret constraints can diverge from the concrete amount now advertised by the static invoice for currency-denominated offers.🐛 Minimal fix
fn create_static_invoice_for_server<R: Router>( &self, offer: &Offer, offer_nonce: Nonce, peers: Vec<MessageForwardNode>, usable_channels: Vec<ChannelDetails>, router: R, ) -> Result<(StaticInvoice, BlindedMessagePath), ()> { let expanded_key = &self.inbound_payment_key; let duration_since_epoch = self.duration_since_epoch(); let secp_ctx = &self.secp_ctx; + let amount_msat = offer.resolve_offer_amount(&self.currency_conversion).map_err(|_| ())?; let offer_relative_expiry = offer .absolute_expiry() .map(|exp| exp.saturating_sub(duration_since_epoch).as_secs()) @@ let payment_secret = inbound_payment::create_for_spontaneous_payment( expanded_key, - None, // The async receive offers we create are always amount-less + amount_msat, offer_relative_expiry, duration_since_epoch.as_secs(), None, )?;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lightning/src/offers/flow.rs` around lines 1701 - 1707, The payment_secret call to inbound_payment::create_for_spontaneous_payment is still passing None for the amount; replace that None with the resolved static-invoice amount so the payment-secret constraints match the advertised concrete amount. Locate where the static invoice amount is computed (the resolved/static invoice amount variable used when building the static invoice) and pass that variable as the second argument to create_for_spontaneous_payment instead of None, keeping the existing expanded_key, offer_relative_expiry, and duration_since_epoch arguments.
🧹 Nitpick comments (1)
lightning/src/offers/invoice.rs (1)
1938-1953: Add one fiat-denominated regression case here.These updates prove the new
CurrencyConversionparameter is threaded through the builders, but this test still exercises an msat-priced offer. A focused currency-priced offer case in this module would cover the newresolve_offer_amountpath and make the feature this PR adds much harder to regress.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lightning/src/offers/invoice.rs` around lines 1938 - 1953, Add a second test case that creates a fiat-denominated offer to exercise resolve_offer_amount: construct an OfferBuilder (same recipient_pubkey()) but set the offer price in a fiat currency (use the TestCurrencyConversion test helper/instance) so the builder path that resolves fiat amounts is taken, then call request_invoice(&expanded_key, nonce, &secp_ctx, payment_id, &conversion) and follow with build_and_sign() and respond_with_no_std(&conversion, payment_paths.clone(), payment_hash, now) as in the existing msat case; assert the invoice amount is resolved correctly to msats so the new CurrencyConversion plumbing is covered.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@fuzz/src/offer_deser.rs`:
- Around line 52-58: The fuzzer uses DefaultCurrencyConversion which rejects all
fiat codes, so replace it with a fuzz-local conversion that resolves fiat
amounts (e.g., implement a SimpleFuzzCurrencyConversion used in this file) and
instantiate that instead of DefaultCurrencyConversion; ensure the new type
implements the same trait/trait methods used by amount.into_msats(&conversion)
(accept common ISO codes or return a deterministic fixed-rate conversion) so
Amount::Currency paths succeed and reach offer.request_invoice/amount.into_msats
rather than early-returning.
---
Duplicate comments:
In `@lightning/src/ln/outbound_payment.rs`:
- Around line 1283-1293: In the Err(_) branch of
InvoiceBuilder::<DerivedSigningPubkey>::amount_msats(...) make the internal and
external errors consistent: update the abandon_with_entry! call and the returned
Bolt12PaymentError so they represent the same failure (e.g., keep
abandon_with_entry!(entry, PaymentFailureReason::UnexpectedError) and change the
return to Err(Bolt12PaymentError::UnexpectedError), or alternatively change the
abandon_with_entry! to PaymentFailureReason::UnknownRequiredFeatures and keep
the current return); apply the change inside the same match arm so the
logged/recorded PaymentFailureReason and the Bolt12PaymentError returned to the
caller are identical.
In `@lightning/src/offers/flow.rs`:
- Around line 1701-1707: The payment_secret call to
inbound_payment::create_for_spontaneous_payment is still passing None for the
amount; replace that None with the resolved static-invoice amount so the
payment-secret constraints match the advertised concrete amount. Locate where
the static invoice amount is computed (the resolved/static invoice amount
variable used when building the static invoice) and pass that variable as the
second argument to create_for_spontaneous_payment instead of None, keeping the
existing expanded_key, offer_relative_expiry, and duration_since_epoch
arguments.
In `@lightning/src/offers/invoice_request.rs`:
- Around line 53-57: The doctest instantiates InvoiceRequestBuilder with only
two generics but the struct now declares four (<'a, 'b, T, CC>); update the
doctest example to use the correct arity — e.g., change InvoiceRequestBuilder<_,
_> to InvoiceRequestBuilder<'_, '_, _, _> (or explicit lifetimes/types matching
the surrounding example) so the helper line matches the pub struct
InvoiceRequestBuilder<'a, 'b, T: secp256k1::Signing, CC: CurrencyConversion>
declaration and the doctest compiles.
In `@lightning/src/offers/offer.rs`:
- Around line 360-367: The amount setter (pub fn amount<CC:
CurrencyConversion>(...)) currently converts Amount::Currency using
currency_conversion and enforces MAX_VALUE_MSAT, making setting fiat
rate-dependent; remove the conversion and MAX_VALUE_MSAT check from this method
so it only stores self.offer.amount = Some(amount) and returns as before, and
instead perform currency-to-msat conversion and MAX_VALUE_MSAT validation at
offer resolution time (e.g., in Offer::try_from or the invoice-request/invoice
creation code paths) so wire-parsed offers remain accepted regardless of the
local rate cache.
---
Nitpick comments:
In `@lightning/src/offers/invoice.rs`:
- Around line 1938-1953: Add a second test case that creates a fiat-denominated
offer to exercise resolve_offer_amount: construct an OfferBuilder (same
recipient_pubkey()) but set the offer price in a fiat currency (use the
TestCurrencyConversion test helper/instance) so the builder path that resolves
fiat amounts is taken, then call request_invoice(&expanded_key, nonce,
&secp_ctx, payment_id, &conversion) and follow with build_and_sign() and
respond_with_no_std(&conversion, payment_paths.clone(), payment_hash, now) as in
the existing msat case; assert the invoice amount is resolved correctly to msats
so the new CurrencyConversion plumbing is covered.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: bfe1b4cb-ab3c-4cb3-93ee-8296f8377903
📒 Files selected for processing (11)
fuzz/src/offer_deser.rslightning/src/ln/async_payments_tests.rslightning/src/ln/channelmanager.rslightning/src/ln/offers_tests.rslightning/src/ln/outbound_payment.rslightning/src/offers/flow.rslightning/src/offers/invoice.rslightning/src/offers/invoice_request.rslightning/src/offers/merkle.rslightning/src/offers/offer.rslightning/src/offers/refund.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- lightning/src/offers/merkle.rs
- lightning/src/offers/refund.rs
| let conversion = DefaultCurrencyConversion; | ||
|
|
||
| let mut builder = offer.request_invoice(&expanded_key, nonce, &secp_ctx, payment_id)?; | ||
|
|
||
| builder = match offer.amount() { | ||
| None => builder.amount_msats(1000).unwrap(), | ||
| Some(Amount::Bitcoin { amount_msats }) => builder.amount_msats(amount_msats + 1)?, | ||
| Some(Amount::Currency { .. }) => return Err(Bolt12SemanticError::UnsupportedCurrency), | ||
| Some(amount) => builder.amount_msats(amount.into_msats(&conversion)?)?, |
There was a problem hiding this comment.
Use a conversion that can actually resolve fiat amounts in this fuzzer.
DefaultCurrencyConversion rejects every fiat code, so amount.into_msats(&conversion)? still bails out on all Amount::Currency inputs. That means fiat-denominated offers never reach invoice-request serialization in this target, which leaves the new currency-aware path effectively unfuzzed here. A small fuzz-local conversion implementation would keep those cases in scope.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@fuzz/src/offer_deser.rs` around lines 52 - 58, The fuzzer uses
DefaultCurrencyConversion which rejects all fiat codes, so replace it with a
fuzz-local conversion that resolves fiat amounts (e.g., implement a
SimpleFuzzCurrencyConversion used in this file) and instantiate that instead of
DefaultCurrencyConversion; ensure the new type implements the same trait/trait
methods used by amount.into_msats(&conversion) (accept common ISO codes or
return a deterministic fixed-rate conversion) so Amount::Currency paths succeed
and reach offer.request_invoice/amount.into_msats rather than early-returning.
0b9310c to
2f5228c
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
lightning-background-processor/src/lib.rs (1)
381-407:⚠️ Potential issue | 🟠 MajorDon't hard-wire
DynChannelManagertoDefaultCurrencyConversion.
DefaultCurrencyConversionis the no-op implementation, so any app that provides a real exchange-rate source can no longer useNO_ONION_MESSENGER/NO_LIQUIDITY_MANAGER*once those helpers are typed through this alias. Using a trait object here preserves the convenience API for custom converters while keeping the default implementation available for callers that want it.Proposed fix
- type DynCurrencyConversion = lightning::offers::currency::DefaultCurrencyConversion; + type DynCurrencyConversion = + dyn lightning::offers::currency::CurrencyConversion + Send + Sync;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lightning-background-processor/src/lib.rs` around lines 381 - 407, The DynChannelManager alias is hard-wired to DefaultCurrencyConversion via the DynCurrencyConversion type alias; change DynCurrencyConversion (the alias used in DynChannelManager) from the concrete DefaultCurrencyConversion to a trait-object type so DynChannelManager uses &'static DynCurrencyConversion where DynCurrencyConversion = dyn lightning::offers::currency::CurrencyConversion + Send + Sync (under the same cfg guards), preserving the existing &'static DynCurrencyConversion reference in DynChannelManager and allowing callers to provide custom CurrencyConversion implementations.lightning/src/ln/outbound_payment.rs (1)
1283-1295:⚠️ Potential issue | 🟠 MajorUse the stored invoice-request amount here instead of re-resolving the offer.
retryable_invoice_request.invoice_requestis our own persisted request. CallingInvoiceBuilder::<DerivedSigningPubkey>::amount_msatshere re-runsresolve_offer_amount(currency_conversion), which makes async/static-invoice handling depend on a fresh FX lookup again. If the cache refreshes or the provider is temporarily unavailable between request creation and static-invoice receipt, a previously valid fiat request can fail here even though the concrete msat amount was already encoded in the request.💡 Suggested direction
- let amount_msat = match InvoiceBuilder::<DerivedSigningPubkey>::amount_msats( - invreq, - currency_conversion, - ) { + let amount_msat = match invreq.amount_msats(currency_conversion) {Based on learnings, the invoice request builder always resolves and encodes a concrete msat amount for fiat offers at construction time.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lightning/src/ln/outbound_payment.rs` around lines 1283 - 1295, The code re-computes the msat by calling InvoiceBuilder::<DerivedSigningPubkey>::amount_msats which triggers resolve_offer_amount(currency_conversion); instead, read the already-resolved concrete msat encoded in our persisted retryable_invoice_request.invoice_request and use that value (do not call amount_msats or resolve_offer_amount). Replace the match/Err branch that invokes InvoiceBuilder::amount_msats with logic that extracts the stored msat from retryable_invoice_request.invoice_request (the invoice_request field) and proceed, keeping the existing abandon_with_entry! handling only if the stored amount is missing or invalid.
♻️ Duplicate comments (1)
lightning/src/offers/offer.rs (1)
360-367:⚠️ Potential issue | 🟠 MajorDon’t make fiat offer creation depend on the current exchange rate.
This setter still converts
Amount::Currencyimmediately and rejects it when today’s rate resolves aboveMAX_VALUE_MSAT, butOffer::try_fromaccepts the same wire offer without any conversion. That makes locally built fiat offers rate-dependent in a way parsed offers are not. The limit check here should stay bitcoin-only and fiat amounts should be validated when the offer is actually resolved for an invoice request/invoice.💡 Directional fix
-pub fn amount<CC: CurrencyConversion>($($self_mut)* $self: $self_type, amount: Amount, currency_conversion: &CC) -> Result<$return_type, Bolt12SemanticError> +pub fn amount<CC: CurrencyConversion>($($self_mut)* $self: $self_type, amount: Amount, _currency_conversion: &CC) -> Result<$return_type, Bolt12SemanticError> { - if amount.into_msats(currency_conversion)? > MAX_VALUE_MSAT { - return Err(Bolt12SemanticError::InvalidAmount); + if let Amount::Bitcoin { amount_msats } = amount { + if amount_msats > MAX_VALUE_MSAT { + return Err(Bolt12SemanticError::InvalidAmount); + } } $self.offer.amount = Some(amount); Ok($return_value) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lightning/src/offers/offer.rs` around lines 360 - 367, The amount setter currently converts Amount::Currency to msats and rejects it if above MAX_VALUE_MSAT, making fiat offer creation rate-dependent; instead, in Offer::amount (the amount method) only perform the MAX_VALUE_MSAT check for Bitcoin/msat amounts and do not call amount.into_msats(currency_conversion) when amount is a fiat variant (Amount::Currency) — simply set self.offer.amount = Some(amount) and return Ok; leave fiat validation to be performed later when the offer is resolved for an invoice request/invoice (matching Offer::try_from behavior).
🧹 Nitpick comments (3)
fuzz/src/full_stack.rs (1)
189-199:FuzzCurrencyConversionstill prevents fiat offer paths from being exercised.
msats_per_minor_unitalways returningErr(())means any currency-denominated amount still bails out withUnsupportedCurrencyas soon as it hitsinto_msats. Since this harness now threads the converter throughChannelManager, a deterministic fixed rate would keep the new currency-aware paths reachable without sacrificing reproducibility.♻️ Minimal change
impl CurrencyConversion for FuzzCurrencyConversion { fn msats_per_minor_unit(&self, _iso4217_code: CurrencyCode) -> Result<u64, ()> { - Err(()) + Ok(1_000) } fn tolerance_percent(&self) -> u8 { 0 } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fuzz/src/full_stack.rs` around lines 189 - 199, FuzzCurrencyConversion currently causes all fiat paths to abort because msats_per_minor_unit always returns Err(()); change msats_per_minor_unit in the FuzzCurrencyConversion impl to return a deterministic Ok(u64) fixed conversion rate (e.g., a constant like 1000) so currency-denominated amounts can be converted and exercised in tests, and keep tolerance_percent as a stable small value (0 or another fixed u8) to preserve reproducibility; update references to CurrencyConversion/msats_per_minor_unit and ensure this deterministic converter is the one threaded into ChannelManager.lightning/src/ln/offers_tests.rs (1)
924-990: Add the async/static-invoice fiat scenario too.This test covers the regular offer → invoice_request → invoice flow, but the new currency-aware path in
OutboundPayments::static_invoice_receivedis only exercised by async/static-invoice handling. A companion fiat test there would catch regressions in the new conversion plumbing.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lightning/src/ln/offers_tests.rs` around lines 924 - 990, Add a companion test that mirrors creates_and_pays_for_offer_with_fiat_amount_using_one_hop_blinded_path but exercises the async/static-invoice path so OutboundPayments::static_invoice_received is exercised: create an offer with a fiat Amount (same as amount setup), have bob initiate payment using the async/static-invoice flow (the code path that produces a static invoice rather than the synchronous invoice_request→invoice exchange), then route and claim the payment similarly (reusing route_bolt12_payment and claim_bolt12_payment helpers) and assert the same currency-conversion results (invoice amount_msats, signing_pubkey checks, reply_path dummy-hop checks) so the new static_invoice_received conversion plumbing is covered. Ensure the new test function name clearly indicates it’s the static/async fiat variant so it’s easy to find alongside creates_and_pays_for_offer_with_fiat_amount_using_one_hop_blinded_path.lightning/src/ln/functional_test_utils.rs (1)
903-919: Reuse the node’s configured converter in this reload sanity-check.This is the one remaining deserialization path that still hard-codes a fresh
TestCurrencyConversioninstead of usingself.currency_conversion. If tests ever swap in a non-default or stateful converter, this round-trip will be validating a different configuration than the liveNode.♻️ Minimal fix
message_router: &test_utils::TestMessageRouter::new_default( network_graph, self.keys_manager, ), - currency_conversion: &test_utils::TestCurrencyConversion, + currency_conversion: self.currency_conversion, chain_monitor: self.chain_monitor,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lightning/src/ln/functional_test_utils.rs` around lines 903 - 919, The reload sanity-check constructs ChannelManagerReadArgs using a hard-coded test converter (test_utils::TestCurrencyConversion) instead of the node's actual converter; update that ChannelManagerReadArgs field to use self.currency_conversion so the deserialization round-trip uses the same converter as the live Node (replace the TestCurrencyConversion reference with self.currency_conversion in the ChannelManagerReadArgs construction).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@lightning-background-processor/src/lib.rs`:
- Around line 381-407: The DynChannelManager alias is hard-wired to
DefaultCurrencyConversion via the DynCurrencyConversion type alias; change
DynCurrencyConversion (the alias used in DynChannelManager) from the concrete
DefaultCurrencyConversion to a trait-object type so DynChannelManager uses
&'static DynCurrencyConversion where DynCurrencyConversion = dyn
lightning::offers::currency::CurrencyConversion + Send + Sync (under the same
cfg guards), preserving the existing &'static DynCurrencyConversion reference in
DynChannelManager and allowing callers to provide custom CurrencyConversion
implementations.
In `@lightning/src/ln/outbound_payment.rs`:
- Around line 1283-1295: The code re-computes the msat by calling
InvoiceBuilder::<DerivedSigningPubkey>::amount_msats which triggers
resolve_offer_amount(currency_conversion); instead, read the already-resolved
concrete msat encoded in our persisted retryable_invoice_request.invoice_request
and use that value (do not call amount_msats or resolve_offer_amount). Replace
the match/Err branch that invokes InvoiceBuilder::amount_msats with logic that
extracts the stored msat from retryable_invoice_request.invoice_request (the
invoice_request field) and proceed, keeping the existing abandon_with_entry!
handling only if the stored amount is missing or invalid.
---
Duplicate comments:
In `@lightning/src/offers/offer.rs`:
- Around line 360-367: The amount setter currently converts Amount::Currency to
msats and rejects it if above MAX_VALUE_MSAT, making fiat offer creation
rate-dependent; instead, in Offer::amount (the amount method) only perform the
MAX_VALUE_MSAT check for Bitcoin/msat amounts and do not call
amount.into_msats(currency_conversion) when amount is a fiat variant
(Amount::Currency) — simply set self.offer.amount = Some(amount) and return Ok;
leave fiat validation to be performed later when the offer is resolved for an
invoice request/invoice (matching Offer::try_from behavior).
---
Nitpick comments:
In `@fuzz/src/full_stack.rs`:
- Around line 189-199: FuzzCurrencyConversion currently causes all fiat paths to
abort because msats_per_minor_unit always returns Err(()); change
msats_per_minor_unit in the FuzzCurrencyConversion impl to return a
deterministic Ok(u64) fixed conversion rate (e.g., a constant like 1000) so
currency-denominated amounts can be converted and exercised in tests, and keep
tolerance_percent as a stable small value (0 or another fixed u8) to preserve
reproducibility; update references to CurrencyConversion/msats_per_minor_unit
and ensure this deterministic converter is the one threaded into ChannelManager.
In `@lightning/src/ln/functional_test_utils.rs`:
- Around line 903-919: The reload sanity-check constructs ChannelManagerReadArgs
using a hard-coded test converter (test_utils::TestCurrencyConversion) instead
of the node's actual converter; update that ChannelManagerReadArgs field to use
self.currency_conversion so the deserialization round-trip uses the same
converter as the live Node (replace the TestCurrencyConversion reference with
self.currency_conversion in the ChannelManagerReadArgs construction).
In `@lightning/src/ln/offers_tests.rs`:
- Around line 924-990: Add a companion test that mirrors
creates_and_pays_for_offer_with_fiat_amount_using_one_hop_blinded_path but
exercises the async/static-invoice path so
OutboundPayments::static_invoice_received is exercised: create an offer with a
fiat Amount (same as amount setup), have bob initiate payment using the
async/static-invoice flow (the code path that produces a static invoice rather
than the synchronous invoice_request→invoice exchange), then route and claim the
payment similarly (reusing route_bolt12_payment and claim_bolt12_payment
helpers) and assert the same currency-conversion results (invoice amount_msats,
signing_pubkey checks, reply_path dummy-hop checks) so the new
static_invoice_received conversion plumbing is covered. Ensure the new test
function name clearly indicates it’s the static/async fiat variant so it’s easy
to find alongside
creates_and_pays_for_offer_with_fiat_amount_using_one_hop_blinded_path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2c5bf53f-45b5-43ea-ad5c-fc2b237ae997
📒 Files selected for processing (22)
fuzz/src/full_stack.rsfuzz/src/offer_deser.rslightning-background-processor/src/lib.rslightning-block-sync/src/init.rslightning-dns-resolver/src/lib.rslightning/src/ln/async_payments_tests.rslightning/src/ln/channelmanager.rslightning/src/ln/functional_test_utils.rslightning/src/ln/functional_tests.rslightning/src/ln/max_payment_path_len_tests.rslightning/src/ln/offers_tests.rslightning/src/ln/outbound_payment.rslightning/src/ln/reload_tests.rslightning/src/offers/flow.rslightning/src/offers/invoice.rslightning/src/offers/invoice_request.rslightning/src/offers/merkle.rslightning/src/offers/offer.rslightning/src/offers/refund.rslightning/src/offers/static_invoice.rslightning/src/offers/test_utils.rslightning/src/util/test_utils.rs
🚧 Files skipped from review as they are similar to previous changes (6)
- lightning-dns-resolver/src/lib.rs
- lightning/src/ln/functional_tests.rs
- lightning/src/util/test_utils.rs
- lightning/src/offers/test_utils.rs
- lightning-block-sync/src/init.rs
- lightning/src/offers/refund.rs
shaavan
force-pushed
the
currency-review
branch
from
2f5228c to
8d617d0
Compare
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
fuzz/src/invoice_request_deser.rs (1)
67-75:⚠️ Potential issue | 🟡 MinorAvoid panic stubs in fuzz conversion implementations.
On Line 69 and Line 73,
unreachable!()makes this harness brittle if these paths are exercised. Prefer returning graceful defaults (Err(())/0) as in the full-stack fuzz target.Suggested patch
impl CurrencyConversion for FuzzCurrencyConversion { fn msats_per_minor_unit(&self, _iso4217_code: CurrencyCode) -> Result<u64, ()> { - unreachable!() + Err(()) } fn tolerance_percent(&self) -> u8 { - unreachable!() + 0 } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fuzz/src/invoice_request_deser.rs` around lines 67 - 75, In impl CurrencyConversion for FuzzCurrencyConversion, avoid panicking in msats_per_minor_unit and tolerance_percent: replace the unreachable!() stub in msats_per_minor_unit(&self, _iso4217_code: CurrencyCode) with a graceful Err(()) return, and replace the unreachable!() in tolerance_percent(&self) with a safe default value (0) so the fuzz harness won't panic if those code paths are exercised.fuzz/src/offer_deser.rs (1)
15-18:⚠️ Potential issue | 🟡 MinorFiat-denominated offers still short-circuit out of this fuzz target.
DefaultCurrencyConversionrejects every fiat code, soamount.into_msats(&conversion)?still returnsUnsupportedCurrencyfor everyAmount::Currency. That means the new fiat-resolution path never reachesbuilder.amount_msats(...)here.🧪 One way to keep fiat inputs in scope
-use lightning::offers::currency::DefaultCurrencyConversion; +use lightning::offers::currency::{CurrencyCode, CurrencyConversion}; use lightning::offers::invoice_request::InvoiceRequest; use lightning::offers::nonce::Nonce; use lightning::offers::offer::{Offer, Quantity}; @@ +struct FuzzCurrencyConversion; + +impl CurrencyConversion for FuzzCurrencyConversion { + fn msats_per_minor_unit(&self, _iso4217_code: CurrencyCode) -> Result<u64, ()> { + Ok(1_000) + } + + fn tolerance_percent(&self) -> u8 { + 0 + } +} + fn build_request(offer: &Offer) -> Result<InvoiceRequest, Bolt12SemanticError> { @@ - let conversion = DefaultCurrencyConversion; + let conversion = FuzzCurrencyConversion;Also applies to: 52-58
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fuzz/src/offer_deser.rs` around lines 15 - 18, The fuzz target uses DefaultCurrencyConversion which rejects fiat so amount.into_msats(&conversion) always errors and never hits builder.amount_msats; replace DefaultCurrencyConversion with a conversion that accepts fiat (e.g., a test/mock conversion implementation or a wrapper that maps fiat codes to msat rates) and pass that to amount.into_msats(&conversion) so Fiat Amount::Currency path proceeds to builder.amount_msats; apply the same replacement where the conversion is used in the other occurrence (around the second block that currently uses DefaultCurrencyConversion).lightning/src/offers/invoice_request.rs (2)
53-58:⚠️ Potential issue | 🟡 MinorFix the hidden doctest’s
InvoiceRequestBuildergeneric arity.
InvoiceRequestBuildernow takes four generic parameters, soInvoiceRequestBuilder<_, _>no longer matches the type and this example will stop compiling.🛠️ Minimal doc fix
-//! # <InvoiceRequestBuilder<_, _>>::from( +//! # <InvoiceRequestBuilder<'_, '_, _, _>>::from(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lightning/src/offers/invoice_request.rs` around lines 53 - 58, The hidden doctest uses the wrong generic arity for InvoiceRequestBuilder (currently written as InvoiceRequestBuilder<_, _>); update the example to the new four-type-parameter form (e.g. InvoiceRequestBuilder<_, _, _, _>) or otherwise remove explicit generics so type inference applies, ensuring the InvoiceRequestBuilder::from(...) doctest compiles with the current InvoiceRequestBuilder signature.
1494-1500:⚠️ Potential issue | 🔴 CriticalDon't drop the deferred amount validation when
UnsupportedCurrencyis ignored.Lines 1494-1500 now let fiat-denominated requests parse without a converter, but that also skips the only check that compares an explicit
invoice_request.amountagainst the offer’s converted minimum andMAX_VALUE_MSAT. A parsed fiat request withamountalready set can therefore survive with an under-minimum or over-max msat value unless the validation is replayed later with the real converter.One straightforward place to finish that deferred check is
InvoiceRequestContents::amount_msats():🛠️ Suggested follow-up
pub(super) fn amount_msats<CC: CurrencyConversion>( &self, currency_conversion: &CC, ) -> Result<u64, Bolt12SemanticError> { match self.inner.amount_msats() { - Some(msats) => Ok(msats), + Some(msats) => { + self.inner.offer.check_amount_msats_for_quantity( + currency_conversion, + Some(msats), + self.quantity(), + )?; + Ok(msats) + }, None => { let unit_msats = self .inner .offer .resolve_offer_amount(currency_conversion)?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lightning/src/offers/invoice_request.rs` around lines 1494 - 1500, The code currently ignores Bolt12SemanticError::UnsupportedCurrency from offer.check_amount_msats_for_quantity, which defers validation and can allow an explicit InvoiceRequest.amount to bypass min/MAX checks; to fix, in InvoiceRequestContents::amount_msats() detect when the request has an explicit amount and the offer is currency-denominated (i.e., the case that previously returned UnsupportedCurrency) and perform the concrete conversion plus validation there: call the currency conversion to get msats, then compare against the offer's converted minimum and MAX_VALUE_MSAT and return the appropriate Bolt12SemanticError on under-minimum or overflow; ensure you keep the existing behavior for missing amounts and reuse the same error variants used by check_amount_msats_for_quantity.
🧹 Nitpick comments (2)
lightning/src/offers/currency.rs (1)
18-39: Document the stability requirement onCurrencyConversion.The units are clear, but the public trait contract still needs the stronger behavior the offer flow relies on: implementations should be synchronous/cache-backed and return stable rates across consecutive calls in one payment flow. Without that spelled out here, downstream implementations can fetch fresh rates per call and make amount resolution internally inconsistent.
📝 Suggested contract wording
/// This convention ensures amounts remain precise and purely integer-based when parsing and /// validating BOLT12 invoice requests. +/// +/// Implementations are expected to be synchronous/cache-backed and to return +/// stable values across consecutive calls within a single offer or invoice +/// flow. Fetching a fresh live rate for each call can make amount resolution +/// and tolerance checks internally inconsistent. pub trait CurrencyConversion {Based on learnings:
CurrencyConversionimplementations inlightning/src/offers/*.rsare expected to be synchronous, cache-backed, and stable across consecutive calls within one payment flow.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lightning/src/offers/currency.rs` around lines 18 - 39, Add a sentence to the CurrencyConversion trait docs that mandates implementations of CurrencyConversion (and its methods msats_per_minor_unit and tolerance_percent) be synchronous, cache-backed, and return stable, repeatable rates across consecutive calls within a single payment flow; state that callers rely on stability for amount resolution and that implementations must not perform fresh network fetches per call but may update caches between flows.lightning/src/ln/offers_tests.rs (1)
2443-2444: UseDefaultCurrencyConversionon this explicit-amount path.This
InvoiceRequestalready has a concrete msat amount, so usingTestCurrencyConversionhere would let an accidental fiat lookup still succeed. Using the default converter would make the regression fail ifrespond_using_derived_keys_no_stdstops honoring the explicit-amount fast path.🔍 Sharpen the regression
- let conversion = TestCurrencyConversion; + let conversion = crate::offers::currency::DefaultCurrencyConversion;Based on learnings: invoice requests with an explicitly encoded amount should never need the fiat-resolution fallback, so
DefaultCurrencyConversionis safe on that path.Also applies to: 2477-2479
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lightning/src/ln/offers_tests.rs` around lines 2443 - 2444, Replace the use of TestCurrencyConversion with DefaultCurrencyConversion on the explicit-amount offer payment paths: locate where TestCurrencyConversion is instantiated for the explicit-msat InvoiceRequest (the call site to david.node.pay_for_offer(&offer, None, payment_id, Default::default()) and the similar instances around the other mentions at the block near lines referenced in the review), and construct/pass DefaultCurrencyConversion instead of TestCurrencyConversion so the explicit-amount fast path cannot fall back to fiat resolution; ensure you update all occurrences called in the explicit-amount tests (including the mentions around the 2477-2479 area) to use DefaultCurrencyConversion.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lightning/src/offers/offer.rs`:
- Around line 348-354: The builder setters (e.g., amount_msats and the amount
setter that accepts Amount::Currency) currently only enforce the upper bound and
allow zero values which later cause OfferContents::try_from to reject the Offer
as InvalidAmount; update these setters (reference: amount_msats and the
Amount-taking setter) to also reject zero by returning
Err(Bolt12SemanticError::InvalidAmount) when amount_msats == 0 (and when
Amount::Currency.amount == 0), so the builder enforces the same non-zero
invariant as the parser.
---
Duplicate comments:
In `@fuzz/src/invoice_request_deser.rs`:
- Around line 67-75: In impl CurrencyConversion for FuzzCurrencyConversion,
avoid panicking in msats_per_minor_unit and tolerance_percent: replace the
unreachable!() stub in msats_per_minor_unit(&self, _iso4217_code: CurrencyCode)
with a graceful Err(()) return, and replace the unreachable!() in
tolerance_percent(&self) with a safe default value (0) so the fuzz harness won't
panic if those code paths are exercised.
In `@fuzz/src/offer_deser.rs`:
- Around line 15-18: The fuzz target uses DefaultCurrencyConversion which
rejects fiat so amount.into_msats(&conversion) always errors and never hits
builder.amount_msats; replace DefaultCurrencyConversion with a conversion that
accepts fiat (e.g., a test/mock conversion implementation or a wrapper that maps
fiat codes to msat rates) and pass that to amount.into_msats(&conversion) so
Fiat Amount::Currency path proceeds to builder.amount_msats; apply the same
replacement where the conversion is used in the other occurrence (around the
second block that currently uses DefaultCurrencyConversion).
In `@lightning/src/offers/invoice_request.rs`:
- Around line 53-58: The hidden doctest uses the wrong generic arity for
InvoiceRequestBuilder (currently written as InvoiceRequestBuilder<_, _>); update
the example to the new four-type-parameter form (e.g. InvoiceRequestBuilder<_,
_, _, _>) or otherwise remove explicit generics so type inference applies,
ensuring the InvoiceRequestBuilder::from(...) doctest compiles with the current
InvoiceRequestBuilder signature.
- Around line 1494-1500: The code currently ignores
Bolt12SemanticError::UnsupportedCurrency from
offer.check_amount_msats_for_quantity, which defers validation and can allow an
explicit InvoiceRequest.amount to bypass min/MAX checks; to fix, in
InvoiceRequestContents::amount_msats() detect when the request has an explicit
amount and the offer is currency-denominated (i.e., the case that previously
returned UnsupportedCurrency) and perform the concrete conversion plus
validation there: call the currency conversion to get msats, then compare
against the offer's converted minimum and MAX_VALUE_MSAT and return the
appropriate Bolt12SemanticError on under-minimum or overflow; ensure you keep
the existing behavior for missing amounts and reuse the same error variants used
by check_amount_msats_for_quantity.
---
Nitpick comments:
In `@lightning/src/ln/offers_tests.rs`:
- Around line 2443-2444: Replace the use of TestCurrencyConversion with
DefaultCurrencyConversion on the explicit-amount offer payment paths: locate
where TestCurrencyConversion is instantiated for the explicit-msat
InvoiceRequest (the call site to david.node.pay_for_offer(&offer, None,
payment_id, Default::default()) and the similar instances around the other
mentions at the block near lines referenced in the review), and construct/pass
DefaultCurrencyConversion instead of TestCurrencyConversion so the
explicit-amount fast path cannot fall back to fiat resolution; ensure you update
all occurrences called in the explicit-amount tests (including the mentions
around the 2477-2479 area) to use DefaultCurrencyConversion.
In `@lightning/src/offers/currency.rs`:
- Around line 18-39: Add a sentence to the CurrencyConversion trait docs that
mandates implementations of CurrencyConversion (and its methods
msats_per_minor_unit and tolerance_percent) be synchronous, cache-backed, and
return stable, repeatable rates across consecutive calls within a single payment
flow; state that callers rely on stability for amount resolution and that
implementations must not perform fresh network fetches per call but may update
caches between flows.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4fca6f5b-c066-4880-9c29-30f6140725ef
📒 Files selected for processing (25)
fuzz/src/full_stack.rsfuzz/src/invoice_request_deser.rsfuzz/src/offer_deser.rslightning-background-processor/src/lib.rslightning-block-sync/src/init.rslightning-dns-resolver/src/lib.rslightning/src/ln/async_payments_tests.rslightning/src/ln/channelmanager.rslightning/src/ln/functional_test_utils.rslightning/src/ln/functional_tests.rslightning/src/ln/max_payment_path_len_tests.rslightning/src/ln/offers_tests.rslightning/src/ln/outbound_payment.rslightning/src/ln/reload_tests.rslightning/src/offers/currency.rslightning/src/offers/flow.rslightning/src/offers/invoice.rslightning/src/offers/invoice_request.rslightning/src/offers/merkle.rslightning/src/offers/mod.rslightning/src/offers/offer.rslightning/src/offers/refund.rslightning/src/offers/static_invoice.rslightning/src/offers/test_utils.rslightning/src/util/test_utils.rs
shaavan
force-pushed
the
currency-review
branch
2 times, most recently
from
fc2db89 to
03b14d4
Compare
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
lightning/src/offers/offer.rs (1)
360-365:⚠️ Potential issue | 🟠 MajorKeep fiat offer construction independent of the current converter.
OfferContents::try_fromstill accepts any positiveAmount::Currency, but this setter now resolves the currency immediately and rejects otherwise wire-valid offers when the local converter lacks that currency or today's rate converts aboveMAX_VALUE_MSAT. That makes long-lived offer creation environment-dependent. Keep the zero/native-bitcoin checks here, but defer converter-based limit checks toresolve_offer_amount/ invoice-request resolution.Minimal fix
-pub fn amount<CC: CurrencyConversion>($($self_mut)* $self: $self_type, amount: Amount, currency_conversion: &CC) -> Result<$return_type, Bolt12SemanticError> +pub fn amount<CC: CurrencyConversion>($($self_mut)* $self: $self_type, amount: Amount, _currency_conversion: &CC) -> Result<$return_type, Bolt12SemanticError> { - let amount_msats = amount.into_msats(currency_conversion)?; - if amount_msats == 0 || amount_msats > MAX_VALUE_MSAT { - return Err(Bolt12SemanticError::InvalidAmount); + match amount { + Amount::Bitcoin { amount_msats } if amount_msats == 0 || amount_msats > MAX_VALUE_MSAT => { + return Err(Bolt12SemanticError::InvalidAmount); + }, + Amount::Currency { amount: 0, .. } => { + return Err(Bolt12SemanticError::InvalidAmount); + }, + _ => {} } $self.offer.amount = Some(amount); Ok($return_value) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lightning/src/offers/offer.rs` around lines 360 - 365, The amount setter (pub fn amount<CC: CurrencyConversion>... -> Result...) currently converts fiat via currency_conversion and rejects offers whose converted msats are 0 or > MAX_VALUE_MSAT, making OfferContents::try_from environment-dependent; change amount so it only enforces zero/native-bitcoin limits (keep the zero check and any checks for Amount::Bitcoin against MAX_VALUE_MSAT) and do not call into CurrencyConversion or reject based on converted fiat values here; move any conversion-based upper-limit checks into resolve_offer_amount / invoice-request resolution so fiat offers remain wire-valid independent of the local converter.lightning/src/ln/outbound_payment.rs (1)
1288-1293:⚠️ Potential issue | 🟡 MinorDon't surface amount-resolution failures as
UnknownRequiredFeatures.
InvoiceBuilder::amount_msats(...)now propagates converter/semantic errors, but this arm still recordsPaymentFailureReason::UnexpectedErrorand returnsBolt12PaymentError::UnknownRequiredFeatures. If amount resolution fails here, callers get the wrong root cause. If this branch is meant to stay defensive-only, make it explicitly unreachable; otherwise map it to a matching outward error.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lightning/src/ln/outbound_payment.rs` around lines 1288 - 1293, The Err(_) arm in the amount-resolution branch incorrectly returns Bolt12PaymentError::UnknownRequiredFeatures and records PaymentFailureReason::UnexpectedError; change this to surface the real amount-resolution failure from InvoiceBuilder::amount_msats by mapping Err(e) to a dedicated outward error (e.g., Bolt12PaymentError::AmountResolutionFailed or a converter/semantic error variant) and set the payment entry failure to a matching PaymentFailureReason (instead of UnexpectedError); alternatively, if you truly believe this code path is unreachable, replace the branch with a debug_assert!(false) and an unreachable!() to make it explicitly defensive rather than returning the wrong error.lightning/src/offers/flow.rs (1)
869-870:⚠️ Potential issue | 🟠 MajorKeep the static-invoice payment secret aligned with the resolved amount.
This now resolves a concrete
amount_msat, butcreate_static_invoice_for_serverstill callsinbound_payment::create_for_spontaneous_payment(..., None, ...). That leaves the static-invoice / blinded-path amount constraints out of sync with the payment secret for amountful async offers.🐛 Suggested fix
let amount_msat = offer.resolve_offer_amount(&self.currency_conversion).map_err(|_| ())?; let payment_secret = inbound_payment::create_for_spontaneous_payment( expanded_key, amount_msat, offer_relative_expiry, duration_since_epoch.as_secs(), None, )?;Based on learnings: In the offers module, the CurrencyConversion trait should be implemented as synchronous and cache-backed. Exchange rates must remain consistent across multiple consecutive calls within a single payment flow.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lightning/src/offers/flow.rs` around lines 869 - 870, The code resolves a concrete amount_msat via offer.resolve_offer_amount(&self.currency_conversion) but then calls inbound_payment::create_for_spontaneous_payment with None for the amount, leaving the static-invoice payment secret out of sync; in create_static_invoice_for_server replace the None amount argument with the resolved amount_msat (propagate any needed map_err or error conversion so resolve_offer_amount failures are handled) so the payment_secret is created using the same amount_msat used earlier, and ensure you reference amount_msat, resolve_offer_amount, create_static_invoice_for_server, and inbound_payment::create_for_spontaneous_payment when making the change.lightning/src/offers/invoice_request.rs (2)
53-58:⚠️ Potential issue | 🟡 MinorFix the doctest's
InvoiceRequestBuildergeneric arity.Line 54 still instantiates
InvoiceRequestBuilderwith two generic arguments, but the struct now has four ('a,'b,T,CC). The hidden doctest won't compile as written.🛠️ Suggested fix
-//! # <InvoiceRequestBuilder<_, _>>::from( +//! # <InvoiceRequestBuilder<'_, '_, _, _>>::from(Verify by comparing the doctest line with the struct definition:
#!/bin/bash set -euo pipefail rg -n -C1 "<InvoiceRequestBuilder<" lightning/src/offers/invoice_request.rs rg -n -C2 "pub struct InvoiceRequestBuilder<'a, 'b, T: secp256k1::Signing, CC: CurrencyConversion>" lightning/src/offers/invoice_request.rs🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lightning/src/offers/invoice_request.rs` around lines 53 - 58, The doctest instantiates InvoiceRequestBuilder with only two generics but the struct now requires four; update the doctest to use all four type parameters (InvoiceRequestBuilder<'a, 'b, T, CC>) matching the struct signature (T: secp256k1::Signing, CC: CurrencyConversion) so the hidden doctest compiles—locate the instantiation line referencing InvoiceRequestBuilder and replace the two-parameter form with the full four-parameter form (including the same trait-bound types used by the struct definition).
289-293:⚠️ Potential issue | 🟠 MajorPersist the resolved fiat amount before signing the request.
Line 289 validates the effective total against
currency_conversion, but ifamount_msatsis stillNoneyou serialize the request unchanged on Line 293. That leaves a checked currency-denominated request rate/cache-dependent: the same signed request can later resolve to a different msat amount, or fail if conversion support changes. Please materialize the resolved total intoself.invoice_request.amount_msatsbeforebuild_without_checks().🛠️ Suggested fix
$self.invoice_request.offer.check_amount_msats_for_quantity( $self.currency_conversion, $self.invoice_request.amount_msats, $self.invoice_request.quantity )?; + if $self.invoice_request.amount_msats.is_none() && $self.offer.is_currency_denominated() { + let quantity = $self.invoice_request.quantity.unwrap_or(1); + let unit_msats = $self + .invoice_request + .offer + .resolve_offer_amount($self.currency_conversion)? + .ok_or(Bolt12SemanticError::MissingAmount)?; + $self.invoice_request.amount_msats = Some( + unit_msats + .checked_mul(quantity) + .filter(|msats| *msats <= MAX_VALUE_MSAT) + .ok_or(Bolt12SemanticError::InvalidAmount)?, + ); + } Ok($self.build_without_checks())Based on learnings: the invoice request builder is expected to resolve and encode a concrete msat amount for fiat offers at construction time.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lightning/src/offers/invoice_request.rs` around lines 289 - 293, The code validates a fiat-denominated total but never stores the resolved msat amount, so the signed request can later re-resolve differently; after calling check_amount_msats_for_quantity (or by invoking the offer/currency conversion helper that computes the concrete msat total for the given quantity), assign that resolved value into self.invoice_request.amount_msats = Some(resolved_msats) before calling self.build_without_checks(), ensuring the builder serializes the concrete msat amount rather than leaving it None.
🧹 Nitpick comments (2)
lightning/src/offers/currency.rs (1)
18-34: Use a named error enum forCurrencyConversionto avoid freezing unit error into public API.This is new public API.
Result<(f64, u8), ()>prevents callers from distinguishing unsupported currencies from unavailable or invalid rates, and changing that later is breaking. The trait's documentation does not explain error semantics, and call sites uniformly map all errors toBolt12SemanticError::UnsupportedCurrency, indicating semantic distinctions should be explicit. I'd strongly recommend a small error enum here and an explicit contract that returned rates are finite and positive.♻️ Proposed API shape
+#[derive(Clone, Copy, Debug, PartialEq, Eq)] +pub enum CurrencyConversionError { + UnsupportedCurrency, + RateUnavailable, + InvalidRate, +} + pub trait CurrencyConversion { /// Returns the conversion rate in **msats per minor unit** for the given /// ISO-4217 currency code together with the acceptable tolerance, expressed /// as a percentage of the resolved msat amount, used when deriving conversion /// ranges. - fn msats_per_minor_unit(&self, iso4217_code: CurrencyCode) -> Result<(f64, u8), ()>; + /// + /// Implementations must return a finite, positive rate. + fn msats_per_minor_unit( + &self, iso4217_code: CurrencyCode + ) -> Result<(f64, u8), CurrencyConversionError>; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lightning/src/offers/currency.rs` around lines 18 - 34, The trait CurrencyConversion's method msats_per_minor_unit currently returns Result<(f64, u8), ()>, which freezes an uninformative unit error into the public API; replace that with a small named error enum (e.g., CurrencyConversionError with variants like UnsupportedCurrency, RateUnavailable, InvalidRate) and change the signature of CurrencyConversion::msats_per_minor_unit to return Result<(f64, u8), CurrencyConversionError>; update the trait docs to explicitly require the returned rate be finite and >0 and document each error variant's meaning so callers (and existing mappings to Bolt12SemanticError::UnsupportedCurrency) can distinguish unsupported currencies from unavailable/invalid rates.lightning/src/ln/channelmanager.rs (1)
1900-1900: Consider documentingDefaultCurrencyConversionlimitations for simple type aliases.Users of
SimpleArcChannelManagerandSimpleRefChannelManagerwill not be able to process offers denominated in fiat currencies (e.g.,Amount::Currency) sinceDefaultCurrencyConversionalways returnsErr(()). This is likely intentional for simple use cases, but users may be surprised when currency-denominated offers fail silently.Consider adding a doc comment on these type aliases noting this limitation, or documenting it in the module-level docs.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lightning/src/ln/channelmanager.rs` at line 1900, Add a doc comment to the SimpleArcChannelManager and SimpleRefChannelManager type aliases (or the surrounding module docs) that states DefaultCurrencyConversion always returns Err(()) and therefore offers denominated as fiat (e.g., Amount::Currency) are not supported by these simple managers; explicitly mention the limitation and recommend using a custom CurrencyConversion implementation if fiat support is required.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lightning/src/offers/offer.rs`:
- Around line 1182-1191: The use of f64::round() on the expression
(msats_per_minor_unit * amount as f64) is not no_std-safe; replace the call to
the inherent method with libm::round() instead: compute the product
(msats_per_minor_unit * amount as f64) and pass that to libm::round(...) to
produce amount_msats, keeping the subsequent is_finite check and the
u64::try_from(...) conversion unchanged; update the file to reference
libm::round where msats_per_minor_unit and amount_msats are handled (the block
using currency_conversion.msats_per_minor_unit and Bolt12SemanticError).
---
Duplicate comments:
In `@lightning/src/ln/outbound_payment.rs`:
- Around line 1288-1293: The Err(_) arm in the amount-resolution branch
incorrectly returns Bolt12PaymentError::UnknownRequiredFeatures and records
PaymentFailureReason::UnexpectedError; change this to surface the real
amount-resolution failure from InvoiceBuilder::amount_msats by mapping Err(e) to
a dedicated outward error (e.g., Bolt12PaymentError::AmountResolutionFailed or a
converter/semantic error variant) and set the payment entry failure to a
matching PaymentFailureReason (instead of UnexpectedError); alternatively, if
you truly believe this code path is unreachable, replace the branch with a
debug_assert!(false) and an unreachable!() to make it explicitly defensive
rather than returning the wrong error.
In `@lightning/src/offers/flow.rs`:
- Around line 869-870: The code resolves a concrete amount_msat via
offer.resolve_offer_amount(&self.currency_conversion) but then calls
inbound_payment::create_for_spontaneous_payment with None for the amount,
leaving the static-invoice payment secret out of sync; in
create_static_invoice_for_server replace the None amount argument with the
resolved amount_msat (propagate any needed map_err or error conversion so
resolve_offer_amount failures are handled) so the payment_secret is created
using the same amount_msat used earlier, and ensure you reference amount_msat,
resolve_offer_amount, create_static_invoice_for_server, and
inbound_payment::create_for_spontaneous_payment when making the change.
In `@lightning/src/offers/invoice_request.rs`:
- Around line 53-58: The doctest instantiates InvoiceRequestBuilder with only
two generics but the struct now requires four; update the doctest to use all
four type parameters (InvoiceRequestBuilder<'a, 'b, T, CC>) matching the struct
signature (T: secp256k1::Signing, CC: CurrencyConversion) so the hidden doctest
compiles—locate the instantiation line referencing InvoiceRequestBuilder and
replace the two-parameter form with the full four-parameter form (including the
same trait-bound types used by the struct definition).
- Around line 289-293: The code validates a fiat-denominated total but never
stores the resolved msat amount, so the signed request can later re-resolve
differently; after calling check_amount_msats_for_quantity (or by invoking the
offer/currency conversion helper that computes the concrete msat total for the
given quantity), assign that resolved value into
self.invoice_request.amount_msats = Some(resolved_msats) before calling
self.build_without_checks(), ensuring the builder serializes the concrete msat
amount rather than leaving it None.
In `@lightning/src/offers/offer.rs`:
- Around line 360-365: The amount setter (pub fn amount<CC:
CurrencyConversion>... -> Result...) currently converts fiat via
currency_conversion and rejects offers whose converted msats are 0 or >
MAX_VALUE_MSAT, making OfferContents::try_from environment-dependent; change
amount so it only enforces zero/native-bitcoin limits (keep the zero check and
any checks for Amount::Bitcoin against MAX_VALUE_MSAT) and do not call into
CurrencyConversion or reject based on converted fiat values here; move any
conversion-based upper-limit checks into resolve_offer_amount / invoice-request
resolution so fiat offers remain wire-valid independent of the local converter.
---
Nitpick comments:
In `@lightning/src/ln/channelmanager.rs`:
- Line 1900: Add a doc comment to the SimpleArcChannelManager and
SimpleRefChannelManager type aliases (or the surrounding module docs) that
states DefaultCurrencyConversion always returns Err(()) and therefore offers
denominated as fiat (e.g., Amount::Currency) are not supported by these simple
managers; explicitly mention the limitation and recommend using a custom
CurrencyConversion implementation if fiat support is required.
In `@lightning/src/offers/currency.rs`:
- Around line 18-34: The trait CurrencyConversion's method msats_per_minor_unit
currently returns Result<(f64, u8), ()>, which freezes an uninformative unit
error into the public API; replace that with a small named error enum (e.g.,
CurrencyConversionError with variants like UnsupportedCurrency, RateUnavailable,
InvalidRate) and change the signature of
CurrencyConversion::msats_per_minor_unit to return Result<(f64, u8),
CurrencyConversionError>; update the trait docs to explicitly require the
returned rate be finite and >0 and document each error variant's meaning so
callers (and existing mappings to Bolt12SemanticError::UnsupportedCurrency) can
distinguish unsupported currencies from unavailable/invalid rates.
🪄 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: badba4c9-59a9-4616-90a7-79203868b4e8
📒 Files selected for processing (25)
fuzz/src/full_stack.rsfuzz/src/invoice_request_deser.rsfuzz/src/offer_deser.rslightning-background-processor/src/lib.rslightning-block-sync/src/init.rslightning-dns-resolver/src/lib.rslightning/src/ln/async_payments_tests.rslightning/src/ln/channelmanager.rslightning/src/ln/functional_test_utils.rslightning/src/ln/functional_tests.rslightning/src/ln/max_payment_path_len_tests.rslightning/src/ln/offers_tests.rslightning/src/ln/outbound_payment.rslightning/src/ln/reload_tests.rslightning/src/offers/currency.rslightning/src/offers/flow.rslightning/src/offers/invoice.rslightning/src/offers/invoice_request.rslightning/src/offers/merkle.rslightning/src/offers/mod.rslightning/src/offers/offer.rslightning/src/offers/refund.rslightning/src/offers/static_invoice.rslightning/src/offers/test_utils.rslightning/src/util/test_utils.rs
shaavan
force-pushed
the
currency-review
branch
from
e72c97d to
10fff8a
Compare
Add a `CurrencyConversion` trait for resolving currency-denominated amounts into millisatoshis. LDK cannot supply exchange rates itself, so applications provide this conversion logic as the foundation for fiat-denominated offer support. Co-Authored-By: OpenAI Codex <codex@openai.com>
shaavan
force-pushed
the
currency-review
branch
from
10fff8a to
f1f6e91
Compare
Exercise the core currency-to-msat conversion helper directly. Cover supported conversions, unsupported currencies, and non-finite conversion outputs so the new trait has focused unit coverage. Co-Authored-By: OpenAI Codex <codex@openai.com>
Store the `CurrencyConversion` implementation on `ChannelManager`. This keeps the conversion dependency owned by the higher-level payment state machine while leaving `OffersMessageFlow` focused on message coordination and path construction. Co-Authored-By: OpenAI Codex <codex@openai.com>
Allow `OfferBuilder` to set currency-denominated amounts when the caller provides a `CurrencyConversion` implementation. Move amount validation into the setters so fiat offers are checked when they are configured and `build()` no longer repeats that work. Co-Authored-By: OpenAI Codex <codex@openai.com>
Retain a `CurrencyConversion` reference on `InvoiceRequestBuilder`. Use that reference for both explicit payer-provided amounts and offer-derived amount lookups, while passing conversion explicitly through `OffersMessageFlow` from `ChannelManager`. Requests that omit `amount_msats` now stay omitted during request construction. The payee resolves those amounts later when building the invoice, so request-time checks only validate explicit payer-supplied amounts. Co-Authored-By: OpenAI Codex <codex@openai.com>
Extend invoice request parsing tests for currency-denominated offers to cover explicit serialized msat amounts. Verify that well-formed explicit amounts still parse and out-of-range values are rejected. Co-Authored-By: OpenAI Codex <codex@openai.com>
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
lightning/src/offers/invoice_request.rs (2)
53-58:⚠️ Potential issue | 🟡 MinorFix the doctest's
InvoiceRequestBuildergeneric arity.The hidden type hint still uses two generic arguments, but
InvoiceRequestBuildernow takes<'a, 'b, T, CC>. This doctest will not compile as written.🛠️ Suggested fix
-//! # <InvoiceRequestBuilder<_, _>>::from( +//! # <InvoiceRequestBuilder<'_, '_, _, _>>::from(#!/bin/bash set -euo pipefail rg -n "pub struct InvoiceRequestBuilder<'a, 'b, T: secp256k1::Signing, CC: CurrencyConversion>" lightning/src/offers/invoice_request.rs rg -n "<InvoiceRequestBuilder<_, _>>::from|<InvoiceRequestBuilder<'_, '_, _, _>>::from" lightning/src/offers/invoice_request.rs🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lightning/src/offers/invoice_request.rs` around lines 53 - 58, The doctest's hidden type hint uses two generics but InvoiceRequestBuilder now requires four (lifetime 'a, 'b and type params T, CC); update the hidden from() hint to match the new arity by replacing <InvoiceRequestBuilder<_, _>>::from with <InvoiceRequestBuilder<'_, '_, _, _>>::from so the doctest compiles; locate the doctest usage in invoice_request.rs around InvoiceRequestBuilder and change the generic parameter list accordingly.
289-295:⚠️ Potential issue | 🟠 MajorValidate implicit offer-derived amounts in
build_with_checkstoo.This branch now rechecks only explicit payer-provided amounts, so
build_and_sign()can succeed for requests whose effective total only exists in the offer.InvoiceRequest::try_fromstill validates that derived total later, so a “checked” request can fail round-trip parsing or invoice creation. For fiat offers it also keeps the signed request dependent on a later conversion lookup instead of freezing the resolved msat total.🛠️ Direction
- if let Some(amount_msats) = $self.invoice_request.amount_msats { - // Omitted amounts are resolved when the payee builds the invoice, so only - // explicit payer-provided amounts need request-time validation here. - $self.invoice_request.offer.check_amount_msats_for_quantity( - $self.currency_conversion, Some(amount_msats), $self.invoice_request.quantity - )?; - } + $self.invoice_request.offer.check_amount_msats_for_quantity( + $self.currency_conversion, + $self.invoice_request.amount_msats, + $self.invoice_request.quantity, + )?;For currency-denominated offers, also persist the resolved total into
self.invoice_request.amount_msatsbefore signing.Based on learnings: the invoice request builder is expected to resolve and encode a concrete msat amount for fiat offers at construction time.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lightning/src/offers/invoice_request.rs` around lines 289 - 295, The branch only validates explicit payer-provided amounts; update build_with_checks (the code around self.invoice_request.amount_msats / self.invoice_request.quantity) to also compute and validate the implicit offer-derived msat total when amount_msats is None by using the offer validation/resolution path (e.g. call the offer's amount-resolution using self.currency_conversion and quantity and then run offer.check_amount_msats_for_quantity with that resolved amount), and for currency-denominated offers persist that resolved msat total into self.invoice_request.amount_msats before calling build_and_sign so the signed request encodes a concrete msat value and later InvoiceRequest::try_from parsing/invoice creation won't fail.lightning/src/offers/flow.rs (1)
1701-1736:⚠️ Potential issue | 🟠 MajorUse the resolved amount when creating the static-invoice payment secret.
create_static_invoice_builder()now resolves a concreteamount_msatand uses it for blinded payment paths, but this helper still callscreate_for_spontaneous_payment(..., None, ...). That can leave the served static invoice amount-bound while the payment secret stays amountless.🛠️ Suggested fix
+ let amount_msat = offer.resolve_offer_amount(currency_conversion).map_err(|_| ())?; + let offer_relative_expiry = offer .absolute_expiry() .map(|exp| exp.saturating_sub(duration_since_epoch).as_secs()) .map(|exp_u64| exp_u64.try_into().unwrap_or(u32::MAX)) .unwrap_or(u32::MAX); @@ let payment_secret = inbound_payment::create_for_spontaneous_payment( expanded_key, - None, // The async receive offers we create are always amount-less + amount_msat, offer_relative_expiry, duration_since_epoch.as_secs(), None, )?;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lightning/src/offers/flow.rs` around lines 1701 - 1736, create_static_invoice_for_server currently creates the payment_secret with inbound_payment::create_for_spontaneous_payment(..., None, ...) leaving the secret amountless while create_static_invoice_builder now resolves a concrete amount_msat for blinded paths; change the code to resolve the concrete amount before calling create_for_spontaneous_payment (using the same resolution logic used by create_static_invoice_builder or by extracting the resolved amount_msat from that logic) and pass Some(resolved_amount_msat) instead of None when an amount is determined; keep passing None only when the invoice truly remains amountless.
🧹 Nitpick comments (2)
lightning/src/ln/offers_tests.rs (1)
1547-1558: Derive the rejection delta from the fixture tolerance.
amount_msats_unchecked(invoice.amount_msats() + 1)only asserts the intended behavior while the test converter's tolerance is zero. If that fixture ever advertises a non-zero tolerance, this test will start checking the wrong contract. Prefer a delta that is guaranteed to exceed the allowed band, or assert zero tolerance before using+ 1.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lightning/src/ln/offers_tests.rs` around lines 1547 - 1558, The test currently uses amount_msats_unchecked(invoice.amount_msats() + 1) which will fail to exceed the allowed band if the fixture advertises non-zero tolerance; update the test to derive the rejection delta from the invoice's tolerance (e.g. compute bad_amount = invoice.amount_msats() + invoice.tolerance() + 1) or first assert that invoice.tolerance() == 0 before using +1, then build the bad invoice with amount_msats_unchecked(bad_amount) so InvoiceRequestVerifiedFromOffer / send_payment_for_bolt12_invoice still yields Err(Bolt12PaymentError::UnexpectedInvoice).lightning/src/ln/functional_test_utils.rs (1)
895-927: Reuseself.currency_conversionin the round-trip read check.This smoke test deserializes with a fresh
&test_utils::TestCurrencyConversion, while_reload_nodereuses the node’s wired converter. Usingself.currency_conversionhere would keep both paths aligned.♻️ Proposed tweak
- currency_conversion: &test_utils::TestCurrencyConversion, + currency_conversion: self.currency_conversion,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lightning/src/ln/functional_test_utils.rs` around lines 895 - 927, The round-trip ChannelManager deserialization currently constructs a fresh &test_utils::TestCurrencyConversion in ChannelManager::read args; change that to reuse the node's wired converter by passing self.currency_conversion (borrowed as &self.currency_conversion if needed) so the deserialization path and _reload_node both use the same currency converter instance; update the ChannelManagerReadArgs.currency_conversion field accordingly where ChannelManager::read is invoked.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lightning/src/ln/offers_tests.rs`:
- Around line 960-970: The test currently calls
invoice_request.amount_msats(alice.node.currency_conversion) which can hide bugs
by falling back to Alice's FX converter; replace the converter with a
failing/default one so the test verifies the request actually encodes concrete
msats. Specifically, in the block around extract_invoice_request and
PaymentContext::Bolt12Offer/Bolt12OfferContext call invoice_request.amount_msats
with DefaultCurrencyConversion (or a deliberately failing converter) instead of
alice.node.currency_conversion to ensure the amount is resolved from the encoded
request rather than from Alice's conversion logic.
In `@lightning/src/ln/outbound_payment.rs`:
- Around line 2173-2182: The InvoiceReceived replay branch currently accepts any
new invoice with the same PaymentId; update the logic in the
PendingOutboundPayment::InvoiceReceived handling to compare the stored identity
(at minimum the stored payment_hash from the original invoice, ideally the
original invoice bytes or full invoice_request) against the incoming
invoice/payment hash and only take the idempotent replay path if they match; if
they differ, return DuplicateInvoice (or the existing duplicate error path)
instead of replacing invoice_request so send_payment_for_bolt12_invoice cannot
be built with a different invoice.
In `@lightning/src/offers/currency.rs`:
- Around line 18-32: The CurrencyConversion trait's docs need to state that
implementations must be synchronous and provide stable, cache-backed conversion
values across a single offer/payment flow so repeated calls do not change
results; update the CurrencyConversion trait doc (and the msats_per_minor_unit
method comment) to require that implementations return a deterministic
msats_per_minor_unit for consecutive calls within one flow (e.g., use local
caching or snapshotting of FX rates) and avoid performing live network fetches
per call that could cause validation/invoice mismatches.
---
Duplicate comments:
In `@lightning/src/offers/flow.rs`:
- Around line 1701-1736: create_static_invoice_for_server currently creates the
payment_secret with inbound_payment::create_for_spontaneous_payment(..., None,
...) leaving the secret amountless while create_static_invoice_builder now
resolves a concrete amount_msat for blinded paths; change the code to resolve
the concrete amount before calling create_for_spontaneous_payment (using the
same resolution logic used by create_static_invoice_builder or by extracting the
resolved amount_msat from that logic) and pass Some(resolved_amount_msat)
instead of None when an amount is determined; keep passing None only when the
invoice truly remains amountless.
In `@lightning/src/offers/invoice_request.rs`:
- Around line 53-58: The doctest's hidden type hint uses two generics but
InvoiceRequestBuilder now requires four (lifetime 'a, 'b and type params T, CC);
update the hidden from() hint to match the new arity by replacing
<InvoiceRequestBuilder<_, _>>::from with <InvoiceRequestBuilder<'_, '_, _,
_>>::from so the doctest compiles; locate the doctest usage in
invoice_request.rs around InvoiceRequestBuilder and change the generic parameter
list accordingly.
- Around line 289-295: The branch only validates explicit payer-provided
amounts; update build_with_checks (the code around
self.invoice_request.amount_msats / self.invoice_request.quantity) to also
compute and validate the implicit offer-derived msat total when amount_msats is
None by using the offer validation/resolution path (e.g. call the offer's
amount-resolution using self.currency_conversion and quantity and then run
offer.check_amount_msats_for_quantity with that resolved amount), and for
currency-denominated offers persist that resolved msat total into
self.invoice_request.amount_msats before calling build_and_sign so the signed
request encodes a concrete msat value and later InvoiceRequest::try_from
parsing/invoice creation won't fail.
---
Nitpick comments:
In `@lightning/src/ln/functional_test_utils.rs`:
- Around line 895-927: The round-trip ChannelManager deserialization currently
constructs a fresh &test_utils::TestCurrencyConversion in ChannelManager::read
args; change that to reuse the node's wired converter by passing
self.currency_conversion (borrowed as &self.currency_conversion if needed) so
the deserialization path and _reload_node both use the same currency converter
instance; update the ChannelManagerReadArgs.currency_conversion field
accordingly where ChannelManager::read is invoked.
In `@lightning/src/ln/offers_tests.rs`:
- Around line 1547-1558: The test currently uses
amount_msats_unchecked(invoice.amount_msats() + 1) which will fail to exceed the
allowed band if the fixture advertises non-zero tolerance; update the test to
derive the rejection delta from the invoice's tolerance (e.g. compute bad_amount
= invoice.amount_msats() + invoice.tolerance() + 1) or first assert that
invoice.tolerance() == 0 before using +1, then build the bad invoice with
amount_msats_unchecked(bad_amount) so InvoiceRequestVerifiedFromOffer /
send_payment_for_bolt12_invoice still yields
Err(Bolt12PaymentError::UnexpectedInvoice).
🪄 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: 94917eff-41c7-478e-a5cf-3c9e4c5b1962
📒 Files selected for processing (25)
fuzz/src/full_stack.rsfuzz/src/invoice_request_deser.rsfuzz/src/offer_deser.rslightning-background-processor/src/lib.rslightning-block-sync/src/init.rslightning-dns-resolver/src/lib.rslightning/src/ln/async_payments_tests.rslightning/src/ln/channelmanager.rslightning/src/ln/functional_test_utils.rslightning/src/ln/functional_tests.rslightning/src/ln/max_payment_path_len_tests.rslightning/src/ln/offers_tests.rslightning/src/ln/outbound_payment.rslightning/src/ln/reload_tests.rslightning/src/offers/currency.rslightning/src/offers/flow.rslightning/src/offers/invoice.rslightning/src/offers/invoice_request.rslightning/src/offers/merkle.rslightning/src/offers/mod.rslightning/src/offers/offer.rslightning/src/offers/refund.rslightning/src/offers/static_invoice.rslightning/src/offers/test_utils.rslightning/src/util/test_utils.rs
shaavan
force-pushed
the
currency-review
branch
2 times, most recently
from
47a2c2a to
9aa6a5e
Compare
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
1 similar comment
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
lightning/src/offers/invoice_request.rs (1)
53-57:⚠️ Potential issue | 🟡 MinorFix the stale
InvoiceRequestBuilderdoctest type hint.
InvoiceRequestBuildernow has four generic parameters, so the hidden line at Line 54 no longer compiles.🛠️ Minimal fix
-//! # <InvoiceRequestBuilder<_, _>>::from( +//! # <InvoiceRequestBuilder<'_, '_, _, _>>::from(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lightning/src/offers/invoice_request.rs` around lines 53 - 57, The doctest's hidden type annotation for InvoiceRequestBuilder is stale (it assumes two generics); update it to match the current signature by changing the hint to include four generic parameters—e.g. use InvoiceRequestBuilder<_, _, _, _>::from(...)—or remove the explicit type hint and let type inference pick it up; adjust the hidden line that references InvoiceRequestBuilder so the doctest compiles with the current InvoiceRequestBuilder generic arity.lightning/src/offers/offer.rs (1)
360-368:⚠️ Potential issue | 🟠 MajorAvoid converter-dependent validation for fiat offer amounts.
OfferContents::try_fromstill acceptsAmount::Currencydirectly from the wire, but this setter now resolves it through the caller'sCurrencyConversionand can fail onUnsupportedCurrencyor on today's exchange rate. That makes locally building a fiat offer stricter than parsing the same offer bytes. Keep the early bound check bitcoin-only here, and defer fiat conversion /MAX_VALUE_MSATenforcement toresolve_offer_amount()and the invoice resolution paths.🛠️ Minimal fix
-pub fn amount<CC: CurrencyConversion>($($self_mut)* $self: $self_type, amount: Amount, currency_conversion: &CC) -> Result<$return_type, Bolt12SemanticError> +pub fn amount<CC: CurrencyConversion>($($self_mut)* $self: $self_type, amount: Amount, _currency_conversion: &CC) -> Result<$return_type, Bolt12SemanticError> { - let amount_msats = amount.into_msats(currency_conversion)?; - if amount_msats == 0 || amount_msats > MAX_VALUE_MSAT { - return Err(Bolt12SemanticError::InvalidAmount); - } + match amount { + Amount::Bitcoin { amount_msats } if amount_msats == 0 || amount_msats > MAX_VALUE_MSAT => { + return Err(Bolt12SemanticError::InvalidAmount); + }, + Amount::Currency { amount: 0, .. } => return Err(Bolt12SemanticError::InvalidAmount), + _ => {} + } $self.offer.amount = Some(amount); Ok($return_value) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lightning/src/offers/offer.rs` around lines 360 - 368, The setter Offer::amount currently calls amount.into_msats(currency_conversion) making fiat (Amount::Currency) offers fail early; change amount(...) so it only performs msat-range validation when the amount is bitcoin-native (match on Amount variant), i.e., for Amount::Bitcoin / sats convert to msats and check ==0 or > MAX_VALUE_MSAT and return Bolt12SemanticError::InvalidAmount if invalid, but for Amount::Currency do not call into_msats or touch currency_conversion—simply set self.offer.amount = Some(amount) and return Ok, deferring fiat conversion and MAX_VALUE_MSAT enforcement to resolve_offer_amount() and invoice resolution paths.lightning/src/offers/flow.rs (1)
856-867:⚠️ Potential issue | 🟠 MajorKeep the static-invoice payment secret aligned with the resolved amount.
create_static_invoice_buildernow resolves a concreteamount_msat, butcreate_static_invoice_for_serverstill creates the payment secret withNoneunder the old “always amount-less” assumption. That leaves the payment-secret constraints looser than the static-invoice amount. Resolve the offer amount once increate_static_invoice_for_serverand reuse it for bothcreate_for_spontaneous_paymentand static-invoice construction.Also applies to: 1701-1737
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lightning/src/offers/flow.rs` around lines 856 - 867, The payment secret is being generated with None while the offer amount is actually resolved later, so update create_static_invoice_for_server to call offer.resolve_offer_amount (using the same currency_conversion) once, store the resulting amount_msat, and pass that concrete amount when generating the payment secret for create_for_spontaneous_payment and when calling create_static_invoice_builder; ensure you reuse the resolved amount_msat variable rather than resolving or leaving the payment secret amount-less in create_for_spontaneous_payment/create_static_invoice_builder to keep the static-invoice payment secret constraints aligned with the resolved amount.
🧹 Nitpick comments (5)
lightning-block-sync/src/init.rs (1)
67-117: Document thatcurrency_conversionmust support restored fiat currencies.The example wires the extra dependency correctly, but a reader can still infer that any stub
CurrencyConversionis acceptable. A short note that the supplied converter must handle any fiat currencies present in restored BOLT 12 state would make this API change much harder to misuse.Based on learnings, fiat invoice requests preserve their currency and only resolve to msats when
amount_msats(converter)is called, so a converter that always errors is insufficient for USD offers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lightning-block-sync/src/init.rs` around lines 67 - 117, Add a short doc note to the init_sync example and the ChannelManager/ChannelMonitor restore docs stating that the CurrencyConversion implementation passed as currency_conversion must be able to handle fiat currencies that may be present in restored BOLT 12 state (i.e., it must resolve preserved fiat invoice requests when amount_msats(converter) is called). Update the doc comment near the init_sync signature and/or the ChannelManagerReadArgs usage (reference symbols: init_sync, currency_conversion, ChannelManagerReadArgs, amount_msats) to explicitly require converters to support restored fiat currencies rather than allowing a stub that always errors.fuzz/src/full_stack.rs (1)
191-194: Let the fuzz target resolve at least one currency.Returning
Err(())for everyCurrencyCodemeans any fiat-denominated offer or invoice-request path aborts before the new conversion logic runs, so this target won't exercise the success-side behavior added in this PR. Consider a deterministic rate for one or two currencies and keep the blanket error for the rest.Based on learnings, fiat invoice requests preserve their currency and only resolve to msats when
amount_msats(converter)is called, so a converter that always errors prevents those paths from being exercised.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fuzz/src/full_stack.rs` around lines 191 - 194, The FuzzCurrencyConversion::msats_per_minor_unit implementation currently returns Err(()) for every CurrencyCode which prevents exercising success paths; update msats_per_minor_unit to return Ok((rate, minor_unit_exponent)) for one or two deterministic currencies (e.g., handle CurrencyCode::Usd and CurrencyCode::Eur by returning a fixed conversion tuple like (1000.0, 2) or similar) and keep returning Err(()) for all other CurrencyCode values so most cases still fail but the fuzz target can exercise the conversion-success behavior.lightning/src/ln/functional_test_utils.rs (1)
895-935: Reuseself.currency_conversionin the drop-time roundtrip.
_reload_nodenow deserializes with the node's configured converter, but this self-check still hardcodes a freshtest_utils::TestCurrencyConversion. Reusing the field here would keep the sanity check aligned with the real reload path.♻️ Small consistency tweak
- currency_conversion: &test_utils::TestCurrencyConversion, + currency_conversion: self.currency_conversion,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lightning/src/ln/functional_test_utils.rs` around lines 895 - 935, In _reload_node's sanity-check roundtrip where ChannelManager::read is called with ChannelManagerReadArgs, replace the hardcoded &test_utils::TestCurrencyConversion with a reference to the node's configured converter (use &self.currency_conversion or appropriate borrowing of self.currency_conversion) so the deserialization sanity check uses the same currency_conversion as the real reload path; update the argument passed into ChannelManagerReadArgs.currency_conversion accordingly (ensure the reference/type matches test_utils::TestCurrencyConversion expected by ChannelManager::read).lightning/src/offers/currency.rs (1)
18-32: Prefer a named result type over(f64, u8).This is a new public API, and a positional tuple makes it too easy to mix up the conversion rate and tolerance at call sites. A tiny struct would make the fields self-documenting and give you a cleaner place to hang field-level docs and invariants later.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lightning/src/offers/currency.rs` around lines 18 - 32, The public trait CurrencyConversion currently returns a positional tuple Result<(f64, u8), ()> from msats_per_minor_unit; replace that tuple with a named struct (e.g., Conversion or ConversionRate) containing clearly named fields like msats_per_minor_unit: f64 and tolerance_percent: u8, update the msats_per_minor_unit signature to return Result<Conversion, ()>, and adjust the doc comment on the trait and the new struct to describe each field and any invariants so call sites no longer rely on positional ordering.lightning/src/ln/async_payments_tests.rs (1)
1453-1479: Use the node’s configured converter here too.Spinning up a fresh
TestCurrencyConversionbypasses the same dependency the rest of the flow uses, so this test can drift from production wiring if the harness changes.Based on learnings: in tests, the node’s configured `currency_conversion` is the intended converter to use for offer / invoice-request flows.♻️ Suggested change
- let conversion = TestCurrencyConversion; - nodes[0].node.pay_for_offer(&offer, Some(amt_msat), payment_id, Default::default()).unwrap(); let release_held_htlc_om_3_0 = pass_async_payments_oms( static_invoice, @@ .request_invoice( &nodes[0].keys_manager.get_expanded_key(), Nonce::from_entropy_source(nodes[0].keys_manager), &secp_ctx, payment_id, - &conversion, + nodes[0].node.currency_conversion, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lightning/src/ln/async_payments_tests.rs` around lines 1453 - 1479, The test creates a fresh TestCurrencyConversion (conversion) but should use the node's configured converter; replace the TestCurrencyConversion instantiation and any local usage with the node's configured currency conversion (the converter attached to nodes[0]) so the call to request_invoice(...) inside test_modify_pending_payment uses the same converter the node flow uses (replace conversion with nodes[0]'s configured currency conversion when calling request_invoice and remove TestCurrencyConversion).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lightning/src/ln/outbound_payment.rs`:
- Around line 1285-1289: When InvoiceBuilder::amount_msats(invreq,
currency_conversion) fails you must clear the pending payment instead of just
returning Bolt12PaymentError::UnexpectedInvoice; change the map_err branch so it
invokes the existing abandon_with_entry! macro to remove/abandon the pending
payment entry (the same way other invoice-rejection paths do) and then return
the error (Bolt12PaymentError::UnexpectedInvoice). In short: replace the current
map_err(|_| Bolt12PaymentError::UnexpectedInvoice) handler with one that calls
abandon_with_entry!(...) to transition out of AwaitingInvoice and then returns
the UnexpectedInvoice error.
In `@lightning/src/offers/flow.rs`:
- Around line 825-826: Update the doc comment for create_invoice_request_builder
to state that the provided CurrencyConversion is recorded for later use but
fiat-denominated amounts are NOT converted during request building; clarify that
InvoiceRequest preserves fiat amounts and conversion to msats happens only when
InvoiceRequest::amount_msats(converter) is called, so the resulting
InvoiceRequest is not fully self-contained until conversion is performed.
In `@lightning/src/offers/invoice_request.rs`:
- Around line 289-295: The current check only validates explicit
invoice_request.amount_msats and misses cases where the offer is
bitcoin-denominated and amount_msats is derived; update the validation in
build_and_sign (or the surrounding pre-sign path that currently calls
invoice_request.offer.check_amount_msats_for_quantity) to also call
offer.check_amount_msats_for_quantity when the offer amount is Amount::Bitcoin
even if invoice_request.amount_msats is None, passing self.currency_conversion
and invoice_request.quantity so the derived unit_msats * quantity is validated
before signing; keep the existing behavior for fiat offers (i.e., do not force
resolution for fiat-denominated offers).
---
Duplicate comments:
In `@lightning/src/offers/flow.rs`:
- Around line 856-867: The payment secret is being generated with None while the
offer amount is actually resolved later, so update
create_static_invoice_for_server to call offer.resolve_offer_amount (using the
same currency_conversion) once, store the resulting amount_msat, and pass that
concrete amount when generating the payment secret for
create_for_spontaneous_payment and when calling create_static_invoice_builder;
ensure you reuse the resolved amount_msat variable rather than resolving or
leaving the payment secret amount-less in
create_for_spontaneous_payment/create_static_invoice_builder to keep the
static-invoice payment secret constraints aligned with the resolved amount.
In `@lightning/src/offers/invoice_request.rs`:
- Around line 53-57: The doctest's hidden type annotation for
InvoiceRequestBuilder is stale (it assumes two generics); update it to match the
current signature by changing the hint to include four generic parameters—e.g.
use InvoiceRequestBuilder<_, _, _, _>::from(...)—or remove the explicit type
hint and let type inference pick it up; adjust the hidden line that references
InvoiceRequestBuilder so the doctest compiles with the current
InvoiceRequestBuilder generic arity.
In `@lightning/src/offers/offer.rs`:
- Around line 360-368: The setter Offer::amount currently calls
amount.into_msats(currency_conversion) making fiat (Amount::Currency) offers
fail early; change amount(...) so it only performs msat-range validation when
the amount is bitcoin-native (match on Amount variant), i.e., for
Amount::Bitcoin / sats convert to msats and check ==0 or > MAX_VALUE_MSAT and
return Bolt12SemanticError::InvalidAmount if invalid, but for Amount::Currency
do not call into_msats or touch currency_conversion—simply set self.offer.amount
= Some(amount) and return Ok, deferring fiat conversion and MAX_VALUE_MSAT
enforcement to resolve_offer_amount() and invoice resolution paths.
---
Nitpick comments:
In `@fuzz/src/full_stack.rs`:
- Around line 191-194: The FuzzCurrencyConversion::msats_per_minor_unit
implementation currently returns Err(()) for every CurrencyCode which prevents
exercising success paths; update msats_per_minor_unit to return Ok((rate,
minor_unit_exponent)) for one or two deterministic currencies (e.g., handle
CurrencyCode::Usd and CurrencyCode::Eur by returning a fixed conversion tuple
like (1000.0, 2) or similar) and keep returning Err(()) for all other
CurrencyCode values so most cases still fail but the fuzz target can exercise
the conversion-success behavior.
In `@lightning-block-sync/src/init.rs`:
- Around line 67-117: Add a short doc note to the init_sync example and the
ChannelManager/ChannelMonitor restore docs stating that the CurrencyConversion
implementation passed as currency_conversion must be able to handle fiat
currencies that may be present in restored BOLT 12 state (i.e., it must resolve
preserved fiat invoice requests when amount_msats(converter) is called). Update
the doc comment near the init_sync signature and/or the ChannelManagerReadArgs
usage (reference symbols: init_sync, currency_conversion,
ChannelManagerReadArgs, amount_msats) to explicitly require converters to
support restored fiat currencies rather than allowing a stub that always errors.
In `@lightning/src/ln/async_payments_tests.rs`:
- Around line 1453-1479: The test creates a fresh TestCurrencyConversion
(conversion) but should use the node's configured converter; replace the
TestCurrencyConversion instantiation and any local usage with the node's
configured currency conversion (the converter attached to nodes[0]) so the call
to request_invoice(...) inside test_modify_pending_payment uses the same
converter the node flow uses (replace conversion with nodes[0]'s configured
currency conversion when calling request_invoice and remove
TestCurrencyConversion).
In `@lightning/src/ln/functional_test_utils.rs`:
- Around line 895-935: In _reload_node's sanity-check roundtrip where
ChannelManager::read is called with ChannelManagerReadArgs, replace the
hardcoded &test_utils::TestCurrencyConversion with a reference to the node's
configured converter (use &self.currency_conversion or appropriate borrowing of
self.currency_conversion) so the deserialization sanity check uses the same
currency_conversion as the real reload path; update the argument passed into
ChannelManagerReadArgs.currency_conversion accordingly (ensure the
reference/type matches test_utils::TestCurrencyConversion expected by
ChannelManager::read).
In `@lightning/src/offers/currency.rs`:
- Around line 18-32: The public trait CurrencyConversion currently returns a
positional tuple Result<(f64, u8), ()> from msats_per_minor_unit; replace that
tuple with a named struct (e.g., Conversion or ConversionRate) containing
clearly named fields like msats_per_minor_unit: f64 and tolerance_percent: u8,
update the msats_per_minor_unit signature to return Result<Conversion, ()>, and
adjust the doc comment on the trait and the new struct to describe each field
and any invariants so call sites no longer rely on positional ordering.
🪄 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: 61c97f01-9d35-4a6b-ae39-dd5657ef6b10
📒 Files selected for processing (25)
fuzz/src/full_stack.rsfuzz/src/invoice_request_deser.rsfuzz/src/offer_deser.rslightning-background-processor/src/lib.rslightning-block-sync/src/init.rslightning-dns-resolver/src/lib.rslightning/src/ln/async_payments_tests.rslightning/src/ln/channelmanager.rslightning/src/ln/functional_test_utils.rslightning/src/ln/functional_tests.rslightning/src/ln/max_payment_path_len_tests.rslightning/src/ln/offers_tests.rslightning/src/ln/outbound_payment.rslightning/src/ln/reload_tests.rslightning/src/offers/currency.rslightning/src/offers/flow.rslightning/src/offers/invoice.rslightning/src/offers/invoice_request.rslightning/src/offers/merkle.rslightning/src/offers/mod.rslightning/src/offers/offer.rslightning/src/offers/refund.rslightning/src/offers/static_invoice.rslightning/src/offers/test_utils.rslightning/src/util/test_utils.rs
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lightning/src/offers/invoice.rs (1)
26-61:⚠️ Potential issue | 🟡 MinorClarify the
DefaultCurrencyConversionlimitation in the example.This snippet now reads like the generic “respond to an invoice request” flow, but
DefaultCurrencyConversionwill fail for fiat-denominated requests that preserve their currency amount. Please either switch the example to a converter stub that supports currency offers or call out that the default converter only works when the request already resolves to msats.Based on learnings: fiat invoice requests preserve the offer currency and require a real converter when
amount_msatsis resolved later.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lightning/src/offers/invoice.rs` around lines 26 - 61, The example uses DefaultCurrencyConversion which doesn't support fiat-denominated requests that preserve their currency amount; replace it with or show a stub converter that implements the necessary conversion logic (or add a clarifying note) and update the example invocation of InvoiceRequest::respond_with / respond_with_no_std to pass that converter; specifically, change the DefaultCurrencyConversion reference to a custom converter stub implementing the conversion trait used by respond_with (so fiat offers resolve correctly when amount_msats is computed later) or add a comment next to DefaultCurrencyConversion explaining it only works when the request already resolves to msats.
♻️ Duplicate comments (4)
lightning/src/ln/outbound_payment.rs (1)
1285-1289:⚠️ Potential issue | 🟠 MajorAbandon the pending payment when static-invoice amount resolution fails.
InvoiceBuilder::amount_msats(invreq, currency_conversion)can now fail for a real runtime reason on fiat requests. ReturningUnexpectedInvoicehere leaves the entry inAwaitingInvoice, so the payment stays pending even though this static invoice was already rejected.Suggested fix
- let amount_msat = InvoiceBuilder::<DerivedSigningPubkey>::amount_msats( - invreq, - currency_conversion, - ) - .map_err(|_| Bolt12PaymentError::UnexpectedInvoice)?; + let amount_msat = match InvoiceBuilder::<DerivedSigningPubkey>::amount_msats( + invreq, + currency_conversion, + ) { + Ok(amount_msat) => amount_msat, + Err(_) => { + abandon_with_entry!(entry, PaymentFailureReason::UnexpectedError); + return Err(Bolt12PaymentError::UnexpectedInvoice); + }, + };Based on learnings: fiat
InvoiceRequest::amount_msats(converter)resolves at call time and can legitimately fail when the supplied converter cannot resolve the offer currency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lightning/src/ln/outbound_payment.rs` around lines 1285 - 1289, When InvoiceBuilder::amount_msats(invreq, currency_conversion) returns Err you must abandon the pending AwaitingInvoice entry instead of just returning Bolt12PaymentError::UnexpectedInvoice; change the error mapping so it first transitions the payment state from AwaitingInvoice to an abandoned/failed state (call whatever local helper exists, e.g. abandon_pending_payment(payment_id) or set payment state to Abandoned/Failed) and then return the error (include the original error details or wrap it); update the map_err around InvoiceBuilder::amount_msats to perform that state transition before returning the Bolt12PaymentError.lightning/src/offers/invoice_request.rs (2)
54-57:⚠️ Potential issue | 🟡 MinorFix the doctest's
InvoiceRequestBuildergeneric arity.Line 54 still uses
InvoiceRequestBuilder<_, _>, but the type now has four generics ('a,'b,T,CC), so this doctest no longer compiles.Minimal doc fix
-//! # <InvoiceRequestBuilder<_, _>>::from( +//! # <InvoiceRequestBuilder<'_, '_, _, _>>::from(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lightning/src/offers/invoice_request.rs` around lines 54 - 57, The doctest uses the wrong generic arity for InvoiceRequestBuilder (currently written as InvoiceRequestBuilder<_, _>); update the example to use four generics (e.g., InvoiceRequestBuilder<_, _, _, _>) or otherwise match the new signature InvoiceRequestBuilder<'a, 'b, T, CC> so the doctest compiles; modify the snippet around InvoiceRequestBuilder in invoice_request.rs to reference the correct generic parameters.
289-295:⚠️ Potential issue | 🟠 MajorValidate derived bitcoin totals before signing.
When
amount_msatsis omitted, this skips amount validation even for bitcoin-denominated offers. That letsbuild_and_sign()emit requests whose ownamount_msats()accessor and parse path reject onceunit_msats * quantityoverflows or exceedsMAX_VALUE_MSAT.Suggested fix
- if let Some(amount_msats) = $self.invoice_request.amount_msats { - // Omitted amounts are resolved when the payee builds the invoice, so only - // explicit payer-provided amounts need request-time validation here. - $self.invoice_request.offer.check_amount_msats_for_quantity( - $self.currency_conversion, Some(amount_msats), $self.invoice_request.quantity - )?; - } + let should_check_amount = $self.invoice_request.amount_msats.is_some() + || matches!( + $self.invoice_request.offer.amount(), + Some(crate::offers::offer::Amount::Bitcoin { .. }) + ); + if should_check_amount { + $self.invoice_request.offer.check_amount_msats_for_quantity( + $self.currency_conversion, + $self.invoice_request.amount_msats, + $self.invoice_request.quantity + )?; + }Based on learnings: fiat-denominated offers intentionally stay unresolved until
amount_msats(converter), so the extra pre-sign check is only needed forAmount::Bitcoinoffers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lightning/src/offers/invoice_request.rs` around lines 289 - 295, The code only validates payer-provided amounts (when invoice_request.amount_msats is Some), but skips validation for derived bitcoin totals when amount_msats is None; update the logic so that when invoice_request.amount_msats is None and the offer is bitcoin-denominated (match invoice_request.offer.amount as Amount::Bitcoin or use the offer's currency check), call invoice_request.offer.check_amount_msats_for_quantity(invoice_request.currency_conversion, None, invoice_request.quantity) before build_and_sign to validate unit_msats * quantity (using the same function check_amount_msats_for_quantity) so overflows or MAX_VALUE_MSAT violations are caught prior to signing.lightning/src/offers/offer.rs (1)
360-368:⚠️ Potential issue | 🟠 MajorDon't make fiat offer construction depend on the current converter.
Amount::Currencystill goes throughinto_msats()here, so building a wire-valid fiat offer now fails whenever the provided converter does not support that currency or today's rate resolves aboveMAX_VALUE_MSAT.OfferContents::try_fromaccepts the same serialized offer, so this reintroduces a builder/parser invariant break and makes offer creation rate-dependent.Suggested fix
-pub fn amount<CC: CurrencyConversion>($($self_mut)* $self: $self_type, amount: Amount, currency_conversion: &CC) -> Result<$return_type, Bolt12SemanticError> +pub fn amount<CC: CurrencyConversion>($($self_mut)* $self: $self_type, amount: Amount, _currency_conversion: &CC) -> Result<$return_type, Bolt12SemanticError> { - let amount_msats = amount.into_msats(currency_conversion)?; - if amount_msats == 0 || amount_msats > MAX_VALUE_MSAT { - return Err(Bolt12SemanticError::InvalidAmount); - } + match amount { + Amount::Bitcoin { amount_msats } if amount_msats == 0 || amount_msats > MAX_VALUE_MSAT => { + return Err(Bolt12SemanticError::InvalidAmount); + }, + Amount::Currency { amount: 0, .. } => return Err(Bolt12SemanticError::InvalidAmount), + _ => {} + } $self.offer.amount = Some(amount); Ok($return_value) }Based on learnings: fiat-denominated invoice requests preserve the fiat denomination and resolve to msats at call time, so resolving the fiat amount in the offer builder is too early.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lightning/src/offers/offer.rs` around lines 360 - 368, The amount method in offer.rs currently calls amount.into_msats(currency_conversion) which forces fiat amounts to be resolved at build time and can fail or exceed MAX_VALUE_MSAT; instead, stop depending on the converter here. Modify pub fn amount<CC: CurrencyConversion>(...) so it does not call into_msats for Amount::Currency: for Amount::Msat / Amount::Sat resolve/validate against MAX_VALUE_MSAT as before, but for Amount::Currency accept and store the Amount unchanged (self.offer.amount = Some(amount)) without conversion or MAX_VALUE_MSAT checks; rely on OfferContents::try_from or the final serialization/validation step to convert fiat to msats at call time. Ensure references to amount.into_msats and the related error branch are removed or conditional only for non-currency variants.
🧹 Nitpick comments (1)
lightning/src/ln/offers_tests.rs (1)
1539-1550: Tighten this negative case to mutate only the amount.Rebuilding
bad_invoicewith syntheticpayment_paths()andDuration::from_secs(1000)meansUnexpectedInvoicecan still come from unrelated differences. Reuse the emitted invoice's paths/timestamp where possible and only bump the msat amount here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lightning/src/ln/offers_tests.rs` around lines 1539 - 1550, The negative test builds bad_invoice using fresh synthetic payment_paths() and Duration::from_secs(1000), which can introduce unrelated differences; instead, locate the DerivedKeys branch where bad_invoice is created (InvoiceRequestVerifiedFromOffer::DerivedKeys and the call chain respond_using_derived_keys_no_std -> build_and_sign) and reuse the emitted invoice's fields (payment_paths and timestamp) from the originally built/signed invoice, then only mutate the amount_msats by calling amount_msats_unchecked with invoice.amount_msats() + 1 (or create a minimal cloned copy that preserves paths/timestamp and only changes the msat) before build/signing so UnexpectedInvoice failures can only come from the amount change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@lightning/src/offers/invoice.rs`:
- Around line 26-61: The example uses DefaultCurrencyConversion which doesn't
support fiat-denominated requests that preserve their currency amount; replace
it with or show a stub converter that implements the necessary conversion logic
(or add a clarifying note) and update the example invocation of
InvoiceRequest::respond_with / respond_with_no_std to pass that converter;
specifically, change the DefaultCurrencyConversion reference to a custom
converter stub implementing the conversion trait used by respond_with (so fiat
offers resolve correctly when amount_msats is computed later) or add a comment
next to DefaultCurrencyConversion explaining it only works when the request
already resolves to msats.
---
Duplicate comments:
In `@lightning/src/ln/outbound_payment.rs`:
- Around line 1285-1289: When InvoiceBuilder::amount_msats(invreq,
currency_conversion) returns Err you must abandon the pending AwaitingInvoice
entry instead of just returning Bolt12PaymentError::UnexpectedInvoice; change
the error mapping so it first transitions the payment state from AwaitingInvoice
to an abandoned/failed state (call whatever local helper exists, e.g.
abandon_pending_payment(payment_id) or set payment state to Abandoned/Failed)
and then return the error (include the original error details or wrap it);
update the map_err around InvoiceBuilder::amount_msats to perform that state
transition before returning the Bolt12PaymentError.
In `@lightning/src/offers/invoice_request.rs`:
- Around line 54-57: The doctest uses the wrong generic arity for
InvoiceRequestBuilder (currently written as InvoiceRequestBuilder<_, _>); update
the example to use four generics (e.g., InvoiceRequestBuilder<_, _, _, _>) or
otherwise match the new signature InvoiceRequestBuilder<'a, 'b, T, CC> so the
doctest compiles; modify the snippet around InvoiceRequestBuilder in
invoice_request.rs to reference the correct generic parameters.
- Around line 289-295: The code only validates payer-provided amounts (when
invoice_request.amount_msats is Some), but skips validation for derived bitcoin
totals when amount_msats is None; update the logic so that when
invoice_request.amount_msats is None and the offer is bitcoin-denominated (match
invoice_request.offer.amount as Amount::Bitcoin or use the offer's currency
check), call
invoice_request.offer.check_amount_msats_for_quantity(invoice_request.currency_conversion,
None, invoice_request.quantity) before build_and_sign to validate unit_msats *
quantity (using the same function check_amount_msats_for_quantity) so overflows
or MAX_VALUE_MSAT violations are caught prior to signing.
In `@lightning/src/offers/offer.rs`:
- Around line 360-368: The amount method in offer.rs currently calls
amount.into_msats(currency_conversion) which forces fiat amounts to be resolved
at build time and can fail or exceed MAX_VALUE_MSAT; instead, stop depending on
the converter here. Modify pub fn amount<CC: CurrencyConversion>(...) so it does
not call into_msats for Amount::Currency: for Amount::Msat / Amount::Sat
resolve/validate against MAX_VALUE_MSAT as before, but for Amount::Currency
accept and store the Amount unchanged (self.offer.amount = Some(amount)) without
conversion or MAX_VALUE_MSAT checks; rely on OfferContents::try_from or the
final serialization/validation step to convert fiat to msats at call time.
Ensure references to amount.into_msats and the related error branch are removed
or conditional only for non-currency variants.
---
Nitpick comments:
In `@lightning/src/ln/offers_tests.rs`:
- Around line 1539-1550: The negative test builds bad_invoice using fresh
synthetic payment_paths() and Duration::from_secs(1000), which can introduce
unrelated differences; instead, locate the DerivedKeys branch where bad_invoice
is created (InvoiceRequestVerifiedFromOffer::DerivedKeys and the call chain
respond_using_derived_keys_no_std -> build_and_sign) and reuse the emitted
invoice's fields (payment_paths and timestamp) from the originally built/signed
invoice, then only mutate the amount_msats by calling amount_msats_unchecked
with invoice.amount_msats() + 1 (or create a minimal cloned copy that preserves
paths/timestamp and only changes the msat) before build/signing so
UnexpectedInvoice failures can only come from the amount change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 877aae41-a6a9-46e5-be26-6a582356dba9
📒 Files selected for processing (25)
fuzz/src/full_stack.rsfuzz/src/invoice_request_deser.rsfuzz/src/offer_deser.rslightning-background-processor/src/lib.rslightning-block-sync/src/init.rslightning-dns-resolver/src/lib.rslightning/src/ln/async_payments_tests.rslightning/src/ln/channelmanager.rslightning/src/ln/functional_test_utils.rslightning/src/ln/functional_tests.rslightning/src/ln/max_payment_path_len_tests.rslightning/src/ln/offers_tests.rslightning/src/ln/outbound_payment.rslightning/src/ln/reload_tests.rslightning/src/offers/currency.rslightning/src/offers/flow.rslightning/src/offers/invoice.rslightning/src/offers/invoice_request.rslightning/src/offers/merkle.rslightning/src/offers/mod.rslightning/src/offers/offer.rslightning/src/offers/refund.rslightning/src/offers/static_invoice.rslightning/src/offers/test_utils.rslightning/src/util/test_utils.rs
Use `CurrencyConversion` while responding to an `InvoiceRequest`. This lets payees resolve currency-denominated offer amounts into the millisatoshi amount carried by the invoice. Requests that omit `amount_msats` remain omitted, and the invoice preserves the original request bytes instead of normalizing them into an explicit request amount. On the payer side, the returned invoice amount is checked against the locally expected amount before payment starts. Receiver-side lower-bound checks still apply only to explicit payer-provided amounts. Co-Authored-By: OpenAI Codex <codex@openai.com>
Add targeted tests for the currency-denominated offer and invoice handling paths introduced by the new conversion flow. Cover end-to-end offer flows, parsed fiat request amounts below an offer minimum, deferred invoice replay identity, and async amount resolution failures. This keeps the invoice-quoting change pinned across its success and rejection paths without spreading closely related test updates across multiple commits. Co-Authored-By: OpenAI Codex <codex@openai.com>
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (7)
fuzz/src/invoice_request_deser.rs (1)
65-71:⚠️ Potential issue | 🔴 CriticalPass the converter into
respond_with.Line 156 still calls
InvoiceRequest::respond_withwith the old signature, so this target will not compile even thoughFuzzCurrencyConversionwas added here.🛠️ Proposed fix
let payment_hash = PaymentHash([42; 32]); - invoice_request.respond_with(vec![payment_path], payment_hash)?.build() + let currency_conversion = FuzzCurrencyConversion; + invoice_request + .respond_with(¤cy_conversion, vec![payment_path], payment_hash)? + .build()Run this to confirm the stale call site against the current signature:
#!/bin/bash set -e echo "== Current call site in fuzz/src/invoice_request_deser.rs ==" rg -n -A2 -B2 '\brespond_with\s*\(' fuzz/src/invoice_request_deser.rs echo echo "== Current InvoiceRequest::respond_with signature ==" rg -n -A6 -B2 'pub fn respond_with<CC: CurrencyConversion>' lightning/src/offers/invoice_request.rs🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fuzz/src/invoice_request_deser.rs` around lines 65 - 71, The call site still uses the old InvoiceRequest::respond_with signature; update the fuzz target to construct or use the new FuzzCurrencyConversion and pass it into InvoiceRequest::respond_with so the generic CurrencyConversion parameter is satisfied. Locate the FuzzCurrencyConversion type in this file and a call to respond_with(...) and change that call to respond_with::<FuzzCurrencyConversion>(...) or pass an instance as the new argument according to the current InvoiceRequest::respond_with<CC: CurrencyConversion> signature so the compiler sees a valid CurrencyConversion implementation.lightning/src/offers/flow.rs (2)
825-826:⚠️ Potential issue | 🟡 MinorReword the request-builder docs for fiat offers.
This still says the amount is resolved while building the request, but fiat-denominated amounts stay on the
InvoiceRequestand are only converted whenInvoiceRequest::amount_msats(converter)is evaluated.📝 Suggested wording
- /// The provided [`CurrencyConversion`] is used to resolve any currency-denominated offer amount - /// while building the request. + /// The provided [`CurrencyConversion`] is retained for later validation and amount resolution. + /// Currency-denominated offer amounts remain on the request and are converted only when + /// [`InvoiceRequest::amount_msats`] is evaluated.Based on learnings:
InvoiceRequest::amount_msatsresolves fiat-to-msat conversion at call time and fiat-denominated offers are preserved in the invoice request as-is.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lightning/src/offers/flow.rs` around lines 825 - 826, The doc comment for the request-builder incorrectly states that the provided CurrencyConversion is used to resolve fiat-denominated amounts "while building the request"; update the wording to state that fiat-denominated amounts are preserved on InvoiceRequest and conversion occurs later when InvoiceRequest::amount_msats(converter) is evaluated. Mention CurrencyConversion and InvoiceRequest::amount_msats explicitly so readers know conversion happens at call time, not during request construction.
1701-1736:⚠️ Potential issue | 🟠 MajorBind the static-invoice payment secret to the resolved amount.
create_static_invoice_buildernow resolves fixed / currency amounts intoamount_msat, butcreate_for_spontaneous_paymentis still called withNone. That leaves the inbound payment secret amount-less while the static-invoice payment paths are amount-bound.🐛 Minimal fix
fn create_static_invoice_for_server<R: Router, CC: CurrencyConversion>( &self, offer: &Offer, offer_nonce: Nonce, peers: Vec<MessageForwardNode>, usable_channels: Vec<ChannelDetails>, router: R, currency_conversion: &CC, ) -> Result<(StaticInvoice, BlindedMessagePath), ()> { let expanded_key = &self.inbound_payment_key; let duration_since_epoch = self.duration_since_epoch(); let secp_ctx = &self.secp_ctx; + let amount_msat = offer.resolve_offer_amount(currency_conversion).map_err(|_| ())?; let offer_relative_expiry = offer .absolute_expiry() .map(|exp| exp.saturating_sub(duration_since_epoch).as_secs()) .map(|exp_u64| exp_u64.try_into().unwrap_or(u32::MAX)) .unwrap_or(u32::MAX); // Set the invoice to expire at the same time as the offer. We aim to update this invoice as // often as possible, so there shouldn't be any reason to have it expire earlier than the // offer. let payment_secret = inbound_payment::create_for_spontaneous_payment( expanded_key, - None, // The async receive offers we create are always amount-less + amount_msat, offer_relative_expiry, duration_since_epoch.as_secs(), None, )?;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lightning/src/offers/flow.rs` around lines 1701 - 1736, The payment secret is created amount-less but the static invoice paths are amount-bound; resolve the fixed/currency amount the same way create_static_invoice_builder does (using offer, offer_nonce and currency_conversion) to obtain amount_msat and pass Some(amount_msat) as the amount parameter to inbound_payment::create_for_spontaneous_payment in create_static_invoice_for_server instead of None so the payment_secret is bound to the resolved amount; keep using the same duration/expiry logic and then call create_static_invoice_builder with that payment_secret.lightning/src/offers/invoice_request.rs (2)
53-57:⚠️ Potential issue | 🟡 MinorFix the doctest's
InvoiceRequestBuildertype hint.The hidden example still uses
InvoiceRequestBuilder<_, _>, but the builder now has four generic parameters ('a,'b,T,CC), so this doctest won't compile as written.🛠️ Minimal fix
-//! # <InvoiceRequestBuilder<_, _>>::from( +//! # <InvoiceRequestBuilder<'_, '_, _, _>>::from(Verify by comparing the struct definition with the doctest helper:
#!/bin/bash set -euo pipefail rg -n "pub struct InvoiceRequestBuilder<" lightning/src/offers/invoice_request.rs rg -n -C1 "InvoiceRequestBuilder<_, _>|InvoiceRequestBuilder<'_, '_, _, _>" lightning/src/offers/invoice_request.rs🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lightning/src/offers/invoice_request.rs` around lines 53 - 57, The doctest uses the old shorthand InvoiceRequestBuilder<_, _> which no longer matches the struct signature; update the hidden example to use the four-parameter form InvoiceRequestBuilder<'_, '_, _, _> (matching the struct pub struct InvoiceRequestBuilder<'a, 'b, T, CC>) so the example compiles; locate the helper usage in the doctest block that currently references InvoiceRequestBuilder<_, _> and replace it with InvoiceRequestBuilder<'_, '_, _, _>.
289-295:⚠️ Potential issue | 🟠 MajorKeep validating derived bitcoin totals before signing.
This guard only runs when the payer set an explicit
amount_msats. If the request omits that field but the offer is bitcoin-denominated,build_and_sign()can still emit an invoice request whose implicitoffer.amount * quantityexceedsMAX_VALUE_MSAT, even though the parser rejects the same payload later.🛠️ Narrow fix that preserves late-bound fiat requests
- if let Some(amount_msats) = $self.invoice_request.amount_msats { - // Omitted amounts are resolved when the payee builds the invoice, so only - // explicit payer-provided amounts need request-time validation here. - $self.invoice_request.offer.check_amount_msats_for_quantity( - $self.currency_conversion, Some(amount_msats), $self.invoice_request.quantity - )?; - } + let should_check_amount = $self.invoice_request.amount_msats.is_some() + || matches!( + $self.invoice_request.offer.amount(), + Some(crate::offers::offer::Amount::Bitcoin { .. }) + ); + if should_check_amount { + $self.invoice_request.offer.check_amount_msats_for_quantity( + $self.currency_conversion, + $self.invoice_request.amount_msats, + $self.invoice_request.quantity, + )?; + }Based on learnings: fiat-denominated offers are preserved on the invoice request and resolved by
amount_msats(converter)at call time, so this missing pre-sign validation only applies to deterministically resolvable cases such as bitcoin-denominated offers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lightning/src/offers/invoice_request.rs` around lines 289 - 295, The pre-sign validation currently only runs when invoice_request.amount_msats is Some, so build_and_sign can sign requests where the payer omitted amount_msats but a deterministically resolvable bitcoin-denominated offer yields amount > MAX_VALUE_MSAT; update build_and_sign (or the signing path that references invoice_request) to resolve the effective msat amount when possible by calling invoice_request.amount_msats(currency_conversion) (or equivalent) and, if that returns Some(derived_msats), invoke offer.check_amount_msats_for_quantity(currency_conversion, Some(derived_msats), invoice_request.quantity) to enforce the same MAX_VALUE_MSAT check; ensure this only runs for deterministically resolvable cases (so fiat/late-bound requests that still return None are left unchanged.lightning/src/ln/outbound_payment.rs (2)
2154-2192:⚠️ Potential issue | 🟠 MajorPersist a full invoice identity for replays.
This replay path only keys idempotency on
payment_hash, butsend_payment_for_bolt12_invoicelater uses the newly supplied invoice's features, expiry, amount, and routing data. A second invoice with the samepayment_hashcan still mutate or abort the original flow. Store and compare a full invoice identity here — e.g.invoice.signable_hash()or the serialized invoice bytes — before treating the replay as idempotent.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lightning/src/ln/outbound_payment.rs` around lines 2154 - 2192, The idempotency check in mark_invoice_received_and_get_details only compares payment_hash; update the PendingOutboundPayment::InvoiceReceived state to persist a full invoice identity (e.g. invoice.signable_hash() or serialized bytes) when transitioning from PendingOutboundPayment::AwaitingInvoice and store it alongside payment_hash (modify the InvoiceReceived variant to include an invoice_identity field). In the InvoiceReceived match arm, compare the stored invoice_identity against the newly received invoice's identity (invoice.signable_hash() or same serialization) and return Bolt12PaymentError::DuplicateInvoice if the identities differ; otherwise treat it as idempotent and continue returning the existing fields. Ensure the creation site in the AwaitingInvoice branch sets the same invoice_identity value and propagate it through any clones/returns (function: mark_invoice_received_and_get_details; enum variant: PendingOutboundPayment::InvoiceReceived; field: invoice_identity).
1285-1295:⚠️ Potential issue | 🟡 MinorReturn a semantic mismatch here, not
UnknownRequiredFeatures.
amount_msats(invreq, currency_conversion)can now fail for a legitimate runtime conversion reason. This branch already abandons withPaymentFailureReason::UnexpectedError, so returningUnknownRequiredFeaturesmisclassifies the failure and makes the outward error disagree with the internal state transition.Minimal fix
let amount_msat = match InvoiceBuilder::<DerivedSigningPubkey>::amount_msats( invreq, currency_conversion, ) { Ok(amount_msat) => amount_msat, Err(_) => { // An amount must be provided explicitly in the invoice request or // remain derivable from the underlying offer at this stage. abandon_with_entry!(entry, PaymentFailureReason::UnexpectedError); - return Err(Bolt12PaymentError::UnknownRequiredFeatures); + return Err(Bolt12PaymentError::UnexpectedInvoice); }, };Based on learnings: fiat
InvoiceRequest::amount_msats(converter)resolves at call time and can legitimately fail when the supplied converter does not support the offer currency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lightning/src/ln/outbound_payment.rs` around lines 1285 - 1295, The Err(_) branch in InvoiceBuilder::<DerivedSigningPubkey>::amount_msats currently abandons the entry with PaymentFailureReason::UnexpectedError but then returns Bolt12PaymentError::UnknownRequiredFeatures; change the return to the appropriate semantic-mismatch Bolt12PaymentError variant (i.e., return a semantic-mismatch error rather than UnknownRequiredFeatures) so the outward error matches the internal abandon (keep the abandon_with_entry!(entry, PaymentFailureReason::UnexpectedError) line and only swap the returned Bolt12PaymentError).
🧹 Nitpick comments (1)
lightning/src/ln/async_payments_tests.rs (1)
63-63: Prefer the sender's configured converter when mutating the stored invoice request.Using a fresh
TestCurrencyConversionhere is fine today, but it can drift from whatever converternodes[0].nodeis actually configured to use. Reusing the node-owned converter keeps this test mutation aligned with the realpay_for_offerpath.♻️ Proposed cleanup
- let conversion = TestCurrencyConversion; - nodes[0].node.test_modify_pending_payment(&payment_id, |pmt| { if let PendingOutboundPayment::StaticInvoiceReceived { invoice_request, .. } = pmt { valid_invreq = Some(invoice_request.clone()); *invoice_request = offer .request_invoice( &nodes[0].keys_manager.get_expanded_key(), Nonce::from_entropy_source(nodes[0].keys_manager), &secp_ctx, payment_id, - &conversion, + nodes[0].node.currency_conversion, )You can then drop the now-unused import on Line 63. Based on learnings: In tests,
alice.node.currency_conversion(backed byTestCurrencyConversion) is the correct converter to use for USD offers.Also applies to: 1453-1479
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lightning/src/ln/async_payments_tests.rs` at line 63, The test currently constructs a fresh TestCurrencyConversion and mutates the stored invoice request with it; change the mutation to reuse the sender node's configured converter (e.g., use nodes[0].node.currency_conversion or alice.node.currency_conversion) so the test uses the same conversion logic as pay_for_offer, and remove the now-unused TestCurrencyConversion import. Update all similar occurrences (including the block mentioned around lines 1453-1479) to reference the node-owned converter instead of creating a new TestCurrencyConversion instance.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lightning/src/ln/channelmanager.rs`:
- Around line 8575-8593: Replace the hard-coded DefaultCurrencyConversion used
in the verified_invreq.amount_msats(...) call with self.currency_conversion to
match surrounding code and intent: in the block referencing verified_invreq,
call verified_invreq.amount_msats(&self.currency_conversion) instead of
&DefaultCurrencyConversion, keeping the same failure path
(fail_htlc!(claimable_htlc, payment_hash)) and leaving the has_amount_msats()
guard and debug_assert untouched so the unreachable Err branch is eliminated in
practice and currency conversion is consistent.
---
Duplicate comments:
In `@fuzz/src/invoice_request_deser.rs`:
- Around line 65-71: The call site still uses the old
InvoiceRequest::respond_with signature; update the fuzz target to construct or
use the new FuzzCurrencyConversion and pass it into InvoiceRequest::respond_with
so the generic CurrencyConversion parameter is satisfied. Locate the
FuzzCurrencyConversion type in this file and a call to respond_with(...) and
change that call to respond_with::<FuzzCurrencyConversion>(...) or pass an
instance as the new argument according to the current
InvoiceRequest::respond_with<CC: CurrencyConversion> signature so the compiler
sees a valid CurrencyConversion implementation.
In `@lightning/src/ln/outbound_payment.rs`:
- Around line 2154-2192: The idempotency check in
mark_invoice_received_and_get_details only compares payment_hash; update the
PendingOutboundPayment::InvoiceReceived state to persist a full invoice identity
(e.g. invoice.signable_hash() or serialized bytes) when transitioning from
PendingOutboundPayment::AwaitingInvoice and store it alongside payment_hash
(modify the InvoiceReceived variant to include an invoice_identity field). In
the InvoiceReceived match arm, compare the stored invoice_identity against the
newly received invoice's identity (invoice.signable_hash() or same
serialization) and return Bolt12PaymentError::DuplicateInvoice if the identities
differ; otherwise treat it as idempotent and continue returning the existing
fields. Ensure the creation site in the AwaitingInvoice branch sets the same
invoice_identity value and propagate it through any clones/returns (function:
mark_invoice_received_and_get_details; enum variant:
PendingOutboundPayment::InvoiceReceived; field: invoice_identity).
- Around line 1285-1295: The Err(_) branch in
InvoiceBuilder::<DerivedSigningPubkey>::amount_msats currently abandons the
entry with PaymentFailureReason::UnexpectedError but then returns
Bolt12PaymentError::UnknownRequiredFeatures; change the return to the
appropriate semantic-mismatch Bolt12PaymentError variant (i.e., return a
semantic-mismatch error rather than UnknownRequiredFeatures) so the outward
error matches the internal abandon (keep the abandon_with_entry!(entry,
PaymentFailureReason::UnexpectedError) line and only swap the returned
Bolt12PaymentError).
In `@lightning/src/offers/flow.rs`:
- Around line 825-826: The doc comment for the request-builder incorrectly
states that the provided CurrencyConversion is used to resolve fiat-denominated
amounts "while building the request"; update the wording to state that
fiat-denominated amounts are preserved on InvoiceRequest and conversion occurs
later when InvoiceRequest::amount_msats(converter) is evaluated. Mention
CurrencyConversion and InvoiceRequest::amount_msats explicitly so readers know
conversion happens at call time, not during request construction.
- Around line 1701-1736: The payment secret is created amount-less but the
static invoice paths are amount-bound; resolve the fixed/currency amount the
same way create_static_invoice_builder does (using offer, offer_nonce and
currency_conversion) to obtain amount_msat and pass Some(amount_msat) as the
amount parameter to inbound_payment::create_for_spontaneous_payment in
create_static_invoice_for_server instead of None so the payment_secret is bound
to the resolved amount; keep using the same duration/expiry logic and then call
create_static_invoice_builder with that payment_secret.
In `@lightning/src/offers/invoice_request.rs`:
- Around line 53-57: The doctest uses the old shorthand InvoiceRequestBuilder<_,
_> which no longer matches the struct signature; update the hidden example to
use the four-parameter form InvoiceRequestBuilder<'_, '_, _, _> (matching the
struct pub struct InvoiceRequestBuilder<'a, 'b, T, CC>) so the example compiles;
locate the helper usage in the doctest block that currently references
InvoiceRequestBuilder<_, _> and replace it with InvoiceRequestBuilder<'_, '_, _,
_>.
- Around line 289-295: The pre-sign validation currently only runs when
invoice_request.amount_msats is Some, so build_and_sign can sign requests where
the payer omitted amount_msats but a deterministically resolvable
bitcoin-denominated offer yields amount > MAX_VALUE_MSAT; update build_and_sign
(or the signing path that references invoice_request) to resolve the effective
msat amount when possible by calling
invoice_request.amount_msats(currency_conversion) (or equivalent) and, if that
returns Some(derived_msats), invoke
offer.check_amount_msats_for_quantity(currency_conversion, Some(derived_msats),
invoice_request.quantity) to enforce the same MAX_VALUE_MSAT check; ensure this
only runs for deterministically resolvable cases (so fiat/late-bound requests
that still return None are left unchanged.
---
Nitpick comments:
In `@lightning/src/ln/async_payments_tests.rs`:
- Line 63: The test currently constructs a fresh TestCurrencyConversion and
mutates the stored invoice request with it; change the mutation to reuse the
sender node's configured converter (e.g., use nodes[0].node.currency_conversion
or alice.node.currency_conversion) so the test uses the same conversion logic as
pay_for_offer, and remove the now-unused TestCurrencyConversion import. Update
all similar occurrences (including the block mentioned around lines 1453-1479)
to reference the node-owned converter instead of creating a new
TestCurrencyConversion instance.
🪄 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: 0c46df3d-084e-43aa-b384-2c7189dfde6e
📒 Files selected for processing (25)
fuzz/src/full_stack.rsfuzz/src/invoice_request_deser.rsfuzz/src/offer_deser.rslightning-background-processor/src/lib.rslightning-block-sync/src/init.rslightning-dns-resolver/src/lib.rslightning/src/ln/async_payments_tests.rslightning/src/ln/channelmanager.rslightning/src/ln/functional_test_utils.rslightning/src/ln/functional_tests.rslightning/src/ln/max_payment_path_len_tests.rslightning/src/ln/offers_tests.rslightning/src/ln/outbound_payment.rslightning/src/ln/reload_tests.rslightning/src/offers/currency.rslightning/src/offers/flow.rslightning/src/offers/invoice.rslightning/src/offers/invoice_request.rslightning/src/offers/merkle.rslightning/src/offers/mod.rslightning/src/offers/offer.rslightning/src/offers/refund.rslightning/src/offers/static_invoice.rslightning/src/offers/test_utils.rslightning/src/util/test_utils.rs
| if verified_invreq.has_amount_msats() { | ||
| // Only explicit payer-provided amounts act as a lower | ||
| // bound here. Omitted amounts are resolved into the | ||
| // invoice amount when the payee creates the invoice. | ||
| match verified_invreq | ||
| .amount_msats(&DefaultCurrencyConversion) | ||
| { | ||
| Ok(invreq_amt_msat) => { | ||
| if payment_data.total_msat < invreq_amt_msat { | ||
| fail_htlc!(claimable_htlc, payment_hash); | ||
| } | ||
| }, | ||
| Err(_) => { | ||
| debug_assert!(false); | ||
| fail_htlc!(claimable_htlc, payment_hash); | ||
| }, | ||
| } | ||
| } | ||
| verified_invreq |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify the semantics of has_amount_msats() to determine if DefaultCurrencyConversion usage is safe
echo "=== Searching for has_amount_msats definition ==="
ast-grep --pattern 'fn has_amount_msats($$$) {
$$$
}'
echo ""
echo "=== Searching in invoice_request.rs for amount-related methods ==="
rg -n -C5 'has_amount_msats' --type rust
echo ""
echo "=== Check how amount_msats is stored/determined in invoice requests ==="
rg -n -C3 'amount_msats.*inner' lightning/src/offers/invoice_request.rsRepository: shaavan/rust-lightning
Length of output: 10947
Replace DefaultCurrencyConversion with self.currency_conversion for consistency and clarity.
This code is technically safe due to the has_amount_msats() guard (which ensures only explicit msat amounts are checked), but using self.currency_conversion would:
- Align with the rest of the codebase
- Eliminate the unreachable error handler that calls
debug_assert!(false) - Make the intent clearer to future maintainers
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lightning/src/ln/channelmanager.rs` around lines 8575 - 8593, Replace the
hard-coded DefaultCurrencyConversion used in the
verified_invreq.amount_msats(...) call with self.currency_conversion to match
surrounding code and intent: in the block referencing verified_invreq, call
verified_invreq.amount_msats(&self.currency_conversion) instead of
&DefaultCurrencyConversion, keeping the same failure path
(fail_htlc!(claimable_htlc, payment_hash)) and leaving the has_amount_msats()
guard and debug_assert untouched so the unreachable Err branch is eliminated in
practice and currency conversion is consistent.
1 participant
rust-lightning#3833
Summary by CodeRabbit
New Features
Bug Fixes / Improvements
Tests
Documentation