Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Adding patch on eth-json-rpc-middleware to disable verifyContract field validation for cosmos #27021

Merged
merged 11 commits into from
Sep 11, 2024

Conversation

jpuri
Copy link
Contributor

@jpuri jpuri commented Sep 10, 2024

Description

Adding patch on eth-json-rpc-middleware to disable verifyContract field validation for cosmos

Related issues

Fixes: #26980

Manual testing steps

  1. Submit a types signature request with verifyingContract set to cosmos
  2. Ensure that you are able to sign it

Screenshots/Recordings

Screenshot 2024-09-10 at 4 08 00 PM

Pre-merge author checklist

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.

Copy link
Contributor

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.

@github-actions github-actions bot added the team-confirmations Push issues to confirmations team label Sep 10, 2024
Copy link

socket-security bot commented Sep 10, 2024

New and removed dependencies detected. Learn more about Socket for GitHub ↗︎

Package New capabilities Transitives Size Publisher
npm/@metamask/[email protected] None 0 64.5 kB lgbot
npm/@metamask/[email protected] None 0 75.8 kB metamaskbot

🚮 Removed packages: npm/@metamask/[email protected], npm/@metamask/[email protected], npm/@metamask/[email protected], npm/[email protected]

View full report↗︎

@jpuri
Copy link
Contributor Author

jpuri commented Sep 10, 2024

@metamaskbot update-policies

@metamaskbot
Copy link
Collaborator

Policies updated

@metamaskbot
Copy link
Collaborator

Builds ready [b7be78f]
Page Load Metrics (1734 ± 81 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint29122241668362174
domContentLoaded15232161171316479
load15332172173416981
domInteractive246641147
Bundle size diffs [🚀 Bundle size reduced!]
  • background: -28.64 KiB (-0.84%)
  • ui: 0 Bytes (0.00%)
  • common: -56.61 KiB (-0.72%)

@jpuri jpuri marked this pull request as ready for review September 10, 2024 15:40
@jpuri jpuri requested review from a team as code owners September 10, 2024 15:40
@metamaskbot
Copy link
Collaborator

Builds ready [7bb95d9]
Page Load Metrics (1772 ± 56 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint15551968177511656
domContentLoaded15141957175011254
load15541969177211656
domInteractive129138199
Bundle size diffs [🚀 Bundle size reduced!]
  • background: -28.64 KiB (-0.84%)
  • ui: 0 Bytes (0.00%)
  • common: -56.61 KiB (-0.72%)

digiwand
digiwand previously approved these changes Sep 10, 2024
Copy link
Contributor

@digiwand digiwand left a comment

Choose a reason for hiding this comment

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

Looks good 👍🏼

I think we can update the description from
Fixes: TODO to Fixes: https://github.com/MetaMask/metamask-extension/issues/26980

It is worth mentioning that the solution appears to have been carried over from MetaMask/eth-json-rpc-middleware#333 (original), created by @mtsitrin, and applied as a patch.

We are applying this as a patch while continuing to discuss handling the verifyingContract spec. Without the patch, the eth-json-rpc-middleware enforces verifyingContracts as an Ethereum contract address in hex format. We are patching this to fix uses of cosmos chains using the https://github.com/evmos/ethermint EVM adapter

@metamaskbot
Copy link
Collaborator

Builds ready [f2c786d]
Page Load Metrics (1887 ± 61 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint26221111800371178
domContentLoaded16362090186212761
load16452106188712761
domInteractive156936115
Bundle size diffs [🚀 Bundle size reduced!]
  • background: -28.64 KiB (-0.84%)
  • ui: 0 Bytes (0.00%)
  • common: -56.61 KiB (-0.72%)

Copy link

codecov bot commented Sep 11, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 70.15%. Comparing base (3797b2f) to head (debeefb).

Additional details and impacted files
@@           Coverage Diff            @@
##           develop   #27021   +/-   ##
========================================
  Coverage    70.15%   70.15%           
========================================
  Files         1425     1425           
  Lines        49656    49656           
  Branches     13892    13892           
========================================
  Hits         34833    34833           
  Misses       14823    14823           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

package.json Outdated
@@ -267,7 +267,8 @@
"@metamask/nonce-tracker@npm:^5.0.0": "patch:@metamask/nonce-tracker@npm%3A5.0.0#~/.yarn/patches/@metamask-nonce-tracker-npm-5.0.0-d81478218e.patch",
"@metamask/keyring-controller@npm:^16.0.0": "patch:@metamask/keyring-controller@npm%3A17.1.1#~/.yarn/patches/@metamask-keyring-controller-npm-17.1.1-098cb41930.patch",
"@metamask/keyring-controller@npm:^17.1.0": "patch:@metamask/keyring-controller@npm%3A17.1.1#~/.yarn/patches/@metamask-keyring-controller-npm-17.1.1-098cb41930.patch",
"@trezor/connect-web@npm:^9.1.11": "patch:@trezor/connect-web@npm%3A9.3.0#~/.yarn/patches/@trezor-connect-web-npm-9.3.0-040ab10d9a.patch"
"@trezor/connect-web@npm:^9.1.11": "patch:@trezor/connect-web@npm%3A9.3.0#~/.yarn/patches/@trezor-connect-web-npm-9.3.0-040ab10d9a.patch",
"@metamask/eth-json-rpc-middleware@npm:^12.1.1": "patch:@metamask/eth-json-rpc-middleware@npm%3A14.0.1#~/.yarn/patches/@metamask-eth-json-rpc-middleware-npm-14.0.1-b6c2ccbe8c.patch"
Copy link
Contributor

@legobeat legobeat Sep 11, 2024

Choose a reason for hiding this comment

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

This forces all usage eth-json-rc-middleware from v12 to v14. While upgrading to v14 seems good, perhaps most safely done separately and this PR can path onto existing v12 as well as directly used v14?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch @legobeat , I removed this line of code.

@metamaskbot
Copy link
Collaborator

Builds ready [b7e6c13]
Page Load Metrics (1708 ± 55 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint24518991633338162
domContentLoaded15351882169310751
load15421889170811455
domInteractive157738168
Bundle size diffs [🚀 Bundle size reduced!]
  • background: -28.64 KiB (-0.84%)
  • ui: 0 Bytes (0.00%)
  • common: -56.61 KiB (-0.72%)

@jpuri
Copy link
Contributor Author

jpuri commented Sep 11, 2024

@metamaskbot update-policies

@metamaskbot
Copy link
Collaborator

Policies updated

Copy link

sonarcloud bot commented Sep 11, 2024

@jpuri jpuri requested review from legobeat and digiwand September 11, 2024 12:08
@metamaskbot
Copy link
Collaborator

Builds ready [debeefb]
Page Load Metrics (1641 ± 84 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint14212011163817484
domContentLoaded14152004162217082
load14222011164117584
domInteractive22104422110

@jpuri jpuri merged commit 4ee09fc into develop Sep 11, 2024
81 checks passed
@jpuri jpuri deleted the eth_json_rpc_middleware_patch branch September 11, 2024 13:22
@github-actions github-actions bot locked and limited conversation to collaborators Sep 11, 2024
@metamaskbot metamaskbot added the release-12.6.0 Issue or pull request that will be included in release 12.6.0 label Sep 11, 2024
@metamaskbot metamaskbot added release-12.5.0 Issue or pull request that will be included in release 12.5.0 and removed release-12.6.0 Issue or pull request that will be included in release 12.6.0 labels Sep 29, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
release-12.5.0 Issue or pull request that will be included in release 12.5.0 team-confirmations Push issues to confirmations team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bug: signTypedData_v4 fails when verifyingContract isn't a evm address
6 participants