Redesign Keyring Controller API and identity state management
Our "identity" related state is spread out amongst many different places:
- The KeyringController (
eth-keyring-controller) holds references to the keyrings, and is responsible for the global lock/unlock and certain keyring interactions - The KeyringController wrapper class (i.e.
KeyringController.ts, which is a wrapper aroundeth-keyring-controller) exposes methods for interacting with the underlying KeyringController, and for performing actions that affect identity-related state throughout many controllers- Side-note: In the extension, the closest equivalent to this wrapper class is a collection of kerying-related methods that are directly in
metamask-controller.js
- Side-note: In the extension, the closest equivalent to this wrapper class is a collection of kerying-related methods that are directly in
- The individual keyrings hold keyring-specific state, and expose certain keyring interactions to both the KeyringController and external callers
- The PreferencesController holds a mapping of accounts to account names, which it calls an "identity"
- The PrefrencesController holds the "current selected address"
- The AccountTrackerController holds a mapping of "identities" to balances
At each of these layers, methods are exposed to directly interact with this data from anywhere else in our codebase, providing few protections against making changes that result in contradictions between these pieces of state.
So my questions are:
- How should we divide these responsibilities? Does the current distribution of responsibilities make sense?
- How can we ensure all of this state remains internally consistent?
- What is the KeyringController (`eth-keyring-controller) responsible for exactly? Why are some keyring methods called through the KeyringController, while others are called directly on keyrings?
- What is the wrapper KeyringController class responsible for? Can we avoid the need for a wrapper class altogether?
The outcome of this task should be a proposal in Notion.
Edit: Work towards this has begun, but there is a bunch of preparation required first. Here's a rough checklist of the work involved:
- [ ] KeyringController
- [x] Standardize and clean up the repository
- [ ] Migrate to TypeScript
- [ ] Update to use new Keyring interface
- [ ] Rename the repository and package
- [ ] eth-simple-keyring
- [x] Standardize and clean up the repository
- [ ] Migrate to TypeScript
- [ ] Update to use new Keyring interface
- [ ] Rename the repository and package
- [ ] eth-hd-keyring
- [x] Standardize and clean up the repository
- [ ] Migrate to TypeScript
- [ ] Update to use new Keyring interface
- [ ] Rename the repository and package
- [ ] eth-trezor-keyring
- [ ] Standardize and clean up the repository
- [ ] Migrate to TypeScript
- [ ] Update to use new Keyring interface
- [ ] Rename the repository and package
- [ ] eth-ledger-bridge-keyring
- [ ] Standardize and clean up the repository
- [ ] Migrate to TypeScript
- [ ] Update to use new Keyring interface
- [ ] Rename the repository and package
Some of these required updates to dependencies as well:
- [ ]
Keyringinterface- [x] Create the
typesrepo and all necessary basic types - [ ] Create new Keyring interface
- [x] Create the
- [x] browser-passworder
- [x] Standardize and clean up the repository
- [x] Migrate to TypeScript
- [x] Rename the repository and package
- [x] obs-store
- [x] Standardize and clean up the repository
- [x] Migrate to TypeScript
- [x] Rename the repository and package
- [x] eth-sig-util
- [x] Standardize and clean up the repository
- [x] Migrate to TypeScript
- [x] Rename the repository and package
I'm not sure where I land yet on all of these questions. I have a draft answer for the first one though:
- The KeyringController (
eth-keyring-controller) holds references to all keyrings, and is responsible for locking/unlocking, but don't provide pass-through methods for interactiing with keyrings. The rest of the codebase interacts directly with keyrings when performing keyring-related operations. - The IdentityController holds the "identities" state currently in the PreferencesController, and handles most identity-related functionality that is currently provided by the KeyringController wrapper. Most of the rest of the codebase doesn't deal directly with keys or keyrings - they deal with this "identity" abstraction.
- The AccountTrackerController remains basically as-is, responding to state updates from the IdentityController.
I've begun work to prepare for the new proposed Keyring APIs. It would have been challenging to move directly from what we have now to the new API because we've fallen behind in maintaining these repos and their dependencies. At the moment they're using JavaScript, they're full of inconsistencies and extra non-standard functions, they have outdated dependencies, they don't follow our standard lint rules, etc. So I'll be making an effort to clean them up and migrate them to TypeScript first. This will make more clear what their existing APIs are, so the proposed changes will be easier to understand, review, and implement.
I've added a rough checklist of the work involved to the issue description.
This may already be covered by the above, but linking this issue in case it gives us ideas: https://github.com/MetaMask/controllers/issues/35
The scope of this issue was too broad. I split out the identity state management to a separate issue, as well as the consolidation of keyring code between extension and mobile.
Closing this in favour of the Keyring Controller API proposal from the accounts team: https://www.notion.so/metamask-consensys/KeyringController-API-b28018e6611940d5a57d5c5a0226902c