feat(rpc-client): support getVoteAccounts #455
Conversation
dnut
changed the title
feat(core): support pubkey json parsing
dnut
force-pushed
the
dnut/feat/rpc/vote-accounts
branch
from
b418ce5 to
b0f5a92
Compare
| @@ -344,6 +344,25 @@ pub const Client = struct { | |||
| return self.sendFetchRequest(allocator, types.RpcVersionInfo, request, .{}); | |||
| } | |||
|
|
|||
| const GetVoteAccountsConfig = struct { | |||
There was a problem hiding this comment.
Not a big deal but originally I had intended to keep rpc types disjoint from sig types, putting the onus on the caller to perform type conversion i.e.
const vote_accounts = client.getVoteAccounts(allocator, .{ .pubkey = pubkey.string().slice() })Also thought the same approach would make sense for the return types for rpc calls.
Let me know your thoughts
There was a problem hiding this comment.
Why? Isn't the whole point of the rpc client to convert between strings and types? Otherwise the user might as well just use json directly with any http client of their choice. Not using Pubkey here just seems to make the client less useful, without any benefit that I can see.
I would be careful about where the types come from though. Ideally the RPC client should not depend on anything outside of core. Depending on plain old data types that define the domain of Solana seems fine to me. But depending on other components like the ledger which depends on rocksdb - that's where I start to see potential costs.
There was a problem hiding this comment.
In my mind the whole purpose of the rpc client is to provide a simple api for making rpc requests. As far as requests go I don't think the above example makes to client any less useful since it is just a couple of extra characters. For responses I also prefer to selectively parse into sig types when needed rather than parsing everything by default. At the end of the day though I don't really mind so if you want you can just go ahead and resolve this comment.
There was a problem hiding this comment.
I don't really mind so if you want you can just go ahead and resolve this comment.
I appreciate this, but I'd like to get on the same page about how the rpc client should be designed overall. I still don't follow your reasoning. If you prefer, we can continue this discussion in another medium.
the whole purpose of the rpc client is to provide a simple api for making rpc requests
But what does this mean? In this context, "providing an API" means you're allowing the user to work with pre-defined structs instead of strings. There's not much going on here other than converting between structs and strings, and sending strings with an http client. Imagine if we removed all type conversions from this client. Then the rpc client would be barely distinguishable from a generic http client. In that case, there would not be much of a point in having an rpc client library.
I don't think the above example makes to client any less useful since it is just a couple of extra characters.
I could extend the same argument to GetAccountInfoConfig. Why don't we eliminate that struct and have the user pass a json string instead? It's just a couple extra characters. In my mind, using Pubkey follows the same logic as using the other structs used by the rpc client. If you're going to provide an rpc client that does the type conversations, why stop halfway? Why not convert everything into safer and more useful types?
| @@ -344,6 +344,25 @@ pub const Client = struct { | |||
| return self.sendFetchRequest(allocator, types.RpcVersionInfo, request, .{}); | |||
| } | |||
|
|
|||
| const GetVoteAccountsConfig = struct { | |||
There was a problem hiding this comment.
In my mind the whole purpose of the rpc client is to provide a simple api for making rpc requests. As far as requests go I don't think the above example makes to client any less useful since it is just a couple of extra characters. For responses I also prefer to selectively parse into sig types when needed rather than parsing everything by default. At the end of the day though I don't really mind so if you want you can just go ahead and resolve this comment.
| @@ -54,6 +57,13 @@ pub const Pubkey = extern struct { | |||
| ) !void { | |||
| return base58.format(self.data, writer); | |||
| } | |||
|
|
|||
| pub fn jsonParse(_: Allocator, source: anytype, _: ParseOptions) !Pubkey { | |||
There was a problem hiding this comment.
Any reason to have the jsonParse function take an Allocator and ParseOptions even though these two are not used within the function?
There was a problem hiding this comment.
Yes. This function exists so that std.json can call it in a generic context. It always passes the same parameters to any instance of jsonParse.
| @@ -151,3 +151,19 @@ pub const RpcVersionInfo = struct { | |||
| }; | |||
|
|
|||
| pub const Signature = []const u8; | |||
|
|
|||
There was a problem hiding this comment.
More of a question: What goes into src/rpc/types.zig and what goes into src/rpc/client.zig. On a cursory look it looks as if the RPC responses are defined in src/rpc/types.zig while inpurs are defined inline in src/rpc/client.zig
Would that be a correct read of the pattern? If so, what the benefit to that vs having all types (both input and output) defined in src/rpc/types.zig
There was a problem hiding this comment.
I don't know why it's organized this way. I agree with you about reorganizing this. I've been messing with this in my spare time and I have methods file that defines all the request and response types, and associates the types with each other. I'll probably open a PR with this pretty soon.
No description provided.