Skip to content

tapd: add itest for grouped asset channel funding #987

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 8 commits into from
Apr 29, 2025
Merged

tapd: add itest for grouped asset channel funding #987

merged 8 commits into from
Apr 29, 2025

Conversation

@guggero
Copy link
Contributor

@guggero guggero commented Feb 26, 2025 •
edited
Loading

@guggero guggero mentioned this pull request Feb 26, 2025
Merged
@guggero guggero self-assigned this Feb 26, 2025
@guggero guggero moved this to 🏗 In progress in Taproot-Assets Project Board Feb 26, 2025
@GeorgeTsagk GeorgeTsagk self-requested a review March 6, 2025 10:27
@guggero guggero force-pushed the group-key-support branch 2 times, most recently from bf764ca to 3c5f6cb Compare March 27, 2025 17:26
@guggero guggero mentioned this pull request Mar 31, 2025
Merged
@guggero guggero force-pushed the group-key-support branch from 3c5f6cb to 4c38293 Compare April 2, 2025 14:46
@guggero guggero mentioned this pull request Apr 3, 2025
Merged
@guggero guggero force-pushed the group-key-support branch 2 times, most recently from 84bf0f8 to d30865e Compare April 9, 2025 14:31
@guggero guggero requested a review from gijswijs April 9, 2025 15:11
@guggero guggero force-pushed the group-key-support branch from d30865e to 103e485 Compare April 15, 2025 16:35
@guggero
Copy link
Contributor Author

guggero commented Apr 15, 2025

Updated go.mod to point to the now merged lightninglabs/taproot-assets#1413.
Ready for review.

@guggero guggero requested review from ffranr and removed request for gijswijs April 15, 2025 17:36
@lightninglabs-deploy
Copy link

@GeorgeTsagk: review reminder
@ffranr: review reminder

@guggero guggero force-pushed the group-key-support branch from 103e485 to e159f66 Compare April 23, 2025 13:50
@guggero guggero mentioned this pull request Apr 23, 2025
Merged
@guggero guggero force-pushed the group-key-support branch from 684cb4e to b14f20b Compare April 23, 2025 14:06
@guggero
Copy link
Contributor Author

guggero commented Apr 23, 2025

Ready for review, @ffranr and @GeorgeTsagk.

@guggero guggero force-pushed the group-key-support branch from b14f20b to 85ae741 Compare April 23, 2025 15:02
@guggero guggero mentioned this pull request Apr 23, 2025
Closed
This was referenced Apr 24, 2025
Open
Closed
@guggero guggero mentioned this pull request Apr 24, 2025
Merged
1 task
@guggero guggero force-pushed the group-key-support branch from 85ae741 to dedf12c Compare April 24, 2025 10:03
@guggero guggero mentioned this pull request Apr 24, 2025
Merged
@levmi levmi moved this from 🏗 In progress to 👀 In review in Taproot-Assets Project Board Apr 24, 2025
@guggero guggero mentioned this pull request Apr 25, 2025
Merged
@guggero guggero force-pushed the group-key-support branch 3 times, most recently from 2ce063a to 23f300d Compare April 25, 2025 08:17
GeorgeTsagk
GeorgeTsagk approved these changes Apr 25, 2025
View reviewed changes
Copy link
Member

@GeorgeTsagk GeorgeTsagk left a comment

Choose a reason for hiding this comment

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

LGTM!

Non-blocking: Since the group keys on taprpc are already merged, we could enhance testCustomChannelsLiquidityEdgeCasesCore with an extra option similar to groupMode, which would control whether we open single asset ID or group funded channels. This way we could incorporate low-hanging coverage for this PR. Not sure what the diff for that would be, could def be a follow-up PR.

@guggero guggero force-pushed the group-key-support branch from 23f300d to 21e38f3 Compare April 29, 2025 13:02
@guggero
Copy link
Contributor Author

guggero commented Apr 29, 2025

Addressed comments.

ffranr
ffranr approved these changes Apr 29, 2025
View reviewed changes
itest/assets_test.go Outdated
Comment on lines 345 to 376
// Then we burn everything but a single asset piece.
assetID1 := mintedAssets[0].AssetGenesis.AssetId
assetID2 := mintedAssets[1].AssetGenesis.AssetId
burnAmount1 := mintedAssets[0].Amount - charlieFundingAmount/2 -
erinFundingAmount/2 - 1
_, err := charlieTap.BurnAsset(ctx, &taprpc.BurnAssetRequest{
Asset: &taprpc.BurnAssetRequest_AssetId{
AssetId: assetID1,
},
AmountToBurn: burnAmount1,
ConfirmationText: taprootassets.AssetBurnConfirmationText,
})
require.NoError(t.t, err)

mineBlocks(t, net, 1, 1)

burnAmount2 := mintedAssets[1].Amount - charlieFundingAmount/2 -
erinFundingAmount/2 - 1
_, err = charlieTap.BurnAsset(ctx, &taprpc.BurnAssetRequest{
Asset: &taprpc.BurnAssetRequest_AssetId{
AssetId: assetID2,
},
AmountToBurn: burnAmount2,
ConfirmationText: taprootassets.AssetBurnConfirmationText,
})
require.NoError(t.t, err)

mineBlocks(t, net, 1, 1)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are we doing this. Please consider adding a bit more to the comment.

Copy link
Member

Choose a reason for hiding this comment

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

I think we burn all but a small piece as we don't support fully burning an asset in a single transaction.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, it's to influence coin selection so we get exactly the pieces we want. Added a comment.

guggero added 7 commits April 29, 2025 21:16
@guggero
itest: use Charlie as proof courier, remove Zane
910c806 
We remove the Zane node from all tests where its only role was being the
universe server. Starting a node takes multiple seconds, so if we can
spin up fewer nodes, we can save some time.
And functionality wise Charlie can easily be the universe, we just need
to define its port upfront so we can configure it to be its own proof
courier.
The topology for all tests is at least the following:

Charlie  --[assets]-->  Dave  --[sats]-->  Erin  --[assets]-->  Fabia

Which means, we the case where one of the channel peers is its own
universe (Charlie in Charlie<->Dave) as well as the case where the
universe server is a remote node (Charlie in the Erin<->Fabia channel).
@guggero
mod: bump to latest version of tapd, lnd, loop
e995fcb
@guggero
itest: refactor to prepare for grouped assets
b2f9e6e 
This commit prepares some of the helper functions to be able to handle
channels that have multiple asset pieces in them.
@guggero
itest: refactor out assertForceCloseSweeps for re-use
a70609b 
We'll be adding a new HTLC force close test where we'll be able to
re-use this functionality.
@guggero
itest: add grouped asset multi tranche test
1dec5b6
@guggero
itest: add grouped asset multi tranche HTLC test
fc35c0a
@guggero
docs: add release notes
4225d65
@guggero guggero force-pushed the group-key-support branch from 21e38f3 to 4225d65 Compare April 29, 2025 19:19
@guggero
Copy link
Contributor Author

guggero commented Apr 29, 2025

Looks like the CLI is a bit more than just a few lines, so going to fix lightninglabs/taproot-assets#1497 in a separate PR and merge this as soon as CI is green.

@guggero guggero merged commit 5d8753a into master Apr 29, 2025
17 of 21 checks passed
@github-project-automation github-project-automation bot moved this from 👀 In review to ✅ Done in Taproot-Assets Project Board Apr 29, 2025
@guggero guggero deleted the group-key-support branch April 29, 2025 20:12
@guggero guggero restored the group-key-support branch April 29, 2025 20:12
@guggero guggero deleted the group-key-support branch April 29, 2025 20:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Roasbeef Roasbeef Roasbeef left review comments

@ffranr ffranr ffranr approved these changes

@GeorgeTsagk GeorgeTsagk GeorgeTsagk approved these changes

Assignees

@guggero guggero

Labels

None yet

Projects

Taproot-Assets Project Board
Status: ✅ Done

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@guggero @lightninglabs-deploy @Roasbeef @ffranr @GeorgeTsagk