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

Required jsonrpc string propriety inside the JSON rpc method request. #4742

Merged
merged 2 commits into from
Aug 30, 2021

Conversation

vincenzopalazzo
Copy link
Collaborator

@vincenzopalazzo vincenzopalazzo commented Aug 26, 2021

I discovered that my java RPC wrapper has some missing requirements with the jsonrpc 2.0 protocol, and the unusual thing is that it is accepted inside c-lightning a quest without "jsonrpc":"2.0" and as specified in the specification protocol the string must be present inside the request.

  • First run it is only for testing.

Signed-off-by: Vincenzo Palazzo vincenzopalazzodev@gmail.com

@vincenzopalazzo vincenzopalazzo changed the title Required jsonrpc inside the method request. Required jsonrpc string propriety inside the JSON rpc method request. Aug 26, 2021
@rustyrussell
Copy link
Contributor

I know Spark used not to do this.

@shesek does it now?

@vincenzopalazzo
Copy link
Collaborator Author

Worried about breaking stuff with this change, I mean it is not a big deal this check from a not protocol view. However, I was thinking to have a deprecated period for this?

As the json rpc specification tell, the "jsonrpc": "2.0" MUST be required.

Signed-off-by: Vincenzo Palazzo <vincenzopalazzodev@gmail.com>
shesek added a commit to shesek/lightning-client-js that referenced this pull request Aug 27, 2021
@shesek
Copy link
Contributor

shesek commented Aug 27, 2021

Spark uses the clightning-client library, which was forked from BHB's lightning-client, which apparently indeed does not include the jsonrpc field.

I fixed that just now in shesek/lightning-client-js@78e103b and released clightning-client v0.1.4 with the fix. The next releases of Spark and Lightning Charge will use that.

Worried about breaking stuff with this change, I mean it is not a big deal this check from a not protocol view. However, I was thinking to have a deprecated period for this?

Agreed, I think there should be a deprecation period for this. I know that quite a few people still use old releases of Spark.

Signed-off-by: Vincenzo Palazzo <vincenzopalazzodev@gmail.com>

Changelog-Deprecated: RPC framwork now require the "jsonrpc" propriety inside the request.
Changelog-Fixed: RPC framwork now required the "jsonrpc" propriety to be specified inside each request.
@vincenzopalazzo
Copy link
Collaborator Author

Added deprecated_apis checks, and added changelog fix and changelog deprecated fields in the last commit.

Copy link
Contributor

@rustyrussell rustyrussell left a comment

Choose a reason for hiding this comment

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

Ack fe294dc

@rustyrussell rustyrussell merged commit f2d0e93 into ElementsProject:master Aug 30, 2021
@rustyrussell
Copy link
Contributor

Thanks everyone, this is great!

@vincenzopalazzo vincenzopalazzo deleted the rpc20 branch March 7, 2022 21:06
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

Successfully merging this pull request may close these issues.

3 participants