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.

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

refactor: Add background Redux slice as proxy for unflattened background state #29694

Draft
wants to merge 50 commits into
base: main
Choose a base branch
from

Conversation

MajorLift
Copy link
Contributor

@MajorLift MajorLift commented Jan 14, 2025

Overview

Instead of unflattening the metamask Redux slice in-place, a new background slice has been created as a placeholder or eventual replacement.

Adopting a "horizontal sharding" approach, we will be iteratively transferring each controller's state properties from metamask to background, simultaneously updating downstream references to point to the background slice.

Pros

  • Less disruptive, as existing code still functions like before with the unaffected metamask slice.
  • Avoids causing metamask slice structure to be inconsistent and in flux while the transition takes place, introducing difficulties for feature development and potentially causing bugs.
  • Avoids triggering the flattening operation on every background state update, along with the potentially significant performance penalties.
  • Enables a complete transition of upstream logic for hydrating Redux with background state, and emitting patches for state updates, without needing to temporarily maintain duplicate versions of PatchStore and ComposableObservableStore.

Cons

  • Introduces temporary dead code into the codebase.

    • The purpose of the background slice will be documented extensively through comments and internal communications, so that feature teams are aware not to reference or modify it.
  • Significantly increased memory usage

    • Filter patches sent to background slice so that only migrated controller state is populated and updated.
      • wrap background slice with devtime feature flag
    • e2e tests ensuring no failure in e.g. low-powered devices affected by memory
      • safe to rely on existing tests and qa? probably not blocker
      • see acceptance criteria no. 2 (no breaking changes, bugs)
  • selector disentanglement for independent prs.

    • only need to update "leaf-node" selectors.

Consolidation of test mock objects (x) -> go through them individually to unflatten
Priority/urgency of clean-up tasks in phase 3 (especially regarding realization of selector memoization performance benefits)

Description

Open in GitHub Codespaces

Related issues

Fixes:

Manual testing steps

  1. Go to this page...

Screenshots/Recordings

Before

After

Pre-merge author checklist

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.

Define types for metamask controller stores
… are assigned composed controller state objects
…hange` listener and avoid iterating through entire `metamask` slice for every background state update
… and exposes their composed state via `.getState()`
…d state and individual controller states as input
…pdates to the entire background state or top-level non-object properties of background state
…re it can handle the unflattened background state
Copy link
Contributor

CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes.

@MajorLift MajorLift changed the base branch from main to jongsun/perf/redux/241008-unflatten-MetamaskController-stores January 16, 2025 10:29
@MajorLift MajorLift force-pushed the jongsun/perf/redux/2501141-add-background-redux-slice branch from 6d32839 to 9fc832c Compare January 16, 2025 11:12
@MajorLift MajorLift force-pushed the jongsun/perf/redux/2501141-add-background-redux-slice branch from 9fc832c to 2b1e3ea Compare January 16, 2025 17:32
@MajorLift MajorLift changed the base branch from jongsun/perf/redux/241008-unflatten-MetamaskController-stores to main January 16, 2025 17:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants