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

[experimental] A types-only RPC implementation using JavaScript proxies #1190

Merged
merged 5 commits into from
Mar 9, 2023

Conversation

steveluscher
Copy link
Collaborator

This PR introduces an implementation of the client RPC that does as little work as possible. It essentially passes calls to the server RPC unmodified, and enforces correctness through nothing but Typescript types.

Problem

The Connection class today contains concrete implementations for every RPC method, past, present, and future. This implies that the bundle size of this library may grow unbounded as the number of RPC methods grows.

Solution

The RPC interface is simple and conforms to JSON RPC 2.0. The only things that are required to make an RPC call are a method name and a list of parameters.

The solution herein creates an rpc object on which you can call any method. That method name and the params get passed through, unmodified.

// Example: this call...
rpc.doSomething(transport, 1, 'foo', {bar: 'baz'});

// ...gets put on the wire as a JSON RPC call of the format:
{
  "jsonrpc": "2.0",
  "id": 1,
  "method": "doSomething",
  "params": [1, "foo", {"bar": "baz"}]
}

In this way, the set of RPC methods can grow, unbounded, without increasing the size of the client library at all.

Correctness, in this world, is enforced only by Typescript. The input params are completely specified on the JavaScript Proxy, and the return types change with the inputs given. See the complicated getAccountInfo commit on this PR for an example of how this is achieved using function overloads.

Crucially, this library will never concern itself with how to send RPC calls – only with how you should format your calls and reason about the return values. The sending is left up to the transport, which you can read about in #1187.

How to review this PR

Click the ‘commits’ tab and treat each commit as though it were its own PR.

@codecov-commenter
Copy link

codecov-commenter commented Mar 9, 2023

Codecov Report

Merging #1190 (c59286a) into master (ad23683) will not change coverage.
The diff coverage is n/a.

❗ Current head c59286a differs from pull request most recent head b87de1a. Consider uploading reports for the commit b87de1a to get more accurate results

@@           Coverage Diff           @@
##           master    #1190   +/-   ##
=======================================
  Coverage   76.35%   76.35%           
=======================================
  Files          56       56           
  Lines        3151     3151           
  Branches      475      475           
=======================================
  Hits         2406     2406           
  Misses        578      578           
  Partials      167      167           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Comment on lines 11 to 19
getBlockHeight(
transport: IJsonRpcTransport,
config?: readonly {
// Defaults to `finalized`
commitment?: Finality;
// The minimum slot that the request can be evaluated at
minContextSlot?: Slot;
}
): Promise<GetBlockHeightApiResponse>;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Comment on lines 9 to 17
getBlocks(
transport: IJsonRpcTransport,
startSlot: Slot,
endSlotInclusive?: Slot,
config?: readonly {
// Defaults to `finalized`
commitment?: Exclude<Finality, 'processed'>;
}
): Promise<GetBlocksApiResponse>;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like the docs are wrong on this one :-\

Copy link
Collaborator Author

@steveluscher steveluscher Mar 9, 2023

Choose a reason for hiding this comment

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

I fixed them! I don't know why the change hasn't deployed yet. https://github.com/solana-labs/solana/pull/30351/files

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

ah yes. Anyway, thanks for fixing!

Comment on lines +66 to +91
getAccountInfo(
transport: IJsonRPCTransport,
address: Base58EncodedAddress,
config?: readonly {
encoding: 'base64';
} &
GetAccountInfoApiCommonConfig &
GetAccountInfoApiBase64EncodingCommonConfig
): Promise<GetAccountInfoApiResponseBase & GetAccountInfoApiResponseWithEncodedData>;
getAccountInfo(
transport: IJsonRPCTransport,
address: Base58EncodedAddress,
config?: readonly {
encoding: 'base64+zstd';
} &
GetAccountInfoApiCommonConfig &
GetAccountInfoApiBase64EncodingCommonConfig
): Promise<GetAccountInfoApiResponseBase & GetAccountInfoApiResponseWithEncodedZStdCompressedData>;
getAccountInfo(
transport: IJsonRPCTransport,
address: Base58EncodedAddress,
config?: readonly {
encoding: 'jsonParsed';
} &
GetAccountInfoApiCommonConfig
): Promise<GetAccountInfoApiResponseBase & GetAccountInfoApiResponseWithJsonData>;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is a much more complicated example, where you can see how modulating the inputs changes the output type (eg. specifying different encoding results in the output being different.

Things to pay attention to:

  • jsonParsed encoding results in data being a parsed data structure or [bytes, encoding] as a fallback if no parser is available.
  • only base64 encoding accepts dataSlice as an input

See https://docs.solana.com/api/http#getaccountinfo.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is extremely cool, and not nearly as complicated to read as I feared when I first opened the file 😅

Copy link
Contributor

@joncinque joncinque left a comment

Choose a reason for hiding this comment

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

Only one tiny substantial point -- looks really great overall!

Comment on lines +4 to +6
declare type Slot =
// TODO(solana-labs/solana/issues/30341) Represent as bigint
number;
Copy link
Contributor

Choose a reason for hiding this comment

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

I know we've talked about this before, so pardon asking it again here... with the BYO-Transport model, is it possible to require that transports properly handle u64s and make this a bigint?

Copy link
Collaborator Author

@steveluscher steveluscher Mar 9, 2023

Choose a reason for hiding this comment

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

So, fast forward to when everything is bigint (or rewind, and have represented all u64s as JavaScript strings in the first place) and all that the transport has to do is to serialize bigints as string over the wire.

We can't do this today, because the Solana RPC doesn't accept them.

# params is normally [5, 10] but here I'm representing them as ["5", "10"]
curl https://api.mainnet-beta.solana.com -X POST -H "Content-Type: application/json" -d '
  {
    "jsonrpc": "2.0", "id": 1,
    "method": "getBlocks",
    "params": ["5", "10"]
  }
'
{"jsonrpc":"2.0","error":{"code":-32602,"message":"Invalid params: invalid type: string \"5\", expected u64."},"id":1}

We need to change the server to parse strings as u64 before we can do that on the client.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

But I see what you're saying. You're saying: make it a bigint now at the outer part of the interface, and have the transport (temporarily) downcast that to a JavaScript number, maybe optionally warning when it overflows. Is that right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I love this!

Copy link
Contributor

Choose a reason for hiding this comment

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

I was wondering it if it would be possible at the transport level to use bigint everywhere, but I forgot that the problem is at the RPC level, and not web3.js. But yeah, since all of these interfaces are new, it might be worth doing everything right and use bigints everywhere.

Comment on lines 13 to 18
config?: readonly {
// Defaults to `finalized`
commitment?: Finality;
// The minimum slot that the request can be evaluated at
minContextSlot?: Slot;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

All these levels of optionals... I can swee why graphql would make this so much neater

Comment on lines 9 to 17
getBlocks(
transport: IJsonRpcTransport,
startSlot: Slot,
endSlotInclusive?: Slot,
config?: readonly {
// Defaults to `finalized`
commitment?: Exclude<Finality, 'processed'>;
}
): Promise<GetBlocksApiResponse>;
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like the docs are wrong on this one :-\

import { IJsonRpcTransport } from '@solana/rpc-transport';
import { JsonRpcApi } from './types/jsonRpcApi';

export const rpc = /* #__PURE__ */ new Proxy<JsonRpcApi>({} as JsonRpcApi, {
Copy link
Contributor

Choose a reason for hiding this comment

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

TIL what a Proxy is, very cool!

const normalizedParams = params.length ? params : undefined;
const result = await transport.send(method, normalizedParams);
return result;
} as unknown as JsonRpcApi[TMethodName];
Copy link
Contributor

Choose a reason for hiding this comment

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

Learning a lot today with ... as unknown as ... -- does this cause potential footguns for transport implementers?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

At that point, it's our job to make sure that the implementation of rpc.* is perfect. Essentially what we're saying is that the dynamic get returns a method that will definitely conform to JsonRpcApi[TMethodName] but the implementation on lines 14-18 does not, from Typescript's perspective.

src/rpc.ts:14:13 - error TS2322: Type '(transport: IJsonRpcTransport, ...params: Parameters<JsonRpcApi[TMethodName]>) => Promise<unknown>' is not assignable to type 'JsonRpcApi[TMethodName]'.
  Type '(transport: IJsonRpcTransport, ...params: Parameters<JsonRpcApi[TMethodName]>) => Promise<unknown>' is not assignable to type '{ (transport: IJsonRPCTransport, address: Base58EncodedAddress, config?: ({ encoding: "base64"; } & GetAccountInfoApiCommonConfig & GetAccountInfoApiBase64EncodingCommonConfig) | undefined): Promise<...>; (transport: IJsonRPCTransport, address: Base58EncodedAddress, config?: ({ ...; } & ... 1 more ... & GetAccountIn...'.
    Type '(transport: IJsonRpcTransport, ...params: Parameters<JsonRpcApi[TMethodName]>) => Promise<unknown>' is not assignable to type '{ (transport: IJsonRPCTransport, address: Base58EncodedAddress, config?: ({ encoding: "base64"; } & GetAccountInfoApiCommonConfig & GetAccountInfoApiBase64EncodingCommonConfig) | undefined): Promise<...>; (transport: IJsonRPCTransport, address: Base58EncodedAddress, config?: ({ ...; } & ... 1 more ... & GetAccountIn...'.
      Types of parameters 'params' and 'address' are incompatible.
        Type '[address: Base58EncodedAddress, config?: ({ encoding: "base64"; } & GetAccountInfoApiCommonConfig & GetAccountInfoApiBase64EncodingCommonConfig) | undefined]' is not assignable to type 'Parameters<JsonRpcApi[TMethodName]>'.

14             target[p] = async function (transport: IJsonRpcTransport, ...params: Parameters<JsonRpcApi[TMethodName]>) {
               ~~~~~~~~~

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It seems like these two should be compatible:

  • [address: Base58EncodedAddress, config?: ({ encoding: "base64"; } & GetAccountInfoApiCommonConfig & GetAccountInfoApiBase64EncodingCommonConfig) | undefined]
  • Parameters<JsonRpcApi[TMethodName]>

…but they aren't.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah I see, that's really weird, thanks for explaining. Is that a typescript bug? Or does Parameters need to have some extra declaration to make that clear?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm not sure, but I think I made a mistake including transport in all the method signatures. I wonder what would happen if I took that out…

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

lamports: number; // TODO(solana-labs/solana/issues/30341) Represent as bigint
owner: Base64EncodedAddress;
rentEpoch: number; // TODO(solana-labs/solana/issues/30341) Represent as bigint
space: number; // TODO(solana-labs/solana/issues/30341) Represent as bigint
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe I'm missing something, but in testing I didn't see space returned here. Here's a token account:

{
  "jsonrpc": "2.0",
  "result": {
    "context": {
      "apiVersion": "1.13.6",
      "slot": 181671824
    },
    "value": {
      "data": [
        "xvp6877brTo9ZfNqq8l0MbG75MLS9uDkfKYCA0UvXWEs07wIp7mh6Iz0YahgZDSe/Psf+oXDeVKNALXKNdFOFAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAQAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA",
        "base64"
      ],
      "executable": false,
      "lamports": 2039280,
      "owner": "TokenkegQfeZyiNwAJbNbGKPFXCWuBvf9Ss623VQ5DA",
      "rentEpoch": 0
    }
  },
  "id": 1
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Haha. In the docs, this is called size, in the docs' example it's called space, in the current web3.js implementation it's absent, and in the server implementation it's space and it's an Option<u64>.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Looks like this is only in v1.15.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah gotcha

Comment on lines +66 to +91
getAccountInfo(
transport: IJsonRPCTransport,
address: Base58EncodedAddress,
config?: readonly {
encoding: 'base64';
} &
GetAccountInfoApiCommonConfig &
GetAccountInfoApiBase64EncodingCommonConfig
): Promise<GetAccountInfoApiResponseBase & GetAccountInfoApiResponseWithEncodedData>;
getAccountInfo(
transport: IJsonRPCTransport,
address: Base58EncodedAddress,
config?: readonly {
encoding: 'base64+zstd';
} &
GetAccountInfoApiCommonConfig &
GetAccountInfoApiBase64EncodingCommonConfig
): Promise<GetAccountInfoApiResponseBase & GetAccountInfoApiResponseWithEncodedZStdCompressedData>;
getAccountInfo(
transport: IJsonRPCTransport,
address: Base58EncodedAddress,
config?: readonly {
encoding: 'jsonParsed';
} &
GetAccountInfoApiCommonConfig
): Promise<GetAccountInfoApiResponseBase & GetAccountInfoApiResponseWithJsonData>;
Copy link
Contributor

Choose a reason for hiding this comment

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

This is extremely cool, and not nearly as complicated to read as I feared when I first opened the file 😅

};

// TODO: Eventually move this into whatever package implements transactions
declare type Finality = 'confirmed' | 'finalized' | 'processed';
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Is Finality the new term? If so, we should probably update the docs and all that. I'd prefer to keep this Commitment to make mental mapping easier, but I won't put up a big fight about it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ooh. I thought these three were what we're calling finality, as opposed to the olde deprecated suite of commitments.

From web3.js today:

/**
* The level of commitment desired when querying state
* <pre>
* 'processed': Query the most recent block which has reached 1 confirmation by the connected node
* 'confirmed': Query the most recent block which has reached 1 confirmation by the cluster
* 'finalized': Query the most recent block which has been finalized by the cluster
* </pre>
*/
export type Commitment =
| 'processed'
| 'confirmed'
| 'finalized'
| 'recent' // Deprecated as of v1.5.5
| 'single' // Deprecated as of v1.5.5
| 'singleGossip' // Deprecated as of v1.5.5
| 'root' // Deprecated as of v1.5.5
| 'max'; // Deprecated as of v1.5.5
/**
* A subset of Commitment levels, which are at least optimistically confirmed
* <pre>
* 'confirmed': Query the most recent block which has reached 1 confirmation by the cluster
* 'finalized': Query the most recent block which has been finalized by the cluster
* </pre>
*/
export type Finality = 'confirmed' | 'finalized';

So yeah, I have this wrong, somehow. I'll rename it.

@steveluscher steveluscher merged commit 9402bd4 into solana-labs:master Mar 9, 2023
@steveluscher steveluscher deleted the proxy-rpc branch March 9, 2023 21:34
steveluscher added a commit that referenced this pull request Mar 11, 2023
…the RPC

# Summary

@joncinque pointed out in #1190 that the Typescript types of `@solana/rpc-core` relied on a cast through `unknown` to work. The more I thought about it the more I realized that the `transport` shouldn't form part of the typespec for the RPC.

In this PR we remove it from the source types, and map it back in where it's needed in the implementation..
steveluscher added a commit that referenced this pull request Mar 11, 2023
…the RPC

# Summary

@joncinque pointed out in #1190 that the Typescript types of `@solana/rpc-core` relied on a cast through `unknown` to work. The more I thought about it the more I realized that the `transport` shouldn't form part of the typespec for the RPC.

In this PR we remove it from the source types, and map it back in where it's needed in the implementation..
steveluscher added a commit that referenced this pull request Mar 11, 2023
…the RPC (#1196)

# Summary

@joncinque pointed out in #1190 that the Typescript types of `@solana/rpc-core` relied on a cast through `unknown` to work. The more I thought about it the more I realized that the `transport` shouldn't form part of the typespec for the RPC.

In this PR we remove it from the source types, and map it back in where it's needed in the implementation..
@github-actions
Copy link
Contributor

🎉 This PR is included in version 1.73.4 🎉

The release is available on:

Your semantic-release bot 📦🚀

@github-actions
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 Mar 31, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants