litcli: drop unused flags from addinvoice command#1335
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request cleans up the command-line interface for 'litcli ln addinvoice' by filtering out flags that are inherited from 'lncli' but are not supported by the Taproot Assets invoice implementation. By removing these irrelevant options, the command's help documentation is more accurate and less confusing for users, while ensuring that all functional flags remain available. Highlights
New Features🧠 You can now enable Memory (public preview) to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
Signed-off-by: 0xfandom <kashyapshivank01@gmail.com>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a mechanism to filter out unsupported inherited flags (such as cltv_expiry_delta and blinded path options) from the addinvoice command in litcli, as they do not apply to Taproot Asset invoices. It also adds a corresponding unit test to verify that the correct flags are retained and unsupported ones are removed. The review feedback highlights a potential issue where flag.GetName() can return comma-separated aliases (e.g., 'blind, b'), which would bypass the filtering logic and cause test inaccuracies. It is recommended to split and trim the flag names by commas in both the filtering function and the test setup to ensure robust matching.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| func withoutUnsupportedAddInvoiceFlags(flags []cli.Flag) []cli.Flag { | ||
| filtered := make([]cli.Flag, 0, len(flags)) | ||
| for _, flag := range flags { | ||
| _, unsupported := addInvoiceUnsupportedFlags[flag.GetName()] | ||
| if unsupported { | ||
| continue | ||
| } | ||
|
|
||
| filtered = append(filtered, flag) | ||
| } | ||
|
|
||
| return filtered | ||
| } |
There was a problem hiding this comment.
In urfave/cli (v1), flag.GetName() returns the raw Name string, 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.
func withoutUnsupportedAddInvoiceFlags(flags []cli.Flag) []cli.Flag {
filtered := make([]cli.Flag, 0, len(flags))
for _, flag := range flags {
var unsupported bool
for _, part := range strings.Split(flag.GetName(), ",") {
if _, ok := addInvoiceUnsupportedFlags[strings.TrimSpace(part)]; ok {
unsupported = true
break
}
}
if unsupported {
continue
}
filtered = append(filtered, flag)
}
return filtered
}| flagNames := make(map[string]struct{}) | ||
| for _, flag := range addInvoiceCommand.Flags { | ||
| flagNames[flag.GetName()] = struct{}{} | ||
| } |
There was a problem hiding this comment.
Similar to the implementation in ln.go, flag.GetName() can return a string containing aliases (e.g., "blind, b"). If a flag has aliases, flagNames[flag.GetName()] will store the full string, and checking flagNames["blind"] will return false even though the flag is still present. This can lead to false positives in the test.
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 "strings" in this test file.
| flagNames := make(map[string]struct{}) | |
| for _, flag := range addInvoiceCommand.Flags { | |
| flagNames[flag.GetName()] = struct{}{} | |
| } | |
| flagNames := make(map[string]struct{}) | |
| for _, flag := range addInvoiceCommand.Flags { | |
| for _, part := range strings.Split(flag.GetName(), ",") { | |
| flagNames[strings.TrimSpace(part)] = struct{}{} | |
| } | |
| } |
9a71faf to
0e04cd5
Compare
|
Dropped the release-notes entry for now — it was conflicting with |
Signed-off-by: 0xfandom <kashyapshivank01@gmail.com>
|
Update: re-added the release note after all — placed it under "Technical and Architectural Updates", which merges cleanly against the current master, so the |
|
@0xfandom, remember to re-request review from reviewers when ready |
|
|
||
| ### Technical and Architectural Updates | ||
|
|
||
| * [Hide unsupported flags from `litcli ln |
There was a problem hiding this comment.
Please update this to instead be part of the release notes for v0.17.1:
https://github.com/lightninglabs/lightning-terminal/blob/master/docs/release-notes/release-notes-0.17.1.md
Signed-off-by: 0xfandom <kashyapshivank01@gmail.com>
1c92ee3 to
16d1284
Compare
|
Done — moved the entry to |
|
Heads-up: the bbolt |
The litcli `ln addinvoice` command inherits all of lncli's addinvoice flags but its action only acts on a subset. The blinded-path flags and cltv_expiry_delta are never read and don't apply to Taproot Asset invoices, yet they still show up in `litcli ln addinvoice -h`, which is confusing. Filter those flags out so the help output only lists options that have an effect, and add a test asserting the command keeps the flags it uses and drops the unsupported ones. Addresses lightninglabs#1121 Signed-off-by: 0xfandom <kashyapshivank01@gmail.com>
Signed-off-by: 0xfandom <kashyapshivank01@gmail.com>
16d1284 to
0f59612
Compare
|
Rebased onto the latest master — #1322 landed a new entry in the same Bug Fixes section, which put this branch into a conflicted state. Both entries are kept now and the branch merges cleanly again. The earlier bbolt itest failure should get a fresh run off the new head. |
3 participants
Change Description
Fixes #1121.
litcli ln addinvoicebuilds its flag set by appending the asset-specific flags ontolncli's fulladdinvoiceflag list. As a result,litcli ln addinvoice -hadvertises a number of flags that the command's action never reads and that don't apply to Taproot Asset invoices.This filters out the flags that are demonstrably unused by the
addInvoiceaction:cltv_expiry_deltablind,min_real_blinded_hops,num_blinded_hops,max_blinded_paths,blinded_path_omit_node,blinded_path_incoming_channel_listThese are never referenced when building the
AddInvoiceRequest, so removing them from the command only changes the help output, not behaviour. A test asserts the command keeps every flag the action consumes (including the asset flags) and drops the unsupported ones.Note:
--private,--ampand--fallback_addrare left in place for now since the action does pass them through to the RPC. If the Taproot Assets invoice path doesn't honour them either, they can be removed in a follow-up — happy to do that here too if preferred.Steps to Test
The blinded-path options and
--cltv_expiry_deltashould no longer be listed. Or run the unit test:Pull Request Checklist