Streamlining KeyringController API

[mikesposito]

This issue aims to provide a list of observations and possible proposals to simplify the KeyringController API, removing unnecessary, redundant, out-of-scope, and confusing methods.

The KeyringController responsibilities should be the following (ideally, this is not written on any stone):

• Hold keyrings instances
• Provide functionalities to lock and unlock the wallet
• Keep in sync its state with the state of each keyring
• Route action calls for a specific account to the corresponding keyring that holds the account, or in other words, it should know which Keyring a specific account belongs to, and which actions are supported

[mikesposito]

Hold Keyrings

The first responsibility listed is already effectively implemented: KeyringController holds all keyring instances in its #keyrings array.

[mikesposito]

Locking and Unlocking

The second responsibility, /providing functionalities to lock and unlock the wallet/, is also already implemented, but we can probably do some refactoring to make it easier to use, and reduce the API coverage that it takes. Currently, we have these methods:

• isUnlocked: in essence, it simply returns the value of isUnlocked from the KeyringController state.
• verifyPassword: Provides a way to check the correctness of a certain password. This is used in certain cases where the user should be prompted for the password again (e.g. before SRP reveal), even if the wallet is already unlocked.
• setLocked: Wipes the keyring array from memory, and sets the wallet as locked.
• submitPassword: Unlocks the wallet with a password, extracting all serialised keyrings from the encrypted vault, and restoring them in the #keyrings array.
• submitEncryptionKey: Same as submitPassword, but with an encryption key that is usually derived from the password. This is mainly needed for MV3 compatibility.

We could do a couple of small improvements here:

• We don’t necessarily need the isUnlocked method, as that should be easily accessible with KeyringController.state.isUnlocked: we can consider it as redundant.
• submitPassword / submitEncryptionKey: we could take away some ambiguity, providing a unique method to unlock the wallet, and that could be, for simplicity, named setUnlocked (to reflect setLocked): this method should then provide a way to unlock the wallet using one of the two different possible methods (password / encryption key + salt).

[mikesposito]

Keep in sync its state with the state of each keyring

This is were things become a little more tricky. Currently, every time one of the “keyring actions” provided by the KeyringController (e.g. addNewAccount, removeAccount..) is executed, in case that action is considered to be mutating the state (e.g. an account is added or removed), each keyring is serialised, re-encrypted and saved into the vault, and its list of accounts is then replicated in the KeyringController’s state.

This mechanism is our strategy to keep the vault, the state of the #keyrings, and the KeyringController state all in sync. But what happens when a client calls some method on the keyring directly? Well, in that case the keyring would potentially mutate, but the vault and the KeyringController state would be left behind!

Unfortunately, as we’ll observe in the next paragraph “Route action calls for a specific account to the corresponding keyring”, not all possible “keyring actions” are supported by the KeyringController API, and this leads to client needing to call methods on Keyrings directly, bypassing KeyringController: this is a real case scenario for Hardware Keyrings.

This is where the KeyringController’s persistAllKeyrings method comes into play: it can be used by external clients to serialise all keyrings, save them encrypted into the vault, and reflect accounts held by them in the KeyringController state. The problem with that, as you can imagine, is that this moves this responsibility to clients.

Including all possible supported methods in KeyringController would probably be the easiest solution, but would also do the exact opposite of what these observations are for.
An alternative that would remove the need for a persistAllKeyrings method could be an event subscription between KeyringController and the single Keyrings included in the array: each Keyring could be an event emitter to which the KeyringController is subscribed to, and at each event emitted the KeyringController would execute the syncing operations. This way, clients don’t event have to know which method/action is going to mutate the state, because it will be kept in sync automatically, under the KeyringController’s hood.

[mikesposito]

Route action calls for a specific account to the corresponding keyring

As already disclosed in the previous paragraphs, not all keyrings have the same features, and some of them present different APIs, in some cases very similar to each other, yet not identical.

There's no clear distinction, as of now, about when such a difference in a specific Keyring should result in a new "special" method on KeyringController, although we are trying to restrict the added methods based on what the Keyring type provides. In some cases, the applied solution was to add a new method on KeyringController, while in other cases the clients simply get access to the keyring instance directly through these two keyring-agnostic methods provided by KeyringController:

• getKeyringForAccount
• getKeyringsByType

Unfortunately, both of the alternatives have downsides:

• Adding new methods on KeyringController to support all "special" keyrings features means increasing the API to maintain and increasing its complexity
• Dropping all the responsibility on the clients (through the above methods), on the other hand, has its own downsides, as we discussed in the previous paragraph

A brilliant proposal from the accounts team aims to turn KeyringController into a router with a method like this:

handleRequest({
  account,
  request,
  origin,
  opts?,
}: {
  address: Hex,
  request: Record<string, unknown>, // JSON-RPC request
  opts?: Record<string, unknown>,
}): Promise<unknown>

With this solution, we can route any request to any keyring, based on the targeted account, while also making sure that we keep everything in sync: this would solve the issue described.

To make that an easier goal, we could cut off some of the KeyringController methods that are not particularly relevant, or that provide features that can be accessed or implemented in other ways.

Methods related to the QRKeyring

Some of the methods related to the QRKeyring are included in the KeyringController: this is somehow a privilege that the QRKeyring has over other less standard keyrings, like other hardware keyrings.

Looking at these methods:

• getQRKeyring
• restoreQRKeyring
• resetQRKeyringState
• getQRKeyringState
• submitQRCryptoHDKey
• submitQRCryptoAccount
• submitQRSignature
• cancelQRSignRequest
• cancelQRSynchronization
• connectQRHardware
• unlockQRHardwareWallet

They are there only to expose methods from the underlying QRKeyring instance. But, if the client can use getKeyringForAccount, or getKeyringsByType (or even getOrAddQRKeyring), it can get access to the keyring directly and do the same, without the need for the above "special" methods. Moreover, there is an open issue to address differences between the QRKeyring API and the general Keyring interface.

[!WARNING] There’s a caveat that is worth noting: The getOrAddQRKeyring method has been intentionally left out from the methods inquired above. Currently, KeyringController is subscribed to events emitted by QRKeyring, as the extension uses those to display modals related to QR syncing and signing (See also https://github.com/MetaMask/core/pull/1702). This will probably make the QRKeyring trickier to align with other keyrings, and also getOrAddQRKeyring difficult to remove along with the others (as the event subscription happens in there). Perhaps, this would be easier if the event subscription between KeyringController and keyrings described in the previous paragraph “Keep in sync its state with the state of each keyring” is in place, as the QRKeyring will be able to use that channel to post its updates.

Methods related to HDKeyring

KeyringController provides some methods that, based on their name, are used to add accounts, or create the initial vault. The KeyringController method naming is not clear though, as some methods are implicitly there for the HDKeyring which is considered the default keyring, and its builder is part of the KeyringController’s default keyring builders set (along with the Simple Keyring).

To answer the question “Is there anything special in the actions executed on the primary (HD) keyring, compared to other keyrings?”, we should take a better look at these methods:

• createNewVaultAndKeychain
• createNewVaultAndRestore
• addNewAccount
• verifySeedPhrase
• exportSeedPhrase

The HDKeyring is, as we said, the primary keyring. It has this nickname because it is the keyring currently created on the first vault creation or restore, through the first two methods listed: This behaviour is being changed by the accounts team, so this will probably no longer be the case, but there's something probably worth noting. Currently, the KeyringController has the responsibility to generate the first random mnemonic and the first account on the HD Keyring when the vault is being created for the first time: this is something that HDKeyring could manage itself, through a .init method that would be called by KeyringController as any other keyring.

Regarding addNewAccount, the only special thing that we do compared to the general addNewAccountForKeyring is calling KeyringController.verifySeedPhrase, which verifies that all HD accounts can be recovered with the mnemonic held by the primary keyring instance: this is probably to cover some edge case, but might deserve some investigation to understand if that’s real useful when adding new accounts, and in case it is, if it should be somehow added also for other keyrings.

Lastly, exportSeedPhrase is just exposing .mnemonic for the primary keyring, which again, can be accessed by using getKeyringsByType and accessing the keyring directly.

Given that, the answer to the question “Is there anything special in the actions executed on the primary (HD) keyring, compared to other keyrings?” is most likely No.

Methods related to SimpleKeyring

The SimpleKeyring is the keyring type used for imported accounts. It can be seen as a keyring that holds a single private key, instead of a mnemonic.

KeyringController has a dedicated method to import accounts: importAccountWithStrategy: this method allows to import an account from a private key or a JSON, and all it does is validate input arguments, convert them, and then call the general KeyringController.addNewKeyring method. It is probably worth evaluating whether would be possible to move this logic to the SimpleKeyring class directly, removing the need of a special method on KeyringController.

[mikesposito]

Proposed solutions

Lock / Unlock

• [ ] Remove isUnlocked method
• [ ] Refactor submitPassword and submitEncryptionKey into one single setUnlocked method

---

State syncing

Phase 1

• [ ] Add subscribe method to the Keyring type

Phase 2

• [ ] Add subscribe method to HDKeyring
• [ ] Add subscribe method to SimpleKeyring
• [ ] Add subscribe method to QRKeyring
• [ ] Add subscribe method to LedgerKeyring
• [ ] Add subscribe method to TrezorKeyring

Phase 3

• [ ] KeyringController should subscribe to all added keyrings
• [ ] Remove persistAllKeyrings method

---

KeyringController redundant methods removal

• [ ] Remove redundant QRKeyring methods from KeyringController:
  • getQRKeyring
  • restoreQRKeyring
  • resetQRKeyringState
  • getQRKeyringState
  • submitQRCryptoHDKey
  • submitQRCryptoAccount
  • submitQRSignature
  • cancelQRSignRequest
  • cancelQRSynchronization
  • connectQRHardware
  • unlockQRHardwareWallet

---

Remove redundant HDKeyring methods (and features) from KeyringController

• [ ] First mnemonic generation and first account creation should happen in HDKeyring .init (instead of KeyringController)
• [ ] Assess if verifySeedPhrase can be removed, and addNewAccount along with it
• [ ] Remove exportSeedPhrase from KeyringController

---

Remove redundant SimpleKeyring methods from KeyringController

• [ ] Move importAccountWithStrategy logic from KeyringController to SimpleKeyring

[mcmire]

@mikesposito Good analysis! Some notes:

• Instead of setLocked and setUnlocked, what do you think about calling these methods lock and unlock? To me, "setXXXX" implies a simple setter, but this would be doing more than that, so it seems like we can use more "natural language" to reflect a more complicated action.
• If KeyringController subscribes to keyrings that are added, will/should there ever be a way to remove keyrings, and if so, should it also unsubscribe when keyrings are removed?
• I see in the KeyringController that there are three "addAccount" methods: addNewAccount, addNewAccountForKeyring, and addAccountWithoutUpdate. In particular I'm curious when addAccountWithoutUpdate is used and whether there is another way to do what it's doing without making a new method. The JSDocs for this method says "Adds a new account to the default (first) HD seed phrase keyring without updating identities in preferences", but the KeyringController shouldn't care about preferences, so perhaps this is old?
• I took a closer look at the proposal by the accounts team, in particular the Proposed API section, and there are a number of differences as compared to your proposal. What are your thoughts on this API?
• You noted above that the handleRequest method which the accounts team proposed could be used to solve the problem that in some cases consumers go straight to keyring objects to perform certain actions. However, in the accounts team's proposal, they use handleRequest not as a way of solving this problem but replacing methods that seem to delegate directly to the keyring, e.g. signTransaction, signMessage, etc.
• You also mention that we can remove all the QR methods, but just to be clear, what do you intend on replacing these with?

[mikesposito]

Thanks for taking a close look @mcmire!

Instead of setLocked and setUnlocked, what do you think about calling these methods lock and unlock? To me, "setXXXX" implies a simple setter, but this would be doing more than that, so it seems like we can use more "natural language" to reflect a more complicated action.

Agreed! lock and unlock make more sense

If KeyringController subscribes to keyrings that are added, will/should there ever be a way to remove keyrings, and if so, should it also unsubscribe when keyrings are removed?

True, they will also have to provide a way to unsubscribe

I see in the KeyringController that there are three "addAccount" methods: addNewAccount, addNewAccountForKeyring, and addAccountWithoutUpdate. In particular I'm curious when addAccountWithoutUpdate is used and whether there is another way to do what it's doing without making a new method. The JSDocs for this method says "Adds a new account to the default (first) HD seed phrase keyring without updating identities in preferences", but the KeyringController shouldn't care about preferences, so perhaps this is old?

That is correct. I didn't mention addAccountWithoutUpdate in this analysis because it will most likely be removed when solving this issue: https://github.com/MetaMask/core/issues/3848 As you said, that is not KeyringController's responsibility anymore

I took a closer look at the proposal by the accounts team, in particular the Proposed API section, and there are a number of differences as compared to your proposal. What are your thoughts on this API?

This issue is meant to be a step in that direction, aligning keyring features better so that it will be easier to apply that proposed API. We should also note that some parts of the linked accounts team proposal might be outdated, as AccountsController has been introduced, EthKeyringController/KeyringController have been merged together, and now both Extension and Mobile use the same KeyringController

You noted above that the handleRequest method which the accounts team proposed could be used to solve the problem that in some cases consumers go straight to keyring objects to perform certain actions. However, in the accounts team's proposal, they use handleRequest not as a way of solving this problem but replacing methods that seem to delegate directly to the keyring, e.g. signTransaction, signMessage, etc.

Ideally, all keyring-specific methods that consumers currently use on Keyrings object directly should be accessed through handleRequest: this can include signTransaction and signMessage, but perhaps also exportAccount and other keyring-specific methods (indeed, these are not included in the accounts team proposal).

I do think that the handleRequest method requires a more detailed technical proposal as the main problem would be to give a way to consumers (and to KeyringController) to know which features are supported by each type of keyring - this is the reason why in this issue we are not introducing that yet

You also mention that we can remove all the QR methods, but just to be clear, what do you intend on replacing these with?

The idea is to simply remove them, as consumers can access them using other KeyringController methods: E.g. restoreQRKeyring can be accessed through:

(await keyringController.getOrAddQRKeyring()).restoreQRKeyring

// or

keyringController.getKeyringsByType('qr')[0].restoreQRKeyring

This is how also other hardware keyrings are accessed by consumers

[mcmire]

@mikesposito Thanks for the replies! Seems like a well-thought-out plan in total 👍🏻