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

Additional calls to provider.enable() should possibly re-prompt the user for account re-selection #8956

Closed
danfinlay opened this issue Jul 10, 2020 · 15 comments

Comments

@danfinlay
Copy link
Contributor

danfinlay commented Jul 10, 2020

This is a bit of a usability hitch for dapps that assume users aren't connected and make them connect on each load, reported by Roman Storm on Twitter.

If you call ethereum.enable() when a user has previously connected to your site, the user is not re-prompted for account selection a second time. This could lead a user who has switched their account and reloaded a page to be surprised what account is connected.

I let Roman know how he could detect previous connections, but this is more about Dapps that are doing a stateless-between-reload experience.

Anyways, an interesting design question, he may get more replies on that tweet with other opinions, we can consider it ourselves too.

EDIT: On deeper analysis we realized that a previously intended improvement to V8 already helps alleviate this major pain, so I'd like to share it here: #9020

@pi0neerpat
Copy link

I see his point that this is confusing, however:

  1. It will only be confusing briefly.
  2. It allows you to do "1 account, 1 dapp", which is actually very comforting!!!
  3. Account switching has never been simple, and no dapp does it well. This doesn't make things any worse.

Any wallet-switching funky-ness should 100% be handled with good dapp ux design, not with MetaMask. Even before the v8 release, we had already committed to building Spendless with "1 account, 1 dapp". Here is what we do, in case anyone wants some suggestions:

Howdy, it looks like you just unlocked your wallet. Way to go! We will ALWAYS use THIS account until you change it by hitting the DISCONNECT button. Don't remember which wallet you were using last week? Just head to /login and you'll see your last used wallet says "continue with" next to it. If all else fails, just hit that disconnect button and start over.

This means I never have to make a user "unlock" their wallets again. Do you click a modal every time you want to use Twitter? There is no reason to need to require an unlock just to interact with a dapp you've already used.

When a user wants to sign a transaction, I prepare it for them and simply "push" it to their wallet via MetaMask/WalletLink/WalletConnect. If they need to unlock their wallet, then they can do this immediately before signing, which doesn't feel like an extra step.

@wighawag
Copy link

To me it feels (did not try it yet, just saw the video, so I might change my opinion) the problem (and the confusing part) is that the plugin state is not reflected to the dapp.

as a user if I switch account, I expect the app to see that account (so if I did not authorize that app to see that account, it should show up as "locked") and not show the other account.
so if the app do .enable then the popup would ask to switch account (this unforrtunately means again asking which account but with the previous selection already chosen)

And to me the "locked" should be directly triggered with accountChanged

@wighawag
Copy link

Maybe the fix is to not be able to switch account in metamask plugin. and to simply pick the account you want to make a tx with every time. Balances would show up as a list (for each account)

this would match with what metamaskl expose to the application : a list of accounts

Because if the metamask plugin has a concept of "current account" it is logical to expect as a user that such account is also the one that application sees (or not see)

@pi0neerpat
Copy link

pi0neerpat commented Jul 10, 2020

@wighawag

the problem (and the confusing part) is that the plugin state is not reflected to the dapp

If a dapp wants to be optimized for multi-account users, it can be designed/built to reflect this. For example you can set a listener to detect account changes. However you bring up a good point that if the user switches account while the app is closed, the dapp would not catch this, and the user could not be warned, in the dapp, that accounts are mismatched.

@danfinlay what about adding a function to detect if the currently unlocked account matches the one with permissions for the dapp? This could be a quick fix until a more robust solution is built (if needed).

as a user if I switch account, I expect the app to see that account (so if I did not authorize that app to see that account, it should show up as "locked") and not show the other account

This is already true

Maybe the fix is to not be able to switch account in metamask plugin. and to simply pick the account you want to make a tx with every time.

ehhh... maybe instead of forcing an extra click on every transaction, MetaMask could display a "pre-flight check" message if accounts are mismatched. Rather than just be a little warning on the checkout screen, it should be its own screen that just says "Switch to Account 1 to continue" with a big "Continue" button. This would make things very obvious what is going on. And it only adds an extra click if absolutely necessary

@pi0neerpat
Copy link

pi0neerpat commented Jul 10, 2020

I just realized that I have no way to "disconnect" a user's account in MetaMask, like I do with WalletConnect and WalletLink. If a user wants to use a different account, they have to manually edit permissions in MetaMask to remove my dapp. It would be awesome to have a function for this I can call.

@tayvano
Copy link
Contributor

tayvano commented Jul 14, 2020

What are the goals?

Protect Globally: discourage behavior that creates glorious onchain dataset to be exploited. aka encourage users to have multiple accounts.

Protect from Dapps: dapps shouldnt get access to all my shit. I should know and opt in to what they do get access to.

"I know where I am" UX: it's inherent and obvious where I am, where I was, where I'm going if I click this button. It's obvious the relationship between MetaMask and the dapp I am on a vice versa.

The issue is related to the connection between MetaMask and dapps. RN what MetaMask shows is a single account. That is presumed to be what the dapp is looking at. But it's not always the case.

Yes you can add more pop ups and choices and cross check each time.

But MetaMask could also move away from this single account view entirely to be a list of accounts and the dapps they are connected to.

Then when I visit a dapp, I expect it to connect with the account it connected to last time. The one with the dapp's logo associated with it in my list. If that's not what I need, I need to switch accounts from the dapps UI. Just like I switch between my Google accounts. (Ps: it's also fucking stupid when I go from google drive to google docs and it moves me back to the first account on my list.)

Dapps can decide how to handle this UI. Do you only use one account at a time and switch (like Google) or does the dapp let you connect to multiple accounts simultaneously to grab your shit and help you with your taxes?

If the latter case if I want to send, the dapp is responsible for telling ME which account I'm sending from, likely allowing ME to choose. And then metamask is the "last line of defense" and shows me the account im sending from again.

This completely eliminates the issue where the account thats in the MetaMask pop up is not the one thats connected to the dapp. This is ideal IMO because MetaMask is not my wallet, its not my dapp, it's not where I go to learn of my balance or my positions or even to take actions. It's the place that keeps my keys safe, allows me to separate concerns, and encourages both me and dapps to behave in ways that create a safer, more private, more secure, etc. ecosystem.

MetaMask doesnt care. MetaMask gives dapps the information that I decide to give them. MetaMask signs and passes along transactions after presenting me with relevant information that allows me to interact with dapps without trusting them bc i trust MetaMask will show me what I'm about to send and not send anything with me approving it.

MetaMask should provide access to "global" or "high level" functionality that dapps may not incentivized to provide. I'm sure there are other examples, but revoking approvals for unlimited token sends is one function that I see as a MetaMask feature, not a dapp feature. Similarly, I should be able to revoke MetaMask's connection to a dapp as well if I want to do so for whatever reason.

Okay so basically

default mm view: account list. multiple accounts. with identicon, address, balance, connected dapps, click to go to single view.

single account view allows you to send, swap, revoke dapp access, view full balance breakdown, transaction history for just that address, etc. basically the default view now + some new stuff.

account component: identicon + label + address + dapps its connected to.

should eliminate some network switching ux issues as well. and, again, starts to encourage behavior where users add another account instead of using it with one thats connected to X platform already. and inherently reminds me not to short mycrypto's governance tokens from the mycrypto donation account or the account that is connected to my gitcoin. 😒

  • small updates to docs and examples for dapp developers so dapp developers start showing the user which account they are using...repeatedly. I dont know why but dapps are the worst at not showing me my address all the goddamn time and acting like i dont have 20 accounts in my metamask.

that should fix all the ux issues and not create new ones. 😉

Let me know if you want me to clarify any points above. I'm going to go to bed instead of editing this. 💖

@pi0neerpat
Copy link

@tayvano I think your comment suggests there needs to be more dapp developer effort here to create good user experience. Before my response, I think its helpful to classify types of developers

  1. Dapp Dabbler who "just wants to make it work" by using direct calls to web3.js or ethers.js
  2. Dapp Tinkerer who uses some web3-focused UI library to handle wallet status, signing flow, tx status (Dapp Hero, BlockNative, Dapparatus, web3-react, scaffold-eth, etc)
  3. Dapp Enthusiast who creates their own UI / UX to handle wallets.

I think most of the things you and I are discussing should really only impact type 3 developers, and the maintainers of the web3 UI tools in the type 2. For example:

when I visit a dapp, I expect it to connect with the account it connected to last time..... the dapp is responsible for telling ME which account I'm sending from, likely allowing ME to choose.

Hopefully. But, type 1 developers will call .enable() and not check accounts, or do any UI stuff. Type 2 rely on whatever the UI library does. And type 3 will hopefully do this....but they could miss some edge cases.

I think it makes sense to have two separate developer experiences for MetaMask.

  1. Normal mode: Let MetaMask take the UI/UX burden. This is for type 1 & 2 developers.
  2. Hard mode: Let the dapp developer take the UI/UX burden. More power and more responsibility for the type 3 developer. More risk if they fail.

This would hopefully mean that

  • A majority of new/concept dapps we see popping up will have much better UX, since developers are encouraged to stick to Normal mode.
  • There will be less dapps stuck in the void of trying to build their own UX, but failing. Attempts at custom UX can be abandoned earlier due to the ease of "falling-back" to Normal mode
  • The type 3 developers still have plenty of flexibility/functionality to play with in Hard mode

@pi0neerpat
Copy link

pi0neerpat commented Jul 14, 2020

I've created #8990 to give dapps developers the ability to disconnect accounts. I think this is a good and fast solution to implement until something else is designed.

Today during testing, a seasoned metamask user was befuddled because of this new change, so I think it definitely needs something to address the issue

@danfinlay
Copy link
Contributor Author

On deeper analysis we realized that a previously intended improvement to V8 already helps alleviate this major pain, so I'd like to share it here: #9020

@wighawag
Copy link

@danfinlay #9020 looks like a better approach indeed. It is actually what i expected to see when I heard about metamask keepin track of account per website/tab. But I think even with that it could result in unexpected behavior if metamask keep the idea of a current account in its UI.

After testing v8 I agree with my comment above and I would consider the current behavior a bug.

As a user if I switch account on metamask I expect it to be reflected in the app.
As it stands, metamask do not always emit the accountsChanged event (nor change the result of eth_accounts) when I change to a account. so app cannot reflect the changes.

More strangely it does emit the account if the account changed to was previously approved. This is in my opinion a confusing behavior.

On top of that the user get presented with a selection screen, that looks to me not user friendly. I think it would be better to let the application take care of that by changing the result of eth_accounts to be an empty array.

@wighawag
Copy link

Another side effect of the current behavior is the following :
when the app is connected to multiple accounts and you disconnect the active one, metamask will trigger accountChanged but choose the previously connected one.
Does not seems natural. why the last one ? I think it would be simpler again to simply return the empty array and let the app deal with that, requesting the user to reconnect. then metamask can offer again the choice of which account

@pi0neerpat
Copy link

@wighawag You could show the initial prompt again if things aren't perfect (act mismatch, or if they want to "disconnect"). That is what I've started to do here #8990 (comment)

@wighawag
Copy link

@pi0neerpat my point is the app is not even aware that the account changed on the metamask ui

@pi0neerpat
Copy link

Oh you are correct. I tried to use ethereum.selectedAddress() but it doesn't work anymore

@vandan
Copy link

vandan commented Jul 12, 2022

The enable() method has been "deprecated" and is not being actively supported so we are closing this issue.

@vandan vandan closed this as not planned Won't fix, can't repro, duplicate, stale Jul 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

8 participants