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: Migration #122 set redesignedConfirmationsEnabled to true #25769

Merged
merged 22 commits into from
Jul 23, 2024

Conversation

digiwand
Copy link
Contributor

@digiwand digiwand commented Jul 11, 2024

Description

This migration sets redesignedConfirmationsEnabled to true. Some users may have explicitly turned off the experimental setting, which this migration will reset to true. This is intentional as we also plan to remove the setting in an upcoming release.

I also added the redesigned confirmation prop to the Sentry state log. I had to decide whether to add it to support the setting in the tests, so I went with adding it.

This PR also partially fixes the Sign Typed Data flaky test when testing the verified result.


Getting the tests to pass were a bit tricky. It turns out the migrations run after the fixtures are set. The withPreferencesController fixture method is no help here.

One way we discussed to set the desired test state is to set the previous migration data to the state and setting the fixture migration version to the current version:

meta: { version: 122 }

This would require opening a live version, extracting the latest migration state, and adding the mock state to the tests.

Instead, we manually toggle the setting off for each test that requires the old signature pages.

Open in GitHub Codespaces

Related issues

Fixes: #24614
following Cherry-pick PR → #26046

Manual testing steps

  1. Turn off the Experimental > Improved signature redesign setting
  2. Run newest version with migration
  3. Observe setting has been turned on

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.

@digiwand digiwand requested a review from a team as a code owner July 11, 2024 13:59
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.

@metamaskbot metamaskbot added the team-confirmations Push issues to confirmations team label Jul 11, 2024
@digiwand
Copy link
Contributor Author

duplication code only applies to migrations 🤔 unsure how we usually go about this. should sonarcloud include these files?

@metamaskbot
Copy link
Collaborator

Builds ready [dca23ea]
Page Load Metrics (75 ± 11 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint66146101199
domContentLoaded98627189
load44131752311
domInteractive98627189
Bundle size diffs
  • background: 0 Bytes (0.00%)
  • ui: 0 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

sleepytanya
sleepytanya previously approved these changes Jul 11, 2024
@jpuri
Copy link
Contributor

jpuri commented Jul 12, 2024

Hey @digiwand : changes look good, can you plz ensure that this does not makes contract interaction visible to users.

@digiwand digiwand changed the title feat: Migration #122 set useTransactionSimulations to true feat: Migration #122 set redesignedConfirmationsEnabled to true Jul 12, 2024
@pedronfigueiredo
Copy link
Contributor

Gentle reminder to import the migration in app/scripts/migrations/index.js

@digiwand
Copy link
Contributor Author

digiwand commented Jul 12, 2024

Thanks @pedronfigueiredo! 🙏🏼 updated

@digiwand
Copy link
Contributor Author

hi @jpuri, lgtm! old contract interaction continues to show. I believe the contract interaction redesign is controlled by the ENV var ENABLE_CONFIRMATION_REDESIGN

app/scripts/migrations/122.ts Outdated Show resolved Hide resolved
app/scripts/migrations/122.ts Show resolved Hide resolved
app/scripts/migrations/122.ts Outdated Show resolved Hide resolved
app/scripts/migrations/122.ts Outdated Show resolved Hide resolved
app/scripts/migrations/122.ts Show resolved Hide resolved
app/scripts/migrations/122.test.ts Outdated Show resolved Hide resolved
matthewwalsh0
matthewwalsh0 previously approved these changes Jul 17, 2024
@digiwand
Copy link
Contributor Author

had to resolve a merge conflict. Rereviews please 🙏🏼 @matthewwalsh0 @pedronfigueiredo

matthewwalsh0
matthewwalsh0 previously approved these changes Jul 17, 2024
Copy link

codecov bot commented Jul 22, 2024

Codecov Report

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

Project coverage is 69.68%. Comparing base (721a38b) to head (cbe032a).

Files Patch % Lines
app/scripts/migrations/122.ts 86.67% 2 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop   #25769      +/-   ##
===========================================
+ Coverage    69.67%   69.68%   +0.01%     
===========================================
  Files         1402     1403       +1     
  Lines        49652    49667      +15     
  Branches     13720    13723       +3     
===========================================
+ Hits         34594    34607      +13     
- Misses       15058    15060       +2     

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

greptile-apps[bot]

This comment was marked as duplicate.

greptile-apps[bot]

This comment was marked as duplicate.

greptile-apps[bot]

This comment was marked as duplicate.

greptile-apps[bot]

This comment was marked as duplicate.

@metamaskbot
Copy link
Collaborator

Builds ready [a8b9686]
Page Load Metrics (79 ± 11 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint771711122411
domContentLoaded98134199
load44126792311
domInteractive98034199
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 1.15 KiB (0.03%)
  • ui: 70 Bytes (0.00%)
  • common: 34 Bytes (0.00%)

test/e2e/helpers.js Outdated Show resolved Hide resolved
Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

PR Summary

(updates since last review)

The pull request introduces migration #122 to enable redesigned confirmations for all users by setting redesignedConfirmationsEnabled to true.

  • Helper Function Addition: Added tempToggleSettingRedesignedConfirmations in test/e2e/helpers.js to manually toggle the redesignedConfirmationsEnabled setting.
  • Test Integrity: Ensures tests requiring the old signature pages can manually toggle the setting off, maintaining test integrity during the transition.
  • Migration Script: Added app/scripts/migrations/122.ts to set redesignedConfirmationsEnabled to true.
  • Test Updates: Added app/scripts/migrations/122.test.ts to verify migration logic.
  • Migration Index: Updated app/scripts/migrations/index.js to include the new migration script.

1 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings

Copy link

sonarcloud bot commented Jul 23, 2024

@metamaskbot
Copy link
Collaborator

Builds ready [b86b30b]
Page Load Metrics (155 ± 181 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint733041125024
domContentLoaded9106302411
load411798155378181
domInteractive9106302411
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 1.15 KiB (0.03%)
  • ui: 70 Bytes (0.00%)
  • common: 34 Bytes (0.00%)

@digiwand digiwand merged commit 3ec4b1e into develop Jul 23, 2024
82 of 83 checks passed
@digiwand digiwand deleted the feat-migration-122-redesignedConfirmationsEnabled branch July 23, 2024 11:46
@github-actions github-actions bot locked and limited conversation to collaborators Jul 23, 2024
@metamaskbot metamaskbot added the release-12.3.0 Issue or pull request that will be included in release 12.3.0 label Jul 23, 2024
@digiwand digiwand added release-12.2.0 Issue or pull request that will be included in release 12.2.0 and removed release-12.3.0 Issue or pull request that will be included in release 12.3.0 labels Jul 23, 2024
@@ -306,6 +313,8 @@ describe('Sign Typed Data Signature Request', function () {

// switch to the Dapp and verify the rejection was successful
await driver.switchToWindowWithTitle('E2E Test Dapp');

await driver.waitForSelector(data.verifyRejectionResultId);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this seemed to have fixed some "Sign Typed Data" flaky tests I ran into

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
release-12.2.0 Issue or pull request that will be included in release 12.2.0 team-confirmations Push issues to confirmations team
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Enable redesigned signatures for all users.
9 participants