Accounts Revamp Fixes: "Overall" Wallet Improvements (#6736)

* change default wallet dir path to not be hidden

* gaz + pass wallet dir

* gaz + move const to flags

* move to flags

* move to flags

* use filepath join in order to create a valid dir name

* add wallet dir

* return err no wallet found issues

* fix up edit remote

* all tests passing

* fix test

* create or open wallet

* ivan feedback

* enter password for account with pubkey

* Update validator/accounts/v2/accounts_create.go

Co-authored-by: Ivan Martinez <ivanthegreatdev@gmail.com>

* works

* preston feedback

* nothing to export

* fmt

* test for create or open

* gaz

Co-authored-by: shayzluf <thezluf@gmail.com>
Co-authored-by: prylabs-bulldozer[bot] <58059840+prylabs-bulldozer[bot]@users.noreply.github.com>
Co-authored-by: Ivan Martinez <ivanthegreatdev@gmail.com>

beacon-chain/forkchoice/protoarray/nodes_test.go

@@ -374,10 +374,10 @@ func TestStore_HasParent(t *testing.T) {
 		want bool
 	}{
 		{r: [32]byte{'a'}, want: false},
-		{m: map[[32]byte]uint64{[32]byte{'a'}: 0}, r: [32]byte{'a'}, want: false},
-		{m: map[[32]byte]uint64{[32]byte{'a'}: 0}, r: [32]byte{'a'},
+		{m: map[[32]byte]uint64{{'a'}: 0}, r: [32]byte{'a'}, want: false},
+		{m: map[[32]byte]uint64{{'a'}: 0}, r: [32]byte{'a'},
 			n: []*Node{{Parent: NonExistentNode}}, want: false},
-		{m: map[[32]byte]uint64{[32]byte{'a'}: 0},
+		{m: map[[32]byte]uint64{{'a'}: 0},
 			n: []*Node{{Parent: 0}}, r: [32]byte{'a'},
 			want: true},
 	}

validator/accounts/v2/BUILD.bazel

@@ -23,6 +23,7 @@ go_library(
         "//validator:__subpackages__",
     ],
     deps = [
+        "//shared/bytesutil:go_default_library",
         "//shared/featureconfig:go_default_library",
         "//shared/params:go_default_library",
         "//shared/petnames:go_default_library",
@@ -73,7 +74,9 @@ go_test(
         "//validator/keymanager/v2/remote:go_default_library",
         "@com_github_dustin_go_humanize//:go_default_library",
         "@com_github_google_uuid//:go_default_library",
+        "@com_github_pkg_errors//:go_default_library",
         "@com_github_sirupsen_logrus//:go_default_library",
+        "@com_github_sirupsen_logrus//hooks/test:go_default_library",
         "@com_github_urfave_cli_v2//:go_default_library",
         "@com_github_wealdtech_go_eth2_wallet_encryptor_keystorev4//:go_default_library",
     ],

validator/accounts/v2/accounts_create.go

@@ -20,25 +20,10 @@ var log = logrus.WithField("prefix", "accounts-v2")
 // a wallet from the user's specified path.
 func CreateAccount(cliCtx *cli.Context) error {
 	ctx := context.Background()
-	walletDir, err := inputDirectory(cliCtx, walletDirPromptText, flags.WalletDirFlag)
-	if err != nil {
-		return errors.Wrapf(err, "Could not retrieve input directory")
-	}
-	ok, err := hasDir(walletDir)
+	wallet, err := createOrOpenWallet(cliCtx, CreateWallet)
 	if err != nil {
 		return err
 	}
-	// Create a new wallet if no directory exists.
-	if !ok {
-		err = CreateWallet(cliCtx)
-		if err != nil {
-			return errors.Wrapf(err, "Could not create wallet")
-		}
-	}
-	wallet, err := OpenWallet(cliCtx)
-	if err != nil {
-		return errors.Wrap(err, "could not open wallet")
-	}
 	skipMnemonicConfirm := cliCtx.Bool(flags.SkipMnemonicConfirmFlag.Name)
 	keymanager, err := wallet.InitializeKeymanager(ctx, skipMnemonicConfirm)
 	if err != nil {
@@ -52,7 +37,7 @@ func CreateAccount(cliCtx *cli.Context) error {
 		if !ok {
 			return errors.New("not a direct keymanager")
 		}
-		password, err := inputPassword(cliCtx, newAccountPasswordPromptText, confirmPass)
+		password, err := inputPassword(cliCtx, flags.AccountPasswordFileFlag, newAccountPasswordPromptText, confirmPass)
 		if err != nil {
 			return errors.Wrap(err, "could not input new account password")
 		}

validator/accounts/v2/accounts_create_test.go

@@ -14,15 +14,17 @@ func TestCreateAccount_Derived(t *testing.T) {
 	walletDir, passwordsDir, passwordFile := setupWalletAndPasswordsDir(t)
 	numAccounts := int64(5)
 	cliCtx := setupWalletCtx(t, &testWalletConfig{
-		walletDir:      walletDir,
-		passwordsDir:   passwordsDir,
-		passwordFile:   passwordFile,
-		keymanagerKind: v2keymanager.Derived,
-		numAccounts:    numAccounts,
+		walletDir:           walletDir,
+		passwordsDir:        passwordsDir,
+		walletPasswordFile:  passwordFile,
+		accountPasswordFile: passwordFile,
+		keymanagerKind:      v2keymanager.Derived,
+		numAccounts:         numAccounts,
 	})
 
 	// We attempt to create the wallet.
-	require.NoError(t, CreateWallet(cliCtx))
+	_, err := CreateWallet(cliCtx)
+	require.NoError(t, err)
 
 	// We attempt to open the newly created wallet.
 	ctx := context.Background()

validator/accounts/v2/accounts_export.go

@@ -28,9 +28,10 @@ func ExportAccount(cliCtx *cli.Context) error {
 	if err != nil {
 		return errors.Wrap(err, "could not parse output directory")
 	}
-
 	wallet, err := OpenWallet(cliCtx)
-	if err != nil {
+	if errors.Is(err, ErrNoWalletFound) {
+		return errors.Wrap(err, "nothing to export, no wallet found")
+	} else if err != nil {
 		return errors.Wrap(err, "could not open wallet")
 	}
 	keymanager, err := wallet.InitializeKeymanager(context.Background(), true /* skip mnemonic confirm */)

validator/accounts/v2/accounts_export_test.go

@@ -36,10 +36,12 @@ func TestZipAndUnzip(t *testing.T) {
 	require.NoError(t, err)
 	require.NoError(t, wallet.SaveWallet())
 	ctx := context.Background()
+	keymanagerCfg := direct.DefaultConfig()
+	keymanagerCfg.AccountPasswordsDirectory = passwordsDir
 	keymanager, err := direct.NewKeymanager(
 		ctx,
 		wallet,
-		direct.DefaultConfig(),
+		keymanagerCfg,
 	)
 	require.NoError(t, err)
 	_, err = keymanager.CreateAccount(ctx, password)
@@ -80,6 +82,7 @@ func TestExport_Noninteractive(t *testing.T) {
 	require.NoError(t, wallet.SaveWallet())
 	ctx := context.Background()
 	keymanagerCfg := direct.DefaultConfig()
+	keymanagerCfg.AccountPasswordsDirectory = passwordsDir
 	encodedCfg, err := direct.MarshalConfigFile(ctx, keymanagerCfg)
 	require.NoError(t, err)
 	require.NoError(t, wallet.WriteKeymanagerConfigToDisk(ctx, encodedCfg))

validator/accounts/v2/accounts_import.go

@@ -23,53 +23,41 @@ import (
 // ImportAccount uses the archived account made from ExportAccount to import an account and
 // asks the users for account passwords.
 func ImportAccount(cliCtx *cli.Context) error {
-	walletDir, err := inputDirectory(cliCtx, walletDirPromptText, flags.WalletDirFlag)
-	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")
+	ctx := context.Background()
+	wallet, err := createOrOpenWallet(cliCtx, func(cliCtx *cli.Context) (*Wallet, error) {
+		w, err := NewWallet(cliCtx, v2keymanager.Direct)
+		if err != nil && !errors.Is(err, ErrWalletExists) {
+			return nil, errors.Wrap(err, "could not create new 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(),
-			)
+		if err = createDirectKeymanagerWallet(cliCtx, w); err != nil {
+			return nil, errors.Wrap(err, "could not initialize wallet")
 		}
-	}
-	passwordsDir, err := inputDirectory(cliCtx, passwordsDirPromptText, flags.WalletPasswordsDirFlag)
+		log.WithField("wallet-path", w.walletDir).Info(
+			"Successfully created new wallet",
+		)
+		return w, err
+	})
 	if err != nil {
-		return err
+		return errors.Wrap(err, "could not initialize wallet")
+	}
+	if wallet.KeymanagerKind() != v2keymanager.Direct {
+		return errors.New(
+			"only non-HD wallets can import accounts, try creating a new wallet with wallet-v2 create",
+		)
 	}
 	keysDir, err := inputDirectory(cliCtx, importKeysDirPromptText, flags.KeysDirFlag)
 	if err != nil {
 		return errors.Wrap(err, "could not parse keys directory")
 	}
-
-	accountsPath := filepath.Join(walletDir, v2keymanager.Direct.String())
-	if err := os.MkdirAll(accountsPath, DirectoryPermissions); err != nil {
-		return errors.Wrap(err, "could not create wallet directory")
-	}
-	if err := os.MkdirAll(passwordsDir, DirectoryPermissions); err != nil {
-		return errors.Wrap(err, "could not create passwords directory")
-	}
-
-	wallet := &Wallet{
-		accountsPath:   accountsPath,
-		passwordsDir:   passwordsDir,
-		keymanagerKind: v2keymanager.Direct,
+	if err := wallet.SaveWallet(); err != nil {
+		return errors.Wrap(err, "could not save wallet")
 	}
-
 	var accountsImported []string
-	ctx := context.Background()
+	var pubKeysImported [][]byte
 	if err := filepath.Walk(keysDir, func(path string, info os.FileInfo, err error) error {
+		if err != nil {
+			return err
+		}
 		if info.IsDir() {
 			return nil
 		}
@@ -90,20 +78,21 @@ func ImportAccount(cliCtx *cli.Context) error {
 			return nil
 		}
 
-		accountName, err := wallet.importKeystore(ctx, path)
+		accountName, pubKey, err := wallet.importKeystore(ctx, path)
 		if err != nil {
 			return errors.Wrap(err, "could not import keystore")
 		}
 		accountsImported = append(accountsImported, accountName)
+		pubKeysImported = append(pubKeysImported, pubKey)
 		return nil
 	}); err != nil {
 		return errors.Wrap(err, "could not walk files")
 	}
 
 	au := aurora.NewAurora(true)
 	fmt.Printf("Importing accounts: %s\n", au.BrightGreen(strings.Join(accountsImported, ", ")).Bold())
-	for _, accountName := range accountsImported {
-		if err := wallet.enterPasswordForAccount(cliCtx, accountName); err != nil {
+	for i, accountName := range accountsImported {
+		if err := wallet.enterPasswordForAccount(cliCtx, accountName, pubKeysImported[i]); err != nil {
 			return errors.Wrap(err, "could not verify password for keystore")
 		}
 	}
@@ -123,25 +112,25 @@ func ImportAccount(cliCtx *cli.Context) error {
 	return nil
 }
 
-func (w *Wallet) importKeystore(ctx context.Context, keystoreFilePath string) (string, error) {
+func (w *Wallet) importKeystore(ctx context.Context, keystoreFilePath string) (string, []byte, error) {
 	keystoreBytes, err := ioutil.ReadFile(keystoreFilePath)
 	if err != nil {
-		return "", errors.Wrap(err, "could not read keystore file")
+		return "", nil, errors.Wrap(err, "could not read keystore file")
 	}
 	keystoreFile := &v2keymanager.Keystore{}
 	if err := json.Unmarshal(keystoreBytes, keystoreFile); err != nil {
-		return "", errors.Wrap(err, "could not decode keystore json")
+		return "", nil, errors.Wrap(err, "could not decode keystore json")
 	}
 	pubKeyBytes, err := hex.DecodeString(keystoreFile.Pubkey)
 	if err != nil {
-		return "", errors.Wrap(err, "could not decode public key string in keystore")
+		return "", nil, errors.Wrap(err, "could not decode public key string in keystore")
 	}
 	accountName := petnames.DeterministicName(pubKeyBytes, "-")
 	keystoreFileName := filepath.Base(keystoreFilePath)
 	if err := w.WriteFileAtPath(ctx, accountName, keystoreFileName, keystoreBytes); err != nil {
-		return "", errors.Wrap(err, "could not write keystore to account dir")
+		return "", nil, errors.Wrap(err, "could not write keystore to account dir")
 	}
-	return accountName, nil
+	return accountName, pubKeyBytes, nil
 }
 
 func logAccountsImported(ctx context.Context, wallet *Wallet, keymanager *direct.Keymanager, accountNames []string) error {

validator/accounts/v2/accounts_import_test.go

@@ -34,17 +34,19 @@ func TestImport_Noninteractive(t *testing.T) {
 	})
 
 	cliCtx := setupWalletCtx(t, &testWalletConfig{
-		walletDir:      walletDir,
-		passwordsDir:   passwordsDir,
-		keysDir:        keysDir,
-		keymanagerKind: v2keymanager.Direct,
-		passwordFile:   passwordFilePath,
+		walletDir:           walletDir,
+		passwordsDir:        passwordsDir,
+		keysDir:             keysDir,
+		keymanagerKind:      v2keymanager.Direct,
+		walletPasswordFile:  passwordFilePath,
+		accountPasswordFile: passwordFilePath,
 	})
 	wallet, err := NewWallet(cliCtx, v2keymanager.Direct)
 	require.NoError(t, err)
 	require.NoError(t, wallet.SaveWallet())
 	ctx := context.Background()
 	keymanagerCfg := direct.DefaultConfig()
+	keymanagerCfg.AccountPasswordsDirectory = passwordsDir
 	encodedCfg, err := direct.MarshalConfigFile(ctx, keymanagerCfg)
 	require.NoError(t, err)
 	require.NoError(t, wallet.WriteKeymanagerConfigToDisk(ctx, encodedCfg))

validator/accounts/v2/accounts_list.go

@@ -24,8 +24,10 @@ func ListAccounts(cliCtx *cli.Context) error {
 	// Read the wallet from the specified path.
 	ctx := context.Background()
 	wallet, err := OpenWallet(cliCtx)
-	if err != nil {
-		return errors.Wrapf(err, "could not read wallet at specified path %s", wallet.AccountsDir())
+	if errors.Is(err, ErrNoWalletFound) {
+		return errors.Wrap(err, "no wallet found at path, create a new wallet with wallet-v2 create")
+	} else if err != nil {
+		return errors.Wrap(err, "could not open wallet")
 	}
 	keymanager, err := wallet.InitializeKeymanager(ctx, true /* skip mnemonic confirm */)
 	if err != nil {
@@ -103,7 +105,7 @@ func listDirectKeymanagerAccounts(
 		if err != nil {
 			return errors.Wrap(err, "could not get timestamp from keystore file name")
 		}
-		fmt.Printf("%s | Created %s\n", au.BrightGreen(accountNames[i]).Bold(), humanize.Time(unixTimestamp))
+		fmt.Printf("%s | %s | Created %s\n", au.BrightBlue(fmt.Sprintf("Account %d", i)).Bold(), au.BrightGreen(accountNames[i]).Bold(), humanize.Time(unixTimestamp))
 		fmt.Printf("%s %#x\n", au.BrightMagenta("[validating public key]").Bold(), pubKeys[i])
 		if !showDepositData {
 			continue

validator/accounts/v2/accounts_list_test.go

@@ -127,10 +127,10 @@ func TestListAccounts_DirectKeymanager(t *testing.T) {
 func TestListAccounts_DerivedKeymanager(t *testing.T) {
 	walletDir, passwordsDir, passwordFilePath := setupWalletAndPasswordsDir(t)
 	cliCtx := setupWalletCtx(t, &testWalletConfig{
-		walletDir:      walletDir,
-		passwordsDir:   passwordsDir,
-		keymanagerKind: v2keymanager.Derived,
-		passwordFile:   passwordFilePath,
+		walletDir:          walletDir,
+		passwordsDir:       passwordsDir,
+		keymanagerKind:     v2keymanager.Derived,
+		walletPasswordFile: passwordFilePath,
 	})
 	wallet, err := NewWallet(cliCtx, v2keymanager.Derived)
 	require.NoError(t, err)

validator/accounts/v2/cmd_accounts.go

@@ -20,8 +20,8 @@ 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.WalletPasswordFileFlag,
+				flags.AccountPasswordFileFlag,
 				flags.NumAccountsFlag,
 				featureconfig.AltonaTestnet,
 				featureconfig.MedallaTestnet,
@@ -38,8 +38,7 @@ this command outputs a deposit data string which is required to become a validat
 			Description: "Lists all validator accounts in a user's wallet directory",
 			Flags: []cli.Flag{
 				flags.WalletDirFlag,
-				flags.WalletPasswordsDirFlag,
-				flags.PasswordFileFlag,
+				flags.WalletPasswordFileFlag,
 				flags.ShowDepositDataFlag,
 				featureconfig.AltonaTestnet,
 				featureconfig.MedallaTestnet,
@@ -56,7 +55,6 @@ this command outputs a deposit data string which is required to become a validat
 			Description: `exports the account of a given directory into a zip of the provided output path. This zip can be used to later import the account to another directory`,
 			Flags: []cli.Flag{
 				flags.WalletDirFlag,
-				flags.WalletPasswordsDirFlag,
 				flags.BackupDirFlag,
 				flags.AccountsFlag,
 				featureconfig.AltonaTestnet,
@@ -76,7 +74,7 @@ this command outputs a deposit data string which is required to become a validat
 				flags.WalletDirFlag,
 				flags.WalletPasswordsDirFlag,
 				flags.KeysDirFlag,
-				flags.PasswordFileFlag,
+				flags.WalletPasswordFileFlag,
 				featureconfig.AltonaTestnet,
 				featureconfig.MedallaTestnet,
 			},

validator/accounts/v2/cmd_wallet.go

@@ -24,12 +24,12 @@ var WalletCommands = &cli.Command{
 				flags.RemoteSignerCertPathFlag,
 				flags.RemoteSignerKeyPathFlag,
 				flags.RemoteSignerCACertPathFlag,
-				flags.PasswordFileFlag,
+				flags.WalletPasswordFileFlag,
 				featureconfig.AltonaTestnet,
 				featureconfig.MedallaTestnet,
 			},
 			Action: func(cliCtx *cli.Context) error {
-				if err := CreateWallet(cliCtx); err != nil {
+				if _, err := CreateWallet(cliCtx); err != nil {
 					log.Fatalf("Could not create a wallet: %v", err)
 				}
 				return nil
@@ -44,6 +44,7 @@ var WalletCommands = &cli.Command{
 				flags.RemoteSignerCertPathFlag,
 				flags.RemoteSignerKeyPathFlag,
 				flags.RemoteSignerCACertPathFlag,
+				flags.WalletPasswordsDirFlag,
 				featureconfig.AltonaTestnet,
 				featureconfig.MedallaTestnet,
 			},
@@ -61,7 +62,7 @@ var WalletCommands = &cli.Command{
 				flags.WalletDirFlag,
 				flags.WalletPasswordsDirFlag,
 				flags.MnemonicFileFlag,
-				flags.PasswordFileFlag,
+				flags.WalletPasswordFileFlag,
 				flags.NumAccountsFlag,
 				featureconfig.AltonaTestnet,
 				featureconfig.MedallaTestnet,