Skip to content

fix: cp-12.15.2 Allow verifyingContract to be omitted from EIP-712 signatures#31613

Merged
adonesky1 merged 1 commit intomainfrom
allow-missing-verifying-contract
Apr 4, 2025
Merged

fix: cp-12.15.2 Allow verifyingContract to be omitted from EIP-712 signatures#31613
adonesky1 merged 1 commit intomainfrom
allow-missing-verifying-contract

Conversation

@Gudahtt
Copy link
Copy Markdown
Member

@Gudahtt Gudahtt commented Apr 4, 2025

Description

The signature controller has been patched with changes from MetaMask/core#5595 to allow the verifyingContract field to be missing from EIP-712 signatures. It is optional in the spec.

Open in GitHub Codespaces

Related issues

Fixes #31607

Manual testing steps

  1. Connect to the test-dapp
  2. Enter this URL:
https://metamask.github.io/test-dapp/request.html?method=eth_signTypedData_v4&params=[%22YOUR_ADDRESSS_HERE%22,{%22types%22:{%22EIP712Domain%22:[{%22name%22:%22name%22,%22type%22:%22string%22}],%22Mail%22:[{%22name%22:%22from%22,%22type%22:%22string%22}]},%22primaryType%22:%22Mail%22,%22domain%22:{%22name%22:%22foo%22},%22message%22:{%from%22:%22foo%22}}]

(replace YOUR_ADDRESS_HERE with a connected address)
3. Previous it would fail. On this branch it should correctly prompt a signature confirmation.

Screenshots/Recordings

Before

After

Pre-merge author checklist

  • I've followed MetaMask Contributor Docs and MetaMask Extension Coding Standards.
  • I've completed the PR template to the best of my ability
  • I’ve included tests if applicable
    • We can manually test this for now, I will follow-up with an E2E test in a later PR
  • I’ve documented my code using JSDoc format if applicable
  • I’ve applied the right labels on the PR (see labeling guidelines). Not required for external contributors.

Pre-merge reviewer checklist

  • I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed).
  • I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots.

The signature controller has been patched with changes from MetaMask/core#5595
to allow the `verifyingContract` field to be missing from EIP-712
signatures. It is optional in the spec.

Fixes #31607
@Gudahtt Gudahtt force-pushed the allow-missing-verifying-contract branch from bd4dfaa to 4460a5f Compare April 4, 2025 17:01
@Gudahtt Gudahtt changed the title fix: Allow verifyingContract to be omitted from EIP-712 signatures fix: cp-12.15.2 Allow verifyingContract to be omitted from EIP-712 signatures Apr 4, 2025
@Gudahtt Gudahtt marked this pull request as ready for review April 4, 2025 17:23
@metamaskbot
Copy link
Copy Markdown
Collaborator

Builds ready [4460a5f]
UI Startup Metrics (1215 ± 68 ms)
PlatformBuildTypePageMetricMean (ms)Min (ms)Max (ms)Std Dev (ms)P 75 (ms)P 95 (ms)
ChromeBrowserifyHomeuiStartup1215110613946812631329
load10709661243661175991
domContentLoaded10639631238671205988
domInteractive16136461528
firstPaint776751248416224992
backgroundConnect13613217910
firstReactRender18123541928
getState10429669
initialActions001001
loadScripts81168598967852936
setupStore7521278
WebpackHomeuiStartup21041706237516322212313
load16261317189913017201823
domContentLoaded16201313189613117161818
domInteractive171270121450
firstPaint169664006024076
backgroundConnect3111213223657
firstReactRender166533461026494
getState1131691689
initialActions315134
loadScripts16111311189213117111807
setupStore446303812898
FirefoxBrowserifyHomeuiStartup13741208189314214361728
load12311078174013412871551
domContentLoaded12311078174013412871551
domInteractive9638192278997
firstPaintNaNNaNNaNNaNNaNNaN
backgroundConnect24177392544
firstReactRender22194432228
getState7420279
initialActions001001
loadScripts12081052171513012611520
setupStore6421267
WebpackHomeuiStartup15561371225817815921976
load13431178202316813731746
domContentLoaded13421178202316813721745
domInteractive9640218279198
firstPaintNaNNaNNaNNaNNaNNaN
backgroundConnect26185272743
firstReactRender37296374051
getState9470878
initialActions102112
loadScripts13191158199816813511720
setupStore8528479
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 0 Bytes (0%)
  • ui: 0 Bytes (0%)
  • common: 23 Bytes (0%)

Copy link
Copy Markdown
Contributor

@adonesky1 adonesky1 left a comment

Choose a reason for hiding this comment

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

LGTM

@adonesky1 adonesky1 added this pull request to the merge queue Apr 4, 2025
Merged via the queue into main with commit a4179bc Apr 4, 2025
177 checks passed
@adonesky1 adonesky1 deleted the allow-missing-verifying-contract branch April 4, 2025 18:31
@github-actions github-actions bot locked and limited conversation to collaborators Apr 4, 2025
@metamaskbot metamaskbot added the release-12.17.0 Issue or pull request that will be included in release 12.17.0 label Apr 4, 2025
@vpintorico vpintorico added the team-confirmations Push issues to confirmations team label Apr 8, 2025
github-merge-queue bot pushed a commit that referenced this pull request Apr 10, 2025
…31630)

<!--
Please submit this PR as a draft initially.
Do not mark it as "Ready for review" until the template has been
completely filled out, and PR status checks have passed at least once.
-->

## **Description**
The typed signatures were broken when we don't pass a verifyingContract
value in the message. This was fixed, and now we add a spec to cover
this user flow.

PR with the fix:
#31613

[![Open in GitHub
Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/31630?quickstart=1)

## **Related issues**

Fixes:

## **Manual testing steps**

1. Check ci or run spec locally

## **Screenshots/Recordings**

<!-- If applicable, add screenshots and/or recordings to visualize the
before and after of your change. -->

### **Before**



https://github.com/user-attachments/assets/63ae3cdd-4d0c-49a0-9dfa-0dc0cec14749



### **After**



https://github.com/user-attachments/assets/7e4f8e55-3cd1-455c-badb-9ef1cb2fdfac



<!-- [screenshots/recordings] -->

## **Pre-merge author checklist**

- [x] I've followed [MetaMask Contributor
Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask
Extension Coding
Standards](https://github.com/MetaMask/metamask-extension/blob/main/.github/guidelines/CODING_GUIDELINES.md).
- [x] I've completed the PR template to the best of my ability
- [x] I’ve included tests if applicable
- [x] I’ve documented my code using [JSDoc](https://jsdoc.app/) format
if applicable
- [x] I’ve applied the right labels on the PR (see [labeling
guidelines](https://github.com/MetaMask/metamask-extension/blob/main/.github/guidelines/LABELING_GUIDELINES.md)).
Not required for external contributors.

## **Pre-merge reviewer checklist**

- [ ] I've manually tested the PR (e.g. pull and build branch, run the
app, test code being changed).
- [ ] I confirm that this PR addresses all acceptance criteria described
in the ticket it closes and includes the necessary testing evidence such
as recordings and or screenshots.
@vpintorico vpintorico added the release-12.15.2 Issue or pull request that will be included in release 12.15.2 label Apr 27, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

release-12.15.2 Issue or pull request that will be included in release 12.15.2 release-12.17.0 Issue or pull request that will be included in release 12.17.0 team-confirmations Push issues to confirmations team

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: Regression in MetaMask v12.15.0 - Non-conformance with EIP-712 for Typed Data Signatures

5 participants