OpenSea security provider metrics#17688
Conversation
|
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. |
Builds ready [46f8e6b]
Page Load Metrics (1360 ± 140 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
|
Changes look good to me, it will be nice to have a review by @brad-decker also. |
| action: 'Sign Request Reject', | ||
| type: msg.type, | ||
| ui_customizations: | ||
| msg.securityProviderResponse?.flagAsDangerous === 1 | ||
| ? ['flagged_as_malicious'] | ||
| : [], |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
@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).
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
@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!
|
A request from data team: we should use |
Also, can you plz ensure that appropriate tests are added for these code changes.
ee6f999 to
351db3f
Compare
Builds ready [1e28710]
Page Load Metrics (1507 ± 47 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
|
Some issues from QA:
provider-method-event.mp4
provider-signatures.mp4 |
There was a problem hiding this comment.
I don't think we need this call here, it should only work with personal-sign. Why do we need the isSIWEMessage?
There was a problem hiding this comment.
Oh, I thought it's not just for the personal-sign. I'll fix it now.
There was a problem hiding this comment.
@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.
Builds ready [cc55ac7]
Page Load Metrics (1636 ± 42 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
Builds ready [d83b223]
Page Load Metrics (1520 ± 29 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
d83b223 to
cbd0e22
Compare
Builds ready [cbd0e22]
Page Load Metrics (1707 ± 62 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
digiwand
left a comment
There was a problem hiding this comment.
if the fetch request from securityProviderCheck somehow fails, it will stop the dapp request. We will need to handle the error here
|
|
||
| properties.ui_customizations = | ||
| securityProviderResponse?.flagAsDangerous === 1 | ||
| ? ['flagged_as_malicious'] |
There was a problem hiding this comment.
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
| } catch (e) { | ||
| console.warn( | ||
| `createRPCMethodTrackingMiddleware: Error calling securityProviderRequest - ${e}`, | ||
| ); |
There was a problem hiding this comment.
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
49a2da2 to
e70fb73
Compare
Builds ready [e70fb73]
Page Load Metrics (1495 ± 33 ms)
|
Builds ready [8b3d3cf]
Page Load Metrics (2260 ± 62 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
| const paramsExamplePassword = req?.params?.[2]; | ||
|
|
||
| msgParams = { | ||
| ...paramsExamplePassword, |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
working with another engineer to apply a fix for this
Explanation
Added metrics for the
OpenSeasecurity provider.Created a new user profile property called
security_provider. This property is a type list and receives the valueopenseaif the user has enabled theOpenSeasecurity provider feature underSettings > Experimental.If the user turns off the security provider under
Settings > Experimental, then the user profile propertysecurity_provideris updated and theopenseavalue removed from it.Additionally there is a new property called
ui_customizationsadded toTransaction Approved,Transaction Rejected,Signature ApprovedandSignature Rejectedevents.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 unknownnullin all other casesby the transaction security provider.
Manual testing steps
In the
.metamaskrcset 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:
OpenSeasecurity provider underSettings > ExperimentalSEND LEGACY TRANSACTIONSEND EIP 1559 TRANSACTIONETH SIGNPERSONAL SIGNSIGN TYPED DATASIGN TYPED DATA V3SIGN TYPED DATA V4