search

huseyindeniz / vite-react-dapp-template

Vite React Template for dApp Frontend Development
https://huseyindeniz.github.io/react-dapp-template-documentation/
MIT License
9 stars 5 forks source link

Disconnecting wallets doesn't work properly #32

Closed oehm-smith closed 7 months ago

oehm-smith commented 8 months ago

Describe the bug

If you use the Disconnect button inside the Template's wallet, this seems to only disconnect it internally from the app. If you hit 'connect' again it will reconnect to the same account.

This is perfectly normal behavior.

To connect to a different account you need to disconnect using Metamask's > Connected Sites > disconnect.

Which works.

As soon as you do this, however the Metamask connect in the App pops up. It feels a bit intrusive and I think this should be prevented from happening.

The bug is that after disconnecting either way (App or MM), the EventChannel on listenAccountChange() in Wallet.ts (ethers 6 - I haven't tried 5) is not firing. Thus your app is not aware of the account disconnection.

I understand that you would most likely isProtected your page. But I don't think you should rely on that and I think the listener should be consistent and complete.

To Reproduce Steps to reproduce the behavior:

  1. Connect to account
  2. Go to metamask in browser and disconnect account from site
  3. In a Page that uses EventChannel on listenAccountChange(), the event doesnt fire so your page is unaware of the change.

Expected behavior Your pages that use EventChannel on listenAccountChange() should fire when an account is disconnected either in App or via Metamask.

Screenshots If applicable, add screenshots to help explain your problem.

Desktop (please complete the following information):

Smartphone (please complete the following information):

Additional context Add any other context about the problem here.

huseyindeniz commented 8 months ago

hi and thanks for the detailed description. I think I know this problem. Like you said, Metamask's "disconnect from site" action doesn't trigger the event that listenAccountChange method listening. I can't remember now, but most probably I couldn't find a way to handle this case at that time while using ethersV5. I'll check this when I have time.

oehm-smith commented 7 months ago

Hi @huseyindeniz , this is biting me. Can you tell me where you think the problem lies and I'll have a look?

More than the disconnect problem, vite-react-dapp-template seems to maintain a connected state when it isn't. I disconnect wallet from the site in MetaMask and hit connect in the app. And it does so. But when I go back to MM it says "not connected to any site". And I can't seem to change the wallet it is connecting to.

I see you have rolled your own. Did you think about using an existing library like https://docs.walletconnect.com/web3modal/about? I've had issues using this but that was in a raw .js context, and the problems I had were easily solved with React features such as 'useEffect'.

There is also https://docs.walletconnect.com/advanced/walletconnectmodal which I'm sure is what I was working with a couple months ago, however it has either changed to be for native apps only or I was completely missunderstanding it! https://docs.walletconnect.com/advanced/walletconnectmodal/usage?platform=react-native.

NOPE, it was web3modal I was using. But that is an alternative you might like to consider if you are working on native apps.

The only way I can connect to a different wallet is go in to Brave > Dev Tools > Application > Storage and clear it. The only persistence I can find for wallet being used is sign in with ethereum cookie - siwe-notepad-session

Investigating:

src/features/wallet/components/ConnectionModal/CheckWallet/CheckWallet.tsx > CheckWallet - when I've disconnected from Site and in MM, then the following states are hit:

case ProviderLoadState.REQUESTED:
      return null;
case ProviderLoadState.INITIALIZED:
      return null;

Happens - maybe these should be doing something here.

I also notice that hitting 'disconnect' in the UI doesn't hit any state in CheckWallet.

huseyindeniz commented 7 months ago

hi @oehm-smith , this UX issue is also bothering me, but I'm not sure whether it's a "web3 wallet's UX issue" or "this template's login flow issue". Let me explain.

When a user connects to a dApp with their Web3 wallet:

So, in this template, or in other dApps, when you click disconnect in the dApp, your account never disconnected from the dApp in the Web3 wallet. Only the dapp's current internal connection cleared.

If you look at the recordings below, you will see that when users click disconnect inside the dApp, it only clears the dApp connection state, not web3 wallet's. But in the second example, there is a problem. It doesn't listen to account changes correctly. And because of this, users could think they connected with their account A but dApp connected to account B. This problem is exists in almost every dApp, because almost all of them use those external web3 modal packages. This is actually the reason I wrote my own. Normally, I prefer delegating these kinds of things to the dedicated packages, but current Web3 login packages have lots of problems.

As a summary, could you record a session so I can see exactly what part is confusing users in my implementation. I agree that there is a scenario that confuses web3 users, but I can't identify the exact problem. I mean, this issue was a bug at the beginning but I solved that bug in this issue: https://github.com/huseyindeniz/vite-react-dapp-template/issues/24 . Now it's more like a design issue.

Note: All the business logic of this flow is under

src/features/wallet/components is UI logic of this flow.

https://github.com/huseyindeniz/vite-react-dapp-template/assets/1695352/58ec0e28-b35e-455c-aeac-6a496b276cbf

https://github.com/huseyindeniz/vite-react-dapp-template/assets/1695352/663071b3-e858-45c5-a539-9712bc52fcc9

oehm-smith commented 7 months ago

I think my problem was related to MM. It somehow became stuck on one account and I couldn't disconnect it. Hence my having to go in to application settings to remove data. But i can't reproduce that now, however I will keep an eye on it and update this or a new issue if I see again.

When it is working properly, as it is now, it makes sense.

What's confusing is - and this is not a problem with this app, but with dapps and wallets in general - connecting to multiple accounts. If you disconnect in the app, I feel that it should give you the choice of which of the connected wallets to connect to. It is not a big problem - it is just that you need to manage the connections in MM. It feels unintuitive. Again, a wallet problem and not your dapp.

Now that I worked out my state got messed up and I've played around with it more, I'm happy with what I'm seeing :). I saw one glitch when I disconnected in MM and it didn't disconnect in the app, but like the other example I can't reproduce this either.

I will close this issue and reopen if I have any solid evidence of a problem.

huseyindeniz commented 7 months ago

Actually, I think there is still a problem in the flow, and I can't reproduce it either. But sometimes login flow can fail exactly because of this issue. I mean, sometimes I need to correct things inside the Web3 wallet manually to fix the problem. But I agree; we can close this for now. When the issue is more clean and consistently reproducible, it can be opened again. Thanks for your effort on this one.