Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Create RPC middleware using RPC services #5290

Merged
merged 3 commits into from
Feb 14, 2025
Merged

Conversation

mcmire
Copy link
Contributor

@mcmire mcmire commented Feb 6, 2025

Explanation

When creating the middleware stack for a network, NetworkController makes use of functions from two packages for sending requests:

  • createInfuraMiddleware from @metamask/eth-json-rpc-infura for Infura networks
  • createFetchMiddleware from @metamask/eth-json-rpc-middleware for custom RPC endpoints

Currently, each of these middleware implements similar heuristics for determining when the network is down and retrying requests appropriately. We want to improve upon this logic so that we incorporate the circuit breaker pattern, exponential backoff, and jitter for retries, and, in the case of Infura, we can fail over to an alternate RPC endpoint when it is down. To make this happen, we have implemented an RpcService class which extracts all of this logic into one place. As a result, we can vastly simplify the Infura and fetch middleware creation functions so that they only need to take an RPC service and the RPC service will handle the rest.

To accommodate these changes, this commit:

  • Upgrades the @metamask/eth-json-rpc-infura and @metamask/eth-json-rpc-middleware packages to allow an RPC service to be passed.
  • Adds two new required options to NetworkController, fetch and btoa. These are also requirements of RpcService and allows consumers to pass in whatever versions of these functions is specific to their platform.
  • Updates createAutoManagedNetworkClient to take fetch and btoa and pass them along to createNetworkClient.
  • Updates createNetworkClient to create an RPC service and pass it along to the two middleware creation functions. Consequently, these functions now handle requests in exactly the same way.

References

Closes #4991.

Changelog

(Updated in PR.)

Checklist

  • I've updated the test suite for new or updated code as appropriate
  • I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate
  • I've highlighted breaking changes using the "BREAKING" category above as appropriate
  • I've prepared draft pull requests for clients and consumer packages to resolve any breaking changes

@mcmire mcmire force-pushed the integrate-rpc-failover-into-nc-2 branch from 5a4f6f1 to 62bea3b Compare February 6, 2025 18:41
@mcmire mcmire changed the base branch from main to include-node-fetch-errors-as-retriable February 6, 2025 21:19
@mcmire mcmire force-pushed the integrate-rpc-failover-into-nc-2 branch 2 times, most recently from 5b7e301 to 40fa2ca Compare February 6, 2025 21:25
Base automatically changed from include-node-fetch-errors-as-retriable to main February 12, 2025 16:43
@mcmire mcmire force-pushed the integrate-rpc-failover-into-nc-2 branch from 40fa2ca to eee47d9 Compare February 12, 2025 20:27
Copy link

socket-security bot commented Feb 12, 2025

Removed dependencies detected. Learn more about Socket for GitHub ↗︎

🚮 Removed packages: npm/@metamask/[email protected]

View full report↗︎

@mcmire mcmire changed the title WIP - Integrate RPC failover into NetworkController Create RPC middleware using RPC services Feb 12, 2025
@mcmire mcmire force-pushed the integrate-rpc-failover-into-nc-2 branch from eee47d9 to da61e84 Compare February 12, 2025 21:37
When creating the middleware stack for a network, NetworkController
makes use of functions from two packages for sending requests:

- `createInfuraMiddleware` from `@metamask/eth-json-rpc-infura` for Infura networks
- `createFetchMiddleware` from `@metamask/eth-json-rpc-middleware` for custom RPC endpoints

Currently, each of these middleware implements similar heuristics for
determining when the network is down and retrying requests
appropriately. We want to improve upon this logic so that we incorporate
the circuit breaker pattern, exponential backoff, and jitter for
retries, and we have implemented an RpcService class which extracts all
of this logic into one place. We can vastly simplify the Infura and
fetch middleware creation functions so that they only need to take an
RPC service and the RPC service will handle the rest.

To accommodate these changes, this commit:

- Upgrades the `@metamask/eth-json-rpc-infura` and
  `@metamask/eth-json-rpc-middleware` packages to allow an RPC service
  to be passed.
- Adds two new required options to NetworkController, `fetch` and
  `btoa`. These are also requirements of RpcService and allows consumers
  to pass in whatever versions of these functions is specific to their
  platform.
- Updates `createAutoManagedNetworkClient` to take `fetch` and `btoa`
  and pass them along to `createNetworkClient`.
- Updates `createNetworkClient` to create an RPC service and pass it
  along to the two middleware creation functions. Consequently, these
  functions now handle requests in exactly the same way.
@mcmire mcmire force-pushed the integrate-rpc-failover-into-nc-2 branch from da61e84 to e690c89 Compare February 12, 2025 21:41
@mcmire mcmire marked this pull request as ready for review February 12, 2025 21:46
@mcmire mcmire requested review from a team as code owners February 12, 2025 21:46
Copy link
Member

@mikesposito mikesposito left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work!

* @param args.fetch - A function that can be used to make an HTTP request,
* compatible with the Fetch API.
* @param args.btoa - A function that can be used to convert a binary string
* into base-64.
Copy link
Member

@mikesposito mikesposito Feb 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit:

Suggested change
* into base-64.
* into a base64-encoded ASCII string.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, makes sense. I plan on making a sort of cleanup PR later so I will make this change then.

Copy link
Contributor

@cryptodev-2s cryptodev-2s left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@mcmire mcmire enabled auto-merge (squash) February 13, 2025 22:27
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are lots of changes to tests here but this is because of the changes to the signature of the NetworkController constructor and also createNetworkClient.

@@ -385,167 +380,45 @@ export function testsForRpcMethodSupportingBlockParam(
});
});

// There is a difference in how we are testing the Infura middleware vs. the
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This commit consolidates the request handling logic between the Infura and fetch middleware, so they now have the same behavior, and as a result, we can simplify the tests that exercise the middleware.

@@ -195,6 +194,10 @@ function mockRpcCall({
// TODO: Replace `any` with type
// eslint-disable-next-line @typescript-eslint/no-explicit-any
return nockRequest.reply(httpStatus, (_, requestBody: any) => {
if (typeof completeResponse === 'string') {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In tests we now need the ability to completely override the response body to test for what happens when the response is not JSON-parseable.

@@ -265,114 +260,32 @@ export function testsForRpcMethodAssumingNoBlockParam(
});
});

// There is a difference in how we are testing the Infura middleware vs. the
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar as block-param.ts: We can simplify these tests because both the Infura and fetch middleware now handle requests in the same way.

* @param args.fetch - A function that can be used to make an HTTP request,
* compatible with the Fetch API.
* @param args.btoa - A function that can be used to convert a binary string
* into base-64.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, makes sense. I plan on making a sort of cleanup PR later so I will make this change then.

Copy link
Member

@OGPoyraz OGPoyraz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmation changes LGTM

@mcmire mcmire merged commit f8c8a90 into main Feb 14, 2025
133 checks passed
@mcmire mcmire deleted the integrate-rpc-failover-into-nc-2 branch February 14, 2025 16:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[network-controller] Integrate new RPC service
6 participants