-
Notifications
You must be signed in to change notification settings - Fork 383
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
fix: add support for a ws client & batch processing over ws #1498
fix: add support for a ws client & batch processing over ws #1498
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1498 +/- ##
==========================================
+ Coverage 46.41% 48.36% +1.95%
==========================================
Files 487 409 -78
Lines 68973 62035 -6938
==========================================
- Hits 32011 30006 -2005
+ Misses 34313 29531 -4782
+ Partials 2649 2498 -151 ☔ View full report in Codecov by Sentry. |
This is amazing. Thank you Milos. 🎉 Would you like to have a preliminary review, or just have the team try to fool around and break ws batch processing? |
@gfanton @ajnavarro |
From the review meeting:
|
I've merged in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good! 👍
Just a few suggestions, nothing critical
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After reviewing @gfanton comments, LGTM
Hey @zivkovicmilos, it seems that your example for testing out the WS batch request is not working correctly:
Not sure if this is something introduced by a recent change. |
Are you sure you've built the |
Description
Let me start this PR description by explaining what I wanted to accomplish, so you are not discouraged when reading the file changes.
I wanted to:
It might seem odd, reading the 3rd point and thinking this is not supported. The truth is actually much more troubling.
Our JSON-RPC server (and client!) implementations are not great. HTTP requests are handled and parsed in a completely different flow than WS requests, even though the output of both should be identical (just over different mediums). Lots of logic is coupled, making it hard to extract and simplify. I've broken the WS / HTTP implementation multiple times over the course of this PR, and all of the tests were passing, even though I've critically broken the module at the time.
The client code for the JSON-RPC server requires a response type (of course, for Amino result parsing) to be given at the moment of calling, which is not amazing considering our WS implementation is async, and doesn't have these response types in the result parsing context (these logic flows are handled by different threads).
What I ended up doing:
baseRPCClient
). The slightly tweaked API (for creating HTTP / WS clients, and using batches) is much simpler to use, and actually has error handling.I will start an effort in the near future for refactoring the JSON-RPC server code from the ground up in a subsequent PR, this time with specs and tests at the forefront.
How to test out the websockets
To test out the WS responses, you can use a tool like websocat.
websocat ws://127.0.0.1:26657/websocket
(this is the default URL)How to test out the updated client code
I created the following snippet for easily testing the functionality updated in this PR:
cc @dongwon8247
Contributors' checklist...
BREAKING CHANGE: xxx
message was included in the description