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

[BUGFIX] Stale WalletConnect Session #1406

Closed
wants to merge 2 commits into from

Conversation

Da-Colon
Copy link
Contributor

@Da-Colon Da-Colon commented Mar 4, 2024

Description

Looking into MPCVault Wallet Connections, I noticed than after disconnecting from Wallets connected via WalletConnect. I am unable to reopen the wallet connect menu and the Console shows errors related to the session key not matching and then showing core/relayer errors.

After looking at the setup. It appears that what is happening is that for some reason after clicking disconnect that the menu closes and causes the func to end before updating local storage that the session has ended. This causes WAGMI and Walletconnect to desync.

Adding a small delay (setTimeout) after execution of disconnect fixes this issue.

Instead of using AppState to determine connection status, as this seems to be updated quicker than wagmi's state. switching out to use useWalletClient to determine connection status seems to be a better fix.

Testing

First see the error on dev:

  • Connect any wallet via WalletConnect and disconnect.
  • Reopen Connect Modal and Select Wallet Connect again. Should do nothing.

On deploy preview do the same thing and you should notice that you are able to click into the WalletConnect QR code again. (You may need to refresh browser after testing the error)

@Da-Colon Da-Colon self-assigned this Mar 4, 2024
Copy link

netlify bot commented Mar 4, 2024

Deploy Preview for fractal-framework-dev ready!

Name Link
🔨 Latest commit 9886b81
🔍 Latest deploy log https://app.netlify.com/sites/fractal-framework-dev/deploys/65e628ec3ebfbb000814e7f0
😎 Deploy Preview https://deploy-preview-1406.app.dev.fractalframework.xyz
📱 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 configuration.

Copy link
Member

@adamgall adamgall left a comment

Choose a reason for hiding this comment

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

I wouldn't think that this code actually delays anything from happening... the onClick handler is synchronous and so execution won't wait for the timer to complete.

@Da-Colon
Copy link
Contributor Author

Da-Colon commented Mar 4, 2024

I wouldn't think that this code actually delays anything from happening... the onClick handler is synchronous and so execution won't wait for the timer to complete.

@adamgall . Yeah it was for delaying the function from finishing. But this delay wasn't fixing the problem. I just edited the PR description and pushed a new commit but.

Instead of using AppState to determine connection status, as this seems to be updated quicker than wagmi's state. switching out to use useWalletClient to determine connection status seems to be a better fix.

@adamgall
Copy link
Member

adamgall commented Mar 4, 2024

@Da-Colon can you please open an issue for this PR, add it to the new Fractal project (yeah yeah i know, going back to just a single project board), and fill in the appropriate custom data?

@Da-Colon
Copy link
Contributor Author

Da-Colon commented Mar 4, 2024

@adamgall no worries can do. can you link the project to fractal-interface repo for easy access whenever you get a chance nvm I did it

@Da-Colon Da-Colon linked an issue Mar 4, 2024 that may be closed by this pull request
@adamgall adamgall self-requested a review March 4, 2024 22:45
@adamgall
Copy link
Member

adamgall commented Mar 4, 2024

@Da-Colon

First see the error on dev:

  1. Connect any wallet via WalletConnect and disconnect.
  2. Reopen Connect Modal and Select Wallet Connect again. Should do nothing.

This is not my experience on the dev site. I'm able to connect via WC, disconnect using the Fractal app, then immediately connect again with WC without a refresh.

@Da-Colon
Copy link
Contributor Author

Da-Colon commented Mar 5, 2024

@adamgall hmmm you are right, I'm not able to reproduce on dev...weird. it was very consistent and was easy to reproduce yesterday

@Da-Colon
Copy link
Contributor Author

Da-Colon commented Mar 5, 2024

Closing PR for now, currently not able to reproduce on dev. Will keep an eye out if it becomes an issue

@Da-Colon Da-Colon closed this Mar 5, 2024
@adamgall adamgall deleted the bugfix/wallet-connect-session-stale branch March 25, 2024 16:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Disconnecting WalletConnect causes LocalStorage desync
2 participants