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

Uni merge: Network selector is enabled when connected to SC wallets #414

Closed
elena-zh opened this issue Apr 19, 2022 · 6 comments
Closed
Assignees
Labels
app:CowSwap CowSwap app Bug Something isn't working Low Severity indicator for defects. It won't cause any major break-down of the system

Comments

@elena-zh
Copy link

Originally reported in #406 (comment) (case 2)

Network selector is enabled when connected to WC wallets (as an example, to Safe). So when I try to change a network, I see this warning message, that seems to me illogical as Safe wallet does not support networks changing. So can we disable this dropdown when connected to SC wallets at least?
image.png

@elena-zh elena-zh added app:CowSwap CowSwap app Bug Something isn't working Low Severity indicator for defects. It won't cause any major break-down of the system labels Apr 19, 2022
@W3stside
Copy link
Contributor

@alfetopito @anxolin @nenadV91 @fairlighteth thoughts?

@W3stside
Copy link
Contributor

i agree we should hide on sc wallet detection

@anxolin
Copy link
Contributor

anxolin commented Apr 27, 2022

Mmm, but doesn't other wallet that use wallet support to change the network using wallet connect?

What about Metamask mobile for example? If i'm not mistaken there.
Also, how does it behave with Minerva? see #233

@elena-zh
Copy link
Author

elena-zh commented Apr 28, 2022

  • With WC MM, Coinbase it network switcher works perfectly fine (in Mobile and in Desktop).
  • With TrustWallet, imToken, TokenPocket, Minerva in mobile: network selector navigates to the wallet, ant that it. In desktop simply nothing happens when press on a network in the selector
  • 1Inch: user sees this message when tries to change a network (both desktop and Mobile)
    image
  • Fortmatic: the app hangs when I try to change a network using selector.

So I'd disable the network selector like we currently do in the Production at all for those wallets that do not support network switching via WC, as the current implementation in the Uni merge works fine with MM and Coinbase wallets only.

@fairlighteth
Copy link
Contributor

I think there are different ways to read and understand ..you must change the network in your wallet..:

  1. You have to switch using a feature in your wallet. This is not always present and mentioned by others here.
  2. You have to connect with your wallet using a different network.

Both mean the same, but on the second point above you have to switch manually. E.g. with a Mainnet (smart contract) Safe wallet, you have to manually switch (use) another network and re-connect with WalletConnect. Much more (manual) steps involved but in the end it achieves the same.

Back to the network switcher: We intend that to trigger the 'switching' function on the client wallet's end. But this obviously doesn't work for each wallet.

In the end, we do want to signal and make it clear to the user what other networks we do support. This gives the users enough cues as to what they could do and allows them to identify and address the problem:

  • (manually) switch networks on their (smart contract) wallet
  • Consider using another wallet they might have, on the right network

So perhaps the message should try to address that in detail. For example:

Failed to automatically switch to the Ethereum network. In order to use CowSwap on Ethereum please manually change/use the network in your wallet.

Our messaging could also be made more specific in case we can detect the connected wallet client. E.g. we can detect it's a Safe and provide detailed steps there:

Failed to automatically switch to Ethereum. In order to use CowSwap on Ethereum please switch the network in your Safe and re-connect with WalletConnect.

@anxolin
Copy link
Contributor

anxolin commented Apr 29, 2022

We can know the connected wallet because WC report their name, so i think @fairlighteth proposal should be feasible.

Can we open a issue for all the wallets that this doesn't work on their side? They should address the problem, or if it's WalletConnect, they should try to sort it out with them.

I see hiding as a pragmatic solution, but i would avoid doing, its sad having to do so when this some wallets support it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
app:CowSwap CowSwap app Bug Something isn't working Low Severity indicator for defects. It won't cause any major break-down of the system
Projects
None yet
Development

No branches or pull requests

4 participants