-
Notifications
You must be signed in to change notification settings - Fork 902
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
Self-pay support. #6399
Self-pay support. #6399
Conversation
9afcb79
to
47cff52
Compare
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.
Always good to look at your refactoring before arrive at the patch core.
I left a question while reading your code
34179d3
to
e8954cb
Compare
We want to allow this, so write a test. Signed-off-by: Rusty Russell <[email protected]>
… sat/msat. Signed-off-by: Rusty Russell <[email protected]>
It's not that uncommon to want to pass NULL through, for optional strings. Signed-off-by: Rusty Russell <[email protected]>
… on null columns The other _arr helpers allow this, and we want it for the next patch. Signed-off-by: Rusty Russell <[email protected]>
We'll need to do this for self-pay as well. Signed-off-by: Rusty Russell <[email protected]>
We open-coded initialization in two places, now we will want a third, it's time to clean up. Signed-off-by: Rusty Russell <[email protected]>
e8954cb
to
0b222e9
Compare
Clean these up: they were debug logs, but we want to pass this information back for self-payments. Also fixes "Attept" typo which altered tests! Signed-off-by: Rusty Russell <[email protected]>
We'll want this without an htlc_out, for self-pay. Signed-off-by: Rusty Russell <[email protected]>
Previously, the "payment" and "invoice" paths were completely separate, but this now calls both. It bypasses htlc_sets (and thus, cannot do MPP), and bypasses the hook too: the former is tied closely to HTLCs, and the hook is also very htlc-centric. Includes finishing unfinished sentence in sendpay man page, as a bonus. Signed-off-by: Rusty Russell <[email protected]> Changelog-Added: Plugins: `sendpay` now allows self-payment of invoices, by specifying an empty route.
This is the simplest solution, not the best, but there's significant risk in try to remove the "we have a path" assumption in the code pay code. Includes removing a `tal_steal` which was incorrect: the buffer has the same lifetime as the plugin, so if we steal it then things get messy when we free the struct payment. Signed-off-by: Rusty Russell <[email protected]> Changelog-Added: Plugins: `pay` will now pay your own invoices if you try.
0b222e9
to
f7a3d1d
Compare
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.
LGTM, and I had just a tiny comment.
I would like to run the fedi test on top of this PR to see if we are covering every case in the self-payment, otherwise to me, this can land ofc :)
if (invoices_find_by_rhash(ld->wallet->invoices, &inv_dbid, payment_hash)) { | ||
log_debug(ld->log, "ALREADY paid invoice %s", | ||
type_to_string(tmpctx, struct sha256, payment_hash)); | ||
*err = tal_fmt(ctx, "Already paid or expired invoice %s", | ||
type_to_string(tmpctx, struct sha256, payment_hash)); | ||
} else { | ||
*err = tal_fmt(ctx, "Unknown invoice %s", | ||
type_to_string(tmpctx, struct sha256, payment_hash)); | ||
} |
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.
we can write the if without {...}
here just to be consistent with the our code
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.
No, if there is multiline code I prefer {} for clarity. It's fine to omit for simple cases, ofc.
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.
ACK f7a3d1d
By popular request. The sendpay infrastructure isn't too bad, but the path to put it into the pay plugin is (perhaps) oversimplified. Still, it's designed not to break normal payments in any way.