Page MenuHomePhabricator

Added RPCServer and defined its API, wrapping the old implementation
ClosedPublic

Authored by jasonbcox on Sep 5 2018, 04:13.

Details

Diff Detail

Repository
rABC Bitcoin ABC
Branch
server-rpc
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 3439
Build 4954: Bitcoin ABC Buildbot (legacy)
Build 4953: arc lint + arc unit

Event Timeline

deadalnix requested changes to this revision.Sep 5 2018, 19:14
deadalnix added inline comments.
src/httpserver.cpp
66 ↗(On Diff #4766)

This seems highly suspicious to me. How's the server a property of a work item ? Should the server be processing work item ?

OK so you got a RPCServer. There is also a HTTP server. The HTTPServer recieve http request and build work items from them and process them. If the work item end up being some rpc request, then the HTTPServer hands it over to the RPCServer.

The RPCServer should be a property of the HTTPServer, not of the work item.

src/init.h
9 ↗(On Diff #4766)

Forward declare instead of including.

This revision now requires changes to proceed.Sep 5 2018, 19:14
jasonbcox added inline comments.
src/httpserver.cpp
66 ↗(On Diff #4766)

Chatted offline. There does appear to be a better way of doing this. Will update the diff once I've gotten it working.

Building on top of D1736 instead of D1630.
Fixed according to feedback as well.

Apparently my laptop isn't building with Qt enabled...

deadalnix requested changes to this revision.Sep 27 2018, 18:38
deadalnix added inline comments.
src/rpc/register.h
6 ↗(On Diff #5089)

Please do this kind of stuff in their own diff.

29 ↗(On Diff #5089)

RegisterAllRPCCOmmands should register all RPC commands.

src/rpc/server.cpp
44 ↗(On Diff #5089)

This is duplicated from somewhere. It would deserve to be merged.

49 ↗(On Diff #5089)

The RPC server should probably be agnostic to the type of commands it runs.

504 ↗(On Diff #5089)

All RPCCommand should look the same from the perspective of the server. See Liskov's substitution principle.

src/test/test_bitcoin.cpp
87 ↗(On Diff #5089)

All RPC command contains all the context free ones.

This revision now requires changes to proceed.Sep 27 2018, 18:38
jasonbcox added inline comments.
src/rpc/server.cpp
49 ↗(On Diff #5089)

It will be once I migrate these calls. This is a just a note to anyone working on this code until that change is made.

504 ↗(On Diff #5089)

Perhaps this comment is much less valid now that I've added the TODO. Similarly, I expect this to go away once the migration is complete. I'll remove this comment entirely for now since it's both redundant and will be confusing if failed to be removed.

jasonbcox added inline comments.
src/rpc/register.h
6 ↗(On Diff #5089)
jasonbcox added inline comments.
src/rpc/server.cpp
44 ↗(On Diff #5089)

I plan on removing it from CRPCTable::execute() and removed all callsites to execute() except for RPCServer::ExecuteCommand(). That enables me to clean up the duplicate. However, this will need to be a separate diff.

I can consolidate them into a standalone function if you like.

504 ↗(On Diff #5089)

On second thought, this should stay because it points readers to the correct portion of code until context-free commands are fully migrated.

jasonbcox marked an inline comment as done.

Adjusted according to feedback.

This revision is now accepted and ready to land.Oct 1 2018, 18:17
This revision was automatically updated to reflect the committed changes.