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

JSON RPC notification #1017

Closed
xlc opened this issue Feb 15, 2023 · 1 comment · Fixed by #1021
Closed

JSON RPC notification #1017

xlc opened this issue Feb 15, 2023 · 1 comment · Fixed by #1021

Comments

@xlc
Copy link
Contributor

xlc commented Feb 15, 2023

Not that I need this feature at all but reading the code makes me think the notification feature implementation isn't really correct.

Firstly, notifications are completely ignored (if handled). I don't see any interface exposed by jsonrpsee to allow subscription of notifications.

Secondly, the websocket server handles notifications inconsistently. For batch request, it will parse notifications and suppress responses. However, for single request, it simply error out with parse error, which I don't think is not spec confirming.

@niklasad1
Copy link
Member

niklasad1 commented Feb 16, 2023

Firstly, notifications are completely ignored (if handled). I don't see any interface exposed by jsonrpsee to allow subscription of notifications.

For the server, that is correct jsonrpsee only support ethereum pubsub which is similar.
We opened an issue for that a while ago but no interest so we decided to close it.

Secondly, the websocket server handles notifications inconsistently. For batch request, it will parse notifications and suppress responses. However, for single request, it simply error out with parse error, which I don't think is not spec confirming.

That's is not correct.
For JSON-RPC notifications:

  • HTTP server: just ACK the notification by send HTTP status code 200
  • WS server: just ignore
  • Batch response: notifications are ignored and only calls are processed

Ok, I see I introduced a bug when I did some refactoring for the WS stuff.

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 a pull request may close this issue.

2 participants