-
Notifications
You must be signed in to change notification settings - Fork 128
litcli: drop unused flags from addinvoice command #1335
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
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,37 @@ | ||||||||||||||||||||||
| package main | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| import ( | ||||||||||||||||||||||
| "testing" | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| "github.com/stretchr/testify/require" | ||||||||||||||||||||||
| ) | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| // TestAddInvoiceCommandFlags ensures that the litcli addinvoice command keeps | ||||||||||||||||||||||
| // the flags its action actually reads (including the asset-specific ones it | ||||||||||||||||||||||
| // adds) while dropping the inherited lncli flags that don't apply to Taproot | ||||||||||||||||||||||
| // Asset invoices. | ||||||||||||||||||||||
| func TestAddInvoiceCommandFlags(t *testing.T) { | ||||||||||||||||||||||
| t.Parallel() | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| flagNames := make(map[string]struct{}) | ||||||||||||||||||||||
| for _, flag := range addInvoiceCommand.Flags { | ||||||||||||||||||||||
| flagNames[flag.GetName()] = struct{}{} | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
Comment on lines
+16
to
+19
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Similar to the implementation in We should split and trim the flag names here as well to ensure all names and aliases are correctly populated in the map. Note that you will need to import
Suggested change
|
||||||||||||||||||||||
|
|
||||||||||||||||||||||
| // Flags that the addInvoice action reads must remain available. | ||||||||||||||||||||||
| expected := []string{ | ||||||||||||||||||||||
| "memo", "preimage", "amt", "amt_msat", "description_hash", | ||||||||||||||||||||||
| "fallback_addr", "expiry", "private", "amp", | ||||||||||||||||||||||
| "asset_id", "group_key", "asset_amount", "rfq_peer_pubkey", | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
| for _, name := range expected { | ||||||||||||||||||||||
| _, ok := flagNames[name] | ||||||||||||||||||||||
| require.Truef(t, ok, "expected flag %q to be present", name) | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| // Flags that don't apply to asset invoices must be removed. | ||||||||||||||||||||||
| for name := range addInvoiceUnsupportedFlags { | ||||||||||||||||||||||
| _, ok := flagNames[name] | ||||||||||||||||||||||
| require.Falsef(t, ok, "expected flag %q to be removed", name) | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In
urfave/cli(v1),flag.GetName()returns the rawNamestring, which can contain comma-separated aliases (e.g.,"blind, b"). If any of the inherited flags are defined with aliases now or in the future,addInvoiceUnsupportedFlags[flag.GetName()]will fail to match, and the unsupported flag will not be filtered out.To make this robust against aliases, we should split the flag name by
","and trim whitespace before checking against the unsupported map.