From 4cbe144a6cea520e7809d122f51ae6495b5792e4 Mon Sep 17 00:00:00 2001 From: james-prysm <90280386+james-prysm@users.noreply.github.com> Date: Wed, 13 Dec 2023 11:11:32 -0600 Subject: [PATCH] CLI: fixing account import ux bugs (#13328) * fixing account import checking wallet twice, and adding sub folder search with a depth of 2 * removing uneeded check * fixing unit test * adding reset cache to fix potential flake * improving test based on feedback --- cmd/validator/accounts/import.go | 83 +----------------- cmd/validator/accounts/wallet_utils_test.go | 5 +- cmd/validator/wallet/BUILD.bazel | 3 - cmd/validator/wallet/create_test.go | 52 ++++------- validator/accounts/BUILD.bazel | 1 - validator/accounts/accounts_import.go | 96 ++++++++++++--------- validator/accounts/wallet/BUILD.bazel | 3 + validator/accounts/wallet/wallet.go | 60 +++++++++++++ validator/accounts/wallet/wallet_test.go | 87 +++++++++++++++++++ validator/accounts/wallet_create.go | 27 +----- validator/keymanager/local/keymanager.go | 16 ++-- validator/node/node.go | 1 + 12 files changed, 240 insertions(+), 194 deletions(-) diff --git a/cmd/validator/accounts/import.go b/cmd/validator/accounts/import.go index 4fbcad175d..c4b966f538 100644 --- a/cmd/validator/accounts/import.go +++ b/cmd/validator/accounts/import.go @@ -6,13 +6,11 @@ import ( "github.com/pkg/errors" "github.com/prysmaticlabs/prysm/v4/cmd" "github.com/prysmaticlabs/prysm/v4/cmd/validator/flags" - "github.com/prysmaticlabs/prysm/v4/io/prompt" "github.com/prysmaticlabs/prysm/v4/validator/accounts" "github.com/prysmaticlabs/prysm/v4/validator/accounts/iface" "github.com/prysmaticlabs/prysm/v4/validator/accounts/userprompt" "github.com/prysmaticlabs/prysm/v4/validator/accounts/wallet" "github.com/prysmaticlabs/prysm/v4/validator/client" - "github.com/prysmaticlabs/prysm/v4/validator/keymanager" "github.com/urfave/cli/v2" ) @@ -62,84 +60,5 @@ func accountsImport(c *cli.Context) error { } func walletImport(c *cli.Context) (*wallet.Wallet, error) { - return wallet.OpenWalletOrElseCli(c, func(cliCtx *cli.Context) (*wallet.Wallet, error) { - walletDir, err := userprompt.InputDirectory(cliCtx, userprompt.WalletDirPromptText, flags.WalletDirFlag) - if err != nil { - return nil, err - } - exists, err := wallet.Exists(walletDir) - if err != nil { - return nil, errors.Wrap(err, wallet.CheckExistsErrMsg) - } - if exists { - isValid, err := wallet.IsValid(walletDir) - if err != nil { - return nil, errors.Wrap(err, wallet.CheckValidityErrMsg) - } - if !isValid { - return nil, errors.New(wallet.InvalidWalletErrMsg) - } - walletPassword, err := wallet.InputPassword( - cliCtx, - flags.WalletPasswordFileFlag, - wallet.PasswordPromptText, - false, /* Do not confirm password */ - wallet.ValidateExistingPass, - ) - if err != nil { - return nil, err - } - return wallet.OpenWallet(cliCtx.Context, &wallet.Config{ - WalletDir: walletDir, - WalletPassword: walletPassword, - }) - } - - wCfg, err := ExtractWalletDirPassword(cliCtx) - if err != nil { - return nil, err - } - w := wallet.New(&wallet.Config{ - KeymanagerKind: keymanager.Local, - WalletDir: wCfg.Dir, - WalletPassword: wCfg.Password, - }) - if err = accounts.CreateLocalKeymanagerWallet(cliCtx.Context, w); err != nil { - return nil, errors.Wrap(err, "could not create keymanager") - } - log.WithField("wallet-path", wCfg.Dir).Info( - "Successfully created new wallet", - ) - return w, nil - }) -} - -// WalletDirPassword holds the directory and password of a wallet. -type WalletDirPassword struct { - Dir string - Password string -} - -// ExtractWalletDirPassword prompts the user for wallet directory and password. -func ExtractWalletDirPassword(cliCtx *cli.Context) (WalletDirPassword, error) { - // Get wallet dir and check that no wallet exists at the location. - walletDir, err := userprompt.InputDirectory(cliCtx, userprompt.WalletDirPromptText, flags.WalletDirFlag) - if err != nil { - return WalletDirPassword{}, err - } - walletPassword, err := prompt.InputPassword( - cliCtx, - flags.WalletPasswordFileFlag, - wallet.NewWalletPasswordPromptText, - wallet.ConfirmPasswordPromptText, - true, /* Should confirm password */ - prompt.ValidatePasswordInput, - ) - if err != nil { - return WalletDirPassword{}, err - } - return WalletDirPassword{ - Dir: walletDir, - Password: walletPassword, - }, nil + return wallet.OpenWalletOrElseCli(c, wallet.OpenOrCreateNewWallet) } diff --git a/cmd/validator/accounts/wallet_utils_test.go b/cmd/validator/accounts/wallet_utils_test.go index c619370bb4..9f3ee1e84d 100644 --- a/cmd/validator/accounts/wallet_utils_test.go +++ b/cmd/validator/accounts/wallet_utils_test.go @@ -70,9 +70,10 @@ func TestWalletWithKeymanager(t *testing.T) { require.NoError(t, err) require.Equal(t, len(keys), 2) require.Equal(t, w.KeymanagerKind(), keymanager.Local) - hexKeys := []string{hexutil.Encode(keys[0][:])[2:], hexutil.Encode(keys[1][:])[2:]} // imported keystores don't include the 0x in name - assert.LogsContain(t, logHook, fmt.Sprintf("Imported accounts %v,", hexKeys)) + assert.LogsContain(t, logHook, fmt.Sprintf("Imported accounts")) + assert.LogsContain(t, logHook, hexutil.Encode(keys[0][:])[2:]) + assert.LogsContain(t, logHook, hexutil.Encode(keys[1][:])[2:]) } func TestWalletWithKeymanager_web3signer(t *testing.T) { diff --git a/cmd/validator/wallet/BUILD.bazel b/cmd/validator/wallet/BUILD.bazel index f7c059952a..db22ca9a3a 100644 --- a/cmd/validator/wallet/BUILD.bazel +++ b/cmd/validator/wallet/BUILD.bazel @@ -37,18 +37,15 @@ go_test( ], embed = [":go_default_library"], deps = [ - "//cmd/validator/accounts:go_default_library", "//cmd/validator/flags:go_default_library", "//config/params:go_default_library", "//testing/assert:go_default_library", "//testing/require:go_default_library", - "//validator/accounts:go_default_library", "//validator/accounts/iface:go_default_library", "//validator/accounts/wallet:go_default_library", "//validator/keymanager:go_default_library", "//validator/keymanager/derived:go_default_library", "//validator/keymanager/local: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", diff --git a/cmd/validator/wallet/create_test.go b/cmd/validator/wallet/create_test.go index ca57732d8b..faa4570f6a 100644 --- a/cmd/validator/wallet/create_test.go +++ b/cmd/validator/wallet/create_test.go @@ -8,13 +8,10 @@ import ( "strconv" "testing" - "github.com/pkg/errors" - cmdacc "github.com/prysmaticlabs/prysm/v4/cmd/validator/accounts" "github.com/prysmaticlabs/prysm/v4/cmd/validator/flags" "github.com/prysmaticlabs/prysm/v4/config/params" "github.com/prysmaticlabs/prysm/v4/testing/assert" "github.com/prysmaticlabs/prysm/v4/testing/require" - "github.com/prysmaticlabs/prysm/v4/validator/accounts" "github.com/prysmaticlabs/prysm/v4/validator/accounts/wallet" "github.com/prysmaticlabs/prysm/v4/validator/keymanager" "github.com/prysmaticlabs/prysm/v4/validator/keymanager/local" @@ -30,7 +27,7 @@ const ( // `cmd/validator/accounts/delete_test.go`. https://pastebin.com/2n2VB7Ez is // the error I couldn't get around. -func setupWalletAndPasswordsDir(t testing.TB) (string, string, string) { +func SetupWalletAndPasswordsDir(t testing.TB) (string, string, string) { walletDir := filepath.Join(t.TempDir(), "wallet") passwordsDir := filepath.Join(t.TempDir(), "passwords") passwordFileDir := filepath.Join(t.TempDir(), "passwordFile") @@ -40,7 +37,7 @@ func setupWalletAndPasswordsDir(t testing.TB) (string, string, string) { return walletDir, passwordsDir, passwordFilePath } -type testWalletConfig struct { +type TestWalletConfig struct { exitAll bool skipDepositConfirm bool keymanagerKind keymanager.Kind @@ -59,9 +56,9 @@ type testWalletConfig struct { passwordsDir string } -func setupWalletCtx( +func SetupWalletCtx( tb testing.TB, - cfg *testWalletConfig, + cfg *TestWalletConfig, ) *cli.Context { app := cli.App{} set := flag.NewFlagSet("test", 0) @@ -110,44 +107,27 @@ func init() { func TestCreateOrOpenWallet(t *testing.T) { hook := logTest.NewGlobal() - walletDir, passwordsDir, walletPasswordFile := setupWalletAndPasswordsDir(t) - cliCtx := setupWalletCtx(t, &testWalletConfig{ + walletDir, passwordsDir, walletPasswordFile := SetupWalletAndPasswordsDir(t) + cliCtx := SetupWalletCtx(t, &TestWalletConfig{ walletDir: walletDir, passwordsDir: passwordsDir, keymanagerKind: keymanager.Local, walletPasswordFile: walletPasswordFile, }) - createLocalWallet := func(cliCtx *cli.Context) (*wallet.Wallet, error) { - cfg, err := cmdacc.ExtractWalletDirPassword(cliCtx) - if err != nil { - return nil, err - } - w := wallet.New(&wallet.Config{ - KeymanagerKind: keymanager.Local, - WalletDir: cfg.Dir, - WalletPassword: cfg.Password, - }) - if err = accounts.CreateLocalKeymanagerWallet(cliCtx.Context, w); err != nil { - return nil, errors.Wrap(err, "could not create keymanager") - } - log.WithField("wallet-path", cfg.Dir).Info( - "Successfully created new wallet", - ) - return w, nil - } - createdWallet, err := wallet.OpenWalletOrElseCli(cliCtx, createLocalWallet) + + createdWallet, err := wallet.OpenWalletOrElseCli(cliCtx, wallet.OpenOrCreateNewWallet) require.NoError(t, err) require.LogsContain(t, hook, "Successfully created new wallet") - openedWallet, err := wallet.OpenWalletOrElseCli(cliCtx, createLocalWallet) + openedWallet, err := wallet.OpenWalletOrElseCli(cliCtx, wallet.OpenOrCreateNewWallet) require.NoError(t, err) assert.Equal(t, createdWallet.KeymanagerKind(), openedWallet.KeymanagerKind()) assert.Equal(t, createdWallet.AccountsDir(), openedWallet.AccountsDir()) } func TestCreateWallet_Local(t *testing.T) { - walletDir, passwordsDir, walletPasswordFile := setupWalletAndPasswordsDir(t) - cliCtx := setupWalletCtx(t, &testWalletConfig{ + walletDir, passwordsDir, walletPasswordFile := SetupWalletAndPasswordsDir(t) + cliCtx := SetupWalletCtx(t, &TestWalletConfig{ walletDir: walletDir, passwordsDir: passwordsDir, keymanagerKind: keymanager.Local, @@ -168,8 +148,8 @@ func TestCreateWallet_Local(t *testing.T) { } func TestCreateWallet_Derived(t *testing.T) { - walletDir, passwordsDir, passwordFile := setupWalletAndPasswordsDir(t) - cliCtx := setupWalletCtx(t, &testWalletConfig{ + walletDir, passwordsDir, passwordFile := SetupWalletAndPasswordsDir(t) + cliCtx := SetupWalletCtx(t, &TestWalletConfig{ walletDir: walletDir, passwordsDir: passwordsDir, walletPasswordFile: passwordFile, @@ -190,8 +170,8 @@ func TestCreateWallet_Derived(t *testing.T) { // TestCreateWallet_WalletAlreadyExists checks for expected error if trying to create a wallet when there is one already. func TestCreateWallet_WalletAlreadyExists(t *testing.T) { - walletDir, passwordsDir, passwordFile := setupWalletAndPasswordsDir(t) - cliCtx := setupWalletCtx(t, &testWalletConfig{ + walletDir, passwordsDir, passwordFile := SetupWalletAndPasswordsDir(t) + cliCtx := SetupWalletCtx(t, &TestWalletConfig{ walletDir: walletDir, passwordsDir: passwordsDir, walletPasswordFile: passwordFile, @@ -207,7 +187,7 @@ func TestCreateWallet_WalletAlreadyExists(t *testing.T) { _, err = CreateAndSaveWalletCli(cliCtx) require.ErrorContains(t, "already exists", err) - cliCtx = setupWalletCtx(t, &testWalletConfig{ + cliCtx = SetupWalletCtx(t, &TestWalletConfig{ walletDir: walletDir, passwordsDir: passwordsDir, walletPasswordFile: passwordFile, diff --git a/validator/accounts/BUILD.bazel b/validator/accounts/BUILD.bazel index c05c96af53..212b0a72f4 100644 --- a/validator/accounts/BUILD.bazel +++ b/validator/accounts/BUILD.bazel @@ -34,7 +34,6 @@ go_library( "//io/file:go_default_library", "//io/prompt:go_default_library", "//proto/prysm/v1alpha1:go_default_library", - "//validator/accounts/iface:go_default_library", "//validator/accounts/petnames:go_default_library", "//validator/accounts/userprompt:go_default_library", "//validator/accounts/wallet:go_default_library", diff --git a/validator/accounts/accounts_import.go b/validator/accounts/accounts_import.go index b7692db522..7f79cad5b3 100644 --- a/validator/accounts/accounts_import.go +++ b/validator/accounts/accounts_import.go @@ -8,7 +8,6 @@ import ( "os" "path/filepath" "regexp" - "sort" "strconv" "strings" @@ -81,52 +80,16 @@ func (acm *CLIManager) Import(ctx context.Context) error { if !ok { return errors.New("keymanager cannot import keystores") } - + log.Info("importing validator keystores...") // Check if the user wishes to import a one-off, private key directly // as an account into the Prysm validator. if acm.importPrivateKeys { return importPrivateKeyAsAccount(ctx, acm.wallet, k, acm.privateKeyFile) } - // Consider that the keysDir might be a path to a specific file and handle accordingly. - isDir, err := file.HasDir(acm.keysDir) + keystoresImported, err := processDirectory(ctx, acm.keysDir, 0) if err != nil { - return errors.Wrap(err, "could not determine if path is a directory") - } - keystoresImported := make([]*keymanager.Keystore, 0) - if isDir { - files, err := os.ReadDir(acm.keysDir) - if err != nil { - return errors.Wrap(err, "could not read dir") - } - if len(files) == 0 { - return fmt.Errorf("directory %s has no files, cannot import from it", acm.keysDir) - } - filesInDir := make([]string, 0) - for i := 0; i < len(files); i++ { - if files[i].IsDir() { - continue - } - filesInDir = append(filesInDir, files[i].Name()) - } - // Sort the imported keystores by derivation path if they - // specify this value in their filename. - sort.Sort(byDerivationPath(filesInDir)) - for _, name := range filesInDir { - keystore, err := readKeystoreFile(ctx, filepath.Join(acm.keysDir, name)) - if err != nil && strings.Contains(err.Error(), "could not decode keystore json") { - continue - } else if err != nil { - return errors.Wrapf(err, "could not import keystore at path: %s", name) - } - keystoresImported = append(keystoresImported, keystore) - } - } else { - keystore, err := readKeystoreFile(ctx, acm.keysDir) - if err != nil { - return errors.Wrap(err, "could not import keystore") - } - keystoresImported = append(keystoresImported, keystore) + return errors.Wrap(err, "unable to process directory and import keys") } var accountsPassword string @@ -176,6 +139,59 @@ func (acm *CLIManager) Import(ctx context.Context) error { return nil } +// Recursive function to process directories and files. +func processDirectory(ctx context.Context, dir string, depth int) ([]*keymanager.Keystore, error) { + maxdepth := 2 + if depth > maxdepth { + log.Infof("stopped checking folders for keystores after max depth of %d was reached", maxdepth) + return nil, nil // Stop recursion after two levels. + } + log.Infof("checking directory for keystores: %s", dir) + isDir, err := file.HasDir(dir) + if err != nil { + return nil, errors.Wrap(err, "could not determine if path is a directory") + } + + keystoresImported := make([]*keymanager.Keystore, 0) + + if isDir { + files, err := os.ReadDir(dir) + if err != nil { + return nil, errors.Wrap(err, "could not read dir") + } + if len(files) == 0 { + return nil, fmt.Errorf("directory %s has no files, cannot import from it", dir) + } + for _, f := range files { + fullPath := filepath.Join(dir, f.Name()) + if f.IsDir() { + subKeystores, err := processDirectory(ctx, fullPath, depth+1) + if err != nil { + return nil, err + } + keystoresImported = append(keystoresImported, subKeystores...) + } else { + keystore, err := readKeystoreFile(ctx, fullPath) + if err != nil { + if strings.Contains(err.Error(), "could not decode keystore json") { + continue + } + return nil, errors.Wrapf(err, "could not import keystore at path: %s", fullPath) + } + keystoresImported = append(keystoresImported, keystore) + } + } + } else { + keystore, err := readKeystoreFile(ctx, dir) + if err != nil { + return nil, errors.Wrap(err, "could not import keystore") + } + keystoresImported = append(keystoresImported, keystore) + } + + return keystoresImported, nil +} + // ImportAccounts can import external, EIP-2335 compliant keystore.json files as // new accounts into the Prysm validator wallet. func ImportAccounts(ctx context.Context, cfg *ImportAccountsConfig) ([]*keymanager.KeyStatus, error) { diff --git a/validator/accounts/wallet/BUILD.bazel b/validator/accounts/wallet/BUILD.bazel index 578ad8cad6..72fb5f31dd 100644 --- a/validator/accounts/wallet/BUILD.bazel +++ b/validator/accounts/wallet/BUILD.bazel @@ -34,12 +34,15 @@ go_test( srcs = ["wallet_test.go"], deps = [ ":go_default_library", + "//cmd/validator/flags:go_default_library", "//config/params:go_default_library", "//testing/assert:go_default_library", "//testing/require:go_default_library", "//validator/accounts/iface:go_default_library", + "//validator/keymanager:go_default_library", "//validator/keymanager/remote-web3signer:go_default_library", "@com_github_ethereum_go_ethereum//common/hexutil:go_default_library", "@com_github_sirupsen_logrus//:go_default_library", + "@com_github_urfave_cli_v2//:go_default_library", ], ) diff --git a/validator/accounts/wallet/wallet.go b/validator/accounts/wallet/wallet.go index 4f5ab548bc..acbc07a1e9 100644 --- a/validator/accounts/wallet/wallet.go +++ b/validator/accounts/wallet/wallet.go @@ -164,6 +164,7 @@ func OpenWalletOrElseCli(cliCtx *cli.Context, otherwise func(cliCtx *cli.Context } isValid, err := IsValid(cliCtx.String(flags.WalletDirFlag.Name)) if errors.Is(err, ErrNoWalletFound) { + // reprompts the user for a valid dir return otherwise(cliCtx) } if err != nil { @@ -193,6 +194,65 @@ func OpenWalletOrElseCli(cliCtx *cli.Context, otherwise func(cliCtx *cli.Context }) } +// OpenOrCreateNewWallet takes a cli and returns a wallet either opening an existing valid wallet or creating a new one. +func OpenOrCreateNewWallet(cliCtx *cli.Context) (*Wallet, error) { + walletDir, err := accountsprompt.InputDirectory(cliCtx, accountsprompt.WalletDirPromptText, flags.WalletDirFlag) + if err != nil { + return nil, err + } + exists, err := Exists(walletDir) + if err != nil { + return nil, errors.Wrap(err, CheckExistsErrMsg) + } + if exists { + isValid, err := IsValid(walletDir) + if err != nil { + return nil, errors.Wrap(err, CheckValidityErrMsg) + } + if !isValid { + return nil, errors.New(InvalidWalletErrMsg) + } + walletPassword, err := InputPassword( + cliCtx, + flags.WalletPasswordFileFlag, + PasswordPromptText, + false, /* Do not confirm password */ + ValidateExistingPass, + ) + if err != nil { + return nil, err + } + return OpenWallet(cliCtx.Context, &Config{ + WalletDir: walletDir, + WalletPassword: walletPassword, + }) + } + // create a new wallet in the dir + walletPassword, err := prompt.InputPassword( + cliCtx, + flags.WalletPasswordFileFlag, + NewWalletPasswordPromptText, + ConfirmPasswordPromptText, + true, /* Should confirm password */ + prompt.ValidatePasswordInput, + ) + if err != nil { + return nil, err + } + w := New(&Config{ + KeymanagerKind: keymanager.Local, + WalletDir: walletDir, + WalletPassword: walletPassword, + }) + if err := w.SaveWallet(); err != nil { + return nil, errors.Wrap(err, "could not save wallet to disk") + } + log.WithField("wallet-path", walletDir).Info( + "Successfully created new wallet", + ) + return w, nil +} + // NewWalletForWeb3Signer returns a new wallet for web3 signer which is temporary and not stored locally. func NewWalletForWeb3Signer() *Wallet { // wallet is just a temporary wallet for web3 signer used to call initialize keymanager. diff --git a/validator/accounts/wallet/wallet_test.go b/validator/accounts/wallet/wallet_test.go index 72e873a665..d0313f14f8 100644 --- a/validator/accounts/wallet/wallet_test.go +++ b/validator/accounts/wallet/wallet_test.go @@ -2,19 +2,24 @@ package wallet_test import ( "context" + "flag" "io" "os" "path/filepath" + "reflect" "testing" "github.com/ethereum/go-ethereum/common/hexutil" + "github.com/prysmaticlabs/prysm/v4/cmd/validator/flags" "github.com/prysmaticlabs/prysm/v4/config/params" "github.com/prysmaticlabs/prysm/v4/testing/assert" "github.com/prysmaticlabs/prysm/v4/testing/require" "github.com/prysmaticlabs/prysm/v4/validator/accounts/iface" "github.com/prysmaticlabs/prysm/v4/validator/accounts/wallet" + "github.com/prysmaticlabs/prysm/v4/validator/keymanager" remoteweb3signer "github.com/prysmaticlabs/prysm/v4/validator/keymanager/remote-web3signer" "github.com/sirupsen/logrus" + "github.com/urfave/cli/v2" ) func init() { @@ -84,3 +89,85 @@ func TestWallet_InitializeKeymanager_web3Signer_nilConfig(t *testing.T) { assert.NotNil(t, err) assert.Equal(t, nil, km) } + +func TestOpenOrCreateNewWallet(t *testing.T) { + walletDir := filepath.Join(t.TempDir(), "wallet") + newDir := filepath.Join(t.TempDir(), "new") + passwordFileDir := filepath.Join(t.TempDir(), "passwordFile") + require.NoError(t, os.MkdirAll(passwordFileDir, params.BeaconIoConfig().ReadWriteExecutePermissions)) + passwordFilePath1 := filepath.Join(passwordFileDir, "password1.txt") + passwordFilePath2 := filepath.Join(passwordFileDir, "password2.txt") + + type args struct { + cliCtx *cli.Context + } + tests := []struct { + name string + args args + want *wallet.Wallet + wantErr bool + }{ + { + name: "New Wallet", + args: args{ + cliCtx: func() *cli.Context { + app := cli.App{} + set := flag.NewFlagSet("test", 0) + require.NoError(t, os.MkdirAll(newDir, 0700)) + require.NoError(t, os.WriteFile(passwordFilePath1, []byte("newnewnew"), os.ModePerm)) + set.String(flags.WalletDirFlag.Name, newDir, "") // don't set it + set.String(flags.KeymanagerKindFlag.Name, keymanager.Local.String(), "") + set.String(flags.WalletPasswordFileFlag.Name, passwordFilePath1, "") + assert.NoError(t, set.Set(flags.KeymanagerKindFlag.Name, keymanager.Local.String())) + assert.NoError(t, set.Set(flags.WalletPasswordFileFlag.Name, passwordFilePath1)) + return cli.NewContext(&app, set, nil) + }(), + }, + want: wallet.New(&wallet.Config{ + WalletDir: newDir, + KeymanagerKind: keymanager.Local, + WalletPassword: "newnewnew", + }), + }, + { + name: "Existing Wallet", + args: args{ + cliCtx: func() *cli.Context { + app := cli.App{} + set := flag.NewFlagSet("test", 0) + set.String(flags.WalletDirFlag.Name, walletDir, "") + set.String(flags.KeymanagerKindFlag.Name, keymanager.Local.String(), "") + set.String(flags.WalletPasswordFileFlag.Name, passwordFilePath2, "") + require.NoError(t, os.WriteFile(passwordFilePath2, []byte("existing"), os.ModePerm)) + w := wallet.New(&wallet.Config{ + WalletDir: walletDir, + KeymanagerKind: keymanager.Local, + WalletPassword: "existing", + }) + require.NoError(t, w.SaveWallet()) + assert.NoError(t, set.Set(flags.WalletDirFlag.Name, walletDir)) + assert.NoError(t, set.Set(flags.KeymanagerKindFlag.Name, keymanager.Local.String())) + assert.NoError(t, set.Set(flags.WalletPasswordFileFlag.Name, passwordFilePath2)) + return cli.NewContext(&app, set, nil) + }(), + }, + want: wallet.New(&wallet.Config{ + WalletDir: walletDir, + KeymanagerKind: keymanager.Local, + WalletPassword: "existing", + }), + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got, err := wallet.OpenOrCreateNewWallet(tt.args.cliCtx) + if (err != nil) != tt.wantErr { + t.Errorf("OpenOrCreateNewWallet() error = %v, wantErr %v", err, tt.wantErr) + return + } + if !reflect.DeepEqual(got, tt.want) { + t.Errorf("OpenOrCreateNewWallet() got = %v, want %v", got, tt.want) + } + }) + } +} diff --git a/validator/accounts/wallet_create.go b/validator/accounts/wallet_create.go index 039062078e..b1b081bf4f 100644 --- a/validator/accounts/wallet_create.go +++ b/validator/accounts/wallet_create.go @@ -5,7 +5,6 @@ import ( "encoding/json" "github.com/pkg/errors" - "github.com/prysmaticlabs/prysm/v4/validator/accounts/iface" "github.com/prysmaticlabs/prysm/v4/validator/accounts/wallet" "github.com/prysmaticlabs/prysm/v4/validator/keymanager" "github.com/prysmaticlabs/prysm/v4/validator/keymanager/derived" @@ -22,19 +21,10 @@ func (acm *CLIManager) WalletCreate(ctx context.Context) (*wallet.Wallet, error) var err error switch w.KeymanagerKind() { case keymanager.Local: - if err = CreateLocalKeymanagerWallet(ctx, w); err != nil { - return nil, errors.Wrap(err, "could not initialize wallet") + if err := w.SaveWallet(); err != nil { + return nil, errors.Wrap(err, "could not initialize wallet: could not save wallet to disk") } - // TODO(#9883) - Remove this when we have a better way to handle this. should be safe to use for now. - km, err := w.InitializeKeymanager(ctx, iface.InitKeymanagerConfig{ListenForChanges: false}) - if err != nil { - return nil, errors.Wrap(err, ErrCouldNotInitializeKeymanager) - } - localKm, ok := km.(*local.Keymanager) - if !ok { - return nil, errors.Wrap(err, ErrCouldNotInitializeKeymanager) - } - accountsKeystore, err := localKm.CreateAccountsKeystore(ctx, make([][]byte, 0), make([][]byte, 0)) + accountsKeystore, err := local.CreateEmptyKeyStoreRepresentationForNewWallet(ctx, w.Password()) if err != nil { return nil, err } @@ -45,7 +35,6 @@ func (acm *CLIManager) WalletCreate(ctx context.Context) (*wallet.Wallet, error) if err = w.WriteFileAtPath(ctx, local.AccountsPath, local.AccountsKeystoreFileName, encodedAccounts); err != nil { return nil, err } - log.WithField("--wallet-dir", acm.walletDir).Info( "Successfully created wallet with ability to import keystores", ) @@ -71,16 +60,6 @@ func (acm *CLIManager) WalletCreate(ctx context.Context) (*wallet.Wallet, error) return w, nil } -func CreateLocalKeymanagerWallet(_ context.Context, wallet *wallet.Wallet) error { - if wallet == nil { - return errors.New("nil wallet") - } - if err := wallet.SaveWallet(); err != nil { - return errors.Wrap(err, "could not save wallet to disk") - } - return nil -} - func createDerivedKeymanagerWallet( ctx context.Context, wallet *wallet.Wallet, diff --git a/validator/keymanager/local/keymanager.go b/validator/keymanager/local/keymanager.go index dac9789e8e..22d06a982d 100644 --- a/validator/keymanager/local/keymanager.go +++ b/validator/keymanager/local/keymanager.go @@ -323,6 +323,13 @@ func CreateAccountsKeystoreRepresentation( }, nil } +// CreateEmptyKeyStoreRepresentationForNewWallet creates a placeholder accounts keystore for a new Prysm Local Wallet. +func CreateEmptyKeyStoreRepresentationForNewWallet(ctx context.Context, walletPassword string) (*AccountsKeystoreRepresentation, error) { + // make sure everything is clean when creating this. + ResetCaches() + return CreateAccountsKeystoreRepresentation(ctx, &accountStore{}, walletPassword) +} + // CreateOrUpdateInMemoryAccountsStore will set or update the local accounts store and update the local cache. // This function DOES NOT save the accounts store to disk. func (km *Keymanager) CreateOrUpdateInMemoryAccountsStore(_ context.Context, privateKeys, publicKeys [][]byte) error { @@ -425,13 +432,10 @@ func (km *Keymanager) ListKeymanagerAccounts(ctx context.Context, cfg keymanager func CreatePrintoutOfKeys(keys [][]byte) string { var keysStr string for i, k := range keys { - if i == 0 { - keysStr += fmt.Sprintf("%#x", bytesutil.Trunc(k)) - } else if i == len(keys)-1 { - keysStr += fmt.Sprintf("%#x", bytesutil.Trunc(k)) - } else { - keysStr += fmt.Sprintf(",%#x", bytesutil.Trunc(k)) + if i != 0 { + keysStr += "," // Add a comma before each key except the first one } + keysStr += fmt.Sprintf("%#x", bytesutil.Trunc(k)) } return keysStr } diff --git a/validator/node/node.go b/validator/node/node.go index 73d9c091c6..c8f256e166 100644 --- a/validator/node/node.go +++ b/validator/node/node.go @@ -216,6 +216,7 @@ func (c *ValidatorClient) initializeFromCLI(cliCtx *cli.Context, router *mux.Rou if cliCtx.IsSet(flags.Web3SignerURLFlag.Name) { c.wallet = wallet.NewWalletForWeb3Signer() } else { + fmt.Println("initializeFromCLI asking for wallet") w, err := wallet.OpenWalletOrElseCli(cliCtx, func(cliCtx *cli.Context) (*wallet.Wallet, error) { return nil, wallet.ErrNoWalletFound })