-
Notifications
You must be signed in to change notification settings - Fork 922
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Remove Account Creation Privilege For Imported Keymanager #7555
Conversation
Codecov Report
@@ Coverage Diff @@
## master #7555 +/- ##
==========================================
- Coverage 61.74% 61.62% -0.13%
==========================================
Files 424 424
Lines 29884 29837 -47
==========================================
- Hits 18452 18387 -65
- Misses 8473 8496 +23
+ Partials 2959 2954 -5 |
case keymanager.Remote: | ||
return errors.New("cannot create a new account for a remote keymanager") | ||
case keymanager.Imported: | ||
km, ok := km.(*imported.Keymanager) | ||
if !ok { | ||
return errors.New("not a imported keymanager") | ||
} | ||
// Create a new validator account using the specified keymanager. | ||
if _, _, err := km.CreateAccount(ctx); err != nil { | ||
return errors.Wrap(err, "could not create account in wallet") | ||
} | ||
return errors.New("cannot create a new account for an imported wallet") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Inconsistent naming: keymanager
vs wallet
I have been thinking about it a few times while looking at accounts' code. Is our use of keymanager
/wallet
while interacting with the user unambiguous throughout the whole package?
validator/rpc/accounts.go
Outdated
if s.wallet.KeymanagerKind() != keymanager.Derived { | ||
return nil, status.Error(codes.InvalidArgument, "Only HD wallets can create accounts") | ||
} | ||
km, ok := s.keymanager.(*derived.Keymanager) | ||
if !ok { | ||
return nil, status.Error(codes.InvalidArgument, "Not a derived keymanager") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are both checks necessary? Is it possible for s.wallet.KeymanagerKind()
to be different than s.keymanager
?
@rauljordan Hmm, this part of the documentation needs to be updated https://docs.prylabs.network/docs/wallet/nondeterministic/#wallet-creation |
Part of #7515
As of beta.0, imported keymanagers cannot create new accounts for security reasons. It is better for a user to create their accounts elsewhere using an HD wallet if possible and the import those keystores as needed into Prysm