NetworkController: provider -> getProviderAndBlockTracker

[cryptodev-2s]

This aligns NetworkController more closely with that in the extension,
which create a proxy to the currently selected provider. This allows the
controller to maintain a persistent reference to a provider object even
if the network changes.

Closes#1018.

---

This change is breaking because it removes the provider property from NetworkController. Instead, a new method getProviderAndBlockTracker method is available for accessing the current provider object. This also exposes the block tracker object that the provider uses internally.

[mcmire]

I will move this back to draft until I've fixed the tests on this.

[legobeat]

One of these would make sense to me:

1. provider and block tracker are always set together, never independently.

• in this case, we want one method setting both together.

2. provider and block tracker are independent and can be set separately.

• in this case, we want independent setProvider and setBlockTracker.

1 seems to be the case here. Given that this is a private method only called only with provider._blockTracker as blockTracker argument, why not simplify it into #setProvider(provider)?

[mcmire]

Oops, sorry, I didn't see this before.

This method is the opposite of getProviderAndBlockTracker — where with that method, you can get both, here you can set both. So, given that getProviderAndBlockTracker is a public method, I think it makes sense to keep this naming. Also, the NetworkController in the extension already has this pair of methods, so this PR simply ports them over and doesn't revise the API. That said, once we bring both the extension NetworkController and core NetworkController in alignment API-wise and once we've fully switched to the core NetworkController, I think we will have an opportunity to take a step back and discuss how we can improve the existing API. And maybe at that point, we can work out a simpler way to get the block tracker. For instance, given that the block tracker is used by the provider, maybe we can expose a property on the provider to access the block tracker rather than keep a separate reference to it. So then we really could have setProvider and getProvider.

In any case, would you mind if we tabled this discussion for later? I don't want to dismiss your suggestion, I'm just not sure if we can make those changes in this PR.

[socket-security]

Socket Security Pull Request Report

👍 No new dependency issues detected in pull request

Pull request report summary

Issue	Status
Install scripts	✅ 0 issues
Native code	✅ 0 issues
Bin script shell injection	✅ 0 issues
Unresolved require	✅ 0 issues
Invalid package.json	✅ 0 issues
HTTP dependency	✅ 0 issues
Git dependency	✅ 0 issues
Potential typo squat	✅ 0 issues
Known Malware	✅ 0 issues
Telemetry	✅ 0 issues
Protestware/Troll package	✅ 0 issues

Bot Commands

To ignore an alert, reply with a comment starting with @SocketSecurity ignore followed by a space separated list of package-name@version specifiers. e.g. @SocketSecurity ignore foo@1.0.0 bar@2.4.2

Powered by socket.dev

[mcmire]

The provider property on NetworkController went away, so we can't use that as a type here. That said, I don't think this was right anyway, it's better to export a type from NetworkController.

[mcmire]

@Gudahtt In #1018 you had alluded to the networkDidChange event in the other network controller. Did you intend for this PR to include that event as well? If so, should we also bring over networkWillChange? As you know, methods are named differently in this network controller, and the code flowchart looks different, so I'm wondering whether it makes sense to come back and add that after the two controllers look a bit more similar.

[Gudahtt]

Hmm, I don't see where I referred to it. I agree that coming back to that makes sense though.

[mcmire]

In order to not make this any we'd have to backfill a lot of the types for web3-provider-engine. Given that we're about to replace that package, I decided to defer using a real type here until we've made that replacement.

[mcmire]

This is now ready for review again!

[Gudahtt]

LGTM!

It'd be great to have a few tests to cover the proxying behavior, like you added on the extension. We can add that in a future PR though.

[mcmire]

@Gudahtt Ah I didn't realize those tests were missing. Hmm alright, I'll make a new ticket for that.