[Merged by Bors] - Interactive account passwords #1623
Conversation
…ractive-account-passwords
There was a problem hiding this comment.
Looks great! I ran it with and without the interactive prompts without issue. I think it's going to be easier for beginners.
I've just left a few nits, happy to merge after that!
It would be cool to update the docs, since I think this interactive password flow is easier. Perhaps we could just do 4.1 and 4.2 in the validator guide here now and do the rest later?
| use crate::{common::ensure_dir_exists, SECRETS_DIR_FLAG, VALIDATOR_DIR_FLAG}; | ||
| use account_utils::{random_password, strip_off_newlines, validator_definitions}; | ||
|
|
There was a problem hiding this comment.
I think it would be nice to delete this line.
| use clap::{App, Arg, ArgMatches}; | ||
| use environment::Environment; | ||
| use eth2_wallet::PlainText; | ||
|
|
account_manager/src/wallet/create.rs
Outdated
| @@ -126,30 +139,84 @@ pub fn create_wallet_from_mnemonic( | |||
| .map_err(|e| format!("Unable to open --{}: {:?}", BASE_DIR_FLAG, e))?; | |||
|
|
|||
| // Create a random password if the file does not exist. | |||
There was a problem hiding this comment.
We might want to move the comment down a few lines, it's not completely accurate here.
common/account_utils/src/lib.rs
Outdated
| /// Takes a string password and checks that it meets minimum requirements. | ||
| /// | ||
| /// The current minimum password requirement is a 12 character length character length. | ||
| pub fn is_password_sufficiently_complex(password: &[u8]) -> bool { |
There was a problem hiding this comment.
It's not very important, but if this returned Result<(), String> you could avoid repeating "Please use at least {} characters for your password." in downstream functions by passing that message via the Result::Err(String).
There was a problem hiding this comment.
That's smart, easier to update error messages if the password requirements change too.
paulhauner
added
waiting-on-author
realbigsean
added
ready-for-review
paulhauner
added
ready-for-merge
## Issue Addressed #1437 ## Proposed Changes - Make the `--wallet-password` flag optional and creates an interactive prompt if not provided. - Make the `--wallet-name` flag optional and creates an interactive prompt if not provided. - Add a minimum password requirement of a 12 character length. - Update the `--stdin-passwords` flag to `--stdin-inputs` because we have non-password user inputs ## Additional Info
|
Pull request successfully merged into master. Build succeeded: |
bors
bot
changed the title
Issue Addressed
#1437
Proposed Changes
--wallet-passwordflag optional and creates an interactive prompt if not provided.--wallet-nameflag optional and creates an interactive prompt if not provided.--stdin-passwordsflag to--stdin-inputsbecause we have non-password user inputsAdditional Info