Introduce checks for existing wallets into two edge cases (#7349)

* Add check for preexisting wallet

* Reminder

* Create another check.  Adjust tests

* Wording in comment

* Remove unnecessary logic.  Add another regression test.

* Spacing

* Prepare to merge master

* Refactor wallet checks

* Fixed test

* Revert back to original location of check

* Fix test

* Fix test

* Address linter feedback

* Align CreateWallet() with recent changes

* Align RecoverWallet with recent changes

* Correct test error message

* Refactor test to minimize duplication

* rename to isValid and adjust function code

* refactor

* Improve IsValid()

* Fix tests

* Remove comment

* Fix documentation of IsValid()

* Align behavior of OpenWalletOrElseCli

* Fix tests

* Realigning error msg with prior behavior

* Fix another test

* Correct logic

* small change in logic

* Fix err messages

* Create consts for repeated strings

* Add comments to new constants

* gofmt

* Adjust WalletConfig behavior so it is closer to prior-to-PR.  Adjust test accordingly

* adjust error messages

Co-authored-by: dv8silencer <15720668+dv8silencer@users.noreply.github.com>

validator/accounts/v2/wallet/wallet.go

@@ -42,6 +42,12 @@ const (
 	// ConfirmPasswordPromptText for confirming a wallet password.
 	ConfirmPasswordPromptText = "Confirm password"
 	hashCost                  = 8
+	// CheckExistsErrMsg for when there is an error while checking for a wallet
+	CheckExistsErrMsg = "could not check if wallet exists"
+	// CheckValidityErrMsg for when there is an error while checking wallet validity
+	CheckValidityErrMsg = "could not check if wallet is valid"
+	// InvalidWalletErrMsg for when a directory does not contain a valid wallet
+	InvalidWalletErrMsg = "directory does not contain valid wallet"
 )
 
 var (
@@ -100,35 +106,73 @@ func New(cfg *Config) *Wallet {
 	}
 }
 
-// Exists check if a wallet at the specified directory
-// exists and has valid information in it.
-func Exists(walletDir string) error {
+// Exists checks if directory at walletDir exists
+func Exists(walletDir string) (bool, error) {
 	dirExists, err := fileutil.HasDir(walletDir)
 	if err != nil {
-		return errors.Wrap(err, "could not parse wallet directory")
+		return false, errors.Wrap(err, "could not parse wallet directory")
 	}
-	if dirExists {
-		isEmptyWallet, err := isEmptyWallet(walletDir)
-		if err != nil {
-			return errors.Wrap(err, "could not check if wallet has files")
-		}
-		if isEmptyWallet {
-			return ErrNoWalletFound
-		}
-		return nil
+	return dirExists, nil
+}
+
+// IsValid checks if a folder contains a single key directory such as `derived`, `remote` or `direct`.
+// Returns true if one of those subdirectories exist, false otherwise.
+func IsValid(walletDir string) (bool, error) {
+	expanded, err := fileutil.ExpandPath(walletDir)
+	if err != nil {
+		return false, err
 	}
-	return ErrNoWalletFound
+	f, err := os.Open(expanded)
+	if err != nil {
+		return false, err
+	}
+	defer func() {
+		if err := f.Close(); err != nil {
+			log.Debugf("Could not close directory: %s", expanded)
+		}
+	}()
+	names, err := f.Readdirnames(-1)
+	if err != nil {
+		return false, err
+	}
+
+	if len(names) == 0 {
+		return false, ErrNoWalletFound
+	}
+
+	// Count how many wallet types we have in the directory
+	numWalletTypes := 0
+	for _, name := range names {
+		// Nil error means input name is `derived`, `remote` or `direct`
+		_, err = v2keymanager.ParseKind(name)
+		if err == nil {
+			numWalletTypes++
+		}
+	}
+	return numWalletTypes == 1, nil
 }
 
 // OpenWalletOrElseCli tries to open the wallet and if it fails or no wallet
 // is found, invokes a callback function.
 func OpenWalletOrElseCli(cliCtx *cli.Context, otherwise func(cliCtx *cli.Context) (*Wallet, error)) (*Wallet, error) {
-	if err := Exists(cliCtx.String(flags.WalletDirFlag.Name)); err != nil {
-		if errors.Is(err, ErrNoWalletFound) {
-			return otherwise(cliCtx)
-		}
-		return nil, errors.Wrap(err, "could not check if wallet exists")
+	exists, err := Exists(cliCtx.String(flags.WalletDirFlag.Name))
+	if err != nil {
+		return nil, errors.Wrap(err, CheckExistsErrMsg)
 	}
+	if !exists {
+		return otherwise(cliCtx)
+	}
+	isValid, err := IsValid(cliCtx.String(flags.WalletDirFlag.Name))
+	if err == ErrNoWalletFound {
+		return otherwise(cliCtx)
+	}
+	if err != nil {
+		return nil, errors.Wrap(err, CheckValidityErrMsg)
+	}
+	if !isValid {
+		return nil, errors.New(InvalidWalletErrMsg)
+	}
+
 	walletDir, err := prompt.InputDirectory(cliCtx, prompt.WalletDirPromptText, flags.WalletDirFlag)
 	if err != nil {
 		return nil, err
@@ -160,12 +204,25 @@ func OpenWalletOrElseCli(cliCtx *cli.Context, otherwise func(cliCtx *cli.Context
 // type of keymanager associated with the wallet by reading files in the wallet
 // path, if applicable. If a wallet does not exist, returns an appropriate error.
 func OpenWallet(ctx context.Context, cfg *Config) (*Wallet, error) {
-	if err := Exists(cfg.WalletDir); err != nil {
-		if errors.Is(err, ErrNoWalletFound) {
-			return nil, ErrNoWalletFound
-		}
-		return nil, errors.Wrap(err, "could not check if wallet exists")
+	exists, err := Exists(cfg.WalletDir)
+	if err != nil {
+		return nil, errors.Wrap(err, CheckExistsErrMsg)
 	}
+	if !exists {
+		return nil, ErrNoWalletFound
+	}
+	valid, err := IsValid(cfg.WalletDir)
+	// ErrNoWalletFound represents both a directory that does not exist as well as an empty directory
+	if err == ErrNoWalletFound {
+		return nil, ErrNoWalletFound
+	}
+	if err != nil {
+		return nil, errors.Wrap(err, CheckValidityErrMsg)
+	}
+	if !valid {
+		return nil, errors.New(InvalidWalletErrMsg)
+	}
+
 	keymanagerKind, err := readKeymanagerKindFromWalletPath(cfg.WalletDir)
 	if err != nil {
 		return nil, errors.Wrap(err, "could not read keymanager kind for wallet")
@@ -445,38 +502,6 @@ func readKeymanagerKindFromWalletPath(walletPath string) (v2keymanager.Kind, err
 	return 0, errors.New("no keymanager folder, 'direct', 'remote', nor 'derived' found in wallet path")
 }
 
-// isEmptyWallet checks if a folder consists key directory such as `derived`, `remote` or `direct`.
-// Returns true if exists, false otherwise.
-func isEmptyWallet(name string) (bool, error) {
-	expanded, err := fileutil.ExpandPath(name)
-	if err != nil {
-		return false, err
-	}
-	f, err := os.Open(expanded)
-	if err != nil {
-		return false, err
-	}
-	defer func() {
-		if err := f.Close(); err != nil {
-			log.Debugf("Could not close directory: %s", expanded)
-		}
-	}()
-	names, err := f.Readdirnames(-1)
-	if err == io.EOF {
-		return true, nil
-	}
-
-	for _, n := range names {
-		// Nil error means input name is `derived`, `remote` or `direct`, the wallet is not empty.
-		_, err := v2keymanager.ParseKind(n)
-		if err == nil {
-			return false, nil
-		}
-	}
-
-	return true, err
-}
-
 func inputPassword(
 	cliCtx *cli.Context,
 	passwordFileFlag *cli.StringFlag,

validator/accounts/v2/wallet/wallet_test.go

@@ -110,18 +110,45 @@ func setupWalletAndPasswordsDir(t testing.TB) (string, string, string) {
 	return walletDir, passwordsDir, passwordFilePath
 }
 
-func Test_IsEmptyWallet_RandomFiles(t *testing.T) {
-	path := testutil.TempDir()
-	walletDir := filepath.Join(path, "test")
-	require.NoError(t, os.MkdirAll(walletDir, params.BeaconIoConfig().ReadWriteExecutePermissions), "Failed to remove directory")
-	err := wallet.Exists(path)
-	require.ErrorContains(t, "no wallet found at path", err)
+func Test_Exists_RandomFiles(t *testing.T) {
+	randPath, err := rand.Int(rand.Reader, big.NewInt(1000000))
+	require.NoError(t, err, "Could not generate random file path")
+	path := filepath.Join(testutil.TempDir(), fmt.Sprintf("/%d", randPath), "wallet")
 
-	walletDir = filepath.Join(path, "direct")
-	require.NoError(t, os.MkdirAll(walletDir, params.BeaconIoConfig().ReadWriteExecutePermissions), "Failed to remove directory")
-	err = wallet.Exists(path)
+	exists, err := wallet.Exists(path)
+	require.Equal(t, false, exists)
 	require.NoError(t, err)
-	require.NoError(t, os.RemoveAll(walletDir), "Failed to remove directory")
+
+	require.NoError(t, os.MkdirAll(path, params.BeaconIoConfig().ReadWriteExecutePermissions), "Failed to create directory")
+	t.Cleanup(func() {
+		require.NoError(t, os.RemoveAll(path), "Failed to remove directory")
+	})
+
+	exists, err = wallet.Exists(path)
+	require.NoError(t, err)
+	require.Equal(t, true, exists)
+}
+
+func Test_IsValid_RandomFiles(t *testing.T) {
+	randPath, err := rand.Int(rand.Reader, big.NewInt(1000000))
+	require.NoError(t, err, "Could not generate random file path")
+	path := filepath.Join(testutil.TempDir(), fmt.Sprintf("/%d", randPath), "wallet")
+
+	require.NoError(t, os.MkdirAll(path, params.BeaconIoConfig().ReadWriteExecutePermissions), "Failed to create directory")
+	t.Cleanup(func() {
+		require.NoError(t, os.RemoveAll(path), "Failed to remove directory")
+	})
+
+	valid, err := wallet.IsValid(path)
+	require.ErrorContains(t, "no wallet found at path", err)
+	require.Equal(t, false, valid)
+
+	walletDir := filepath.Join(path, "direct")
+	require.NoError(t, os.MkdirAll(walletDir, params.BeaconIoConfig().ReadWriteExecutePermissions), "Failed to create directory")
+
+	valid, err = wallet.IsValid(path)
+	require.NoError(t, err)
+	require.Equal(t, true, valid)
 }
 
 func Test_LockUnlockFile(t *testing.T) {

validator/accounts/v2/wallet_create.go

@@ -6,7 +6,6 @@ import (
 
 	"github.com/manifoldco/promptui"
 	"github.com/pkg/errors"
-	"github.com/prysmaticlabs/prysm/shared/fileutil"
 	"github.com/prysmaticlabs/prysm/shared/promptutil"
 	"github.com/prysmaticlabs/prysm/validator/accounts/v2/prompt"
 	"github.com/prysmaticlabs/prysm/validator/accounts/v2/wallet"
@@ -39,7 +38,7 @@ func CreateAndSaveWalletCli(cliCtx *cli.Context) (*wallet.Wallet, error) {
 	}
 
 	dir := createWalletConfig.WalletCfg.WalletDir
-	dirExists, err := fileutil.HasDir(dir)
+	dirExists, err := wallet.Exists(dir)
 	if err != nil {
 		return nil, err
 	}
@@ -47,6 +46,7 @@ func CreateAndSaveWalletCli(cliCtx *cli.Context) (*wallet.Wallet, error) {
 		return nil, errors.New("a wallet already exists at this location. Please input an" +
 			" alternative location for the new wallet or remove the current wallet")
 	}
+
 	w, err := CreateWalletWithKeymanager(cliCtx.Context, createWalletConfig)
 	if err != nil {
 		return nil, errors.Wrap(err, "could not create wallet with keymanager")
@@ -60,11 +60,6 @@ func CreateAndSaveWalletCli(cliCtx *cli.Context) (*wallet.Wallet, error) {
 
 // CreateWalletWithKeymanager specified by configuration options.
 func CreateWalletWithKeymanager(ctx context.Context, cfg *CreateWalletConfig) (*wallet.Wallet, error) {
-	if err := wallet.Exists(cfg.WalletCfg.WalletDir); err != nil {
-		if !errors.Is(err, wallet.ErrNoWalletFound) {
-			return nil, errors.Wrap(err, "could not check if wallet exists")
-		}
-	}
 	w := wallet.New(&wallet.Config{
 		WalletDir:      cfg.WalletCfg.WalletDir,
 		KeymanagerKind: cfg.WalletCfg.KeymanagerKind,

validator/accounts/v2/wallet_recover.go

@@ -53,11 +53,6 @@ func RecoverWalletCli(cliCtx *cli.Context) error {
 	if err != nil {
 		return err
 	}
-	if err := wallet.Exists(walletDir); err != nil {
-		if !errors.Is(err, wallet.ErrNoWalletFound) {
-			return errors.Wrap(err, "could not check if wallet exists")
-		}
-	}
 	numAccounts, err := inputNumAccounts(cliCtx)
 	if err != nil {
 		return errors.Wrap(err, "could not get number of accounts to recover")
@@ -84,6 +79,15 @@ func RecoverWalletCli(cliCtx *cli.Context) error {
 
 // RecoverWallet uses a menmonic seed phrase to recover a wallet into the path provided.
 func RecoverWallet(ctx context.Context, cfg *RecoverWalletConfig) (*wallet.Wallet, error) {
+	// Ensure that the wallet directory does not contain a wallet already
+	dirExists, err := wallet.Exists(cfg.WalletDir)
+	if err != nil {
+		return nil, err
+	}
+	if dirExists {
+		return nil, errors.New("a wallet already exists at this location. Please input an" +
+			" alternative location for the new wallet or remove the current wallet")
+	}
 	w := wallet.New(&wallet.Config{
 		WalletDir:      cfg.WalletDir,
 		KeymanagerKind: v2keymanager.Derived,

validator/accounts/v2/wallet_recover_test.go

@@ -19,56 +19,70 @@ import (
 	"github.com/urfave/cli/v2"
 )
 
-func TestRecoverDerivedWallet(t *testing.T) {
+type recoverCfgStruct struct {
+	walletDir        string
+	passwordFilePath string
+	mnemonicFilePath string
+	numAccounts      int64
+}
+
+func setupRecoverCfg(t *testing.T) *recoverCfgStruct {
 	testDir := testutil.TempDir()
 	walletDir := filepath.Join(testDir, walletDirName)
-	passwordsDir := filepath.Join(testDir, passwordDirName)
-	exportDir := filepath.Join(testDir, exportDirName)
-	defer func() {
-		assert.NoError(t, os.RemoveAll(walletDir))
-		assert.NoError(t, os.RemoveAll(passwordsDir))
-		assert.NoError(t, os.RemoveAll(exportDir))
-	}()
-
 	passwordFilePath := filepath.Join(testDir, passwordFileName)
 	require.NoError(t, ioutil.WriteFile(passwordFilePath, []byte(password), os.ModePerm))
 	mnemonicFilePath := filepath.Join(testDir, mnemonicFileName)
 	require.NoError(t, ioutil.WriteFile(mnemonicFilePath, []byte(mnemonic), os.ModePerm))
 
-	numAccounts := int64(4)
+	t.Cleanup(func() {
+		assert.NoError(t, os.RemoveAll(walletDir))
+		assert.NoError(t, os.Remove(passwordFilePath))
+		assert.NoError(t, os.Remove(mnemonicFilePath))
+	})
+
+	return &recoverCfgStruct{
+		walletDir:        walletDir,
+		passwordFilePath: passwordFilePath,
+		mnemonicFilePath: mnemonicFilePath,
+	}
+}
+
+func createRecoverCliCtx(t *testing.T, cfg *recoverCfgStruct) *cli.Context {
 	app := cli.App{}
 	set := flag.NewFlagSet("test", 0)
-	set.String(flags.WalletDirFlag.Name, walletDir, "")
-	set.String(flags.DeprecatedPasswordsDirFlag.Name, passwordsDir, "")
-	set.String(flags.WalletPasswordFileFlag.Name, passwordFilePath, "")
+	set.String(flags.WalletDirFlag.Name, cfg.walletDir, "")
+	set.String(flags.WalletPasswordFileFlag.Name, cfg.passwordFilePath, "")
 	set.String(flags.KeymanagerKindFlag.Name, v2keymanager.Derived.String(), "")
-	set.String(flags.MnemonicFileFlag.Name, mnemonicFilePath, "")
-	set.Int64(flags.NumAccountsFlag.Name, numAccounts, "")
-	assert.NoError(t, set.Set(flags.WalletDirFlag.Name, walletDir))
-	assert.NoError(t, set.Set(flags.DeprecatedPasswordsDirFlag.Name, passwordsDir))
-	assert.NoError(t, set.Set(flags.WalletPasswordFileFlag.Name, passwordFilePath))
+	set.String(flags.MnemonicFileFlag.Name, cfg.mnemonicFilePath, "")
+	set.Int64(flags.NumAccountsFlag.Name, cfg.numAccounts, "")
+	assert.NoError(t, set.Set(flags.WalletDirFlag.Name, cfg.walletDir))
+	assert.NoError(t, set.Set(flags.WalletPasswordFileFlag.Name, cfg.passwordFilePath))
 	assert.NoError(t, set.Set(flags.KeymanagerKindFlag.Name, v2keymanager.Derived.String()))
-	assert.NoError(t, set.Set(flags.MnemonicFileFlag.Name, mnemonicFilePath))
-	assert.NoError(t, set.Set(flags.NumAccountsFlag.Name, strconv.Itoa(int(numAccounts))))
-	cliCtx := cli.NewContext(&app, set, nil)
+	assert.NoError(t, set.Set(flags.MnemonicFileFlag.Name, cfg.mnemonicFilePath))
+	assert.NoError(t, set.Set(flags.NumAccountsFlag.Name, strconv.Itoa(int(cfg.numAccounts))))
+	return cli.NewContext(&app, set, nil)
+}
 
+func TestRecoverDerivedWallet(t *testing.T) {
+	cfg := setupRecoverCfg(t)
+	cfg.numAccounts = 4
+	cliCtx := createRecoverCliCtx(t, cfg)
 	require.NoError(t, RecoverWalletCli(cliCtx))
 
 	ctx := context.Background()
 	w, err := wallet.OpenWallet(cliCtx.Context, &wallet.Config{
-		WalletDir:      walletDir,
+		WalletDir:      cfg.walletDir,
 		WalletPassword: password,
 	})
 	assert.NoError(t, err)
 
 	encoded, err := w.ReadKeymanagerConfigFromDisk(ctx)
 	assert.NoError(t, err)
-	cfg, err := derived.UnmarshalOptionsFile(encoded)
+	walletCfg, err := derived.UnmarshalOptionsFile(encoded)
 	assert.NoError(t, err)
-
 	// We assert the created configuration was as desired.
 	wantCfg := derived.DefaultKeymanagerOpts()
-	assert.DeepEqual(t, wantCfg, cfg)
+	assert.DeepEqual(t, wantCfg, walletCfg)
 
 	keymanager, err := w.InitializeKeymanager(cliCtx.Context, true)
 	require.NoError(t, err)
@@ -78,43 +92,29 @@ func TestRecoverDerivedWallet(t *testing.T) {
 	}
 	names, err := km.ValidatingAccountNames(ctx)
 	assert.NoError(t, err)
-	require.Equal(t, len(names), int(numAccounts))
-
+	require.Equal(t, len(names), int(cfg.numAccounts))
 }
 
 // TestRecoverDerivedWallet_OneAccount is a test for regression in cases where the number of accounts recovered is 1
 func TestRecoverDerivedWallet_OneAccount(t *testing.T) {
-	testDir := testutil.TempDir()
-	walletDir := filepath.Join(testDir, walletDirName)
-	defer func() {
-		assert.NoError(t, os.RemoveAll(walletDir))
-	}()
-
-	passwordFilePath := filepath.Join(testDir, passwordFileName)
-	require.NoError(t, ioutil.WriteFile(passwordFilePath, []byte(password), os.ModePerm))
-	mnemonicFilePath := filepath.Join(testDir, mnemonicFileName)
-	require.NoError(t, ioutil.WriteFile(mnemonicFilePath, []byte(mnemonic), os.ModePerm))
-
-	numAccounts := int64(1)
-	app := cli.App{}
-	set := flag.NewFlagSet("test", 0)
-	set.String(flags.WalletDirFlag.Name, walletDir, "")
-	set.String(flags.WalletPasswordFileFlag.Name, passwordFilePath, "")
-	set.String(flags.KeymanagerKindFlag.Name, v2keymanager.Derived.String(), "")
-	set.String(flags.MnemonicFileFlag.Name, mnemonicFilePath, "")
-	set.Int64(flags.NumAccountsFlag.Name, numAccounts, "")
-	assert.NoError(t, set.Set(flags.WalletDirFlag.Name, walletDir))
-	assert.NoError(t, set.Set(flags.WalletPasswordFileFlag.Name, passwordFilePath))
-	assert.NoError(t, set.Set(flags.KeymanagerKindFlag.Name, v2keymanager.Derived.String()))
-	assert.NoError(t, set.Set(flags.MnemonicFileFlag.Name, mnemonicFilePath))
-	assert.NoError(t, set.Set(flags.NumAccountsFlag.Name, strconv.Itoa(int(numAccounts))))
-	cliCtx := cli.NewContext(&app, set, nil)
-
+	cfg := setupRecoverCfg(t)
+	cfg.numAccounts = 1
+	cliCtx := createRecoverCliCtx(t, cfg)
 	require.NoError(t, RecoverWalletCli(cliCtx))
 
 	_, err := wallet.OpenWallet(cliCtx.Context, &wallet.Config{
-		WalletDir:      walletDir,
+		WalletDir:      cfg.walletDir,
 		WalletPassword: password,
 	})
 	assert.NoError(t, err)
 }
+
+func TestRecoverDerivedWallet_AlreadyExists(t *testing.T) {
+	cfg := setupRecoverCfg(t)
+	cfg.numAccounts = 4
+	cliCtx := createRecoverCliCtx(t, cfg)
+	require.NoError(t, RecoverWalletCli(cliCtx))
+
+	// Trying to recover an HD wallet into a directory that already exists should give an error
+	require.ErrorContains(t, "a wallet already exists at this location", RecoverWalletCli(cliCtx))
+}