Add currency conversion support for BOLT 12 offers#3833
Add currency conversion support for BOLT 12 offers#3833shaavan wants to merge 6 commits intolightningdevkit:mainfrom
Conversation
|
👋 Thanks for assigning @jkczyz 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
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
shaavan
changed the title
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:
|
|
🔔 1st Reminder Hey @TheBlueMatt @jkczyz! This PR has been waiting for your review. |
1 similar comment
|
🔔 1st Reminder Hey @TheBlueMatt @jkczyz! This PR has been waiting for your review. |
TheBlueMatt
left a comment
TheBlueMatt
left a comment
There was a problem hiding this comment.
I think this looks good, yea.
| /// 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.
Is this actually enough precision? For USD its fine (result of ~14K these days), but there's some currencies where this is a much lower value (Lebanese Pound this is something like 16 currently, afaict). If LBP inflates a bit more its pretty easy to see this getting into the mid/low single digits, which would make this too low precision.
There was a problem hiding this comment.
I can think of a few possible approaches to address this:
-
Return
msats_per_major_unit.
This would be the simplest option. However, it doesn't fully solve the precision issue for very weak currencies such as LBP, where even the major unit may still map to a relatively small number of msats. -
Return
microsats_per_minor_unit.
This increases the resolution while keeping the API relatively simple. It should provide sufficient precision for most currencies in the near future without significantly complicating the interface. -
Return a rational conversion rate, e.g.
(numerator: u128, denominator: u128), where:
1 CUR = (numerator / denominator) * 1 msat
This is the most flexible approach and would preserve precision even for extremely weak currencies. The trade-off is that it introduces a bit more complexity to the API.
Happy to hear thoughts on which direction would make the most sense here.
There was a problem hiding this comment.
Note that if define this in terms of major unit, then we'd need to know all the currency exponents since the amount is in terms of minor unit. We could make CurrencyCode an enum for this if we want to go in that direction.
There was a problem hiding this comment.
I think we should definitely keep it in terms of minor unit. I generally don't love floats, but this does seem like a case where we should just return an f64 and call it a day.
There was a problem hiding this comment.
Let me update the branch with the float version and see how it looks in action.
| /// This represents a user-level policy (e.g., allowance for exchange-rate | ||
| /// drift or cached data) and does not directly affect fiat-to-msat conversion | ||
| /// outside of range computation. | ||
| fn tolerance_percent(&self) -> u8; |
There was a problem hiding this comment.
This doesn't appear to be called in the current patchset? Also the docs aren't nearly clear enough on how this is (will be?) used.
There was a problem hiding this comment.
I kept tolerance_percent here mainly to discuss whether this is the right place to introduce the tolerance mechanism.
The idea is to account for possible exchange-rate drift between the payer and the payee by deriving a small acceptable range around the converted msat value. The intended usage would roughly be as follows.
If we are the payee and receive an InvoiceRequest for a fiat-denominated offer:
If the amount is specified in the InvoiceRequest, we accept it as long as:
invoice_request_amount ≥ offer.resolved_amount() - tolerance
If the amount is not specified in the InvoiceRequest, we simply set:
invoice_amount = offer.resolved_amount()
If we are the payer, we can set:
invoice_request_amount = offer.resolved_amount() + tolerance
This gives the payee some headroom when constructing the invoice, ensuring the payment does not fail if the exchange rate shifts slightly between the time the InvoiceRequest is created and when the invoice is issued.
There was a problem hiding this comment.
I kept tolerance_percent here mainly to discuss whether this is the right place to introduce the tolerance mechanism.
Should it just be a second return value from the conversion method? Not really sure why someone would want to but they could even have different tolerance values for different currencies. Doesn't really seem like we need a second method.
If we are the payer, we can set:
I think the spec should clarify this. Its weird that we always pessimistically overpay. Honestly IMO the receiver should always resolve the amount (ie as the payer we'd never set anything), cause ultimately its the receiver who decides what the amount is they want to receive for the given offer, not the payer.
There was a problem hiding this comment.
Should it just be a second return value from the conversion method?
That feels like a cleaner design. I’ll experiment with it and see how it plays out in practice.
I think the spec should clarify this.
That’s a fair point, and I can see the value in having clearer guidance at the spec level.
That said, I’m not sure this needs to be fully specified at the protocol level. Tolerance feels more like an application-level decision, since different payers may have different acceptable ranges for conversion drift.
Standardizing a single tolerance model in the spec could end up being too restrictive, especially given that exchange rates, rounding strategies, and risk preferences can vary across implementations.
Perhaps a middle ground could be for the spec to acknowledge that some tolerance is expected, without prescribing how it should be handled.
Honestly IMO the receiver should always resolve the amount (ie as the payer we'd never set anything)
I agree with the intuition here that the receiver is ultimately the one defining the amount they expect to receive.
At the same time, whether the payer or the payee defines the amount doesn’t fundamentally remove the need for the payer to perform conversion locally and apply some tolerance when validating the invoice. Exchange rates and rounding differences still require the payer to maintain its own acceptance range.
So even in a payee-resolved model, that tolerance or slight overpayment doesn’t really go away, it just shifts to a later stage.
Where this starts to matter more is at the implementation level.
In LDK’s current structure, not specifying an amount in the invoice request pushes all currency validation to invoice parsing time. However, our try_from parsing layer does not have access to a currency conversion source, which means we can’t enforce conversion correctness at parse time.
That either weakens our ability to provide deterministic correctness guarantees after parsing, or requires restructuring parsing to depend on an external conversion source.
By specifying the amount earlier in the flow, we’re able to validate it within the existing parsing model and guarantee correctness deterministically for invoice requests constructed by LDK, as shown here: [Reference](https://github.com/shaavan/rust-lightning/blob/currency/lightning/src/offers/invoice.rs#L1790-L1804)
Given that, while a payee-resolved model does feel conceptually cleaner, specifying the amount at the payer side ends up being a more robust and verifiable approach under the current constraints.
Curious to hear your thoughts on this.
| InvoiceBuilder::<DerivedSigningPubkey>::amount_msats(&invoice_request.inner)?; | ||
| let amount_msats = InvoiceBuilder::<DerivedSigningPubkey>::amount_msats( | ||
| &invoice_request.inner, | ||
| conversion, |
There was a problem hiding this comment.
Do we want to allow for a more flexible API in the flow itself? eg leaving amount_msats alone and having the logic here do the currency conversion by hand instead so that someone could use an OffersMessageFlow with their own currency conversion logic?
There was a problem hiding this comment.
Isn't that what parameterizing the flow achieves? Or are you suggesting having each method take a currency conversion rate, which the user would pass in based on the Offer's currency? Also, passing 1 for when the currency is bitcoin.
There was a problem hiding this comment.
Or are you suggesting having each method take a currency conversion rate, which the user would pass in based on the Offer's currency? Also, passing 1 for when the currency is bitcoin.
Yea, this is what I was thinking. Avoids parameterizing the flow and letting the flow be more "manual"/configurable.
|
Updated .11 → .12 Changes:
|
|
🔔 2nd Reminder Hey @jkczyz! This PR has been waiting for your review. |
|
🔔 3rd Reminder Hey @jkczyz! This PR has been waiting for your review. |
|
🔔 4th Reminder Hey @jkczyz! This PR has been waiting for your review. |
|
🔔 5th Reminder Hey @jkczyz! This PR has been waiting for your review. |
|
🔔 6th Reminder Hey @jkczyz! This PR has been waiting for your review. |
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.
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.
|
Rebased: .12 → .13 |
| pub fn amount_msats($($self_mut)* $self: $self_type, amount_msats: u64) -> Result<$return_type, Bolt12SemanticError> { | ||
| if amount_msats > MAX_VALUE_MSAT { | ||
| return Err(Bolt12SemanticError::InvalidAmount); | ||
| } |
There was a problem hiding this comment.
Bug: amount_msats(0) is accepted here because the check is only > MAX_VALUE_MSAT, but the test builds_offer_with_amount at line 1763 expects amount_msats(0) to return Err(Bolt12SemanticError::InvalidAmount). The parser at line 1367 correctly rejects 0-amount offers, creating an inconsistency between builder and parser. A zero-amount offer is invalid per BOLT 12.
| pub fn amount_msats($($self_mut)* $self: $self_type, amount_msats: u64) -> Result<$return_type, Bolt12SemanticError> { | |
| if amount_msats > MAX_VALUE_MSAT { | |
| return Err(Bolt12SemanticError::InvalidAmount); | |
| } | |
| pub fn amount_msats($($self_mut)* $self: $self_type, amount_msats: u64) -> Result<$return_type, Bolt12SemanticError> { | |
| if amount_msats == 0 || amount_msats > MAX_VALUE_MSAT { | |
| return Err(Bolt12SemanticError::InvalidAmount); | |
| } |
| 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); |
There was a problem hiding this comment.
Same issue: amount() with Amount::Bitcoin { amount_msats: 0 } passes this check since 0 > MAX_VALUE_MSAT is false. Also, Amount::Currency { ..., amount: 0 } would pass if msats_per_minor_unit returns 0 (since 0 * anything = 0). The parser rejects both of these at line 1367/1372. Should add a zero-amount check here for consistency.
| 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); | |
| pub fn amount<CC: CurrencyConversion>($($self_mut)* $self: $self_type, amount: Amount, currency_conversion: &CC) -> Result<$return_type, Bolt12SemanticError> | |
| { | |
| let msats = amount.into_msats(currency_conversion)?; | |
| if msats == 0 || msats > MAX_VALUE_MSAT { | |
| return Err(Bolt12SemanticError::InvalidAmount); | |
| } |
| .inner | ||
| .offer | ||
| .resolve_offer_amount(currency_conversion)? | ||
| .ok_or(Bolt12SemanticError::UnsupportedCurrency)?; |
There was a problem hiding this comment.
Bug: Wrong error variant. When resolve_offer_amount returns Ok(None) (meaning the offer has no amount set at all), this maps it to Bolt12SemanticError::UnsupportedCurrency. The correct error is Bolt12SemanticError::MissingAmount — the issue is that neither the invoice request nor the offer specifies an amount, not that currency conversion failed.
| .ok_or(Bolt12SemanticError::UnsupportedCurrency)?; | |
| .ok_or(Bolt12SemanticError::MissingAmount)?; |
| pub(crate) fn amount_msats<CC: CurrencyConversion>( | ||
| invoice_request: &InvoiceRequest, currency_conversion: &CC, | ||
| ) -> Result<u64, Bolt12SemanticError> { | ||
| match invoice_request.contents.inner.amount_msats() { | ||
| Some(amount_msats) => Ok(amount_msats), | ||
| None => match invoice_request.contents.inner.offer.amount() { | ||
| Some(Amount::Bitcoin { amount_msats }) => amount_msats | ||
| .checked_mul(invoice_request.quantity().unwrap_or(1)) | ||
| .ok_or(Bolt12SemanticError::InvalidAmount), | ||
| Some(Amount::Currency { .. }) => Err(Bolt12SemanticError::UnsupportedCurrency), | ||
| None => Err(Bolt12SemanticError::MissingAmount), | ||
| }, | ||
| let quantity = invoice_request.quantity().unwrap_or(1); | ||
| let requested_msats = invoice_request.amount_msats(currency_conversion)?; | ||
|
|
||
| let minimum_offer_msats = match invoice_request | ||
| .resolve_offer_amount(currency_conversion)? | ||
| { | ||
| Some(unit_msats) => Some( | ||
| unit_msats.checked_mul(quantity).ok_or(Bolt12SemanticError::InvalidAmount)?, | ||
| ), | ||
| None => None, | ||
| }; | ||
|
|
||
| if let Some(minimum) = minimum_offer_msats { | ||
| if requested_msats < minimum { | ||
| return Err(Bolt12SemanticError::InsufficientAmount); | ||
| } | ||
| } | ||
|
|
||
| if requested_msats > MAX_VALUE_MSAT { | ||
| return Err(Bolt12SemanticError::InvalidAmount); | ||
| } | ||
|
|
||
| Ok(requested_msats) | ||
| } |
There was a problem hiding this comment.
TOCTOU risk: This method calls currency_conversion twice independently — once via invoice_request.amount_msats(currency_conversion) at line 405, and again via invoice_request.resolve_offer_amount(currency_conversion) at line 408. If the CurrencyConversion implementation returns live exchange rates, the two calls may get different rates, causing the requested_msats < minimum check at line 417 to operate on inconsistent data.
Consider resolving the offer amount once and reusing it, or documenting that CurrencyConversion implementations must be stable within a single call context.
| /// Returns the acceptable tolerance, expressed as a percentage, used when | ||
| /// deriving conversion ranges. | ||
| /// | ||
| /// This represents a user-level policy (e.g., allowance for exchange-rate | ||
| /// drift or cached data) and does not directly affect fiat-to-msat conversion | ||
| /// outside of range computation. | ||
| fn tolerance_percent(&self) -> u8; |
There was a problem hiding this comment.
tolerance_percent() is defined in the trait but never called anywhere in the codebase. It's dead code in this PR. If the intent is for downstream code to use it, consider documenting where/how, or remove it until it's actually needed to keep the API surface minimal.
| match verified_invreq | ||
| .amount_msats(&self.flow.currency_conversion) | ||
| { | ||
| if payment_data.total_msat < invreq_amt_msat { | ||
| Ok(invreq_amt_msat) => { | ||
| if payment_data.total_msat < invreq_amt_msat { | ||
| fail_htlc!(claimable_htlc, payment_hash); | ||
| } |
There was a problem hiding this comment.
At HTLC claim time, amount_msats() resolves the currency-denominated offer amount using the current exchange rate, which may differ significantly from the rate used when the invoice was created. This could cause legitimate payments to be failed (if the rate increased) or allow underpayments (if the rate decreased).
For currency-denominated offers where the invoice request has no explicit amount_msats set by a remote payer, the check here becomes rate-dependent in a way that may not match the invoice amount. The comment at lines 8551-8558 acknowledges this can only happen for our own offers, but the logic should ideally compare against the invoice's amount_msats (which was locked in at invoice creation) rather than re-resolving from the exchange rate.
| 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 catch of UnsupportedCurrency is fragile: it silently swallows any UnsupportedCurrency error, but this error can also be produced by into_msats when the currency conversion legitimately fails (not just because DefaultCurrencyConversion doesn't support it). If in the future DefaultCurrencyConversion starts supporting some currencies, the semantics of this catch change silently.
Consider using a distinct sentinel (e.g., a dedicated method like is_currency_denominated()) to check whether to skip validation, rather than catching a specific error variant that has multiple possible causes.
| fn msats_per_minor_unit(&self, _iso4217_code: CurrencyCode) -> Result<u64, ()> { | ||
| unreachable!() | ||
| } | ||
|
|
||
| fn tolerance_percent(&self) -> u8 { | ||
| unreachable!() | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Using unreachable!() in a fuzz target is dangerous — if the fuzzer ever manages to construct an input that reaches this code (e.g., a valid currency-denominated offer), it will panic and be treated as a crash/finding rather than gracefully handling the case. Consider returning Err(()) and 0 instead, matching the pattern used in FuzzCurrencyConversion in full_stack.rs.
| let payment_id = PaymentId([1; 32]); | ||
| let conversion = DefaultCurrencyConversion; | ||
|
|
||
| let mut builder = offer.request_invoice(&expanded_key, nonce, &secp_ctx, payment_id)?; |
There was a problem hiding this comment.
Bug: Missing currency_conversion argument. request_invoice now requires a &CC parameter as the last argument, but &conversion is not passed here. This won't compile.
| let mut builder = offer.request_invoice(&expanded_key, nonce, &secp_ctx, payment_id)?; | |
| let mut builder = offer.request_invoice(&expanded_key, nonce, &secp_ctx, payment_id, &conversion)?; |
| /// fee_estimator, chain_monitor, tx_broadcaster, router, message_router, currency_conversion, | ||
| /// logger, entropy_source, node_signer, signer_provider, config.clone(), params, current_timestamp, |
There was a problem hiding this comment.
Nit: Mixed indentation — line 2101 uses spaces, line 2102 uses a tab. Should be consistent.
| /// fee_estimator, chain_monitor, tx_broadcaster, router, message_router, currency_conversion, | |
| /// logger, entropy_source, node_signer, signer_provider, config.clone(), params, current_timestamp, | |
| /// fee_estimator, chain_monitor, tx_broadcaster, router, message_router, currency_conversion, | |
| /// logger, entropy_source, node_signer, signer_provider, config.clone(), params, current_timestamp, |
|
|
8 participants
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.