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

use serde_json::Value for Client::request() #477

Closed
gbaranski opened this issue Sep 17, 2021 · 9 comments
Closed

use serde_json::Value for Client::request() #477

gbaranski opened this issue Sep 17, 2021 · 9 comments

Comments

@gbaranski
Copy link

It'd be much easier to use serde_json::Value and serialize structs using serde_json::to_value

@niklasad1
Copy link
Member

Can you elaborate what you mean with a reference to a code snippet?

I guess you are talking about the RpcParams but those can not be any JsonValue according to the spec those must be array or map FWIW.

I don't get the benefit of using serde_json::to_value that seems more complicated than using serde_json::to_string as we still need to convert them to Bytes/String in order to send over the network via hyper.

@gbaranski
Copy link
Author

gbaranski commented Sep 17, 2021

Can you elaborate what you mean with a reference to a code snippet?

I guess you are talking about the RpcParams but those can not be any JsonValue according to the spec those must be array or map FWIW.

I don't get the benefit of using serde_json::to_value that seems more complicated than using serde_json::to_string as we still need to convert them to Bytes/String in order to send over the network via hyper.

e.g

#[derive(Serialize, Deserialize)]
struct SomeStruct {
    a: u8,
    b: u32,
}

let rpc_client = ...;
let some_instance = SomeStruct { a: 2, b: 534 };
rpc_client.request("send-some-struct", some_instance);

that'd be best and easiest to use if request function would accept T: serde::Serialize.

another way would be

#[derive(Serialize, Deserialize)]
struct SomeStruct {
    a: u8,
    b: u32,
}

let rpc_client = ...;
let some_instance = SomeStruct { a: 2, b: 534 };
rpc_client.request("send-some-struct", serde_json::to_value(some_instance).unwrap());

but that involves some boilerplate.

At the moment request() accepts JsonRpcParams which is quite painful to use.

@niklasad1
Copy link
Member

Ok, I see.

The intention is that you shouldn't use Client::request directly instead you can generate your API with the proc macro API.
Have a look at the example here

Thus, you need to define a trait and then you don't have to do the serialize/deserialize stuff it's generated for you

Sure, your idea could work but the nothing that prevents that from passing in a String or some invalid JSON-RPC param type.

@gbaranski
Copy link
Author

Ok, I see.

The intention is that you shouldn't use Client::request directly instead you can generate your API with the proc macro API.
Have a look at the example here

Thus, you need to define a trait and then you don't have to do the serialize/deserialize stuff it's generated for you

Sure, your idea could work but the nothing that prevents that from passing in a String or some invalid JSON-RPC param type.

Ah, I'd like to avoid using any macros, they always introduce a lot of problems.

@niklasad1
Copy link
Member

niklasad1 commented Oct 14, 2021

Closing because this is not priority from our side as the main use case is to use api macros to generate a client API to avoid having to do manual conversion of JSON-RPC params.

However, we have now introduced rpc_params! that you can use to convert types (that impl serde::Serialize) to JsonRpcParams

  let x = 0_usize;
  let y = "hello";
  client.request("my_method", rpc_params! [x, y]);

@FistedByDionysus
Copy link

FistedByDionysus commented Sep 7, 2022

+1 rpc_params is super annoying to use when trying to use against against an external json rpc endpoint. I gave up and went back to just using an http client directly.

If I have time I'll open up a PR that adds support for a client.request_json with a serde_json struct

edit

After thinking about this for a minute. To clarify is the idea you would do the following?

#[rpc(client)]
pub trait SomeNonRustJsonRpc {
    fn echo(&self, val: String) -> RpcResult<String>;
}

@niklasad1
Copy link
Member

If I have time I'll open up a PR that adds support for a client.request_json with a serde_json struct

you mean an API like:

client.request_json(json!({"jsonrpc":"2.0,"method": "foo","id":1})).await.unwrap()

I guess that is okay but would return errors at runtime for invalid JSON-RPC calls instead.
FWIW, the idea is that users doesn't have to deal with lower-level JSON-RPC stuff such as ID and jsonrpc version stuff which seems unnecessary unless you already got a raw JSON-RPC request.

After thinking about this for a minute. To clarify is the idea you would do the following?

#[rpc(client)]
pub trait SomeNonRustJsonRpc {
    #[method(name = "foo")]
    fn echo(&self, val: String) -> RpcResult<String>;
}

That would generate some code under the hood that serialized/converting the params to JSON-RPC Params but it won't allow you send raw JSON just the params.
So you could just do

#[rpc(client)]
pub trait SomeNonRustJsonRpc {
    #[method(name = "foo")]
    fn echo(&self, val: String) -> RpcResult<String>;
}

async fn main() {
   let client = WsClientBuilder::default().build("ws://myurl.com:80).await?;
   let echo = client.echo("lo".to_string().await.unwrap();
}

@niklasad1
Copy link
Member

Related to that we just merged #864,

So you should be able to:

   let json = json!({"p": 1}); 
   let params = ArrayParams::new();
   params.insert(&json).unwrap();
   rpc.request("method", params).await.unwrap();

but we could probably impl TryFrom<T: Serialize> for ArrayParams as well then one could do:

   let json = json!({"p": 1}); 
   rpc.request("method", json.try_into().unwrap()).await.unwrap();

//cc @lexnv thoughts?

@FistedByDionysus
Copy link

@niklasad1 Thanks for the response! I ended up using the proc_macro which works great.

I think for situations where someone can't use the proc macros but still wants to use the client the PR you posted is pretty ergonomic. A TryFrom impl would be icing on the cake.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants