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: Add eth_sign friction #6378

Merged
merged 60 commits into from
Jun 15, 2023
Merged

Conversation

NicolasMassart
Copy link
Contributor

@NicolasMassart NicolasMassart commented May 12, 2023

Development & PR Process

  1. Follow MetaMask Mobile Coding Standards
  2. Add release-xx label to identify the PR slated for a upcoming release (will be used in release discussion)
  3. Add needs-dev-review label when work is completed
  4. Add needs-qa label when dev review is completed
  5. Add QA Passed label when QA has signed off

Description

1. What is the reason for the change?

Add friction when enabling the eth_sign RPC endpoint

2. What is the improvement/solution?

  • Update the setting switch to clarify the risk
  • Add a bottom sheet that pops when attempting to switch the value to on
  • have a two steps friction on the bottom sheet: 1 - checkbox, 2 - text field to fill (case insensitive, like for "delete wallet" and prevent pasting)
  • Enable setting only after all friction steps completed
  • add tracking events for analytics

QA builds

Other QA Artifacts are available on Bitrise Pipeline build #8122

Screenshots/Recordings

Light / Dark modes (on iOS)

Mode Advanced setting (off) CheckBox step TextInput step (matching) Advanced setting (on)
Light Mode Light mode Advanced setting (off) Light mode Checkbox step Light mode Text field step Light mode Advanced setting (on)
Dark Mode Dark Mode Advanced setting (off) Dark Mode Checkbox step Dark Mode Text field step Mode Text Advanced setting (on)

Screen recording (Light mode on iOS)

Simulator.Screen.Recording.-.iPhone.12.Pro.-.2023-05-30.at.17.37.07.mp4

Metrics

metrics event log look like this:

 LOG  [MetaMask DEBUG]: Analytics 'trackEvent' - {"category": "Settings Viewed", "properties": {"action": "eth_sign_checkbox_seen"}} {} undefined undefined
 LOG  [MetaMask DEBUG]: Analytics 'trackEvent' - {"category": "Settings Viewed", "properties": {"action": "eth_sign_input_seen"}} {} undefined undefined
 LOG  [MetaMask DEBUG]: Analytics 'trackEvent' - {"category": "Settings Updated", "properties": {"action": "eth_sign_enabled"}} {} undefined undefined
 LOG  [MetaMask DEBUG]: Analytics 'trackEvent' - {"category": "Settings Updated", "properties": {"action": "eth_sign_disabled"}} {} undefined undefined

Lines meaning:

  1. switch was off and I clicked on i, popping the bottom sheet, viewing the checkbox
  2. clicked on continue and moved to second sheet step with text input
  3. clicked enabled, it sets the eth_sign setting value to true and closes the bottom sheet
  4. then clicked switch off again in the advanced settings without the bottom sheet shown.

looks like the following on MixPanel:
image
image

Issue

fixes MetaMask/mobile-planning#918

Checklist

  • There is a related GitHub issue
  • Tests are included if applicable
  • Any added code is fully documented

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

@NicolasMassart NicolasMassart force-pushed the 918_add_eth_sign_setting_friction branch from d391121 to b4a0ad6 Compare May 15, 2023 18:18
NicolasMassart and others added 5 commits May 15, 2023 21:15
@MetaMask MetaMask deleted a comment from socket-security bot May 23, 2023
@socket-security
Copy link

socket-security bot commented May 23, 2023

No dependency changes detected. Learn more about Socket for GitHub ↗︎

👍 No dependency changes detected in pull request

@NicolasMassart NicolasMassart force-pushed the 918_add_eth_sign_setting_friction branch from fa62c0a to 5c4fe81 Compare May 23, 2023 12:06
@NicolasMassart NicolasMassart force-pushed the 918_add_eth_sign_setting_friction branch from 7d05535 to 467ebd1 Compare May 24, 2023 13:44
@NicolasMassart NicolasMassart force-pushed the 918_add_eth_sign_setting_friction branch from 467ebd1 to fd7446e Compare May 24, 2023 13:44
@NicolasMassart NicolasMassart changed the title Add eth_sign friction Feature: Add eth_sign friction May 30, 2023
@NicolasMassart NicolasMassart changed the title Feature: Add eth_sign friction Feat: Add eth_sign friction May 30, 2023
fail as the readMore is not implemeted yet
@NicolasMassart NicolasMassart requested a review from Cal-L June 6, 2023 12:04
Cal-L
Cal-L previously approved these changes Jun 7, 2023
Copy link
Contributor

@Cal-L Cal-L left a comment

Choose a reason for hiding this comment

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

LGTM

@Cal-L Cal-L added needs-qa Any New Features that needs a full manual QA prior to being added to a release. team-mobile-client and removed needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) labels Jun 7, 2023
# Conflicts:
#	app/components/Views/Settings/AdvancedSettings/index.js
…sk/metamask-mobile into 918_add_eth_sign_setting_friction
Copy link
Contributor

@Cal-L Cal-L left a comment

Choose a reason for hiding this comment

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

Lgtm

@seaona
Copy link
Contributor

seaona commented Jun 15, 2023

A couple of findings/comments from QA, the rest looks good 💯

Sending ENTER twice triggers other functionalities (i.e. Reset Account function, IPFS Gateway..)

Whenever I input the text and I submit ENTER key, I see that the the first one validates the text, but the second one, instead of Enabling the function it triggers another action, depending on where I am on the background screen. This could be Reset Account, Show Hex Data, IPFS Gateway etc..

eth_sign_enter.mp4

Enable button is not enabled unless I submit the text

This I am not sure if it's an issue, but commenting just in case. Whenever I enter the correct text, I can see how the button is not enabled, unless I submit (ENTER)

eth-sign-friction.mp4

Bold text

I can see how the ON/OFF text is not in bold in Android. Not sure if this is expected

image

@NicolasMassart
Copy link
Contributor Author

thanks @seaona, I will investigate the first issue "Sending ENTER twice triggers".

For the second one, I'm going to confirm with design team what the expected behaviour is as this is what I understood from their Figma design (link in the issue)

For the third one, it doesn't look very bold but it is 😅 this is the sBodyLGMedium style as indicated in the design.
image

@seaona
Copy link
Contributor

seaona commented Jun 15, 2023

thank you for your comments @NicolasMassart @hesterbruikman ! A summary of our discussions and outcomes:

Given all that, PR looks good from QA side

@seaona seaona added QA Passed A successful QA run through has been done and removed needs-qa Any New Features that needs a full manual QA prior to being added to a release. labels Jun 15, 2023
@sonarcloud
Copy link

sonarcloud bot commented Jun 15, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 1 Code Smell

87.2% 87.2% Coverage
0.0% 0.0% Duplication

@NicolasMassart NicolasMassart merged commit ea43840 into main Jun 15, 2023
11 checks passed
@NicolasMassart NicolasMassart deleted the 918_add_eth_sign_setting_friction branch June 15, 2023 17:22
@github-actions github-actions bot locked and limited conversation to collaborators Jun 15, 2023
@gauthierpetetin gauthierpetetin added the team-mobile-ux DEPRECATED: please use "team-wallet-ux" label instead label Feb 2, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
QA Passed A successful QA run through has been done release-7.2.0 team-mobile-ux DEPRECATED: please use "team-wallet-ux" label instead
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants