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: Remove 'Improved transactions requests' toggle #29695

Merged
merged 1 commit into from
Jan 16, 2025
Merged

Conversation

pedronfigueiredo
Copy link
Contributor

@pedronfigueiredo pedronfigueiredo commented Jan 14, 2025

Description

We recently enabled the transaction redesign for all users by default. We also migrated some existing e2e tests to corresponding e2e tests for redesigned flows.

This PR removes the toggle that allowed users to opt out of the redesigned confirmations. It also removes the e2e tests for old confirmation flows that are no longer visible in any scenario.

Open in GitHub Codespaces

Related issues

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

Manual testing steps

  1. Open experimental settings.
  2. The option toggle shouldn't be there.
  3. Create a new transaction confirmation
  4. It should be redesigned.

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.

@pedronfigueiredo pedronfigueiredo added the team-confirmations Push issues to confirmations team label Jan 14, 2025
@pedronfigueiredo pedronfigueiredo self-assigned this Jan 14, 2025
@pedronfigueiredo pedronfigueiredo requested review from a team as code owners January 14, 2025 14:12
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
Copy link
Collaborator

Builds ready [6919546]
Page Load Metrics (1739 ± 53 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint55819891669278133
domContentLoaded15341914170410550
load15471987173911153
domInteractive25200484120
backgroundConnect1282342010
firstReactRender17100442713
getState576192110
initialActions01000
loadScripts1155148212849345
setupStore65815168
uiStartup17682675200419392
Bundle size diffs [🚀 Bundle size reduced!]
  • background: -103.01 KiB (-1.77%)
  • ui: 79 Bytes (0.00%)
  • common: -358 Bytes (-0.00%)

@metamaskbot
Copy link
Collaborator

Builds ready [1d2f569]
Page Load Metrics (1584 ± 50 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint14611887159010450
domContentLoaded14431824155910149
load14561888158410550
domInteractive20108372311
backgroundConnect86627199
firstReactRender1588382613
getState585142110
initialActions01000
loadScripts1064136811648239
setupStore68414189
uiStartup16582198187116378
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 553 Bytes (0.01%)
  • ui: -4.86 KiB (-0.06%)
  • common: -475 Bytes (-0.01%)

@metamaskbot
Copy link
Collaborator

Builds ready [8e8b6c4]
Page Load Metrics (1594 ± 48 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint1450180615989947
domContentLoaded1440178515699546
load1453180715949948
domInteractive2110034188
backgroundConnect792262110
firstReactRender1596463015
getState44410115
initialActions040294
loadScripts1051134311757938
setupStore55412136
uiStartup16472191189716680
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 553 Bytes (0.01%)
  • ui: -1015 Bytes (-0.01%)
  • common: -475 Bytes (-0.01%)

}

function transformState(state: Record<string, unknown>) {
const preferencesControllerState = state?.PreferencesController as
Copy link
Member

Choose a reason for hiding this comment

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

Minor, is there any need for this line and the complex cast if we only reference preferences?

preferences &&
hasProperty(preferences, 'redesignedTransactionsEnabled')
) {
delete preferences.redesignedTransactionsEnabled;
Copy link
Member

Choose a reason for hiding this comment

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

Minor, is delete a no-op if the property doesn't exist so could we remove the if?

@pedronfigueiredo pedronfigueiredo added this pull request to the merge queue Jan 16, 2025
Merged via the queue into main with commit 66c994a Jan 16, 2025
80 checks passed
@pedronfigueiredo pedronfigueiredo deleted the pnf/3030 branch January 16, 2025 14:02
@github-actions github-actions bot locked and limited conversation to collaborators Jan 16, 2025
@metamaskbot metamaskbot added the release-12.11.1 Issue or pull request that will be included in release 12.11.1 label Jan 16, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
release-12.11.1 Issue or pull request that will be included in release 12.11.1 team-confirmations Push issues to confirmations team
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

6 participants