Add currency conversion support for BOLT 12 offers#3833
Conversation
|
👋 Thanks for assigning @TheBlueMatt as a reviewer! |
|
cc @jkczyz |
|
🔔 1st Reminder Hey @joostjager! This PR has been waiting for your review. |
|
Is this proposed change a response to a request from a specific user/users? |
|
Hi @joostjager! This PR is actually a continuation of the original thread that led to the The motivation behind it was to provide users with the ability to handle So, as a first step, we worked on refactoring most of the Offers-related code out of 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! |
|
Another use case is Fedimint, where they'll want to include their own payment hash in the |
Does Fedimint plan to use the |
I believe with one. |
|
🔔 2nd Reminder Hey @joostjager! This PR has been waiting for your review. |
|
🔔 3rd Reminder Hey @joostjager! This PR has been waiting for your review. |
|
🔔 4th Reminder Hey @joostjager! This PR has been waiting for your review. |
|
🔔 5th Reminder Hey @joostjager! This PR has been waiting for your review. |
|
🔔 6th Reminder Hey @joostjager! This PR has been waiting for your review. |
|
🔔 7th Reminder Hey @joostjager! This PR has been waiting for your review. |
|
🔔 8th Reminder Hey @joostjager! This PR has been waiting for your review. |
|
🔔 9th Reminder Hey @joostjager! This PR has been waiting for your review. |
|
🔔 10th Reminder Hey @joostjager! This PR has been waiting for your review. |
|
🔔 11th Reminder Hey @joostjager! This PR has been waiting for your review. |
|
Removing @joostjager for now to stop bot spam. @shaavan and I have been working through some variations of this approach. |
vincenzopalazzo
left a comment
There was a problem hiding this comment.
Concept ACK for me
I was just looking around to sync with this Offer Flow
Codecov Report❌ Patch coverage is 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
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
🔔 19th Reminder Hey @TheBlueMatt @jkczyz! This PR has been waiting for your review. |
|
🔔 3rd Reminder Hey @TheBlueMatt @jkczyz! This PR has been waiting for your review. |
| /// Returns the conversion rate in **msats per minor unit** for the given | ||
| /// ISO-4217 currency code together with an application-defined tolerance, | ||
| /// expressed as a percentage. | ||
| fn msats_per_minor_unit(&self, iso4217_code: CurrencyCode) -> Result<(f64, u8), ()>; |
There was a problem hiding this comment.
The u8 for the tolerance is a bit restrictive. Let's define a Tolerance type with methods like from_bips. This will also serve to document the method's return type.
There was a problem hiding this comment.
| /// This convention ensures amounts remain precise and purely integer-based when parsing and | ||
| /// validating BOLT12 invoice requests. | ||
| pub trait CurrencyConversion { | ||
| /// Returns the conversion rate in **msats per minor unit** for the given |
There was a problem hiding this comment.
Not sure if we care about the precision if we are checking within a tolerance, but f64 may not be sufficient when multiplying by a large fiat value.
We could define ExchangeRatio where it internally uses a numerator and denominator. Normally, the denominator would be 1, but it could be larger (e.g., 100) for highly inflated currencies such that the numerator can also be larger. Maybe overkill if we have a tolerance.
| /// - `Ok(Some(range))` if the offer specifies an amount and it can be resolved. | ||
| /// - `Ok(None)` if the offer does not specify an amount. | ||
| /// - `Err(_)` if the amount cannot be resolved (e.g., unsupported currency). | ||
| pub fn resolve_offer_amount<CC: CurrencyConversion>(&$self, currency_conversion: &CC) -> Result<Option<MsatsRange>, Bolt12SemanticError> |
There was a problem hiding this comment.
Does this need to be public?
There was a problem hiding this comment.
I reasoned that this could be useful for external users directly dealing with Amount, allowing them to convert it to msats through a simple API.
That said, would love to hear your thoughts if you still think this should remain internal.
| pub struct MsatsRange { | ||
| pub(crate) amount_msats: u64, | ||
| pub(crate) tolerance: u8, | ||
| } |
There was a problem hiding this comment.
Meh... I'm not sold on this abstraction. Multiplying a range isn't very intuitive. And do we check / care about the maximum?
Seems the code would be more readable if resolve_offer_amount returned a tuple (amount_msats, tolerance) and the multiplication and tolerance check was done at the call site.
There was a problem hiding this comment.
With the move from tolerance: u8 to ExchangeRange in .18, I also removed the MsatsRange abstraction and moved multiplication and range checks to the use sites.
I think this makes the flow more explicit and avoids carrying arithmetic inside the range abstraction itself. Please see the comment for more details on the reasoning.
|
🔔 4th Reminder Hey @TheBlueMatt! This PR has been waiting for your review. |
|
🔔 5th Reminder Hey @TheBlueMatt! This PR has been waiting for your review. |
|
🔔 6th Reminder Hey @TheBlueMatt! This PR has been waiting for your review. |
|
🔔 7th Reminder Hey @TheBlueMatt! This PR has been waiting for your review. |
|
🔔 8th Reminder Hey @TheBlueMatt! This PR has been waiting for your review. |
|
🔔 9th Reminder Hey @TheBlueMatt! This PR has been waiting for your review. |
|
🔔 10th Reminder Hey @TheBlueMatt! This PR has been waiting for your review. |
|
🔔 11th Reminder Hey @TheBlueMatt! This PR has been waiting for your review. |
|
🔔 12th Reminder Hey @TheBlueMatt! This PR has been waiting for your review. |
TheBlueMatt
left a comment
There was a problem hiding this comment.
Any updates after @jkczyz's review above?
Add a `CurrencyConversion` trait for resolving currency-denominated amounts into millisatoshis. LDK cannot supply exchange rates itself, so applications provide this conversion logic as the foundation for fiat-denominated offer support.
Thread `CurrencyConversion` through `ChannelManager` type parameters and construction APIs. BOLT12 offer amounts will require currency conversion during invoice construction and payment validation. Wire the conversion dependency through `ChannelManager` now so later commits can use a single conversion path instead of duplicating conversion logic across call sites. Wire the dependency through the related test, fuzz, and helper scaffolding to support the new manager integration. AI-assisted: Dependency plumbing and scaffolding. Co-Authored-By: OpenAI Codex <codex@openai.com>
BOLT12 currency-denominated offers will require currency conversion during offer construction to validate the set amount. This commit handles the plumbing needed to introduce that behavior, threading `CurrencyConversion` through the relevant offer-building paths and scaffolding. The next commit will introduce the actual conversion logic. Keep the plumbing and logical changes separate to make the transition easier to review.
This commit completes the second part of currency conversion support for offers by adding validation for currency-denominated amounts during offer construction. With the plumbing introduced in the previous commit now in place, `OfferBuilder` can support currency-denominated offer amounts and expose the related amount-setting APIs publicly. Add minimal tests to verify that the public amount APIs work correctly and that fiat-denominated offers build successfully.
For currency-denominated offers, the payer may not reliably derive the final msat amount during invoice request creation. Instead, defer the final amount resolution to invoice creation (for the payee) and invoice handling (for the payer), where the currency conversion can be verified against the offer's fiat amount. This also updates a previously failing test to cover the new behavior. Additional reasoning is documented in the commit.
The old `InvoiceRequest` amount accessor blurred two different concepts: the amount explicitly carried in the request TLV and the amount derived from the offer and quantity. Callers had to pair `amount_msats` with `has_amount_msats` to determine whether the amount was actually present in the request or synthesized on read. Split those meanings into separate accessors: - `amount_msats()` returns the amount explicitly requested in the invoice request. - `payable_amount_msats()` returns the payable amount for the invoice request, deriving it from the offer when needed. As part of the recent currency conversion support, `payable_amount_msats()` now accepts a conversion trait parameter, allowing callers to derive payable amounts even when the offer is currency-denominated. This lays the groundwork for future commits adding currency conversion support to `Bolt12Invoice` creation and handling logic.
Thread CurrencyConversion through the InvoiceBuilder construction flow and the related upstream APIs. This sets up the plumbing needed for currency-denominated invoice handling without introducing the actual verification logic yet. The plumbing and logical changes are separated to make the transition easier to review. The next commit adds payer-side invoice amount verification, completing the end-to-end currency conversion flow.
Currency-denominated offers may not include an explicit msat amount in the invoice request. During invoice building, we now use the configured currency conversion to either validate the requested amount or derive the payable amount from the offer amount. This completes the currency conversion support on the payee side. The next commit adds payer-side invoice amount verification, completing the end-to-end currency conversion flow.
Add tests covering invoice request and invoice response handling for currency-denominated offers. This combines coverage for the standard flow that derives the final invoice amount through currency conversion and the insufficient-msat request path that must be rejected while building the invoice response. The merged test coverage exercises both the positive and deferred- validation paths for currency-denominated invoice responses. AI-assisted: Planning and writing the tests Co-Authored-By: OpenAI Codex <codex@openai.com>
This completes the invoice handling side of currency conversion support. When paying an invoice for a currency-denominated offer, and the invoice request did not specify an explicit amount, we now use the configured CurrencyConversion to derive the acceptable msat range for the offer amount. The invoice is considered valid only if the quoted amount falls within that acceptable range, preventing the payer from being overcharged due to exchange-rate differences or unexpected invoice amounts.
Add end-to-end and payer-side tests for currency-denominated offers and invoices. This consolidates coverage for the standard payment flow, excessive invoice rejection, unverifiable fiat invoices when conversion support is unavailable, and quantity-scaled invoice requests. The combined coverage exercises the main invoice amount verification paths introduced by currency-denominated offer support. AI-assisted: Planning and writing the tests Co-Authored-By: OpenAI Codex <codex@openai.com>
|
Updated .17 → .18 Thanks, @jkczyz - Changes:
Reasoning: I favoured returning an
See the |
|
|
||
| #[cfg(all(feature = "std", not(fuzzing)))] | ||
| let builder = invoice_request.respond_using_derived_keys(payment_paths, payment_hash); | ||
| let builder = invoice_request.respond_using_derived_keys(converter, payment_paths, payment_hash); |
There was a problem hiding this comment.
Bug (won't compile under no_std/fuzzing): The std path here correctly passes converter, but the no_std/fuzzing path at lines 1048-1052 does NOT pass converter as the first argument to respond_using_derived_keys_no_std. The function signature requires it:
pub fn respond_using_derived_keys_no_std<'a, CC: CurrencyConversion>(
&'a $self, converter: &'a CC, payment_paths: ..., payment_hash: ..., created_at: ...
)The same issue exists at lines 1114-1118 where respond_with_no_std is called without converter.
Both no_std and fuzzing builds will fail to compile.
|
|
||
| #[cfg(all(feature = "std", not(fuzzing)))] | ||
| let builder = invoice_request.respond_with(payment_paths, payment_hash); | ||
| let builder = invoice_request.respond_with(converter, payment_paths, payment_hash); |
There was a problem hiding this comment.
Same bug as above: the no_std/fuzzing path at lines 1114-1118 calls respond_with_no_std without converter as the first argument. Won't compile under cfg(any(not(feature = "std"), fuzzing)).
|
|
||
| let amount_msat = match InvoiceBuilder::<DerivedSigningPubkey>::amount_msats( | ||
| invreq, | ||
| invreq, converter, |
There was a problem hiding this comment.
Bug: Wrong error type and incorrect debug_assert in the error branch (lines 1308-1314). For currency-denominated offers where the payer's CurrencyConversion can't resolve the currency (e.g., NullCurrencyConversion), build_with_checks allows the invoice request to proceed without amount_msats (it catches UnsupportedCurrency). When InvoiceBuilder::amount_msats(invreq, converter) is called here, it will fail with UnsupportedCurrency.
This means:
- The
debug_assert!(false)at line 1311 will panic in debug builds — this path IS reachable for currency-denominated offers. Bolt12PaymentError::UnknownRequiredFeaturesat line 1313 is semantically wrong — this is an amount resolution failure, not a features issue.
Consider returning Bolt12PaymentError::UnverifiableAmount and removing or softening the debug_assert.
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
CurrencyConversiontrait 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,
OffersMessageFlowis parameterized over aCurrencyConversionimplementation and the abstraction is threaded through the offer handling pipeline.With this in place:
OfferBuildercan now create Offers whose amounts are denominated in currencies instead of millisatoshis•
InvoiceRequesthandling can resolve Offer amounts when validating requests•
InvoiceBuilderenforces that the final invoice amount satisfies the Offer’s requirements after resolving any currency denominationCurrency 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.