From 89e279f9c8185b2ae39e7d395a528a5992962933 Mon Sep 17 00:00:00 2001 From: rkapka Date: Thu, 20 Aug 2020 21:14:03 +0200 Subject: [PATCH] Account commands cleanup (#7065) * initial implementation of command * Add second confirmation step * Merge branch 'origin-master' into voluntary-exit-command * fix variable name * added unit tests * Merge branch 'origin-master' into voluntary-exit-command * add comment about stdin * remove backup-related code from accounts_delete_test * fix comments * move filterPublicKeysFromUserInput to new account helper * remove SkipMnemonicConfirmFlag * Merge refs/heads/master into voluntary-exit-command * Merge refs/heads/master into voluntary-exit-command * Merge refs/heads/master into voluntary-exit-command * Merge refs/heads/master into voluntary-exit-command * Merge refs/heads/master into voluntary-exit-command * Merge refs/heads/master into voluntary-exit-command * Merge refs/heads/master into voluntary-exit-command * code review fixes * removed commented-out code * Merge branch 'voluntary-exit-command' into account-commands-cleanup # Conflicts: # shared/promptutil/prompt.go * Merge branch 'origin-master' into account-commands-cleanup # Conflicts: # shared/promptutil/prompt.go # validator/accounts/v2/BUILD.bazel # validator/accounts/v2/accounts_exit.go # validator/accounts/v2/accounts_exit_test.go * Merge refs/heads/master into account-commands-cleanup --- shared/promptutil/prompt.go | 2 +- validator/accounts/v2/BUILD.bazel | 1 + validator/accounts/v2/accounts_backup.go | 36 --------------- validator/accounts/v2/accounts_create.go | 4 +- validator/accounts/v2/accounts_delete.go | 6 +-- validator/accounts/v2/accounts_delete_test.go | 21 +-------- validator/accounts/v2/accounts_exit.go | 4 +- validator/accounts/v2/accounts_helper.go | 46 +++++++++++++++++++ validator/accounts/v2/wallet_test.go | 2 - validator/flags/flags.go | 9 +--- 10 files changed, 58 insertions(+), 73 deletions(-) create mode 100644 validator/accounts/v2/accounts_helper.go diff --git a/shared/promptutil/prompt.go b/shared/promptutil/prompt.go index 138eb285347f..01f6cf17f0ce 100644 --- a/shared/promptutil/prompt.go +++ b/shared/promptutil/prompt.go @@ -29,7 +29,7 @@ const ( ConfirmPass ) -// ValidatePrompt requests the user for text and expects it to fulfil la provided validation function. +// ValidatePrompt requests the user for text and expects the user to fulfill the provided validation function. func ValidatePrompt(r io.Reader, promptText string, validateFunc func(string) error) (string, error) { var responseValid bool var response string diff --git a/validator/accounts/v2/BUILD.bazel b/validator/accounts/v2/BUILD.bazel index d7b4b51dfbc5..5566b3334e9a 100644 --- a/validator/accounts/v2/BUILD.bazel +++ b/validator/accounts/v2/BUILD.bazel @@ -8,6 +8,7 @@ go_library( "accounts_create.go", "accounts_delete.go", "accounts_exit.go", + "accounts_helper.go", "accounts_import.go", "accounts_list.go", "cmd_accounts.go", diff --git a/validator/accounts/v2/accounts_backup.go b/validator/accounts/v2/accounts_backup.go index 9df47385e170..43d53dd979c0 100644 --- a/validator/accounts/v2/accounts_backup.go +++ b/validator/accounts/v2/accounts_backup.go @@ -3,7 +3,6 @@ package v2 import ( "archive/zip" "context" - "encoding/hex" "encoding/json" "fmt" "os" @@ -114,41 +113,6 @@ func BackupAccounts(cliCtx *cli.Context) error { return zipKeystoresToOutputDir(keystoresToBackup, backupDir) } -func filterPublicKeysFromUserInput( - cliCtx *cli.Context, - publicKeysFlag *cli.StringFlag, - validatingPublicKeys [][48]byte, - selectionPrompt string, -) ([]bls.PublicKey, error) { - var filteredPubKeys []bls.PublicKey - if cliCtx.IsSet(publicKeysFlag.Name) { - pubKeyStrings := strings.Split(cliCtx.String(publicKeysFlag.Name), ",") - if len(pubKeyStrings) == 0 { - return nil, fmt.Errorf( - "could not parse %s. It must be a string of comma-separated hex strings", - publicKeysFlag.Name, - ) - } - for _, str := range pubKeyStrings { - pkString := str - if strings.Contains(pkString, "0x") { - pkString = pkString[2:] - } - pubKeyBytes, err := hex.DecodeString(pkString) - if err != nil { - return nil, errors.Wrapf(err, "could not decode string %s as hex", pkString) - } - blsPublicKey, err := bls.PublicKeyFromBytes(pubKeyBytes) - if err != nil { - return nil, errors.Wrapf(err, "%#x is not a valid BLS public key", pubKeyBytes) - } - filteredPubKeys = append(filteredPubKeys, blsPublicKey) - } - return filteredPubKeys, nil - } - return selectAccounts(selectionPrompt, validatingPublicKeys) -} - // Ask user to select accounts via an interactive prompt. func selectAccounts(selectionPrompt string, pubKeys [][48]byte) ([]bls.PublicKey, error) { pubKeyStrings := make([]string, len(pubKeys)) diff --git a/validator/accounts/v2/accounts_create.go b/validator/accounts/v2/accounts_create.go index 68a95381eaa7..9d355560214a 100644 --- a/validator/accounts/v2/accounts_create.go +++ b/validator/accounts/v2/accounts_create.go @@ -25,8 +25,8 @@ func CreateAccount(cliCtx *cli.Context) error { if err != nil { return err } - skipMnemonicConfirm := cliCtx.Bool(flags.SkipMnemonicConfirmFlag.Name) - keymanager, err := wallet.InitializeKeymanager(cliCtx, skipMnemonicConfirm) + + keymanager, err := wallet.InitializeKeymanager(cliCtx, false /* skip mnemonic confirm */) if err != nil && strings.Contains(err.Error(), "invalid checksum") { return errors.New("wrong wallet password entered") } diff --git a/validator/accounts/v2/accounts_delete.go b/validator/accounts/v2/accounts_delete.go index de6c1c121440..84875eb0a5fa 100644 --- a/validator/accounts/v2/accounts_delete.go +++ b/validator/accounts/v2/accounts_delete.go @@ -24,8 +24,8 @@ func DeleteAccount(cliCtx *cli.Context) error { } else if err != nil { return errors.Wrap(err, "could not open wallet") } - skipMnemonicConfirm := cliCtx.Bool(flags.SkipMnemonicConfirmFlag.Name) - keymanager, err := wallet.InitializeKeymanager(cliCtx, skipMnemonicConfirm) + + keymanager, err := wallet.InitializeKeymanager(cliCtx, false /* skip mnemonic confirm */) if err != nil { return errors.Wrap(err, "could not initialize keymanager") } @@ -36,7 +36,7 @@ func DeleteAccount(cliCtx *cli.Context) error { if len(validatingPublicKeys) == 0 { return errors.New("wallet is empty, no accounts to delete") } - // Allow the user to interactively select the accounts to backup or optionally + // Allow the user to interactively select the accounts to delete or optionally // provide them via cli flags as a string of comma-separated, hex strings. filteredPubKeys, err := filterPublicKeysFromUserInput( cliCtx, diff --git a/validator/accounts/v2/accounts_delete_test.go b/validator/accounts/v2/accounts_delete_test.go index 887f34f15c38..a60051d39602 100644 --- a/validator/accounts/v2/accounts_delete_test.go +++ b/validator/accounts/v2/accounts_delete_test.go @@ -5,7 +5,6 @@ import ( "crypto/rand" "encoding/hex" "fmt" - "io/ioutil" "math/big" "os" "path/filepath" @@ -14,7 +13,6 @@ import ( "time" "github.com/prysmaticlabs/prysm/shared/bytesutil" - "github.com/prysmaticlabs/prysm/shared/params" "github.com/prysmaticlabs/prysm/shared/testutil" "github.com/prysmaticlabs/prysm/shared/testutil/assert" "github.com/prysmaticlabs/prysm/shared/testutil/require" @@ -30,15 +28,7 @@ func TestDeleteAccounts_Noninteractive(t *testing.T) { keysDir := filepath.Join(testutil.TempDir(), fmt.Sprintf("/%d", randPath), "keysDir") require.NoError(t, os.MkdirAll(keysDir, os.ModePerm)) - // Write a directory where we will backup accounts to. - backupDir := filepath.Join(testutil.TempDir(), fmt.Sprintf("/%d", randPath), "backupDir") - require.NoError(t, os.MkdirAll(backupDir, os.ModePerm)) - t.Cleanup(func() { - require.NoError(t, os.RemoveAll(keysDir), "Failed to remove directory") - require.NoError(t, os.RemoveAll(backupDir), "Failed to remove directory") - }) - - // Create 2 keystore files in the keys directory we can then + // Create 3 keystore files in the keys directory we can then // import from in our wallet. k1, _ := createKeystore(t, keysDir) time.Sleep(time.Second) @@ -49,15 +39,6 @@ func TestDeleteAccounts_Noninteractive(t *testing.T) { // Only delete keys 0 and 1. deletePublicKeys := strings.Join(generatedPubKeys[0:2], ",") - // Write a password for the accounts we wish to backup to a file. - backupPasswordFile := filepath.Join(backupDir, "backuppass.txt") - err = ioutil.WriteFile( - backupPasswordFile, - []byte("Passw0rdz4938%%"), - params.BeaconIoConfig().ReadWritePermissions, - ) - require.NoError(t, err) - // We initialize a wallet with a direct keymanager. cliCtx := setupWalletCtx(t, &testWalletConfig{ // Wallet configuration flags. diff --git a/validator/accounts/v2/accounts_exit.go b/validator/accounts/v2/accounts_exit.go index 5091fd2216fc..0b9f1b7dff89 100644 --- a/validator/accounts/v2/accounts_exit.go +++ b/validator/accounts/v2/accounts_exit.go @@ -24,8 +24,8 @@ func ExitAccounts(cliCtx *cli.Context, r io.Reader) error { } else if err != nil { return errors.Wrap(err, "could not open wallet") } - skipMnemonicConfirm := cliCtx.Bool(flags.SkipMnemonicConfirmFlag.Name) - keymanager, err := wallet.InitializeKeymanager(cliCtx, skipMnemonicConfirm) + + keymanager, err := wallet.InitializeKeymanager(cliCtx, false /* skip mnemonic confirm */) if err != nil { return errors.Wrap(err, "could not initialize keymanager") } diff --git a/validator/accounts/v2/accounts_helper.go b/validator/accounts/v2/accounts_helper.go new file mode 100644 index 000000000000..3f13e57f83ac --- /dev/null +++ b/validator/accounts/v2/accounts_helper.go @@ -0,0 +1,46 @@ +package v2 + +import ( + "encoding/hex" + "fmt" + "strings" + + "github.com/pkg/errors" + "github.com/prysmaticlabs/prysm/shared/bls" + "github.com/urfave/cli/v2" +) + +func filterPublicKeysFromUserInput( + cliCtx *cli.Context, + publicKeysFlag *cli.StringFlag, + validatingPublicKeys [][48]byte, + selectionPrompt string, +) ([]bls.PublicKey, error) { + var filteredPubKeys []bls.PublicKey + if cliCtx.IsSet(publicKeysFlag.Name) { + pubKeyStrings := strings.Split(cliCtx.String(publicKeysFlag.Name), ",") + if len(pubKeyStrings) == 0 { + return nil, fmt.Errorf( + "could not parse %s. It must be a string of comma-separated hex strings", + publicKeysFlag.Name, + ) + } + for _, str := range pubKeyStrings { + pkString := str + if strings.Contains(pkString, "0x") { + pkString = pkString[2:] + } + pubKeyBytes, err := hex.DecodeString(pkString) + if err != nil { + return nil, errors.Wrapf(err, "could not decode string %s as hex", pkString) + } + blsPublicKey, err := bls.PublicKeyFromBytes(pubKeyBytes) + if err != nil { + return nil, errors.Wrapf(err, "%#x is not a valid BLS public key", pubKeyBytes) + } + filteredPubKeys = append(filteredPubKeys, blsPublicKey) + } + return filteredPubKeys, nil + } + return selectAccounts(selectionPrompt, validatingPublicKeys) +} diff --git a/validator/accounts/v2/wallet_test.go b/validator/accounts/v2/wallet_test.go index 331b4a4a7d6e..b42a9408d75d 100644 --- a/validator/accounts/v2/wallet_test.go +++ b/validator/accounts/v2/wallet_test.go @@ -71,7 +71,6 @@ func setupWalletCtx( set.String(flags.BackupPublicKeysFlag.Name, cfg.backupPublicKeys, "") set.String(flags.WalletPasswordFileFlag.Name, cfg.walletPasswordFile, "") set.String(flags.AccountPasswordFileFlag.Name, cfg.accountPasswordFile, "") - set.Bool(flags.SkipMnemonicConfirmFlag.Name, true, "") set.Int64(flags.NumAccountsFlag.Name, cfg.numAccounts, "") if cfg.privateKeyFile != "" { set.String(flags.ImportPrivateKeyFileFlag.Name, cfg.privateKeyFile, "") @@ -88,7 +87,6 @@ func setupWalletCtx( assert.NoError(tb, set.Set(flags.BackupPasswordFile.Name, cfg.backupPasswordFile)) assert.NoError(tb, set.Set(flags.WalletPasswordFileFlag.Name, cfg.walletPasswordFile)) assert.NoError(tb, set.Set(flags.AccountPasswordFileFlag.Name, cfg.accountPasswordFile)) - assert.NoError(tb, set.Set(flags.SkipMnemonicConfirmFlag.Name, "true")) assert.NoError(tb, set.Set(flags.NumAccountsFlag.Name, strconv.Itoa(int(cfg.numAccounts)))) return cli.NewContext(&app, set, nil) } diff --git a/validator/flags/flags.go b/validator/flags/flags.go index e166dd62da6f..582f567cfb60 100644 --- a/validator/flags/flags.go +++ b/validator/flags/flags.go @@ -157,10 +157,10 @@ var ( Usage: "Path to a wallet directory on-disk for Prysm validator accounts", Value: filepath.Join(DefaultValidatorDir(), WalletDefaultDirName), } - // AccountPasswordFileFlag is path to a file containing a password for a new validator account. + // AccountPasswordFileFlag is path to a file containing a password for a validator account. AccountPasswordFileFlag = &cli.StringFlag{ Name: "account-password-file", - Usage: "Path to a plain-text, .txt file containing a password for a new validator account", + Usage: "Path to a plain-text, .txt file containing a password for a validator account", } // WalletPasswordFileFlag is the path to a file containing your wallet password. WalletPasswordFileFlag = &cli.StringFlag{ @@ -177,11 +177,6 @@ var ( Name: "mnemonic-file", Usage: "File to retrieve mnemonic for non-interactively passing a mnemonic phrase into wallet recover.", } - // SkipMnemonicConfirmFlag is used to skip the withdrawal key mnemonic phrase prompt confirmation. - SkipMnemonicConfirmFlag = &cli.BoolFlag{ - Name: "skip-mnemonic-confirm", - Usage: "Skip the withdrawal key mnemonic phrase prompt confirmation", - } // ShowDepositDataFlag for accounts-v2. ShowDepositDataFlag = &cli.BoolFlag{ Name: "show-deposit-data",