Skip to content

Stop throwing an error if verifyingContract field in EIP712 payloads is undefined or not a string#5595

Merged
adonesky1 merged 2 commits intomainfrom
ad/extension-31607/fix-eip712
Apr 4, 2025
Merged

Stop throwing an error if verifyingContract field in EIP712 payloads is undefined or not a string#5595
adonesky1 merged 2 commits intomainfrom
ad/extension-31607/fix-eip712

Conversation

@adonesky1
Copy link
Copy Markdown
Contributor

Explanation

It seems we've broken some dapps that rely on EIP712 / eth_signTypedData_v4 signatures which don't include verifyingContract as part of their payload.

References

See: MetaMask/metamask-extension#31607

For example:

  • Fixes #12345
  • Related to #67890
    -->

Changelog

Checklist

  • I've updated the test suite for new or updated code as appropriate
  • I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate
  • I've communicated my changes to consumers by updating changelogs for packages I've changed, highlighting breaking changes as necessary
  • I've prepared draft pull requests for clients and consumer packages to resolve any breaking changes

@adonesky1 adonesky1 requested a review from a team as a code owner April 4, 2025 16:31
Copy link
Copy Markdown
Member

@Gudahtt Gudahtt 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 changed the title allow verifyingContract field in EIP712 payloads to be undefined or falsy Stop throwing an error if verifyingContract field in EIP712 payloads is undefined or not a string Apr 4, 2025
@adonesky1 adonesky1 enabled auto-merge (squash) April 4, 2025 16:41
@adonesky1 adonesky1 merged commit 76651f0 into main Apr 4, 2025
197 checks passed
@adonesky1 adonesky1 deleted the ad/extension-31607/fix-eip712 branch April 4, 2025 16:45
@adonesky1 adonesky1 mentioned this pull request Apr 4, 2025
Gudahtt added a commit to MetaMask/metamask-extension that referenced this pull request Apr 4, 2025
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
adonesky1 added a commit that referenced this pull request Apr 4, 2025
## [27.1.0]

### Changed

- Bump `@metamask/controller-utils` to `^11.7.0`
([#5583](#5583))

### Fixed

- Stop throwing an error if `verifyingContract` field in EIP712 payloads
is undefined or not a string
([#5595](#5595))
github-merge-queue bot pushed a commit to MetaMask/metamask-extension that referenced this pull request Apr 4, 2025
…signatures (#31613)

## **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](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/31613?quickstart=1)

## **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**

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

### **Before**

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

### **After**

<!-- [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
- [ ] 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
- [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.
MajorLift pushed a commit to MetaMask/metamask-extension that referenced this pull request Apr 4, 2025
…signatures (#31613)

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](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/31613?quickstart=1)

Fixes #31607

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.

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

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

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

- [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
- [ ] 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
- [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.

- [ ] 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.
dbrans pushed a commit to MetaMask/metamask-extension that referenced this pull request Apr 7, 2025
…signatures (#31613)

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](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/31613?quickstart=1)

Fixes #31607

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.

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

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

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

- [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
- [ ] 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
- [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.

- [ ] 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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants