-
Notifications
You must be signed in to change notification settings - Fork 24
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
Cli: add pay offer command #91
Conversation
e0c9dc2
to
b558281
Compare
2b72d7c
to
8d7d885
Compare
b558281
to
1402f4c
Compare
8d7d885
to
db39cfa
Compare
93eaf27
to
52a30b8
Compare
b41c013
to
b1d8e94
Compare
52a30b8
to
889c0cd
Compare
b1d8e94
to
8a8b2a4
Compare
889c0cd
to
8f2e25e
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.
Straightforward, will give this deeper look when the preceeding PRs are in 👍
b574ebf
to
fc061b7
Compare
8f2e25e
to
9442eb7
Compare
cbcb32a
to
661e889
Compare
be2cbd5
to
7192583
Compare
How did you test this flow end to end? I tried creating an offer with CLN (recent commit 3f4306eea) using the setup described here, but it doesn't include a blinded path (which makes LNDK panic because we expect one). Offer decode:
Fetch and decode invoice:
|
@carlaKC Ah to be honest, so far I've only tested against ldk-sample. I'll give CLN a try and see what I can come up with. We might also encounter this problem, though, when trying to send a payment: ElementsProject/lightning#6588 If that is still an issue, what do you think of testing it against a live LDK-based node that distributes offers (I just mean, something a little more realistic than our ldk-sample branch. I'd have to check with the LDK team if there are any such nodes yet) or against Eclair as an alternative? |
661e889
to
3c0d2ee
Compare
7192583
to
c1eb02d
Compare
3c0d2ee
to
fe63c9e
Compare
c1eb02d
to
34ea993
Compare
fe63c9e
to
4b491cc
Compare
34ea993
to
fa684df
Compare
Tested this against LDK sample with direct connection and ran into:
I think that this may be an issue with the LDK node's networking, but we should probably be setting PathID in the meantime? I also think that we should add a really basic timeout, because right now we just hang waiting to hear back:
|
Just posting things as I go on this PR, even if they're something that we should have in followups. Don't want to merge the command itself until we've tested it with various impls. |
Good idea, I'll add something along those lines. Re: the "failed to find path" error. Is this the upstream ldk-sample or our fork of it? Are ldk and lnd connected directly in your setup? I'll take a look, though even if that's fixed, the upstream ldk-sample won't work right now because of another bug I just reported to the LDK team. So in short we can't pay offers created by ldk-sample or CLN due to bugs on their end. Eclair is the only one that works right now. But hopefully some fixes go in pretty soon - I'll try to keep this up to date #93 |
fa684df
to
00a2d13
Compare
Added that timeout :) |
088272e
to
4e3672d
Compare
4e3672d
to
4d2d513
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.
Going ahead with merge since we've got interop w/ Eclair 🎉
Can improve on our invoice retrieval sophistication in followups (multiple attempts, better timeouts etc).
This PR adds CLI command
lndk-cli pay-offer
, and adds some documentation for doing so. Depends on #90Probably the main change I need to make is to move the reply path code to #87, it makes more sense there!