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

Would implementing a basic support for the JsonRPC 1.0 be useful on the client? #1312

Open
YaCodesDevelopment opened this issue Mar 5, 2024 · 3 comments

Comments

@YaCodesDevelopment
Copy link

If so, I could implement it, so that it wouldn't fail when doing requests to JsonRPC 1.0. (I'll test it on Bitcoin RPC node).

It's mostly useful for Bitcoin RPC I guess, as it's very popular use-case and it only supports 1.0

@niklasad1
Copy link
Member

niklasad1 commented Mar 5, 2024

Hey @YaCodesDevelopment.

@Velnbur commented on this recently as well

Sorry that I'm returning back to this discussion, but could we possibly have support of both protocol's versions as there not so many differences?

As a naїve solution it could be a compile time feature that "skips" these line. However I see a problem with cargo's dependencies resolving, when you will try to use both JSONRPC V1 and V2 in the same project

But also it could a separate client I think

UPD: Actually, also check that "error" is null of course

After double checking the JSON-RPC v1 spec the difference is as follows:

  • Notifications has "id":null instead of omitting that field
  • Responses, error and result can't be set at the "same time", for example if it's "error":{"bar"} then "result" must be null.

I'm not sure whether it's possible to support that via feature flags because it may have some edge causes especially
when an "id":null comes in call we may terminate the connection. To me it looks quite scary to support both... for instance we need to deserialize a call with id = null to notification not a request in v1 which is not the case in v2.

So if any of you want to tackle to add the feature flags "v1 and v2" to jsonrpsee_types go ahead with the risk that we may not accept it if there is no clean and maintainable solution to it (my gut feeling is that it may be easier to just create another library for it)

Another thoughts from you? Bitcoin is not going to migrate to JSON-RPC v2 soon?

@Velnbur
Copy link

Velnbur commented Mar 5, 2024

just create another library for it

I had the idea to create another library for it too, especially since I don't need the server part. It shouldn't be too difficult to implement.

Just wanted to know your thoughts about that, as I couldn't find any mentions that jsonrpsee supports only second version of JSONRPC spec.

Bitcoin is not going to migrate to JSON-RPC v2 soon?

It has been under development for over a year with some activity, but it seems it won't be merged in nearest future (checkout bitcoin/bitcoin#27101)

@YaCodesDevelopment
Copy link
Author

Hey @YaCodesDevelopment.

@Velnbur commented on this recently as well

Sorry that I'm returning back to this discussion, but could we possibly have support of both protocol's versions as there not so many differences?

As a naїve solution it could be a compile time feature that "skips" these line. However I see a problem with cargo's dependencies resolving, when you will try to use both JSONRPC V1 and V2 in the same project

But also it could a separate client I think

UPD: Actually, also check that "error" is null of course

After double checking the JSON-RPC v1 spec the difference is as follows:

  • Notifications has "id":null instead of omitting that field
  • Responses, error and result can't be set at the "same time", for example if it's "error":{"bar"} then "result" must be null.

I'm not sure whether it's possible to support that via feature flags because it may have some edge causes especially when an "id":null comes in call we may terminate the connection. To me it looks quite scary to support both... for instance we need to deserialize a call with id = null to notification not a request in v1 which is not the case in v2.

So if any of you want to tackle to add the feature flags "v1 and v2" to jsonrpsee_types go ahead with the risk that we may not accept it if there is no clean and maintainable solution to it (my gut feeling is that it may be easier to just create another library for it)

Another thoughts from you? Bitcoin is not going to migrate to JSON-RPC v2 soon?

Thanks for your reply. I guess in such case I'm really better of creating my own library. Sorry for bothering you)

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