Migrate from structopt to clap.

rajarshimaitra commented 1 year ago

Description

Fixes #113.

This is an attempt to migrate from structopt to clap v0.3 which provides very similar kind of derives as structopt. Changes are straight forward. But this comes with few more problems.

with clap v3.2.22 the MSRV pushes up to 1.57.0.. The last clap of MSRV 1.56.0 was clap 3.2.5.. But even that doesn't seem to be working at MSRV 1.56.0 anymore, as bunch of underlying lib has upgraded.
clap v3.0 doesn't seem to support custom vector parsing well, reported here https://github.com/clap-rs/clap/issues/1704. This is required for recipient parsing which is a Vec<(Script, u32)>. Workaround for that is to use vecs of strings and parse them at runtime in create_tx handler. Included in the PR.

Notes to the reviewers

structopt is currently freezed at clap 2.0 and doesn't seem to intend on updating and currently its has unmaintained vulnerability. And clap v3.0 onward seems to replacing everything that structopt did before. So this means we should also look for ways to migrate from strcutopt to clap.. But clap seemed to have moved ahead than our MSRV 1.56.0.. So we need to take up a call on that.. Opened this PR to facilitate that discussion..

This is draft until we figure what to do..

Checklists

All Submissions:

[x] I've signed all my commits
[x] I followed the contribution guidelines
[x] I ran cargo fmt and cargo clippy before committing

notmandatory commented 1 year ago

Thanks for starting this! I don't see a problem updating bdk-cli MSRV to 1.57.0. It's only the core bdk libs that we want to be able to support projects using older rust versions.

rajarshimaitra commented 1 year ago

Thanks @notmandatory .. If we can move up to 1.57 for bdk-cli then that resolves major chunk of the issue.. Will update the PR accordingly..

rajarshimaitra commented 1 year ago

@notmandatory updated with few more fixes..

The clap vector of string parsing is not working properly.. Creating a new issue for it..
The 1.56 tests are still running.. Anyway to disable them in this PR?

rajarshimaitra commented 1 year ago

Documented the issue here https://github.com/bitcoindevkit/bdk-cli/issues/126

notmandatory commented 1 year ago

I'll need to change the branch protection rules to require 1.57 instead of 1.56. That means this this PR would need to get merged next and any others rebased on it. Is that the order you want to do things?

rajarshimaitra commented 1 year ago

I think #123 should go in first.. This can go after.. Or maybe we can also combine this and #125 together and update the CI stuffs in one go?

rajarshimaitra commented 1 year ago

@notmandatory Updated this to bdk v0.23.. Fixed #126 .. Turns out the only way to parse Vec is by using multiple flags.. This is now good to go.. And other PRs would be rebased..

notmandatory commented 1 year ago

I updated the branch protection rules to check for the tests run against 1.57 instead of 1.56. Looks like you just need to fix up the WASM.

rajarshimaitra commented 1 year ago

Sorry its taking some time and iteration.. I am not being able to play the wasm run in local.. So trying up with the CI outputs here..

Seems like it fixed the issue but now clippy is throwing an unused import warning, which is error is CI.

  cargo clippy -- -D warnings
  shell: /usr/bin/bash -e {0}
   Compiling bdk-cli v0.6.0 (/home/runner/work/bdk-cli/bdk-cli)
error: unused import: `Args`
  --> src/commands.rs:16:25
   |
16 | use clap::{AppSettings, Args, Parser, Subcommand};
   |                         ^^^^
   |
   = note: `-D unused-imports` implied by `-D warnings`

error: could not compile `bdk-cli` due to previous error
Error: Process completed with exit code 101.

But that import Args is definitely used, and removing it causes build failure.. Any idea??

rajarshimaitra commented 1 year ago

:tada: :tada: It worked.. @notmandatory this is now good for merge..

bitcoindevkit / bdk-cli