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

chore: Migrate AsyncStorage to mmkv #10153

Merged
merged 20 commits into from
Jul 5, 2024
Merged

chore: Migrate AsyncStorage to mmkv #10153

merged 20 commits into from
Jul 5, 2024

Conversation

smilingkylan
Copy link
Contributor

@smilingkylan smilingkylan commented Jun 27, 2024 •
edited
Loading

Description

While debugging a sqlite error related to AsyncStorage (surpassing storage limit on Android) we have decided to migrate from AsyncStorage to mmkv due to it not having said issue and also for performance reasons. This includes an actual migration of any existing items saved in AsyncStorage to mmkv

Related issues

Fixes: https://github.com/orgs/MetaMask/projects/60/views/6?pane=issue&itemId=63268337

Manual testing steps

  1. Start a fresh install
  2. Create new wallet and set password, etc
  3. Reboot app and see if it gives you normal EXISTING_USER flow (since that value is taken from mmkv now)

Screenshots/Recordings

Should be no discernible different in application behavior.

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.

@smilingkylan smilingkylan added team-mobile-platform Run Smoke E2E Triggers smoke e2e on Bitrise labels Jun 27, 2024
@github-actions GitHub Actions
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.

@smilingkylan smilingkylan changed the title Kylan/mmkv final Migrate AsyncStorage to mmkv Jun 28, 2024
@smilingkylan smilingkylan added Run Smoke E2E Triggers smoke e2e on Bitrise and removed Run Smoke E2E Triggers smoke e2e on Bitrise labels Jun 28, 2024
@smilingkylan smilingkylan changed the title Migrate AsyncStorage to mmkv chore: Migrate AsyncStorage to mmkv Jun 28, 2024
@smilingkylan smilingkylan marked this pull request as ready for review June 28, 2024 18:02
@smilingkylan smilingkylan requested a review from a team as a code owner June 28, 2024 18:02
@smilingkylan smilingkylan requested a review from vinistevam June 28, 2024 18:03
@smilingkylan smilingkylan linked an issue Jun 28, 2024 that may be closed by this pull request
[Sentry] Error: database or disk is full (code 13 SQLITE_FULL) #9670
Closed
@smilingkylan smilingkylan added Run Smoke E2E Triggers smoke e2e on Bitrise and removed Run Smoke E2E Triggers smoke e2e on Bitrise labels Jul 1, 2024
tommasini
tommasini reviewed Jul 1, 2024
View reviewed changes
app/store/async-storage-wrapper.js Outdated Show resolved Hide resolved
app/store/async-storage-wrapper.js Outdated Show resolved Hide resolved
app/store/async-storage-wrapper.js Outdated Show resolved Hide resolved
app/store/migrations/049.test.ts Show resolved Hide resolved
app/store/migrations/049.ts Outdated Show resolved Hide resolved
app/store/migrations/049.ts Outdated Show resolved Hide resolved
@smilingkylan smilingkylan added Run Smoke E2E Triggers smoke e2e on Bitrise and removed Run Smoke E2E Triggers smoke e2e on Bitrise labels Jul 3, 2024
@smilingkylan smilingkylan added the needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) label Jul 3, 2024
tommasini
tommasini previously approved these changes Jul 3, 2024
View reviewed changes
Copy link
Contributor

@tommasini tommasini left a comment

Choose a reason for hiding this comment

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

LGTM!

@smilingkylan smilingkylan added Run Smoke E2E Triggers smoke e2e on Bitrise and removed Run Smoke E2E Triggers smoke e2e on Bitrise labels Jul 3, 2024
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Jul 3, 2024 •
edited by metamaskbot
Loading

https://bitrise.io/ Bitrise

✅✅✅ pr_smoke_e2e_pipeline passed on Bitrise! ✅✅✅

Commit hash: 4b0e45a
Build link: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/dbcb1549-49cc-4d25-a015-eeef0dd2db4b

Note

  • You can kick off another pr_smoke_e2e_pipeline on Bitrise by removing and re-applying the Run Smoke E2E label on the pull request

vinistevam
vinistevam previously approved these changes Jul 4, 2024
View reviewed changes
Copy link
Contributor

@vinistevam vinistevam left a comment

Choose a reason for hiding this comment

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

Great PR!
One small question, Will the renaming of async-storage-wrapper.js be handled in a separate ticket?

tommasini
tommasini reviewed Jul 4, 2024
View reviewed changes
Copy link
Contributor

@tommasini tommasini left a comment

Choose a reason for hiding this comment

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

I'm wondering if we should clear all the async storage, after migrating
https://react-native-async-storage.github.io/async-storage/docs/api#clear

WDYT?

app/store/migrations/049.ts Outdated Show resolved Hide resolved
@smilingkylan smilingkylan dismissed stale reviews from vinistevam and tommasini via f002f17 July 4, 2024 14:40
@smilingkylan smilingkylan added Run Smoke E2E Triggers smoke e2e on Bitrise and removed Run Smoke E2E Triggers smoke e2e on Bitrise labels Jul 4, 2024
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Jul 4, 2024 •
edited by metamaskbot
Loading

https://bitrise.io/ Bitrise

✅✅✅ pr_smoke_e2e_pipeline passed on Bitrise! ✅✅✅

Commit hash: f002f17
Build link: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/2a63afed-cb42-4d88-a30d-77dd63a98c6d

Note

  • You can kick off another pr_smoke_e2e_pipeline on Bitrise by removing and re-applying the Run Smoke E2E label on the pull request

@sonarqubecloud SonarQubeCloud
Copy link

sonarqubecloud bot commented Jul 4, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
85.2% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

@smilingkylan smilingkylan added Run Smoke E2E Triggers smoke e2e on Bitrise and removed Run Smoke E2E Triggers smoke e2e on Bitrise labels Jul 4, 2024
@smilingkylan smilingkylan added Run Smoke E2E Triggers smoke e2e on Bitrise and removed Run Smoke E2E Triggers smoke e2e on Bitrise labels Jul 5, 2024
@smilingkylan smilingkylan merged commit 62d18ec into main Jul 5, 2024
49 of 50 checks passed
@smilingkylan smilingkylan deleted the kylan/mmkv-final branch July 5, 2024 20:07
@github-actions github-actions bot locked and limited conversation to collaborators Jul 5, 2024
@github-actions github-actions bot removed the needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) label Jul 5, 2024
@metamaskbot metamaskbot added the release-7.27.1 Issue or pull request that will be included in release 7.27.1 label Jul 5, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@tommasini tommasini tommasini approved these changes

@vinistevam vinistevam vinistevam left review comments

Assignees
No one assigned
Labels
release-7.27.1 Issue or pull request that will be included in release 7.27.1 Run Smoke E2E Triggers smoke e2e on Bitrise team-mobile-platform
Projects
PR review queue
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Sentry] Error: database or disk is full (code 13 SQLITE_FULL)
4 participants
@smilingkylan @vinistevam @tommasini @metamaskbot