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

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
@GeorgeTsagk GeorgeTsagk force-pushed the rfq-track-htlc-itests branch from 6b56147 to f055328 Compare November 25, 2024 10:37
@ffranr
Copy link
Contributor

ffranr commented Nov 27, 2024

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

@GeorgeTsagk GeorgeTsagk force-pushed the rfq-track-htlc-itests branch from f055328 to e52a8c5 Compare November 27, 2024 11:45
@GeorgeTsagk
Copy link
Member Author

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

itest/litd_custom_channels_test.go Outdated Show resolved Hide resolved
itest/litd_custom_channels_test.go Outdated Show resolved Hide resolved
itest/litd_custom_channels_test.go Outdated Show resolved Hide resolved
itest/litd_custom_channels_test.go Outdated Show resolved Hide resolved
itest/litd_custom_channels_test.go Outdated Show resolved Hide resolved
@guggero guggero force-pushed the update-to-lnd-18-4 branch 3 times, most recently from b0c1550 to d48feca Compare November 28, 2024 15:39
@GeorgeTsagk GeorgeTsagk force-pushed the rfq-track-htlc-itests branch 2 times, most recently from 9ef04d6 to 82d567c Compare November 28, 2024 18:49
@GeorgeTsagk GeorgeTsagk requested a review from ffranr November 28, 2024 18:51
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.

Needs a couple of updates still, but definitely the better approach!

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 Outdated 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
Copy link
Contributor

@ffranr ffranr left a comment

Choose a reason for hiding this comment

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

I've reviewed the latest commits. I think we just need to address Oli's comments here.

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.

LGTM pending dependent PRs.

itest/assets_test.go Outdated 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
itest/assets_test.go Outdated Show resolved Hide resolved
itest/litd_accounts_test.go Outdated Show resolved Hide resolved
itest/litd_custom_channels_test.go Outdated Show resolved Hide resolved
itest/litd_custom_channels_test.go Outdated Show resolved Hide resolved
itest/litd_custom_channels_test.go Outdated Show resolved Hide resolved
@guggero guggero force-pushed the update-to-lnd-18-4 branch from d4505dd to 868e57c Compare December 5, 2024 11:34
@GeorgeTsagk GeorgeTsagk force-pushed the rfq-track-htlc-itests branch from 83801f5 to acbc6b5 Compare December 5, 2024 12:15
@GeorgeTsagk GeorgeTsagk requested a review from ffranr December 5, 2024 12:15
@GeorgeTsagk GeorgeTsagk force-pushed the rfq-track-htlc-itests branch from acbc6b5 to 0381304 Compare December 5, 2024 12:27
itest/assets_test.go Outdated Show resolved Hide resolved
@GeorgeTsagk GeorgeTsagk force-pushed the rfq-track-htlc-itests branch from 0381304 to 99ab0ce Compare December 5, 2024 13:38
@GeorgeTsagk
Copy link
Member Author

Will wait for base PR to merge, then will update go.mod before merging this

@@ -882,44 +896,61 @@ func payInvoiceWithAssets(t *testing.T, payer, rfqPeer *HarnessNode,
sendReq.MaxShardSizeMsat = 80_000_000
}

var rfqBytes []byte
manualRfq.WhenSome(func(i rfqmsg.ID) {
copy(rfqBytes, i[:])
Copy link
Member

Choose a reason for hiding this comment

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

copy() is tricky if the target slice is nil or empty, nothing happens. So you need to do rfqBytes = make([]byte, len(i[:])) first.

Copy link
Member Author

Choose a reason for hiding this comment

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

Nice catch! Interestingly enough the tests did not fail as the daemon would automatically acquire fresh quotes for the payment. Specifying an rfqID here was a effectively a noop.

The reason this wasn't caught by the next test case (which requires you use a specific rfq scid and none else) is because it is enforced by the receiver, while crafting the invoice.

@GeorgeTsagk GeorgeTsagk force-pushed the rfq-track-htlc-itests branch from 99ab0ce to aff79f2 Compare December 5, 2024 15:59
@GeorgeTsagk GeorgeTsagk force-pushed the rfq-track-htlc-itests branch from aff79f2 to 73bbd6a Compare December 6, 2024 10:34
@GeorgeTsagk GeorgeTsagk merged commit 82c8511 into lightninglabs:update-to-lnd-18-4 Dec 6, 2024
12 of 13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

4 participants