fix: change types signatures verifyingContract validation to allow 'cosmos' as address#334
fix: change types signatures verifyingContract validation to allow 'cosmos' as address#334
Conversation
| // https://github.com/MetaMask/metamask-extension/issues/26980 | ||
| if ( | ||
| verifyingContract && | ||
| (verifyingContract as string) !== 'cosmos' && |
There was a problem hiding this comment.
We're going to introduce this single exception? Doesn't it seem possible there are other cases here that we're going to be chasing? Do we have a plan to deprecate support for this exception and start enforcing this more strictly or?
cc @MetaMask/wallet-api-platform-engineers
There was a problem hiding this comment.
Cosmos network DAPPs are using this deprecated SDK which has verifyingContract hard coded to cosmos https://github.com/evmos/ethermint/blob/main/ethereum/eip712/domain.go#L29 thus we need to do it to support cosmos network. Once this old library goes out of use we can revert the change.
We do not see more such cases currently.
There was a problem hiding this comment.
if I understand correctly we are planning on removing this sometime in the future: if so, to make sure we don't lose track of it, perhaps we can open a new issue and mention it in the code comment instead of using the issue that will be closed by this PR (#26980)?
There was a problem hiding this comment.
Good point @mikesposito , I added this issue https://github.com/MetaMask/eth-json-rpc-middleware/issues/337 and updated the comment.
mikesposito
left a comment
There was a problem hiding this comment.
Looks good to me as a temporary fix, I only have a doubt about how we can state more clearly that this is indeed a temporary fix
* origin/main: fix: change types signatures verifyingContract validation to allow 'cosmos' as address (#334) Update `main` with changes from v14.0.1 (#332) Request validation should not throw if verifyingContract is not defined in typed signature (#328) Add changelog entries for `#318` (#327) remove eth_sign (#320)
## **Description** Bump `@metamask/eth-json-rpc-middleware` from v14.0.1 (+ patch) to v14.0.2. The v14.0.2 release contains the same change as was added in the patch being removed. This bump has no functional changes. Changelog: https://github.com/MetaMask/eth-json-rpc-middleware/blob/main/CHANGELOG.md#1402 [](https://codespaces.new/MetaMask/metamask-extension/pull/28755?quickstart=1) ## **Related issues** Context: * #27021 * MetaMask/eth-json-rpc-middleware#334 ## **Manual testing steps** N/A ## **Screenshots/Recordings** N/A ## **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/develop/.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/develop/.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.
Fixes: MetaMask/metamask-extension#27070
Following the discussion in the thread, I am moving this validation to middleware. https://consensys.slack.com/archives/C02GENU377A/p1725914308614879
Original issue which got a hotfix: MetaMask/metamask-extension#26980