Allow enabling at most one blockchain client feature

notmandatory commented 3 years ago

Description

Allow at most one blockchain client feature be enabled at a time for builds. If no blockchain client feature is enabled then online wallet commands are disabled. This will simplify the options shown to the user and make adding new blockchain clients (such as #36) easier. ~~Electrum is still the default, to make a build with a different blockchain client the --no-default-features build option will need to be used.~~ No blockchain client is included in the default features, so if one is needed it must be specified with --features. I also added a default esplora server url so the user doesn't need to specify one if selecting that client, which is how the electrum and compact_filters clients work.

Notes to the reviewers

I changed the server option for both electrum and esplora to --server or -s since that now won't cause a conflict. I also simplified the CHANGELOG to focus on what a user would see as a change while using the bin.

I've also added a build.rs file to prevent more than one blockchain client feature from being enabled.

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

New Features:

[ ] I've added tests for the new feature
[x] I've added docs for the new feature
[x] I've updated CHANGELOG.md

Bugfixes:

[ ] This pull request breaks the existing API
[ ] I've added tests to reproduce the issue which are now passing
[ ] I'm linking the issue being fixed by this PR

rajarshimaitra commented 3 years ago

Also, I am not sure if we should silently ignore "no binary built" scenarios in CI.

https://github.com/bitcoindevkit/bdk-cli/blob/378b33aabcd2812ed431ed650a3f4418acc034d5/.github/workflows/cont_integration.yml#L43

with current changes for example cargo build --features esplora --no-default-features will not build any binary, but the CI doesn't complain.

you can check that locally with

$ cargo clean
$ cargo build --features esplora --no-default-features
$ ./target/debug/bdk-cli wallet --help
bash: ./target/debug/bdk-cli: No such file or directory

I am not sure how then it's passing the tests without a binary.

notmandatory commented 3 years ago

I think tests were passing because they only needed a lib build to run. But I agree it would be nice to not have to use the --no-default-features flag. I did the following to try and address this:

split the repl feature into cli for the required dependencies to build the cli bin, and repl to enable the cli repl commands, then made the cli feature required to build the cli bin, but the cli and repl features on by default.
added cfg statements so someone could build the cli without the repl command if they want fewer dependencies and use the --no-default-features flag
update the CI to test with the default (no blockchain client) features, or each blockchain client, or the 'compiler' feature

rajarshimaitra commented 3 years ago

The following structure and function call could use feature guard. They exist in binary compiled without a backend, but are never used.

https://github.com/notmandatory/bdk-cli/blob/69b1d80bcd4715a3b78af4aafacebe2fc5ca2b88/src/lib.rs#L892

https://github.com/notmandatory/bdk-cli/blob/69b1d80bcd4715a3b78af4aafacebe2fc5ca2b88/src/lib.rs#L638

rajarshimaitra commented 3 years ago

ACK https://github.com/bitcoindevkit/bdk-cli/pull/38/commits/062542ae8957b2fef182588f7c786412545f228f

notmandatory commented 3 years ago

Thanks for the suggestions and review on this @rajarshimaitra, all ready to rebase your #36 PR.

bitcoindevkit / bdk-cli