[Bug]: Wallet errors not propagated to onboard promises

[mario-iliev]

Current Behavior

Hey guys. I noticed something which makes handling errors impossible and lead to wrong/bad UX.
I only tested this with MetaMask and I can't confirm for other wallets but I suspect it's the same.
When using onboard.connectWallet({ autoSelect: { label: 'MetaMask', disableModals: true }, })

Case 1: User choose to connect with MetaMask, MM extension window is promped but user clicks outside of it so it gets hidden automatically. If the user tries to connect again, MetaMask fires this error MetaMask - RPC Error: Already processing eth_requestAccounts. Please wait. {code: -32002, message: 'Already processing eth_requestAccounts. Please wait.'} but the onboard.connectWallet Promise is not resolved so we can't show anything to the user and we actually don't know the current status of the "connectWallet" method.

Case 2: User choose to connect with MetaMask, MM extension is promped but user decides to cancel the connection. MM will fire this error MetaMask - RPC Error: User rejected the request. {code: 4001, message: 'User rejected the request.'} and the onboard.connectWallet will resolve with the currently connected wallets instead of rejecting with an error.

Expected Behavior

Wallet errors should propagate to web3-onboard Promises which caused them.

Steps To Reproduce

Steps to reproduce are described in the "Current Behavior" section.

What package is effected by this issue?

@web3-onboard/core

Is this a build or a runtime issue?

Runtime

Package Version

2.8.1

Node Version

16.14.2

What browsers are you seeing the problem on?

No response

Relevant log output

No response

Anything else?

No response

Sanity Check

• If this is a build issue, I have included my build config. If this is a runtime issue, I have included reproduction steps and/or a Minimal, Reproducible Example.

[taylorjdawson]

Hey @mario-iliev! Thank you for bringing this to our attention! Does this issue only occur for you when using: onboard.connectWallet({ autoSelect: { label: 'MetaMask', disableModals: true }, }) or does it also occur if simply calling: onboard.connectWallet()?

[mario-iliev]

I'm building custom modal for the initial connection. Calling web3Onboard.connectWallet() is simply opening the original Blocknative modal. I tried to continue the flow and click to connect to MetaMask trough the original modal and then reject the connection, but still I don't receive the MM error in the web3Onboard.connectWallet().catch()

[lnbc1QWFyb24]

@mario-iliev yes usually the Onboard UI will handle all errors and direct the user on what to do next and so the connectWallet function will not ever throw an error, but in the case of autoselecting a wallet with the modal disabled we may need to throw the error instead.

[mario-iliev]

@aaronbarnardsound I think that all wallet errors should propagate to the connectWallet Promise without special cases if we are using the "autoselecting" flag or not. Not being able to handle an error in a Promise caused by itself is causing a lot of problems.
Thank you for paying attention to this matter.

[lnbc1QWFyb24]

In the current design, the UI handles all of these errors, for example if the user rejects the connect attempt:

We handle the error and allow the user to re-trigger the request.

From the dev point of view this allows you to call the connect function and receive the updated (if a new one was connected) connected wallets and not have to worry about the UI state (ie is the modal still rendered, do I need to close the modal) and saves having to complicate the API by adding functions to manually control the state of the rendered modal.

In the edge case you mention above I do agree that we may need to implement some additional logic to handle it. My only concern is that it would be a breaking change as there would be apps that are not setup to catch the error.

[mario-iliev]

@aaronbarnardsound I agree. So it's going to be great if the error is propagated if connectWallet is used with disableModals: true because with this flag being true we should delegate the entire responsibility to the dev and he needs those errors.

[lnbc1QWFyb24]

Ok cool, we will take a look at getting a PR created that solves this soon.

[mario-iliev]

@aaronbarnardsound
I have another case which made me think if maybe another approach will be better: to have a global flag in the config upon onboard.init() which will allow all wallet providers errors to propagate to Blocknative promises which caused them.
What I've hit in another case: if I try to switch the current MetaMask network, by documentation we have const success = await onboard.setChain({ chainId: '0x89' }) which should resolve to true or false depending on the outcome. If MetaMask is prompted to change network but the user clicks Cancel then this promise returns false as expected. But if the network is not available in MetaMask then the change is cancelled and Blocknative initiates adding the network to MetaMask but if the user cancels it, the promise is never resolved.
In that case by the way there is a "Switch Chain" modal from Blocknative, which I'm not sure if it should appear since I'm using disableModals: true.

[lnbc1QWFyb24]

@mario-iliev Let me take a look at that example as that sounds more like a bug in that case. The disableModals flag should just apply to the connectWallet call and if a user rejects the request to add the new network, then that should return false for the setChain call.