fix: use up-to-date chainId/accounts when querying EIP1193-derived wallets #749
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
There was a problem hiding this comment.
wow this is really subtle, nice catch. i have a concern about the metamask change, see comment
the non-null assertions felt a little clunky but i went through them all and i didn't see anything problematic; it's maybe slightly worse because we're now relying on the coinbase wallet/metamask providers to set properties correctly, but doesn't really bother me
| // Wallets may resolve eth_chainId and hang on eth_accounts pending user interaction, which may include changing | ||
| // chains; they should be requested serially, with accounts first, so that the chainId can settle. | ||
| const accounts = await this.provider.request<string[]>({ method: 'eth_requestAccounts' }) | ||
| const chainId = await this.provider.request<string>({ method: 'eth_chainId' }) |
There was a problem hiding this comment.
nit: probably out of scope for this PR, but I would consider adding to a todo list: potential candidate for abstracting away? Looks like a common code path here and few lines earlier.
There was a problem hiding this comment.
I think that web3-react is the abstraction but, more importantly, I don't want to make this a large change because it's meant to be a bug fix.
packages/eip1193/src/index.ts
Outdated
| try { | ||
| // Wallets may resolve eth_chainId and hang on eth_accounts pending user interaction, which may include changing | ||
| // chains; they should be requested serially, with accounts first, so that the chainId can settle. | ||
| const accounts = await (eager ? this.provider.request({ method: 'eth_accounts' }) : this.provider |
There was a problem hiding this comment.
Question about this eager flag. I don't see it being used before? We were running eth_accounts in both instances. Is this change intentional?
There was a problem hiding this comment.
Ok, I see this has to do with eager that can be true or false, depending on the call-site. Is there a better way to describe its intent? Is it something like connect if already authorised vs explicitly authorise? In that case, why fallback to eth_accounts when eth_requestAccounts failed? This is probably a bit out of scope for this PR, but a good opportunity to expand my web3-react knowledge a bit!
There was a problem hiding this comment.
I believe this is done because EIP1193 does not necessarily support eth_requestAccounts, so a fallback is necessary. You don't see this in other connectors because they are not explicitly EIP1193 connectors.
The codepaths for connectEagerly and activate were identical except for the usage of eth_accounts vs eth_requestAccounts, so I added connect as a shared method to express that with less code duplication.
I've refactored to activateAccounts(requestAccounts: () => Promise<string[]>).
| .then(() => this.activate(desiredChainId)) | ||
| // Wallets may resolve eth_chainId and hang on eth_accounts pending user interaction, which may include changing | ||
| // chains; they should be requested serially, with accounts first, so that the chainId can settle. | ||
| const accounts = await this.provider.request({ method: 'eth_requestAccounts' }) as string[] |
There was a problem hiding this comment.
follow-up to my previous question: why no fallback to eth_accounts here?
There was a problem hiding this comment.
MetaMask is known to implement this method, whereas a pure EIP1193 may not.
| .then((chainId) => parseChainId(chainId)) | ||
|
|
||
| this.actions.update({ chainId, accounts }) | ||
| this.actions.update({ chainId: parseChainId(chainId), accounts }) |
There was a problem hiding this comment.
nit: maybe chainId should already be parsed? parseChainId(await ....), otherwise I think this line is a bit confusing: chainId: parseChainId(chainId) vs this.actions.update({ chainId })
There was a problem hiding this comment.
If considered a worthy nit, this most likely applies to other files too.
There was a problem hiding this comment.
fwiw, I agree with you here, but I don't want to change it because it expands the scope of this PR - I've omitted it to de-risk this bug fix.
Serially requests
accountsand thenchainId, so that if a user changes chains whilst connecting, the new chain (after connection /accounts) will be used. Practically, this prevents web3-react from using incorrect initial state when connecting to certain wallets, in certain cases where the user selects a non-active chain. This is resolved by waiting foraccountsto settle before requestingchainId.For
chainIdandaccounts, the values were fetched using aPromise.all. In some wallets,chainIdresolves immediately, and then the user is prompted to change chains. In these cases,chainIdwill be outdated by the timeaccountsresolves:eth_chainIdresolves to "stale" chainchainChangedis emittedeth_accountsresolves, resolving thePromise.allwith an out-of-datechainIdNB: WalletConnect did not exhibit this bug, but I updated it to keep it in sync with other connectors' idioms. It likely already had this "fix" in it, based on existing comments.