Allow filtering by address state when fetching all addresses

[jonathanknowles]

Issue Number

#356

Overview

I have:

• Modified the listAddresses API server function to filter by the optional state query parameter.
• Added a parseOptionalArg function to module Cardano.CLI.
• Added an optional state filter argument to the cardano-wallet address list CLI command.

Comments

[akegalj]

It would be nice if we could print help (the Docopt) to the console after the failure instead (what getArgOrExitWith does)

[jonathanknowles]

@akegalj wrote:

It would be nice if we could print help (the Docopt) to the console after the failure instead (what getArgOrExitWith does)

Thanks for this suggestion.

Actually, I can't help wondering if it wouldn't be nicer to print out something even more specific. Instead of printing out the entire help string whenever there's a problem, perhaps we could print out just the specific part that is relevant.

For example:

$ cardano-wallet address list --state on-fire 959b511ffa8213da57443226e3f9e230f956c446

Error: invalid address state: it should be either "used" nor "unused"

Usage: cardano-wallet address list [--port=INT] [--state=STRING] <wallet-id>

See here for a longer discussion: #371

I'm not sure if it's easy to do this with docopt. (Perhaps switching to optparse-applicative or some other library would make this easier.)

[jonathanknowles]

@akegalj Given that we can't (currently) print help that is specific to just one command, is it still your preference to print out the whole help string on failure? If so, I can add this in.

[jonathanknowles]

@akegalj We currently have this output when a user supplies an invalid value for --state:

$ cardano-wallet address list --state on-fire 959b511ffa8213da57443226e3f9e230f956c446
Unable to decode address state: it's neither "used" nor "unused"

[akegalj]

I am fine with the error output in this case.

And in general I am not sure that printing out the whole help as we usually do it atm is a user friendly thing to do. I would rather say "See 'cardano-wallet bla --help' for more details instead if you are interested" because if the whole help is printed then that usually hides the relevant error message.

I have proposed to print out help in this case just so that we can be consistent with the rest of the code - and so that we don't mix multiple styles of error handling in our CLI - and that is on our roadmap so I guess it will be handled soon.

[KtorZ]

@jonathanknowles Maybe this would have been useful to use here instead:

https://github.com/input-output-hk/cardano-wallet/blob/master/exe/wallet/Main.hs#L369-L377 ?

An example:

https://github.com/input-output-hk/cardano-wallet/blob/master/exe/wallet/Main.hs#L224

[jonathanknowles]

@jonathanknowles Maybe this would have been useful to use here instead:

https://github.com/input-output-hk/cardano-wallet/blob/master/exe/wallet/Main.hs#L369-L377 ?

An example:

https://github.com/input-output-hk/cardano-wallet/blob/master/exe/wallet/Main.hs#L224

@KtorZ I'll take a look at this later on to see whether we can use optional to simplify this code.

[rvl]

Looks fine, though the sooner we get rid of docopt the better.

With docopt you need to write two "parsers" for each option. The first is in the help string (written in the docopt external DSL), and the second is where our code gets the argument and applies a parse function to it.

[rvl]

Not strictly relevant to this PR, but maybe in future we should cache addresses and their status in the database?

[KtorZ]

@rvl How do you envision caching here exactly 🤔 ?

[rvl]

Rebased on latest master to resolve merge conflict.

[KtorZ]

Looks fine, though the sooner we get rid of docopt the better.

@rvl As soon as we are done with Jormungandr's integration, I think this one will be good to have in the pipeline.