Fix disabled 'eth_sign' event & add 'Request Disabled' event type by digiwand · Pull Request #18007 · MetaMask/metamask-extension

digiwand · 2023-03-06T19:55:15Z

Explanation

Fixes #17974

Fixes bug where 'Signature Approved' event was incorrectly called when 'eth_sign' rpc method is called while the method is disabled by advanced settings
Replaces 'Signature Approved' event with new 'Signature Failed' event when the disabled 'eth_sign' event is called (spec discussed here in Slack)

note:

sometimes, the 'Signature Failed' effect may be received before 'Signature Request', but the 'sent_at' time still records it being sent earlier.
clean-up code, including more use of constants, is applied to this follow-up PR Fix Sign-in With Ethereum (SIWE) metametric, add tests, and clean RPC method middleware event logic #18008

Screenshots/Screencaps

Before

After

Manual Testing Steps

Open Segment or open background console for check the network requests to Segment
Disable the functionality of eth_sign from MetaMask Advanced Settings
On another window, go to test dapp
Connect to test dapp
Click Eth Sign
See Signature Request and Request Denied events are fired. Signature Approval should no longer be fired.

Pre-merge author checklist

I've clearly explained:
- What problem this PR is solving
- How this problem was solved
- How reviewers can test my changes
Sufficient automated test coverage has been added

Pre-merge reviewer checklist

Manual testing (e.g. pull and build branch, run in browser, test code being changed)
PR is linked to the appropriate GitHub issue
IF this PR fixes a bug in the release milestone, add this PR to the release milestone

If further QA is required (e.g. new feature, complex testing steps, large refactor), add the Extension QA Board label.

In this case, a QA Engineer approval will be be required.

github-actions · 2023-03-06T19:55:30Z

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.

digiwand · 2023-03-07T16:48:18Z

moving back to draft. More discussions are being had to plan how we will want to handle this event

edit:
ready for review again. We removed the Signature Request event here as well 485e700

jpuri · 2023-03-08T03:46:56Z

+      method === MESSAGE_TYPE.ETH_SIGN &&
+      res.error?.code === errorCodes.rpc.methodNotFound;
+
+    const isDisabledRequest = isDisabledEthSignAdvancedSetting;


hey, reason for duplicating the variable ?

hey, I did this for readability down the line. hope it helps, unless it's just me - In that case, happy to change it back

jpuri

Great work 👍

metamaskbot · 2023-03-08T16:26:38Z

Builds ready [5aa2298]

builds: chrome, firefox
builds (beta): chrome, firefox
builds (flask): chrome, firefox
build viz: Build System
mv3: Background Module Init Stats
mv3: UI Init Stats
mv3: Module Load Stats
mv3: Bundle Size Stats
mv2: E2e Actions Stats
code coverage: Report
storybook: Storybook
typescript migration: Dashboard
all artifacts
bundle viz:
- background: 0, 1, 2, 3, 4
- common: 0, 1, 2, 3, 4, 5
- content-script: 0
- contentscript: contentscript
- disable-console: disable-console
- inpage: inpage
- policy-load: policy-load
- sentry-install: sentry-install
- ui: 0, 1, 10, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (1703 ± 72 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	108	155	127	13	6
		domContentLoaded	1457	1967	1667	141	68
		load	1457	1975	1703	150	72
		domInteractive	1457	1967	1667	141	68

Bundle size diffs [🚨 Warning! Bundle size has increased!]

background: 287 bytes
ui: 0 bytes
common: 36 bytes

codecov · 2023-03-08T16:27:00Z

Codecov Report

Merging #18007 (bf06192) into develop (d0417da) will increase coverage by 0.00%.
The diff coverage is 71.43%.

@@           Coverage Diff            @@
##           develop   #18007   +/-   ##
========================================
  Coverage    63.44%   63.44%           
========================================
  Files          903      903           
  Lines        35267    35281   +14     
  Branches      8920     8925    +5     
========================================
+ Hits         22373    22383   +10     
- Misses       12894    12898    +4

Impacted Files	Coverage Δ
shared/constants/metametrics.js	`100.00% <ø> (ø)`
...p/scripts/lib/createRPCMethodTrackingMiddleware.js	`81.25% <71.43%> (-1.68%)`	⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

blackdevelopa

LGTM

seaona

oh I am still seeing the Signature Requested and Signature Approved event sent to Segment, with eth_sign disabled and no Request Disabled event thrown.

I believe I have the last changes 🤔 : could you double check this behaviour to discard it's just me? 🙏 :

segment-eth-sign.mp4

digiwand · 2023-03-13T18:34:38Z

oof, great catch @seaona!

Derp.. when we decided to exclude Signature Request, I updated the code and moved the res.error?.code check up, except the response, res, doesn't receive the values at this point...

I'm reworking this PR now

metamaskbot · 2023-03-13T23:25:09Z

Builds ready [bf06192]

builds: chrome, firefox
builds (beta): chrome, firefox
builds (flask): chrome, firefox
build viz: Build System
mv3: Background Module Init Stats
mv3: UI Init Stats
mv3: Module Load Stats
mv3: Bundle Size Stats
mv2: E2e Actions Stats
code coverage: Report
storybook: Storybook
typescript migration: Dashboard
all artifacts
bundle viz:
- background: 0, 1, 2, 3, 4
- common: 0, 1, 2, 3, 4, 5
- content-script: 0
- contentscript: contentscript
- disable-console: disable-console
- inpage: inpage
- policy-load: policy-load
- sentry-install: sentry-install
- ui: 0, 1, 10, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (1625 ± 65 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	98	165	127	17	8
		domContentLoaded	1366	1966	1603	131	63
		load	1366	1966	1625	136	65
		domInteractive	1366	1966	1603	131	63

Bundle size diffs [🚨 Warning! Bundle size has increased!]

background: 291 bytes
ui: 0 bytes
common: 36 bytes

seaona · 2023-03-14T08:53:28Z

thank you @digiwand ! Now I can see that whenever we have eth_sign disabled, the events are fired:

Signature Requested
Signature Failed

eth_sign_disabled.mp4

When we have eth_sign enabled, I can see the correct requests too:

Signature Requested
Signature Approved or
Signature Rejected

digiwand added 2 commits March 7, 2023 02:39

Fix #17974 / add REQUEST_DISABLED event

4a1b635

RPCMiddleware: add eth_sign REQUEST_DISABLED test

424b49d

digiwand requested a review from a team as a code owner March 6, 2023 19:55

digiwand requested a review from hmalik88 March 6, 2023 19:55

Merge branch 'develop' into fix-disabled-eth-sign-event

42e22fe

digiwand mentioned this pull request Mar 6, 2023

Fix Sign-in With Ethereum (SIWE) metametric, add tests, and clean RPC method middleware event logic #18008

Merged

8 tasks

digiwand marked this pull request as draft March 7, 2023 16:43

digiwand added 2 commits March 8, 2023 02:32

don't call Signature Request for disabled eth_sign

485e700

clean: alphabetize

a204fce

digiwand marked this pull request as ready for review March 7, 2023 19:33

jpuri reviewed Mar 8, 2023

View reviewed changes

jpuri previously approved these changes Mar 8, 2023

View reviewed changes

clean: rn isDisabledRequest -> isDisabledRPCMethod

ecce0ac

digiwand dismissed jpuri’s stale review via ecce0ac March 8, 2023 13:47

digiwand requested a review from jpuri March 8, 2023 13:48

jpuri previously approved these changes Mar 8, 2023

View reviewed changes

fix test

810f613

digiwand dismissed jpuri’s stale review via 810f613 March 8, 2023 15:52

rpc middeware: add eth_sign comment

5aa2298

digiwand requested a review from jpuri March 8, 2023 15:53

jpuri previously approved these changes Mar 9, 2023

View reviewed changes

blackdevelopa previously approved these changes Mar 13, 2023

View reviewed changes

seaona requested changes Mar 13, 2023

View reviewed changes

fix eth_sign events and re-add Signature Request

467f6a9

digiwand dismissed blackdevelopa’s stale review via 467f6a9 March 13, 2023 19:17

digiwand dismissed jpuri’s stale review via 467f6a9 March 13, 2023 19:17

send 'Signature Failed' for disabled eth_sigh

bf06192

digiwand requested review from blackdevelopa, jpuri and seaona March 13, 2023 22:47

jpuri approved these changes Mar 13, 2023

View reviewed changes

seaona approved these changes Mar 14, 2023

View reviewed changes

digiwand merged commit 5468c2c into develop Mar 14, 2023

digiwand deleted the fix-disabled-eth-sign-event branch March 14, 2023 18:31

github-actions Bot locked and limited conversation to collaborators Mar 14, 2023

PeterYinusa added the rc-cherry-picked label Mar 30, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix disabled 'eth_sign' event & add 'Request Disabled' event type#18007

Fix disabled 'eth_sign' event & add 'Request Disabled' event type#18007
digiwand merged 10 commits intodevelopfrom
fix-disabled-eth-sign-event

digiwand commented Mar 6, 2023 •

edited

Loading

Uh oh!

github-actions Bot commented Mar 6, 2023

Uh oh!

digiwand commented Mar 7, 2023 •

edited

Loading

Uh oh!

jpuri Mar 8, 2023

Uh oh!

digiwand Mar 8, 2023

Uh oh!

jpuri left a comment

Uh oh!

metamaskbot commented Mar 8, 2023

Uh oh!

codecov Bot commented Mar 8, 2023 •

edited

Loading

Uh oh!

blackdevelopa left a comment

Uh oh!

seaona left a comment •

edited

Loading

Uh oh!

digiwand commented Mar 13, 2023

Uh oh!

metamaskbot commented Mar 13, 2023

Uh oh!

seaona commented Mar 14, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Conversation

digiwand commented Mar 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Explanation

Screenshots/Screencaps

Before

After

Manual Testing Steps

Pre-merge author checklist

Pre-merge reviewer checklist

Uh oh!

github-actions Bot commented Mar 6, 2023

Uh oh!

digiwand commented Mar 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jpuri Mar 8, 2023

Choose a reason for hiding this comment

Uh oh!

digiwand Mar 8, 2023

Choose a reason for hiding this comment

Uh oh!

jpuri left a comment

Choose a reason for hiding this comment

Uh oh!

metamaskbot commented Mar 8, 2023

Uh oh!

codecov Bot commented Mar 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

blackdevelopa left a comment

Choose a reason for hiding this comment

Uh oh!

seaona left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

digiwand commented Mar 13, 2023

Uh oh!

metamaskbot commented Mar 13, 2023

Uh oh!

seaona commented Mar 14, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

digiwand commented Mar 6, 2023 •

edited

Loading

digiwand commented Mar 7, 2023 •

edited

Loading

codecov Bot commented Mar 8, 2023 •

edited

Loading

seaona left a comment •

edited

Loading