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

feat(migration): enable token auto-detection when basic functionality is on #26406

Merged
merged 24 commits into from
Sep 12, 2024
Merged

feat(migration): enable token auto-detection when basic functionality is on #26406

merged 24 commits into from
Sep 12, 2024

Conversation

bergeron
Copy link
Contributor

@bergeron bergeron commented Aug 14, 2024 •
edited
Loading

Description

Adds a migration that enables token auto-detection if the basic functionality toggle is on.

It also removes an unused property showTokenAutodetectModalOnUpgrade from the app metadata controller.

It is versioned 125.1 because it's planned to be cherry picked to release 12.3, and that's the subsequent migration version in that branch.

Open in GitHub Codespaces

Related issues

Manual testing steps

  • Install an older build like tag v12.1.0
  • In settings, disable token auto detection but keep basic functionality on
  • Upgrade to this build, reload extension
    • should see Running migration 125.1... in console
  • Verify token autodetection is enabled in settings
  • Download state from advanced settings
  • Verify state does not contain showTokenAutodetectModalOnUpgrade

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.

@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.

@bergeron bergeron added team-assets needs-assets-ux-review A shared label between the Assets and UX team to flag PRs ready for consolidated team review. labels Aug 14, 2024
@bergeron bergeron marked this pull request as ready for review August 14, 2024 02:45
@bergeron bergeron requested a review from a team as a code owner August 14, 2024 02:45
@bergeron bergeron marked this pull request as draft August 14, 2024 02:48
@bergeron bergeron marked this pull request as ready for review August 14, 2024 05:52
@metamaskbot
Copy link
Collaborator

Builds ready [3f487ba]
Page Load Metrics (67 ± 9 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint64144982010
domContentLoaded95825147
load4110667199
domInteractive95825147
Bundle size diffs [🚀 Bundle size reduced!]
  • background: 496 Bytes (0.01%)
  • ui: -10.05 KiB (-0.14%)
  • common: -4.21 KiB (-0.06%)

@codecov Codecov
Copy link

codecov bot commented Aug 14, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 70.13%. Comparing base (5053c3e) to head (f4bdf42).

Additional details and impacted files 
@@             Coverage Diff             @@
##           develop   #26406      +/-   ##
===========================================
+ Coverage    70.12%   70.13%   +0.01%     
===========================================
  Files         1426     1427       +1     
  Lines        49705    49722      +17     
  Branches     13905    13912       +7     
===========================================
+ Hits         34853    34870      +17     
  Misses       14852    14852

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

gambinish
gambinish previously approved these changes Aug 14, 2024
View reviewed changes
Copy link
Contributor

@gambinish gambinish left a comment

Choose a reason for hiding this comment

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

Ran testing steps and confirmed autodetection is toggled on by default after old build -> this build transition ✅

Base automatically changed from brian/token-detection-remove-modals to develop August 19, 2024 01:16
@bergeron bergeron dismissed gambinish’s stale review August 19, 2024 01:16

The base branch was changed.

@bergeron bergeron changed the title feat (migration): enable token auto-detection when basic functionality is on feat(migration): enable token auto-detection when basic functionality is on Aug 19, 2024
@Gudahtt Gudahtt mentioned this pull request Aug 22, 2024
Closed
14 tasks
@metamaskbot
Copy link
Collaborator

Builds ready [d0ef069]
Page Load Metrics (91 ± 24 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint672901164924
domContentLoaded47265874924
load53276915024
domInteractive167432178
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 1.01 KiB (0.03%)
  • ui: 0 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

@metamaskbot
Copy link
Collaborator

Builds ready [ef855fc]
Page Load Metrics (68 ± 5 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint7011793147
domContentLoaded448565136
load498568115
domInteractive9402494
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 1.01 KiB (0.03%)
  • ui: 0 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

@bergeron
Copy link
Contributor Author

bergeron commented Aug 28, 2024 •
edited
Loading

This should still go into 12.3
It may need a new migration number after merging 12.2 + being combined with 12.4
done

@metamaskbot
Copy link
Collaborator

Builds ready [d4a5206]
Page Load Metrics (1754 ± 91 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint14822102175518890
domContentLoaded14742092173318287
load14822104175418991
domInteractive1790392010
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 1.01 KiB (0.03%)
  • ui: 0 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

@sonarqubecloud SonarQubeCloud
Copy link

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

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

See analysis details on SonarCloud

@metamaskbot
Copy link
Collaborator

Builds ready [f4bdf42]
Page Load Metrics (1789 ± 114 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint22124871546560269
domContentLoaded152523771770223107
load153224851789238114
domInteractive146335147
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 1.01 KiB (0.03%)
  • ui: 0 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

@bergeron bergeron merged commit b305de8 into develop Sep 12, 2024
78 checks passed
@bergeron bergeron deleted the brian/migrate-token-detection-enable branch September 12, 2024 15:20
@github-actions github-actions bot locked and limited conversation to collaborators Sep 12, 2024
@metamaskbot metamaskbot added the release-12.6.0 Issue or pull request that will be included in release 12.6.0 label Sep 12, 2024
@metamaskbot metamaskbot added release-12.5.0 Issue or pull request that will be included in release 12.5.0 and removed release-12.6.0 Issue or pull request that will be included in release 12.6.0 labels Sep 29, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@salimtb salimtb salimtb approved these changes

@sahar-fehri sahar-fehri sahar-fehri approved these changes

@gambinish gambinish gambinish left review comments

Assignees
No one assigned
Labels
needs-assets-ux-review A shared label between the Assets and UX team to flag PRs ready for consolidated team review. release-12.5.0 Issue or pull request that will be included in release 12.5.0 team-assets
Projects
PR review queue
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@bergeron @metamaskbot @sahar-fehri @salimtb @gambinish