You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
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.
We can retain the KeyringController constructor, merging EthKeyringController's init logic in it
KeyringController.#keyring will have to be removed, and EthKeyringController.keyrings should be moved to KeyringController.#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 to KeyringController.#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
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, targeting KeyringController
The text was updated successfully, but these errors were encountered:
We should merge
EthKeyringController
functionalities intoKeyringController
, without changing any of the features exposed to clients byKeyringController
.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
KeyringController.#keyring
will have to be removed, andEthKeyringController.keyrings
should be moved toKeyringController.#keyrings
EthKeyringController.#cacheEncryptionKey
and.#encryptor
should be moved toKeyringController.#cacheEncryptionKey
and.#encryptor
Controller State & Messenger
State
(same as current KeyringController)
Events
(same as current KeyringController)
Actions
(same as current KeyringController)
Public Methods
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 updatesThe resulting API should look like this:
(same as current KeyringController)
Types & Utils
@metamask/keyring-controller
packageBlockers
This issue is blocked by these prerequisites:
core
repo, targetingKeyringController
The text was updated successfully, but these errors were encountered: