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

refactor(experimental): add cluster level API for transports #2053

Conversation

buffalojoec
Copy link
Contributor

@buffalojoec buffalojoec commented Jan 21, 2024

This change introduces cluster-level transports as well as cluster-level JSON RPCs
that infer the cluster from the provided transport.

@lorisleiva
Copy link
Contributor

I like this stack a lot. I love that only with types, we can cherry-pick the RPC methods available in different clusters.

The only API change I want to put on the table is, instead of requesting a second TCluster type parameter which makes it tricky when you don't need to offer the first one, why don't we infer that type from the config object provided. IIRC @steveluscher mentioned something like this a while ago. For instance, we could attach that piece of information on the URL directly.

type MainnetUrl = string & { '~cluster': 'mainnet' }
type DevnetUrl = string & { '~cluster': 'devnet' }
type TestnetUrl = string & { '~cluster': 'testnet' }
type ClusterUrl = string | MainnetUrl | DevnetUrl | TestnetUrl

function mainnet(putativeString: string): MainnetUrl { return putativeString as MainnetUrl }
function devnet(putativeString: string): DevnetUrl { return putativeString as DevnetUrl }
function testnet(putativeString: string): TestnetUrl { return putativeString as TestnetUrl }

// Devnet example.
const devnetTransport = createDefaultRpcTransport({ url: devnet('https://api.devnet.solana.com') });
createSolanaRpc({ transport: devnetTransport });
//  ^ RpcDevnet<DevnetSolanaRpcMethods> 

// Generic example.
const genericTransport = createDefaultRpcTransport({ url: 'http://127.0.0.1:8899' });
createSolanaRpc({ transport: genericTransport });
//  ^ Rpc<SolanaRpcMethods> 

Wdyt?

@buffalojoec
Copy link
Contributor Author

buffalojoec commented Jan 30, 2024

The only API change I want to put on the table is, instead of requesting a second TCluster type parameter which makes it tricky when you don't need to offer the first one, why don't we infer that type from the config object provided.

Interesting. Just thinking about your suggestion out loud here:

For just @solana/rpc-transport, we have three methods:

  • createHttpTransport(..): This one takes a url argument and can probably be refactored to use your approach. I actually left this one alone intentionally.
  • createJsonRpcApi<TMethods>(..): This one requires the API generic, and we can optionally pass TCluster after it. The problem is that without TCluster, this function doesn't care what the cluster is. There's no url parameter here.
  • createJsonRpc(.): This one does not take a generic, and we infer the cluster from it's config arg.

In either case, now that I look at it some more, when it comes to the main library in @solana/web3.js, I definitely can refactor the createDefaultRpcTransport(..) to apply the cluster argument there and eliminate the need for it - in either form - on createSolanaRpc(..).

There is one problem here. If we put the cluster configuration on the URL as provided to transport, there's no way to also get it inside of createJsonRpcApi(..), since it only takes the transformers as arguments. We'd still need to provide it there as TCluster, and then we'd have a diverging API.

Wdyt @lorisleiva ?

P.S. I edited/re-wrote this after checking out the code some more. These commits are a tad old for me, had to refresh.

@buffalojoec buffalojoec force-pushed the 01-21-refactor_experimental_add_cluster_level_API_for_transports branch from 8b5554c to 3fbed8a Compare January 30, 2024 21:31
@lorisleiva
Copy link
Contributor

What if we don't tag the API methods with a cluster though? What if we just have various API methods types available like SolanaRpcMethodsMainnet, SolanaRpcMethodsDevnet that includes requestAirdrop, etc.

You can see them as various libraries of RPC methods available in different clusters.

Then we select the one we need based on the given transport URL when we create the RPC (transport + api). Is this making any sense? 😅

@buffalojoec
Copy link
Contributor Author

What if we don't tag the API methods with a cluster though? What if we just have various API methods types available like SolanaRpcMethodsMainnet, SolanaRpcMethodsDevnet that includes requestAirdrop, etc.

You can see them as various libraries of RPC methods available in different clusters.

Then we select the one we need based on the given transport URL when we create the RPC (transport + api). Is this making any sense? 😅

I had them isolated into per-cluster sets when I originally created this stack (or one before it I forget). Is this what you're thinking?

createJsonRpc({
    api: createJsonRpcApi<SolanaRpcMethodsDevnet>(),
    transport: createHttpTransport({ url: devnet('https://api.devnet.solana.com') }),
}); // RpcDevnet<SolanaRpcMethodsDevnet>

createJsonRpc({
    api: createJsonRpcApi<SolanaRpcMethodsTestnet>(),
    transport: createHttpTransport({ url: testnet('https://api.testnet.solana.com') }),
}); // RpcTestnet<SolanaRpcMethodsTestnet>

createJsonRpc({
    api: createJsonRpcApi<SolanaRpcMethodsMainnet>(),
    transport: createHttpTransport({ url: mainnet('https://api.mainnet-beta.solana.com') }),
}); // RpcMainnet<SolanaRpcMethodsMainnet>

@lorisleiva
Copy link
Contributor

Yes, exactly that. Then it's the responsibility of createSolanaRpc to provide the right default api based on the provided transport. Was there any issues with this approach?

@buffalojoec
Copy link
Contributor Author

Yes, exactly that. Then it's the responsibility of createSolanaRpc to provide the right default api based on the provided transport. Was there any issues with this approach?

No I think that will work fine. Anyone who creates their own RPC client from our libraries would just have to make sure they align the proper API based on cluster as well, similar to what I just did above.

For our Solana-implemented helpers, we can wrap that.

@buffalojoec buffalojoec force-pushed the 01-21-refactor_experimental_add_cluster_level_API_for_transports branch from 3fbed8a to d6e8247 Compare February 1, 2024 01:51
@buffalojoec buffalojoec changed the base branch from master to 01-31-refactor_experimental_refactor_rpc_core_test_APIs February 1, 2024 01:51
@buffalojoec buffalojoec force-pushed the 01-21-refactor_experimental_add_cluster_level_API_for_transports branch from d6e8247 to 0f48e23 Compare February 1, 2024 03:59
@buffalojoec buffalojoec changed the base branch from 01-31-refactor_experimental_refactor_rpc_core_test_APIs to 01-31-refactor_experimental_move_transport_types_out_of_rpc-transport February 1, 2024 03:59
Comment on lines +71 to +91
export function createJsonRpc<TRpcMethods>(
rpcConfig: Readonly<{
api: IRpcApi<TRpcMethods>;
transport: IRpcTransportDevnet;
}>,
): RpcDevnet<TRpcMethods>;
export function createJsonRpc<TRpcMethods>(
rpcConfig: Readonly<{
api: IRpcApi<TRpcMethods>;
transport: IRpcTransportTestnet;
}>,
): RpcTestnet<TRpcMethods>;
export function createJsonRpc<TRpcMethods>(
rpcConfig: Readonly<{
api: IRpcApi<TRpcMethods>;
transport: IRpcTransportMainnet;
}>,
): RpcMainnet<TRpcMethods>;
export function createJsonRpc<TRpcMethods>(
rpcConfig: Readonly<{
api: IRpcApi<TRpcMethods>;
transport: IRpcTransport;
}>,
): Rpc<TRpcMethods>;
export function createJsonRpc<TRpcMethods, TConfig extends RpcConfig<TRpcMethods>>(
rpcConfig: TConfig,
): RpcFromTransport<TRpcMethods, TConfig['transport']> {
return makeProxy(rpcConfig) as RpcFromTransport<TRpcMethods, TConfig['transport']>;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I went with overloads here because we've designed this API for users to provide their RPC methods. It can be removed, but this function would then take 2 generic arguments, and the moment you provide your RPC methods it will complain you didn't provide TConfig as well.

@buffalojoec buffalojoec force-pushed the 01-21-refactor_experimental_add_cluster_level_API_for_transports branch from 0f48e23 to 43492b5 Compare February 1, 2024 04:47
@buffalojoec buffalojoec force-pushed the 01-21-refactor_experimental_add_cluster_level_API_for_transports branch from 43492b5 to 33d3cb8 Compare February 1, 2024 21:27
@buffalojoec buffalojoec changed the base branch from 01-31-refactor_experimental_move_transport_types_out_of_rpc-transport to 01-31-refactor_experimental_add_ClusterUrl_types February 1, 2024 21:27
Copy link
Contributor

@lorisleiva lorisleiva left a comment

Choose a reason for hiding this comment

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

Love it!

@buffalojoec buffalojoec force-pushed the 01-31-refactor_experimental_add_ClusterUrl_types branch from f883a80 to c9bb45e Compare February 2, 2024 15:58
@buffalojoec buffalojoec force-pushed the 01-21-refactor_experimental_add_cluster_level_API_for_transports branch from 33d3cb8 to 9daa3c2 Compare February 2, 2024 15:58
Copy link
Contributor Author

buffalojoec commented Feb 2, 2024

Merge activity

  • Feb 2, 11:35 AM EST: @buffalojoec started a stack merge that includes this pull request via Graphite.
  • Feb 2, 11:37 AM EST: Graphite rebased this pull request as part of a merge.
  • Feb 2, 11:38 AM EST: @buffalojoec merged this pull request with Graphite.

Base automatically changed from 01-31-refactor_experimental_add_ClusterUrl_types to master February 2, 2024 16:36
buffalojoec added a commit that referenced this pull request Feb 2, 2024
As per @lorisleiva's suggestion, here's some types for cluster URLs!
Straight from [this comment](#2053 (comment))
@buffalojoec buffalojoec force-pushed the 01-21-refactor_experimental_add_cluster_level_API_for_transports branch from 9daa3c2 to 96362d5 Compare February 2, 2024 16:36
@buffalojoec buffalojoec merged commit e58bb22 into master Feb 2, 2024
5 checks passed
@buffalojoec buffalojoec deleted the 01-21-refactor_experimental_add_cluster_level_API_for_transports branch February 2, 2024 16:38
Copy link
Contributor

github-actions bot commented Feb 7, 2024

🎉 This PR is included in version 1.90.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

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 Feb 21, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants