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

Fix/injected connector #817

Closed
wants to merge 5 commits into from
Closed

Fix/injected connector #817

wants to merge 5 commits into from

Conversation

karczuRF
Copy link
Contributor

Summary

Fixes #624 #683 #699

Bug Description
Activation of the Injected Connector (which is used with MetaMask and Coinbase wallets) results in displaying pop-ups from both wallets at the same time when the user has both extensions installed. Activation of the Caoinbase wallet doesn't cause the problem.

More details can be found as the description of solved issues mentioned above.

To Test

  1. Install MetaMask and Coinbase Wallet chrome extensions.
  • Button shows 'connect wallet'
  • Non of the wallet is connected automatically
  1. Click the "Connect Wallet" button.
  • If MetaMask is selected only MetaMask popup should be displayed
  • If Coinbase is selected only Coinbase popup should be displayed
  1. Proceed with connection process until wallet is Connected and address can be seen
  • the button in upper right corner and eg bridge button works as expected and open wallet list to be selected
    image

  • connect manually with unsupported network (eg Kovan) - switching from unsupported to supported network (eg. Mainnet) works as expected

  • try any transaction, like swap - works as expected and correct wallet popup is shown

  1. Click the "Disconnect Wallet" button and connect with different one.
  • only one popup appears and allows for connection
  • switching networks and transactions flow work as expected
  • important! addresses are different after wallet switching and matches to the wallets addresses appropriately
    image
    image
  1. Disconnect wallet manually. This HAS TO BE DONE MANUALLY and it's not the same as "Disconnect wallet" on our app (this is for purpose and it's not the bug!). To disconnect manually use Metamask and Coinbase wallet app as shown below.
    Try connect to the wallet again and Reject transaction.
    image
    image
  • Error modal should be shown
    image

  • "Try again" button works as expected and allow to try again

  • Closing modal and trying to activate another wallet works as expected

  1. Any "randomly" usage the app and wallets to test edge cases will be appreciated :)

@netlify
Copy link

netlify bot commented Mar 24, 2022

Deploy Preview for tender-kalam-456245 ready!

Name Link
🔨 Latest commit d94a27b
🔍 Latest deploy log https://app.netlify.com/sites/tender-kalam-456245/deploys/623cf4c20e71dd0008d36d7f
😎 Deploy Preview https://deploy-preview-817--tender-kalam-456245.netlify.app/
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@niemam29
Copy link
Contributor

niemam29 commented Mar 25, 2022

Still there are some problems - sometimes when user wants to connect coinbase wallet both of them are trying to connect and two popups are displayed. It occurs pretty often - either on the first try or as in the record below after connecting and disconnecting wallets. @karczuRF

screen-capture.2.mp4

@karczuRF
Copy link
Contributor Author

Still there are some problems - sometimes when user wants to connect coinbase wallet both of them are trying to connect and two popups are displayed. It occurs pretty often - either on the first try or as in the record below after connecting and disconnecting wallets. @karczuRF

That's interesting, because I wasn't able to reproduce this. I can't see the problem. I'll wait then until someone else can confirm what you showed above.

@niemam29
Copy link
Contributor

After computer restart there is no such bug - that is propably non reproductive error. For me looks good now, but would be perfect if some other QA take a deep look at this @MilanVojnovic95 @kingalg

@MilanVojnovic95
Copy link
Collaborator

MilanVojnovic95 commented Mar 31, 2022

Found a new bug.
I installed the Conbase wallet and connected to Swapr for the first time but after that it is not possible to connect the Metamask weallet whenever I try to connect to the Metamask wallet I get the message 'Error connecting.'
And when I click on 'Try Again' the same situation happens.
I do not receive any notifications on the Metamask that I should accept the connection.
Connecting to Coinbase Wallet is successful.
And there is no problem with calling a wallet that is selected in the 'Connect wallet' option for me.

@karczuRF
After installing Coinbase I went straight to the Swapr (without Metamask or some other wallet already connected) to connect and
after successful connection I was trying to switch networks and it was all ok.
Then I disconnected Coinbase wallet and tried to connect with Metamask and then
I get that error mentioned before, so I tried with hard refresh but that made no difference.

For this testing I was using Brave browser.

image

@karczuRF
Copy link
Contributor Author

karczuRF commented Apr 1, 2022

For this testing I was using Brave browser.

@MilanVojnovic95 thanks for testing! As I thought the problem you've found is because of Brave browser. Brave wallet "overrides" MetaMask and error occures, because action has to be done with Brave wallet, please see scr below.

image

For Brave wallet the issue is already created and I'll handle that asap: #649

Until Brave wallet is implemented it's possible to use Brave browser with MetaMask by disabling Brave wallet as default option.

image

I'll add temporary fix for that problem although I believe to solve all wallet related problems Brave and Tally wallets need to be implemented as well as package update, but it's another issue.

@adamazad
Copy link
Contributor

adamazad commented May 26, 2022

Hey @karczuRF, what is the status of this PR?

@karczuRF
Copy link
Contributor Author

Hey @karczuRF, what is the status of this PR?

It's suspended, because of two things:

  1. I've been asking but didn't get the answer which version of web3-react package should be used. At the moment we use v6, but it's not gonna be maintained anymore and it's suggested by the author moving to v8. The thing is v8 is described as beta version, that's why I have doubts and would be good if someone could approve upgrade. Anyway I would do that, because a lot of issues I'm trying to fix with this PR are already solved and also Brave problems should be fixed with package upgrade.
  2. Still waiting for Tally wallet to be merged. As long as it's not, I can't fully test and fix problems caused by multiple wallets, which was discussed under Tally PR - Add Tally wallet #652

@karczuRF karczuRF marked this pull request as draft June 13, 2022 11:56
@karczuRF
Copy link
Contributor Author

Current status changed to draft, because as it was discussed to best solution is to upgrade web3-react. Also no need to wait for Tally Wallet merge now.

@karczuRF
Copy link
Contributor Author

This will be closed with #1330.

@karczuRF karczuRF closed this Aug 25, 2022
@adamazad adamazad deleted the fix/injected-connector branch November 29, 2022 11:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants