-
Notifications
You must be signed in to change notification settings - Fork 108
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
feature(rpc): add an rpc server to Zebra #3589
Conversation
The RPC server of this PR is off by default. A custom config with
With that in place, when
With zebra running, from a new window one can execute the
|
e4ca421
to
b51e994
Compare
Codecov Report
@@ Coverage Diff @@
## main #3589 +/- ##
==========================================
+ Coverage 78.34% 79.86% +1.51%
==========================================
Files 267 280 +13
Lines 31526 32530 +1004
==========================================
+ Hits 24698 25979 +1281
+ Misses 6828 6551 -277 |
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.
Thanks, this looks really good.
I noticed some things that might be bugs:
- blocking the tokio runtime
- not checking if the RPC server is still running
- using the default
#[rpc]
client derive
I also had some design questions we might want to discuss with the team:
- does the RPC server code go in
zebra-rpc
orzebrad
? - what config should we use to disable the RPC port?
This tests for JSON RPC 2.0, but $ curl -H "Content-Type: application/json" -d '{"method": "getinfo", "params":[], "id":123 }' 127.0.0.1:8232
{"result":{"build":"Zebra v1.0.0 ...","subversion":"/Zebra:1.0.0-beta.4/"},"id":123} |
This is the first RPC used by lightwalletd, so we need it for testing.
I did some testing with It sends an incorrect "jsonrpc: 1.0" field in requests, so I created this fix for Zebra: With this config file:
Before the fix, I see:
This is a bug in Zebra, we should accept the request. After the fix, I see:
This failure is the correct behaviour, because the |
fix(rpc): ignore non-standard "jsonrpc = 1.0" in lightwalletd requests
Co-authored-by: teor <[email protected]>
Co-authored-by: teor <[email protected]>
We expect to use all the listed tokio features to implement and test RPC methods.
I repeated these tests with commit 49f563c in this PR. I got the same response from |
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.
I did a bunch of cleanups, but the original code was fine and working.
I tested a ZcashFoundation repository PR and all the tests passed:
So it looks like the other test failures were due to repository secrets bugs, we've got tickets for those.
Nice work, thanks @teor2345 |
Motivation
We want to add an rpc component into zebra so we can start serving rpc commands. The crates we are using are the ones described at #3140
This pull request will close #3140 if the general idea is accepted and the pull request gets merged.
The RPC config also closes #3141.
Solution
A new component was added and a first call
getinfo
was mocked to test this is working. So this pull request also started the work described in #3141 and #3142 but more work is needed to close those.This was tested manually (#3589 (comment)), no tests were done as they will be added in the context of #3165
Review
I am mainly interested in feedback for the overall idea, there are currently details to be fixed. Any review from anyone is welcome.
Reviewer Checklist
Follow Up Work
No further features should be added to this pull request so we can keep the scope small, there is more to do in the other related tickets.