Remove KeyringController methods that are not keyring-agnostic

KeyringController currently provides some methods specific to certain types of keyrings (that is, certain Keyring classes). Though, KeyringController should be managing all keyrings in the same way, and its API should provide access to keyrings in a generic manner, agnostic to the specific keyring type the consumer wants to interact with.

This should include removing all methods related to the QR keyring, as well as all other methods which are not applicable to the generic Keyring type, like:

addNewAccount as it is a specific method for the main HDKeyring
addNewAccountForKeyring as it is not compatible with all keyrings, and does not guarantee that the keyring passed is even managed by KeyringController
importAccountWithStrategy as it is only applicable to SimpleKeyring
- Private keys (or JSON containing keys, depending on the strategy) could also be passed directly to SimpleKeyring builder options, with some adjustments to how the SimpleKeyring class handles initialization params
exportSeedPhrase as it is only applicable to HDKeyring
exportAccount as it is only compatible with HDKeyring and SimpleKeyring
getQRKeyring as it is a QR-specific method
getOrAddQRKeyring
restoreQRKeyring
resetQRKeyringState
getQRKeyringState
submitQR*
cancelQR*
connectQRHardware
unlockQRHardwareWalletAccount
forgetQRDevice

@danroc

Before finalizing the methods we want to nuke, meet with @danroc to make sure we have a more complete list. We may need another issue for making Keyring API changes.

@mikesposito

@mikesposito Will there be some kind of "drop-in" replacement for addNewAccount ?

We @MetaMask/identity solely rely on this method for account syncing, and while the derivation path approach will tremendously help us when it's implemented in the future, we still need to maintain backwards compatibility until all users have their derivation paths uploaded to user storage.

PS: just flagging this as non-trivial and a hard dependency because it would be a breaking change for account syncing

@mathieuartu

@mikesposito Will there be some kind of "drop-in" replacement for addNewAccount ?

We @MetaMask/identity solely rely on this method for account syncing, and while the derivation path approach will tremendously help us when it's implemented in the future, we still need to maintain backwards compatibility until all users have their derivation paths uploaded to user storage.

PS: just flagging this as non-trivial and a hard dependency because it would be a breaking change for account syncing

@mathieuartu the method will be removed from KeyringController as it can be used only for the HDKeyring. Though, accounts on HD and other keyrings can still be added this way:

  const numberOfAccountsToAdd = 1;
  const newAccounts = await keyringController.withKeyring(
    { type: 'HD Key Tree' },
    async (keyring) => keyring.addAccounts(numberOfAccountsToAdd),
  );

@mathieuartu

@mathieuartu I believe you could also leverage the AccountsController in that case.

I haven't tested it, but something like this could work:

this.#messenger.subscribe('AccountsController:accountAdded', (account) => {
  if (account.metadata.keyring.type === KeyringTypes.HD) {
    // Track new HD account.
    ...
  }
});

Unless there was a specific reason to rely on the new KeyringController here 😄

EDIT:

After re-discussing with @mathieuartu actually the event won't help here, cause it's more the "action of creating HD accounts" that's required here, not "hooking" onto this kind of events. Sorry 🙃

Navigation Menu

Repository navigation

Remove `KeyringController` methods that are not keyring-agnostic #5153

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Remove KeyringController methods that are not keyring-agnostic #5153

Description

Activity

desi commented on Jan 20, 2025

mathieuartu commented on Jan 27, 2025

mikesposito commented on Jan 30, 2025

ccharly commented on Feb 6, 2025

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions

Remove `KeyringController` methods that are not keyring-agnostic #5153