core icon indicating copy to clipboard operation
core copied to clipboard

Redesign Keyring Controller API and identity state management

Open Gudahtt opened this issue 5 years ago • 3 comments

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 around eth-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
  • 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:

  • [ ] Keyring interface
    • [x] Create the types repo and all necessary basic types
    • [ ] Create new Keyring interface
  • [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

Gudahtt avatar Mar 24 '21 13:03 Gudahtt

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.

Gudahtt avatar Mar 24 '21 13:03 Gudahtt

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.

Gudahtt avatar Jul 14 '21 22:07 Gudahtt

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

mcmire avatar Mar 23 '22 17:03 mcmire

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.

Gudahtt avatar Feb 17 '23 17:02 Gudahtt

Closing this in favour of the Keyring Controller API proposal from the accounts team: https://www.notion.so/metamask-consensys/KeyringController-API-b28018e6611940d5a57d5c5a0226902c

Gudahtt avatar Apr 22 '24 19:04 Gudahtt