Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[EPIC] Unflatten metamask Redux slice #29600

Open
7 of 27 tasks
MajorLift opened this issue Jan 9, 2025 · 0 comments · Fixed by #28703 · May be fixed by #28723
Open
7 of 27 tasks

[EPIC] Unflatten metamask Redux slice #29600

MajorLift opened this issue Jan 9, 2025 · 0 comments · Fixed by #28703 · May be fixed by #28723
Assignees
@MajorLift
Labels
contributor experience An issue that impacts, or planned improvement to, the contributor experience. epic release-12.11.0 Issue or pull request that will be included in release 12.11.0 team-extension-platform team-tiger Tiger team (for tech debt reduction + performance improvements) type-tech-debt Technical debt

Comments

@MajorLift
Copy link
Contributor

MajorLift commented Jan 9, 2025 •
edited
Loading

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
2. Horizontal sharding

Repeat the following steps for each controller (group by domain i.e. codeowner team).

  • [Unflattening #4: [[ControllerName]] #1] Unflatten metamask 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
2. Redux
3. Downstream

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 😜).

https://redux.js.org/usage/structuring-reducers/normalizing-state-shape#organizing-normalized-data-in-state

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.

  1. 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.
  1. 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?
  • [accepted rejected] Feature branch
    • Keeping in sync with main is a challenge, but has so far been manageable.
  • [rejected accepted] “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 the metamask 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.

-- https://consensys.slack.com/archives/C07E8DSKV0X/p1734037304467689?thread_ts=1734027692.174159&cid=C07E8DSKV0X

Converting selectors to TypeScript

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
@MajorLift MajorLift added contributor experience An issue that impacts, or planned improvement to, the contributor experience. team-extension-platform team-tiger Tiger team (for tech debt reduction + performance improvements) type-tech-debt Technical debt labels Jan 9, 2025
@MajorLift MajorLift self-assigned this Jan 9, 2025
@MajorLift MajorLift added the epic label Jan 9, 2025
@MajorLift MajorLift changed the title [EPIC] Unflatten metamask Redux slice EPIC: Unflatten metamask Redux slice Jan 9, 2025
This was referenced Jan 9, 2025
Merged
Draft
Draft
Draft
Draft
Draft
Draft
@gauthierpetetin gauthierpetetin changed the title EPIC: Unflatten metamask Redux slice [EPIC] Unflatten metamask Redux slice Jan 10, 2025
github-merge-queue bot pushed a commit that referenced this issue Jan 13, 2025
@MajorLift
fix: Move foreground state properties from metamask slice to `appSt…
2b377b3 
…ate` slice (#28703)

## **Description**

1. Moves properties in the `metamask` Redux slice that are not part of
background (i.e. controller-derived) state into the `appState` slice.

The affected state properties are as follows:

```
  customNonceValue: string;
  isAccountMenuOpen: boolean;
  isNetworkMenuOpen: boolean;
  nextNonce: string | null;
  pendingTokens: {
    [address: string]: Token & { isCustom?: boolean; unlisted?: boolean };
  };
  welcomeScreenSeen: boolean;
  confirmationExchangeRates: ContractExchangeRates;
```

- Foreground properties that are a part of `AppStateController` state
have not been moved into the `appState` slice, and will remain in the
unflattened `metamask` slice.
- It's not immediately clear why the properties listed above were
excluded from `AppStateController` state, but since all of them appear
to neither require persistence beyond a given session, nor anonymization
of PII, I'm opting to maintain the status quo and keep them out of
controller state.
- The `isInitialized` property is not included, because it's derived
from background state before the Redux store is instantiated, and is
updated by the `PatchStore` which supplies it directly to the `metamask`
slice.

2. Groups `appState` slice selectors in `ui/selectors/selectors.js`
together.

[![Open in GitHub
Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/28703?quickstart=1)

## **Related issues**

- Closes #29601
- Closes MetaMask/MetaMask-planning#2895
- 0 of 7 PRs that will close
#29600
- Blocking MetaMask/MetaMask-planning#2894
  - #28723
  - #28929
  - #28776

## **Manual testing steps**

## **Screenshots/Recordings**

<!-- If applicable, add screenshots and/or recordings to visualize the
before and after of your change. -->

### **Before**

<!-- [screenshots/recordings] -->

### **After**

<!-- [screenshots/recordings] -->

## **Pre-merge author checklist**

- [x] I've followed [MetaMask Contributor
Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask
Extension Coding
Standards](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/CODING_GUIDELINES.md).
- [x] I've completed the PR template to the best of my ability
- [x] I’ve included tests if applicable
- [x] I’ve documented my code using [JSDoc](https://jsdoc.app/) format
if applicable
- [x] I’ve applied the right labels on the PR (see [labeling
guidelines](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/LABELING_GUIDELINES.md)).
Not required for external contributors.

## **Pre-merge reviewer checklist**

- [ ] I've manually tested the PR (e.g. pull and build branch, run the
app, test code being changed).
- [ ] I confirm that this PR addresses all acceptance criteria described
in the ticket it closes and includes the necessary testing evidence such
as recordings and or screenshots.
@metamaskbot metamaskbot added the release-12.11.0 Issue or pull request that will be included in release 12.11.0 label Jan 13, 2025
@MajorLift MajorLift reopened this Jan 13, 2025
@MajorLift MajorLift mentioned this issue Jan 16, 2025
Draft
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@MajorLift MajorLift

Labels
contributor experience An issue that impacts, or planned improvement to, the contributor experience. epic release-12.11.0 Issue or pull request that will be included in release 12.11.0 team-extension-platform team-tiger Tiger team (for tech debt reduction + performance improvements) type-tech-debt Technical debt
Projects
None yet
Milestone
No milestone
2 participants
@MajorLift @metamaskbot