search

cowprotocol / cowswap

🐮 CowSwap: First CoW Protocol UI
https://swap.cow.fi/
GNU General Public License v3.0
108 stars 73 forks source link

[3] fix: remove custom wallets #4420

Closed shoom3301 closed 1 week ago

shoom3301 commented 2 weeks ago

Summary

Context: https://cowservices.slack.com/archives/C0369B2UF6J/p1714648983313309?thread_ts=1714384014.599879&cid=C0369B2UF6J

Removed custom logic for wallets:

  1. Alpha
  2. Ambire
  3. Tally
  4. Trust
  5. Keystone

It means, we don't display separate buttons in the wallets list for them.

image

To Test

This PR includes https://github.com/cowprotocol/cowswap/pull/4414 and https://github.com/cowprotocol/cowswap/pull/4416

  1. All wallet-related functionalities: connect, disconnect, change wallet
  2. All chainId features: changing chainId in the URL, in wallet, etc.
  3. Add to Metamask
  4. Check all supported wallets (injected, walletconnect, hardware, smart-contract)
  5. Safe App integration
  6. Widget integration
  7. No injected providers
  8. Single injected provider
  9. Several injected providers
  10. Mobile wallet dapp browser should be auto-connected
vercel[bot] commented 2 weeks ago

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
cosmos ✅ Ready (Inspect) Visit Preview May 21, 2024 8:01am
cowfi 🔄 Building (Inspect) Visit Preview May 21, 2024 8:01am
explorer-dev 🔄 Building (Inspect) Visit Preview May 21, 2024 8:01am
swap-dev 🔄 Building (Inspect) Visit Preview May 21, 2024 8:01am
widget-configurator ✅ Ready (Inspect) Visit Preview May 21, 2024 8:01am
alfetopito commented 2 weeks ago

Have not checked the code yet, just the behaviour for now.

  1. When I open the page, it tries to connect metamask without any action from my side.

https://github.com/cowprotocol/cowswap/assets/43217/c6eaa815-805f-4dfd-8cd9-6e894943792a

  1. The app seems to think I'm connected to WC when I hover over the icon, but I'm not connected at all image
shoom3301 commented 2 weeks ago

@alfetopito thank you! Fixed

alfetopito commented 2 weeks ago

Working nice now!

image

Caveats, which might be on the wallet side, not sure:

  1. Taho is enable but doesn't show as an option I changed in the wallet setting to NOT be recognized as MM and it still didn't show up image

  2. Frame did not prompt the wallet This one can be on the wallet side as I was not connected to the desktop app, but still, no pop up or anything 🤷

Metamask, rabbit and coinbase worked perfectly

alfetopito commented 2 weeks ago

Oh and check the linter.

shoom3301 commented 2 weeks ago

Taho is enable but doesn't show as an option

Because it doesn't support EIP6963

Frame did not prompt the wallet

¯_(ツ)_/¯

shoom3301 commented 2 weeks ago

@elena-zh thank you!

  1. Fixed
  2. Fixed
  3. Fixed. Also added support of the feature for Rabby
  4. Fixed
  5. Fixed
  6. Fixed
  7. Fixed
  8. Fixed
  9. Fixed
  10. Fixed
  11. Fixed
  12. Was not able to reproduce
  13. Fixed. This is consequence of the broken auto-connection, you should not be able to disconnect/change wallet in the dapp browser
  14. Same with 13
  15. I see the same on prod. Actually seems already fixed. This icon cames from Safe manifest side
  16. Fixed
  17. Was not able to reproduce
  18. Was not able to reproduce
  19. Fixed
  20. Was not able to reproduce
  21. Was not able to reproduce
  22. It's not new, I remember we had an issue for it or at least one of integrators reported that
shoom3301 commented 2 weeks ago

@elena-zh I also removed some excessive logic related Argent and Ambire wallets, specifically logic is responsible for detecting if it's a smart-contract wallet. Could you check it please as well?

elena-zh commented 2 weeks ago

Hey @shoom3301 , thank you!

Issues that I see today:

  1. Connected with Injected is displayed when I'm conencted to MM image
  2. Order of wallets is not fixed (p.1 from the previous comment) image
  3. Coinbase option (when extension is disabled) calls regular WC modal instead their native modal image This one should be displayed image
  4. When I close this WC modal (from the step 3), and select CB option once again, the modal hangs in the Connecting state image
  5. I'm connected to MM. I press on the change wallet button and select Coinbase option. --> the app behaves like I'm connected to Coinbase wallet, but it is disabled in my extensions list image image
  6. I'm connected to Sepolia (from the previous case). I navigate to the Account page https://swap-dev-git-feat-clean-up-wallets-cowswap.vercel.app/#/account , Open Account modal, and press on the 'Change wallet', select MM --> the app requires me to swtich to Ethereum image When I cancel network switching request, I'm not reconnected to the new wallet.
  7. Add token to Rabby is not working on Sepolia image Also, not too clear to my why a token can't be added to Rabby if it exists there=) image
  8. I can see 'Add token to Trezor' that is not working image
  9. Change wallet from Rabby to MM when networks mismatch: the app requires to change a network in MM, otherwise it will not be connected. Currently, on Prod it picks the network that is set in a newly connected wallet. image
  10. (continuation of 9): I cancelled network change request in MM, and the modal highlighted to me all options with connected wallets. I'd hightlight only the currently active one image When I closed this modal, I appeared to be connected to MM!
  11. I've been connected to Rabby/MM/Trezor and Safe WC, after tests 9-10 (above) WC option stopped to work: https://www.loom.com/share/e4d4d8e0811f48b8a3d33b3faf44224f
  12. I've disconnected WC option from Safe, and initiated new conention. But the modal still shows me Rabby image image Refreshing helps.
  13. I was connected with Safe(WC), then I picked MM to change the wallet. When I approved network switch in MM, I got this: image
  14. When I switch between MM and Safe (WC), and then refresh the page this error appears in the console image

Thanks!

shoom3301 commented 2 weeks ago

@elena-zh

  1. Fixed
  2. EIP6963 doesn't guarantee an order of providers. We could hardcode some specific order, but it might be not fair and and will show that we are more inclined to certain providers. @anxolin wdyt?
  3. Couldn't reproduce
  4. Couldn't reproduce
  5. Couldn't reproduce
  6. Couldn't reproduce
  7. It's ok since it Sepolia (testnet)
  8. Fixed
  9. Fixed
  10. Couldn't reproduce (maybe because the previous point was fixed)
  11. Couldn't reproduce (I guess it may happen even on prod)
  12. Couldn't reproduce
  13. Fixed
  14. The error comes from WC side and it's specific only for Vercel. If it doesn't affect anything, please ignore it (ref)