Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cowswap trade on Gnosis via Rabby does not work #5138

Closed
koeppelmann opened this issue Aug 22, 2023 · 6 comments
Closed

Cowswap trade on Gnosis via Rabby does not work #5138

koeppelmann opened this issue Aug 22, 2023 · 6 comments
Labels
bug Something isn't working V3 Swapper

Comments

@koeppelmann
Copy link

Overview

Connected with Rabby/ Chrome/ iOS.
Switched to Gnosis in the Shapeshift UI -> Rabby also switches to Gnosis.
Want to do trade via Cowswap.
When initiating the trade as expected message to sign pops up.

Issue 1) It is not formatted
Issue 2) It seems that it is signing a message for mainnet and not for Gnosis

  1. is likely the reason why after signing I get an error message.
Screenshot 2023-08-22 at 09 38 25

--
Screenshot 2023-08-22 at 09 38 39

Screenshot 2023-08-22 at 09 38 46

References and additional details

..

Acceptance Criteria

..

Need By Date

No response

Screenshots/Mockups

No response

Estimated effort

No response

@gomesalexandre
Copy link
Contributor

gomesalexandre commented Aug 23, 2023

Hi @koeppelmann thank you for reporting this! Please note that we don't support Rabby, I suppose you've used MetaMask and rabby injected at MM.
I'm not familiar with the internals of Rabby, are you currently connected to Gnosis chain on it when attempting the trade? If not, the way things work is there is a first JSON-RPC call to wallet_switchEthereumChain before the actual signature (in the case of CoW)/Tx is prompted for signing, so I would assume Rabby doesn't support this method.

@twblack88
Copy link
Contributor

@koeppelmann thanks so much for bringing this in! We've got some rough edges to smooth on COWswap and we absolutely welcome your feedback. If you're trying to use it as walletconnect we are going to prioritize that at the next roadmap call (hopefully) this might be a great use case.

@gomesalexandre I think Rabby runs their own RPC IIRC, they're stack is 🐐 -ed but that flow makes sense.

@gomesalexandre
Copy link
Contributor

gomesalexandre commented Aug 28, 2023

@twblack88 this wouldn't be related to the JSON-RPC endpoint used, but to:

  1. wallet_switchEthereumChain method not being triggered. Disregard my previous message about Rabby supposedly not supporting this method, this is irrelevant of the wallet used. The reason why switch chain isn't triggered is because this is a signature, not a transaction. Hence, we don't need to switch chains, see this CoW signature while connected to BSC:
Screenshot 2023-08-28 at 10 48 59

And the matching CoW Tx on Gnosis chain: https://explorer.cow.fi/gc/orders/0xf46f02d45e3c2d3d528105f1448361662a8f9ba61e0a6f61dd456fe834a5e4f45daf465a9ccf64deb146eeae9e7bd40d6761c98664ec666b?tab=overview

@twblack88 I think this raises a valid point for signatures, while the wallet doesn't need to be connected to a specific chain, it can definitely confusing for users. What about we switch chains before signing messages, similar to before signing and broadcasting a Tx?

  1. Confirmed with MM our messages are not currently structured, at least not from a user-facing perspective. This is CoW's UI for comparison:
image

Looking at hdwallet, we indeed use personal_sign vs. eth_signTypedData_v4 if applicable for EIP-712. We should implement hdwallet signing using eth_signTypedData_v4 to make this human-readable.

@twblack88
Copy link
Contributor

twblack88 commented Sep 27, 2023

We should implement hdwallet signing using eth_signTypedData_v4 to make this human-readable.

@gomesalexandre agreed, slot in when we have the time to reduce UX confusion.

@gomesalexandre
Copy link
Contributor

@twblack88 I wasn't able to repro this - Rabby signing is happy and I was able to complete a CoW swap end-to-end

image

I suggest we close this and open an issue for eth_signTypedData_v4 instead. This is a relatively high effort with huge risks for regressions (in particular signing with Ledger and Trezor through MM, see https://docs.metamask.io/wallet/how-to/sign-data/). I do believe we need this so users aren't blind signing and are able to see the structured EIP-712 data, but as far as this is concerned, this is a non-issue.

@gomesalexandre
Copy link
Contributor

Closing in favor of #5916 as discussed with @twblack88

@github-project-automation github-project-automation bot moved this from To schedule to Done in ShapeShift Dashboard Dec 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working V3 Swapper
Projects
Archived in project
Development

No branches or pull requests

4 participants