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.

Sign up for GitHub

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

Jump to bottom

multi: hold keysend payments #4167

Merged
merged 1 commit into from
Jun 30, 2020
Merged

multi: hold keysend payments #4167

merged 1 commit into from
Jun 30, 2020

Conversation

joostjager
Copy link
Contributor

@joostjager joostjager commented Apr 8, 2020 •
edited
Loading

Adds a new option to lnd to hold received keysend payments in the accepted state for a configured period of time. During this time, an application can inspect the payment parameters and decide whether to cancel or settle this payment.

Example: a keysend payment with an embedded order comes in. The payment is held and an external application checks that the paid amount is sufficient for the ordered goods. If not, the payment is canceled without the need to refund anything. If the amount is sufficient, the payment is settled and the order processed.

This PR makes development of applications like tlvshop.com possible without forking lnd.

@joostjager joostjager force-pushed the hold-keysend branch 4 times, most recently from 76c9316 to b470334 Compare April 9, 2020 11:41
@joostjager joostjager changed the title multi: add explicit hodl invoice flag to invoice multi: hold keysend payments Apr 9, 2020
@joostjager joostjager force-pushed the hold-keysend branch 2 times, most recently from d89a44b to 2464fdf Compare April 9, 2020 14:18
@Roasbeef Roasbeef added this to the 0.11.0 milestone Apr 14, 2020
@Roasbeef Roasbeef added enhancement Improvements to existing features / behaviour payments Related to invoices/payments v0.11 labels Apr 14, 2020
@joostjager joostjager marked this pull request as ready for review April 15, 2020 09:06
@joostjager joostjager removed the request for review from Roasbeef April 15, 2020 09:07
@joostjager
Copy link
Contributor Author

Looking for concept ack

Roasbeef
Roasbeef reviewed May 1, 2020
View reviewed changes
@joostjager joostjager force-pushed the hold-keysend branch 3 times, most recently from 94595bf to 01e486f Compare May 6, 2020 18:29
cfromknecht
cfromknecht reviewed May 6, 2020
View reviewed changes
Copy link
Contributor

@cfromknecht cfromknecht left a comment

Choose a reason for hiding this comment

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

quick pass. main thing for this pr is keysends payments should imo be settled by payment addr, o/w there will be different flows for experimental keysend and AMP keysends. however, i think that first requires a change to start indexing payments by payment address. i have some commits that do this in a branch for AMP, i can throw that up

@joostjager joostjager force-pushed the hold-keysend branch 3 times, most recently from 3bc422c to e603862 Compare May 7, 2020 13:21
@joostjager
Copy link
Contributor Author

Discussed offline with @cfromknecht. Steps:

  • First merge PR that adds a payment address index to the invoice database
  • Convert on-the-fly keysend invoice insertion to add a payment addr if not yet present. To prevent hodl-keysends without payment addr from ending up in the database while we already know that we eventually won't support this combination. This is to keep users on master safe.
  • Allow settling of keysend invoices via payment address

@cfromknecht cfromknecht mentioned this pull request May 13, 2020
Merged
@cfromknecht
Copy link
Contributor

@joostjager dependent PR is up: #4285

@joostjager
Copy link
Contributor Author

joostjager commented Jun 24, 2020 •
edited
Loading

Now that AMP is postponed, I reverted this PR back to the original approach. While rebasing the [poc] commit, I realized we don't need rpc changes at all. Decided together with @cfromknecht to bring this back into the 0.11 scope.

@joostjager joostjager added v0.11 and removed v0.12 labels Jun 24, 2020
@joostjager joostjager added this to the 0.11.0 milestone Jun 24, 2020
@joostjager
Copy link
Contributor Author

PR is not working atm because it needs to be rebased on top of the fix for broken keysend in master.

@cfromknecht
Copy link
Contributor

@joostjager the fix we discussed for keysend in master has been updated: #4407

cfromknecht
cfromknecht reviewed Jun 25, 2020
View reviewed changes
Copy link
Contributor

@cfromknecht cfromknecht left a comment

Choose a reason for hiding this comment

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

minor nits, otherwise i think we're ready to roll! final commit can be dropped now 👍

@alexbosworth
Copy link
Contributor

This could be helpful for keysend based sloshing to prevent unwanted payments from external senders

@joostjager
Copy link
Contributor Author

PR rebased on top of #4407. Works now. PoC commit will be dropped after review is completed.

cfromknecht
cfromknecht reviewed Jun 27, 2020
View reviewed changes
@joostjager joostjager requested a review from cfromknecht June 27, 2020 07:47
cfromknecht
cfromknecht approved these changes Jun 27, 2020
View reviewed changes
Copy link
Contributor

@cfromknecht cfromknecht left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

@joostjager
invoices: optionally hold and auto-cancel keysend payments
24e3234 
Adds a new configuration flag to lnd that will keep keysend payments in
the accepted state. An application can then inspect the payment
parameters and decide whether to settle or cancel.

The on-the-fly inserted keysend invoices get a configurable expiry time.
This is a safeguard in case the application that should decide on the
keysend payments isn't active.
Roasbeef
Roasbeef approved these changes Jun 30, 2020
View reviewed changes
Copy link
Member

@Roasbeef Roasbeef left a comment

Choose a reason for hiding this comment

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

LGTM ⛩

@Roasbeef Roasbeef merged commit 7cda30b into lightningnetwork:master Jun 30, 2020
@nicolasburtey nicolasburtey mentioned this pull request Sep 18, 2020
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Roasbeef Roasbeef Roasbeef approved these changes

@cfromknecht cfromknecht cfromknecht approved these changes

Assignees
No one assigned
Labels
enhancement Improvements to existing features / behaviour payments Related to invoices/payments
Projects
None yet
Milestone
0.11.0
Development

Successfully merging this pull request may close these issues.
4 participants
@joostjager @cfromknecht @Roasbeef @alexbosworth