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: enable signatures redesign new users #24466

Merged
merged 5 commits into from
May 10, 2024

Conversation

cryptotavares
Copy link
Contributor

@cryptotavares cryptotavares commented May 9, 2024

Description

🎉 Initial release for signatures redesign 🎉

Users will be able to toggle into the new confirmation pages for personal and typed data signatures. New users will have it enabled by default.

Open in GitHub Codespaces

Related issues

Fixes: #24312

Manual testing steps

[Existing user:]

  1. Go to experimental settings
  2. Enable Improved signature requests
  3. Go to test-dapp and test both personal sign and typed data sign (v1, v3 and v4)

[New installation]

  1. Remove the wallet and its data
  2. Install it clean
  3. Go through onboarding
  4. Go to test-dapp and test both personal sign and typed data sign (v1, v3 and v4)

Screenshots/Recordings

Before After
Screenshot 2024-05-09 at 23 31 17 Screenshot 2024-05-09 at 23 32 57
Screenshot 2024-05-09 at 23 37 00 Screenshot 2024-05-09 at 23 36 05

Pre-merge author checklist

  • I’ve followed MetaMask Coding Standards.
  • I've completed the PR template to the best of my ability
  • 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.

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.

Copy link
Contributor

github-actions bot commented May 9, 2024

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.

@cryptotavares cryptotavares changed the title feat enable signatures redesign new users feat: enable signatures redesign new users May 9, 2024
@cryptotavares cryptotavares added the team-confirmations Push issues to confirmations team label May 9, 2024
@cryptotavares cryptotavares marked this pull request as ready for review May 9, 2024 18:18
@cryptotavares cryptotavares requested review from a team as code owners May 9, 2024 18:18
Comment on lines -509 to +515

if (process.env.ENABLE_CONFIRMATION_REDESIGN) {
SETTINGS_CONSTANTS.push({
{
tabMessage: (t) => t('experimental'),
sectionMessage: (t) => t('redesignedConfirmationsEnabledToggle'),
descriptionMessage: (t) => t('redesignedConfirmationsToggleDescription'),
route: `${EXPERIMENTAL_ROUTE}#redesigned-confirmations`,
icon: 'fas fa-flask',
});
}
},
];
Copy link
Contributor

Choose a reason for hiding this comment

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

[in and partially out of scope]

would be nice to move these above the snaps experimental settings. We should update the references as well. Looks like the snaps one has gotten out of order. With this change, we should have

  // experimental settingsRefs[0]
  {
    tabMessage: (t) => t('experimental'),
    sectionMessage: (t) => t('petnamesEnabledToggle'),
    descriptionMessage: (t) => t('petnamesEnabledToggleDescription'),
    route: `${EXPERIMENTAL_ROUTE}#nicknames`,
    icon: 'fas fa-flask',
  },
  // experimental settingsRefs[1]
  {
    tabMessage: (t) => t('experimental'),
    sectionMessage: (t) => t('notificationsFeatureToggle'),
    descriptionMessage: (t) => t('notificationsFeatureToggleDescription'),
    route: `${EXPERIMENTAL_ROUTE}#notifications`,
    icon: 'fas fa-flask',
  },
  // experimental settingsRefs[2]
  {
    tabMessage: (t) => t('experimental'),
    sectionMessage: (t) => t('redesignedConfirmationsEnabledToggle'),
    descriptionMessage: (t) => t('redesignedConfirmationsToggleDescription'),
    route: `${EXPERIMENTAL_ROUTE}#redesigned-confirmations`,
    icon: 'fas fa-flask',
  },
  ///: BEGIN:ONLY_INCLUDE_IF(keyring-snaps)
  // since this route is only included with keyring-snaps feature flag, this needs to be the last settingsRef for the experimental tab
  // experimental settingsRefs[3]
  {
    tabMessage: (t) => t('experimental'),
    sectionMessage: (t) => t('snaps'),
    descriptionMessage: (t) => t('addSnapAccountToggle'),
    route: `${EXPERIMENTAL_ROUTE}#snaps`,
    icon: 'fas fa-flask',
  },

then the refs in ui/pages/settings/experimental-tab/experimental-tab.component.js should be updated appropriately

digiwand
digiwand previously approved these changes May 9, 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.

Lgtm! minor comment above that can be handled in a separate PR
https://github.com/MetaMask/metamask-extension/pull/24466/files#r1595902839

We are moving the toggle from within
the redesign env var, as from now on
all users will be able to see the new
redesign toggle under experimental settings
Personal sign and typed sign
confirmations are no longer under
the redesign env var. From this
commit onwards, if a user has the
preferences toggle on, he will be
routed through the new redesign
signature pages.
By default, new users will have
the redesign toggle enabled
redesign env is not required
to be set up for signatures e2e
or unit tests
@cryptotavares cryptotavares force-pushed the feat/enable-signatures-redesign-new-users branch from 728b1d4 to 1108bf5 Compare May 9, 2024 22:38
@cryptotavares cryptotavares force-pushed the feat/enable-signatures-redesign-new-users branch from 1108bf5 to 689d58a Compare May 9, 2024 23:10
Copy link

codecov bot commented May 10, 2024

Codecov Report

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

Project coverage is 67.43%. Comparing base (6813ea0) to head (689d58a).
Report is 1 commits behind head on develop.

Files Patch % Lines
ui/pages/confirmations/utils/confirm.ts 50.00% 2 Missing ⚠️
app/scripts/metamask-controller.js 0.00% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff            @@
##           develop   #24466   +/-   ##
========================================
  Coverage    67.43%   67.43%           
========================================
  Files         1286     1286           
  Lines        50105    50101    -4     
  Branches     12995    12991    -4     
========================================
- Hits         33787    33785    -2     
+ Misses       16318    16316    -2     

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

@metamaskbot
Copy link
Collaborator

Builds ready [689d58a]
Page Load Metrics (793 ± 539 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint65140872512
domContentLoaded8261242
load5328687931122539
domInteractive8261242
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 77 Bytes (0.00%)
  • ui: 858 Bytes (0.01%)
  • common: -66 Bytes (-0.00%)

@cryptotavares cryptotavares merged commit ad6a387 into develop May 10, 2024
72 checks passed
@cryptotavares cryptotavares deleted the feat/enable-signatures-redesign-new-users branch May 10, 2024 08:25
@github-actions github-actions bot locked and limited conversation to collaborators May 10, 2024
@metamaskbot
Copy link
Collaborator

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

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
release-12.0.0 team-confirmations Push issues to confirmations team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feat: Remove ENABLE_CONFIRMATION_REDESIGN ENV variable from Signature Redesign related code
7 participants