Skip to content

Conversation

@GeorgeTsagk
Copy link
Member

@GeorgeTsagk GeorgeTsagk commented May 16, 2025

Description

Uses the existing LND arguments for sat/msat amount for the litcli ln addinvoice command. This new argument exposes the behavior introduced in lightninglabs/taproot-assets#1448

Closes #1073

@GeorgeTsagk GeorgeTsagk self-assigned this May 16, 2025
cmd/litcli/ln.go Outdated
"exclusive to msat_amount",
},
cli.Uint64Flag{
Name: "msat_amount",
Copy link
Contributor

Choose a reason for hiding this comment

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

If you look at litcli ln addinvoice, you see that we already have all flags from commands.AddInvoiceCommand.Flags:

   --amt value                      the amt of satoshis in this invoice (default: 0)
   --amt_msat value                 the amt of millisatoshis in this invoice (default: 0)

So we should use those.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks!

Also wondering why we do field related checks on the cmd level, as they are also checked in tapd/lnd

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok realized the RPCs don't always return user-friendly errors, so made the checks in cmd too (much better to get a single text message back saying must only set X or Y)

@GeorgeTsagk GeorgeTsagk force-pushed the ln-addinvoice-msats branch 2 times, most recently from 996a024 to ebd84bc Compare May 16, 2025 14:41
@GeorgeTsagk GeorgeTsagk changed the title Add new msat_amount flag to ln addinvoice Use sat/msat arguments of ln addinvoice May 16, 2025
@GeorgeTsagk GeorgeTsagk requested a review from guggero May 19, 2025 10:42
@GeorgeTsagk GeorgeTsagk force-pushed the ln-addinvoice-msats branch 2 times, most recently from 5e9819e to d29c669 Compare May 19, 2025 11:51
@GeorgeTsagk GeorgeTsagk requested a review from guggero May 19, 2025 13:16
Copy link
Contributor

@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.

tACK. While testing I found out that --amt doesn't work, only --amt_msat. But that needs to be fixed in tapd, which I'm going to do now.

@guggero
Copy link
Contributor

guggero commented May 20, 2025

Needs a rebase to get the itests running.

@GeorgeTsagk
Copy link
Member Author

tACK. While testing I found out that --amt doesn't work, only --amt_msat. But that needs to be fixed in tapd, which I'm going to do now.

Ah! great catch, I believe I only tried amt_msat while testing locally and didn't run into it. Ty!

@guggero
Copy link
Contributor

guggero commented May 20, 2025

LOL, apparently if you use the "fixes" keyword and then link to another PR, that one is closed... Sorry, my bad.

@guggero guggero merged commit 605bf51 into lightninglabs:master May 20, 2025
40 of 44 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[bug]: litcli ln addinvoice --amt 1000 does not work

2 participants