-
-
Notifications
You must be signed in to change notification settings - Fork 188
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
Add addNewAccountForKeyring
method
#1591
Conversation
dd7226e
to
4ed6001
Compare
const [primaryKeyring] = controller.getKeyringsByType( | ||
KeyringTypes.hd, | ||
) as Keyring<Json>[]; | ||
const addedAccountAddress = |
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.
Nit: Typically we separate the test into sections separated by newlines (the sections being "arrange, act, assert" for the setup, code-under-test, and assertions respectively). These tests would be a bit easier to read if we had those newlines surrounding each of these calls under test
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.
LGTM!
feat: add new account for keyring Co-authored-by: Mark Stacey <[email protected]>
feat: add new account for keyring Co-authored-by: Mark Stacey <[email protected]>
Explanation
This PR adds the
addNewAccountForKeyring
method toKeyringController
, that can be used to add a new account for a specific keyring, which might not be the primary simple keyrings.To align it with the
addNewAccount
method that works for primaryKeyring, alsoaddNewAccountForKeyring
expects an optionalaccountCount
parameter to make the function idempotent.This method is needed to migrate some controllers from
@metamask/eth-keyring-controller
to@metamask/keyring-controller
.References
KeyringController
#1595Changelog
@metamask/keyring-controller
addNewAccountForKeyring
methodChecklist