Propagate routing errors up to `send_payment_for_bolt12_invoice` about rust-lightning HOT 8 OPEN

SGTM. I can work off a hacked interface change in the interim.

from rust-lightning.

I'll likely need to fix this as part of addressing #3085 (comment). 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?

from rust-lightning.

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?

Hmm... but that would mean we'd need to hold the lock while calling find_route.

from rust-lightning.

@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 👍

from rust-lightning.

@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 👍

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 functions
• send_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, respectively
• send_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.

from rust-lightning.

• send_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, respectively

Alternatively, we could probably just add another variant to Bolt12PaymentError that wraps RetryableSendFailure.

from rust-lightning.

@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 👍

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 functions
• send_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, respectively
• send_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.

@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.

from rust-lightning.

Now moved the second part to #3174

from rust-lightning.