Integrate RPC failover into NetworkController #5360
Conversation
mcmire
force-pushed
the
integrate-rpc-service-chain-into-nc
branch
from
5df00c8 to
0c25600
Compare
When configuring an Infura network, a list of failover endpoint URLs can now be supplied. If Infura is perceived to be down (after making 15 attempts to make a request, all of which either throw an error, return a non-200 response, or return a non-parseable response) then the request will be forwarded automatically to the first failover URL (using subsequent URLs as failovers for previous URLs). By default, all Infura networks will use Quicknode as a failover. Under the hood, the list of failover URLs are loaded into an RpcServiceChain (added in a previous commit) and this is then passed to the Infura middleware.
mcmire
force-pushed
the
integrate-rpc-service-chain-into-nc
branch
from
0c25600 to
e2b3b18
Compare
|
@metamaskbot publish-previews |
|
Preview builds have been published. See these instructions for more information about preview builds. Expand for full list of packages and versions. |
| : configuration.rpcUrl; | ||
| const failoverEndpointUrls = | ||
| configuration.type === NetworkClientType.Infura | ||
| ? configuration.failoverEndpointUrls |
There was a problem hiding this comment.
This property is new on the network client configuration. Now that we have it, we can feed it into an RpcServiceChain. This class will automatically handle the failover.
| @@ -345,119 +346,125 @@ export function testsForRpcMethodSupportingBlockParam( | |||
| }); | |||
| }); | |||
|
|
|||
| it('throws an error with a custom message if the request to the RPC endpoint returns a 405 response', async () => { | |||
There was a problem hiding this comment.
The diff here is pretty ugly, but I added a new test for each of the existing failure cases which exercise the failover behavior.
| async (primaryComms) => { | ||
| await withMockedCommunications( | ||
| { | ||
| providerType: 'custom', |
There was a problem hiding this comment.
It's kind of weird that we have to nest withMockedCommunications, and even weirder that we are supposedly treating the failover endpoint as a custom RPC endpoint. But this is just because the API for withMockedCommunications is awkward, and it would be out of scope to fix it, so I used what I had.
At the end of the day, though, what we are doing is mocking requests to two different endpoints, which should make sense as we now have the failover endpoint that we are hitting.
| @@ -2617,6 +2627,7 @@ export class NetworkController extends BaseController< | |||
| networkClientConfiguration: { | |||
| type: NetworkClientType.Infura, | |||
| network: infuraNetworkName, | |||
| failoverEndpointUrls: rpcEndpoint.failoverUrls, | |||
There was a problem hiding this comment.
There is a new property on InfuraRpcEndpoint and this is where that property is used to create the network client.
| @@ -387,6 +387,7 @@ describe('NetworkController', () => { | |||
| "nativeCurrency": "ETH", | |||
| "rpcEndpoints": Array [ | |||
| Object { | |||
| "failoverUrls": Array [], | |||
There was a problem hiding this comment.
There are a few kinds of changes being made to these tests:
- For tests which do not create a new NetworkController with a specific set of network configurations, we need to now verify that Infura RPC endpoints use an empty list of failover URLs.
- For tests which do create a new NetworkController with a specific set of network configurations, particularly if we are mocking
createNetworkClientand testing the presence of network configurations in state, we pass in an explicit set of failover RPC endpoints so we can use it for comparison. I guess in most cases we don't need to care about this, it's a lot of extra information, but that's how the tests are written.
| @@ -274,4 +279,707 @@ export function testsForRpcMethodsThatCheckForBlockHashInResponse( | |||
| }); | |||
| }); | |||
| } | |||
|
|
|||
| describe.each([ | |||
There was a problem hiding this comment.
This file (which is used to test two RPC methods) didn't have any tests around handling of responses. I originally wrote them for block-param.ts and no-block-param.ts and then copied them over.
| @@ -235,86 +236,456 @@ export function testsForRpcMethodAssumingNoBlockParam( | |||
| }); | |||
| }); | |||
|
|
|||
| it('throws a custom error if the request to the RPC endpoint returns a 405 response', async () => { | |||
There was a problem hiding this comment.
The changes to this file are a mirror of block-param.ts.
|
@metamaskbot publish-previews |
|
Preview builds have been published. See these instructions for more information about preview builds. Expand for full list of packages and versions. |
|
I'll need to update this PR to also support custom RPC endpoints. I'll request another approval when ready. |
| @@ -430,9 +425,8 @@ export async function waitForPromiseToBeFulfilledAfterRunningAllTimers( | |||
|
|
|||
| // `hasPromiseBeenFulfilled` is modified asynchronously. | |||
| /* eslint-disable-next-line no-unmodified-loop-condition */ | |||
| while (!hasPromiseBeenFulfilled && numTimesClockHasBeenAdvanced < 15) { | |||
| clock.runAll(); | |||
| await new Promise((resolve) => originalSetTimeout(resolve, 10)); | |||
There was a problem hiding this comment.
Making this change was necessary to get the new provider API tests I'm adding for the failover logic to work without running into timeout issues.
| btoa, | ||
| }) | ||
| .mockReturnValue(fakeNetworkClients[1]); | ||
| createNetworkClientMock.mockImplementation( |
There was a problem hiding this comment.
There were tons of tests that were doing an exact match when mocking the call to createNetworkClient (if the network client configuration is exactly this, return this fake network client; if it's exactly this, return another one; etc.). We don't need to do this; in most cases we can figure out which network client to return by doing a simpler check. I took the opportunity to clean this up as I was updating these tests. This also allows us to not worry about whether the RPC endpoints that we use to set up the test are configured with failover URLs or not.
Explanation
When configuring an Infura network, a list of failover endpoint URLs can now be supplied. If Infura is perceived to be down (after 15 unsuccessful attempts to make a request) then the request will be forwarded automatically to the first failover URL (using subsequent URLs as failovers for previous URLs). By default, all Infura networks will use Quicknode as a failover.
Under the hood, the list of failover URLs are loaded into an RpcServiceChain (added in a previous commit) and this is then passed to the Infura middleware.
References
Closes #4993.
Changelog
(Updated in PR.)
Checklist