Immutable Providers means network changes go undetected

[Neurone]

I'm using ethers.js 4.0.33 and MetaMask, and I query the provider for block events with:

provider.on("block", (blockNumber) => { ... })

When I change the network ethers.js stucks and the application with it, and it seems true in particular when the difference between blocks in the network is very large. For example, first go with Goerli, 1M blocks, and then switch to Kovan, 12M blocks: the library tries to fire 11M of remaining events.

I think the problem is here: https://github.com/ethers-io/ethers.js/blob/master/src.ts/providers/base-provider.ts#L587-L589 and I think this should be fixed resetting all events when network changes.

To be more conservative, I tried to use the resetEventsBlock(blockNumber: number) function, but it seems it contains an error because it does not reset _emitted.block. I sent a PR to solve this, so you can do something like this: https://github.com/Neurone/mom/blob/development/src/index.js#L91-L93

[ricmoo]

Hmmm... This is an interesting issue. We basically need a provider to become mute once the network changes. Providers are immutable (including the network), and this can cause strange issues if we allow the same provider to be queried when the backend changes.

I'll think about this case too, as I already have a few related things to ponder.

Basically, I recommend in general using the default provider, connected to your network of choice, and use MetaMask as a Signer, and when the signer's network changes, respawn the Provider and set everything up. I'm working on a library to facilitate this into a more meaningful API.

Really, I think MetaMask and ilk should not permit non-homestead networks (or changing networks) unless they are switched into developer mode, as it can be dangerous for "normal" users to have access to (and have to think about) development networks. If only developers have testnets, many of these issues go away... Anyways, that is a topic for another day. I will think more about this, thanks. :)

[crazyrabbitLTC]

Checking in on this to see if there is any update.

[samajammin]

I'm also curious if there's any update on this. MetaMask has claimed that soon, "We will stop reloading the page on network change", so I'm wondering if there will be some ability for ethers.js to listen & adapt to network changes.

Thanks!

[ricmoo]

So, after working more on this and related things, I don't think this will be addressed in the Provider until ETH2.0 when there is a need for the Provider to be network-change aware.

I have made my case to MetaMask why it should refresh the page on network change, but I will provide a cookbook recipe to do this yourself too. A supplementary library I'm working on will also help with this, the ProviderManager which will be added to the experimental package.

The main reason I think pages should refresh is that this is only an issue developers have to deal with, normal users shouldn't ever be changing to testnets (and it can lead to uninformed users into being scammed). Dealing with changing networks is actually very difficult, with a lot of edge cases a developer must handle, and if even small mistakes are made, unhappy things will happen, and it becomes very difficult to debug/verify (and results in the developers' classic "well it works fine on my computer" and therefore "you must be using it wrong").

In my opinion, the ability to change networks should not even be active by default, in the same way the developer menu is hidden in a browser and requires a user to specifically undergo an action to enable it. Developer mode can very quickly damage a non-developers ability to understand their browser.

Obviously these are just my opinions, and for those who wish to work around it, a ProviderManager will make it less difficult.

[ricmoo]

(after reading my above comment, I realize I've repeated myself quite a bit... sorry.)

[Neurone]

@ricmoo I see your point about the overall design and the refresh page topic but, in the meanwhile, I think that an explicit reset by the developer can be a good compromise to avoid dapps that use ethers.js stop working when users change network (i.e. like I do here). To be effective, the resetEventsBlock function should be upgraded with a simple one line fix that you can find here #590.

[ricmoo]

The reset events block would need to do a few other things too. There are several things cached internally to the Provider to ensure that the dapp received a consistent view, in the face of querying an eventually consisted backend.

For example, if polling has getBlockNumber return 1234 and emits the "block" event for 1234, but the dev then calls getBlock(1234), if null is returned internally (because the first call to INFURA hit a node that was momentarily ahead of the second call), it stalls and waits for the node to return the block 1234. It stores these types of values internally. It would be confusing to a developer if they received a "block" event for 1234, but getBlock(1234) returned null. :)

The Provider also manages transaction hashes, receipts and logs in a similar way. If some tx with hash A is emitted as mined, but it isn't in the node's database yet, the call to getTransaction(A) will stall until it is. When the network changes, care needs to be considered as to what should happen to these caches and stalls.

Basically, it's really not an easy change to make, as it makes the Provider mutable in a way it wasn't designed for. I do have another library in the works to make this easier, and it is a lot of extra work that must be handled. I think that change you have may also break the FallbackProvider in v5, which has guards in place to prevent block rollback; it might indefinitely stall all provider operations to that backend.

The correct way in the current design to reset the provider, is to re-instantiate it and assign it. This is also fairly required to remain safe, since you don't want any existing signers or contracts with potentially in-flight transactions or calls to start on one network and end on another.

Quick example, imagine a simple "sweep app" that will query your balance of DAI, then send it all to a new address. Basically a call, followed by a send. If the provider is on testnet for the call and the mainnet for the send, unexpected behaviour would likely ensue. And I'm sure you can imagine much more insane examples. ;)

So, I would expect a developer that wishes to support multiple networks safely to have a design similar to:

function startApp(provider) {
  function networkChanged() {
    // Ideally here we would emit some event or do something
    // so inflight things will stop gracefully. Sending transactions
    // will abruptly stop working though, since we force the chain ID
    // into transactions using EIP-155, and those chain IDs will stop
    // matching the network; mostly.
    startApp(new ethers.providers.Web3Provider(window.ethereum));
  }

  // When the network changes, the above function must be called...

  // App logic here...
}

startApp(new ethers.providers.Web3Provider(window.ethereum));

Keep in mind this is just a stub; there are a lot of edge cases a developer must worry about. But there are ways to solve this correctly using ethers. It just means a dapp needs to be designed for network changes, which it will need to be if it wishes to handle network changes anyways.

If you really want to keep the Provider object reference static, and allow semi-unsafe operations, it would be fairly trivial to wrap it in a Proxy, with support for swapping out the underlying provider. I would advise against it, but it is an option to minimize code change. :)

I think a recipe to automatically refresh the page when the network changes is likely easier. Which I'll get added to the cookbook soon, because it is important...

One quick note; resetEventsBlock will be going away in a future version; it was only a hack put in place to support historic Contract queries, which is now much better supported in v5 with the contract.queryFilter API. :)

[Neurone]

Thanks for the extensive answer, I'll go for the refresh of the page then, because I agree it seems easier, cleaner and it does not need custom code to be implemented. I don't know if the "refresh page" trick can work for all the cases (i.e. if it's an angular app you will loose your current state) but for simple dapp like mine it is ok :)

[danfinlay]

it's really not an easy change to make, as it makes the Provider mutable in a way it wasn't designed for.

I have grown to agree with this position. I think we eagerly announced an endorsement for this change early on because some developers swore it was better developer experience, but in practice it can be dangerous:

Things like sending a transaction need to be fully explicit. If a user (or site!) could switch the network out from under a transaction request, the meaning of the transaction could change unpredictably, and that's plainly not safe.

[ricmoo]

As an aside, in ethers v5, the Provider can be instantiated with the network "any", which allows for the wildly unsafe mutable network. This is very much an opt-in, and developers who use it should be aware of all the caveats. I am working on the docs to cover all the issues now.

But as a result of making this change, it frees up the Provider to be far more draconianly secure and consistent (since there is a way to opt-out of draconian by opting into the wildly unsafe ;)).

Basically, if you created a Provider connected to a homestead backend, and that backend changes to ropsten, all operations (like getBalance) will fail and all event listeners will be suspended. If the network switches back, it can continue to work. Historically, the Provider would just keep on trucking, returning possibly inconsistent and unexpected data. Now developers have to deal with it. :)

I expect to have issues of confusion opened as a result, but I think this is far better. :)

See #495 #861 2bc7bb6

I'm actually going to close this issue now (cleaning up older issues a bit for the release next week) and recommend any further comments be put on #866. :)

Thanks! :)