Group up Software and Trezor args in separate subcommands #1907
Conversation
OBorce
force-pushed
the
feature/wallet-cli-trezor-commands
branch
2 times, most recently
from
9c3f541 to
4a31c9a
Compare
OBorce
force-pushed
the
feature/trezor-recconect
branch
from
844ee5b to
37def6a
Compare
OBorce
force-pushed
the
feature/wallet-cli-trezor-commands
branch
from
4a31c9a to
5921893
Compare
wallet/wallet-cli-lib/src/lib.rs
Outdated
| let wallet = match args.hardware_wallet { | ||
| None => OpenWalletSubCommand::Software { | ||
| wallet_path, | ||
| encryption_password: args.wallet_password, | ||
| force_change_wallet_type: args.force_change_wallet_type, | ||
| }, | ||
| Some(hw) => match hw { | ||
| #[cfg(feature = "trezor")] | ||
| wallet_rpc_lib::cmdline::CliHardwareWalletType::Trezor => { | ||
| OpenWalletSubCommand::Trezor { | ||
| wallet_path, | ||
| device_id: None, | ||
| } | ||
| } | ||
| }, | ||
| }; |
There was a problem hiding this comment.
- A "hardware" wallet file can still be encrypted. Not sure how useful this feature is, but we still have it.
- The "wallet_password" option can still be specified when opening a HW wallet file, but now it will be silently ignored, which is not good.
I suggest putting encryption_password into OpenWalletSubCommand::Trezor too.
Later we can decide whether encrypting a HW wallet file makes sense, but if it doesn't, we should remove this feature globally (e.g. in GUI too).
There was a problem hiding this comment.
added, forgot that the standalone private keys are also encrypted.
| CreateWallet { | ||
| #[derive(Debug, Subcommand, Clone)] | ||
| pub enum CreateWalletSubCommand { | ||
| /// Create a software wallet |
There was a problem hiding this comment.
In the "recover" case this looks a bit off, same for the Trezor case below.
E.g. wallet-recover --help currently prints "Recover new wallet, this will rescan the blockchain upon creation". And then it prints
Commands:
software Create a software wallet
"Create" looks strange here IMO.
I guess we should at least fix the "Recover new wallet" part (which sounds strange on its own). E.g. "Recover a previously existing wallet. This will create a new wallet file and rescan the blockchain upon creation.". This way "Create" may refer to the creation of the wallet file, so it should sound ok-ish.
But wallet-recover software --help also prints "Create a software wallet" and in this case no additional info is printed and "Create" still sounds off.
So I'm wondering whether we need a separate RecoverWalletSubCommand with the same contents but different doc strings. (Or may be I'm overthinking this)
There was a problem hiding this comment.
split up Create and Recover into 2 types with different comments
OBorce
force-pushed
the
feature/wallet-cli-trezor-commands
branch
from
5921893 to
32f5d6e
Compare
Uh oh!
There was an error while loading. Please reload this page.