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

refactor: Unflatten metamask Redux slice and convert to TypeScript #28929

Draft
wants to merge 13 commits into
base: jongsun/perf/redux/241008-unflatten-MetamaskController-stores
Choose a base branch
from
Draft

refactor: Unflatten metamask Redux slice and convert to TypeScript #28929

wants to merge 13 commits into from

Conversation

MajorLift
Copy link
Contributor

@MajorLift MajorLift commented Dec 4, 2024 •
edited
Loading

Important

CI fails will be addressed in subsequent PRs. Review on high-level approach needed first.

Description

Unflattens the Redux store metamask slice with a new source of truth: the BackgroundStateProxy-type object that is now supplied by the MetamaskController to Redux (#28723).

Replaces CombinedBackgroundAndReduxState, TemporaryBackgroundState.

The key changes in this PR are contained in the following two files, with the remaining files being downstream updates:

The initialState for the metamask slice has been consolidated and moved to ui/ducks/metamask/constants.ts. We should be able to remove this eventually, but the existing initial values are preserved for now.

Open in GitHub Codespaces

Related issues

Manual testing steps

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.

@MajorLift MajorLift added the team-tiger Tiger team (for tech debt reduction + performance improvements) label Dec 4, 2024
@MajorLift MajorLift self-assigned this Dec 4, 2024
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Dec 4, 2024

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 force-pushed the jongsun/perf/redux/241204-unflatten-metamask-slice branch from 236e650 to ebbef43 Compare December 4, 2024 16:19
@MajorLift MajorLift added DO-NOT-MERGE Pull requests that should not be merged and removed team-wallet-framework labels Dec 4, 2024
@MajorLift MajorLift marked this pull request as ready for review December 4, 2024 16:27
@MajorLift MajorLift requested review from a team as code owners December 4, 2024 16:27
@MajorLift MajorLift mentioned this pull request Dec 4, 2024
Draft
7 tasks
@MajorLift MajorLift changed the title perf: Unflatten metamask Redux slice perf: Unflatten metamask Redux slice (2/3) Dec 4, 2024
@MajorLift MajorLift mentioned this pull request Dec 4, 2024
Merged
7 tasks
@MajorLift MajorLift force-pushed the jongsun/perf/redux/241204-unflatten-metamask-slice branch 2 times, most recently from 4ecfafa to 865b649 Compare December 4, 2024 17:12
@MajorLift MajorLift changed the title perf: Unflatten metamask Redux slice (2/3) perf: Unflatten metamask Redux slice and convert to TypeScript (2/3) Dec 4, 2024
@MajorLift MajorLift force-pushed the jongsun/perf/redux/241204-unflatten-metamask-slice branch from 865b649 to 0ce95c9 Compare December 5, 2024 03:06
@MajorLift MajorLift force-pushed the jongsun/perf/redux/241008-unflatten-MetamaskController-stores branch 2 times, most recently from 4c9f32c to 286bcab Compare December 5, 2024 06:20
@MajorLift MajorLift force-pushed the jongsun/perf/redux/241204-unflatten-metamask-slice branch 2 times, most recently from 59c3d27 to 860d590 Compare December 5, 2024 13:10
@MajorLift MajorLift force-pushed the jongsun/perf/redux/241008-unflatten-MetamaskController-stores branch from c22768b to ee9c45e Compare December 6, 2024 16:48
@MajorLift MajorLift requested a review from a team as a code owner December 6, 2024 16:48
@MajorLift MajorLift force-pushed the jongsun/perf/redux/241204-unflatten-metamask-slice branch from 860d590 to 431ba66 Compare December 6, 2024 16:54
@MajorLift MajorLift force-pushed the jongsun/perf/redux/241008-unflatten-MetamaskController-stores branch from ee9c45e to 5de8ba9 Compare December 6, 2024 18:05
@MajorLift MajorLift force-pushed the jongsun/perf/redux/241204-unflatten-metamask-slice branch from 431ba66 to a0dea86 Compare December 6, 2024 18:06
@MajorLift MajorLift force-pushed the jongsun/perf/redux/241008-unflatten-MetamaskController-stores branch from 5de8ba9 to 9f7195b Compare December 6, 2024 18:34
@MajorLift MajorLift force-pushed the jongsun/perf/redux/241204-unflatten-metamask-slice branch 3 times, most recently from b4e3d7a to c4c5dc0 Compare December 8, 2024 18:25
@MajorLift MajorLift force-pushed the jongsun/perf/redux/241204-unflatten-metamask-slice branch from af56482 to b5ddb61 Compare December 12, 2024 20:14
@MajorLift MajorLift force-pushed the jongsun/perf/redux/241008-unflatten-MetamaskController-stores branch from c0736de to 996b126 Compare December 18, 2024 03:29
@MajorLift MajorLift force-pushed the jongsun/perf/redux/241204-unflatten-metamask-slice branch from b5ddb61 to 62d01dd Compare December 18, 2024 03:30
@MajorLift MajorLift force-pushed the jongsun/perf/redux/241008-unflatten-MetamaskController-stores branch from 996b126 to f4ef2f1 Compare January 6, 2025 16:11
@MajorLift MajorLift force-pushed the jongsun/perf/redux/241204-unflatten-metamask-slice branch 2 times, most recently from b1ce481 to 1add77c Compare January 6, 2025 18:15
@MajorLift MajorLift force-pushed the jongsun/perf/redux/241008-unflatten-MetamaskController-stores branch from da73439 to cd636c5 Compare January 6, 2025 19:38
@MajorLift MajorLift force-pushed the jongsun/perf/redux/241204-unflatten-metamask-slice branch from 1add77c to c47ed91 Compare January 6, 2025 19:39
@MajorLift MajorLift changed the title refactor: Unflatten metamask Redux slice and convert to TypeScript (2/5) refactor: Unflatten metamask Redux slice and convert to TypeScript (2/6) Jan 7, 2025
@MajorLift MajorLift mentioned this pull request Jan 9, 2025
Open
27 tasks
@MajorLift MajorLift changed the title refactor: Unflatten metamask Redux slice and convert to TypeScript (2/6) refactor: Unflatten metamask Redux slice and convert to TypeScript Jan 9, 2025
@MajorLift MajorLift requested review from HowardBraham and removed request for HowardBraham January 9, 2025 15:57
@MajorLift MajorLift marked this pull request as draft January 10, 2025 17:31
github-merge-queue bot pushed a commit that referenced this pull request 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.
@MajorLift MajorLift force-pushed the jongsun/perf/redux/241008-unflatten-MetamaskController-stores branch from cd636c5 to 83316a5 Compare January 14, 2025 11:04
@MajorLift
Move initialMetamaskState into ui/ducks/metamask/constants.ts
312898f
@MajorLift
Remove TemporaryBackgroundState type and merge into `initialMetamas…
46d9d90 
…kState` for inference of `metamask` slice type
@MajorLift
Merge MetamaskState defined in metametrics-controller into `initi…
42607b3 
…alMetamaskState`
@MajorLift
Convert to TypeScript: combineReducer, metamask slice
ad32360
@MajorLift
Unflatten metamask slice and convert to TypeScript
161b882
@MajorLift
Replace CombinedBackgroundAndReduxState type with fixed `MetaMaskRe…
d05b4ec 
…duxState` type
@MajorLift
Remove unused import
fdcc097
@MajorLift
Unflatten SENTRY_UI_STATE object
6b66633
@MajorLift
Add missing PreferencesController.preferences properties to `SENTRY…
cce8b65 
…_BACKGROUND_STATE`
@MajorLift
Reduce diffs
578ccb6
@MajorLift
Rename MetamaskSliceState to MetaMaskSliceState
a06e900
@MajorLift
Unflatten metametrics-controller
5f65555
@MajorLift
Update type in updateMetamaskState
7af2f6a
@MajorLift MajorLift force-pushed the jongsun/perf/redux/241204-unflatten-metamask-slice branch from c47ed91 to 7af2f6a Compare January 14, 2025 11:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@matthewwalsh0 matthewwalsh0 Awaiting requested review from matthewwalsh0

@danjm danjm Awaiting requested review from danjm

Assignees

@MajorLift MajorLift

Labels
DO-NOT-MERGE Pull requests that should not be merged team-extension-platform team-tiger Tiger team (for tech debt reduction + performance improvements)
Projects
PR review queue
Status: Needs more work from the author
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@MajorLift @metamaskbot