Skip to content

Support Currency-Based Offers and Async Invoice Handling via FlowEvents#3833

Open
shaavan wants to merge 6 commits intolightningdevkit:mainfrom
shaavan:currency
Open

Support Currency-Based Offers and Async Invoice Handling via FlowEvents#3833
shaavan wants to merge 6 commits intolightningdevkit:mainfrom
shaavan:currency

Conversation

@shaavan
Copy link
Member

@shaavan shaavan commented Jun 7, 2025

This PR adds support for currency-denominated Offers in LDK’s BOLT 12 offer-handling flow.

Previously, Offers could only specify their amount in millisatoshis. However, BOLT 12 allows Offers to be denominated in other currencies such as fiat. Supporting this requires converting those currency amounts into millisatoshis at runtime when validating payments and constructing invoices.

Because exchange rates are external, time-dependent, and application-specific, LDK cannot perform these conversions itself. Instead, this PR introduces a CurrencyConversion trait which allows applications to provide their own logic for resolving currency-denominated amounts into millisatoshis. LDK remains exchange-rate agnostic and simply invokes this trait whenever a currency amount must be resolved.

To make this conversion logic available throughout the BOLT 12 flow, OffersMessageFlow is parameterized over a CurrencyConversion implementation and the abstraction is threaded through the offer handling pipeline.

With this in place:

  • OfferBuilder can now create Offers whose amounts are denominated in currencies instead of millisatoshis

InvoiceRequest handling can resolve Offer amounts when validating requests

InvoiceBuilder enforces that the final invoice amount satisfies the Offer’s requirements after resolving any currency denomination

Currency validation is intentionally deferred until invoice construction when necessary, keeping earlier stages focused on structural validation while ensuring the final payable amount is correct.

Tests are added to cover the complete Offer → InvoiceRequest → Invoice flow when the original Offer amount is specified in a currency.

@ldk-reviews-bot
Copy link

ldk-reviews-bot commented Jun 7, 2025

👋 Thanks for assigning @jkczyz as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

@shaavan
Copy link
Member Author

shaavan commented Jun 7, 2025

cc @jkczyz

@ldk-reviews-bot
Copy link

🔔 1st Reminder

Hey @joostjager! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

@joostjager
Copy link
Contributor

Is this proposed change a response to a request from a specific user/users?

@shaavan
Copy link
Member Author

shaavan commented Jun 11, 2025

Hi @joostjager!

This PR is actually a continuation of the original thread that led to the OffersMessageFlow: link to thread.

The motivation behind it was to provide users with the ability to handle InvoiceRequests asynchronously—just like we already allow for Bolt12Invoices. However, adding more events into the middle of the ChannelManager flow felt suboptimal.

So, as a first step, we worked on refactoring most of the Offers-related code out of ChannelManager into the new OffersMessageFlow (#3639). Now that the refactor is complete, this PR picks up the original goal again: to let users asynchronously handle both InvoiceRequests and Invoices. This not only gives them more flexibility in analyzing these Offer messages, but also opens the door for creating custom interfaces—for example, to support Offers in different currency denominations.

Hope that gives a clear picture of the intent behind this! Let me know if you have any thoughts or suggestions—would love to hear them. Thanks a lot!

@jkczyz
Copy link
Contributor

jkczyz commented Jun 11, 2025

Another use case is Fedimint, where they'll want to include their own payment hash in the Bolt12Invoice.

@valentinewallace
Copy link
Contributor

Another use case is Fedimint, where they'll want to include their own payment hash in the Bolt12Invoice.

Does Fedimint plan to use the OffersMessageFlow without a ChannelManager?

@jkczyz
Copy link
Contributor

jkczyz commented Jun 11, 2025

Does Fedimint plan to use the OffersMessageFlow without a ChannelManager?

I believe with one.

@ldk-reviews-bot
Copy link

🔔 2nd Reminder

Hey @joostjager! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

@ldk-reviews-bot
Copy link

🔔 3rd Reminder

Hey @joostjager! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

@ldk-reviews-bot
Copy link

🔔 4th Reminder

Hey @joostjager! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

@ldk-reviews-bot
Copy link

🔔 5th Reminder

Hey @joostjager! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

@ldk-reviews-bot
Copy link

🔔 6th Reminder

Hey @joostjager! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

@ldk-reviews-bot
Copy link

🔔 7th Reminder

Hey @joostjager! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

@ldk-reviews-bot
Copy link

🔔 8th Reminder

Hey @joostjager! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

@ldk-reviews-bot
Copy link

🔔 9th Reminder

Hey @joostjager! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

@ldk-reviews-bot
Copy link

🔔 10th Reminder

Hey @joostjager! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

@ldk-reviews-bot
Copy link

🔔 11th Reminder

Hey @joostjager! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

@jkczyz jkczyz removed the request for review from joostjager July 2, 2025 13:38
@jkczyz
Copy link
Contributor

jkczyz commented Jul 2, 2025

Removing @joostjager for now to stop bot spam. @shaavan and I have been working through some variations of this approach.

Copy link
Contributor

@vincenzopalazzo vincenzopalazzo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Concept ACK for me

I was just looking around to sync with this Offer Flow

@shaavan shaavan changed the title Introduce Event Model for Offers Flow Introduce Synchronous Currency Conversion Support in Offers Aug 2, 2025
@codecov
Copy link

codecov bot commented Aug 2, 2025

Codecov Report

❌ Patch coverage is 90.32258% with 33 lines in your changes missing coverage. Please review.
✅ Project coverage is 89.37%. Comparing base (6749bc6) to head (a4742bd).
⚠️ Report is 85 commits behind head on main.

Files with missing lines Patch % Lines
lightning/src/offers/flow.rs 79.01% 16 Missing and 1 partial ⚠️
lightning/src/offers/offer.rs 61.53% 5 Missing ⚠️
lightning/src/offers/invoice.rs 94.36% 3 Missing and 1 partial ⚠️
lightning/src/offers/invoice_request.rs 94.44% 3 Missing ⚠️
lightning/src/ln/channelmanager.rs 92.30% 1 Missing and 1 partial ⚠️
lightning/src/ln/offers_tests.rs 97.70% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3833      +/-   ##
==========================================
+ Coverage   89.34%   89.37%   +0.02%     
==========================================
  Files         180      180              
  Lines      138480   140045    +1565     
  Branches   138480   140045    +1565     
==========================================
+ Hits       123730   125164    +1434     
- Misses      12129    12295     +166     
+ Partials     2621     2586      -35     
Flag Coverage Δ
fuzzing 35.13% <4.10%> (-0.84%) ⬇️
tests 88.71% <90.32%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@shaavan shaavan force-pushed the currency branch 2 times, most recently from 4656246 to ab92369 Compare December 11, 2025 18:27
@shaavan
Copy link
Member Author

shaavan commented Dec 11, 2025

Updated .08 → .09

Thanks, @jkczyz - Changes:

  • Update fiat_to_msats function return type to align better with it's use case.
  • Cleaned up functions to be more clearer and idiomatic
  • Polish documentation.
  • Fix rustfmt

@shaavan
Copy link
Member Author

shaavan commented Dec 11, 2025

Updated .09 → .10

Changes:

  • Expanded pay_for_offer documentation.
  • Fix linting CI.

Copy link
Collaborator

@TheBlueMatt TheBlueMatt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a bit unclear on why the events are here, they don't seem complete and could presumably go in a different PR?

As for the currency conversion, presumably we want to also add support for currency conversion to sending invreqs? Given the intended use-case for currency conversion is mostly recurring invoices, a payer likely really wants to ensure that they're actually gonna pay $5 and not have the recipient decide that actually $5 is 1 BTC. We also need to figure out tolerance thresholds for that (are there any mentioned in the spec?) - someone trying to pay us $5 where we think its $4.99 is probably okay. Not quite sure what to do there, maybe including it as a part of the interface makes sense - rather than an amount, a min amount and a selected amount?

///
/// Once generated, these events are stored in the [`OffersMessageFlow`], where they can be
/// manually inspected and responded to.
pub enum OfferMessageFlowEvent {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, so is the intention that these cannot be used for those using ChannelManager? We don't currently expose the flow object itself in ChannelManager so these events wouldn't be reachable (and you can't set the config on them either).

@@ -15699,6 +15360,7 @@ where
InvoiceRequestVerifiedFromOffer::DerivedKeys(request) => {
let result = self.flow.create_invoice_builder_from_invoice_request_with_keys(
&self.router,
&DefaultCurrencyConversion,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So is the intention that some future PR will enable currency conversion for ChannelManager users? I'm not clear on why we're passing the trait impl in to each function rather than having the flow object own a CurrencyConversion (Deref).

/// Returning msats per major unit will be off by a factor of 10^exponent (e.g. 100× for USD).
///
/// This convention ensures amounts remain precise and purely integer-based when parsing and
/// validating BOLT12 invoice requests.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You might want to add some phrasing here noting that implementations should almost certainly rely on fetching and caching latest rates and should never fetch rates live in response to a query.

@shaavan
Copy link
Member Author

shaavan commented Dec 23, 2025

@TheBlueMatt

I'm a bit unclear on why the events are here, they don't seem complete and could presumably go in a different PR?

The reason the (currently incomplete) FlowEvents appear here is that they’re tied to a broader architectural decision we need to make around where currency conversion should live. The underlying question is whether currency conversion should be treated as:

  1. A synchronous (cache-based, just-in-case), flow-owned functionality, or
  2. A potentially just-in-time concern that may require asynchronous handling (via FlowEvents)?

This PR currently builds on the second approach, but before moving further I wanted to lay out the trade-offs and get your thoughts on which direction we should commit to.

Option A: Flow-owned (synchronous) currency conversion

In this approach, OfferMessageFlow (and transitively ChannelManager) is parameterized over a CurrencyConversion trait, allowing all internal logic to perform conversion directly.

Pros

  • Simple end-user API: implement one trait and conversion works everywhere
  • Uniform behavior across invoice request creation and handling
  • No coupling between FlowEvents and currency logic

Cons

  • Adds another parameterization to OfferMessageFlow, which trickles up to ChannelManager
  • Locks users into synchronous, cache-based conversion
  • No way to opt into just-in-time or async rate fetching

Option B: Event/handler-owned (async-capable) conversion

In this model, users who want currency conversion opt into manual FlowEvent handling. When an onion message (e.g., an InvoiceRequest) is received, a FlowEvent is emitted, allowing the user to perform conversion (sync or async) before constructing and sending the response.

Pros

  • Avoids additional ChannelManager parameterization
  • Enables async or externally-fetched conversion logic

Cons

  • Couples currency conversion to FlowEvents
  • Requires opting into a less-settled event API
  • Introduces some internal complexity (e.g., default/no-op handling paths)

Why FlowEvents appear in this PR

If we go with Option B, FlowEvents are the natural seam for introducing currency conversion into the core flow. If we choose Option A, these events can be deferred or split into a separate PR.

Before fleshing FlowEvents out further, I wanted to get your thoughts on which trade-off we should commit to.


As for the currency conversion, presumably we want to also add support for currency conversion to sending invreqs?

Makes sense. That would require additional API changes (e.g., passing conversion context into pay_for_offer) if we proceed with the event-based approach. That’s another reason I wanted to first settle the architectural direction.


We also need to figure out tolerance thresholds for that (are there any mentioned in the spec?)…

The spec doesn’t define a tolerance threshold, so this seems best left as a user-level decision. One possible approach would be to extend the user-implemented fiat_to_msat API to also convey a tolerance (e.g., returning (msat: u64, tolerance_percent: u8)), which LDK could then use when determining sufficiency (on both receive and send paths).

@shaavan shaavan requested a review from TheBlueMatt January 15, 2026 08:25
@TheBlueMatt
Copy link
Collaborator

Hmm, yea, its an interesting question. Naively it seems like going the async direction makes sense - fetching currency conversion is likely something that happens via the internet so it should be async. But once we complete the event pipeline (presumably by bubbling these new events up as top-level Events) I think the DoS risk of it is gonna be pretty severe. Once we issue an offer, someone can spam us with as many InvocieRequests as they want. While our message processing will get rate-limited and they shouldn't (in theory) be able to cause too much in the way of problems, it gets more complicated with events. Our Event pipeline (via the background processor) is currently entirely single-threaded and if we get a flood of events for something like currency conversion we'll end up blocking stuff like payment claims. Now, we can limit the event buffer size for onion message-related events, which is fine, but if currency conversion is actually async in response to every request even a fairly tight limit is gonna be too much. Thus, it seems likely we really want to force devs into caching the currency conversion values anyway.

On the flip side, doing currency conversion via a sync (cached) API doesn't seem so bad when you look at actual currency conversion APIs. Instead of being individual APIs that you hit to request the conversion from currency A to B, my glance through available options seems to mostly point to them being a single request that returns a JSON map of all the known currencies to their value in some fixed base currency (eg see https://openexchangerates.org/). Thus, in practice caching should be quite simple/a single up-front request.

This does leave some complexity in ensuring you dont block the node on startup waiting for the conversion API response but presumably we want a fallible API anyway...

@TheBlueMatt TheBlueMatt removed their request for review January 15, 2026 16:58
@shaavan
Copy link
Member Author

shaavan commented Jan 22, 2026

Thanks for the thoughtful points, @TheBlueMatt

On currency conversion specifically, I agree that a synchronous API is the better default. In practice it nudges users toward exactly the model they should be using anyway: cached, pre-fetched rates rather than just-in-time lookups.

That said, I wanted to discuss one broader issue around Flow events in general.

Our Event pipeline (via the background processor) is currently entirely single-threaded and if we get a flood of events for something like currency conversion we'll end up blocking stuff like payment claims

I think this is a very fair criticism of the event-based model as it stands. Even if we move currency conversion to a synchronous, flow-owned API, we still seem to want some mechanism that lets a user “catch” an InvoiceRequest or Invoice, analyse it based on custom logic before deciding how (or whether) to respond.

For example, @jkczyz mentioned a Fedimint use case that would require exactly this kind of interception and analysis.

One idea we had explored was keeping a separate event queue inside OffersMessageFlow, distinct from ChannelManager’s Event queue, and having the background processor drive a separate handler for it. But if event handling is fundamentally single-threaded anyway, that separation may not actually buy us much in practice.

So the one question left is this:

How should we introduce Flow-events, or a Flow-event-like mechanism, that allows users to asynchronously inspect and reason about InvoiceRequest / Invoice messages before responding, without creating DoS or head-of-line blocking risks for the rest of the node?

Would love to get your thoughts on this!

@shaavan shaavan requested a review from TheBlueMatt January 22, 2026 17:34
@ldk-reviews-bot
Copy link

🔔 1st Reminder

Hey @TheBlueMatt! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

@ldk-reviews-bot
Copy link

🔔 2nd Reminder

Hey @TheBlueMatt! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

@ldk-reviews-bot
Copy link

🔔 3rd Reminder

Hey @TheBlueMatt! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

@ldk-reviews-bot
Copy link

🔔 4th Reminder

Hey @TheBlueMatt! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

@TheBlueMatt
Copy link
Collaborator

Oops sorry took me a minute to get back to this. Its a really good question I don't have a great answer for.

In general in the OffersMessageFlow, ISTM we're generally "returning" the "events" from the methods that generate them - when code wants to manually process a Bolt12Invoice, that code code calls OffersMessageFlow::verify_bolt12_invoice and can then do what they want with it (similar to OffersMessageFlow::verify_invoice_request). There's no need for actual events as the code driving the OffersMessageFlow simply gets back the validation result and/or response message and can do what they want with it.

This doesn't, of course, help us in ChannelManager. There, we get back a message but are in the middle of an *OnionMessageHandler call and can't "return" the response to some downstream code. I think there's roughly three options for that side:

  • For non-default handling, we could use Events (relying on downstream code to clear them quickly and dropping incoming messages if the event queue gets too large). We definitely can't do this for any default message handling, and there's perf questions.
  • We could require some new trait, which implies a new generic bound on ChannelManager for probably-not-a-common-case, but at least the perf questions aren't so bad,
  • We could simply not allow for super non-default behavior in ChannelManager, requiring that folks who want to do strange things intercept the messages and pass them directly to OffersMessageFlow, handling the responses by hand. This is great cause we don't have to do anything but makes using the more manual options way more challenging.

IMO we should generally default to the last option, avoiding adding any specific handling until we have a real specific use-case in mind and can see how to implement for that clearly. eg the fedi logic may well often run without a channelmanager or at least can punt the offers message handling out to a flow anyway.

@TheBlueMatt TheBlueMatt removed their request for review February 2, 2026 17:34
shaavan added 4 commits March 6, 2026 23:16
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.
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.
shaavan added 2 commits March 6, 2026 23:53
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.
@shaavan
Copy link
Member Author

shaavan commented Mar 6, 2026

Updated .10 → .11

Changes:

  • Switch to the synchronous currency conversion approach.

See the updated PR description for full details.

@shaavan shaavan requested review from TheBlueMatt and jkczyz March 6, 2026 18:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants