core icon indicating copy to clipboard operation
core copied to clipboard

Published 20 hours ago • verified publisher icon MetaMask

Merge extension and mobile controllers

Open Gudahtt opened this issue 3 years ago • 7 comments

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

  • [ ] https://github.com/MetaMask/core/issues/1809
    • [x] Extract any API interactions or complex utility functions
    • [x] 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
  • [x] AddressBook
    • [x] Extract any API interactions or complex utility functions
    • [x] Refactor to be more easily understood
    • [x] Write comprehensive unit tests
    • [x] Normalize the functionality across both controllers
    • [x] Migrate
  • [ ] Alert
    • [x] Extract any API interactions or complex utility functions
    • [x] Refactor to be more easily understood
    • [ ] Write comprehensive unit tests
    • [x] ~Normalize the functionality across both controllers~ not needed (extension only)
    • [ ] Convert to TypeScript
    • [ ] Migrate
  • [x] Announcement
    • [x] Extract any API interactions or complex utility functions
    • [x] Refactor to be more easily understood
    • [x] Write comprehensive unit tests
    • [x] Normalize the functionality across both controllers
    • [x] Migrate
  • [x] Approval
    • [x] Extract any API interactions or complex utility functions
    • [x] Refactor to be more easily understood
    • [x] Write comprehensive unit tests
    • [x] Normalize the functionality across both controllers
    • [x] Migrate
  • [ ] AppState
    • [x] Extract any API interactions or complex utility functions
    • [x] Refactor to be more easily understood
    • [ ] Write comprehensive unit tests
    • [x] ~Normalize the functionality across both controllers~ not needed (only in extension)
    • [ ] Convert to TypeScript
    • [ ] Migrate
  • [ ] AssetsContract
    • [ ] Convert to library
  • [x] CurrencyRate
    • [x] Extract any API interactions or complex utility functions
    • [ ] Refactor to be more easily understood: https://github.com/MetaMask/core/issues/1604
    • [x] Write comprehensive unit tests
    • [x] Normalize the functionality across both controllers
    • [x] Migrate
  • [x] Ens
    • [ ] Extract any API interactions or complex utility functions: https://github.com/MetaMask/core/issues/1605
    • [ ] Refactor to be more easily understood: https://github.com/MetaMask/core/issues/1605
    • [x] Write comprehensive unit tests
    • [x] Normalize the functionality across both controllers
    • [x] Migrate https://github.com/MetaMask/metamask-extension/issues/23223
  • [x] GasFee
    • [x] Extract any API interactions or complex utility functions
    • [x] Refactor to be more easily understood
    • [ ] Write comprehensive unit tests: https://github.com/MetaMask/core/issues/1600
    • [x] Normalize the functionality across both controllers
    • [x] Migrate
  • [x] Keyring (wrapper) / keyring related metamask-controller methods / Identity related state (#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
  • [x] Message managers
    • [x] Extract any API interactions or complex utility functions
    • [x] Refactor to be more easily understood
    • [x] Write comprehensive unit tests
    • [x] Normalize the functionality across both controllers
    • [x] 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
  • [x] Network (#753)
    • [x] Extract any API interactions or complex utility functions
    • [x] Refactor to be more easily understood
    • [x] Write comprehensive unit tests
    • [x] Normalize the functionality across both controllers
    • [x] Migrate
  • [x] ~Collectibles~ Nft
    • [ ] Extract any API interactions or complex utility functions: https://github.com/MetaMask/core/issues/1606
    • [ ] Refactor to be more easily understood: https://github.com/MetaMask/core/issues/1606
    • [ ] Write comprehensive unit tests: https://github.com/MetaMask/core/issues/1597
    • [x] Normalize the functionality across both controllers
    • [ ] Remove patches from mobile (pending update to controller packages in mobile)
    • [x] Migrate
  • [x] NftDetection
    • [ ] Extract any API interactions or complex utility functions: https://github.com/MetaMask/core/issues/1607
    • [ ] Refactor to be more easily understood: https://github.com/MetaMask/core/issues/1607
    • [ ] Write comprehensive unit tests: https://github.com/MetaMask/core/issues/1601
    • [x] Normalize the functionality across both controllers
    • [x] Migrate
  • [x] Notification
    • [x] Extract any API interactions or complex utility functions
    • [x] Refactor to be more easily understood
    • [x] Write comprehensive unit tests
    • [x] ~Normalize the functionality across both controllers~ not needed (core only)
    • [x] Migrate
  • [ ] Onboarding
    • [x] Extract any API interactions or complex utility functions
    • [x] Refactor to be more easily understood
    • [ ] Write comprehensive unit tests
    • [ ] ~Normalize the functionality across both controllers~ not needed (extension only)
    • [ ] Migrate
  • [x] Permission
    • [x] Extract any API interactions or complex utility functions
    • [x] Refactor to be more easily understood
    • [x] Write comprehensive unit tests
    • [x] Normalize the functionality across both controllers
    • [x] Migrate
  • [x] https://github.com/MetaMask/core/issues/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)
    • [x] Migrate https://github.com/MetaMask/metamask-extension/issues/23181
    • [x] Convert to Typescript
  • [x] Phishing
    • [ ] Extract any API interactions or complex utility functions: https://github.com/MetaMask/core/issues/1608
    • [x] Refactor to be more easily understood
    • [x] Write comprehensive unit tests
    • [x] Normalize the functionality across both controllers
    • [x] Migrate
  • [x] 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
    • [x] Extract any API interactions or complex utility functions
    • [x] Refactor to be more easily understood
    • [ ] Write comprehensive unit tests: https://github.com/MetaMask/core/issues/1609
    • [ ] Normalize the functionality across both controllers
    • [ ] Migrate
  • [x] SmartTransactions
    • [ ] Extract any API interactions or complex utility functions: https://github.com/MetaMask/smart-transactions-controller/issues/188
    • [ ] Refactor to be more easily understood: https://github.com/MetaMask/smart-transactions-controller/issues/188
    • [ ] Write comprehensive unit tests: https://github.com/MetaMask/smart-transactions-controller/issues/189
    • [ ] ~Normalize the functionality across both controllers~ not needed (already extracted)
    • [ ] Migrate
  • [x] 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
  • [x] SubjectMetadata
    • [x] Extract any API interactions or complex utility functions
    • [x] Refactor to be more easily understood
    • [x] Write comprehensive unit tests
    • [x] Normalize the functionality across both controllers
    • [x] 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
  • [x] TokenList
    • [x] Extract any API interactions or complex utility functions
    • [ ] Refactor to be more easily understood: https://github.com/MetaMask/core/issues/1610
    • [ ] Write comprehensive unit tests: https://github.com/MetaMask/core/issues/1611
    • [x] Normalize the functionality across both controllers
    • [x] Migrate
  • [x] TokenRates
    • [x] Extract any API interactions or complex utility functions
    • [ ] Refactor to be more easily understood: https://github.com/MetaMask/core/issues/1612
    • [ ] Write comprehensive unit tests: https://github.com/MetaMask/core/issues/1598
    • [x] Normalize the functionality across both controllers
    • [ ] Remove patches from mobile (pending update to controller packages in mobile)
    • [x] Migrate
  • [x] Tokens
    • [ ] Extract any API interactions or complex utility functions: https://github.com/MetaMask/core/issues/1613
    • [ ] Refactor to be more easily understood: https://github.com/MetaMask/core/issues/1613
    • [ ] Write comprehensive unit tests: https://github.com/MetaMask/core/issues/1599
    • [x] Normalize the functionality across both controllers
    • [ ] Remove patches from mobile (pending update to controller packages in mobile)
    • [x] Migrate
  • [x] TokenBalances
    • [x] Extract any API interactions or complex utility functions
    • [x] Refactor to be more easily understood
    • [x] Write comprehensive unit tests
    • [x] ~Normalize the functionality across both controllers~ not needed (core only)
    • [x] Migrate
    • [x] https://github.com/MetaMask/core/issues/1808
  • [x] https://github.com/MetaMask/core/issues/1812
    • [x] Extract any API interactions or complex utility functions
    • [x] Refactor to be more easily understood: https://github.com/MetaMask/core/issues/1614
    • [ ] Write comprehensive unit tests: https://github.com/MetaMask/core/issues/1615
    • [x] Normalize the functionality across both controllers
    • [x] Remove patches from mobile (pending update to controller packages in mobile)
    • [x] Migrate https://github.com/MetaMask/metamask-extension/pull/22928
  • [x] 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

Gudahtt avatar Mar 01 '22 20:03 Gudahtt

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

mcmire avatar Mar 23 '22 17:03 mcmire

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.

Gudahtt avatar Apr 08 '22 19:04 Gudahtt

Nice!!

mcmire avatar Apr 08 '22 20:04 mcmire

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

Gudahtt avatar Jun 26 '23 16:06 Gudahtt

I've created https://github.com/MetaMask/core/issues/1509 which is related to this effort.

mcmire avatar Jul 14 '23 16:07 mcmire