diff --git a/packages/network-controller/CHANGELOG.md b/packages/network-controller/CHANGELOG.md index e8ee19a6104..2f3452ddb05 100644 --- a/packages/network-controller/CHANGELOG.md +++ b/packages/network-controller/CHANGELOG.md @@ -7,6 +7,14 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Fixed + +- Improved handling of HTTP status codes to prevent unnecessary circuit breaker triggers ([#5767](https://github.com/MetaMask/core/pull/5767)) + - 405 (Method Not Allowed) now returns a JSON-RPC error with code -32601 (Method not found) + - 429 (Too Many Requests) now returns a JSON-RPC error with code -32005 (Request rate limit exceeded) + - Added retry delay information in 429 error responses when available via Retry-After header + - These status codes no longer trigger the circuit breaker, preventing unnecessary failover to backup endpoints + ## [23.4.0] ### Added diff --git a/packages/network-controller/src/rpc-service/rpc-service.test.ts b/packages/network-controller/src/rpc-service/rpc-service.test.ts index 2bc9c93ae03..e2d430cf52c 100644 --- a/packages/network-controller/src/rpc-service/rpc-service.test.ts +++ b/packages/network-controller/src/rpc-service/rpc-service.test.ts @@ -214,7 +214,9 @@ describe('RpcService', () => { method: 'eth_chainId', params: [], }; - await ignoreRejection(service.request(jsonRpcRequest)); + await expect(service.request(jsonRpcRequest)).rejects.toThrow( + 'Nock: No match for request', + ); expect(failoverService.request).not.toHaveBeenCalled(); }); @@ -236,13 +238,14 @@ describe('RpcService', () => { }); service.onBreak(onBreakListener); - const promise = service.request({ - id: 1, - jsonrpc: '2.0', - method: 'eth_chainId', - params: [], - }); - await ignoreRejection(promise); + await expect( + service.request({ + id: 1, + jsonrpc: '2.0', + method: 'eth_chainId', + params: [], + }), + ).rejects.toThrow('Nock: No match for request'); expect(onBreakListener).not.toHaveBeenCalled(); }); }); @@ -331,7 +334,7 @@ describe('RpcService', () => { ); describe('if the endpoint has a 405 response', () => { - it('throws a non-existent method error without retrying the request', async () => { + it('returns a method not supported error response for 405 status', async () => { const endpointUrl = 'https://rpc.example.chain'; nock(endpointUrl) .post('/', { @@ -347,15 +350,21 @@ describe('RpcService', () => { endpointUrl, }); - const promise = service.request({ + const response = await service.request({ id: 1, jsonrpc: '2.0', method: 'eth_unknownMethod', params: [], }); - await expect(promise).rejects.toThrow( - 'The method does not exist / is not available.', - ); + + expect(response).toStrictEqual({ + id: 1, + jsonrpc: '2.0', + error: { + code: -32601, + message: 'The method does not exist / is not available.', + }, + }); }); it('does not forward the request to a failover service if given one', async () => { @@ -382,7 +391,7 @@ describe('RpcService', () => { method: 'eth_unknownMethod', params: [], }; - await ignoreRejection(service.request(jsonRpcRequest)); + await service.request(jsonRpcRequest); expect(failoverService.request).not.toHaveBeenCalled(); }); @@ -404,19 +413,18 @@ describe('RpcService', () => { }); service.onBreak(onBreakListener); - const promise = service.request({ + await service.request({ id: 1, jsonrpc: '2.0', method: 'eth_unknownMethod', params: [], }); - await ignoreRejection(promise); expect(onBreakListener).not.toHaveBeenCalled(); }); }); describe('if the endpoint has a 429 response', () => { - it('throws a rate-limiting error without retrying the request', async () => { + it('returns a rate limit error response for 429 status', async () => { const endpointUrl = 'https://rpc.example.chain'; nock(endpointUrl) .post('/', { @@ -432,13 +440,62 @@ describe('RpcService', () => { endpointUrl, }); - const promise = service.request({ + const response = await service.request({ + id: 1, + jsonrpc: '2.0', + method: 'eth_chainId', + params: [], + }); + + expect(response).toStrictEqual({ + id: 1, + jsonrpc: '2.0', + error: { + code: -32005, + message: 'Request is being rate limited.', + data: { + retryAfter: 1000, + }, + }, + }); + }); + + it('includes retry delay from Retry-After header when available', async () => { + const endpointUrl = 'https://rpc.example.chain'; + nock(endpointUrl) + .post('/', { + id: 1, + jsonrpc: '2.0', + method: 'eth_chainId', + params: [], + }) + .reply(429, '', { + 'Retry-After': '5', + }); + const service = new RpcService({ + fetch, + btoa, + endpointUrl, + }); + + const response = await service.request({ id: 1, jsonrpc: '2.0', method: 'eth_chainId', params: [], }); - await expect(promise).rejects.toThrow('Request is being rate limited.'); + + expect(response).toStrictEqual({ + id: 1, + jsonrpc: '2.0', + error: { + code: -32005, + message: 'Request is being rate limited.', + data: { + retryAfter: 5000, + }, + }, + }); }); it('does not forward the request to a failover service if given one', async () => { @@ -465,7 +522,7 @@ describe('RpcService', () => { method: 'eth_chainId', params: [], }; - await ignoreRejection(service.request(jsonRpcRequest)); + await service.request(jsonRpcRequest); expect(failoverService.request).not.toHaveBeenCalled(); }); @@ -487,13 +544,12 @@ describe('RpcService', () => { }); service.onBreak(onBreakListener); - const promise = service.request({ + await service.request({ id: 1, jsonrpc: '2.0', method: 'eth_chainId', params: [], }); - await ignoreRejection(promise); expect(onBreakListener).not.toHaveBeenCalled(); }); }); diff --git a/packages/network-controller/src/rpc-service/rpc-service.ts b/packages/network-controller/src/rpc-service/rpc-service.ts index e2766fd2a08..7ff64009f87 100644 --- a/packages/network-controller/src/rpc-service/rpc-service.ts +++ b/packages/network-controller/src/rpc-service/rpc-service.ts @@ -333,12 +333,10 @@ export class RpcService implements AbstractRpcService { * @param jsonRpcRequest - The JSON-RPC request to send to the endpoint. * @param fetchOptions - An options bag for {@link fetch} which further * specifies the request. - * @returns The decoded JSON-RPC response from the endpoint. - * @throws A "method not found" error if the response status is 405. - * @throws A rate limiting error if the response HTTP status is 429. + * @returns The decoded JSON-RPC response from the endpoint. For 405 status, returns a JSON-RPC error response with code -32601. + * For 429 status, returns a JSON-RPC error response with code -32005. * @throws A timeout error if the response HTTP status is 503 or 504. - * @throws A generic error if the response HTTP status is not 2xx but also not - * 405, 429, 503, or 504. + * @throws A generic error if the response HTTP status is not 2xx and not one of the specifically handled status codes (405, 429, 503, 504). */ async request( jsonRpcRequest: JsonRpcRequest & { method: 'eth_getBlockByNumber' }, @@ -357,12 +355,10 @@ export class RpcService implements AbstractRpcService { * @param jsonRpcRequest - The JSON-RPC request to send to the endpoint. * @param fetchOptions - An options bag for {@link fetch} which further * specifies the request. - * @returns The decoded JSON-RPC response from the endpoint. - * @throws A "method not found" error if the response status is 405. - * @throws A rate limiting error if the response HTTP status is 429. + * @returns The decoded JSON-RPC response from the endpoint. For 405 status, returns a JSON-RPC error response with code -32601. + * For 429 status, returns a JSON-RPC error response with code -32005. * @throws A timeout error if the response HTTP status is 503 or 504. - * @throws A generic error if the response HTTP status is not 2xx but also not - * 405, 429, 503, or 504. + * @throws A generic error if the response HTTP status is not 2xx and not one of the specifically handled status codes (405, 429, 503, 504). */ async request( jsonRpcRequest: JsonRpcRequest, @@ -465,12 +461,10 @@ export class RpcService implements AbstractRpcService { * @param jsonRpcRequest - The JSON-RPC request to send to the endpoint. * @param fetchOptions - The options for `fetch`; will be combined with the * fetch options passed to the constructor - * @returns The decoded JSON-RPC response from the endpoint. - * @throws A "method not found" error if the response status is 405. - * @throws A rate limiting error if the response HTTP status is 429. + * @returns The decoded JSON-RPC response from the endpoint. For 405 status, returns a JSON-RPC error response with code -32601. + * For 429 status, returns a JSON-RPC error response with code -32005. * @throws A timeout error if the response HTTP status is 503 or 504. - * @throws A generic error if the response HTTP status is not 2xx but also not - * 405, 429, 503, or 504. + * @throws A generic error if the response HTTP status is not 2xx and not one of the specifically handled status codes (405, 429, 503, 504). */ async #executePolicy< Params extends JsonRpcParams, @@ -484,11 +478,31 @@ export class RpcService implements AbstractRpcService { const response = await this.#fetch(this.endpointUrl, fetchOptions); if (response.status === 405) { - throw rpcErrors.methodNotFound(); + return { + id: jsonRpcRequest.id, + jsonrpc: jsonRpcRequest.jsonrpc, + error: { + code: -32601, + message: 'The method does not exist / is not available.', + }, + }; } if (response.status === 429) { - throw rpcErrors.internal({ message: 'Request is being rate limited.' }); + const retryAfter = response.headers.get('Retry-After'); + const retryDelay = retryAfter ? parseInt(retryAfter, 10) * 1000 : 1000; + + return { + id: jsonRpcRequest.id, + jsonrpc: jsonRpcRequest.jsonrpc, + error: { + code: -32005, + message: 'Request is being rate limited.', + data: { + retryAfter: retryDelay, + }, + }, + }; } if (response.status === 503 || response.status === 504) {