Regression in CLI tool

[RCasatta]

Current master (4ede4a4ad0bbf1b9c10fda9bea33a34cfb9587ba) doesn't warn the user in case of wrong command, like:

RUST_LOG=info cargo run --release --example repl --features cli-utils,esplora,electrum -- --wallet single_change_deep --descriptor "wpkh(tpubD6NzVbkrYhZ4YmSHJMXPEvd6dnPgH55EwGBs2AJHyiBgnT7zJzQ1ywHDxxZveoApLeBeSFcjysjQ5PebSg4gsdrVdRxCLAgHK8jKUiydMrg/*)" sinc 

while for example a while back (ae16c8b) an error with suggestions was shown

error: The subcommand 'sinc' wasn't recognized
    Did you mean 'sign'?

probably due to structopt migration

[afilini]

@notmandatory do you have time to look into it?

[notmandatory]

Yes sounds like a 'structopt' issue I'll take a look today.

[notmandatory]

I verified this issue is caused by the WalletSubCommand::Other variant (with the structopt(external_subcommand) macro) which makes it impossible for structopt/clap to give a suggestion on a wrong command because it might be an external subcommand. As I see it we have four options to fix:

1. Removing the structopt(external_subcommand) and replace Other with a Repl variant
2. Keep code as is without subcommand error suggestions
3. Reverting bitcoindevkit/bdk#208 and only use clap as we were doing before
4. Remove the cli module completely and move code into repl example app

Pros/Cons

Description	1	2	3	1 + 4
Keeps easy docs and cleaner code provided by structopt	x	x		x
Can be extended with new commands without changing bdk lib code		x	x	x
Give user suggestions when command is mistyped	x		x	x

My preference is option 1 + 4 because bdk is meant to be a library and UI issues such as a CLI should be handled by the app builders who are free to copy/paste from the repl example if they want to build their own CLI tool.

Any of these options are easy changes I'd just like to get some feedback on the approach before submitting a PR.

[afilini]

I agree that we should move the cli code out of the library, but the problem with moving it into the repl module is that the code is also used by the playground we have in our website, so we would have to duplicate it because you can't import an example as a dependency.

If I had to do it, I would probably refactor it like this:

1. Make a separate crate called bdk-cli that pretty much only contains the current cli module
2. Move the repl example into the bdk-cli crate. We could add that as a binary instead of an example and also maybe use a better name, so that if somebody installs the binary on their system they can run it with bdk or bdk-cli
3. Update the playground to import bdk-cli instead of bdk

This would in the process fix a few more issues, like the fact that people are starting to use the repl example as a wallet more than a development tool. For the bdk-cli crate we could also publish pre-built binaries so that whenever somebody wants to try it out they can just download and run those.

[afilini]

Oh and I would also do what you suggested in your point 1 by removing the Other variant to get back the suggestions

[notmandatory]

Yes I agree, a new bdk-cli crate is the best way forward. This will also help simplify bdk features and dependencies.

I'll create the new repo and I should be able to keep the relevant history by starting with a clone of bdk and strip out everything except the cli and repl related parts (similar to how I removed blobs from the BDWallet repo). Once that's all working I'll do a PR to remove the cli and repl stuff from bdk.

[notmandatory]

I created the bdk-cli project and lib.rs tests and repl.rs tool are working fine there. I also fixed the bug for this issue in that repo. The next step is I'll make a PR to remove the cli.rs module and repl.rs example and their dependencies from the bdk project.

$ cargo run -- syn 
    Finished dev [unoptimized + debuginfo] target(s) in 0.09s
     Running `./repl syn`
error: The subcommand 'syn' wasn't recognized
        Did you mean 'sync'?

If you believe you received this message in error, try re-running with 'repl -- syn'

USAGE:
    repl [OPTIONS] --descriptor <DESCRIPTOR> <SUBCOMMAND>

For more information try --help

[RCasatta]

issue solved, tested with 8cd25ced0a8e37e5da94b6206258623f41569458