-
Notifications
You must be signed in to change notification settings - Fork 919
refactor(experimental): add generic createJsonRpcApi
function for custom APIs
#1781
refactor(experimental): add generic createJsonRpcApi
function for custom APIs
#1781
Conversation
Current dependencies on/for this PR:
This stack of pull requests is managed by Graphite. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm… interesting.
Backing way up, the point of the IRpcApi
type is to say: ‘hey, if you wanna define an API for some JSON RPC, then give me something that conforms to this type’
(...params: WhateverParams[]) => {
methodName: string;
params: unknown[];
responseProcessor?: (response: unknown) => WhateverResponse;
}
One way of doing that is the fancy Proxy
solution that I wrote for the Solana JSON-RPC API. That proxy solution works for Solana because all of the methods are conveniently named the same as one would like the JavaScript functions to be, and the params are mostly pass-throughs.
I designed the IRpcApi
such that you could write a helper that produced it in some other way, though. In QuickNode's case, for instance, it's very unlikely that they will want to mirror the method names as JavaScript function names.
// I doubt they will want to release this as a JavaScript API
await rpc.qn_fetchNFTCollectionDetails(...).send();
// It's more likely they'll want this
await quickNodeRpc.fetchNFTCollectionDetails(...).send();
They could achieve that by releasing this:
function createQuickNodeRpcApi(): IRpcApi<QuickNodeRpcMethods> {
return {
fetchNFTCollectionDetails(...params) {
return {
methodName: 'qn_fetchNFTCollectionDetails',
params,
}
},
};
}
That conforms to the IRpcApi
type, and they're good. That's not a great design for an API creator (QuickNode, please don't do that) but it works.
The API creator in this PR adds a layer of indirection without actually letting QuickNode decouple the name of the JavaScript functions and the name of the JSON-RPC methods, so it's handy but maybe not useful.
We could solve this in two ways:
- do nothing and let people make their own API creators (fork ours if they like)
- double down on the ‘API creator in the proxy style’ helper you have here and let people transform the method names too.
Ok thanks for that insight. My thinking was that our "here's my API's type spec, now make me an RPC API that conforms to the JSON RPC spec with them" approach was pretty slick. Then people can just fork One might think it introduces the idea of it being more restrictive, but if we split out So I guess it comes down to: Should we be nice and de-couple the fancy In my opinion, we have absolutely nothing to lose by doing so and we give developers better tooling for creating custom APIs, which is cool. |
I guess one follow-up question is how difficult is it to build an API spec with this the right way, and not use the
This example seems pretty simple tbh haha. |
Yeah! If we do offer it then we just need to add a method name transformer to the config, so that the method that gets called on the proxy can be different than the method that gets called on the actual RPC. Oh, and then you have to do this all over again for subscriptions, in case folks have custom subscription methods too.
Yeah, the bad part of the simple example is ‘grows linearly with the number of methods’ whereas the |
Yes I anticipated this. 🥞
I think it's (the generic API-creator thing) valuable so I'm going to keep going with it & subscriptions. I'll definitely add your suggestions! |
6857712
to
a7504e1
Compare
aa08194
to
e729d1d
Compare
a7504e1
to
6af0f6f
Compare
e729d1d
to
d619bc8
Compare
6af0f6f
to
12f7ab5
Compare
12f7ab5
to
ba95963
Compare
@@ -23,7 +23,9 @@ function createPendingRpcRequest<TRpcMethods, TResponse>( | |||
if ('error' in response) { | |||
throw new SolanaJsonRpcError(response.error); | |||
} else { | |||
return (responseTransformer ? responseTransformer(response.result) : response.result) as TResponse; | |||
return ( | |||
responseTransformer ? responseTransformer(response.result, methodName) : response.result |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Why wouldn't we provide the entire response to the transformer?
- Would it be possible to wrap the method name, the payload and the URL in a request object so that the entire request can also be provided to the transformer?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Why wouldn't we provide the entire response to the transformer?
We can, but I think we're only shedding jsonrpc
and id
.
Check out a response payload in the docs.
- Would it be possible to wrap the method name, the payload and the URL in a request object so that the entire request can also be provided to the transformer?
I believe so, yes!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason I'm saying all this is I think this should be comparable to Axios interceptors (or equivalent) where we have no opinion over what could be transformed and we're just like "here's a request I'm about to send, do whatever you want with it" and same with the response. 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, in my opinion I think createJsonRpcApi(..)
should be designed to create APIs that adhere to the JSON RPC spec, without deviation. So, to me, it makes sense to leave part of the request and response unchanged, since modifying request.parameters
or response.result
still adheres to the JSON RPC interface.
It seems like you probably have the Helius example in mind from #1740 but here, I think this function would not be used to implement the custom transport you referenced there. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah that's a fair point actually. So you're allowing the library to transform anything that can be transformed without affecting the JSON RPC specs. I like it!
Am I right to say that, in order to support a urlTransformer
on the API side, we would need something like a requestTransformer
on the Transport side? Similarly to the responseTransformer
, the requestTransformer
would only allow you to edit the components of the requests that don't affect the JSON RPC specs. Namely, the parameters and the URL.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Am I right to say that, in order to support a
urlTransformer
on the API side, we would need something like arequestTransformer
on the Transport side? Similarly to theresponseTransformer
, therequestTransformer
would only allow you to edit the components of the requests that don't affect the JSON RPC specs. Namely, the parameters and the URL.
We could do this in theory, but I can't decide if I think it's something we should support.
Are you thinking you'd be able to plug in a requestTransformer
that, for example, takes all of the parameters and the method name from the POST body and maps them to URL parameters?
As an aside, I'm also not sure if the Helius endpoints for example are GETs instead of POSTs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah that's what I'm thinking. I guess it all depends what is technically valid in the JSON RPC specs. Is it okay to use dynamic URLs? Is it okay to use different HTTP methods? If not, then maybe we shouldn't mess with it. If it is valid according to the specs though, then I strongly believe we should offer that level of flexibility.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't see anything in the spec that says definitively one way or the other. But it does seem like the intention is a request body.
74fa32d
to
47e4190
Compare
ba95963
to
603b5c6
Compare
47e4190
to
bfd2570
Compare
603b5c6
to
8b04dee
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Merge activity
|
bfd2570
to
2f2b534
Compare
8b04dee
to
98b44c4
Compare
…or custom APIs Continuing the work from #1781, this PR adds a generic function for creating an RPC Subscriptions API, and drives this function from the main `@solana/web3.js`.
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. |
This PR is a concept for now. It serves to potentially address the discussion raised in #1740.
The goal of the collective
@solana/rpc-*
packages has been to allow complete customization, as long as their API adheres to the official JSON RPC spec.However, I couldn't find the best way someone would actually go about doing this with our current implementation. Maybe I'm missing something.
In the actual
library
package, we're providing what's been dubbed the "default" Solana API. The functioncreateSolanaRpc(..)
allows you to provide your own transport, but it automatically uses the Solana RPC API, defined by@solana/rpc-core
for the client API.solana-web3.js/packages/library/src/rpc.ts
Lines 19 to 24 in 589c379
If one wants to create their own RPC client manually, they can use the following code, comprised of
@solana/rpc-transport
and@solana/rpc-core
, not the main library.You can see you can choose to define your API and provide it as a parameter to
createJsonRpc(..)
, however unless I'm missing something in our code, there's no generic API-creator.This PR attempts to roll that generic API-creator as
createJsonRpcApi(..)
.I envision this function being extremely useful for projects who wish to define their own
rpc-core
type-spec via interfaces, as we have withSolanaRpcMethods
, and simply create it like so:Of course you could also combine your type spec with Solana's:
Let me know any thoughts.