Accounts V2: Fix issues reported by rkapka (#6725)

* Fix accounts v2 runtime * Accounts V2: Fix issues found by rkapka * Add to help * Merge refs/heads/master into fix-v2-runtime * Merge refs/heads/master into fix-v2-runtime * Merge refs/heads/master into fix-v2-runtime * Merge refs/heads/master into fix-v2-runtime * Fixes * add back error * Make new wallet error cleaner * Fix text * Fix log * Add case for normal error * Update validator/flags/flags.go Co-authored-by: Raul Jordan <raul@prysmaticlabs.com> * Merge branch 'master' of github.com:prysmaticlabs/prysm into fix-v2-runtime * Merge refs/heads/master into fix-v2-runtime * Fix comment * Merge refs/heads/master into fix-v2-runtime * fix comment * Fix test * Merge branch 'master' into fix-v2-runtime * Merge refs/heads/master into fix-v2-runtime * Merge refs/heads/master into fix-v2-runtime * Merge refs/heads/master into fix-v2-runtime * Merge refs/heads/master into fix-v2-runtime
prysmaticlabs · Jul 28, 2020 · ee1addd · ee1addd
1 parent 06ee569
commit ee1addd
Show file tree

Hide file tree

Showing 13 changed files with 47 additions and 26 deletions.
diff --git a/validator/accounts/v2/accounts_import.go b/validator/accounts/v2/accounts_import.go
@@ -25,6 +25,23 @@ func ImportAccount(cliCtx *cli.Context) error {
 	if err != nil && !errors.Is(err, ErrNoWalletFound) {
 		return errors.Wrap(err, "could not parse wallet directory")
 	}
+	// Check if the user has a wallet at the specified path. If so, only let them continue if it is a non-HD wallet.
+	walletExists, err := hasDir(walletDir)
+	if err != nil {
+		return errors.Wrap(err, "could not check if wallet exists")
+	}
+	if walletExists {
+		keymanagerKind, err := readKeymanagerKindFromWalletPath(walletDir)
+		if err != nil {
+			return errors.Wrap(err, "could not read keymanager kind for existing wallet")
+		}
+		if keymanagerKind != v2keymanager.Direct {
+			return fmt.Errorf(
+				"importing non-HD accounts into a non-direct wallet is not allowed, given wallet path contains a %s wallet",
+				keymanagerKind.String(),
+			)
+		}
+	}
 	passwordsDir, err := inputDirectory(cliCtx, passwordsDirPromptText, flags.WalletPasswordsDirFlag)
 	if err != nil {
 		return err

diff --git a/validator/accounts/v2/accounts_list.go b/validator/accounts/v2/accounts_list.go
@@ -93,8 +93,6 @@ func listDirectKeymanagerAccounts(
 	}
 	for i := 0; i < len(accountNames); i++ {
 		fmt.Println("")
-		fmt.Printf("%s\n", au.BrightGreen(accountNames[i]).Bold())
-		fmt.Printf("%s %#x\n", au.BrightMagenta("[public key]").Bold(), pubKeys[i])
 
 		// Retrieve the account creation timestamp.
 		keystoreFileName, err := wallet.FileNameAtPath(ctx, accountNames[i], direct.KeystoreFileName)
@@ -105,7 +103,8 @@ func listDirectKeymanagerAccounts(
 		if err != nil {
 			return errors.Wrap(err, "could not get timestamp from keystore file name")
 		}
-		fmt.Printf("%s %s\n", au.BrightCyan("[created at]").Bold(), humanize.Time(unixTimestamp))
+		fmt.Printf("%s | %s\n", au.BrightGreen(accountNames[i]).Bold(), humanize.Time(unixTimestamp))
+		fmt.Printf("%s %#x\n", au.BrightMagenta("[validating public key]").Bold(), pubKeys[i])
 		if !showDepositData {
 			continue
 		}

diff --git a/validator/accounts/v2/cmd_accounts.go b/validator/accounts/v2/cmd_accounts.go
@@ -15,14 +15,13 @@ var AccountCommands = &cli.Command{
 		// AccountCommands for accounts-v2 for Prysm validators.
 		{
 			Name: "create",
-			Description: `creates a new validator account for eth2. If no account exists at the wallet path, creates a new wallet for a user based on
+			Description: `creates a new validator account for eth2. If no wallet exists at the given wallet path, creates a new wallet for a user based on
 specified input, capable of creating a direct, derived, or remote wallet.
 this command outputs a deposit data string which is required to become a validator in eth2.`,
 			Flags: []cli.Flag{
 				flags.WalletDirFlag,
 				flags.WalletPasswordsDirFlag,
 				flags.PasswordFileFlag,
-				flags.SkipMnemonicConfirmFlag,
 				flags.NumAccountsFlag,
 				featureconfig.AltonaTestnet,
 				featureconfig.MedallaTestnet,

diff --git a/validator/accounts/v2/prompt.go b/validator/accounts/v2/prompt.go
@@ -111,11 +111,11 @@ func inputPassword(cliCtx *cli.Context, promptText string, confirmPassword passw
 		if err != nil {
 			return "", errors.Wrap(err, "could not read password file")
 		}
-		enteredPassword := string(data)
+		enteredPassword := strings.TrimRight(string(data), "\r\n")
 		if err := validatePasswordInput(enteredPassword); err != nil {
 			return "", errors.Wrap(err, "password did not pass validation")
 		}
-		return strings.TrimRight(enteredPassword, "\r\n"), nil
+		return enteredPassword, nil
 	}
 	var hasValidPassword bool
 	var walletPassword string

diff --git a/validator/accounts/v2/wallet.go b/validator/accounts/v2/wallet.go
@@ -48,6 +48,13 @@ const (
 	DirectoryPermissions = os.ModePerm
 )
 
+var (
+	// ErrWalletExists is an error returned when a wallet already exists in the path provided.
+	ErrWalletExists = errors.New("you already have a wallet at the specified path. You can " +
+		"edit your wallet configuration by running ./prysm.sh validator wallet-v2 edit-config",
+	)
+)
+
 // Wallet is a primitive in Prysm's v2 account management which
 // has the capability of creating new accounts, reading existing accounts,
 // and providing secure access to eth2 secrets depending on an
@@ -81,10 +88,7 @@ func NewWallet(
 		return nil, errors.Wrap(err, "could not check if wallet exists")
 	}
 	if walletExists {
-		return nil, errors.New(
-			"you already have a wallet at the specified path. You can " +
-				"edit your wallet configuration by running ./prysm.sh validator wallet-v2 edit",
-		)
+		return nil, ErrWalletExists
 	}
 	accountsPath := filepath.Join(walletDir, keymanagerKind.String())
 	w := &Wallet{

diff --git a/validator/accounts/v2/wallet_create.go b/validator/accounts/v2/wallet_create.go
@@ -21,8 +21,11 @@ func CreateWallet(cliCtx *cli.Context) error {
 		return err
 	}
 	w, err := NewWallet(cliCtx, keymanagerKind)
-	if err != nil {
-		return errors.Wrap(err, "could not create a new wallet")
+	if err != nil && !errors.Is(err, ErrWalletExists) {
+		return errors.Wrap(err, "could not check if wallet directory exists")
+	}
+	if errors.Is(err, ErrWalletExists) {
+		return ErrWalletExists
 	}
 	switch w.KeymanagerKind() {
 	case v2keymanager.Direct:

diff --git a/validator/accounts/v2/wallet_recover.go b/validator/accounts/v2/wallet_recover.go
@@ -100,7 +100,7 @@ func inputMnemonic(cliCtx *cli.Context) (string, error) {
 		return enteredMnemonic, nil
 	}
 	prompt := promptui.Prompt{
-		Label:    "Enter the wallet recovery seed phrase you would like to recover",
+		Label:    "Enter the seed phrase for the wallet you would like to recover",
 		Validate: validateMnemonic,
 	}
 	menmonicPhrase, err := prompt.Run()

diff --git a/validator/flags/flags.go b/validator/flags/flags.go
@@ -136,7 +136,7 @@ var (
 	// PasswordFileFlag is used to enter a file to get a password for new account creation, non-interactively.
 	PasswordFileFlag = &cli.StringFlag{
 		Name:  "password-file",
-		Usage: "Path to a file containing a password to interact with wallets/accounts in a non-interactive way",
+		Usage: "Path to a plaintext password.txt file",
 	}
 	// MnemonicFileFlag is used to enter a file to mnemonic phrase for new wallet creation, non-interactively.
 	MnemonicFileFlag = &cli.StringFlag{

diff --git a/validator/keymanager/v2/derived/derived.go b/validator/keymanager/v2/derived/derived.go
@@ -9,6 +9,7 @@ import (
 	"io/ioutil"
 	"path"
 	"strconv"
+	"strings"
 	"sync"
 
 	"github.com/google/uuid"
@@ -208,7 +209,11 @@ func InitializeWalletSeedFile(ctx context.Context, password string, skipMnemonic
 // appropriate seed file for recovering a derived wallets.
 func SeedFileFromMnemonic(ctx context.Context, mnemonic string, password string) (*SeedConfig, error) {
 	walletSeed, err := bip39.EntropyFromMnemonic(mnemonic)
-	if err != nil {
+	if err != nil && strings.Contains(err.Error(), "not found in reverse map") {
+		return nil, errors.New("could not convert mnemonic to wallet seed: invalid seed word entered")
+	} else if errors.Is(err, bip39.ErrInvalidMnemonic) {
+		return nil, errors.Wrap(bip39.ErrInvalidMnemonic, "could not convert mnemonic to wallet seed")
+	} else if err != nil {
 		return nil, errors.Wrap(err, "could not convert mnemonic to wallet seed")
 	}
 	encryptor := keystorev4.New()

diff --git a/validator/keymanager/v2/derived/mnemonic.go b/validator/keymanager/v2/derived/mnemonic.go
@@ -2,6 +2,7 @@ package derived
 
 import (
 	"fmt"
+	"strings"
 
 	"github.com/manifoldco/promptui"
 	"github.com/tyler-smith/go-bip39"
@@ -55,7 +56,7 @@ func (m *EnglishMnemonicGenerator) ConfirmAcknowledgement(phrase string) error {
 	expected := "y"
 	var result string
 	var err error
-	for result != expected {
+	for strings.ToLower(result) != expected {
 		result, err = prompt.Run()
 		if err != nil {
 			log.Errorf("Could not confirm acknowledgement of prompt, please enter y")

diff --git a/validator/main.go b/validator/main.go
@@ -72,6 +72,7 @@ var appFlags = []cli.Flag{
 	flags.SlasherRPCProviderFlag,
 	flags.SlasherCertFlag,
 	flags.WalletPasswordsDirFlag,
+	flags.PasswordFileFlag,
 	flags.WalletDirFlag,
 	cmd.MinimalConfigFlag,
 	cmd.E2EConfigFlag,

diff --git a/validator/node/node.go b/validator/node/node.go
@@ -9,7 +9,6 @@ import (
 	"io/ioutil"
 	"os"
 	"os/signal"
-	"path"
 	"strings"
 	"sync"
 	"syscall"
@@ -83,14 +82,6 @@ func NewValidatorClient(cliCtx *cli.Context) (*ValidatorClient, error) {
 	var keyManagerV1 v1.KeyManager
 	var keyManagerV2 v2.IKeymanager
 	if featureconfig.Get().EnableAccountsV2 {
-		walletDir := cliCtx.String(flags.WalletDirFlag.Name)
-		if walletDir == flags.DefaultValidatorDir() {
-			walletDir = path.Join(walletDir, accountsv2.WalletDefaultDirName)
-		}
-		passwordsDir := cliCtx.String(flags.WalletPasswordsDirFlag.Name)
-		if passwordsDir == flags.DefaultValidatorDir() {
-			passwordsDir = path.Join(passwordsDir, accountsv2.PasswordsDefaultDirName)
-		}
 		// Read the wallet from the specified path.
 		wallet, err := accountsv2.OpenWallet(cliCtx)
 		if err != nil {

diff --git a/validator/usage.go b/validator/usage.go
@@ -83,6 +83,7 @@ var appHelpFlagGroups = []flagGroup{
 			flags.KeyManagerOpts,
 			flags.KeystorePathFlag,
 			flags.PasswordFlag,
+			flags.PasswordFileFlag,
 			flags.DisablePenaltyRewardLogFlag,
 			flags.UnencryptedKeysFlag,
 			flags.GraffitiFlag,