Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add reason to Event::PaymentFailed and Event::ChannelClosed #260

Merged
merged 5 commits into from
Feb 23, 2024

Conversation

tnull
Copy link
Collaborator

@tnull tnull commented Feb 22, 2024

Fixes #259.
Fixes #247.

To provide some more insight why a payment failed, we expose the PaymentFailureReason in our event. We also expose the reason in ChannelClosed.

@tnull tnull force-pushed the 2024-02-add-payment-failed-reason branch 2 times, most recently from 573e0dc to 311c500 Compare February 22, 2024 10:20
@tnull tnull changed the title Add reason to Event::PaymentFailed Add reason to Event::PaymentFailed and Event::ChannelClosed Feb 22, 2024
@tnull tnull requested a review from jkczyz February 22, 2024 12:57
@tnull tnull force-pushed the 2024-02-add-payment-failed-reason branch from 3358062 to c4b53d5 Compare February 22, 2024 12:58
@tnull tnull force-pushed the 2024-02-add-payment-failed-reason branch from c4b53d5 to 1aab36a Compare February 22, 2024 15:34
@tnull
Copy link
Collaborator Author

tnull commented Feb 22, 2024

FWIW, flaky Python test should be unrelated.

@tnull tnull force-pushed the 2024-02-add-payment-failed-reason branch 2 times, most recently from 6704dad to f697dc7 Compare February 22, 2024 15:59
Copy link
Contributor

@jkczyz jkczyz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Please squash.

@tnull tnull force-pushed the 2024-02-add-payment-failed-reason branch from f697dc7 to a062cb2 Compare February 22, 2024 17:28
@tnull
Copy link
Collaborator Author

tnull commented Feb 22, 2024

LGTM. Please squash.

Squashed the fixup without further changes.

Copy link
Contributor

@jkczyz jkczyz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any thoughts on including the reason in PaymentStatus?

@tnull tnull merged commit 299620e into lightningdevkit:main Feb 23, 2024
12 of 13 checks passed
@tnull
Copy link
Collaborator Author

tnull commented Feb 23, 2024

Any thoughts on including the reason in PaymentStatus?

Yeah, maybe we should, although I kind of like the simplicity of PaymentStatus currently. In any case, I'd be hesitant to touch anything payment info related until we're clear where the 'bigger picture' is going as part the BOLT12 upgrade.

@tnull tnull mentioned this pull request Mar 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Expose PaymentFailureReason in PaymentFailed Channel Closed "message"
2 participants