fix: eth_accounts / accountsChanged behavior when wallet is locked #405
Conversation
| // EIP-1193 connect | ||
| this._handleConnect(chainId); | ||
| this._handleChainChanged({ chainId, networkVersion }); | ||
| this._handleUnlockStateChanged({ accounts, isUnlocked }); |
There was a problem hiding this comment.
i'm not confident we initialize the value of isUnlocked to true without this
There was a problem hiding this comment.
Meaning we should still get isUnlocked from deconstructing initial state
const { accounts, chainId, isUnlocked, networkVersion } = initialState;and set it in this function
this._state.isUnlocked = isUnlocked;Or just default to true ?
There was a problem hiding this comment.
Seems it's not necessary to set this after df0acbf
…r-eth_accounts-accountsChanged
adonesky1
force-pushed
the
fix/locked-wallet-behaviour-eth_accounts-accountsChanged
branch
from
2ecc8af to
e109526
Compare
Gudahtt
left a comment
Gudahtt
left a comment
There was a problem hiding this comment.
One minor suggestion, but LGTM!
| } | ||
| return this._state.isUnlocked; | ||
| }, | ||
| isUnlocked: async () => !this._state.isPermanentlyDisconnected, |
There was a problem hiding this comment.
Nit: Would be nice to have a unit test for this, I don't see one. I know it wasn't covered previously (so it seems), but it looks like it would be easy to add. And coverage is decent for the rest of the file otherwise.
There was a problem hiding this comment.
I'm finding it quite difficult to simulate the unrecoverable disconnection event to actually toggle the isPermanentlyDisconnected state... any ideas on how to achieve this?
There was a problem hiding this comment.
@adonesky1 seems not much has to be done since the state isPermanentlyDisconnected initializes to false on BaseProvider parent class, so we just assert the return is it's negated value
ffmcgee725
deleted the
fix/locked-wallet-behaviour-eth_accounts-accountsChanged
branch
This PR fixes #3853:
Currently when the user locks the wallet (either manually or by timer) the wallet emits an
accountsChangedevent with an empty array to all connected dapps. This is very confusing since it cannot be immediately distinguished from permissions being revoked, resulting in dapps not handling these two cases gracefully.We should stop emitting the
accountsChangedevent when the wallet is locked.This change addresses this by removing
handleUnlockStateChangedfunctionality from theBaseProvider, as it was decided it does not make sense to announce any account related information when the extension locks/unlocks:Extensions pull request that will be pulling this change for the fix: MetaMask/metamask-extension#30067
Extension working with updated providers lib:
Screen.Recording.2025-02-03.at.15.44.50.mov