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
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

jasonbcox created this revision.Sep 5 2018, 04:13
Herald added a reviewer: Restricted Project. · View Herald TranscriptSep 5 2018, 04:13
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 planned changes to this revision.Sep 6 2018, 15:45
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.

jasonbcox updated this revision to Diff 5038.Sep 25 2018, 23:34

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

jasonbcox planned changes to this revision.Sep 26 2018, 00:10

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

jasonbcox updated this revision to Diff 5045.Sep 26 2018, 00:46

Fixed build

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 planned changes to this revision.Sep 27 2018, 22:42
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 marked 2 inline comments as done.Sep 27 2018, 22:47
jasonbcox added inline comments.
src/rpc/register.h
6 ↗(On Diff #5089)
jasonbcox marked 6 inline comments as done.Sep 28 2018, 00:11
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 edited the summary of this revision. (Show Details)Sep 28 2018, 20:50
jasonbcox updated this revision to Diff 5166.Sep 28 2018, 21:37
jasonbcox marked an inline comment as done.

Adjusted according to feedback.

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