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: Migrate users that currently have opensea enabled to have blockaid enabled #23460

Merged
merged 12 commits into from
Mar 22, 2024

Conversation

blackdevelopa
Copy link
Contributor

@blackdevelopa blackdevelopa commented Mar 13, 2024

Description

Open in GitHub Codespaces

With Blockaid successfully rolled to users by default and across multiple networks, we will be deprecating the earlier Opensea security alert feature and migrating users to Blockaid.

Related issues

Fixes: https://github.com/MetaMask/MetaMask-planning/issues/2133

Manual testing steps

  1. Go to Settings
  2. See that OpenSea + Blockaid is removed

Screenshots/Recordings

Before

Screen.Recording.2024-03-20.at.12.20.59.mov

After

Screen.Recording.2024-03-20.at.12.19.25.mov

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.

@blackdevelopa blackdevelopa requested a review from a team as a code owner March 13, 2024 14:49
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.

@blackdevelopa blackdevelopa changed the title Migrate users that currently have opensea enabled to have blockaid enabled [WIP] Migrate users that currently have opensea enabled to have blockaid enabled Mar 13, 2024
@blackdevelopa blackdevelopa added the team-confirmations-secure-ux DEPRECATED: please use "team-confirmations" label instead label Mar 13, 2024
@blackdevelopa blackdevelopa self-assigned this Mar 13, 2024
@blackdevelopa blackdevelopa force-pushed the 2133/deprecate_opensea_alert branch 2 times, most recently from af7bd92 to da97e45 Compare March 19, 2024 16:27
@blackdevelopa blackdevelopa changed the title [WIP] Migrate users that currently have opensea enabled to have blockaid enabled Migrate users that currently have opensea enabled to have blockaid enabled Mar 20, 2024
@blackdevelopa blackdevelopa changed the title Migrate users that currently have opensea enabled to have blockaid enabled fix: Migrate users that currently have opensea enabled to have blockaid enabled Mar 20, 2024
@metamaskbot
Copy link
Collaborator

Builds ready [94a52a8]
Page Load Metrics (1176 ± 521 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint732801425225
domContentLoaded108631199
load58277211761085521
domInteractive108631199
Bundle size diffs [🚀 Bundle size reduced!]
  • background: -1.11 KiB (-0.03%)
  • ui: -480 Bytes (-0.01%)
  • common: -227 Bytes (-0.00%)

@segun segun added team-confirmations-secure-ux-PR PRs from the confirmations team needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) labels Mar 20, 2024
segun
segun previously approved these changes Mar 20, 2024
Copy link
Contributor

@digiwand digiwand left a comment

Choose a reason for hiding this comment

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

based on the description, should we transfer the true states from transactionSecurityCheckEnabled to securityAlertsEnabled?

app/scripts/controllers/metametrics.js Outdated Show resolved Hide resolved
app/scripts/metamask-controller.js Show resolved Hide resolved
app/scripts/metamask-controller.js Show resolved Hide resolved
ui/pages/settings/security-tab/security-tab.component.js Outdated Show resolved Hide resolved
app/scripts/migrations/113.ts Outdated Show resolved Hide resolved
jpuri
jpuri previously approved these changes Mar 21, 2024
@blackdevelopa blackdevelopa dismissed stale reviews from jpuri and segun via b2e6911 March 21, 2024 10:43
@metamaskbot
Copy link
Collaborator

Builds ready [b2e6911]
Page Load Metrics (452 ± 359 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint65160992813
domContentLoaded95823178
load532152452748359
domInteractive95823178
Bundle size diffs [🚀 Bundle size reduced!]
  • background: -1.01 KiB (-0.03%)
  • ui: -480 Bytes (-0.01%)
  • common: -227 Bytes (-0.00%)

app/scripts/migrations/113.ts Outdated Show resolved Hide resolved
app/scripts/migrations/113.ts Outdated Show resolved Hide resolved
app/scripts/migrations/114.ts Outdated Show resolved Hide resolved
@pedronfigueiredo
Copy link
Contributor

@blackdevelopa made one comment, the rest LGTM!

@metamaskbot
Copy link
Collaborator

Builds ready [82517a0]
Page Load Metrics (719 ± 466 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint673181327235
domContentLoaded109528189
load542403719971466
domInteractive109528189
Bundle size diffs [🚀 Bundle size reduced!]
  • background: -2.3 KiB (-0.06%)
  • ui: -480 Bytes (-0.01%)
  • common: -227 Bytes (-0.00%)

Copy link

codecov bot commented Mar 22, 2024

Codecov Report

Attention: Patch coverage is 89.47368% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 68.59%. Comparing base (fca2302) to head (82517a0).
Report is 2 commits behind head on develop.

Files Patch % Lines
app/scripts/migrations/114.ts 88.24% 2 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop   #23460      +/-   ##
===========================================
+ Coverage    68.54%   68.59%   +0.06%     
===========================================
  Files         1163     1163              
  Lines        44170    44104      -66     
  Branches     11816    11803      -13     
===========================================
- Hits         30272    30252      -20     
+ Misses       13898    13852      -46     

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

@jpuri
Copy link
Contributor

jpuri commented Mar 22, 2024

There is also code related to displaying validation result on transaction pages that we would need to cleanup, but may be in separate PR.

@blackdevelopa blackdevelopa merged commit 9d88f80 into develop Mar 22, 2024
66 checks passed
@blackdevelopa blackdevelopa deleted the 2133/deprecate_opensea_alert branch March 22, 2024 18:50
@github-actions github-actions bot locked and limited conversation to collaborators Mar 22, 2024
@github-actions github-actions bot removed the needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) label Mar 22, 2024
@metamaskbot metamaskbot added release-11.14.1 Issue or pull request that will be included in release 11.14.1 release-11.14.0 Issue or pull request that will be included in release 11.14.0 and removed release-11.14.1 Issue or pull request that will be included in release 11.14.1 labels Mar 22, 2024
@metamaskbot
Copy link
Collaborator

Missing release label release-11.14.0 on PR. Adding release label release-11.14.0 on PR and removing other release labels(release-11.14.1), as PR was added to branch 11.14.0 when release was cut.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
release-11.14.0 Issue or pull request that will be included in release 11.14.0 team-confirmations-secure-ux DEPRECATED: please use "team-confirmations" label instead team-confirmations-secure-ux-PR PRs from the confirmations team
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

7 participants