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

563/fix eth sign v4 signature not supported #576

Merged
merged 10 commits into from
May 27, 2021
Merged

563/fix eth sign v4 signature not supported #576

merged 10 commits into from
May 27, 2021

Conversation

alfetopito
Copy link
Contributor

@alfetopito alfetopito commented Apr 30, 2021 •
edited
Loading

Update 2021-05-27

  • Using new version of contracts NPM lib
  • No longer using my hacked version of ethers.js lib
  • This change fixes 2 wallets:
    • TrustWallet mobile browser
    • Opera browser wallet (Fantom)
  • Trying different signing methods in sequence:
    • v4 (default and preferred)
    • v3 (when v4 not available)
    • eth_sign (when v3 not available)

Summary

Closes #563

After this change, I'm able to sign orders on Trust wallet dapp browser.
Need to test with more wallets of this type (injected)

Testing

  1. Using Trust wallet mobile dapp browser, open this PR's deployed app
  2. Place an order like you normally do (fill in all fields, allow a token if needed)
  • A signature request of a typed data should be requested
  • The signature should succeed and the order should be placed successfully
  1. Using Opera browser, setup the built in crypto wallet
  2. In your mobile device, install Opera and setup crypto wallet there as well
  3. Open this PR's deployed app
  4. Place an order
  • On your phone, you should see the signature request of the hash
  • The signature should succeed and the order should be placed successfully

@alfetopito alfetopito self-assigned this Apr 30, 2021
@alfetopito alfetopito added the DONT_MERGE Indicates the PR should not be merged. Probably a WIP or PoC. label Apr 30, 2021
@github-actions
Copy link
Contributor

  • 🔭 GP Swap: Gnosis Protocol v2 Swap UI

@alfetopito alfetopito mentioned this pull request May 4, 2021
Closed
@alfetopito
Copy link
Contributor Author

This change alone won't do it.
We need also a change to ethers.js, as Trustwallet needs to use personal_sign instead of eth_sign, like metamask.
Opened a PR against their repo ethers-io/ethers.js#1542

@alfetopito alfetopito mentioned this pull request May 4, 2021
Closed
@alfetopito alfetopito force-pushed the 563/fix-eth_sign-v4-signature-not-supported branch 2 times, most recently from f066ec9 to 472eb79 Compare May 5, 2021 22:54
@alfetopito alfetopito removed the DONT_MERGE Indicates the PR should not be merged. Probably a WIP or PoC. label May 5, 2021
@anxolin
Copy link
Contributor

anxolin commented May 7, 2021

Maybe @MareenG could also run some tests in some of the failing wallets https://docs.google.com/spreadsheets/u/1/d/177RoN3x0gHDwKAgu-TXUYEuz-z-dPu3RybZvOumRrAA/edit#gid=0

anxolin
anxolin reviewed May 7, 2021
View reviewed changes
cypress/support/commands.js Outdated Show resolved Hide resolved
src/custom/utils/trade.ts Outdated Show resolved Hide resolved
@MareenG
Copy link

MareenG commented May 7, 2021

Yes, I will do some more tests. Are there some you would consider as most important to test?

@alfetopito alfetopito mentioned this pull request May 7, 2021
Closed
@alfetopito alfetopito force-pushed the 563/fix-eth_sign-v4-signature-not-supported branch from 472eb79 to 9593528 Compare May 10, 2021 13:38
@alfetopito alfetopito mentioned this pull request May 10, 2021
Closed
@W3stside
Copy link
Contributor

@alfetopito is the new ethers package being used here or still yours? is there anything pending in this PR or should i review?

@anxolin
Copy link
Contributor

anxolin commented May 10, 2021

For IOS people! now you can re-enable the Dapp browser that was removed in a past version https://link.trustwallet.com/browser_enable

anxolin
anxolin reviewed May 10, 2021
View reviewed changes
Copy link
Contributor

@anxolin anxolin left a comment

Choose a reason for hiding this comment

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

It didn't work for me 😅

image

@alfetopito
Copy link
Contributor Author

alfetopito commented May 10, 2021 •
edited
Loading

@alfetopito is the new ethers package being used here or still yours? is there anything pending in this PR or should i review?

@david (wrong David, sorry) @W3stside Yes, it should be mine :)
Ethers.js hasn't released a new version yet. Neither has my PR been merged.

How to review?
Not much in terms of code change, just one additional conditional on the trade utils module.
I'd say the main things is to test the app, which is basically test using TrustWallet's dApp browser (see Test steps)
The test must be done on mainnet, and you can find details for a shared wallet on 1password to setup it up in your phone.

@anxolin Can you try again? Several times?
Their RPC connection is not very reliable. Doesn't work all time for me.

@anxolin
Copy link
Contributor

anxolin commented May 10, 2021 •
edited
Loading

@anxolin Can you try again? Several times?
Their RPC connection is not very reliable. Doesn't work all time for me.

I've tried like 20 times, it looks stable to me the read operations, even wrap operations. It's just the signing operation is not working

It's the same error I get in cowswap.exchange by the way

@alfetopito
Copy link
Contributor Author

@anxolin Can you try again? Several times?
Their RPC connection is not very reliable. Doesn't work all time for me.

I've tried like 20 times, it looks stable to me the read operations, even wrap operations. It's just the signing operation is not working

It's the same error I get in cowswap.exchange by the way

😞

Can't help you there. You'll have to debug this one for me, since I have no apple device to test with.

@W3stside
Copy link
Contributor

@david Yes, it should be mine :)

lmaoooo i love what you tagged me as 😂 😂 😂

@alfetopito
Copy link
Contributor Author

@david Yes, it should be mine :)

lmaoooo i love what you tagged me as joy joy joy

I got that from @anxolin always tagging the wrong Michel =P

But anyway, it's not too far off. He's your neighbor ;)

@fairlighteth
Copy link
Contributor

@alfetopito Nice find. We found David's twin brother by luck of the draws 🎲

@anxolin
Copy link
Contributor

anxolin commented May 14, 2021

I tried to follow trust debug instructions, but they are hard to follow for IOS.

I opened this issue:
trustwallet/trust-web3-provider#148

Probably we will need to inject some remote debug library for this. According to what they say, they implemented v3, which we also default back to.

@alfetopito
Copy link
Contributor Author

I tried to follow trust debug instructions, but they are hard to follow for IOS.

I opened this issue:
trustwallet/trust-web3-provider#148

Probably we will need to inject some remote debug library for this. According to what they say, they implemented v3, which we also default back to.

Where do you see we default to v3?

We currently don't do that, AFAIAA

@alfetopito alfetopito force-pushed the 563/fix-eth_sign-v4-signature-not-supported branch from 9593528 to 394f698 Compare May 21, 2021 20:45
@alfetopito
Copy link
Contributor Author

I know it's broken.
Needs the changes from gnosis/gp-v2-contracts#650 to be released first.

@alfetopito alfetopito added the On_hold There's unresolved blockers. label May 21, 2021
@anxolin
Copy link
Contributor

anxolin commented May 25, 2021

Trust wallet is addressing the issue that we have in IOS: trustwallet/trust-web3-provider#149

@alfetopito
Copy link
Contributor Author

Trust wallet is addressing the issue that we have in IOS: trustwallet/trust-web3-provider#149

Great news!

Although, I've made such a nice logic for trying different versions sequentially 😞

No matter, I'm sure it'll be useful for other wallets.

@alfetopito alfetopito mentioned this pull request May 26, 2021
Merged
@alfetopito
Copy link
Contributor Author

Currently broken.
Depends on gnosis/gp-v2-contracts#659 be published to NPM

@alfetopito alfetopito force-pushed the 563/fix-eth_sign-v4-signature-not-supported branch from abc6d8a to 64bb88d Compare May 27, 2021 15:23
@alfetopito alfetopito removed the On_hold There's unresolved blockers. label May 27, 2021
@alfetopito alfetopito requested a review from anxolin May 27, 2021 15:49
@anxolin anxolin merged commit b1f0fcf into develop May 27, 2021
@alfetopito alfetopito deleted the 563/fix-eth_sign-v4-signature-not-supported branch May 27, 2021 15:54
@nlordell nlordell mentioned this pull request Jun 10, 2021
Merged
mergify bot pushed a commit to gnosis/gp-v2-contracts that referenced this pull request Jun 10, 2021
@nlordell
Added TypedDataV3Signer (#686)
7fd36ca 
This PR just forward ports #659 onto `main`:

> Closes (supersedes) #650
>
> As per comment on that PR (#650 (comment)), this change introduces a Signer class that uses `eth_signTypedData_v3` instead of `eth_signTypedData_v4` on the method `signTypedData`
>
> ### Test Plan
>
> Tested via `yalc` on gnosis/cowswap#576 with TrustWallet
@gomesalexandre gomesalexandre mentioned this pull request Dec 20, 2023
Closed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@anxolin anxolin anxolin approved these changes

@W3stside W3stside Awaiting requested review from W3stside

@fairlighteth fairlighteth Awaiting requested review from fairlighteth

Assignees

@alfetopito alfetopito

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

eth_signTypedData_v4 not supported
5 participants
@alfetopito @anxolin @MareenG @W3stside @fairlighteth