Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Expose pending payments through
ChannelManager
#1873Expose pending payments through
ChannelManager
#1873Changes from all commits
c98f80d
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the
pending_outbound_payments
field inOutboundPayments
may be worth renaming to avoidself.pending_outbound_payments.pending_outbound_payments
.thoughts? cc // @jkczyz @valentinewallace @TheBlueMatt
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I would be supportive of reaming. Perhaps
id_to_pending_payment
,payment_by_id
, or the like? Open to other alternatives given we may want to be deliberate about where "pending" is used.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's leave it for another PR - there's a ton of code motion in
channelmanager.rs
and friends right now between lock changes, payment retries, etc. The smaller we can keep this PR the better.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@TheBlueMatt What's the use case for including these two? Looking back at #1157, seems the issue was originally filed to support the sample node. But a later comment noted using
PendingOutboundPayment
isn't sufficient: lightningdevkit/ldk-sample#40 (comment)If we instead just want anything pending, shouldn't
Retryable
be enough? Do we care that there are still inflight HTLCs if we've either given up on the payment or already have the preimage?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bump @TheBlueMatt
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops, sorry I missed this. I think we need to include the
Fulfilled
andAbandoned
cases here as one use-case for listing pending payments is to figure out if you need to retry a payment - if you don't see a payment there, you should (consider) retrying it (as noted at #1157 (comment)). If the payment has been fulfilled, but you have not yet seen/processed thePaymentSent
event, you may retry it, but shouldn't.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for clarifying @TheBlueMatt. I thought this was worth clarifying in docs too so I added it in d626653 for
ChannelManager::list_pending_payments
. Let me know if the wording can be improved/if I missed anything