Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Re-organize wallet sub-commands and add key sub-commands #14

Merged
merged 18 commits into from
Feb 10, 2021

Conversation

notmandatory
Copy link
Member

@notmandatory notmandatory commented Feb 3, 2021

Description

Depends on bitcoindevkit/bdk#274

Added in Lib

  • CliOpts struct and CliSubCommand enum representing top level cli options and commands
  • KeySubCommand enum
  • handle_key_subcommand function

Changed in Lib

  • Renamed WalletOpt struct to WalletOpts
  • WalletSubCommand enum split into OfflineWalletSubCommand and OnlineWalletSubCommand
  • Split handle_wallet_subcommand into two functions, handle_offline_wallet_subcommand and handle_online_wallet_subcommand
  • A wallet without a Blockchain is used when handling offline wallet sub-commands

Added in bdk-cli

  • Top level commands "wallet", "key", and "repl"
  • "key" command has sub-commands to "generate" and "restore" a master extended key
  • "repl" command now has an "exit" sub-command, fixes Add subcommand to break out of the repl #12

Changed in bdk-cli

  • "wallet" sub-commands and options must be proceeded by "wallet" command
  • "repl" command loop now includes both "wallet" and "key" sub-commands

Notes to the reviewers

There's a little bug in the help / about info while in REPL mode. I don't think there's a way to work around it but there is a structopt issue for it. I found a workaround for this issue.

Checklists

All Submissions:

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

New Features:

  • I've added tests for the new feature
  • I've added docs for the new feature
  • I've updated CHANGELOG.md

@notmandatory notmandatory added the enhancement New feature or request label Feb 3, 2021
@notmandatory notmandatory changed the title Feat/key cmds Re-organize wallet sub-commands and add key sub-commands Feb 3, 2021
@notmandatory notmandatory marked this pull request as ready for review February 4, 2021 07:03
@thunderbiscuit
Copy link
Member

ACK.

I have tested this locally and everything works well! I have not reviewed the code, however.

@notmandatory
Copy link
Member Author

I added a small change to bdk_cli.rs to remove the unneeded ReplOpt struct.

@notmandatory notmandatory force-pushed the feat/key_cmds branch 2 times, most recently from 2b67326 to e64696d Compare February 6, 2021 18:14
@afilini
Copy link
Member

afilini commented Feb 8, 2021

Reviewing it now :)

src/bdk_cli.rs Outdated Show resolved Hide resolved
src/bdk_cli.rs Outdated Show resolved Hide resolved
src/bdk_cli.rs Outdated Show resolved Hide resolved
src/bdk_cli.rs Outdated
let repl_subcommand: Result<ReplOpt, clap::Error> =
ReplOpt::from_iter_safe(split_line);
let split_line: Vec<&str> =
split_regex.find_iter(&line).map(|m| m.as_str()).collect();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

... then here you can iterate over the captured groups and remove the quotes at the same time:

Suggested change
split_regex.find_iter(&line).map(|m| m.as_str()).collect();
split_regex.captures_iter(&line).map(|c| c.get(1).or(c.get(2)).unwrap().as_str()).collect();

The c.get(1).or(c.get(2)) is necessary because sometimes it's the first capture group that matches and sometimes the second, but since there are only two of them you can safely unwrap() later because either one or the other always matches.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh cool, I'll give it a try!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes this is much better, I also added the capture group you suggested for single quotes, and fixed it so the double quote and single quote capture groups can take non-alphanumeric characters (other than " or ')

src/lib.rs Outdated
Comment on lines 151 to 162
/// #[cfg(feature = "electrum")]
/// proxy: None,
/// #[cfg(feature = "electrum")]
/// retries: 5,
/// #[cfg(feature = "electrum")]
/// timeout: None,
/// #[cfg(feature = "electrum")]
/// electrum: "ssl://electrum.blockstream.info:60002".to_string(),
/// #[cfg(feature = "esplora")]
/// esplora: None,
/// #[cfg(feature = "esplora")]
/// esplora_concurrency: 4,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe you could add another enum called ElectrumOpts/EsploraOpts and then use #[structopt(flatten)]? I think it would keep the same "public" interface, but internally it would be a bit more organized

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes good call, much cleaner this way. I originally had these two structs but took them out when I was trouble shooting some structopt help issues. Back in now.

src/lib.rs Outdated
Comment on lines 551 to 552
pub fn handle_offline_wallet_subcommand<D>(
wallet: &Wallet<(), D>,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could change this to take a generic type T instead of just (), which would allow you to call this with a ref to an online wallet

Suggested change
pub fn handle_offline_wallet_subcommand<D>(
wallet: &Wallet<(), D>,
pub fn handle_offline_wallet_subcommand<T, D>(
wallet: &Wallet<T, D>,

src/bdk_cli.rs Outdated
Comment on lines 189 to 190
let offline_wallet = new_offline_wallet(network, &wallet_opts, database).unwrap();
let offline_wallet = Arc::new(offline_wallet);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

... then you could remove this and just use the online_wallet everywhere

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added the generic type to allow handle_offline_wallet_subcommand to accept an online wallet and works great, removed the offline_wallet for REPL mode.

I'm going to keep the offline_wallet for the offline commands when not in REPL mode. My goal is to eventually make offline, online, and repl optional cargo features. So someone could make a bdk-cli build to only do offline CLI commands. Or maybe only offline via CLI or REPL, etc.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, sounds good!

src/bdk_cli.rs Outdated
let result = match repl_subcommand {
ReplSubCommand::OnlineWalletSubCommand(online_subcommand) => {
bdk_cli::handle_online_wallet_subcommand(
&Arc::clone(&online_wallet),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you could just use &online_wallet here and in all the other places

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, I think you could remove the Arc stuff entirely.. I don't think we are using them anymore, we are just passing normal references around

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed the Arc and it works fine.

Cargo.toml Outdated
@@ -12,8 +12,8 @@ readme = "README.md"
license = "MIT"

[dependencies]
bdk = { version = "^0.3", default-features = false }
bdk-macros = "^0.2"
bdk = { git = "https://github.com/bitcoindevkit/bdk.git", rev = "c4f2179", features = ["all-keys"]}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs default-features = false, otherwise it'll include electrum by default which can't be compiled under wasm32

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed. Had to tweek docs tests a bit but should all work now.

src/lib.rs Show resolved Hide resolved
@afilini afilini merged commit 9fb50b1 into bitcoindevkit:master Feb 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add subcommand to break out of the repl
3 participants