Plan Merge `EthKeyringController` with `KeyringController`

[mikesposito]

What is this about?

Historically, the KeyringController (@metamask/keyring-controller) and EthKeyringController (@metamask/eth-keyring-controller) were designed to cater to different clients, each with its own set of similar, yet not identical features, such as encryption mechanisms, supported keyrings, and business logic.

Over recent quarters, we've undertaken a significant overhaul of the KeyringController's API, enabling it to accommodate both sets of clients. As a result, the EthKeyringController has transitioned to a role where it operates as an internal component within the KeyringController.

Given the blurred lines of responsibility between the two controllers, and their shared management of the same state, it's becoming evident that the EthKeyringController functions more like an internal module of the KeyringController. It specifically manages certain aspects of the logic that the KeyringController's API presents.

With this in mind, it seems possible (if not necessary) to consider the fusion of these two entities into a single, unified controller. This integrated controller would retain the KeyringController's API and name, thereby allowing for a seamless transition for clients.

Such a consolidation would significantly streamline complexity and reduce the overhead associated with maintenance, simultaneously paving the way for more efficient and targeted future API enhancements (like, for example, the Keyring API consolidation).

To visualize it

The Past

The Present

The Future

Nice! What are we waiting for?

Prerequisite assumptions

Without these assumptions, this plan should be considered blocked

• All clients use KeyringController, EthKeyringController is used by KeyringController only
• KeyringController uses the latest version of EthKeyringController
• EthKeyringController has no unpublished changes
• EthKeyringController has no open PRs

How should the resulting controller look like?

Constructor & Class properties

1. We can retain the KeyringController constructor, merging EthKeyringController's init logic in it
2. KeyringController.#keyring will have to be removed, and EthKeyringController.keyrings should be moved to KeyringController.#keyrings
3. In addition to the above, all public class properties of EthKeyringController should be moved to KeyringController (if necessary) as sharp private properties, so that they are inaccessible from outside KeyringController
4. EthKeyringController.#cacheEncryptionKey and .#encryptor should be moved to KeyringController.#cacheEncryptionKey and .#encryptor

Controller State & Messenger

1. We can retain the current state metadata and all (and only) the messenger actions & events from core KeyringController: this should be reasonably easy and non-breaking, as we can assume that currently no client uses (or have access to) the internal EthKeyringController, and all other controllers don’t have access to events emitted by EthKeyringController already.
2. Any update logic for EthKeyringController's store and memStore, should update the KeyringController state instead

State

(same as current KeyringController)

{
  vault: { persist: true, anonymous: false },
  isUnlocked: { persist: false, anonymous: true },
  keyrings: { persist: false, anonymous: false },
  encryptionKey: { persist: false, anonymous: false },
  encryptionSalt: { persist: false, anonymous: false },
}

Events

(same as current KeyringController)

• KeyringControllerStateChangeEvent
• KeyringControllerLockEvent
• KeyringControllerUnlockEvent
• KeyringControllerAccountRemovedEvent
• KeyringControllerQRKeyringStateChangeEvent

Actions

(same as current KeyringController)

• KeyringControllerGetStateAction
• KeyringControllerSignMessageAction
• KeyringControllerSignPersonalMessageAction
• KeyringControllerSignTypedMessageAction
• KeyringControllerDecryptMessageAction
• KeyringControllerGetEncryptionPublicKeyAction
• KeyringControllerGetAccountsAction
• KeyringControllerGetKeyringsByTypeAction
• KeyringControllerGetKeyringForAccountAction
• KeyringControllerPersistAllKeyringsAction

Public Methods

1. For the same reason as above, the minimum API coverage should be represented by the current KeyringController public API.
2. All current public & private methods in EthKeyringController can be considered as potential private methods in the resulting new KeyringController API
3. Some of the KeyringController public methods have a direct counterpart in EthKeyringController, most of the times even with the same name: there are instances of these where we can merge (or cut-paste :D) the logic from the EthKeyringController method into the KeyringController’s counterpart (which will be the one we’ll retain).
4. There is some logic in the current KeyringController which is directly related to maintaining the two controller state in sync (e.g. fullUpdate): we can remove this logic completely, as the resulting single controller will have a single state, and all external clients can use the KeyringController messenger to subscribe to state updates

The resulting API should look like this:

(same as current KeyringController)

• addNewAccount
• addNewAccountForKeyring
• addNewAccountWithoutUpdate
• createNewVaultWithKeyring
• addNewKeyring
• verifyPassword
• isUnlocked
• exportSeedPhrase
• exportAccount
• getAccounts
• getEncryptionPublicKey
• decryptMessage
• getKeyringForAccount
• getKeyringsByType
• persistAllKeyrings
• importAccountWithStrategy
• removeAccount
• setLocked
• signMessage
• signPersonalMessage
• signTypedMessage
• signTransaction
• submitEncryptionKey
• submitPassword
• verifySeedPhrase
• getQRKeyring
• getOrAddQRKeyring
• restoreQRKeyring
• resetQRKeyringState
• getQRKeyringState
• submitQRCryptoHDKey
• submitQRCryptoAccount
• submitQRSignature
• cancelQRSignRequest
• cancelQRSynchronization
• connectQRHardware
• unlockQRHardwareWalletAccount
• getAccountKeyringType
• forgetQRDevice

Types & Utils

1. The keyring builder factory should be migrated to the @metamask/keyring-controller package
2. Types related to the Encryptor should also be migrated

Tests

The starting point of the controllers is not bad in terms of test coverage, but when porting the logic of EthKeyringController’s methods into KeyringController’s one, there will be a significant drop: we’ll have to compensate that, adding new test cases where necessary (porting EthKeyringController’s tests when possible)

Additional Notes

1. Currently, the accounts team is adding User Operations support on EthKeyringController
2. Currently, createNewVaultAndKeyring and createNewVaultAndRestore are being unified on EthKeyringController (potentially also on KeyringController)

How can we do this?

Action items (Potential PRs/Issues)

1. Merge EthKeyringController into KeyringController #3766
2. Bump KeyringController test coverage to 100% #3767
3. Mark EthKeyringController as in Maintenance #3768

[desi]

We should come up with a strategy here to figure out best plan forward and then estimate. We can convert this ticket to be to determine the plan.

[mikesposito]

Plan added to main comment

[mikesposito]

The following action point:

4. Migrate (if any) open issues and PRs from EthKeyringController to KeyringController

Has been removed, as it will be difficult to move PRs across repos. We'll have to close (or land) any PR that is currently open on EthKeyringController as a prerequisite for the merge.

[Gudahtt]

Looks good to me!

[mikesposito]

Thanks all for taking a look at it!

I think we can close this, and the work will be tracked by #3766, #3767, #3768