-
Notifications
You must be signed in to change notification settings - Fork 91
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
Add custom channels liquidity edge cases itest #842
Conversation
d85b327
to
d77401e
Compare
select { | ||
case <-done: | ||
case <-timeoutChan: | ||
t.Fatalf("Payment didn't fail within expected time duration") |
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'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)
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.
Great to have a specific test for these edge cases 👍
|
||
} | ||
|
||
logBalance(t.t, nodes, assetID, "after failed 50 assets") |
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.
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.
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.
yeap good idea, it's already available here through the SendPayment
RPC response, result.FailureReason
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.
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
d77401e
to
fcd4227
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.
Linter and itest failed, other than that looks good 🎉
fcd4227
to
15e694a
Compare
|
15e694a
to
6a2097a
Compare
6a2097a
to
7f75248
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.
Great catches, LGTM 🎉
fn.None[lnrpc.PaymentFailureReason](), | ||
) | ||
|
||
logBalance(t.t, nodes, assetID, "after 50 sats backwards") |
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.
nit: after 50 assets backwards, not sats
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