Skip to content
This repository has been archived by the owner on Jun 24, 2022. It is now read-only.

fix for wallet change issue #1921

Closed
wants to merge 1 commit into from
Closed

Conversation

nenadV91
Copy link
Contributor

Summary

Fixes #1909

Fix for the issue with changing wallets and the previous connection with WalletConnect is not saved

@github-actions
Copy link
Contributor

  • 🔭 GP Swap: Gnosis Protocol v2 Swap UI

Copy link

@elena-zh elena-zh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @nenadV91 !

The wallet is still appears disconnected when change accounts. See the video: connection is removed from the connected wallet as soon as I change a wallet. When I get back, I can see my previously connected wallet details, but I will not able to run a transaction
https://watch.screencastify.com/v/JpMBCqFM5sEhMfVvn7l3

I have also encountered a new issue: the app does not see that a wallet is disconnected after refreshing the page.
Steps:

  1. Connect a WC wallet
  2. Reload the page
  3. Disconnect the app inside the connected wallet

For more details please see the video: https://watch.screencastify.com/v/dZeenarRgbaOugtKacI4

@nenadV91
Copy link
Contributor Author

Hmm good catch, so we are actually being disconnected from the WC when we change the wallet, so saving the data from the local storage doesn't actually make sense.

@elena-zh
Copy link

@nenadV91 , the 2nd case I reported is not related to this PR: it is reproducible on Prod now.
I created a separate issue for this case: #1923

const tmpWCdata = localStorage.getItem(WALLET_CONNECT_STORAGE_KEY)

if (tmpWCdata) {
setTimeout(() => localStorage.setItem(WALLET_CONNECT_STORAGE_KEY, tmpWCdata), 1500)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not in love with this but it does cover the issue.

i would open an issue with them and ask about:

  1. a constructor param or sth allowing to set whether we want this storage wipe feature
  2. them to fix their listeners .on('Web3ReactUpdate') to allow us to subscribe to changes there

Copy link
Contributor

@anxolin anxolin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why this PR is using master as a base branch?

If you are starting a hotfix, you should open a branch from master, and point this PR to this hotfix branch

@W3stside
Copy link
Contributor

W3stside commented Dec 6, 2021

@nenadV91 what's the status with this? last comment is 11 days ago. can we close?

@nenadV91
Copy link
Contributor Author

nenadV91 commented Dec 6, 2021

@W3stside As we said on the sync this will not be implemented for now and I think we can close.

@anxolin anxolin changed the base branch from master to develop January 20, 2022 18:09
@anxolin
Copy link
Contributor

anxolin commented Jan 20, 2022

@nenadV91 i changed the base branch to develop, since this has been open for a bit. Whenever we resume this one, should be merged there

@nenadV91 nenadV91 closed this Mar 15, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[1.5.0] WC wallet is disconnected when change wallet
4 participants