-
-
Notifications
You must be signed in to change notification settings - Fork 242
Closed
Labels
Description
We should merge EthKeyringController
functionalities into KeyringController
, without changing any of the features exposed to clients by KeyringController
.
This issue does not (necessarily) cover the work needed to keep test coverage at the 100% threshold, which is tracked by #3767 instead.
The planned work is described in #2043.
Details
Constructor & Class properties
- We can retain the KeyringController constructor, merging EthKeyringController's init logic in it
KeyringController.#keyring
will have to be removed, andEthKeyringController.keyrings
should be moved toKeyringController.#keyrings
- 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
EthKeyringController.#cacheEncryptionKey
and.#encryptor
should be moved toKeyringController.#cacheEncryptionKey
and.#encryptor
Controller State & Messenger
- 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.
- 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
- For the same reason as above, the minimum API coverage should be represented by the current KeyringController public API.
- All current public & private methods in EthKeyringController can be considered as potential private methods in the resulting new KeyringController API
- 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).
- 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
- The keyring builder factory should be migrated to the
@metamask/keyring-controller
package - Types related to the Encryptor should also be migrated
Blockers
This issue is blocked by these prerequisites:
- 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
- This PR will be opened again later on
core
repo, targetingKeyringController
- This PR will be opened again later on