[keyring-controller] Remove reliance upon the PreferencesController
#3699
Comments
|
Hey team! Please add your planning poker estimate with Zenhub @Gudahtt @kanthesha @MajorLift @mcmire |
|
@Gudahtt What is the goal of this ticket? Is it just to remove the constructor options, or is it also to ensure that other controllers are listening to keyring changes? It looks like AccountsController has code to listen to keyring state and update accounts already (which was added here: #1839), however, PreferencesController does not. I know that we are transitioning from PreferencesController to AccountsController, but how far do we think we're into that? Can we fully rely on AccountsController here or should we also have PreferencesController listen for the keyring changes since some clients may not be fully using AccountsController yet? |
It's just the former, to remove the options. Ensuring other controllers are listening to keyring changes is covered by linked tickets already. |
…#3853) ## Explanation Removes all `PreferencesController` callbacks from `KeyringController` class, methods, and tests. ## References - Closes #3699 ## Changelog ## `@metamask/keyring-controller` ### Removed - **BREAKING:** Remove `updateIdentities`, `syncIdentities`, `setSelectedAddress`, `setAccountLabel` from constructor options of `KeyringController` class. ## Checklist - [x] I've updated the test suite for new or updated code as appropriate - [x] I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate - [x] I've highlighted breaking changes using the "BREAKING" category above as appropriate
…erences controller (#3976) ## Explanation After completion of #3794 and #3699, the method `syncIdentities` is now only used internally on `KeyringController:stateChange` event and `updateIdentities` is no longer being used. ## References * Fixes #3795 ## Changelog ### `@metamask@preferences-controller` - **REMOVED**: `syncIdentities` is now private as it's only used internally to update state on KeyringController:stateChange event. - **REMOVED**: `updateIdentities` has been removed, as it's not in use anymore. ## Checklist - [x] I've updated the test suite for new or updated code as appropriate - [x] I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate - [ ] I've highlighted breaking changes using the "BREAKING" category above as appropriate
The
KeyringControllercurrently depends upon four methods from thePreferencesControllerbeing passed in as constructor arguments. These methods are called to synchronize state, and to update preferences in response to certain keyring actions.Essentially those operations don't belong in the
KeyringController. It's not responsible for thePreferencesControllerstate, thePreferencesControlleris. Conventionally a controller manages its own state, and that's it. The conventional approach here would be to update thePreferencesControllerto listen for relevant keyring related events, and react to them as appropriate internally. This keeps responsibilities clearly delineated between each controller. Event-based syncing is also safer than active syncing, in that we don't have to be concerned about forgetting to sync when we need to.The text was updated successfully, but these errors were encountered: