From e1dfb2c0a9ffe0d313b09f9c1e1e321b9ecd2cd8 Mon Sep 17 00:00:00 2001 From: Loris Leiva Date: Tue, 10 Sep 2024 08:53:27 +0100 Subject: [PATCH] Make `RpcResponse` provide the resopnse payload directly (#3185) This PR reverts some of the changes in the previous stack by making the following key change: ```ts // Before. type RpcResponse = { readonly json: () => Promise; readonly text: () => Promise; }; // After. type RpcResponse = TResponse; ``` This is because we no longer believe that it is the responsibility of the RPC API to affect the response as text or JSON. This aspect should be encapsulated by the RPC Transport layer, meaning any custom JSON parsing/stringifying will need to be done at the RPC Transport layer. Note that, we could simply delete the `RpcResponse` type since it returns its type parameter directly but keeping it makes the code clearer as we will use `RpcResponse` instead of `unknown` everywhere in the codebase. --- .changeset/light-bugs-compare.md | 2 +- .../rpc-api/src/__tests__/get-health-test.ts | 11 +--- packages/rpc-spec/README.md | 21 +------ .../rpc-spec/src/__tests__/rpc-shared-test.ts | 56 ------------------- packages/rpc-spec/src/__tests__/rpc-test.ts | 16 ++---- packages/rpc-spec/src/rpc-shared.ts | 19 +------ packages/rpc-spec/src/rpc.ts | 3 +- .../__tests__/response-transformer-test.ts | 43 +++++--------- .../src/response-transformer-result.ts | 6 +- ...response-transformer-throw-solana-error.ts | 8 +-- .../rpc-transformers/src/tree-traversal.ts | 11 +--- .../__tests__/http-transport-abort-test.ts | 5 +- .../rpc-transport-http/src/http-transport.ts | 5 +- .../__tests__/rpc-request-coalescer-test.ts | 19 ++----- 14 files changed, 41 insertions(+), 184 deletions(-) delete mode 100644 packages/rpc-spec/src/__tests__/rpc-shared-test.ts diff --git a/.changeset/light-bugs-compare.md b/.changeset/light-bugs-compare.md index 216e8add1b38..6ae264a9c0d7 100644 --- a/.changeset/light-bugs-compare.md +++ b/.changeset/light-bugs-compare.md @@ -2,4 +2,4 @@ '@solana/rpc-spec': patch --- -Add new `RpcRequest` and `RpcResponse` types with `RpcRequestTransformer`, `RpcResponseTransformer` and `createJsonRpcResponseTransformer` functions +Add new `RpcRequest` and `RpcResponse` types with `RpcRequestTransformer` and `RpcResponseTransformer` functions diff --git a/packages/rpc-api/src/__tests__/get-health-test.ts b/packages/rpc-api/src/__tests__/get-health-test.ts index 68ebc81c4896..e93b3614f051 100644 --- a/packages/rpc-api/src/__tests__/get-health-test.ts +++ b/packages/rpc-api/src/__tests__/get-health-test.ts @@ -1,16 +1,9 @@ import { SOLANA_ERROR__JSON_RPC__SERVER_ERROR_NODE_UNHEALTHY, SolanaError } from '@solana/errors'; -import { createRpc, type Rpc, type RpcResponse } from '@solana/rpc-spec'; +import { createRpc, type Rpc } from '@solana/rpc-spec'; import { createSolanaRpcApi, GetHealthApi } from '../index'; import { createLocalhostSolanaRpc } from './__setup__'; -function createMockResponse(jsonResponse: T): RpcResponse { - return { - json: () => Promise.resolve(jsonResponse), - text: () => Promise.resolve(JSON.stringify(jsonResponse)), - }; -} - describe('getHealth', () => { describe('when the node is healthy', () => { let rpc: Rpc; @@ -36,7 +29,7 @@ describe('getHealth', () => { beforeEach(() => { rpc = createRpc({ api: createSolanaRpcApi(), - transport: jest.fn().mockResolvedValue(createMockResponse({ error: errorObject })), + transport: jest.fn().mockResolvedValue({ error: errorObject }), }); }); it('returns an error message', async () => { diff --git a/packages/rpc-spec/README.md b/packages/rpc-spec/README.md index a8606330632f..79d21cb24ebf 100644 --- a/packages/rpc-spec/README.md +++ b/packages/rpc-spec/README.md @@ -54,12 +54,7 @@ A function that accepts an `RpcRequest` and returns another `RpcRequest`. This a ### `RpcResponse` -An object that represents the response from a JSON RPC server. It contains two asynchronous methods that can be used to access the response data: - -- `await response.json()`: Returns the data as a JSON object. -- `await response.text()`: Returns the data, unparsed, as a JSON string. - -This allows the `RpcApi` to decide whether they want the parsed JSON object or the raw JSON string. Ultimately, the `json` method will be used by the `Rpc` to provide the final response to the caller. +A type that represents the response from a JSON RPC server. This could be any sort of data which is why `RpcResponse` defaults to `unknown`. You may use a type parameter to specify the shape of the response — e.g. `RpcResponse<{ result: number }>`. ### `RpcResponseTransformer` @@ -140,17 +135,3 @@ A config object with the following properties: - `requestTransformer(request: RpcRequest): RpcRequest`: An optional function that transforms the `RpcRequest` before it is sent to the JSON RPC server. - `responseTransformer(response: RpcResponse, request: RpcRequest): RpcResponse`: An optional function that transforms the `RpcResponse` before it is returned to the caller. - -### `createJsonRpcResponseTransformer(jsonTransformer)` - -Creates an `RpcResponseTransformer` function from a function that transforms any JSON value to a value of type `T` by wrapping it in a `json` async function. - -```ts -const getResultTransformer = createJsonRpcResponseTransformer((json: unknown): unknown => { - return (json as { result: unknown }).result; -}); -``` - -#### Arguments - -- `jsonTransformer: (json: unknown, request: RpcRequest) => T`: A function that transforms an unknown JSON value to a value of type `T`. diff --git a/packages/rpc-spec/src/__tests__/rpc-shared-test.ts b/packages/rpc-spec/src/__tests__/rpc-shared-test.ts deleted file mode 100644 index cdb580cc00da..000000000000 --- a/packages/rpc-spec/src/__tests__/rpc-shared-test.ts +++ /dev/null @@ -1,56 +0,0 @@ -import '@solana/test-matchers/toBeFrozenObject'; - -import { createJsonRpcResponseTransformer, RpcRequest, RpcResponse } from '../rpc-shared'; - -describe('createJsonRpcResponseTransformer', () => { - it('can alter the value of the json Promise', async () => { - expect.assertions(1); - - // Given a request and a response that returns a number. - const request = { methodName: 'someMethod', params: [123] }; - const response = { - json: () => Promise.resolve(123), - text: () => Promise.resolve('123'), - }; - - // When we create a JSON transformer that doubles the number. - const transformer = createJsonRpcResponseTransformer((json: unknown) => (json as number) * 2); - - // Then the transformed response should return the doubled number. - const transformedResponse = transformer(response, request); - transformedResponse satisfies RpcResponse; - await expect(transformedResponse.json()).resolves.toBe(246); - }); - - it('does not alter the value of the text Promise', async () => { - expect.assertions(1); - - // Given a request and a response that returns a number. - const request = { methodName: 'someMethod', params: [123] }; - const response = { - json: () => Promise.resolve(123), - text: () => Promise.resolve('123'), - }; - - // When we create a JSON transformer that doubles the number. - const transformer = createJsonRpcResponseTransformer((json: unknown) => (json as number) * 2); - - // Then the text should function should return the original string. - const transformedResponse = transformer(response, request); - await expect(transformedResponse.text()).resolves.toBe('123'); - }); - - it('returns a frozen object as the Reponse', () => { - // Given any response. - const response = { - json: () => Promise.resolve(123), - text: () => Promise.resolve('123'), - }; - - // When we pass it through a JSON transformer. - const transformedResponse = createJsonRpcResponseTransformer(x => x)(response, {} as RpcRequest); - - // Then we expect the transformed response to be frozen. - expect(transformedResponse).toBeFrozenObject(); - }); -}); diff --git a/packages/rpc-spec/src/__tests__/rpc-test.ts b/packages/rpc-spec/src/__tests__/rpc-test.ts index 21cc95b06630..b56042e461b4 100644 --- a/packages/rpc-spec/src/__tests__/rpc-test.ts +++ b/packages/rpc-spec/src/__tests__/rpc-test.ts @@ -2,20 +2,12 @@ import { createRpcMessage } from '@solana/rpc-spec-types'; import { createRpc, Rpc } from '../rpc'; import { RpcApi, RpcApiRequestPlan } from '../rpc-api'; -import { createJsonRpcResponseTransformer, RpcResponse } from '../rpc-shared'; import { RpcTransport } from '../rpc-transport'; interface TestRpcMethods { someMethod(...args: unknown[]): unknown; } -function createMockResponse(jsonResponse: T): RpcResponse { - return { - json: () => Promise.resolve(jsonResponse), - text: () => Promise.resolve(JSON.stringify(jsonResponse)), - }; -} - describe('JSON-RPC 2.0', () => { let makeHttpRequest: RpcTransport; let rpc: Rpc; @@ -41,7 +33,7 @@ describe('JSON-RPC 2.0', () => { }); it('returns results from the transport', async () => { expect.assertions(1); - (makeHttpRequest as jest.Mock).mockResolvedValueOnce(createMockResponse(123)); + (makeHttpRequest as jest.Mock).mockResolvedValueOnce(123); const result = await rpc.someMethod().send(); expect(result).toBe(123); }); @@ -81,7 +73,7 @@ describe('JSON-RPC 2.0', () => { let responseTransformer: jest.Mock; let rpc: Rpc; beforeEach(() => { - responseTransformer = jest.fn(createJsonRpcResponseTransformer(json => `${json} processed response`)); + responseTransformer = jest.fn(json => `${json} processed response`); rpc = createRpc({ api: { someMethod(...params: unknown[]): RpcApiRequestPlan { @@ -97,14 +89,14 @@ describe('JSON-RPC 2.0', () => { }); it('calls the response transformer with the response from the JSON-RPC 2.0 endpoint', async () => { expect.assertions(1); - const rawResponse = createMockResponse(123); + const rawResponse = 123; (makeHttpRequest as jest.Mock).mockResolvedValueOnce(rawResponse); await rpc.someMethod().send(); expect(responseTransformer).toHaveBeenCalledWith(rawResponse, { methodName: 'someMethod', params: [] }); }); it('returns the processed response', async () => { expect.assertions(1); - (makeHttpRequest as jest.Mock).mockResolvedValueOnce(createMockResponse(123)); + (makeHttpRequest as jest.Mock).mockResolvedValueOnce(123); const result = await rpc.someMethod().send(); expect(result).toBe('123 processed response'); }); diff --git a/packages/rpc-spec/src/rpc-shared.ts b/packages/rpc-spec/src/rpc-shared.ts index 3c69dc56cd9b..7e4b1e74c02c 100644 --- a/packages/rpc-spec/src/rpc-shared.ts +++ b/packages/rpc-spec/src/rpc-shared.ts @@ -3,10 +3,7 @@ export type RpcRequest = { readonly params: TParams; }; -export type RpcResponse = { - readonly json: () => Promise; - readonly text: () => Promise; -}; +export type RpcResponse = TResponse; export type RpcRequestTransformer = { (request: RpcRequest): RpcRequest; @@ -15,17 +12,3 @@ export type RpcRequestTransformer = { export type RpcResponseTransformer = { (response: RpcResponse, request: RpcRequest): RpcResponse; }; - -export function createJsonRpcResponseTransformer( - jsonTransformer: (json: unknown, request: RpcRequest) => TResponse, -): RpcResponseTransformer { - return function (response: RpcResponse, request: RpcRequest): RpcResponse { - return Object.freeze({ - ...response, - json: async () => { - const json = await response.json(); - return jsonTransformer(json, request); - }, - }); - }; -} diff --git a/packages/rpc-spec/src/rpc.ts b/packages/rpc-spec/src/rpc.ts index 60f0bb8638e1..4421186401a9 100644 --- a/packages/rpc-spec/src/rpc.ts +++ b/packages/rpc-spec/src/rpc.ts @@ -79,8 +79,7 @@ function createPendingRpcRequest(jsonResponse: T): RpcResponse { - return { - json: () => Promise.resolve(jsonResponse), - text: () => Promise.resolve(JSON.stringify(jsonResponse)), - }; -} - describe('getDefaultResponseTransformerForSolanaRpc', () => { describe('given an array as input', () => { const input = [10, 10n, '10', ['10', [10n, 10], 10]] as const; const request = {} as RpcRequest; - const response = createMockResponse({ result: input }); - it('casts the numbers in the array to a `bigint`, recursively', async () => { - expect.assertions(1); + const response = { result: input }; + it('casts the numbers in the array to a `bigint`, recursively', () => { const transformer = getDefaultResponseTransformerForSolanaRpc(); - const transformedResponse = transformer(response, request); - await expect(transformedResponse.json()).resolves.toStrictEqual([ + expect(transformer(response, request)).toStrictEqual([ BigInt(input[0]), input[1], input[2], @@ -31,13 +22,11 @@ describe('getDefaultResponseTransformerForSolanaRpc', () => { describe('given an object as input', () => { const input = { a: 10, b: 10n, c: { c1: '10', c2: 10 } } as const; const request = {} as RpcRequest; - const response = createMockResponse({ result: input }); + const response = { result: input }; - it('casts the numbers in the object to `bigints`, recursively', async () => { - expect.assertions(1); + it('casts the numbers in the object to `bigints`, recursively', () => { const transformer = getDefaultResponseTransformerForSolanaRpc(); - const transformedResponse = transformer(response, request); - await expect(transformedResponse.json()).resolves.toStrictEqual({ + expect(transformer(response, request)).toStrictEqual({ a: BigInt(input.a), b: input.b, c: { c1: input.c.c1, c2: BigInt(input.c.c2) }, @@ -54,29 +43,25 @@ describe('getDefaultResponseTransformerForSolanaRpc', () => { ${'numeric response'} | ${[[]]} | ${10} | ${10} `( 'performs no `bigint` upcasts on $description when the allowlist is of the form in test case $#', - async ({ allowedKeyPaths, expectation, input }) => { - expect.assertions(1); + ({ allowedKeyPaths, expectation, input }) => { const transformer = getDefaultResponseTransformerForSolanaRpc({ allowedNumericKeyPaths: { getFoo: allowedKeyPaths }, }); const request = { methodName: 'getFoo' } as RpcRequest; - const response = createMockResponse({ result: input }); - const transformedResponse = transformer(response, request); - await expect(transformedResponse.json()).resolves.toStrictEqual(expectation); + const response = { result: input }; + expect(transformer(response, request)).toStrictEqual(expectation); }, ); }); describe('given a JSON RPC error as input', () => { const request = {} as RpcRequest; - const response = createMockResponse({ + const response = { error: { code: SOLANA_ERROR__JSON_RPC__PARSE_ERROR, message: 'o no' }, - }); + }; - it('throws it as a SolanaError', async () => { - expect.assertions(1); + it('throws it as a SolanaError', () => { const transformer = getDefaultResponseTransformerForSolanaRpc(); - const transformedResponse = transformer(response, request); - await expect(transformedResponse.json()).rejects.toThrow( + expect(() => transformer(response, request)).toThrow( new SolanaError(SOLANA_ERROR__JSON_RPC__PARSE_ERROR, { __serverMessage: 'o no' }), ); }); diff --git a/packages/rpc-transformers/src/response-transformer-result.ts b/packages/rpc-transformers/src/response-transformer-result.ts index 074603ad5b80..6a074dc9355a 100644 --- a/packages/rpc-transformers/src/response-transformer-result.ts +++ b/packages/rpc-transformers/src/response-transformer-result.ts @@ -1,7 +1,7 @@ -import { createJsonRpcResponseTransformer } from '@solana/rpc-spec'; +import { RpcResponseTransformer } from '@solana/rpc-spec'; type JsonRpcResponse = { result: unknown }; -export function getResultResponseTransformer() { - return createJsonRpcResponseTransformer(json => (json as JsonRpcResponse).result); +export function getResultResponseTransformer(): RpcResponseTransformer { + return json => (json as JsonRpcResponse).result; } diff --git a/packages/rpc-transformers/src/response-transformer-throw-solana-error.ts b/packages/rpc-transformers/src/response-transformer-throw-solana-error.ts index 40e55aebf9f9..0190bb816141 100644 --- a/packages/rpc-transformers/src/response-transformer-throw-solana-error.ts +++ b/packages/rpc-transformers/src/response-transformer-throw-solana-error.ts @@ -1,14 +1,14 @@ import { getSolanaErrorFromJsonRpcError } from '@solana/errors'; -import { createJsonRpcResponseTransformer } from '@solana/rpc-spec'; +import { RpcResponseTransformer } from '@solana/rpc-spec'; type JsonRpcResponse = { error: Parameters[0] } | { result: unknown }; -export function getThrowSolanaErrorResponseTransformer() { - return createJsonRpcResponseTransformer(json => { +export function getThrowSolanaErrorResponseTransformer(): RpcResponseTransformer { + return json => { const jsonRpcResponse = json as JsonRpcResponse; if ('error' in jsonRpcResponse) { throw getSolanaErrorFromJsonRpcError(jsonRpcResponse.error); } return jsonRpcResponse; - }); + }; } diff --git a/packages/rpc-transformers/src/tree-traversal.ts b/packages/rpc-transformers/src/tree-traversal.ts index 4988e10d21d0..c9c704250ee1 100644 --- a/packages/rpc-transformers/src/tree-traversal.ts +++ b/packages/rpc-transformers/src/tree-traversal.ts @@ -1,9 +1,4 @@ -import { - createJsonRpcResponseTransformer, - RpcRequest, - RpcRequestTransformer, - RpcResponseTransformer, -} from '@solana/rpc-spec'; +import { RpcRequest, RpcRequestTransformer, RpcResponseTransformer } from '@solana/rpc-spec'; export type KeyPathWildcard = { readonly __brand: unique symbol }; export type KeyPath = ReadonlyArray; @@ -61,7 +56,5 @@ export function getTreeWalkerResponseTransformer( visitors: NodeVisitor[], initialState: TState, ): RpcResponseTransformer { - return createJsonRpcResponseTransformer(json => { - return getTreeWalker(visitors)(json, initialState); - }); + return json => getTreeWalker(visitors)(json, initialState); } diff --git a/packages/rpc-transport-http/src/__tests__/http-transport-abort-test.ts b/packages/rpc-transport-http/src/__tests__/http-transport-abort-test.ts index 0d091f1d23ec..19ce152a28c8 100644 --- a/packages/rpc-transport-http/src/__tests__/http-transport-abort-test.ts +++ b/packages/rpc-transport-http/src/__tests__/http-transport-abort-test.ts @@ -76,10 +76,7 @@ describe('createHttpTransport and `AbortSignal`', () => { } as unknown as Response); const sendPromise = makeHttpRequest({ payload: 123, signal: abortSignal }); abortController.abort('I got bored waiting'); - const response = await sendPromise; - await expect(response.json()).resolves.toMatchObject({ - ok: true, - }); + await expect(sendPromise).resolves.toMatchObject({ ok: true }); }); }); }); diff --git a/packages/rpc-transport-http/src/http-transport.ts b/packages/rpc-transport-http/src/http-transport.ts index daafb4949baa..0fb9f0e6996e 100644 --- a/packages/rpc-transport-http/src/http-transport.ts +++ b/packages/rpc-transport-http/src/http-transport.ts @@ -66,9 +66,6 @@ export function createHttpTransport(config: Config): RpcTransport { statusCode: response.status, }); } - return Object.freeze({ - json: () => response.json(), - text: () => response.text(), - }); + return await response.json(); }; } diff --git a/packages/rpc/src/__tests__/rpc-request-coalescer-test.ts b/packages/rpc/src/__tests__/rpc-request-coalescer-test.ts index 38b2ab4c9eb8..171f4326ff86 100644 --- a/packages/rpc/src/__tests__/rpc-request-coalescer-test.ts +++ b/packages/rpc/src/__tests__/rpc-request-coalescer-test.ts @@ -2,13 +2,6 @@ import type { RpcResponse, RpcTransport } from '@solana/rpc-spec'; import { getRpcTransportWithRequestCoalescing } from '../rpc-request-coalescer'; -function createMockResponse(jsonResponse: T): RpcResponse { - return { - json: () => Promise.resolve(jsonResponse), - text: () => Promise.resolve(JSON.stringify(jsonResponse)), - }; -} - describe('RPC request coalescer', () => { let coalescedTransport: RpcTransport; let hashFn: jest.MockedFunction<() => string | undefined>; @@ -37,7 +30,7 @@ describe('RPC request coalescer', () => { }); it('multiple requests in the same tick receive the same response', async () => { expect.assertions(2); - const mockResponse = createMockResponse({ response: 'ok' }); + const mockResponse = { response: 'ok' }; mockTransport.mockResolvedValueOnce(mockResponse); const responsePromiseA = coalescedTransport({ payload: null }); const responsePromiseB = coalescedTransport({ payload: null }); @@ -48,8 +41,8 @@ describe('RPC request coalescer', () => { }); it('multiple requests in different ticks receive different responses', async () => { expect.assertions(2); - const mockResponseA = createMockResponse({ response: 'okA' }); - const mockResponseB = createMockResponse({ response: 'okB' }); + const mockResponseA = { response: 'okA' }; + const mockResponseB = { response: 'okB' }; mockTransport.mockResolvedValueOnce(mockResponseA); mockTransport.mockResolvedValueOnce(mockResponseB); const responsePromiseA = coalescedTransport({ payload: null }); @@ -164,7 +157,7 @@ describe('RPC request coalescer', () => { it('delivers responses to all but the aborted requests', async () => { expect.assertions(2); abortControllerA.abort('o no A'); - const mockResponse = createMockResponse({ response: 'ok' }); + const mockResponse = { response: 'ok' }; transportResponsePromise(mockResponse); await Promise.all([ expect(responsePromiseA).rejects.toBe('o no A'), @@ -199,8 +192,8 @@ describe('RPC request coalescer', () => { }); it('multiple requests in the same tick receive different responses', async () => { expect.assertions(2); - const mockResponseA = createMockResponse({ response: 'okA' }); - const mockResponseB = createMockResponse({ response: 'okB' }); + const mockResponseA = { response: 'okA' }; + const mockResponseB = { response: 'okB' }; mockTransport.mockResolvedValueOnce(mockResponseA); mockTransport.mockResolvedValueOnce(mockResponseB); const responsePromiseA = coalescedTransport({ payload: null });