open-contracts / open-contracts.github.io

The OpenContracts Interface
0 stars 1 forks source link

misleading MetaMask loading screen #16

Closed l-monninger closed 2 years ago

l-monninger commented 2 years ago

Close all windows of your browser. Then visit the website. You'll get a metamask error, until the user manually opens metamask, enters their password, and refreshed. Compare this behavior to that of /old, where Metamask pops up automatically once the user clicks 'load contract', even in a fresh browser session. The reason is that you're calling opencontracts = OpenContracts() right when the website loads, and (I think) metamask blocks the request on window load in recent versions. Instead, change the metamask connection prompt slightly: try to call opencontracts = OpenContracts() but if it fails, instead of the current error, display a connect metamask button, which calls opencontracts = OpenContracts() again. Only if that fails, display the error message. This will also make sure the 'connect metamask' button only displays when it is needed: in a fresh browser session.

l-monninger commented 2 years ago

@Jonas-Metzger I was unable to replicate this error on any browser. But, I can use Cypress to go deeper. I'll just need your browser, browser version, and a list of installed extensions.

window.OpenContracts() currently isn't being called until an attempt to load the Dapp is made. The check for a MetaMask account connection is performed separately from the protocol.

open-contracts commented 2 years ago

I don't know how you check MetaMask currently, but you're probably missing a step in the init logic. You should not implement your own web3 logic, this code duplication makes it hard to debug things.

Simply try calling var opencontracts = OpenContracts() on website load and if it succeeds, grab the address via await opencontracts.signer.getAddress(). If it doesn't succeed, catch this error and let it trigger the current metamask load error. Make sure the 'Retry' button only calls var opencontracts = OpenContracts() again (instead of refreshing the whole page).

I expect this to solve the issue, at least on the first retry button press.

open-contracts commented 2 years ago

Related to https://github.com/open-contracts/open-contracts.github.io/issues/21

open-contracts commented 2 years ago

so the error does not get thrown anymore, that's progress. but there's still some open questions.

  1. can we remove web3js from package.json now?
  2. OpenContracts() returns a promise which is either resolved to the opencontracts object, or eventually rejected with a clientError. Make sure you await it/catch it. Right now, browsers without metamask get stuck awaiting: image
l-monninger commented 2 years ago

I still need web3 for types.

l-monninger commented 2 years ago

Hmm... I believe I am catching everything I can through the protocol...

window.OpenContracts().then(()=>{
            resolve({
                wallet : "ready"
            })
        }).catch(()=>{
            reject({
                wallet : "failed"
            })
        });

I'll take another look at the protocol to see what might be up.

l-monninger commented 2 years ago

I also have a timeout btw.

l-monninger commented 2 years ago

So it is a bit strange that it doesn't fail.

open-contracts commented 2 years ago

what do you need web3js types for? everything should be ethers types, right?

open-contracts commented 2 years ago

Re the error, maybe try getting rid of the timeout. OpenContracts() already implements a 5 sec timeout after which the promise will be rejected.

l-monninger commented 2 years ago

I will double check on the types.

l-monninger commented 2 years ago

I think there may be couple issues here. Found one in the way that the check is being consumed. But, it doesn't actually solve the problem.

open-contracts commented 2 years ago

There must be a bunch of stuff happening on load given that this console error: image Is thown immediately, way before OpenContracts() rejects the promise. Maybe this stops the code execution and thus the handling of the actual reject. Would make most sense to await OpenContracts() 's resolve/reject before doing anything else.

l-monninger commented 2 years ago

Okay, for whatever reason, I had to listAccounts separately to properly check whether the use was logged in. This in combination with a different consumption of the check solved the problem on chrome. Will check brave and firefox.

l-monninger commented 2 years ago

Working for me on Brave and Firefox.

l-monninger commented 2 years ago

There must be a bunch of stuff happening on load given that this console error: image Is thown immediately, way before OpenContracts() rejects the promise. Maybe this stops the code execution and thus the handling of the actual reject. Would make most sense to await OpenContracts() 's resolve/reject before doing anything else.

So, I'm not using window.onload in any direct sense. I believe React initializes the virtual DOM on window.onload and starts its render cycling. But, I wouldn't think that would have the same effect.

open-contracts commented 2 years ago

image

Seems like the failed condition is triggered now, but the metamask loading screen doesn't get updated yet!

open-contracts commented 2 years ago

image

The opposite misbehavior happens when metamask is installed and opened bc of a fresh browser session - there we would like to keep the yellow awaiting screen while the user enters their pw. Here, I didn't see anything specific in the console.

l-monninger commented 2 years ago

@Jonas-Metzger Which browsers are these last two?

open-contracts commented 2 years ago

@l-monninger happened to be edge and firefox, but the last one also happens on brave

l-monninger commented 2 years ago

@Jonas-Metzger I still haven't been able to replicate this. Any thoughts?

open-contracts commented 2 years ago

Bro visit the site in an incognito tab (where metamask is off by default) on literally any browser. It will get stuck here: image somehow your "wallet failed" error does not get rendered properly