Merge extension and mobile controllers

[Gudahtt]

We want to use one shared set of controllers on both MetaMask extension and MetaMask mobile. This is to reduce to maintenance burden of each team, so that it is easier for us to provide a stable and secure codebase.

This issue description is still a work in progress. This will be updated with a list of each set of controllers to merge.

Each pair of controllers is listed in this checklist, each with sub-tasks that roughly map out the work required to merge the two controllers.

Tasks

Extract any API interactions or complex utility functions

Much of the complexity in our controllers is tied to functionality that is better managed as a separate module. Extracting API interactions and complex functionality can simplify the migration process and reduce maintenance.

Edit: More context from Mark: This refers to "reducing the scope of what controllers are responsible for (I guess you could call it the single responsibility principle)".

Refactor to be more easily understood

Some of our controllers are built in a way that makes them very difficult to reason about. For example, they might expose too many internal methods/properties, or they might have race conditions and complex polling logic. We need to understand how these controllers work in order to merge them, so we may have to spend some time simplifying them before merging in some cases.

Edit: More context from Mark: This is a "prompt to think about the controller API and simplifying it ... to improve the code first in cases where it would be really valuable, where it might save time and produce a better result".

Write comprehensive unit tests

By writing comprehensive unit tests for the controllers before we merge them, we can reduce the chance of introducing a regression.

Normalize the functionality across both controllers

The more similar we make the two controllers prior to merging them, the easier the final step will be.

Migrate

Migrate the extension and mobile projects to use the new shared controller. This is the last step.

Checklist

• Merge Extension AccountsTrackerController with the core one #1809
  • Extract any API interactions or complex utility functions
  • Refactor to be more easily understood
  • Write comprehensive unit tests
  • Normalize the functionality across both controllers
  • Remove patches from mobile (pending update to controller packages in mobile)
  • Migrate
• AddressBook
  • Extract any API interactions or complex utility functions
  • Refactor to be more easily understood
  • Write comprehensive unit tests
  • Normalize the functionality across both controllers
  • Migrate
• Alert
  • Extract any API interactions or complex utility functions
  • Refactor to be more easily understood
  • Write comprehensive unit tests
  • Normalize the functionality across both controllers not needed (extension only)
  • Convert to TypeScript
  • Migrate
• Announcement
  • Extract any API interactions or complex utility functions
  • Refactor to be more easily understood
  • Write comprehensive unit tests
  • Normalize the functionality across both controllers
  • Migrate
• Approval
  • Extract any API interactions or complex utility functions
  • Refactor to be more easily understood
  • Write comprehensive unit tests
  • Normalize the functionality across both controllers
  • Migrate
• AppState
  • Extract any API interactions or complex utility functions
  • Refactor to be more easily understood
  • Write comprehensive unit tests
  • Normalize the functionality across both controllers not needed (only in extension)
  • Convert to TypeScript
  • Migrate
• AssetsContract
  • Convert to library
• CurrencyRate
  • Extract any API interactions or complex utility functions
  • Refactor to be more easily understood: CurrencyRateController: Refactor #1604
  • Write comprehensive unit tests
  • Normalize the functionality across both controllers
  • Migrate
• Ens
  • Extract any API interactions or complex utility functions: EnsController: Refactor #1605
  • Refactor to be more easily understood: EnsController: Refactor #1605
  • Write comprehensive unit tests
  • Normalize the functionality across both controllers
  • Migrate Replace EnsController with core EnsController metamask-extension#23223
• GasFee
  • Extract any API interactions or complex utility functions
  • Refactor to be more easily understood
  • Write comprehensive unit tests: GasFeeController: Write comprehensive unit tests #1600
  • Normalize the functionality across both controllers
  • Migrate
• Keyring (wrapper) / keyring related metamask-controller methods / Identity related state (Redesign Keyring Controller API #431)
  • Extract any API interactions or complex utility functions
  • Refactor to be more easily understood
  • Write comprehensive unit tests
  • Normalize the functionality across both controllers
  • Migrate
• Message managers
  • Extract any API interactions or complex utility functions
  • Refactor to be more easily understood
  • Write comprehensive unit tests
  • Normalize the functionality across both controllers
  • Migrate
• MetaMetrics
  • Extract any API interactions or complex utility functions
  • Refactor to be more easily understood
  • Write comprehensive unit tests
  • Normalize the functionality across both controllers not needed (extension only)
  • Convert to TypeScript
  • Migrate
• Network (Merge the two network controllers #753)
  • Extract any API interactions or complex utility functions
  • Refactor to be more easily understood
  • Write comprehensive unit tests
  • Normalize the functionality across both controllers
  • Migrate
• Collectibles Nft
  • Extract any API interactions or complex utility functions: NftController: Refactor #1606
  • Refactor to be more easily understood: NftController: Refactor #1606
  • Write comprehensive unit tests: NftController: Write comprehensive unit tests #1597
  • Normalize the functionality across both controllers
  • Remove patches from mobile (pending update to controller packages in mobile)
  • Migrate
• NftDetection
  • Extract any API interactions or complex utility functions: NftDetectionController: Refactor #1607
  • Refactor to be more easily understood: NftDetectionController: Refactor #1607
  • Write comprehensive unit tests: NftDetectionController: Write comprehensive unit tests #1601
  • Normalize the functionality across both controllers
  • Migrate
• Notification
  • Extract any API interactions or complex utility functions
  • Refactor to be more easily understood
  • Write comprehensive unit tests
  • Normalize the functionality across both controllers not needed (core only)
  • Migrate
• Onboarding
  • Extract any API interactions or complex utility functions
  • Refactor to be more easily understood
  • Write comprehensive unit tests
  • Normalize the functionality across both controllers not needed (extension only)
  • Migrate
• Permission
  • Extract any API interactions or complex utility functions
  • Refactor to be more easily understood
  • Write comprehensive unit tests
  • Normalize the functionality across both controllers
  • Migrate
• Migrate PermissionLogController (from the extension) to this repo #1825
  • Extract any API interactions or complex utility functions
  • Refactor to be more easily understood
  • Write comprehensive unit tests
  • Normalize the functionality across both controllers not needed (extension only)
  • Migrate Use PermissionLogController from core metamask-extension#23181
  • Convert to Typescript
• Phishing
  • Extract any API interactions or complex utility functions: PhishingController: Refactor #1608
  • Refactor to be more easily understood
  • Write comprehensive unit tests
  • Normalize the functionality across both controllers
  • Migrate
• PPOM
  • Extract any API interactions or complex utility functions
  • Refactor to be more easily understood
  • Write comprehensive unit tests
  • Normalize the functionality across both controllers
  • Migrate
• Preferences
  • Extract any API interactions or complex utility functions
  • Refactor to be more easily understood
  • Write comprehensive unit tests: PreferencesController: Write comprehensive unit tests #1609
  • Normalize the functionality across both controllers
  • Migrate
• SmartTransactions
  • Extract any API interactions or complex utility functions: Refactor to extract API interactions and reduce complexity smart-transactions-controller#188
  • Refactor to be more easily understood: Refactor to extract API interactions and reduce complexity smart-transactions-controller#188
  • Write comprehensive unit tests: Write comprehensive unit tests smart-transactions-controller#189
  • Normalize the functionality across both controllers not needed (already extracted)
  • Migrate
• Snap — not sure if we want to do this or not?
  • Extract any API interactions or complex utility functions
  • Refactor to be more easily understood
  • Write comprehensive unit tests
  • Normalize the functionality across both controllers
  • Migrate
• SubjectMetadata
  • Extract any API interactions or complex utility functions
  • Refactor to be more easily understood
  • Write comprehensive unit tests
  • Normalize the functionality across both controllers
  • Migrate
• Swaps
  • Extract any API interactions or complex utility functions: https://github.com/MetaMask/swaps-controller/issues/158
  • Refactor to be more easily understood: https://github.com/MetaMask/swaps-controller/issues/158
  • Write comprehensive unit tests: https://github.com/MetaMask/swaps-controller/issues/159
  • Normalize the functionality across both controllers not needed (already extracted)
  • Migrate
• TokenList
  • Extract any API interactions or complex utility functions
  • Refactor to be more easily understood: TokenListController: Refactor #1610
  • Write comprehensive unit tests: TokenListController: Write comprehensive unit tests #1611
  • Normalize the functionality across both controllers
  • Migrate
• TokenRates
  • Extract any API interactions or complex utility functions
  • Refactor to be more easily understood: TokenRatesController: Refactor #1612
  • Write comprehensive unit tests: TokenRatesController: Write comprehensive unit tests #1598
  • Normalize the functionality across both controllers
  • Remove patches from mobile (pending update to controller packages in mobile)
  • Migrate
• Tokens
  • Extract any API interactions or complex utility functions: TokensController: Refactor #1613
  • Refactor to be more easily understood: TokensController: Refactor #1613
  • Write comprehensive unit tests: TokensController: Write comprehensive unit tests #1599
  • Normalize the functionality across both controllers
  • Remove patches from mobile (pending update to controller packages in mobile)
  • Migrate
• TokenBalances
  • Extract any API interactions or complex utility functions
  • Refactor to be more easily understood
  • Write comprehensive unit tests
  • Normalize the functionality across both controllers not needed (core only)
  • Migrate
  • Update TokenBalancesController to BaseControllerV2 #1808
• Merge TokenDetectionController with DetectTokensController (from extension), and migrate to this repo #1812
  • Extract any API interactions or complex utility functions
  • Refactor to be more easily understood: TokenDetectionController: Refactor #1614
  • Write comprehensive unit tests: TokenDetectionController: Write comprehensive unit tests #1615
  • Normalize the functionality across both controllers
  • Remove patches from mobile (pending update to controller packages in mobile)
  • Migrate refactor: Replace DetectTokensController with core monorepo's TokenDetectionController metamask-extension#22928
• Transaction / IncomingTransactions — NOTE: This is being addressed by the Confirmation Systems team
  • Extract any API interactions or complex utility functions
  • Refactor to be more easily understood
  • Write comprehensive unit tests
  • Normalize the functionality across both controllers
  • Migrate

[mcmire]

Notes from Monday's standup:

We probably need to prioritize Network and Keyring over other controllers.

• We're starting to ramp up work on multi-network support
• We've been wanting to fix the keyring stuff for a long time

[Gudahtt]

I have just finished making a first pass on this, checking off anything that is clearly completed already. 40 tasks are completed out of 188, that's our starting point.

[mcmire]

Nice!!

[Gudahtt]

Here are a few jotnotes regarding the current status of remaining migrations:

Potentially straightforward (4):

• AccountTracker / Cached Balances
• TokenBalances
• TokenDetection
• PermissionLog

Collaboration (6):

• AppState
  • Tricky
• Preferences
  • Tricky
• Announcement
  • Check with mobile team first
• ENS
  • Integration is leftover
• Swaps
  • Swaps team
• SmartTransactions
  • Involve swaps team

Not applicable at the moment (5):

• Alert
  • Not relevant to mobile
• Snap
  • Snaps
• Notification
  • Snaps
• Onboarding
  • Mobile feature that they don't currently support
• MetaMetrics
  • Major effort already planned to be done by mobile team

[mcmire]

I've created #1509 which is related to this effort.