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

itest: Extend custom channel liquidity itest for RFQ HTLC tracking #906

Open
wants to merge 2 commits into
base: update-to-lnd-18-4
Choose a base branch
from

Conversation

GeorgeTsagk
Copy link
Member

@GeorgeTsagk GeorgeTsagk commented Nov 22, 2024

Descriptions

Adds an extra edge case to the custom channels itest which verifieds that the rfq htlc tracking mechanism works as expected.

Related tapd PR: lightninglabs/taproot-assets#1186

@GeorgeTsagk GeorgeTsagk self-assigned this Nov 22, 2024
@dstadulis dstadulis changed the title Extend custom channel liquidity itest for RFQ HTLC tracking itest: Extend custom channel liquidity itest for RFQ HTLC tracking Nov 22, 2024
@dstadulis dstadulis added this to the tapd v0.5 milestone Nov 22, 2024
@ffranr
Copy link
Contributor

ffranr commented Nov 27, 2024

@GeorgeTsagk do I only need to review the last three commits?

@GeorgeTsagk
Copy link
Member Author

@ffranr this was just rebased, it's just one commit now (if you skip the temp one)

Comment on lines +2079 to +2082
// We set the waiting period for the slosh payment to occur. This is set
// to half of the default payment timeout, as we want it to occur half
// way through the in-flight payment.
sloshWait := PaymentTimeout / 3
Copy link
Contributor

Choose a reason for hiding this comment

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

should be /2?

Comment on lines +2096 to +2098
// To avoid goroutine uncertainty, we wait as much as the above routine
// minus a small delta. This is enough for us to be sure that the
// payment will complete due to the slosh.
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a way to make this more events based driven? Instead of timing can we detect the arrival of HTLCs and respond when appropriate?

Comment on lines +2102 to +2125
go func() {
// Now Charlie pays the invoice with assets. What happens on the
// Charlie-Dave channel doesn't matter. This payment will block
// until the previous slosh payment completes. That slosh will
// allow for the rest of the HTLCs to be forwarded.
payInvoiceWithAssets(
t.t, charlie, dave, invoiceResp.PaymentRequest, assetID,
true, defaultPaymentStatusOpt,
)

done <- true
}()

select {
case <-done:
// If the payment completes before the slosh payment occurs then
// something went wrong, this is not the expected test case
// behavior.
t.Fatalf("payment completed before expected block period")
case <-timeoutChan:
// The expected time delay for the payment to complete has been
// passed, so now we wait for the payment to complete.
<-done
}
Copy link
Contributor

Choose a reason for hiding this comment

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

We have BeforeTimeout as a helper function. Would be good to have AfterTimestamp perhaps. But I think this sort of timing analysis strategy should be events driven instead.


//nolint:lll
go func() {
// payInvoiceWithSatoshi(t.t, erin, iResp, lnrpc.Payment_FAILED)
Copy link
Contributor

Choose a reason for hiding this comment

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

needs to be removed?

Comment on lines +2196 to +2199
stream, err := erin.RouterClient.SendPaymentV2(ctxc, sendReq)
if err == nil {
_, _ = getPaymentResult(stream)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you should use this patter here:

stream, err := erin.RouterClient.SendPaymentV2(ctxc, sendReq)
require.NoError(t.t, err)

_, _ = getPaymentResult(stream)

Need error check on getPaymentResult result?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: 🏗 In progress
Development

Successfully merging this pull request may close these issues.

3 participants