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

Make RPC transports responsible for preparing payloads #3186

Conversation

lorisleiva
Copy link
Contributor

@lorisleiva lorisleiva commented Sep 1, 2024

This PR slightly adjusts the boundaries of the RPC Transport layer by making RPC transports responsible for creating the payload they will send over the wire.

This mean, the RpcRequest type flows from the RPC API layer to the RPC Transport layer (with the addition of the abort signal when reaching the send method).

In other words, this is the key change this PR makes:

// Before.
type RpcTransportRequest = Readonly<{
    payload: unknown;
    signal?: AbortSignal;
}>;

// After.
type RpcTransportRequest<TParams = unknown> = RpcRequest<TParams> & {
    readonly signal?: AbortSignal;
};

Copy link

changeset-bot bot commented Sep 1, 2024

🦋 Changeset detected

Latest commit: f62ca3a

The changes in this PR will be included in the next version bump.

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Copy link
Contributor Author

lorisleiva commented Sep 1, 2024

@lorisleiva lorisleiva force-pushed the loris/rpc-transports-responsible-for-payload-creation branch from 0d3222d to 2e132bb Compare September 1, 2024 20:06
@lorisleiva lorisleiva marked this pull request as ready for review September 1, 2024 20:12
@lorisleiva lorisleiva force-pushed the loris/revert-rpc-response-type branch from cf99f67 to de2bb81 Compare September 2, 2024 15:42
@lorisleiva lorisleiva force-pushed the loris/rpc-transports-responsible-for-payload-creation branch 2 times, most recently from 6d224dd to 56c4dbc Compare September 2, 2024 15:47
@lorisleiva lorisleiva force-pushed the loris/revert-rpc-response-type branch from de2bb81 to e8fc7ad Compare September 3, 2024 21:35
@lorisleiva lorisleiva force-pushed the loris/rpc-transports-responsible-for-payload-creation branch from 56c4dbc to 4430dda Compare September 3, 2024 21:35
@lorisleiva lorisleiva force-pushed the loris/revert-rpc-response-type branch from e8fc7ad to efdef44 Compare September 3, 2024 22:11
@lorisleiva lorisleiva force-pushed the loris/rpc-transports-responsible-for-payload-creation branch from 4430dda to dd8d6b7 Compare September 3, 2024 22:11
@lorisleiva lorisleiva requested review from steveluscher, buffalojoec and mcintyre94 and removed request for steveluscher September 3, 2024 22:20
@lorisleiva lorisleiva force-pushed the loris/revert-rpc-response-type branch from efdef44 to 50e9859 Compare September 5, 2024 09:20
@lorisleiva lorisleiva force-pushed the loris/rpc-transports-responsible-for-payload-creation branch from dd8d6b7 to 70bfa6f Compare September 5, 2024 09:21
@lorisleiva lorisleiva force-pushed the loris/revert-rpc-response-type branch from 50e9859 to 191c075 Compare September 6, 2024 11:49
@lorisleiva lorisleiva force-pushed the loris/rpc-transports-responsible-for-payload-creation branch from 70bfa6f to 3a681f1 Compare September 6, 2024 11:49
Copy link
Collaborator

@steveluscher steveluscher left a comment

Choose a reason for hiding this comment

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

So, this makes sense if you think of the transport as particularly a JSON-RPC transport. I expected that we'd stop short of this thing knowing anything about JSON-RPC.

These are the levels of ‘knowing about stuff.’

  1. I know how to send and receive bytes over HTTP
  2. I know how to send and receive JavaScript objects over HTTP
  3. I know how to prepare JSON-RPC method calls as JavaScript objects and interpret the responses
  4. I know how to fulfil @solana/web3.js API calls.

This is how I thought we'd divide these responsibilities up.

  1. createHttpTransport() makes something that just ferries bytes (as text) back and forth
  2. createHttpTransportForSolanaRpc() something that wraps createHttpTransport. Inputs and outputs are JavaScript primitives (as unknown). It looks inside them (ie. check if it's an object, then a JSON-RPC message, then one of the allowlisted methods) to see if it should muck around with them or not according to the specific rules we have around upcasts. Ultimately passes text to the underlying transport.
  3. createSolanaRpcApi knows about the JSON-RPC protocol. Can prepare a struct representing a JSON-RPC method call and interpret a response struct.
  4. Delegates userland API calls to the RpcApi as normal.

This would mean that these calls live at these levels.

  1. fetch
  2. Your fancy JSON.stringify implementation, isJsonRpcMessage(payload), bigint upcast stuff
  3. createRpcMessage(request)
  4. the usual crap

The implication of this architecture is that you could swap out 1 & 2 for the createCarrierPigeon() transport, and as long as the carrier pigeon knew how to take the JavaScript object {jsonrpc: '2.0', method: 'getSlot'. id: 1} to its destination and to bring back the JavaScript object {id: 1, result: 123n} everything else continues to work without modification. Carrier pigeons, after all, understand large numbers and don't need our stupid JSON serialization code.

@lorisleiva lorisleiva changed the base branch from loris/revert-rpc-response-type to graphite-base/3186 September 10, 2024 07:51
@lorisleiva lorisleiva force-pushed the loris/rpc-transports-responsible-for-payload-creation branch from 3a681f1 to 23f22d1 Compare September 10, 2024 07:53
@lorisleiva lorisleiva changed the base branch from graphite-base/3186 to master September 10, 2024 07:54
@lorisleiva lorisleiva force-pushed the loris/rpc-transports-responsible-for-payload-creation branch from 23f22d1 to f62ca3a Compare September 10, 2024 07:54
@lorisleiva lorisleiva closed this Sep 10, 2024
Copy link
Contributor

Because there has been no activity on this PR for 14 days since it was merged, it has been automatically locked. Please open a new issue if it requires a follow up.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 25, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants