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
prysmaticlabs · Aug 20, 2020 · 89e279f · 89e279f
1 parent 40db6cb
commit 89e279f
Show file tree

Hide file tree

Showing 10 changed files with 58 additions and 73 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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",