cli: add group key support to channel related calls. #1052

guggero · 2025-04-30T09:36:45Z

Depends on lightninglabs/taproot-assets#1498.

Fixes lightninglabs/taproot-assets#1497.
Fixes lightninglabs/taproot-assets#1183.

@ZZiigguurraatt can you help test this PR please?

ZZiigguurraatt · 2025-04-30T16:24:00Z

@ZZiigguurraatt can you help test this PR please?

Working on it.

ZZiigguurraatt · 2025-05-02T14:58:47Z

For litcli ln sendpayment, why do we have the requirement Note that this will only work in concert with the --keysend argument.? This conflicts with the statement

One can either specify the full
  parameters of the payment, or just use a payment request which encodes
  all the payment details.

  If payment isn't manually specified, then only a payment request needs
  to be passed using the --pay_req argument.

.

Confused if this comment belongs here or in lightninglabs/taproot-assets#1498 ?

ZZiigguurraatt · 2025-05-02T15:00:45Z

litcli ln sendpayment - Send a payment over Lightning, potentially using a mulit-asset channel as the first hop needs multi spelled right.

ZZiigguurraatt · 2025-05-02T15:13:41Z

Here we also need to mention group_key

https://github.com/lightninglabs/lightning-terminal/pull/1052/files#diff-f9c05a4a36f7d2dcc59718bda9351aeab75a35de48e367bcd08d8cbf1f4be8f7R666-R667

https://github.com/lightninglabs/lightning-terminal/pull/1052/files#diff-f9c05a4a36f7d2dcc59718bda9351aeab75a35de48e367bcd08d8cbf1f4be8f7R671-R672

ZZiigguurraatt · 2025-05-02T15:51:48Z

At https://github.com/lightninglabs/lightning-terminal/pull/1052/files#diff-f9c05a4a36f7d2dcc59718bda9351aeab75a35de48e367bcd08d8cbf1f4be8f7R451 you say Pay an invoice over lightning using an asset. and at https://github.com/lightninglabs/lightning-terminal/pull/1052/files#diff-f9c05a4a36f7d2dcc59718bda9351aeab75a35de48e367bcd08d8cbf1f4be8f7R298-R299 you say Send a payment over Lightning, potentially using a mulit-asset channel as the first hop. I think these descriptions should be more consistent so we aren't confused there is some major difference in functionality.

ZZiigguurraatt · 2025-05-02T15:53:55Z

At https://github.com/lightninglabs/lightning-terminal/pull/1052/files#diff-f9c05a4a36f7d2dcc59718bda9351aeab75a35de48e367bcd08d8cbf1f4be8f7R37 you say Interact with the Lightning Network., but I think we should be more specific so that when running litcli ln we know under NAME that this is about taproot assets without having to read through the description of each sub-command in detail.

ZZiigguurraatt · 2025-05-02T15:56:54Z

In https://github.com/lightninglabs/lightning-terminal/pull/1052/files#diff-f9c05a4a36f7d2dcc59718bda9351aeab75a35de48e367bcd08d8cbf1f4be8f7R301-R302 you say To send an multi-asset LN payment to a single hop, the --asset_id=X argument should be used. This does not make any mention of group_key. Also, I'm a bit confused because with this command, I must specify either a asset_id or group_key, there is no other option, but the word choice leads me to believe that maybe I can send a non multi-asset payment with the command.

ZZiigguurraatt · 2025-05-02T16:14:35Z

If all these things can be fixed, I think we can also close lightninglabs/taproot-assets#1183 .

guggero · 2025-05-05T11:17:25Z

Here we also need to mention group_key

https://github.com/lightninglabs/lightning-terminal/pull/1052/files#diff-f9c05a4a36f7d2dcc59718bda9351aeab75a35de48e367bcd08d8cbf1f4be8f7R666-R667

https://github.com/lightninglabs/lightning-terminal/pull/1052/files#diff-f9c05a4a36f7d2dcc59718bda9351aeab75a35de48e367bcd08d8cbf1f4be8f7R671-R672

Will update. As an aside: if you go to the "Files changed" tab in a PR, then you can add comments to a specific line of a PR. And then you can also group multiple such comments into a single review, which makes it easy to see and resolve. Less work for you and also less work for the author.

guggero · 2025-05-05T11:26:31Z

@ZZiigguurraatt thanks for your feedback! I think I've addressed all your comments in the latest push.

ZZiigguurraatt · 2025-05-05T13:17:24Z

As an aside: if you go to the "Files changed" tab in a PR, then you can add comments to a specific line of a PR.

this did not work for lines out of range from the edits you made

guggero · 2025-05-05T13:30:47Z

Fixed the compilation issue.

ZZiigguurraatt · 2025-05-05T18:25:13Z

@ZZiigguurraatt thanks for your feedback! I think I've addressed all your comments in the latest push.

Okay, what do you think about this comment I made above "If I try sendpayment --pay_req= instead, I get [litcli] the --keysend flag must be set.", but litcli ln payinvoice -h says This command is a shortcut for 'sendpayment --pay_req=', so I feel like we are getting some conflicting information. This is also related to #1052 (comment)

guggero · 2025-05-06T07:25:35Z

@ZZiigguurraatt thanks for your feedback! I think I've addressed all your comments in the latest push.

Okay, what do you think about this comment I made above "If I try sendpayment --pay_req= instead, I get [litcli] the --keysend flag must be set.", but litcli ln payinvoice -h says This command is a shortcut for 'sendpayment --pay_req=', so I feel like we are getting some conflicting information. This is also related to #1052 (comment)

I didn't see that comment anywhere before...
But I'll look into it.

cmd/litcli/ln.go

ZZiigguurraatt · 2025-05-02T16:04:00Z

cmd/litcli/ln.go

@@ -437,17 +453,18 @@ var payInvoiceCommand = cli.Command{
 	This command attempts to pay an invoice using an asset channel as the
 	source of the payment. The asset ID of the channel must be specified
 	using the --asset_id flag.
+
+	This command is a shortcut for 'sendpayment --pay_req='.


If I try sendpayment --pay_req= instead, I get [litcli] the --keysend flag must be set.

Yeah, that was an incorrect check. Fixed that.

Okay, that didn't work. See main comment at the bottom.

cmd/litcli/ln.go

ZZiigguurraatt · 2025-05-06T17:24:04Z

I didn't see that comment anywhere before...

actually, maybe you don't get them unless I click "submit review"? I thought that meant I was satisfied with changes, but looks like I actually have a few options when I submit.

guggero · 2025-05-07T08:38:45Z

I didn't see that comment anywhere before...

actually, maybe you don't get them unless I click "submit review"? I thought that meant I was satisfied with changes, but looks like I actually have a few options when I submit.

Yeah, as long as it says "pending", only you see the comment.

guggero · 2025-05-07T10:34:23Z

Okay, so the attempt at making This command is a shortcut for 'sendpayment --pay_req= work made me realize that we never intended the sendpayment command to work in any other way than with --keysend.

This whole confusion came out of the attempt to re-use a lot of the description and also validation logic from lnd. But that just made it very hard to understand.

So I did this instead:

Remove the This command is a shortcut for 'sendpayment --pay_req= part from the description of payinvoice.
Updated the description of sendpayment to make it very explicit the command is only for keysend payments.
I considered renaming the command to keysend and removing the need for the --keysend flag. But that would break existing users/scripts and would lead to a bunch of duplicated code. So I decided to leave things this way. It's a bit verbose but at least the description should be more clear now.

I think with that things should be way more clear. I hope you agree with the changes, @ZZiigguurraatt.

ZZiigguurraatt · 2025-05-08T11:43:09Z

I tested FundChannel and I'm getting the following:

fd3988c2dd94:/$ tapcli assets groups
{
    "groups":  {
        "025ab6480a9af4d284c1e7c4b9391e2198cd54e96f41c3450499b51b0af194af57":  {
            "assets":  [
                {
                    "id":  "f1d09444f6d9935d1a22a00416dbcb9a2c1eb7d923d322871e4df1ce024415af",
                    "amount":  "2100000",
                    "lock_time":  0,
                    "relative_lock_time":  0,
                    "tag":  "Asset1",
                    "meta_hash":  "827c23a4b1a5f4f9a9a42e61aa7bf7bf7e7925b4723728b60ad1918d632e2b0a",
                    "type":  "NORMAL",
                    "version":  "ASSET_VERSION_V0"
                },
                {
                    "id":  "4b4be6b3f93c992aac615d7534eb52a81c2d077fb35717e0d8ac1ba226e1ba54",
                    "amount":  "2100000",
                    "lock_time":  0,
                    "relative_lock_time":  0,
                    "tag":  "Asset1",
                    "meta_hash":  "827c23a4b1a5f4f9a9a42e61aa7bf7bf7e7925b4723728b60ad1918d632e2b0a",
                    "type":  "NORMAL",
                    "version":  "ASSET_VERSION_V0"
                },
                {
                    "id":  "6c360362f0f62bcf0a6abd1b50d3c77dfd6d685fdbbfd9804a2b1f786cd0e9b5",
                    "amount":  "2100000",
                    "lock_time":  0,
                    "relative_lock_time":  0,
                    "tag":  "Asset1",
                    "meta_hash":  "827c23a4b1a5f4f9a9a42e61aa7bf7bf7e7925b4723728b60ad1918d632e2b0a",
                    "type":  "NORMAL",
                    "version":  "ASSET_VERSION_V0"
                }
            ]
        }
    }
}
fd3988c2dd94:/$ 
fd3988c2dd94:/$ 
fd3988c2dd94:/$ 
fd3988c2dd94:/$ litcli ln fundchannel --node_key 025974657e16df81f32623ed5fe33d49529b26659ef2de9a87d8b102e576b26af3 --asset_amount 100000 --group_key 025ab6480a9af4d284c1e7c4b9391e2198cd54e96f41c3450499b51b0af194af57
[litcli] asset with ID  not found or no combined UTXOs with at least amount 100000 is available
fd3988c2dd94:/$

guggero · 2025-05-08T12:18:39Z

[litcli] asset with ID not found or no combined UTXOs with at least amount 100000 is available

Oops, my bad, sorry. Fixed that issue and tested the flow manually as well this time.

gijswijs

Made some comments throughout. The most important one is this one: #1052 (comment)

The others are all nits.

I tested the itest locally.

gijswijs · 2025-05-08T11:51:01Z

cmd/litcli/ln.go

+		}
+	}
+
+	if len(assetIDBytes) == 0 && len(groupKeyBytes) == 0 {


Surely this must be

Suggested change

if len(assetIDBytes) == 0 && len(groupKeyBytes) == 0 {

if len(assetIDBytes) > 0 && len(groupKeyBytes) > 0 {

or I'm not understanding the logic here.

gijswijs · 2025-05-08T12:05:36Z

cmd/litcli/ln.go

+	To send a multi-asset LN keysend payment, the --asset_id=X or
+	--group_key=X argument can be used to specify the asset to use.
+
+	Note that this command will only work with the --keysend argument set.


👍 for the B1-level writing.

gijswijs · 2025-05-08T12:12:51Z

cmd/litcli/ln.go

-	}
-
-	assetAmountToSend := cliCtx.Uint64(assetAmountFlag.Name)
-	if assetAmountToSend == 0 {


We don't need to check for zero amounts? Or is this offloaded to tapd?

gijswijs · 2025-05-08T12:24:56Z

itest/litd_custom_channels_test.go

-			PayReqString: payReq,
-		},
-	)
+	decodeResp, err := aliceTap.DecodeAssetPayReq(ctx, &tchrpc.AssetPayReq{


This doesn't follow our dev guidelines. When wrapping long function calls, all arguments should begin in a new line after the open parenthesis and the close parenthesis should be placed on its own line. This is also true for the alice.AddInvoice function call above, and the aliceTap.DecodeAssetPayReq call below.

guggero mentioned this pull request Apr 30, 2025

misc: fix litcli command docs, add group key support to DecodeAssetPayReq lightninglabs/taproot-assets#1498

Open

guggero force-pushed the group-key-support-cli branch from 672e373 to 96b3339 Compare April 30, 2025 16:35

levmi assigned guggero May 1, 2025

levmi added this to Taproot-Assets Project Board May 1, 2025

github-project-automation bot moved this to 🆕 New in Taproot-Assets Project Board May 1, 2025

levmi moved this from 🆕 New to 👀 In review in Taproot-Assets Project Board May 1, 2025

guggero force-pushed the group-key-support-cli branch from 96b3339 to 4f688aa Compare May 5, 2025 11:26

guggero force-pushed the group-key-support-cli branch from 4f688aa to c63c403 Compare May 5, 2025 13:30

levmi requested review from ffranr, Roasbeef, gijswijs and GeorgeTsagk and removed request for Roasbeef and ffranr May 6, 2025 16:56

ZZiigguurraatt suggested changes May 6, 2025

View reviewed changes

guggero force-pushed the group-key-support-cli branch 3 times, most recently from e3e8b5e to 9d9d26a Compare May 7, 2025 10:34

guggero added 3 commits May 8, 2025 14:10

mod: bump to latest tapd version

0d08752

cmd: add --group_key CLI flag to all channel commands

192c3cc

itest: test invoice decoding with group key

747706f

guggero force-pushed the group-key-support-cli branch from 9d9d26a to 747706f Compare May 8, 2025 12:18

gijswijs reviewed May 8, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cli: add group key support to channel related calls. #1052

cli: add group key support to channel related calls. #1052

guggero commented Apr 30, 2025 •

edited

Loading

ZZiigguurraatt commented Apr 30, 2025

ZZiigguurraatt commented May 2, 2025

ZZiigguurraatt commented May 2, 2025

ZZiigguurraatt commented May 2, 2025 •

edited

Loading

ZZiigguurraatt commented May 2, 2025

ZZiigguurraatt commented May 2, 2025

ZZiigguurraatt commented May 2, 2025

ZZiigguurraatt commented May 2, 2025

guggero commented May 5, 2025

guggero commented May 5, 2025

ZZiigguurraatt commented May 5, 2025

guggero commented May 5, 2025

ZZiigguurraatt commented May 5, 2025 •

edited

Loading

guggero commented May 6, 2025

ZZiigguurraatt May 2, 2025

guggero May 7, 2025

guggero May 7, 2025

ZZiigguurraatt commented May 6, 2025

guggero commented May 7, 2025

guggero commented May 7, 2025

ZZiigguurraatt commented May 8, 2025

guggero commented May 8, 2025

gijswijs left a comment

gijswijs May 8, 2025

gijswijs May 8, 2025

gijswijs May 8, 2025

gijswijs May 8, 2025

	if len(assetIDBytes) == 0 && len(groupKeyBytes) == 0 {
	if len(assetIDBytes) > 0 && len(groupKeyBytes) > 0 {

cli: add group key support to channel related calls. #1052

Are you sure you want to change the base?

cli: add group key support to channel related calls. #1052

Conversation

guggero commented Apr 30, 2025 • edited Loading

ZZiigguurraatt commented Apr 30, 2025

ZZiigguurraatt commented May 2, 2025

ZZiigguurraatt commented May 2, 2025

ZZiigguurraatt commented May 2, 2025 • edited Loading

ZZiigguurraatt commented May 2, 2025

ZZiigguurraatt commented May 2, 2025

ZZiigguurraatt commented May 2, 2025

ZZiigguurraatt commented May 2, 2025

guggero commented May 5, 2025

guggero commented May 5, 2025

ZZiigguurraatt commented May 5, 2025

guggero commented May 5, 2025

ZZiigguurraatt commented May 5, 2025 • edited Loading

guggero commented May 6, 2025

ZZiigguurraatt May 2, 2025

Choose a reason for hiding this comment

guggero May 7, 2025

Choose a reason for hiding this comment

guggero May 7, 2025

Choose a reason for hiding this comment

ZZiigguurraatt commented May 6, 2025

guggero commented May 7, 2025

guggero commented May 7, 2025

ZZiigguurraatt commented May 8, 2025

guggero commented May 8, 2025

gijswijs left a comment

Choose a reason for hiding this comment

gijswijs May 8, 2025

Choose a reason for hiding this comment

gijswijs May 8, 2025

Choose a reason for hiding this comment

gijswijs May 8, 2025

Choose a reason for hiding this comment

gijswijs May 8, 2025

Choose a reason for hiding this comment

guggero commented Apr 30, 2025 •

edited

Loading

ZZiigguurraatt commented May 2, 2025 •

edited

Loading

ZZiigguurraatt commented May 5, 2025 •

edited

Loading