Skip to content

Detect and track UI customizations on Personal Sign Requests#16222

Merged
digiwand merged 29 commits intoMetaMask:developfrom
spruceid:feat/add-siwe-event
Feb 8, 2023
Merged

Detect and track UI customizations on Personal Sign Requests#16222
digiwand merged 29 commits intoMetaMask:developfrom
spruceid:feat/add-siwe-event

Conversation

@skgbafa
Copy link
Copy Markdown
Contributor

@skgbafa skgbafa commented Oct 17, 2022 •
edited
Loading

Explanation

This PR addresses Issue 15155 and detects if the SIWE view was enabled for the personal sign message.

More Information

Below are examples of the updated event data of a personal sign messages that do and don't conform to SIWE

SIWE Personal Sign

// requested
{"event":"Signature Requested","category":"inpage_provider","referrer":{"url":"https://spruceid.github.io"},"properties":{"signature_type":"personal_sign","ui_customizations":["sign_in_with_ethereum"]}}

// approved
{"event":"Signature Approved","category":"inpage_provider","referrer":{"url":"https://spruceid.github.io"},"properties":{"signature_type":"personal_sign","ui_customizations":["sign_in_with_ethereum"]}}

// rejected
{"event":"Signature Rejected","category":"inpage_provider","referrer":{"url":"https://spruceid.github.io"},"properties":{"signature_type":"personal_sign","ui_customizations":["sign_in_with_ethereum"]}}

Non-SIWE Personal Sign

// requested
{"event":"Signature Requested","category":"inpage_provider","referrer":{"url":"https://spruceid.github.io"},"properties":{"signature_type":"personal_sign"}}

// approved
{"event":"Signature Approved","category":"inpage_provider","referrer":{"url":"https://spruceid.github.io"},"properties":{"signature_type":"personal_sign"}}

// rejected
{"event":"Signature Rejected","category":"inpage_provider","referrer":{"url":"https://spruceid.github.io"},"properties":{"signature_type":"personal_sign"}}

Note: In testing the non-siwe personal sign, subsequent legacy metric calls were found, for approve and reject

{"category":"Transactions","event":"Cancel","properties":{"action":"Sign Request","legacy_event":true,"type":"personal_sign"},"environmentType":"notification","page":{"path":"/confirm-transaction/:id/signature-request","title":"Signature Request Page","url":"/confirm-transaction/:id/signature-request"}}

Manual Testing Steps

  1. Turn on the feature flag by adding SIWE_V1=1 to metamask-extension/.metamaskrc
  2. Build and run this version of the app.
  3. Go to Metamask's Test Dapp and scroll down to the Sign-In with Ethereum section. (If the dapp doesn't have this Add Sign-in with Ethereum Test Cases test-dapp#164, use this version).
  4. Test the 6 tests cases: (SIWE/non-SIWE on Request/Approve/Reject)
    i. A normal SIWE message (Request/Approve/Reject)
    ii. Personal Sign (Request/Approve/Reject)

@skgbafa skgbafa requested a review from a team as a code owner October 17, 2022 18:35
@skgbafa skgbafa requested a review from ryanml October 17, 2022 18:35
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Oct 17, 2022

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.

@skgbafa skgbafa changed the title detect and track ui customizations on personal sign requests Detect and track UI customizations on personal sign requests Oct 17, 2022
@skgbafa skgbafa changed the title Detect and track UI customizations on personal sign requests Detect and track UI customizations on Personal Sign Requests Oct 17, 2022
brad-decker
brad-decker previously approved these changes Oct 19, 2022
View reviewed changes
jpuri
jpuri previously approved these changes Oct 25, 2022
View reviewed changes
@brad-decker
Copy link
Copy Markdown
Contributor

brad-decker commented Nov 2, 2022

@skgbafa Could you rebase and push this again and i'll approve after tests pass?

@skgbafa skgbafa requested review from brad-decker and removed request for ryanml November 11, 2022 16:18
brad-decker
brad-decker previously approved these changes Nov 14, 2022
View reviewed changes
@brad-decker
Copy link
Copy Markdown
Contributor

brad-decker commented Nov 14, 2022

looks like the unit test failure is legit. Could you check it out @skgbafa

@skgbafa skgbafa requested review from brad-decker and jpuri and removed request for brad-decker and jpuri November 14, 2022 18:41
@jpuri
Copy link
Copy Markdown
Contributor

jpuri commented Jan 13, 2023

The issue here #15155 (comment)

includes signature_type: "eth_signTypedData_v4" also for SIWE

digiwand
digiwand reviewed Jan 13, 2023
View reviewed changes
Comment thread app/scripts/lib/createRPCMethodTrackingMiddleware.js Outdated
Comment thread app/scripts/lib/createRPCMethodTrackingMiddleware.js Outdated
Comment thread app/scripts/lib/createRPCMethodTrackingMiddleware.js Outdated
@bschorchit bschorchit mentioned this pull request Jan 27, 2023
Closed
2 tasks
digiwand
digiwand reviewed Feb 6, 2023
View reviewed changes
Comment thread app/scripts/lib/createRPCMethodTrackingMiddleware.js Outdated
digiwand
digiwand reviewed Feb 6, 2023
View reviewed changes
Comment thread app/scripts/lib/createRPCMethodTrackingMiddleware.js
METAMETRIC_KEY_OPTIONS[METAMETRIC_KEY.UI_CUSTOMIZATIONS].SIWE,
];
}
}
Copy link
Copy Markdown
Contributor

@digiwand digiwand Feb 6, 2023

Choose a reason for hiding this comment

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

thinking we could abstract quite a few lines from the custom middleware and next callbacks such that the logic could be easier to read and manage, but this isn't a blocker

Copy link
Copy Markdown
Contributor

@jpuri jpuri Feb 8, 2023

Choose a reason for hiding this comment

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

Yep above 2 blocks of code are exactly similar.

digiwand
digiwand previously approved these changes Feb 6, 2023
View reviewed changes
skgbafa and others added 2 commits February 6, 2023 17:02
@skgbafa
Merge branch 'develop' into feat/add-siwe-event
e59c61f
@skgbafa @digiwand
Apply suggestions from code review
8ff0b08 
Co-authored-by: Ariella Vu <20778143+digiwand@users.noreply.github.com>
@digiwand digiwand merged commit 53205b6 into MetaMask:develop Feb 8, 2023
@digiwand digiwand deleted the feat/add-siwe-event branch February 8, 2023 15:16
@github-actions github-actions Bot locked and limited conversation to collaborators Feb 8, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@jpuri jpuri jpuri approved these changes

@brad-decker brad-decker Awaiting requested review from brad-decker

+1 more reviewer

@digiwand digiwand digiwand approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@skgbafa @brad-decker @jpuri @digiwand