Closed tnull closed 1 month ago
I'll likely need to fix this as part of addressing https://github.com/lightningdevkit/rust-lightning/pull/3085#discussion_r1670791548. Question is how should we structure the error type. Currently, we have:
We could expand this to include a variant for PaymentFailureReason
. But there are other places where we return Ok
when we may want to return an error. For instance, payment id not found, abandoned, or fulfilled already. Though maybe we should refactor this to hold the lock prior to calling find_route_and_send_payment
. This would eliminate those errors and possibly eliminate the need for PendingOutboundPayment::InvoiceReceived
. Thoughts?
Though maybe we should refactor this to hold the lock prior to calling
find_route_and_send_payment
. This would eliminate those errors and possibly eliminate the need forPendingOutboundPayment::InvoiceReceived
. Thoughts?
Hmm... but that would mean we'd need to hold the lock while calling find_route
.
@jkczyz if you want, I could take this off your hands!? I can expand the Bolt12PaymentError
to include a PaymentFailureReason
variant and handle the places Ok
is returned with more descriptive errors. Also, in pay_for_offer
abort the payment if the offer amount > the available channel liquidity. If that's the approach you'd be okay with 👍
@jkczyz if you want, I could take this off your hands!? I can expand the
Bolt12PaymentError
to include aPaymentFailureReason
variant and handle the placesOk
is returned with more descriptive errors. Also, inpay_for_offer
abort the payment if the offer amount > the available channel liquidity. If that's the approach you'd be okay with 👍
Discussing with @TheBlueMatt offline. Doesn't seem like this is the right approach. Instead, we need to refactor the code such that:
send_payment_for_bolt12_invoice
calls pay_route_internal
just like send_payment_internal
instead of calling find_route_and_send_payment
send_payment_for_bolt12_invoice
, send_payment_internal
, and find_route_and_send_payment
use a new utility to find the route to avoid repeating that code again as is already happening in the latter two functionssend_payment_for_bolt12_invoice
calls this new route finding helper before calling pay_route_internal
and uses an error enum that has two variants wrapping Bolt12PaymentError
and RetryableSendFailure
, respectivelysend_payment_for_bolt12_invoice
transitions the state from PendingOutboundPayment::InvoiceReceived
to PendingOutboundPayment::Retryable
instead of find_route_and_send_payment
It seems this will better align our BOLT11 and BOLT12 flows.
@slanesuke Feel free to take that on if you can make it a priority. We have some other work that is blocked on it. It should be mostly straightforward, though feel free to ping me on Discord if you have any questions. Otherwise, I may need to jump on it to unblock some other work.
send_payment_for_bolt12_invoice
calls this new route finding helper before callingpay_route_internal
and uses an error enum that has two variants wrappingBolt12PaymentError
andRetryableSendFailure
, respectively
Alternatively, we could probably just add another variant to Bolt12PaymentError
that wraps RetryableSendFailure
.
@jkczyz if you want, I could take this off your hands!? I can expand the
Bolt12PaymentError
to include aPaymentFailureReason
variant and handle the placesOk
is returned with more descriptive errors. Also, inpay_for_offer
abort the payment if the offer amount > the available channel liquidity. If that's the approach you'd be okay with 👍Discussing with @TheBlueMatt offline. Doesn't seem like this is the right approach. Instead, we need to refactor the code such that:
send_payment_for_bolt12_invoice
callspay_route_internal
just likesend_payment_internal
instead of callingfind_route_and_send_payment
send_payment_for_bolt12_invoice
,send_payment_internal
, andfind_route_and_send_payment
use a new utility to find the route to avoid repeating that code again as is already happening in the latter two functionssend_payment_for_bolt12_invoice
calls this new route finding helper before callingpay_route_internal
and uses an error enum that has two variants wrappingBolt12PaymentError
andRetryableSendFailure
, respectivelysend_payment_for_bolt12_invoice
transitions the state fromPendingOutboundPayment::InvoiceReceived
toPendingOutboundPayment::Retryable
instead offind_route_and_send_payment
It seems this will better align our BOLT11 and BOLT12 flows.
@slanesuke Feel free to take that on if you can make it a priority. We have some other work that is blocked on it. It should be mostly straightforward, though feel free to ping me on Discord if you have any questions. Otherwise, I may need to jump on it to unblock some other work.
@jkczyz thanks for the feedback! It sounds doable, I can take a stab at it and aim to open a PR by the end of the day tomorrow at the latest. If timing becomes/is an issue and it's more urgent, feel free to take care of it. Let me know, but either way works for me.
SGTM. I can work off a hacked interface change in the interim.
Now moved the second part to https://github.com/lightningdevkit/rust-lightning/issues/3174
1.When sending a payment via the async BOLT12 flow introduced in #3078, we would log-and-swallow any path finding errors and silently abandon the payment:https://github.com/lightningdevkit/rust-lightning/blob/669a4596a694ec966be0b8d0dcd37accd9adf867/lightning/src/ln/outbound_payment.rs#L1019-L1021
In order to tell the user what went wrong, it would be great to bubble up the
Err
fromfind_route_and_send_payment
rather than continuing and returningOk(())
.~~2. We should also early-abort sending in the regular
pay_for_offer
flow if we're certain we don't have sufficient liquidity (i.e. the offer amount surpasses the sum of our availablenext_outbound_htlc_limit_msat
s) ~~