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.

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

Add custom channels liquidity edge cases itest #842

Merged

Conversation

GeorgeTsagk
Copy link
Member

@GeorgeTsagk GeorgeTsagk commented Sep 10, 2024

This PR adds an itest that exposes the liquidity related edge cases of custom channels.

The following PRs need to be merged first, in order to bump the dependencies on this PR:

Closes lightninglabs/taproot-assets#1060
Closes lightninglabs/taproot-assets#1073
Closes lightninglabs/taproot-assets#1013

@GeorgeTsagk GeorgeTsagk self-assigned this Sep 10, 2024
@GeorgeTsagk GeorgeTsagk marked this pull request as ready for review September 12, 2024 12:44
@GeorgeTsagk GeorgeTsagk force-pushed the custom-channel-liquidity-itest branch 2 times, most recently from d85b327 to d77401e Compare September 12, 2024 13:04
select {
case <-done:
case <-timeoutChan:
t.Fatalf("Payment didn't fail within expected time duration")
Copy link
Member Author

Choose a reason for hiding this comment

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

I'm doing this because both timeout and actual payment error result in lnrpc.Payment_FAILED above, so I wanna make sure we failed but not because we got stuck in a loop (behavior prior to bug fix)

Copy link
Member

@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.

Great to have a specific test for these edge cases 👍

go.mod Outdated Show resolved Hide resolved
itest/assets_test.go Outdated Show resolved Hide resolved
itest/litd_custom_channels_test.go Show resolved Hide resolved
itest/litd_custom_channels_test.go Outdated Show resolved Hide resolved
itest/litd_custom_channels_test.go Show resolved Hide resolved

}

logBalance(t.t, nodes, assetID, "after failed 50 assets")
Copy link
Member

Choose a reason for hiding this comment

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

Would be nice if we could also assert the payment's failure reason, that it's actually the INSUFFICIENT_BALANCE code. I think we return that somewhere.

Copy link
Member Author

Choose a reason for hiding this comment

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

yeap good idea, it's already available here through the SendPayment RPC response, result.FailureReason

Copy link
Member Author

Choose a reason for hiding this comment

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

Since this is meant to fail in the paymentLifecycle phase, we should get a NO_ROUTE instead

The reason the failure happens there is because that's when we actually provide the 500 sats amount to the PaymentBandwidth hook. See more on this comment

@GeorgeTsagk
Copy link
Member Author

GeorgeTsagk commented Sep 17, 2024

go.mod has now been updated to point to the tip of the tapd PR branch which contains the latest fix

Copy link
Member

@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.

Linter and itest failed, other than that looks good 🎉

@GeorgeTsagk
Copy link
Member Author

go.mod dependencies updated, after dependent PRs got merged

Copy link

@jharveyb jharveyb left a comment

Choose a reason for hiding this comment

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

Great catches, LGTM 🎉

fn.None[lnrpc.PaymentFailureReason](),
)

logBalance(t.t, nodes, assetID, "after 50 sats backwards")

Choose a reason for hiding this comment

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

nit: after 50 assets backwards, not sats

@Roasbeef Roasbeef merged commit fab6936 into lightninglabs:0-19-staging Sep 23, 2024
12 of 13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

4 participants