OpenSea security provider metrics #17688

filipsekulic · 2023-02-09T15:00:02Z

Explanation

Fixes Wallet Tx Security Provider: add a new user property and a ui_customizations property #16262
Fixes Add metrics for transactions and signatures flagged as unknown for the Tx Security Provider feature #18150

Added metrics for the OpenSea security provider.

Created a new user profile property called security_provider. This property is a type list and receives the value opensea if the user has enabled the OpenSea security provider feature under Settings > Experimental.
If the user turns off the security provider under Settings > Experimental, then the user profile property security_provider is updated and the opensea value removed from it.

Additionally there is a new property called ui_customizations added to Transaction Approved, Transaction Rejected, Signature Approved and Signature Rejected events.
This property receives one of the following values:

['flagged_as_malicious'] when the transaction was flagged as malicious
['flagged_as_safety_unknown'] when the transaction was flagged as safety unknown
null in all other cases

by the transaction security provider.

Manual testing steps

In the .metamaskrc set the transaction security feature flag to 1 - TRANSACTION_SECURITY_PROVIDER=1.

Follow the procedure for testing the metrics event and check the response in the background console of the extension - background.html.

Cases to check:

enable/disable the OpenSea security provider under Settings > Experimental
SEND LEGACY TRANSACTION
SEND EIP 1559 TRANSACTION
ETH SIGN
PERSONAL SIGN
SIGN TYPED DATA
SIGN TYPED DATA V3
SIGN TYPED DATA V4

github-actions · 2023-02-09T15:00:16Z

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 · 2023-02-14T10:50:47Z

Builds ready [46f8e6b]

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, 11, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (1360 ± 140 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	93	141	112	12	6
		domContentLoaded	959	1798	1321	291	140
		load	959	1827	1360	292	140
		domInteractive	959	1798	1321	291	140

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

background: 1549 bytes
ui: 0 bytes
common: 38 bytes

jpuri · 2023-02-14T18:42:42Z

Changes look good to me, it will be nice to have a review by @brad-decker also.

brad-decker · 2023-02-14T22:55:43Z

app/scripts/lib/message-manager.js

+          action: 'Sign Request Reject',
          type: msg.type,
+          ui_customizations:
+            msg.securityProviderResponse?.flagAsDangerous === 1
+              ? ['flagged_as_malicious']
+              : [],


I believe the ui_customization should be added to the events in createRPCMethodTrackingMiddleware versus adding a new event and renaming events here and in the other message managers.

Hey @brad-decker,
Do you have a suggestion how it should look like? I mean, in the middleware you mentioned I need the information I get from the message - msg.securityProviderResponse?.flagAsDangerous.
Also, I have to mention the presence of the property that shares the same name as the one mentioned above - https://github.com/MetaMask/metamask-extension/blob/develop/app/scripts/lib/createRPCMethodTrackingMiddleware.js#L173.

You would add your value to the array already present. The events in these confirmations are not the ones we rely upon. Let me dig in a bit and see how you can get access to the value you need

@filipsekulic it looks to me you can pass securityProviderRequest to the createRPCMethodTrackingMiddleware instantiation in metamask controller. You will have access to data and method type on the request and can call the securityProviderRequest. Now its a little wasteful, but we don't add our internal id to the response of signatures so we can't retrieve it from this level we'll have to call it again.

@brad-decker I tried it, but without success. The reason is that the data being passed to the securityProviderRequest from createRPCMethodTrackingMiddleware (req) is not proper, so as the response in that case. The proper data being sent is shown in the attached image (personal-message-manager.js).

@filipsekulic look at the detectSIWE implementation. Its done in both places as well. You will need to formulate the data object that securityProviderCheck needs -- note that in perosnal-message-manager that includes the 'id' of the transaction but the id key isn't used.

@brad-decker I implemented the solution you suggested. Thanks! However, I could not fix the failing test - createRPCMethodTrackingMiddleware.test.js. I must be missing something. Hope you can help me. I would appreciate your help!

bschorchit · 2023-02-15T00:58:55Z

A request from data team: we should use security_providers (plural) instead of security_provider (singular) for the name of the user profile property. Could you make this small change, @filipsekulic ? Thank you!

Also, can you plz ensure that appropriate tests are added for these code changes.

metamaskbot · 2023-03-02T06:58:16Z

Builds ready [1e28710]

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, 11, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (1507 ± 47 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	154	208	177	17	8
		domContentLoaded	1315	1561	1443	61	29
		load	1315	1664	1507	98	47
		domInteractive	1315	1561	1443	61	29

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

background: 1405 bytes
ui: 0 bytes
common: 40 bytes

seaona · 2023-03-02T11:02:13Z

Some issues from QA:

When I load MM for the first time, I can see a background console error related to detectSIWE , saying TypeError: The first argument must be one of type string, Buffer, ArrayBuffer, Array, or Array-like Object. Received type undefined

Sometimes the event for Send/Send Legacy is not emitted.

provider-method-event.mp4

I haven't observed any metrics event related to the provider, when interacting with Signatures (any kind of signature)

provider-signatures.mp4

app/scripts/lib/personal-message-manager.js

app/scripts/lib/message-manager.js

app/scripts/lib/typed-message-manager.js

app/scripts/lib/createRPCMethodTrackingMiddleware.js

brad-decker · 2023-03-02T16:44:55Z

app/scripts/lib/createRPCMethodTrackingMiddleware.js

+        const from = req?.params?.[1];
+        const paramsExamplePassword = req?.params?.[2];
+
+        const { isSIWEMessage } = detectSIWE({ data });


I don't think we need this call here, it should only work with personal-sign. Why do we need the isSIWEMessage?

Oh, I thought it's not just for the personal-sign. I'll fix it now.

@brad-decker
You were right, there is no need for the isSIWEMessage... I don't know why I put it, thought it might was mandatory for all signatures, because at that moment I was looking into the request data being sent for the personal-sign. Suppose that was the case.
Thanks once again! Hope it's okay now.

metamaskbot · 2023-03-02T17:07:06Z

Builds ready [cc55ac7]

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, 11, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (1636 ± 42 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	104	160	118	13	6
		domContentLoaded	1448	1742	1594	74	36
		load	1448	1766	1636	87	42
		domInteractive	1448	1742	1594	74	36

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

background: 1026 bytes
ui: 0 bytes
common: 40 bytes

metamaskbot · 2023-03-02T18:27:28Z

Builds ready [d83b223]

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, 11, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (1520 ± 29 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	95	145	117	14	7
		domContentLoaded	1412	1619	1505	56	27
		load	1412	1702	1520	61	29
		domInteractive	1412	1619	1505	56	27

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

background: 970 bytes
ui: 0 bytes
common: 40 bytes

metamaskbot · 2023-03-02T22:42:21Z

Builds ready [cbd0e22]

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, 11, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (1707 ± 62 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	100	165	122	17	8
		domContentLoaded	1496	1978	1667	126	61
		load	1507	1978	1707	129	62
		domInteractive	1496	1978	1667	126	61

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

background: 970 bytes
ui: 0 bytes
common: 40 bytes

jpuri

👍

digiwand

if the fetch request from securityProviderCheck somehow fails, it will stop the dapp request. We will need to handle the error here

digiwand · 2023-03-08T18:08:56Z

app/scripts/lib/createRPCMethodTrackingMiddleware.js

+
+        properties.ui_customizations =
+          securityProviderResponse?.flagAsDangerous === 1
+            ? ['flagged_as_malicious']


we should add this to METAMETRICS_KEY_OPTIONS in constants/metametrics.js. We could update this here or I can update this in a follow-up PR: #18008

Sounds okay to me

app/scripts/lib/createRPCMethodTrackingMiddleware.js

digiwand · 2023-03-08T20:21:32Z

app/scripts/lib/createRPCMethodTrackingMiddleware.js

+        } catch (e) {
+          console.warn(
+            `createRPCMethodTrackingMiddleware: Error calling securityProviderRequest - ${e}`,
+          );


choosing console.warn here since we already log the error in securityProviderRequest (metamask-controller.js). maybe there are better ways to go about this. happy to hear other suggestions

to not block dapp rpc requests

metamaskbot · 2023-03-22T08:45:52Z

Builds ready [e70fb73]

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 (1495 ± 33 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	90	153	113	14	7
		domContentLoaded	1370	1631	1485	66	32
		load	1393	1647	1495	70	33
		domInteractive	1370	1631	1485	66	32

metamaskbot · 2023-03-22T18:37:38Z

Builds ready [8b3d3cf]

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 (2260 ± 62 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	141	213	162	17	8
		domContentLoaded	1952	2390	2245	119	57
		load	1973	2465	2260	130	62
		domInteractive	1952	2390	2245	119	57

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

background: 2004 bytes
ui: 0 bytes
common: 40 bytes

digiwand · 2023-04-14T17:34:11Z

app/scripts/lib/createRPCMethodTrackingMiddleware.js

+        const paramsExamplePassword = req?.params?.[2];
+
+        msgParams = {
+          ...paramsExamplePassword,


upon a second look today, I realized this is an issue. It can splay a string. Not sure how we missed this the first time

working with another engineer to apply a fix for this

filipsekulic self-assigned this Feb 9, 2023

filipsekulic mentioned this pull request Feb 9, 2023

[WIP] Security provider request metrics #16833

Closed

filipsekulic marked this pull request as ready for review February 14, 2023 15:14

filipsekulic requested a review from a team as a code owner February 14, 2023 15:14

filipsekulic requested a review from georgewrmarshall February 14, 2023 15:15

jpuri previously approved these changes Feb 14, 2023

View reviewed changes

brad-decker suggested changes Feb 14, 2023

View reviewed changes

filipsekulic force-pushed the open-sea-security-provider-metrics branch 2 times, most recently from ee6f999 to 351db3f Compare March 1, 2023 14:16

brad-decker suggested changes Mar 2, 2023

View reviewed changes

brad-decker previously approved these changes Mar 2, 2023

View reviewed changes

filipsekulic dismissed brad-decker’s stale review via cbd0e22 March 2, 2023 22:11

filipsekulic force-pushed the open-sea-security-provider-metrics branch from d83b223 to cbd0e22 Compare March 2, 2023 22:11

filipsekulic requested review from brad-decker and jpuri March 2, 2023 22:42

jpuri previously approved these changes Mar 3, 2023

View reviewed changes

digiwand requested changes Mar 8, 2023

View reviewed changes

digiwand dismissed jpuri’s stale review via 4288143 March 8, 2023 20:07

digiwand reviewed Mar 8, 2023

View reviewed changes

filipsekulic and others added 18 commits March 22, 2023 09:19

Lint fixed

9c5ddf5

Removed unnecessary code

4529087

Fix build

5282ff7

Fix e2e

fa4fc14

Cleanup

38ace4a

Fix e2e

d17b356

Code refactor

f372b82

Removed unnecessary code

58f87db

rpc middleware: catch securityProviderCheck errors

dbdd848

to not block dapp rpc requests

Fixed an issue

b02df04

Added aditional test

d559e14

Applied some changes

ac5932a

Fixed a test

5e23961

Fixed a test

40d85ec

Code refactor

fdc6924

Covered more code with tests

ef4e9fb

Updated a test

6bbb49a

Fixed an issue

e70fb73

filipsekulic force-pushed the open-sea-security-provider-metrics branch from 49a2da2 to e70fb73 Compare March 22, 2023 08:19

brad-decker approved these changes Mar 22, 2023

View reviewed changes

Merge branch 'develop' into open-sea-security-provider-metrics

8b3d3cf

filipsekulic requested a review from digiwand March 23, 2023 11:39

jpuri approved these changes Mar 23, 2023

View reviewed changes

bschorchit merged commit 0351309 into develop Mar 23, 2023

bschorchit deleted the open-sea-security-provider-metrics branch March 23, 2023 17:01

github-actions bot locked and limited conversation to collaborators Mar 23, 2023

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

digiwand reviewed Apr 14, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OpenSea security provider metrics #17688

OpenSea security provider metrics #17688

filipsekulic commented Feb 9, 2023 •

edited

Loading

github-actions bot commented Feb 9, 2023

metamaskbot commented Feb 14, 2023

jpuri commented Feb 14, 2023 •

edited

Loading

brad-decker Feb 14, 2023

filipsekulic Feb 20, 2023 •

edited

Loading

brad-decker Feb 21, 2023

brad-decker Feb 23, 2023

filipsekulic Feb 24, 2023

brad-decker Feb 24, 2023

filipsekulic Mar 1, 2023

bschorchit commented Feb 15, 2023

metamaskbot commented Mar 2, 2023

seaona commented Mar 2, 2023

brad-decker Mar 2, 2023

filipsekulic Mar 2, 2023

filipsekulic Mar 2, 2023

metamaskbot commented Mar 2, 2023

metamaskbot commented Mar 2, 2023

metamaskbot commented Mar 2, 2023

jpuri left a comment

digiwand left a comment

digiwand Mar 8, 2023 •

edited

Loading

filipsekulic Mar 9, 2023 •

edited

Loading

digiwand Mar 8, 2023

metamaskbot commented Mar 22, 2023

metamaskbot commented Mar 22, 2023

digiwand Apr 14, 2023

digiwand Apr 14, 2023

OpenSea security provider metrics #17688

OpenSea security provider metrics #17688

Conversation

filipsekulic commented Feb 9, 2023 • edited Loading

Explanation

Manual testing steps

github-actions bot commented Feb 9, 2023

metamaskbot commented Feb 14, 2023

jpuri commented Feb 14, 2023 • edited Loading

Choose a reason for hiding this comment

filipsekulic Feb 20, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bschorchit commented Feb 15, 2023

metamaskbot commented Mar 2, 2023

seaona commented Mar 2, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

metamaskbot commented Mar 2, 2023

metamaskbot commented Mar 2, 2023

metamaskbot commented Mar 2, 2023

jpuri left a comment

Choose a reason for hiding this comment

digiwand left a comment

Choose a reason for hiding this comment

digiwand Mar 8, 2023 • edited Loading

Choose a reason for hiding this comment

filipsekulic Mar 9, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

metamaskbot commented Mar 22, 2023

metamaskbot commented Mar 22, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

filipsekulic commented Feb 9, 2023 •

edited

Loading

jpuri commented Feb 14, 2023 •

edited

Loading

filipsekulic Feb 20, 2023 •

edited

Loading

digiwand Mar 8, 2023 •

edited

Loading

filipsekulic Mar 9, 2023 •

edited

Loading