-
Notifications
You must be signed in to change notification settings - Fork 7
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
Liquid/Lightning drain #553
Conversation
04a693c
to
f164bf9
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.
Looks good!
d46a579
to
81ad0fb
Compare
Do you think it is reasonable to calculate for one case (let's say without routing hints) and if we get one with routing hints fetch again? |
@@ -373,7 +373,7 @@ dictionary LnUrlPayRequest { | |||
|
|||
dictionary PrepareSendRequest { | |||
string destination; | |||
u64? amount_sat = null; | |||
PayOnchainAmount? amount = null; |
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.
The name is a bit confusing because we use onchain for bitcoin.
do you think it makes sense to use amount_sat as before and add the drain option default to false?
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 feel like the drain option should be an explicit opt-in and would be more consistent with the drain to Bitcoin. I would rather rename the PayOnchainAmount to something like DrainableAmount. Happy to revert to use the amount_sat if you prefer.
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.
Perhaps rename to SendPaymentAmount or PayAmount ?
I think it's a reasonable solution. I thought about just sending the drain amount to the MRH of the first invoice also, but this way seems cleaner |
This PR:
It's difficult to add a drain option to the PrepareLnUrlPayRequest because at the point when preparing we don't know if the LNURL invoice response will contain an MRH or not for a direct Liquid tx. Because of this we can't calculate an invoice amount to drain for both options. See #559
TODO
Fixes #543
Fixes #550