Skip to content

Asset Invoice with sats value itest #1020

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
Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Asset Invoice with sats value itest #1020

wants to merge 1 commit into from

Conversation

GeorgeTsagk
Copy link
Member

Description

This itest tests the behavior exposed in lightninglabs/taproot-assets#1448

It is very minimal, and currently only to be used as a proof-of-concept sanity check for the above feature.

@GeorgeTsagk GeorgeTsagk self-assigned this Mar 31, 2025
@GeorgeTsagk GeorgeTsagk force-pushed the sats-asset-invoices branch from 01d92fe to 53446fc Compare April 14, 2025 12:13
guggero
guggero reviewed Apr 16, 2025
View reviewed changes
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.

I know this wasn't marked for review yet, but did a drive-by anyway.

itest/litd_custom_channels_test.go Outdated
// Yara with satoshi. This is a multi-hop payment going over 2 asset
// channels, where the total asset value is less than the default anchor
// amount of 354 sats.
createAssetInvoice(t.t, dave, charlie, 1, assetID, withInvoiceErrSubStr(
Copy link
Member

Choose a reason for hiding this comment

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

Why are we removing this edge case test? This should still apply IMO. The error message has changed a bit though.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, I thought I introduced an extra check implicitly with latest diff, yeah it was just the error. Rebased on latest master after merging #1040 and looks fine, will drop this commit

itest/litd_custom_channels_test.go Outdated
@@ -2098,7 +2086,7 @@ func testCustomChannelsLiquidityEdgeCases(ctx context.Context,

// Now Fabia creates the normal invoice.
invoiceResp = createAssetInvoice(
t.t, erin, fabia, 10_000, assetID,
t.t, erin, fabia, 0, assetID, withMsatAmount(170_000_000),
Copy link
Member

Choose a reason for hiding this comment

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

Should we add a new case for this new functionality? Or at least update the comment above this to point out that we're testing a specific case?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah can add a comment describing what's happening, I'd also like to have a case explicitly for this, but seems like there isn't much to test here.

The invoice is meant to have as many units as you provide the request with, and the RFQ is meant to satisfy the volume of it. The first is checked in assets_test.go, the latter is checked via the payment succeeding.

@GeorgeTsagk GeorgeTsagk force-pushed the sats-asset-invoices branch 2 times, most recently from 18f36e7 to 45a38e9 Compare April 17, 2025 06:32
@GeorgeTsagk GeorgeTsagk added the no-changelog This PR is does not require a release notes entry label Apr 17, 2025
@GeorgeTsagk
Copy link
Member Author

will wait for lightninglabs/taproot-assets#1462 to merge first as compilation fails against LND due to aux interface changes

@GeorgeTsagk GeorgeTsagk force-pushed the sats-asset-invoices branch from 45a38e9 to 03013b5 Compare April 24, 2025 11:53
@GeorgeTsagk GeorgeTsagk requested a review from guggero April 24, 2025 11:54
@lightninglabs-deploy
Copy link

@guggero: review reminder

guggero
guggero approved these changes Apr 25, 2025
View reviewed changes
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 🎉

But needs to be based on top of #987 as well to fully satisfy the CI. Then the first commit can be dropped.

@GeorgeTsagk GeorgeTsagk force-pushed the sats-asset-invoices branch from 03013b5 to 7ccb618 Compare April 25, 2025 08:38
@GeorgeTsagk GeorgeTsagk changed the base branch from master to group-key-support April 25, 2025 08:38
@GeorgeTsagk
Copy link
Member Author

Changed base to group-key-support

@guggero guggero force-pushed the group-key-support branch 2 times, most recently from 21e38f3 to 4225d65 Compare April 29, 2025 19:19
@guggero guggero force-pushed the sats-asset-invoices branch from 7ccb618 to 1304cf2 Compare April 29, 2025 20:11
@guggero guggero changed the base branch from group-key-support to master April 29, 2025 20:11
@guggero
Copy link
Member

guggero commented Apr 29, 2025

Rebased and ready for review, @ffranr.

@guggero guggero force-pushed the sats-asset-invoices branch from 1304cf2 to 9bfaa5d Compare April 29, 2025 20:14
@ZZiigguurraatt
Copy link

I get

e563e22bdadf:/$ litcli ln addinvoice --asset_id 5f2e3073ef4d23ac2b9482ad9988fb6ae7d3cc48d260ed00b0f9f655d4a7e56e --amt=1000
[litcli] asset_amount argument missing
e563e22bdadf:/$

is that something you want to fix in this PR, or a separate one?

@levmi levmi requested a review from ffranr May 1, 2025 15:25
@levmi levmi moved this from 🆕 New to 🏗 In progress in Taproot-Assets Project Board May 1, 2025
@levmi levmi moved this from 🏗 In progress to 👀 In review in Taproot-Assets Project Board May 1, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@guggero guggero guggero approved these changes

@ffranr ffranr Awaiting requested review from ffranr

Assignees

@GeorgeTsagk GeorgeTsagk

Labels
no-changelog This PR is does not require a release notes entry
Projects
Taproot-Assets Project Board
Status: 👀 In review
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@GeorgeTsagk @lightninglabs-deploy @guggero @ZZiigguurraatt