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

fix: Move foreground state properties from metamask slice to appState slice #28703

Open
wants to merge 13 commits into
base: main
Choose a base branch
from

Conversation

MajorLift
Copy link
Contributor

@MajorLift MajorLift commented Nov 25, 2024

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 leave 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.
  1. Consolidates appState slice selectors from ui/selectors/selectors.js and ui/app/app.ts to ui/app/selectors.ts.

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.

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 force-pushed the jongsun/perf/redux/241122-move-metamask-slice-ui-properties-to-appstate-slice branch 4 times, most recently from ae56b24 to 904d5b3 Compare November 26, 2024 10:42
@MajorLift MajorLift changed the title Clean up metamask Redux slice fix: Move foreground state properties from metamask slice to appState slice Nov 26, 2024
@MajorLift MajorLift added team-tiger Tiger team (for tech debt reduction + performance improvements) type-tech-debt Technical debt and removed team-wallet-framework labels Nov 26, 2024
@MajorLift MajorLift self-assigned this Nov 26, 2024
@MajorLift MajorLift force-pushed the jongsun/perf/redux/241122-move-metamask-slice-ui-properties-to-appstate-slice branch from 904d5b3 to a0e1a64 Compare November 27, 2024 10:50
@MajorLift MajorLift force-pushed the jongsun/perf/redux/241122-move-metamask-slice-ui-properties-to-appstate-slice branch 14 times, most recently from ffb2fea to 36eb3e6 Compare December 2, 2024 18:50
@MajorLift MajorLift marked this pull request as ready for review December 2, 2024 19:18
@MajorLift MajorLift requested review from a team as code owners December 2, 2024 19:18
mcmire
mcmire previously approved these changes Dec 5, 2024
Copy link
Contributor

@mcmire mcmire left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems pretty straightforward to me. Nothing jumped out at me.

@MajorLift MajorLift force-pushed the jongsun/perf/redux/241122-move-metamask-slice-ui-properties-to-appstate-slice branch from ee88f1e to 58f01fe Compare December 6, 2024 16:40
@MajorLift MajorLift requested a review from a team as a code owner December 6, 2024 16:40
@MajorLift MajorLift force-pushed the jongsun/perf/redux/241122-move-metamask-slice-ui-properties-to-appstate-slice branch 10 times, most recently from d4384c5 to b63607f Compare December 6, 2024 17:50
@MajorLift MajorLift force-pushed the jongsun/perf/redux/241122-move-metamask-slice-ui-properties-to-appstate-slice branch from b63607f to 537fef8 Compare December 6, 2024 18:04
@metamaskbot
Copy link
Collaborator

Builds ready [537fef8]
Page Load Metrics (1944 ± 69 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint17562219194414268
domContentLoaded17062188191114871
load17432222194414469
domInteractive268635136
backgroundConnect897342311
firstReactRender1691352814
getState793931617938
initialActions00000
loadScripts13061686149412058
setupStore719931
uiStartup197029652305260125
Bundle size diffs [🚀 Bundle size reduced!]
  • background: 0 Bytes (0.00%)
  • ui: -130 Bytes (-0.00%)
  • common: -54 Bytes (-0.00%)

@metamaskbot
Copy link
Collaborator

Builds ready [f2da87c]
Page Load Metrics (1917 ± 98 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint167325551921208100
domContentLoaded16482543189420196
load16762558191720398
domInteractive256033105
backgroundConnect96827178
firstReactRender158822157
getState1002921344019
initialActions00000
loadScripts12592056147917282
setupStore6241042
uiStartup190429662214248119
Bundle size diffs [🚀 Bundle size reduced!]
  • background: 0 Bytes (0.00%)
  • ui: -130 Bytes (-0.00%)
  • common: -54 Bytes (-0.00%)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
team-tiger Tiger team (for tech debt reduction + performance improvements) type-tech-debt Technical debt
Projects
Status: Needs dev review
Development

Successfully merging this pull request may close these issues.

3 participants