search

MetaMask / core

This monorepo is a collection of packages used across multiple MetaMask clients
MIT License
250 stars 174 forks source link

Merge extension and mobile controllers #700

Open Gudahtt opened 2 years ago

Gudahtt commented 2 years ago

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

mcmire commented 2 years ago

Notes from Monday's standup:

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

Gudahtt commented 2 years ago

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 commented 2 years ago

Nice!!

Gudahtt commented 1 year ago

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

Potentially straightforward (4):

Collaboration (6):

Not applicable at the moment (5):

mcmire commented 1 year ago

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