Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merge EthKeyringController into KeyringController #3766

Closed
4 tasks done
mikesposito opened this issue Jan 11, 2024 · 1 comment · Fixed by #3830
Closed
4 tasks done

Merge EthKeyringController into KeyringController #3766

mikesposito opened this issue Jan 11, 2024 · 1 comment · Fixed by #3830
Assignees

Comments

@mikesposito
Copy link
Member

mikesposito commented Jan 11, 2024

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

  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

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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants