Skip to content

lntest/itest: fix SendPaymentAMP test#5725

Merged
guggero merged 1 commit into
lightningnetwork:masterfrom
bottlepay:fix-amp-itest
Sep 14, 2021
Merged

lntest/itest: fix SendPaymentAMP test#5725
guggero merged 1 commit into
lightningnetwork:masterfrom
bottlepay:fix-amp-itest

Conversation

@joostjager
Copy link
Copy Markdown
Contributor

Fixes timing issue that caused missed invoice notification.

@orijbot
Copy link
Copy Markdown

orijbot commented Sep 14, 2021

Copy link
Copy Markdown
Collaborator

@guggero guggero left a comment

Choose a reason for hiding this comment

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

Nice that you found the fix so quickly! One question about the timeout.

Comment thread lntest/itest/lnd_amp_test.go Outdated
Copy link
Copy Markdown
Collaborator

@guggero guggero left a comment

Choose a reason for hiding this comment

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

LGTM 🎉

@guggero guggero requested a review from bhandras September 14, 2021 18:41
Copy link
Copy Markdown
Collaborator

@bhandras bhandras left a comment

Choose a reason for hiding this comment

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

Great find! LGTM 👍

// Subscribe to bob's invoices. Do this early in the test to make sure
// that the subscription has actually been completed when we add an
// invoice. Otherwise the notification will be missed.
req := &lnrpc.InvoiceSubscription{}
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I belive we could fix this below in testSendToRouteAMP too.

Copy link
Copy Markdown
Contributor Author

@joostjager joostjager Sep 14, 2021

Choose a reason for hiding this comment

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

I think that one has sufficient delay because it jit-inserts the invoice when the htlc arrives. It is kind of unsatisfactory that the subscribe call doesn't give a signal when it is actually subscribed though.

@guggero guggero merged commit 1ea6db1 into lightningnetwork:master Sep 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants