-
Notifications
You must be signed in to change notification settings - Fork 58
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
feature/upgrade-wallet-library #1330
base: develop
Are you sure you want to change the base?
Conversation
✅ Deploy Preview for swapr ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
Tested, the condition is much better than it was, it is possible to connect different wallets, it works normally when swapping, it always opens the appropriate wallet and there is no more problem with changing. After several wallet changes, it happens that the indicators show that the user is connected to both: Also, after several wallet changes, it is not possible to disconnect all wallets, it is only possible to change Coinbase or Metamask. It also happens that sometimes the 'Change Wallet' button doesn't work and the 'Disconnect wallet' doesn't react after clicking: Swapr_05.08.webm |
Thank you @MilanVojnovic95 for tests and feedback!
This is expected behavior, consulted with Zett. Indicators show which wallet is connected with the dapp and indeed it is possible now to have more than one. This is the way how eg Uniswap works now.
Nice catch! I'll try to reproduce and fix it! |
Tested and looks very nice! Great job @karczuRF |
Great job 👏 I agree, it would be good to show which wallet is really in use. We can show the green bullets on the connected but the wallet in current use wallet could also have a different background or something, maybe @zamli has a suggestion. |
When using mobile version of Coinbase network in Swapr should be switched after user approve it in app |
In my case switching network both via Swapr and Coinbase Wallet App on phone works without need of approve. So I'm not sure if there's a problem from our side or Coinbase App. Anyway I've made some test and all works fine. Networks can be switched and transactions can be approved as well, see scr below. |
Still got that problem, it may differ for me because i use ios version of Coinbase wallet |
This might be the case. I'll investigate it then with ios. |
Still the same, I wasn't able to reproduce this also on ios. Indeed, during network switching app doesn't wait for user's approval, but this is the Coinbase Wallet case (checked for different apps like Uniswap). Except this I couldn't find any other issues. |
For now, i also don't see any major problems with this pr. I would accept that as mvp and merge it to develop |
fb69230
to
0c5053c
Compare
ff47650
to
b3b3815
Compare
91e4f23
to
af06da1
Compare
Summary
Upgrade web3-react library from v6 to v8. This should fix most of bugs/issues caused by multiple wallets used in the same time. Also prepare dapp for new wallets integration (like TallyHo!) and support for injected wallets (like Brave Wallet).
Upgrading can not cause regression, so swapr functionality and usage should be as expected and out of bugs.
Fixes #1440
Fixes #1133
Fixes #940
Fixes #699
Related to: #649 and #1169. Will be completed after this is merged.
Related but needs to be investigated: #1439
To Test
Background
For a long time swapr has been struggling with some wallet-dependant issues, like conflicts between injected connectors used the same time (Brave Wallet) or problems with network switching. After a few temporary fixes we decided to use the newest version of wallet library named web3-react. Although this one is still tagged as beta it is reccomended by authors to upgrade, mainly because of highly modular design, known bugs fixes and long term support (also for new third-party connectors). This means for us easier integration for wallets like TallyHo! or BitKeep.
More information can be found on web3-react github: https://github.com/Uniswap/web3-react