-
Notifications
You must be signed in to change notification settings - Fork 24
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
Replace REST with JSON-RPC #1644
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
emmiegit
force-pushed
the
WJ-1192-rpc
branch
2 times, most recently
from
October 16, 2023 05:00
ceb7201
to
5339cff
Compare
This reverts commit 50d1687. We are now using reqwest again, because we're switching to Tokio.
JSONRPC uses snake_case, we should be consistent in our structures and parameters here. Also this removes a lot of boilerplate we need to keep consistent since Rust uses snake_case for fields.
It's useless in JSON and takes a lot of visual space.
Our server always returns HTTP 200, so this is useless. We have json-rpc-2.0 create errors from failed requests, which are then thrown for us.
We're already using this one in lib/fetch.ts
Codecov Report
@@ Coverage Diff @@
## develop #1644 +/- ##
===========================================
- Coverage 41.02% 1.75% -39.28%
===========================================
Files 342 129 -213
Lines 10537 5128 -5409
===========================================
- Hits 4323 90 -4233
+ Misses 6214 5038 -1176
|
Yossipossi1
approved these changes
Oct 20, 2023
thanks @Yossipossi1 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Since we've finalized use of DEEPWELL as an internal API only, external-facing requirements no longer apply. We've already removed things like per-IP ratelimiting and now we've been facing a limitation of REST APIs, which is our desire to send more rich error data than one of a handful of relevant HTTP status codes. As such, we're shifting to a JSON-RPC-based internal API for DEEPWELL, which gets rid of the unnecessary HTTP complexities we don't need, and instead essentially has a remote method invocation system, with
serde_json
-backed serialization for input/output.tide
(the web server) has been entirely removed, and so a lot of the lines changed are from log line diffs.Likewise, I have taken the existing
services::Error
object and made it have a larger number of more specific error cases, since we are now sending those over the wire instead of bunching them into much more vague HTTP statuses.I have also been able to use the transaction closure, which is wrapped outside of each method. Thus, boilerplate related to setting up the transaction and ensuring it is committed is now removed. For most methods, this means the wrapper code is very simple. Take the
message_draft_send
method:For local testing of DEEPWELL (for instance, using curl), the endpoint is now
POST http://localhost:2747/jsonrpc
. This always returns HTTP 200, error information is passed in the JSON result, in accordance with the JSON-RPC standard. I have modified the necessary code in framerail as well, and refactored it a bit. There, we are now using thejson-rpc-2.0
library to act as a JSON-RPC client.