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.

Sign up for GitHub

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

Jump to bottom

fix: change types signatures verifyingContract validation to allow 'cosmos' as address #334

Merged
merged 2 commits into from
Sep 23, 2024
Merged

fix: change types signatures verifyingContract validation to allow 'cosmos' as address #334

merged 2 commits into from
Sep 23, 2024

Conversation

jpuri
Copy link
Contributor

@jpuri jpuri commented Sep 17, 2024

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

@jpuri jpuri requested a review from a team as a code owner September 17, 2024 12:05
@jpuri jpuri changed the title fix: hange types signatures verifyingContract validation to allow 'cosmos' as address fix: change types signatures verifyingContract validation to allow 'cosmos' as address Sep 17, 2024
adonesky1
adonesky1 reviewed Sep 18, 2024
View reviewed changes
src/wallet.ts
// https://github.com/MetaMask/metamask-extension/issues/26980
if (
verifyingContract &&
(verifyingContract as string) !== 'cosmos' &&
Copy link
Contributor

@adonesky1 adonesky1 Sep 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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)?

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 point @mikesposito , I added this issue #337 and updated the comment.

mikesposito
mikesposito previously approved these changes Sep 18, 2024
View reviewed changes
Copy link
Member

@mikesposito mikesposito 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 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

@jpuri jpuri merged commit 7fbac8b into main Sep 23, 2024
19 checks passed
@jpuri jpuri deleted the add_cosmos_support branch September 23, 2024 10:22
@Gudahtt Gudahtt mentioned this pull request Oct 2, 2024
Merged
Gudahtt added a commit that referenced this pull request Oct 2, 2024
@Gudahtt
Merge remote-tracking branch 'origin/main' into sync-v14-main
568232d 
* 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)
@Gudahtt Gudahtt mentioned this pull request Nov 27, 2024
Merged
7 tasks
github-merge-queue bot pushed a commit to MetaMask/metamask-extension that referenced this pull request Nov 27, 2024
@Gudahtt
chore: Bump @metamask/eth-json-rpc-middleware to v14.0.2 (#28755)
b757d95 
## **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

[![Open in GitHub
Codespaces](https://github.com/codespaces/badge.svg)](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.
github-merge-queue bot pushed a commit to MetaMask/metamask-extension that referenced this pull request Nov 27, 2024
@Gudahtt
chore: Bump @metamask/eth-json-rpc-middleware to v14.0.2 (#28755)
78a218f 
## **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

[![Open in GitHub
Codespaces](https://github.com/codespaces/badge.svg)](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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mikesposito mikesposito mikesposito approved these changes

@adonesky1 adonesky1 Awaiting requested review from adonesky1

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Changes in validation for verifyingContract address
3 participants
@jpuri @mikesposito @adonesky1