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:
|
|
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, |
|
|
|
🔔 7th Reminder Hey @jkczyz! This PR has been waiting for your review. |
|
🔔 8th Reminder Hey @jkczyz! This PR has been waiting for your review. |
|
🔔 9th Reminder Hey @jkczyz! This PR has been waiting for your review. |
|
🔔 10th Reminder Hey @jkczyz! This PR has been waiting for your review. |
|
🔔 11th Reminder Hey @jkczyz! This PR has been waiting for your review. |
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.