gnosis / cowswap

🐮 CowSwap: First Gnosis Protocol v2 UI
https://gnosis.io
GNU General Public License v3.0
113 stars 55 forks source link

Wallet Connect working bad (EOA) #301

Closed anxolin closed 3 years ago

anxolin commented 3 years ago

At least for Metamask Mobile App

EDIT 1: It happens also in trust wallet EDIT 2: I had issues with the safe too

MareenG commented 3 years ago

Release v0.8 I tried to connect MM via wallet connect is only worked like every third time. I had to relaod the website and scan again.

alfetopito commented 3 years ago

One issue that we have regarding using WallectConnect on a network other than Mainnet is that the app assumes it'll always be running on Mainnet.

See https://github.com/gnosis/gp-swap-ui/blob/develop/src/connectors/index.ts#L35

That is fine for Uniswap where they only have their prod version on Mainnet, and can enforce no one will ever try to connect to anything else, so the app is served with only Mainnet in mind and screw whomever tries another network.

In our case though, that might be a problem. We could of course enforce that, with a different deployed version per network, but I believe that's not ideal, neither how wallets work, allowing you to switch the network as the user sees fit.

That's not even just for making the testing easier. We also have the xDai version to support.

So, what I'm thinking to address this issue specifically is to have one WallectConnectConnector instance per network. Which also means one REACT_APP_NETWORK_URL parameter per network, such as:

We can keep the related config REACT_APP_CHAIN_ID as a default network id, but ultimately the wallet would provide the current network id.

anxolin commented 3 years ago

Agree! good finding

alfetopito commented 3 years ago

Still don't know what's wrong, but I think I'm getting closer.

The issue

Any wallet connected via WC (so far only tested Trust Wallet and MetaMask mobile) does not show any balance. All calls are permanently stuck and we can see lots of failures such as this in the console: screenshot_2021-04-13_14-57-32

Upstream differences

As it's known, this is a fork on Uniswap's code. I've tried out the same on Uniswap's website and it works fine :thinking: Comparing their latest release with our latest develop branch, I did not find any changes that could be related https://github.com/gnosis/gp-swap-ui/compare/140ff7a674d53b3c3bb95abb32d416bffd6be54c...develop The dependencies are mostly the same, Anxo's SDK fork is pretty straight forward as well. Our mod files did not touch multicall code as far as I can tell.

Can anyone see something that I don't?

Multicall contract

The problem is on the multicall aggregated contract call https://github.com/gnosis/gp-swap-ui/blob/develop/src/state/multicall/updater.tsx#L36

Looking at the contract source https://github.com/makerdao/multicall, there's nothing special.

It's recommended to use it with https://github.com/makerdao/multicall.js lib, but Uniswap doesn't do it.

Still that seems to work for everything else, can't find anything different for WC so far.

One difference that I found though is that the contract abi on etherscan is slightly different from what we have in the project: screenshot_2021-04-14_13-54-29

But then again, it's working so, nothing wrong there

Network calls

Next I went to check the network calls, and could see the RPC eth_calls that are failing.

Request:

{
  "id": 1618432983950563,
  "jsonrpc": "2.0",
  "method": "eth_call",
  "params": [
    {
      "to": "0xeefba1e63905ef1d7acba5a8513c70307c1ce441",
      "data": "0x252dba420000000000000000000000000000000000000000000000000000000000000020000000000000000000000000000000000000000000000000000000000000000700000000000000000000000000000000000000000000000000000000000000e00000000000000000000000000000000000000000000000000000000000000180000000000000000000000000000000000000000000000000000000000000022000000000000000000000000000000000000000000000000000000000000002c0000000000000000000000000000000000000000000000000000000000000034000000000000000000000000000000000000000000000000000000000000003c0000000000000000000000000000000000000000000000000000000000000044000000000000000000000000000000000000c2e074ec69a0dfb2997ba6c7d2e1e000000000000000000000000000000000000000000000000000000000000004000000000000000000000000000000000000000000000000000000000000000240178b8bff4d2e2ed0b6d573cde234d89c618cf5bc9b2db7d00c1b0e2e4e145b8b2e1babc0000000000000000000000000000000000000000000000000000000000000000000000000000000065770b5283117639760bea3f867b69b3697a91dd0000000000000000000000000000000000000000000000000000000000000040000000000000000000000000000000000000000000000000000000000000002470a082310000000000000000000000004cbc3b6b16ac4218169078add37947b76127e10b00000000000000000000000000000000000000000000000000000000000000000000000000000000c02aaa39b223fe8d0a0e5c4f27ead9083c756cc20000000000000000000000000000000000000000000000000000000000000040000000000000000000000000000000000000000000000000000000000000002470a082310000000000000000000000004cbc3b6b16ac4218169078add37947b76127e10b00000000000000000000000000000000000000000000000000000000000000000000000000000000c778417e063141139fce010982780140aa0cd5ab0000000000000000000000000000000000000000000000000000000000000040000000000000000000000000000000000000000000000000000000000000000406fdde0300000000000000000000000000000000000000000000000000000000000000000000000000000000c778417e063141139fce010982780140aa0cd5ab00000000000000000000000000000000000000000000000000000000000000400000000000000000000000000000000000000000000000000000000000000004313ce56700000000000000000000000000000000000000000000000000000000000000000000000000000000c778417e063141139fce010982780140aa0cd5ab0000000000000000000000000000000000000000000000000000000000000040000000000000000000000000000000000000000000000000000000000000000495d89b4100000000000000000000000000000000000000000000000000000000000000000000000000000000eefba1e63905ef1d7acba5a8513c70307c1ce441000000000000000000000000000000000000000000000000000000000000004000000000000000000000000000000000000000000000000000000000000000244d2301cc0000000000000000000000004cbc3b6b16ac4218169078add37947b76127e10b00000000000000000000000000000000000000000000000000000000"
    },
    "latest"
  ]
}

Response:

{
  "jsonrpc": "2.0",
  "id": 1618432983950563,
  "result": "0x"
}

Hmmm ok

Tenderly

So, I went to Tenderly and used the same parameters on the same contract and used their Simulator.

(keep in mind I've used parsed data as is in the internal state, so I didn't manually decoded the RPC call data)

It worked fine screenshot_2021-04-14_14-00-31

Here are the input params for reference:

[["0x00000000000C2E074eC69A0dFb2997BA6C7d2e1e","0x0178b8bff4d2e2ed0b6d573cde234d89c618cf5bc9b2db7d00c1b0e2e4e145b8b2e1babc"],["0x65770b5283117639760beA3F867b69b3697a91dd","0x70a082310000000000000000000000004cbc3b6b16ac4218169078add37947b76127e10b"],["0xC02aaA39b223FE8D0A0e5C4F27eAD9083C756Cc2","0x70a082310000000000000000000000004cbc3b6b16ac4218169078add37947b76127e10b"],["0xc778417E063141139Fce010982780140Aa0cD5Ab","0x06fdde03"],["0xc778417E063141139Fce010982780140Aa0cD5Ab","0x313ce567"],["0xc778417E063141139Fce010982780140Aa0cD5Ab","0x95d89b41"],["0xeefBa1e63905eF1D7ACbA5a8513c70307C1cE441","0x4d2301cc0000000000000000000000004cbc3b6b16ac4218169078add37947b76127e10b"]]

So, it must be something with the JSON RPC client when connected via WC? That's where I'll look next...

alfetopito commented 3 years ago

Logging bunch of stuff here and there to debug this, and look at this beauty: screenshot_2021-04-14_14-23-19

My initial guess: It's getting the chainId from WC bridge, which is 1. But in my MM I'm connected on Rinkeby. So, when I connect with WC, the provider still has rinkeby RPC endpoint (which I could have seen in the network tabs if I was paying any attention) screenshot_2021-04-14_14-26-17

Let's keep digging to see whether I'm right.

anxolin commented 3 years ago

Great investigation!!

I think is just that, what u mentioned yesterday, that we3-react doesn't allow to use more than one network for wallet connect. I did a fix in their repository https://github.com/NoahZinsmeister/web3-react/pull/185 that I hope it fixes the issue.

Also added multi network config support for cow swap #435

Additionally, I published the https://github.com/NoahZinsmeister/web3-react/pull/185 fix not NPM (https://www.npmjs.com/package/@anxolin/walletconnect-connector) and include it in a fix that I hope closes this issue. At least for the part of reading the balances #436

anxolin commented 3 years ago

I've just merged https://github.com/gnosis/gp-swap-ui/pull/436#issuecomment-820306702, wich have some additional test and information from Leandro that will come handy when we tackle this issue (and the related https://github.com/gnosis/gp-swap-ui/issues/431)

anxolin commented 3 years ago

Related https://github.com/gnosis/gp-swap-ui/issues/618