Introduce LegacyEthereumProvider interface

[mcmire]

In some controller tests it's useful to create a fake provider that conforms to the same interface as a "real" provider but doesn't actually hit the network. To make such a fake provider, however, we must create a class that extends SafeEventEmitterProvider. This is unfortunate as it introduces the possibility that the network could be hit accidentally if not all of the methods in the interface are appropriately overridden.

This commit introduces an interface that allows us to implement the same interface without having to extend it.

---

For a real-world example of where an interface could be useful, see: https://github.com/MetaMask/core/blob/e506abc29fcddd74591ac32f32a3c2bb0712178e/packages/network-controller/tests/fake-provider.ts#L85

An alternative solution to MetaMask/core#1974.

[Gudahtt]

To make such a fake provider, however, we must create a class that extends SafeEventEmitterProvider.

It seems that we could already do this today just using the type. We can use that today without extending anything. I might be missing something though.

[mcmire]

@Gudahtt Right, so in the case of FakeProvider, the reason why I don't think using the class as an interface will work is that SafeEventEmitterProvider must be instantiated with a JsonRpcEngine, but I don't want FakeProvider to take those arguments. So if I just replace extends SafeEventEmitterProvider with implements SafeEventEmitterProvider, then not only do I get an error because I can't call super() anymore, but I also get a general error. I believe this is the case because SafeEventEmitterProvider has private fields. This example on the TS playground sort of demonstrates the problem.

[Gudahtt]

Oh I see. Hmm. In that case, it seems like we don't really want a SafeEventEmitterProvider type, but rather a Provider type. If we omit the event omitter methods and the constructor, what's left are the two provider methods.

Perhaps the name of this type could reflect that? That would let us drop the leading I as well, which is nice, as that seemed unconventional for us.

[mcmire]

@Gudahtt Ah, right. We care about the event emitter part of the API — for instance, our provider in NetworkController needs to be properly typed so that we can pass it to swappable-obj-proxy — but Ethers doesn't care, and same goes for eth-query, now that I think about it. I'll rename it.

[mcmire]

Alright, I've replaced SimpleEventEmitterProvider with JsonRpcProvider.

By the way I wasn't sure what to name this type. I didn't want to name it Provider because it could clash with other Provider types that we have defined elsewhere (currently we have three types across this repo, detect-provider, and providers). I noticed that we consistently call the provider an "Ethereum provider" in the JSDocs, so I guess that could make sense, although there's nothing about the interface that's Ethereum-specific. If anything it's a pre-EIP-1159 legacy JSON-RPC provider API that only MetaMask uses. That's kind of a mouthful though.

[Gudahtt]

I'd consider it an Ethereum provider; the loose conventions that it follows are specific to the Ethereum ecosystem.

LegacyEthereumProvider maybe? 🤷 The name you chose seems OK too though.

[Gudahtt]

LGTM!

[mcmire]

@Gudahtt I thought about it some more and I like LegacyEthereumProvider. I think it's important to remember that while using this interface.

[mcmire]

Ah, I'd forgotten about this PR. I'll try to get this updated today.

[legobeat]

Had completely forgot about the existence of this PR when making MetaMask/utils#140

Do I get this right?

• types: export EIP1193Provider and LegacyEthereumProvider interfaces utils#140 and this PR are dealing with the same providers but different scope on "legacy" (here) and "standard" (types: export EIP1193Provider and LegacyEthereumProvider interfaces utils#140)
  • both a standard EIP1993Provider and a LegacyEthereumProvider make sense and classes could implement EthereumProvider = EIP1993Provider & LegacyEthereumProvider. They should be exported from the same package, whichever it is
  • EthQuery implements EthereumProvider and references to the EthQuery type can/should be replaced with this new interface

[jiexi]

I think LegacyEthereumProvider would match nicely with the other things we have prefixed with Legacy that are also provider related https://github.com/MetaMask/metamask-extension/blob/develop/app/scripts/contentscript.js#L42

Should we look back into phasing out send() and sendAsync()?

Looks like we tried to mark them deprecated at some point but ended up backing out MetaMask/providers#29

[mcmire]

Should we look back into phasing out send() and sendAsync()?

Yes. Since we always control the provider anywhere it's used, ideally we should not be using a legacy provider anywhere, we should make our provider EIP-1193-compatible. From there, we can either modify eth-query/ethjs-query to support such a provider or even drop them altogether and use Ethers everywhere, or whatever we want to end up doing down the road.

[mcmire]

I know we are working on creating unified types, but I think it would be worth it to have this type around in the meantime. My goal here isn't to unify anything, but merely to provide a lightweight version of the provider type that this package exports, so that if we want to make a fake provider in tests we can do so without having to subclass SafeEventEmitterProvider (which poses the risk that the network could actually be hit in tests).

So I'm proposing that we merge this PR first. Any thoughts on this? /cc @legobeat

[MajorLift]

This library has now been migrated into the core monorepo. This PR has been locked and this repo will be archived shortly. Going forward, any changes to this library will only be included in future releases if they are made in the core repo.

• Please push this branch to core and open a new PR there.
• Optionally, add a link pointing to the discussion here in the PR to provide context.