Some Lotus CLI commands don't enforce number of arguments
rvagg opened this issue · 5 comments
Ref: #12788
Lines 735 to 739 in 7f2068e
lotus wallet add
does a < 1
check on NArg()
, but it really should be != 1
to match the usage <amount>
. This would pick up any mis-use, like supplying the options too late (thinking that it's got GNU CLI flexibility when it's unfortunately Go's frustrating typical BSD CLI inflexibility). I haven't looked but it's likely that there are more in here with this same problem.
Marking this job as good first issue
because it's really just a fairly straightforward audit; walking through all of the commands attached to the lotus
CLI (we'll scope it to that for now) and checking that NArgs
is being properly enforced to match the usage (or vice versa).
Ideally the CLI package should be able to understand its usage string and enforce that, or maybe we could come up with our own usage string parser (e.g. "<foo> [bar] [baz]" should have >=1 && <=3
); but that could be a later-job and for now just getting them matched would be good.
would like to take this up!
Great, consider it yours @ameeetgaikwad! I don't know how many commands are in this situation so this is going to require a manual walk through the lotus
CLI commands. There are few things to be aware of:
- There is more than just
lotus
in the codebase, there's a bunch of CLI tools that aren't in scope here (lotus-miner
,lotus-shed
, etc.); the scope of all of them is pretty huge so let's limit the task to justlotus
. - #12717 is due to land soon which will change things a bit, so be aware there will be some code churn (you're welcome to start from that branch if you like under the expectation that it represents the way the code will be soon)
- Command setup is done here: https://github.com/filecoin-project/lotus/blob/c76d6b97f44b5a86888c41e521d47670d86d1f20/cmd/lotus/main.go#L116-L117 - note that there's two separate batches of commands appended,
local
and the ones inclicommands.Commands
. The latter are the larger main group but the fromer have a few special commands too that might be worth a look. You want to walk through each of the commands that go into this and see if they are doing the checks properly.
@ameeetgaikwad I assigned the issue to you, and set its status to "In Progress". Let us know if you have any questions with regards to the issue, or need help to get unblocked on any techincal matters