Wallet connect and metamask interactions are different (some WC don't even work)

decentraland / unity-renderer

Unity implementation of Decentraland Client

https://play.decentraland.org

Apache License 2.0

137 stars 93 forks source link

Wallet connect and metamask interactions are different (some WC don't even work) #2004

Closed pravusjif closed 2 years ago

pravusjif commented 2 years ago

The wallet interactions freamework/API should be wallet-agnostic
Apparently that's not the case right now, for example the experience of signing into the GoltfCraft (https://play.decentraland.org/?position=48%2C-45) scene is completely different (and is broken with wallet connect):

Signing with Metamask: Screen Shot 2022-03-16 at 18 42 25

Signing with Wallet Connect: Screen Shot 2022-03-16 at 18 46 01 Screenshot_20220316-184636

Even if we have that extra pop-up warning when being logged into Decentraland with Wallet Connect, the signing and transactions should all work regardless of the wallet.

pabloes commented 2 years ago

For now we use @decentraland/web3-provider and eth_signTypedData_v4

nachomazzara commented 2 years ago

Hi, @pabloes can you share a repo and how to reproduce the issue? I need more context about the library versions, their usage, etc

pabloes commented 2 years ago

The problem here is not about decentraland or wallet-connect, it's about wallets supporting RPC method "eth_signtypeddata_v4"

But this EIP-712 is not "Final", and only some wallets support it:

Metamask
Metamask mobile
Rainbow

But not TrustWallet for example.

I liked its UX and seeing that metamask adopted it, I just used it also. The only we need to work as always is to still have access to the provider sendAsync method to be able to send "eth_signtypeddata_v4" RPC (or any other method allowing to send RPC to the provider), but EIP-712 signatures with walletconnect wallets not supporting it, won't work.

If something is broken in decentraland for us we will apply a migration, but it's working fine right now as always with the mentioned wallets, I always had in mind a migration in future btw.

pabloes commented 2 years ago

a mistake in previous message, it is not that TrustWallet doesn't support eth_signtypeddata_v4, is that we have something wrong on our RPC message.

pravusjif commented 2 years ago

Thanks for all the info and testing on your side Pablo, so the problem lies in the usage of eth_signtypeddata_v4 method in the scene code (with @decentraland/web3-provider library), as apparently if the RPC message hasn't been configured correctly then some wallets with wallet-connect (in this case TrustWallet) can have problems on their signing interactions.

This way of using that method in the marketplace should be a good example of how it should be used to work wallet-agnosticly: https://github.com/decentraland/marketplace-contracts/blob/d27b695e8006e70a5b7f245a57a13a5ec97f7ccf/test/helpers/metaTx.js#L68