Review: Support Currency-Based Offers and Async Invoice Handling via FlowEvents #10
Conversation
Adds the `CurrencyConversion` trait to allow users to define custom logic for converting fiat amounts into millisatoshis (msat). This abstraction lays the groundwork for supporting Offers denominated in fiat currencies, where conversion is inherently context-dependent.
This commit updates the Bolt12Invoice amount creation logic to utilize the `CurrencyConversion` trait, enabling more flexible and customizable handling of fiat-to-msat conversions. Reasoning The `CurrencyConversion` trait is passed upstream into the invoice's amount creation flow, where it is used to interpret the Offer’s currency amount (if present) into millisatoshis. This change establishes a unified mechanism for amount handling—regardless of whether the Offer’s amount is denominated in Bitcoin or fiat, or whether the InvoiceRequest specifies an amount or not.
We introduce this check in pay_for_offer, to ensure that if the offer amount is specified in currency, a corresponding amount to be used in invoice request must be provided. **Reasoning:** When responding to an offer with currency, we enforce that the invoice request must always include an amount. This ensures we never receive an invoice tied to a currency-denominated offer without a corresponding request amount. By moving currency conversion upfront into the invoice request creation where the user can supply their own conversion logic — we avoid pushing conversion concerns into invoice parsing. This significantly reduces complexity during invoice verification.
Previously, the `enqueue_invoice` function in the `Flow` component accepted a `Refund` as input and dispatched the invoice either directly to a known `PublicKey` or via `BlindedMessagePath`s, depending on what was available within the `Refund`. While this worked for the refund-based flow, it tightly coupled invoice dispatch logic to the `Refund` abstraction, limiting its general usability outside of that context. The upcoming commits will introduce support for constructing and enqueuing invoices from manually handled `InvoiceRequest`s—decoupled from the `Refund` flow. To enable this, we are preemptively introducing more flexible, destination-specific variants of the enqueue function. Specifically, the `Flow` now exposes two dedicated methods: - `enqueue_invoice_using_node_id`: For sending an invoice directly to a known `PublicKey`. - `enqueue_invoice_using_reply_paths`: For sending an invoice over a set of explicitly provided `BlindedMessagePath`s. This separation improves clarity, enables reuse in broader contexts, and lays the groundwork for more composable invoice handling across the Offers/Refund flow.
Adds an API to send an `InvoiceError` to the counterparty via the flow. This becomes useful with the introduction of Flow events in upcoming commits, where the user can choose to either respond to Offers Messages or return an `InvoiceError`. Note: Given the small scope of changes in this commit, we also take the opportunity to perform minor documentation cleanups in `flow.rs`.
Until now, offers messages were processed internally without exposing intermediate steps. This made it harder for callers to intercept or analyse offer messages before deciding how to respond to them. `FlowEvents` provide an optional mechanism to surface these events back to the user. With events enabled, the caller can manually inspect an incoming message, choose to construct and sign an invoice, or send back an InvoiceError. This shifts control to the user where needed, while keeping the default automatic flow unchanged.
📝 WalkthroughWalkthroughThese changes introduce a currency conversion abstraction and event-driven flow handling throughout the Lightning offers system. A new Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant OffersFlow as OffersMessageFlow
participant EventQueue as Flow Events Queue
participant Request as InvoiceRequest Handler
participant Invoice as Invoice Builder
Client->>OffersFlow: invoke with enable_events=true
activate OffersFlow
OffersFlow->>OffersFlow: receive_invoice_request()
OffersFlow->>Request: verify_invoice_request(request, responder)
activate Request
alt enable_events is true
Request->>EventQueue: push InvoiceRequestReceived event
rect rgb(200, 220, 240)
Note over Request,EventQueue: Async handling branch
end
Request-->>OffersFlow: AsynchronouslyHandleResponse
else enable_events is false
rect rgb(240, 240, 240)
Note over Request,Invoice: Synchronous handling
end
Request->>Invoice: create_invoice_builder_from_invoice_request_with_keys<br/>(currency_conversion, ...)
activate Invoice
Invoice-->>Request: InvoiceBuilder
deactivate Invoice
Request-->>OffersFlow: SendInvoice(verified_request)
end
deactivate Request
deactivate OffersFlow
Client->>OffersFlow: release_pending_flow_events()
OffersFlow-->>Client: [InvoiceRequestReceived{...}, ...]
Client->>Invoice: create_invoice_builder_from_invoice_request(...)<br/>with currency_conversion
activate Invoice
Invoice->>Invoice: amount_msats(invoice_request, currency_conversion)
Invoice-->>Client: InvoiceBuilder
deactivate Invoice
Client->>Invoice: build_and_sign()
Client->>Client: send invoice via reply_path
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes The changes span 8 files with heterogeneous modifications: a new currency conversion abstraction that threads through multiple systems, event-driven flow control with async handling, enhanced payment validation logic across channel manager, and coordinated test updates. Understanding requires tracing currency conversion propagation through builder APIs and payment flows, plus validating the new event queueing and async response mechanisms. Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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.
| use crate::offers::invoice_request::CurrencyConversion; | ||
| use crate::offers::invoice_request::InvoiceRequest; |
There was a problem hiding this comment.
Currency-conversion aware static_invoice_received looks correct; minor note on error mapping
Threading currency_conversion: CC with CC: Deref<Target = CurrencyConversion> into static_invoice_received and delegating amount derivation to InvoiceBuilder::<DerivedSigningPubkey>::amount_msats(invreq, currency_conversion) centralizes the BOLT‑12 amount semantics and matches the new currency‑aware builder API.
The only nit is the Err(_) branch from amount_msats: it debug_assert!s, abandons with PaymentFailureReason::UnexpectedError, but surfaces Bolt12PaymentError::UnknownRequiredFeatures. If this branch can ever be hit in practice (e.g., a custom CurrencyConversion returning an error), you might want to align the outward error variant with the internal PaymentFailureReason (or explicitly mark the branch as unreachable) for clearer debugging and caller expectations. Functionally it’s safe as written.
Also applies to: 1119-1185
🤖 Prompt for AI Agents
In lightning/src/ln/outbound_payment.rs around lines 26-27 and also covering
1119-1185, the review notes that the Err(_) branch from
InvoiceBuilder::amount_msats maps internally to
PaymentFailureReason::UnexpectedError but returns
Bolt12PaymentError::UnknownRequiredFeatures outward; update the error mapping so
the outward Bolt12PaymentError aligns with the internal PaymentFailureReason
(or, if the branch is truly impossible, replace the branch with an explicit
unreachable!(...) / debug_assert!(false, ...) and document that assumption).
Concretely: locate the Err(_) arm(s) in static_invoice_received where
amount_msats is called and either map the error to a Bolt12PaymentError variant
that corresponds to UnexpectedError (preserving the original error info in the
log) or change the arm to an unreachable path with a clear debug assertion and a
matching return in release builds.
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.