Fix issue signing using WC for some wallets #528
Conversation
|
CLA Assistant Lite: I have read the CLA Document and I hereby sign the CLA You can retrigger the CLA Action by commenting recheckcla in this Pull Request |
|
anxolin
changed the title
| @@ -137,16 +137,19 @@ async function _signPayload( | |||
| payload: any, | |||
| signFn: typeof _signOrder | typeof _signOrderCancellation, | |||
| signer: Signer, | |||
| signingMethod: 'v4' | 'int_v4' | 'v3' | 'eth_sign' = 'v4' | |||
| signingMethod: 'default' | 'v4' | 'int_v4' | 'v3' | 'eth_sign' = 'v4' | |||
There was a problem hiding this comment.
We now allow to specify the "default" version. In this case, will be same as v4, at least now. In the future who knows :)
Setting to default is what makes us to send the RPC call with eth_signTypedData instead of eth_signTypedData_v4
| ): Promise<SigningResult> { | ||
| const signingScheme = signingMethod === 'eth_sign' ? SigningScheme.ETHSIGN : SigningScheme.EIP712 | ||
| let signature: Signature | null = null | ||
|
|
||
| let _signer | ||
| try { | ||
| switch (signingMethod) { | ||
| case 'default': | ||
| _signer = new TypedDataVersionedSigner(signer) |
There was a problem hiding this comment.
If we use the default, we don't specify the version. Will use eth_signTypedData
Co-authored-by: dave <[email protected]>
|
I'll merge to consolidate in staging so we can start testing there asap please, postmerge reviews are needed |
|
I can see the JSON with all the order details when signing orders using 1Inch, but the this fix does not resolve issues with an intermediate step (opening AppStore) when signing all types of transactions using this wallet. |
Summary
This PR fixes the issue with 1inch wallet. Resullt of the review #496
Short description
This PR fixes the issue where, when trading, the user was prompted to sign only a hash instead of the JSON with all the order details.
This was happening to 1inch wallet, and possibly other wallets too.
Before, in 1inch wallet
After, in 1inch wallet
Short technical description
Uses a new version of the smart contracts which allows us to use
eth_signTypedDatainstead ofeth_signTypedData_v4for signing.Longer explanation
The issue was making the signing method to default to ETHSIGN instead of using EIP-712
This is because the wallet was not understanding the signing method we provide with the request. And the app tries to downgrade to v3 and ETHSIGN if this is the case.
The actual problem is that by upgrading web3-react, a new wallet-connect version came with it. This version was using a different wallet connect provider (@walletconnect/ethereum-provider instead of @walletconnect/web3-provider).
This shouldn't be a big deal, because they generate the same signing request. However, at some point for the old version, wallet connect was replacing the signing method name from
eth_signTypedData_v4to justeth_signTypedData.Apparently 1inch wallet understand the latter but not the former.
Note for the reviewers
Please excuse me for adding all the new org changes for the NPM package.
Allow me this in this case. I can point out to the only 2 changes, please see the comments in the code.
To Test