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: Fix invalid state causing migration 88 to fail #26397

Merged
merged 1 commit into from
Aug 13, 2024

Conversation

Gudahtt
Copy link
Member

@Gudahtt Gudahtt commented Aug 13, 2024

Description

We have found evidence that migration 88 is failing for some users due to a null key in the TokensController.allTokens state. The migration has been updated to delete any invalid null-keys prior to migrating it, preventing the error.

Open in GitHub Codespaces

Related issues

Fixes #25938

Manual testing steps

The unit tests demonstrate the affected scenario fairly well. We addressed the "null key" case for a variety of different parts of state, but specifically the one we are seeing in prod is the allTokens state having a null key.

Screenshots/Recordings

N/A

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.

We have found evidence that migration 88 is failing for some users due
to a `null` key in the `TokensController.allTokens` state. The
migration has been updated to delete any invalid `null`-keys prior to
migrating it, preventing the error.

Fixes #25938
Copy link
Member Author

Choose a reason for hiding this comment

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

Strongly recommend using "Hide whitespace" to view this diff, it's only 38 lines with that enabled.

Copy link

sonarcloud bot commented Aug 13, 2024

Copy link

codecov bot commented Aug 13, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 70.14%. Comparing base (b3c2323) to head (47a2415).
Report is 1 commits behind head on develop.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop   #26397      +/-   ##
===========================================
+ Coverage    70.13%   70.14%   +0.01%     
===========================================
  Files         1435     1435              
  Lines        50309    50327      +18     
  Branches     13897    13915      +18     
===========================================
+ Hits         35283    35301      +18     
  Misses       15026    15026              

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

@Gudahtt Gudahtt marked this pull request as ready for review August 13, 2024 20:21
@Gudahtt Gudahtt requested a review from a team as a code owner August 13, 2024 20:21
@metamaskbot
Copy link
Collaborator

Builds ready [47a2415]
Page Load Metrics (66 ± 7 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint7512395157
domContentLoaded95121115
load419666157
domInteractive95121115
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 72 Bytes (0.00%)
  • ui: 0 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

@Gudahtt Gudahtt added the needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) label Aug 13, 2024
if (chainId === 'undefined' || chainId === undefined) {
if (
chainId === 'undefined' ||
chainId === undefined ||
Copy link
Member Author

Choose a reason for hiding this comment

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

Pretty sure Object.keys would always return undefined as a string literal, do we shouldn't need this condition. But it was already there, so I left it to minimize changes.

I confirmed that using null here didn't work, I needed to check for the string 'null'.

@Gudahtt Gudahtt merged commit 8901b68 into develop Aug 13, 2024
85 of 86 checks passed
@Gudahtt Gudahtt deleted the fix-migration-88-invalid-state branch August 13, 2024 20:44
@github-actions github-actions bot locked and limited conversation to collaborators Aug 13, 2024
@github-actions github-actions bot removed the needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) label Aug 13, 2024
@metamaskbot metamaskbot added the release-12.4.0 Issue or pull request that will be included in release 12.4.0 label Aug 13, 2024
@gauthierpetetin gauthierpetetin added release-12.3.0 Issue or pull request that will be included in release 12.3.0 and removed release-12.4.0 Issue or pull request that will be included in release 12.4.0 labels Sep 11, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
release-12.3.0 Issue or pull request that will be included in release 12.3.0 team-wallet-framework
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

[Bug]: "MetaMask migration error #88: Invalid Character" prevents metamask from loading
5 participants