Various BOLT11 payment interface fixes #3810
Conversation
We previously changed the BOLT11 payment API, also introducing a new error type in `outbound_payment`. However, we didn't actualy re-export that in `ChannelManager`, so it's currently inaccessible.
.. as we'll reuse it in LDK Node, where we'll need `Debug`.
|
👋 Thanks for assigning @jkczyz as a reviewer! |
Previously, we refactored our BOLT11 payment API which made invoice-provided and user-provided amounts mutually exclusive. Here, we restore the original behavior that allows overpaying for invoices, even if they specify a (default) amount.
tnull
force-pushed
the
2025-05-fix-new-bolt11-payment-interface
branch
from
367d0cc to
41cccc3
Compare
Previoously, we added a bunch of constructors for `PaymentParameters` that would panic if we ever misused the wrong APIs on a `Clear` or `Blinded` payee. While this was not immediately reachable, we here refactor these constructors to avoid the risk going forward.
tnull
force-pushed
the
2025-05-fix-new-bolt11-payment-interface
branch
from
ab30ce6 to
b4da06c
Compare
| /// An invalid or incompatible invoice was provided to [`ChannelManager::pay_for_bolt11_invoice`]. | ||
| /// | ||
| /// [`ChannelManager::pay_for_bolt11_invoice`]: crate::ln::channelmanager::ChannelManager::pay_for_bolt11_invoice | ||
| InvalidInvoice, |
There was a problem hiding this comment.
Why do we want to add this if it will never be created? It should be an invariant of from_bolt11_invoice that it will never fail. We can change the unwraps to expects, IMO.
There was a problem hiding this comment.
Hmm, I agree these are not reachable at the moment, but it's pretty risky to have unwraps and expects sprinkled over the codebase that could get reachable, e.g., by a refactoring in the future (it would just take introducing one more error condition that depends on the user input in with_route_hints or with_{bolt11,bolt12}_features and we'd get reachable panics in the corresponding from_ method(s)).
IMO, unwraps (outside of trivial ones like on lock), should generally be avoided.
If you dislike the extra error variant I guess we could also see if there are other ways to make this safer (more comments, debug_asserts, maybe tests), or through the type system, so open for suggestions.
There was a problem hiding this comment.
Yeah, ideally through the type system where the Payee enum is pulled out of PaymentParameters. Probably something like:
pub enum Payee {
Bolt11(Bolt11Payee),
Bolt12(Bolt12Payee),
}
pub struct Bolt11Payee {
node_id: PublicKey,
route_hints: Vec<RouteHint>,
features: Option<Bolt11InvoiceFeatures>,
final_cltv_expiry_delta: u32,
params: PaymentParameters,
}
pub struct Bolt12Payee {
route_hints: Vec<BlindedPaymentPath>,
features: Option<Bolt12InvoiceFeatures>,
params: PaymentParameters,
}And where RouteParameters holds a Payee instead of PaymentParameters.
| /// An invalid or incompatible invoice was provided to [`ChannelManager`]. | ||
| /// | ||
| /// [`ChannelManager`]: crate::ln::channelmanager::ChannelManager | ||
| InvalidInvoice, |
|
👋 The first review has been submitted! Do you think this PR is ready for a second reviewer? If so, click here to assign a second reviewer. |
| .with_expiry_time(invoice.created_at().as_secs().saturating_add(invoice.relative_expiry().as_secs())) | ||
| /// | ||
| /// Will return an error if an incompatible invoice is provided. | ||
| pub fn from_bolt12_invoice(invoice: &Bolt12Invoice) -> Result<Self, ()> { |
There was a problem hiding this comment.
Honestly, why is this public to begin with? Presumably no one should ever use it directly?
|
|
||
| if let Some(expiry) = invoice.expires_at() { | ||
| payment_params = payment_params.with_expiry_time(expiry.as_secs()); | ||
| } | ||
| if let Some(features) = invoice.features() { | ||
| payment_params = payment_params.with_bolt11_features(features.clone()).unwrap(); | ||
| payment_params = payment_params.with_bolt11_features(features.clone())?; |
There was a problem hiding this comment.
Okay, but neither of these calls an fail, right? Why bother returning a result for an infallible operation.
Recently, we refactored our BOLT11 payments interface, which unfortunately broke a few things.
Here, we collect a few smaller fixes that allow us to use the API downstream in LDK Node.
Also note that the APIs for spontaneous payments and payment probing are now somewhat inconsistent with the remaining BOLT11/BOLT12 payment APIs, which now take a
RouteParametersConfigfor instance. We may want to refactor them, although at least for the case of spontaneous payments that would be a larger refactor best done in a follow-up, IMO.