Fix race condition with user-provided web3 #17

New Issue

2017-06-08T13:30:10Z

raucao commented

2017-06-08 13:30:10 +00:00

(Migrated from github.com)

There's currently a race condition where sometimes (for me ~50% or more) the user-provided web3 is loaded on window, after it has been checked for in the app. Which results in the app just using its own and pretending the user doesn't have a wallet.

There's currently a race condition where sometimes (for me ~50% or more) the user-provided web3 is loaded on `window`, after it has been checked for in the app. Which results in the app just using its own and pretending the user doesn't have a wallet.

bumi commented

2017-06-12 14:08:36 +00:00

(Migrated from github.com)

I was experimenting with that in an isolated app (not kredits-web) but where I could reproduce the error.
I could fix it there with something like:

window.addEventListener('load', function() {
  if(!window.web3) {
    $('#alert').html('no web3 provided. use metamask');
    return;
  }
  web3.eth.getAccounts(function(e, accounts) {
    if(e || accounts.length === 0) {
      $('#alert').html('please unlock your accounts');
      return;
    }
    // ...
  });
});

so onload we try to get the accounts, if we get an empty array as accounts it means the accounts/metamask is locked.

I was experimenting with that in an isolated app (not kredits-web) but where I could reproduce the error. I could fix it there with something like: ```javascript window.addEventListener('load', function() { if(!window.web3) { $('#alert').html('no web3 provided. use metamask'); return; } web3.eth.getAccounts(function(e, accounts) { if(e || accounts.length === 0) { $('#alert').html('please unlock your accounts'); return; } // ... }); }); ``` so onload we try to get the accounts, if we get an empty array as accounts it means the accounts/metamask is locked.

raucao commented

2017-06-12 15:11:06 +00:00

(Migrated from github.com)

Cool. However, I don't quite see how that helps with the race condition, as this issue is about web3 not being available on window when trying to set the Metamask-provided one in the service. The code you wrote is just using web3 from window later on, so that's the Metamask one after it has loaded.

But there's a different issue, which you did solve: when web3 is provided, but we get that empty array, we need to tell the user to unlock their account before taking any actions like e.g. voting. We should implement that as an enhancement in addition to solving the race condition.

One option for solving the race condition would be to actually just use it from window, of course, but then we still have the issue that our own code needs to reproducibly put it there first afaics.

Cool. However, I don't quite see how that helps with the race condition, as this issue is about web3 not being available on `window` when trying to set the Metamask-provided one in the service. The code you wrote is just using web3 from window later on, so that's the Metamask one after it has loaded. But there's a different issue, which you did solve: when web3 is provided, but we get that empty array, we need to tell the user to unlock their account before taking any actions like e.g. voting. We should implement that as an enhancement in addition to solving the race condition. One option for solving the race condition would be to actually just use it from window, of course, but then we still have the issue that our own code needs to reproducibly put it there first afaics.

bumi commented

2017-06-12 15:34:34 +00:00

(Migrated from github.com)

yeah kind of.
but I thought about initializing ember or the service later. as it seems we only can be sure that everything is working after some call like getAccounts.

for example I also had the issue of not getting the list of accounts when I am directly accessing web3.eth.accounts and not using the async method - it seems that was still too early.

yeah kind of. but I thought about initializing ember or the service later. as it seems we only can be sure that everything is working after some call like `getAccounts`. for example I also had the issue of not getting the list of accounts when I am directly accessing web3.eth.accounts and not using the async method - it seems that was still too early.

raucao commented

2017-06-12 16:11:14 +00:00

(Migrated from github.com)

~~We only need it to be unlocked when you want to do sth. At that point we can check and then tell the user to unlock it. No?~~

Ah, we also need it to set the current user. Although that could theoretically be done later.

@galfert is App.deferReadiness or whatever that was called still a thing?

<del>We only need it to be unlocked when you want to do sth. At that point we can check and then tell the user to unlock it. No?</del> Ah, we also need it to set the current user. Although that could theoretically be done later. @galfert is `App.deferReadiness` or whatever that was called still a thing?

galfert commented

2017-06-13 00:22:09 +00:00

(Migrated from github.com)

I was trying to have a look at this. But at the moment I just get an endless loading screen. I don't get any explicit errors in the console, just warnings about IPFS (i.e. "proposal from blockchain is missing IPFS hash").

Any ideas what's the problem here?

FYI: I had my Metamask unlocked and connected to the kosmos parity.

I was trying to have a look at this. But at the moment I just get an endless loading screen. I don't get any explicit errors in the console, just warnings about IPFS (i.e. "proposal from blockchain is missing IPFS hash"). Any ideas what's the problem here? FYI: I had my Metamask unlocked and connected to the kosmos parity.

raucao commented

2017-06-13 09:57:13 +00:00

(Migrated from github.com)

Those are only warnings, not errors. It's just the proposals that didn't have an IPFS hash stored (before I wrote all the IPFS stuff recently), as the message explains.

I also had an endless loading screen on Friday (see #kosmos-dev chan), when for some reason I couldn't connect to ipfs.kosmos.org via my VPN, while the normal connection worked just fine. I couldn't find out wth that was, as it should just be HTTPS.

Those are only warnings, not errors. It's just the proposals that didn't have an IPFS hash stored (before I wrote all the IPFS stuff recently), as the message explains. I also had an endless loading screen on Friday (see #kosmos-dev chan), when for some reason I couldn't connect to `ipfs.kosmos.org` via my VPN, while the normal connection worked just fine. I couldn't find out wth that was, as it should just be HTTPS.

galfert commented

2017-06-13 10:15:06 +00:00

(Migrated from github.com)

Yes, I thought as much. So it's probably one of the many Promises on the index route that doesn't resolve properly.

raucao commented

2017-06-13 10:42:10 +00:00

(Migrated from github.com)

It can only be a timeout somewhere, otherwise there would be an error or rejection. Are you certain that your connections to ipfs.kosmos.org are working fine, i.e. can you see the correct responses in the network panel?

bumi commented

2017-06-13 10:44:15 +00:00

(Migrated from github.com)

@galfert that is strange, haven't seen that yet without an exception. do you have the console log level set to "verbose" ?
also are you using metamask? what happens if you try to load it without metamask?

@galfert that is strange, haven't seen that yet without an exception. do you have the console log level set to "verbose" ? also are you using metamask? what happens if you try to load it without metamask?

bumi commented

2017-06-13 10:45:57 +00:00

(Migrated from github.com)

@skddc I did the account loading up there because I needed one async call. After that web3 was reliably available, before that it sometimes still failed for some reason. - very strange and I can not explain it.

raucao commented

2017-06-13 10:54:31 +00:00

(Migrated from github.com)

That's just luck then. There's no way to call a method on something that is undefined.

galfert commented

2017-06-13 10:56:18 +00:00

(Migrated from github.com)

Ok, it's the requests to my local IPFS node that just keep in pending state. If I use the Kosmos IPFS it's working.

bumi commented

2017-06-13 10:57:19 +00:00

(Migrated from github.com)

I am not calling something that is undefined. I just could confirm that once the callback of the first asyc call resolves web3 is fully loaded and usable.

bumi commented

2017-06-13 10:59:59 +00:00

(Migrated from github.com)

maybe related:
https://github.com/MetaMask/metamask-plugin/issues/1575
https://github.com/MetaMask/metamask-plugin/issues/1538

maybe related: https://github.com/MetaMask/metamask-plugin/issues/1575 https://github.com/MetaMask/metamask-plugin/issues/1538

galfert commented

2017-06-13 11:12:05 +00:00

(Migrated from github.com)

Ok, the advance and defer readiness magic seems to do the trick. Now I always see

[kredits] Using user-provided instance, e.g. from Mist browser or Metamask

in the logs.

I'm creating a PR for this.

Ok, the advance and defer readiness magic seems to do the trick. Now I always see > [kredits] Using user-provided instance, e.g. from Mist browser or Metamask in the logs. I'm creating a PR for this.

raucao commented

2017-06-13 11:18:52 +00:00

(Migrated from github.com)

Yay!

raucao commented

2017-06-13 12:30:22 +00:00

(Migrated from github.com)

@bumi Could you create a seperate issue for the accounts loading problem? I'll close this one from the PR that fixed the original race condition this one was for.

bumi commented

2017-06-13 12:40:07 +00:00

(Migrated from github.com)

you mean for the locked accounts?
up there the getAccounts call was only to try to "wait" for the loading.

you mean for the locked accounts? up there the `getAccounts` call was only to try to "wait" for the loading.

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: kredits/kredits-web#17