RPC: Strict JSON-RPC 2.0 compliance (gated behind flag) #12435
Conversation
src/univalue/include/univalue.h
Outdated
There was a problem hiding this comment.
univalue changes need to be submitted upstream, not here :)
|
Concept ACK |
src/rpc/server.cpp
Outdated
There was a problem hiding this comment.
I commented in the univalue PR that this catch should probably be for a derived class of std::exception, not int. If this is the case, there will be no need to re-throw, as the std:exception handler will catch all else below.
There was a problem hiding this comment.
Yes, throwing an int is really weird.
src/httprpc.cpp
Outdated
There was a problem hiding this comment.
Does this create different behavior for degenerate returns when not passing -strictjsonrpcspec?
src/httprpc.cpp
Outdated
There was a problem hiding this comment.
Could just do std::string strReply(ret.write() + "\n");
src/rpc/protocol.cpp
Outdated
There was a problem hiding this comment.
Should this have some kind of g prefix given it's a global? Also, may want to use snake_case.
There was a problem hiding this comment.
Yes, should be g_strict_jsonrpc_spec or such w/ the new guidelines.
src/univalue/include/univalue.h
Outdated
There was a problem hiding this comment.
These exception constructors are usually made explicit to prevent implicit conversion. You can see uint_error as an example.
|
Tests are failing on the latest commit. Looking in to it. |
|
doh! |
|
Squash needed at some point. |
|
Nice. Concept ACK. |
esotericnonsense
force-pushed
the
2018-02-jsonrpc-2.0
branch
3 times, most recently
from
43f349c to
76eb200
Compare
|
My attempt to squash this seems to have gone awfully wrong. Trying to fix it up... edit: should be fine now. |
esotericnonsense
force-pushed
the
2018-02-jsonrpc-2.0
branch
from
76eb200 to
d87d60a
Compare
|
Related: #2960 |
| extern const UniValue NullUniValue; | ||
|
|
||
| const UniValue& find_value( const UniValue& obj, const std::string& name); | ||
| const UniValue& find_value(const UniValue& obj, const std::string& name, enum UniValue::keystatus *status=NULL); |
There was a problem hiding this comment.
Are you sure this univalue API change is really necessary? You could find out if the key is there with an additional call to Univalue::exists? (or am I missing something)
|
(travis failure is due to subtree check for univalue) |
| The last travis run for this pull request was 151 days ago and is thus outdated. To trigger a fresh travis build, this pull request should be closed and re-opened. |
| Needs rebase |
|
Closing this for now, and adding "up for grabs" tag. Let me know if you want to start work on this again and I'll reopen… |
|
Removing "Up for grabs" as JSON 2.0 was done in #27101. cc @pinheadmz |
8 participants
This patch adds a -strictjsonrpcspec flag.
If the flag is used, bitcoind enters JSON-RPC 2.0 mode, which allows it to be fully spec-compliant (and thus work with libraries like libjson-rpc-cpp without modification).
I've added a functional test for the specific bits of the spec that I've changed.
univalue changes are included in this commit for ease of review but I can pull those out (see bitcoin-core/univalue-subtree#12)