Description
What is this about?
Q1 2025 O2KR2:
"Unflatten Redux state for [100]% of our controllers to improve load time of Extension views."
Primary Outcome
In the Extension, our background state object gets “flattened” before being sent to the UI, which obscures the origin of each piece of state and negatively impacts performance (incl. the “UI startup” span). We should stop doing this and instead leave our state grouped by controller.
There are also non-performance benefits:
- Better debugging: Removes unnecessary obfuscation on which state property belongs to which controller, making runtime error messages easier to understand.
- Reduces errors: By isolating each controller's state in Redux, the possibility of state pollution due to one controller overriding state properties that belong to another controller is removed.
Phases
1. Preparation
- [Unflattening #0] Remove non controller-state properties from
metamask
slice. #29601 - [Unflattening #1] Create Redux
background
slice and hydrate with composed controller state #29602- Supply unflattened state to Redux.
- Add temporary flattening step to state-update patches, which are then be applied to the flattened
metamask
slice. - Corresponding unflattened patches for migrated controllers are applied to the unflattened
background
slice.
- [Unflattening #2] Convert all remaining JavaScript selectors for
metamask
slice to TypeScript #29628- Type all selector state types with entire
metamask
slice as temporary measure.
- Type all selector state types with entire
- [Unflattening #3] Construct exhaustive state object for
metamask
slice with default values for all nested properties. #29629
2. Horizontal sharding
Repeat the following steps for each controller (group by domain i.e. codeowner team).
- [
Unflattening #4: [[ControllerName]] #1
] Unflattenmetamask
slice- Remove flattening step added in PR 1.
- Only unflatten the targeted controller's state.
- [
Unflattening #4: [[ControllerName]] #2
] Unflatten selectors - [
Unflattening #4: [[ControllerName]] #3
] Unflatten hooks, components- Spread exhaustive default state constructed in PR 3.
- [
Unflattening #4: [[ControllerName]] #4
] Unflatten mock objects- Spread exhaustive default state constructed in PR 3.
- [
Unflattening #4: [[ControllerName]] #5
] Unflatten tests - [
Unflattening #4: [[ControllerName]] #6
] Unflatten remaining downstream references
Some participation and oversight from the feature teams will be necessary to ensure acceptance criteria (see below) are satisfied.
Some smaller PRs or controllers could be merged together, while larger controllers may require the full 6 PRs. TBD on a case-by-case basis.
3. Clean-up temporary measures
- [
Unflattening #5
] [EPIC] Fix selector state types to use composition and inference - [
Unflattening #6
] Update test mocks to remove exhaustive default state spreading. - [
Unflattening #7
] Update hooks and components to remove exhaustive default state spreading.
Deprecated "feature branch" approach
1. Upstream
- [Unflattening #0] Remove non controller-state properties from
metamask
slice. #29601 - [Unflattening #1] Create Redux
background
slice and hydrate with composed controller state #29602
2. Redux
- [Unflattening #2] Unflatten
metamask
slice. #29603 - [Unflattening #3] Unflatten Selectors #29604
- 6k+ LoC. Splitting into reviewable sub-PRs is going to be time-consuming.
- Needs close review from feature teams to catch unintentional breaking changes or bugs.
3. Downstream
- [Unflattening #4] Unflatten Hooks #29605
- Probably do-able without comprehensive TypeScript conversions.
- [Unflattening #5] Unflatten Mock objects #29606
- ~800 test objects to update spanning 20k+ LoC
- Progress accelerated via AST transformation scripts
- Parallelizable by sharing source-of-truth reverse mapping of property names to parent controllers.
- Manual review not required. Can rely on tests.
- ~800 test objects to update spanning 20k+ LoC
- [Unflattening #6] Unflatten Tests #29607
- 80 updates to property references.
- Unknown number of updates to test logic.
- Parallelizable with cooperation from feature teams.
- [Unflattening #7] Unflatten remaining references to
metamask
slice #29608- Just needs 57 updates across 24 files.
- Search term:
state\.metamask\.(?!(\w+Controller|AccountTracker|SnapsRegistry))
- Include:
*.ts, *.tsx, *.js, *.jsx, *.json
- Exclude:
*.test.*
- Include:
- Search term:
- Just needs 57 updates across 24 files.
Scenario
No response
Design
No response
Technical Details
Doesn't the Redux team recommend normalizing state?
It also suggests organizing state by entity/domain (instead of turning it into a soup of unrelated properties 😜).
Reviews, reviews, reviews
- Reviews are the most time-consuming, necessary, and so far deficient area of progress on this epic. Coordination needed.
- Due to the very large surface area of the epic, I'm prioritizing getting MVPs in front of reviewers with more experience in the affected sections of the codebase, rather than aiming for zero mistakes on my own.
- Feature teams will need to do close reviews of sections they have ownership over, instead of the usual post-hoc codeowner approval.
- Close examination by QA is advisable, as tests are also significantly refactored.
Refine feature branch into independently testable shards
The current approach of merging the entire refactor at once has been determined to be too risky. The epic needs to be reorganized so that it can be reviewed, tested, and merged in smaller chunks (grouped by controller and/or domain).
The previous estimate of 1 month to completion is now revised. The new goal will be 1 month to implement this reorganization, and end of Q1 or later for completion. Overall, this will increase total project duration, but not significantly beyond the time estimate for having taken the revised approach from the beginning.
- Redux state hydration flow
- PRs 1-2 will be refactored so that they can each be merged in isolation.
- This will be accomplished by temporarily moving the flattening step to happen in Redux state, and eliminating it at a later stage.
- Selectors
- The refactor for fixing TypeScript selector state types to be inferred via
createSelector
will be extracted to a separate ticket. - Instead, all selectors will be converted to TypeScript by being temporarily assigned a common state type -- the entire Redux
metamask
slice. - All tests, hooks, and components will supply selectors with, at minimum, a mock object containing default values for all nested properties of the
metamask
slice.- Without this change, we would need to essentially turn off TypeScript in all tests and components due to the overwhelming number of type errors.
- A raw state snapshot from an initially loaded app will be used as the basis for building this comprehensive mock object.
- During this process, we will attempt to consolidate overlapping mock test objects as much as possible to reduce the workload of unflattening them.
Previous discussion: Is this an "all or nothing" task?
- [
acceptedrejected] Feature branch- Keeping in sync with
main
is a challenge, but has so far been manageable.
- Keeping in sync with
- [
rejectedaccepted] “Horizontal sharding”: Unflatten one controller state at a time.metamask
slice structure is inconsistent and in flux.- Makes feature development difficult and prone to bugs.
- [rejected] “Vertical sharding”:
- A. Keep the existing
metamask
slice as is, and apply changes to a new temporary slice one PR at a time, and then replace themetamask
slice at the end.- Large chunks of nonfunctional, duplicated code in production.
- Confusing for developers and increases the risk for bugs.
- B. Move the
metamask
slice properties to a new unflattened slice one controller at a time.- Wouldn’t really decrease the overall time or amount of work.
- Would be trickier to get the benefit of formal verification from TypeScript conversions.
- Confusing for developers and increases the risk for bugs.
- A. Keep the existing
Converting selectors to TypeScript
- See
Motivation
section of refactor: Unflatten selectors and convert to TypeScript #29014 - Completes at least 50% of Extension Platform
Q1 2025 Non-OKR Priority-1 (Convert [100]% of strategic files to Typescript)
. - Keyword: "heterogeneously-typed selector composition"
- See:
Motivation
section of chore: Bumpreselect
to^5.1.1
for heterogeneously-typed selectors support #29094
- See:
Threat Modeling Framework
No response
Acceptance Criteria
- All references/definitions for existing
metamask
slice properties are updated to be nested under their correct parent controllers. - Does not introduce breaking changes or bugs to features or business logic.
- Is broken into PRs of testable, reviewable, and independently mergeable size.
- Does not introduce dead code into the main branch.
Stakeholder review needed before the work gets merged
- Engineering (needed in most cases)
- Design
- Product
- QA (automation tests are required to pass before merging PRs but not all changes are covered by automation tests - please review if QA is needed beyond automation tests)
- Security
- Legal
- Marketing
- Management (please specify)
- Other (please specify)
References
- For additional context, see https://github.com/MetaMask/MetaMask-planning/issues/2894
- Succeeds https://github.com/MetaMask/MetaMask-planning/issues/2895
- Corpus:
- Related: