cowprotocol / cowswap

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

[2] feat(wallets): support eip6963 multi-injected providers #4416

Closed shoom3301 closed 1 week ago

shoom3301 commented 2 weeks ago

Summary

Fixes #3987

The implementation is done following the specification: https://eips.ethereum.org/EIPS/eip-6963

Technical details

  1. Added multiInjectedProvidersAtom which contains all announced injected providers (commit)
  2. Added Eip6963Option component to display a button to connect Eip6963 provider (commit)
  3. Added selectedEip6963ProviderAtom which contains current selected Eip6963 provider. In the next commits it will be renamed to selectedEip6963ProviderRdnsAtom (commit)
  4. Updated useWalletMetaData() in order to support Eip6963 providers (commit)
  5. We used to have INJECTED and INJECTED_WIDGET connection types and it is very confusing. Because in fact, INJECTED is Metamask and INJECTED_WIDGET is any injected provider. Since now we don't have a special flow for Metamask and consider it as any other Eip6963 provider I removed INJECTED and renamed INJECTED_WIDGET -> INJECTED. Now we have only INJECTED and it covers any injected wallets (Eip6963, window.ethereum, widget) (commit)
  6. Added a migration to support the change above. The migration resets selectedWallet and selectedWalletBackfilled values in localStorage (commit)
  7. Enhanced useEagerlyConnect() to support Eip6963 providers. We have selectedWallet parameter in redux store and selectedEip6963ProviderRdnsAtom in jotai atom. Both values are persisted in localStorage. Now useEagerlyConnect() checks, if there is a selectedWallet and selectedEip6963ProviderRdns in localStorage, then we wait until selected Eip6963 provider is announced and then connect it (commit)
  8. We have separate connection buttons in the wallets list for Coinbase and Trust. But when there are injected providers for those wallets it doesn't make sense to display them. So I added conditions for the buttons displaying. P.s. Trust wallet icon will be deleted in the next PR (commit)

Functional changes

  1. When there is an inejcted Conibase wallet, then we don't display a special button for it. Otherwise, we display the special button which opens a modal with QR.
  2. Please ignore special Trust wallet icon, it will be deleted in the next PR. So, the wallet should work as regular EIP6963 provider.
  3. 3541 case was affected. I've tested it and it was good, but anyway, please take into account.

image

To Test

Please, do tests in the last PR of the waterfall: https://github.com/cowprotocol/cowswap/pull/4420

vercel[bot] commented 2 weeks ago

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

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

Have not reviewed yet, just based on the PR description, the item 6:

Added a migration to support the change above. The migration resets selectedWallet and selectedWalletBackfilled values in localStorage (commit)

Won't the hotfix to clear the local storage make this migration unreachable? My understanding is that the storage clearing happens on emergency.js, before the app is loaded. Then, when we look for the migration, there's no old state to clear as it was removed, no?

shoom3301 commented 1 week ago

Have not reviewed yet, just based on the PR description, the item 6:

Added a migration to support the change above. The migration resets selectedWallet and selectedWalletBackfilled values in localStorage (commit)

Won't the hotfix to clear the local storage make this migration unreachable? My understanding is that the storage clearing happens on emergency.js, before the app is loaded. Then, when we look for the migration, there's no old state to clear as it was removed, no?

The fix in emergency.js only fixes localStorage keys with versions. The migration in this PR is about redux_localstorage_simple_user localStorage key.