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:
📝 WalkthroughWalkthroughAdds a CurrencyConversion trait with default/test implementations and threads a currency_conversion provider (generic CC: CurrencyConversion) through offers, invoice/request builders, offer resolution, ChannelManager, sync/background, fuzz, and test plumbing to resolve fiat amounts to millisatoshis. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant OffersFlow as OffersMessageFlow
participant Conv as CurrencyConversion
participant Invoice as InvoiceBuilder
participant ChanMgr as ChannelManager
Client->>OffersFlow: request CreateInvoice/Offer (with Amount)
OffersFlow->>Conv: query msats_per_minor_unit / tolerance
Conv-->>OffersFlow: msats per minor unit or Err
OffersFlow->>Invoice: create builder (..., &conversion)
Invoice->>Conv: amount_msats(invoice_request, &conversion)
Conv-->>Invoice: resolved msats or Err
Invoice-->>OffersFlow: built invoice/request (msat-resolved)
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 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.41.0)lightning/src/ln/channelmanager.rsThanks 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.
Adds a `CurrencyConversion` trait allowing users to provide logic for converting currency-denominated amounts into millisatoshis. LDK itself cannot perform such conversions as exchange rates are external, time-dependent, and application-specific. Instead, the conversion logic must be supplied by the user. This trait forms the foundation for supporting Offers denominated in fiat currencies while keeping exchange-rate handling outside the core protocol logic.
|
@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.
Makes OffersMessageFlow generic over a CurrencyConversion implementation, propagating the parameter through to ChannelManager. Upcoming changes will introduce currency conversion support in BOLT 12 message handling, which requires access to conversion logic from both ChannelManager and OffersMessageFlow. By threading the conversion abstraction through OffersMessageFlow now, subsequent commits can use it directly without introducing temporary plumbing or refactoring the type hierarchy later.
Extends `OfferBuilder` to allow creating Offers whose amount is denominated in a fiat currency instead of millisatoshis. To ensure such Offers can later be processed correctly, currency amounts may only be set when the caller provides a `CurrencyConversion` implementation capable of resolving the amount into millisatoshis. Since amount correctness checks are now performed directly in the amount setters, they can be removed from the `build()` method. This introduces the first layer of currency support in Offers, allowing them to be created with currency-denominated amounts.
To support currency-denominated Offers, the InvoiceRequest builder needs to resolve the Offer amount at multiple points during construction. This occurs when explicitly setting `amount_msats` and again when the InvoiceRequest is finalized via `build()`. To avoid repeatedly passing a `CurrencyConversion` implementation into these checks, the builder now stores a reference to it at creation time. This allows the builder to resolve currency-denominated Offer amounts whenever validation requires it. As part of this change, `InvoiceRequest::amount_msats()` is updated to use the provided `CurrencyConversion` to resolve the underlying Offer amount when necessary.
Adds currency conversion support when responding to an `InvoiceRequest` and constructing the `InvoiceBuilder`. When the underlying Offer specifies its amount in a currency denomination, the `CurrencyConversion` implementation is used to resolve the payable amount into millisatoshis and ensure the invoice amount satisfies the Offer's requirements. This reintroduces the currency validation intentionally skipped during `InvoiceRequest` parsing, keeping parsing focused on structural validation while enforcing amount correctness at the time the Invoice is constructed.
Adds tests covering Offers whose amounts are denominated in fiat currencies. These tests verify that: * currency-denominated Offer amounts can be created * InvoiceRequests correctly resolve amounts using CurrencyConversion * Invoice construction validates and enforces the payable amount This ensures the full Offer → InvoiceRequest → Invoice flow works correctly when the original Offer amount is specified in currency. Co-authored By: CodeX
shaavan
force-pushed
the
currency-review
branch
from
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
1 participant
rust-lightning#3833
Summary by CodeRabbit
New Features
Bug Fixes
Tests