Skip to content
This repository has been archived by the owner on Jun 24, 2022. It is now read-only.

eth_signTypedData_v4 not supported #563

Closed
anxolin opened this issue Apr 29, 2021 · 12 comments · Fixed by #576
Closed

eth_signTypedData_v4 not supported #563

anxolin opened this issue Apr 29, 2021 · 12 comments · Fixed by #576
Assignees
Labels
app:CowSwap CowSwap app Bug Something isn't working

Comments

@anxolin
Copy link
Contributor

anxolin commented Apr 29, 2021

Describe the bug
Error signing the transaction

Details
eth_signTypedData_v4 not supported.

Additional context
This comes from the feedback button, but no email was left to get more context on how to reproduce.

The message the user gets is:
the method eth_signTypedData_v4 does not exist/is not available

There's 6 methods created for historic reasons, maybe we can fallback to v3, and eth_sign as last resources?

  • eth_sign
  • personal_sign
  • signTypedData (currently identical to signTypedData_v1)
  • signTypedData_v1
  • signTypedData_v3
  • signTypedData_v4

It would be great to get to reproduce this.

More info in the device:
image

I bet is a webview within an app, probably imToken if I have to bet on one (seeing the user is from Singapore)

@nlordell WDYT?

@anxolin anxolin added Bug Something isn't working app:CowSwap CowSwap app labels Apr 29, 2021
@nlordell
Copy link
Contributor

Looking at MetaMask/metamask-extension#6868 it appears that v3 and v4 work the same way, but v4 adds support for nested structs and arrays of complex types.

Since we don't make use of either of these things, I think its OK to
eth_signTypedData_v4 > eth_signTypedData_v3 > eth_sign.

Although, I think it would also be find to just eth_signTypedData_v4 > eth_sign, which would also be a bit simpler.

@alfetopito
Copy link
Contributor

This is the issue we have for instance with Ledger through MM.
Also the issue I had with Ledger directly.

What we need is to add a fallback more general to switch to eth_sign whenever the EIP712 signature methods are not supported.

Right now it's hard coded only for the specific MM error code: https://github.com/gnosis/gp-swap-ui/blob/b9bc216a1b9293d2e0554d7dd6acbfd85605c102/src/custom/utils/trade.ts#L105

If the wallet is imToken as you say, we need to test and see what's the error code we get there.

@anxolin
Copy link
Contributor Author

anxolin commented Apr 29, 2021

If the wallet is imToken as you say, we need to test and see what's the error code we get there.

Yep, it's just a guess. Im not actually sure. I say that cause i know is used is china a lot, has a webview, and uses an injected web3 instance, works similar to metamask

We should try more webviews and mobile browsers. It would be great to have a matrix with wallets we want to try out in a document, and see if we are compatible. What do you think @MareenG

We don't need to try them out in every version, but is good to test as many as we can (the ones with some market quota i mean of course)

@MareenG
Copy link

MareenG commented Apr 30, 2021

I will start such a document.
We could focus on the ones that are most used and test occasionally .

@alfetopito
Copy link
Contributor

Got a report via Anna from twitter of a user having the same issue on Trust Wallet

https://gnosisinc.slack.com/archives/CPZA1AGKY/p1619745085192100
https://twitter.com/MesquitaDaniell/status/1387937258515443714?s=20

@alfetopito
Copy link
Contributor

Summarizing what happened so far.
With the change on #576 the signature no longer fails due to lack of eth_sign_v4, and the order is sent to the backend.

But, the signed message is incorrect.
Here's the full slack thread https://gnosisinc.slack.com/archives/CPZA1AGKY/p1620080626246100

What was said there is basically that the recovered address from the signature is not the user's address.
Then the backend rejects the order since the random address has no funds.

With Nick's and Valentin's investigative skill, they found out that TrustWallet behaves the same as Metamask, as we have to use personal_sign instead of eth_sign.

So, the change alone in the PR (#376) won't be enough.

We need also a change to ethers.js.
I've opened a PR against their repo ethers-io/ethers.js#1542

@anxolin
Copy link
Contributor Author

anxolin commented May 6, 2021

One of the users reported he used https://crypto.com/defi-wallet

EDIT: I installed the app, but i had to go through a KYC which will be done in 2-3 days

@alfetopito
Copy link
Contributor

One of the users reported he used https://crypto.com/defi-wallet

EDIT: I installed the app, but i had to go through a KYC which will be done in 2-3 days

Oh boy.
I was starting to install that. I'll leave it up to you then =P

@alfetopito
Copy link
Contributor

One of the users reported he used https://crypto.com/defi-wallet
EDIT: I installed the app, but i had to go through a KYC which will be done in 2-3 days

Oh boy.
I was starting to install that. I'll leave it up to you then =P

@Anxo Where where you asked for KYC? I just finished the setup and did not need any.
I think you installed the wrong app.
I know, super confusing, makes no sense, but they do have two apps with very similar names and different purpose.

You have to install the Defi wallet, which an EOA. The other app is their custodial wallet.

@alfetopito
Copy link
Contributor

Regarding Status wallet -> #608

As for Crypto.com, there's no dApp browser... I mean, not in the Defi wallet. Or does the other wallet (the one you are waiting for KYC) have?

@alfetopito
Copy link
Contributor

Crypto.com wallet -> #610

Doesn't work at all :(

@alfetopito
Copy link
Contributor

alfetopito commented May 12, 2021

One more to the list.
Opera (Fantom wallet).

It's basically an injected wallet provider.

Fails with the same error "The method eth_signTypedData_v4 does not exist/is not available"

It does work with the fix on #576 🥳

@alfetopito alfetopito self-assigned this May 26, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
app:CowSwap CowSwap app Bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants