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

feat: #870 - Slice Storage Engine #8081

Merged
merged 44 commits into from
Dec 21, 2023
Merged

Conversation

MarioAslau
Copy link
Contributor

@MarioAslau MarioAslau commented Dec 12, 2023

Description

Currently, our Redux data is persisted under a single root key made up of the entire Redux store. We need to begin slicing the data so that we have better control over what we persist as well as limiting the errors caused by corrupt/malformed data. We will begin by normalizing the controller data. Ex. Each controller should be persisted under their own keys.

Changes:

  • engine slice now creates individual reducers for each controller with it's own initialise and update action - RTK action creators were removed until persisting the whole store. We can re-add RTK to engine and even investigate it better now as we can see exactly which reducers for which controllers would fail.
  • updated EngineService file to support individual reducers for controllers
  • Moved MigratedStorage to redux folder
  • removed persistTransform from persistConfig, due to now this being handled individual per controller through the denyList
  • marked navigation reducer as deprecated - not being used in DrawerView anymore, code is no longer reached

E2E Tests:
https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/43496a97-2d07-4b8d-9006-7cd871ccfba7

Related issues

Fixes:

Manual testing steps

  1. Go to this page...

Screenshots/Recordings

Before

After

Pre-merge author checklist

  • I’ve followed MetaMask Coding Standards.
  • I've clearly explained what problem this PR is solving and how it is solved.
  • I've linked related issues
  • I've included manual testing steps
  • I've included screenshots/recordings if applicable
  • I’ve included tests if applicable
  • I’ve documented my code using JSDoc format if applicable
  • I’ve applied the right labels on the PR (see labeling guidelines). Not required for external contributors.
  • I’ve properly set the pull request status:
    • In case it's not yet "ready for review", I've set it to "draft".
    • In case it's "ready for review", I've changed it from "draft" to "non-draft".

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.

@MarioAslau MarioAslau added needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) team-mobile-client labels Dec 12, 2023
@MarioAslau MarioAslau self-assigned this Dec 12, 2023
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.

@codecov-commenter
Copy link

codecov-commenter commented Dec 15, 2023

Codecov Report

Attention: 12 lines in your changes are missing coverage. Please review.

Comparison is base (968016f) 39.46% compared to head (203f089) 39.50%.

Files Patch % Lines
app/redux/storage/MigratedStorage.ts 8.33% 11 Missing ⚠️
app/redux/slices/engine/index.ts 93.75% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #8081      +/-   ##
==========================================
+ Coverage   39.46%   39.50%   +0.04%     
==========================================
  Files        1227     1228       +1     
  Lines       29796    29780      -16     
  Branches     2833     2831       -2     
==========================================
+ Hits        11759    11765       +6     
+ Misses      17345    17323      -22     
  Partials      692      692              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

E2E test started on Bitrise: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/8300353a-5b35-4fa2-b7dd-d6f172534b40
You can also kick off another Bitrise E2E smoke test by removing and re-applying the (Run Smoke E2E) label

app/redux/storage/MigratedStorage.ts Outdated Show resolved Hide resolved
app/redux/storage/MigratedStorage.ts Outdated Show resolved Hide resolved
app/redux/storage/MigratedStorage.ts Outdated Show resolved Hide resolved
Copy link

sonarcloud bot commented Dec 19, 2023

Please retry analysis of this Pull-Request directly on SonarCloud

Copy link

sonarcloud bot commented Dec 19, 2023

Quality Gate Passed Quality Gate passed

The SonarCloud Quality Gate passed, but some issues were introduced.

3 New issues
0 Security Hotspots
71.4% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

Copy link
Contributor

@Cal-L Cal-L left a comment

Choose a reason for hiding this comment

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

LGTM

app/redux/slices/engine/index.ts Outdated Show resolved Hide resolved
@metamaskbot metamaskbot added needs-qa Any New Features that needs a full manual QA prior to being added to a release. and removed needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) labels Dec 19, 2023
@MarioAslau MarioAslau added No QA Needed/E2E Only Apply this label when your PR does not need any QA effort. and removed needs-qa Any New Features that needs a full manual QA prior to being added to a release. Run Smoke E2E labels Dec 21, 2023
@MarioAslau MarioAslau merged commit e2121e9 into main Dec 21, 2023
31 of 33 checks passed
@MarioAslau MarioAslau deleted the feat/870-slice-storage-enginePR branch December 21, 2023 19:03
@github-actions github-actions bot locked and limited conversation to collaborators Dec 21, 2023
@metamaskbot metamaskbot added the release-7.15.0 Issue or pull request that will be included in release 7.15.0 label Dec 21, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
No QA Needed/E2E Only Apply this label when your PR does not need any QA effort. release-7.15.0 Issue or pull request that will be included in release 7.15.0 team-mobile-platform
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants