[IMPROVEMENT] Support signTypedData_v4 and use signTypedData_v3 by default in WalletConnect

[sssemil]

Description
Change the default signTypedData version used in WalletConnect to v3 and add support for the v4 version.

[github-actions]

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.

[sssemil]

I have read the CLA Document and I hereby sign the CLA

[Muradm373]

Waited this change for long time

[sssemil]

Hey @sethkfman is this good? :)

[sethkfman]

@sssemil Is there a specific dapp you are using to test? We just need to confirm the functionality with QA.

[midgerate]

@sethkfman you can use snapshot. I can create a version which you can test.

[midgerate]

@sethkfman

Click this https://bafybeidi4gxba5q7qpl4kr7u4zuks3hfkywkbyhoptf4m6j5s2aky7qw2a.on.fleek.co/#/midgerate.eth/proposal/0x1ccddbe103253ffc71b0ba1759b2ee5902d5717777ddba39159548b4b76a6016

and try to vote with walletconnect + metamask and it will work (FYI, there seems to be a problem with loader so just wait a bit)

Then go to https://bafybeidi4gxba5q7qpl4kr7u4zuks3hfkywkbyhoptf4m6j5s2aky7qw2a.on.fleek.co/#/midgerate.eth/proposal/0xf11b762089e0cbe7307b052390da68ce3a67d61c16f3e615f9d1d062f731520b

and try to vote - This will get stuck because this proposal uses arrays and requires v4 to work correctly.

Then try to use the QA metamask (which I believe includes this fix) and see if it works with the second proposal.

I can help with testing if you tell me how to get the QA build of metamask mobile.

[midgerate]

@sethkfman is there anything I could help to speed this up?

[sethkfman]

@midgerate & @sssemil In order to maintain backward compatibility the check for payload.method === 'eth_signTypedData' should be removed from v4 (line 212) and added to v3 (line 239). Would you like me to make the change?

[sssemil]

@sethkfman done. Sorry, was away for a week, I don't have a dapp that I can provide for testing.

[midgerate]

@sethkfman @sssemil I am not sure if this would work with walletconnect, walletconnect only sends signTypedData as method and we should be using v4 as default. isn't v4 compatible with v1 and v3?

[midgerate]

@sethkfman done. Sorry, was away for a week, I don't have a dapp that I can provide for testing.

@sssemil I already gave an link to the dapp which can be used to test

[sssemil]

@midgerate you can send custom calls I think, with this - https://docs.walletconnect.com/1.0/client-api#send-custom-request , right?

[midgerate]

@sssemil I see, cool. I could use that. @sethkfman if you can share the QA build I can test it with snapshot.org and let you know if it works.

[sssemil]

@sethkfman @sssemil I am not sure if this would work with walletconnect, walletconnect only sends signTypedData as method and we should be using v4 as default. isn't v4 compatible with v1 and v3?

v4 has breaking changes, I don't think it works with v3 or older. I would agree that we should use v4 by default. Metamask docs are confusing on the defaults question as well - https://docs.metamask.io/guide/signing-data.html#sign-typed-data-v4 saying that both v3 and v4 are the latest version, and not saying which one is the default one. What makes this even more confusing is that for example if you write tests in hardhat, the default will be v4, and one may end up debugging a weird invalid signature bug for a long time, as I was unfortunate enough to do.

[midgerate]

@sssemil I can create a PR in the walletConnect repo to also implement v4 method. So it does not matter how we implement backward compatibility in metamask wallet, as long as this PR is merged.

[sssemil]

@sssemil I can create a PR in the walletConnect repo to also implement v4 method. So it does not matter how we implement backward compatibility in metamask wallet, as long as this PR is merged.

sure, why not :)

[sethkfman]

To keep backwards compatibility with our current integrations we should keep payload.method === 'eth_signTypedData' || payload.method === 'eth_signTypedData_v3'. Once wallet connect makes v4 there standard then we can update the logic. I think the PR is acceptable, currently.

[midgerate]

@sethkfman great! I hope it gets merged soon.

[sssemil]

@sethkfman anything I can do to get this approved?

[sethkfman]

LGTM