Skip to content

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
@GeorgeTsagk GeorgeTsagk requested a review from guggero September 12, 2024 15:45
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
Contributor

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


}

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

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 GeorgeTsagk force-pushed the custom-channel-liquidity-itest branch from d77401e to fcd4227 Compare September 16, 2024 14:31
@GeorgeTsagk GeorgeTsagk requested a review from guggero September 16, 2024 14:32
@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
Contributor

@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

@GeorgeTsagk GeorgeTsagk force-pushed the custom-channel-liquidity-itest branch from 15e694a to 6a2097a Compare September 19, 2024 15:19
@GeorgeTsagk GeorgeTsagk force-pushed the custom-channel-liquidity-itest branch from 6a2097a to 7f75248 Compare September 19, 2024 15:33
@dstadulis dstadulis requested a review from jharveyb September 19, 2024 15:44
@GeorgeTsagk GeorgeTsagk requested a review from guggero September 19, 2024 17:11
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