Migrate KeyringController to BaseControllerV2 #1378

mikesposito · 2023-05-16T14:00:55Z

Description

This PR refactors KeyringController to extend BaseControllerV2.

This controller's state is completely non-persistent and is kept in sync with the inner EthKeyringController's memStore through event subscription. For this reason, the fullUpdate method is now unnecessary and has been removed - Tests have been refactored to use controller state for assertions so that this mechanism is implicitly well covered.

All the methods that returned fullUpdate, now return the KeyringController's state with vault, encryptionKey and encryptionSalt removed as sensitive data; these parameters can always be found with KeyringController.state

Changes

BREAKING: KeyringController now inherits from BaseControllerV2
- The constructor takes a single argument, an options bag, instead of three arguments
- The state of the controller is accessible via controller.state instead of controller.store.getState()
- KeyringController now expects a messenger option (and corresponding type KeyringControllerMessenger is now available)
- The messenger will now publish KeyringController:getState and KeyringController:stateChange events (and corresponding types KeyringControllerGetStateAction and KeyringControllerStateChangeEvent are now available)
- Removed KeyringState, KeyringMemState, and KeyringConfig in favor of new types KeyringControllerState, KeyringControllerActions, KeyringControllerEvents, and KeyringControllerOptions
  - KeyringControllerState is like the previous KeyringMemState but with an extra vault property
  - KeyringControllerOptions incorporates the previous set of options and the KeyringConfig
- Added immer as a dependency
BREAKING: Removed index from the Keyring type
BREAKING: The keyringState property in the return value of addNewAccount, addNewAccountWithoutUpdate, createNewVaultAndRestore, createNewVaultAndKeychain, importAccountWithStrategy, removeAccount, setLocked, submitEncryptionKey, and submitPassword is now a type of KeyringControllerMemState instead of KeyringMemState (and may omit vault, encryptionKey, and encryptionSalt)
BREAKING: Removed subscribe and unsubscribe methods
- State changes can be directly subscribed to (or unsubscribed from) via the messenger if necessary.
BREAKING: Removed lock and unlock methods
- KeyringController:lock and KeyringController:unlock may now be subscribed to via the messenger if necessary.
BREAKING: Removed fullUpdate method
- There is no need to call this method anymore because the controller should always be up to date with the EthKeyringController instance.
ADDED: Added KeyringControllerMemState type
- This is like the previous KeyringMemState but without encryptionKey or encryptionSalt
ADDED: Added messenger events NetworkController:lock and NetworkController:unlock (and corresponding types KeyringControllerLockEvent and KeyringControllerUnlockEvent)
- The messenger will publish NetworkController:lock when the inner EthKeyringController instance publishes its lock event and NetworkController:unlock when it publishes its unlock event

References

Fixes KeyringController: Update to BaseControllerV2 #1258

Checklist

I've updated the test suite for new or updated code as appropriate
I've updated documentation for new or updated code as appropriate (note: this will usually be JSDoc)
I've highlighted breaking changes using the "BREAKING" category above as appropriate

socket-security · 2023-05-18T09:09:18Z

👍 Dependency issues cleared. Learn more about Socket for GitHub ↗︎

This PR previously contained dependency changes with security issues that have been removed or ignored.

Next steps

Take a deeper look at the dependency

Take a moment to review the security alert above. Review the linked package source code to understand the potential risk. Ensure the package is not malicious before proceeding. If you're unsure how to proceed, reach out to your security team or ask the Socket team for help at support [AT] socket [DOT] dev.

Remove the package

If you happen to install a dependency that Socket reports as Known Malware you should immediately remove it and select a different dependency. For other alert types, you may may wish to investigate alternative packages or consider if there are other ways to mitigate the specific risk posed by the dependency.

Mark a package as acceptable risk

To ignore an alert, reply with a comment starting with @SocketSecurity ignore followed by a space separated list of package-name@version specifiers. e.g. @SocketSecurity ignore [email protected] bar@* or ignore all packages with @SocketSecurity ignore-all

packages/keyring-controller/src/KeyringController.ts

packages/keyring-controller/package.json

packages/keyring-controller/src/KeyringController.ts

mikesposito · 2023-06-01T12:37:19Z

packages/keyring-controller/src/KeyringController.test.ts

    ...rest,
  });
-  const initialState = await controller.createNewVaultAndKeychain(password);


As we removed vault, encryptionKey and encryptionSalt from methods returns, we now need to take the initial state from controller.state, which contains all state properties

mcmire · 2023-06-01T23:15:10Z

I audited the set of changes here and here are my notes for the changelog:

BREAKING: KeyringController now inherits from BaseControllerV2
- The constructor takes a single argument, an options bag, instead of three arguments
- The state of the controller is accessible via controller.state instead of controller.store.getState()
- KeyringController now expects a messenger option (and corresponding type KeyringControllerMessenger is now available)
- The messenger will now publish KeyringController:getState and KeyringController:stateChange events (and corresponding types KeyringControllerGetStateAction and KeyringControllerStateChangeEvent are now available)
- Removed KeyringState, KeyringMemState, and KeyringConfig in favor of new types KeyringControllerState, KeyringControllerActions, KeyringControllerEvents, and KeyringControllerOptions
  - KeyringControllerState is like the previous KeyringMemState but with an extra vault property
  - KeyringControllerOptions incorporates the previous set of options and the KeyringConfig
- Added immer as a dependency
BREAKING: Removed index from the Keyring type
BREAKING: The keyringState property in the return value of addNewAccount, addNewAccountWithoutUpdate, createNewVaultAndRestore, createNewVaultAndKeychain, importAccountWithStrategy, removeAccount, setLocked, submitEncryptionKey, and submitPassword is now a type of KeyringControllerMemState instead of KeyringMemState (and may omit vault, encryptionKey, and encryptionSalt)
BREAKING: Removed subscribe and unsubscribe methods
- State changes can be directly subscribed to (or unsubscribed from) via the messenger if necessary.
BREAKING: Removed lock and unlock methods
- KeyringController:lock and KeyringController:unlock may now be subscribed to via the messenger if necessary.
BREAKING: Removed fullUpdate method
- There is no need to call this method anymore because the controller should always be up to date with the EthKeyringController instance.
ADDED: Added KeyringControllerMemState type
- This is like the previous KeyringMemState but without encryptionKey or encryptionSalt
ADDED: Added messenger events NetworkController:lock and NetworkController:unlock (and corresponding types KeyringControllerLockEvent and KeyringControllerUnlockEvent)
- The messenger will publish NetworkController:lock when the inner EthKeyringController instance publishes its lock event and NetworkController:unlock when it publishes its unlock event

@mikesposito I believe this aligns with what you had written in your changelog. I just had a question about the behavior of restoreQRKeyring, but I'll ask separately. Does the rest of this sound right though?

mcmire

I had a couple of questions but I've looked this over and these changes make sense to me.

mcmire · 2023-06-01T23:06:26Z

packages/keyring-controller/src/KeyringController.ts

@@ -237,7 +280,7 @@ export class KeyringController extends BaseController<
      // we return the account already existing at index `accountCount`
      const primaryKeyringAccounts = await primaryKeyring.getAccounts();
      return {
-        keyringState: await this.fullUpdate(),
+        keyringState: this.state,


Is it important to filter vault, encryptionKey and encryptionSalt out of this value?

Ops! yes, it is. I forgot this occurrence, I just changed it to this.#getMemState()

packages/keyring-controller/src/KeyringController.ts

mikesposito · 2023-06-05T08:32:55Z

@mcmire thanks for auditing this PR changes and writing down a nice changelog! I just updated the main description

mcmire

Looks good to me! Nice work 💪🏻

* feat: supp cacheEncryptionKey and submitEncryptionKey method * docs: update jsdoc * refactor: migrate KeyringController to BaseControllerV2 * fix: use immer v9 * rollback: controller parameters change * refactor: change KeyringConfig to KeyringControllerConfig * refactor: sync controller state with keyring state * refactor: no vault sync * refactor: use state value for isUnlocked * refactor: use controller state for test assertions * refactor: remove onLock and onUnlock in favor of events * refactor: sync mem and persistent store * refactor: apply review feedback * refactor: use option bag for constructor * fix: set immer as prod dependency * fix: add encryption key and salt to memstore * refactor: remove vault encryptionKey and encryptionSalt from returns * fix: remove forgotten state return

Base automatically changed from feat/1257-cache-encryption-key to main May 18, 2023 08:52

mikesposito force-pushed the refactor/update-to-basecontroller-v2 branch from 0ef1d78 to 40ebd18 Compare May 18, 2023 09:07

mikesposito commented May 18, 2023

View reviewed changes

packages/keyring-controller/src/KeyringController.ts Outdated Show resolved Hide resolved

mikesposito commented May 19, 2023

View reviewed changes

packages/keyring-controller/src/KeyringController.ts Show resolved Hide resolved

mikesposito force-pushed the refactor/update-to-basecontroller-v2 branch 2 times, most recently from 77c7655 to 706b0e7 Compare May 19, 2023 15:34

mikesposito marked this pull request as ready for review May 19, 2023 15:59

mikesposito requested a review from a team as a code owner May 19, 2023 15:59

mcmire reviewed May 22, 2023

View reviewed changes

packages/keyring-controller/src/KeyringController.ts Show resolved Hide resolved

mikesposito force-pushed the refactor/update-to-basecontroller-v2 branch from a0171f4 to cc408fe Compare May 23, 2023 10:11

mikesposito mentioned this pull request May 23, 2023

Sync stale vault on keyring store update #1384

Merged

3 tasks

mikesposito force-pushed the refactor/update-to-basecontroller-v2 branch 2 times, most recently from 965a2f9 to 65d38ca Compare May 25, 2023 11:54

Gudahtt reviewed May 25, 2023

View reviewed changes

packages/keyring-controller/package.json Outdated Show resolved Hide resolved

Gudahtt reviewed May 25, 2023

View reviewed changes

packages/keyring-controller/src/KeyringController.ts Outdated Show resolved Hide resolved

Gudahtt reviewed May 25, 2023

View reviewed changes

packages/keyring-controller/src/KeyringController.ts Outdated Show resolved Hide resolved

Gudahtt reviewed May 25, 2023

View reviewed changes

packages/keyring-controller/src/KeyringController.ts Outdated Show resolved Hide resolved

Gudahtt reviewed May 26, 2023

View reviewed changes

packages/keyring-controller/src/KeyringController.ts Outdated Show resolved Hide resolved

mikesposito force-pushed the refactor/update-to-basecontroller-v2 branch from 522e462 to 281d6ff Compare May 29, 2023 11:33

mikesposito commented Jun 1, 2023

View reviewed changes

mikesposito force-pushed the refactor/update-to-basecontroller-v2 branch from e7e6e41 to 1caf12e Compare June 1, 2023 12:40

mcmire previously approved these changes Jun 1, 2023

View reviewed changes

mikesposito dismissed mcmire’s stale review via 7b2577b June 5, 2023 08:25

mikesposito added 5 commits June 5, 2023 10:29

feat: supp cacheEncryptionKey and submitEncryptionKey method

e0f1878

docs: update jsdoc

5f9e16a

refactor: migrate KeyringController to BaseControllerV2

0b1cba7

fix: use immer v9

afc2e66

rollback: controller parameters change

5ad25f7

mikesposito added 13 commits June 5, 2023 10:29

refactor: change KeyringConfig to KeyringControllerConfig

f1a8f8f

refactor: sync controller state with keyring state

fb1a96c

refactor: no vault sync

f303c0b

refactor: use state value for isUnlocked

f9e837c

refactor: use controller state for test assertions

3efbd4f

refactor: remove onLock and onUnlock in favor of events

21f0252

refactor: sync mem and persistent store

e42f23e

refactor: apply review feedback

4c86fb5

refactor: use option bag for constructor

2dc7a3c

fix: set immer as prod dependency

7d1d265

fix: add encryption key and salt to memstore

2b78519

refactor: remove vault encryptionKey and encryptionSalt from returns

f3f0807

fix: remove forgotten state return

a0d8096

mikesposito force-pushed the refactor/update-to-basecontroller-v2 branch from 7b2577b to a0d8096 Compare June 5, 2023 08:29

mcmire approved these changes Jun 5, 2023

View reviewed changes

Merge branch 'main' into refactor/update-to-basecontroller-v2

5336c99

mikesposito merged commit 371dda3 into main Jun 6, 2023

mikesposito deleted the refactor/update-to-basecontroller-v2 branch June 6, 2023 08:42

MajorLift mentioned this pull request Dec 6, 2023

Ensure that all controllers used across clients are using BaseControllerV2 #1509

Open

29 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate KeyringController to BaseControllerV2 #1378

Migrate KeyringController to BaseControllerV2 #1378

mikesposito commented May 16, 2023 •

edited

Loading

socket-security bot commented May 18, 2023 •

edited

Loading

mikesposito Jun 1, 2023

mcmire commented Jun 1, 2023 •

edited

Loading

mcmire left a comment

mcmire Jun 1, 2023

mikesposito Jun 5, 2023

mikesposito commented Jun 5, 2023

mcmire left a comment

Migrate KeyringController to BaseControllerV2 #1378

Migrate KeyringController to BaseControllerV2 #1378

Conversation

mikesposito commented May 16, 2023 • edited Loading

Description

Changes

References

Checklist

socket-security bot commented May 18, 2023 • edited Loading

Next steps

mikesposito Jun 1, 2023

Choose a reason for hiding this comment

mcmire commented Jun 1, 2023 • edited Loading

mcmire left a comment

Choose a reason for hiding this comment

mcmire Jun 1, 2023

Choose a reason for hiding this comment

mikesposito Jun 5, 2023

Choose a reason for hiding this comment

mikesposito commented Jun 5, 2023

mcmire left a comment

Choose a reason for hiding this comment

mikesposito commented May 16, 2023 •

edited

Loading

socket-security bot commented May 18, 2023 •

edited

Loading

mcmire commented Jun 1, 2023 •

edited

Loading