search

poanetwork / poa-dapps-validators

DApp for a list of validators with metadata for POA Network (Core/Sokol). Validators can update metadata using DApp.
https://validators.poa.network
GNU General Public License v3.0
13 stars 41 forks source link

Make network select more stable #112

Closed maxaleks closed 5 years ago

maxaleks commented 5 years ago

Added network select to have ability to switch the network in the app with and without injected web3

Also fixed:

Both old and new versions of MetaMask work the same way

The network of the app can be switched only using the in-app switcher

igorbarinov commented 5 years ago

@varasev @vbaranov please review when you have time

varasev commented 5 years ago

@maxaleks Please describe here all the scenarios of switching networks (what happens when we switch in the old version of MetaMask; when we switch in the new version of MetaMask; when we switch with a drop-down list of the DApp; when we switch to a non-POA network). This will help to understand what has changed compared to the current version in the master branch.

varasev commented 5 years ago

Also, it would be better to rebase it onto the master branch.

maxaleks commented 5 years ago

Also, it would be better to rebase it onto the master branch.

@varasev I'm not sure that other changes are good. My task was to fix the network select that is implemented on prepared-for-network-select branch

varasev commented 5 years ago

@maxaleks I used MetaMask 7.0.1 ☝️ Please test different cases of switching after code changing.

varasev commented 5 years ago

Steps to reproduce:

  1. Initially, I have the following set of networks in my MetaMask, and my selected network is Kovan:

  2. Run npm start and http://localhost:3000/poa-dapps-validators is opened. The selected network in the DApp is Kovan as expected.

  3. Add a new RPC address to MetaMask: https://sokol.poa.network. The network in the DApp is switched to Sokol as expected.

  4. Then I switch back to Kovan in MetaMask. DApp switched to Kovan as well.

  5. Then I switch to Sokol in MetaMask. DApp page is refreshed but displays Kovan network.

  6. I refresh the browser page but still see Kovan selected in DApp and Sokol selected in MetaMask.

maxaleks commented 5 years ago

Update: Now the network of the app can be switched only using the in-app switcher

varasev commented 5 years ago

@vbaranov Would you check this PR again? Or can we merge this immediately? Today I made here a few fixes and refactorings, tested this with poa-test-setup (using https://github.com/poanetwork/poa-test-setup/pull/76), and manually tested different scenarios. Everything looks fine. If you test this, please note that the list of changes was updated: https://github.com/poanetwork/poa-dapps-validators/pull/112#issue-303847125 (changes were simplified a bit).

vbaranov commented 5 years ago

@varasev I am on it

vbaranov commented 5 years ago

@varasev

The network of the app can be switched only using the in-app switcher

what is the reason for that?

varasev commented 5 years ago

The network of the app can be switched only using the in-app switcher

what is the reason for that?

@vbaranov As far as I understand, there were some troubles with switching networks in different MetaMask (and Nifty Wallet) versions, so we decided to remove the switching and add a check for netId matching when a transaction is about to be executed. @maxaleks can provide details of the problem

maxaleks commented 5 years ago

@vbaranov As far as I understand, there were some troubles with switching networks in different MetaMask (and Nifty Wallet) versions, so we decided to remove the switching and add a check for netId matching when a transaction is about to be executed. @maxaleks can provide details of the problem

In old versions of Metamask and NW the network switching reloads the page and in most cases the page is reloaded before we receive a network change event The task was to store the selected network, so in this case we can't determine if user switched the network or loaded the app with other network for the first time

vbaranov commented 5 years ago

@maxaleks

In old versions of Metamask and NW the network switching reloads the page and in most cases the page is reloaded before we receive a network change event

I tried today the latest version of NW and the currently deployed version of validators dapp and it perfectly switches between chains if I switch them in NW.

maxaleks commented 5 years ago

@vbaranov

I tried today the latest version of NW and the currently deployed version of validators dapp and it perfectly switches between chains if I switch them in NW.

The task was also to show the in-app network switch in both cases: with and without NW/Metamask. So there were conflicts when we allowed switching the network in the app and in NW/Metamask

varasev commented 5 years ago

What if we turn on the switching in MM/NW again like it is currently implemented in the current https://validators.poa.network/ ? I.e. when we switch the network in MM/NW, the network is automatically switched in App.

If we switch network in App, it won't be switched in MM/NW, but the alert will be shown if we try to make a transaction in a different network (as already implemented in this PR).

@maxaleks Would you expect any troubles in this case?

maxaleks commented 5 years ago

What if we turn on the switching in MM/NW again like it is currently implemented in the current https://validators.poa.network/ ? I.e. when we switch the network in MM/NW, the network is automatically switched in App. If we switch network in App, it won't be switched in MM/NW, but the alert will be shown if we try to make a transaction in a different network (as already implemented in this PR). @maxaleks Would you expect any troubles in this case?

Should we store the selected network?

  1. If I have Kovan selected in my Metamask and I switch the network to Sokol in the app, and then I reload the page manually, then which network should be selected after reloading?
  2. I have Kovan selected in both app and Metamask. Then I go to another dApp (our tab is not active) and switch the network to Sokol. The page of our app will be reloaded. Which network should be selected in this case?

Earlier we discussed that we should store the selected network, so these are those conflicts that I mentioned in the previous comment

varasev commented 5 years ago

The second case (when a user opens another DApp in another browser tab) is unlikely, so if the network is switched in MetaMask, we should switch it in DApp as well.

And, in general, it's better to automatically switch the network in DApp when it's switched in MetaMask (if that network is Core/Sokol/Kovan/xDai, i.e. supported by DApp).

But in the case when the user reloads DApp's page, the user would expect to see the same network they had seen before they reloaded the page (regardless of which network is chosen in MetaMask).

I think we could handle such a case with page reloading, couldn't we?

I.e. switch the network in DApp to MetaMask's if this is not a page reloading, but keep the network the same if this is a page reloading.

Is that possible to distinguish the cause of the current network's switching (reloading by the user or by MetaMask)?

If that's not possible, then I think the reloading is an unlikely operation, especially when a user just watches the content of the DApp and is not going to make any transactions. So, in general, we would just always switch the network in App automatically when it is occurred in MetaMask, and keep the current logic implemented in this PR (comparing netIds when needed).

varasev commented 5 years ago

In old versions of Metamask and NW the network switching reloads the page and in most cases the page is reloaded before we receive a network change event

That problem is only actual for old versions of MM/NW, right?

maxaleks commented 5 years ago

That problem is only actual for old versions of MM/NW, right?

For old version of MM and any version of NW

varasev commented 5 years ago

On the other hand, users won't use MetaMask's network switcher because now it's easier and faster to do that through the upper right drop-down list, so we could leave the current behavior since it seems to be better from UX/UI perspective (because it keeps the network when a user switches to another browser tab or reloads the page).

Another kind of user - validator - to make a transaction will switch the network in MetaMask only once, so it's infrequent operation.

I propose to leave the current implementation.

varasev commented 5 years ago

Another possible solution: automatically switch the network in App when it's switched in MM only when the current user is a validator (i.e. miningKey is not 0x000...000). @vbaranov what do you think?

varasev commented 5 years ago

Another possible solution: automatically switch the network in App when it's switched in MM only when the current user is a validator (i.e. miningKey is not 0x000...000).

Sorry, bad idea, because miningKey is always zero until we are in right network.

maxaleks commented 5 years ago

I agree with @varasev. Most users of our app don't need Metamask at all, because they don't send any transactions. So for these users it will be easier if they could switch network only in app

vbaranov commented 5 years ago

@maxaleks @varasev the last point makes to change my mind regarding the updates ✔️

6proof commented 5 years ago

As of time of this post, the switch at https://validators.poa.network/poa-dapps-validators seems to only work manually; no longer switches automatically based on RPC server selection. It would be nice to have the 'correct' page / network appear based on RPC server selection when first visiting page, with the ability to choose other networks manually via drop down selection.

varasev commented 5 years ago

@6proof OK, opened an issue for that: https://github.com/poanetwork/poa-dapps-validators/issues/115