Page MenuHomePhabricator

Added Execute(JSONRPCRequest) overload in RPCCommandBase for wallet and help commands
ClosedPublic

Authored by jasonbcox on Feb 26 2019, 23:40.

Details

Summary

This is necessary for migration of wallet and help RPC commands that depend on
the entire JSONRPCRequest context.

Test Plan

ninja check

Diff Detail

Repository
rABC Bitcoin ABC
Branch
rpc-exec-req
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 5152
Build 8367: Bitcoin ABC Buildbot (legacy)
Build 8366: arc lint + arc unit

Event Timeline

deadalnix requested changes to this revision.Feb 28 2019, 17:39

Currently there is a grand total of zero commands that are handled by that framework. I'm not even sure this is required or not, but I'm fairly confident that it's better to have something that handle many the RPC calls rather than have something that could handle them all but handle none in practice. Beside priorities, it is also very possible that other limitation in the current design will be uncovered when actually using it, and any complication of the design will get in the way rather than help.

src/rpc/command.h
8 ↗(On Diff #7493)

Now everything using that header has a dependency on jsonrpcrequest.h, even all the cases you state are uncommon and only need RPCCommand.

21 ↗(On Diff #7493)

On could hardly come up with a less descriptive name. The fact that RPCCommandBase feels the needs to explain how it should be inherited from is a strong tell that the design is not very solid (See liskov's substitution principle).

It'd better that this indicate what it does via it's name and API, rather than ask everybody else to do something specific, which never works.

57 ↗(On Diff #7493)

It looks like this is the only place where the dependency is actually required.

src/rpc/server.cpp
72 ↗(On Diff #7493)

This is actually UB. GetName from a moved command can return pretty much anything.

src/test/rpc_server_tests.cpp
66 ↗(On Diff #7493)

Don't have all the test cases be the same.

This revision now requires changes to proceed.Feb 28 2019, 17:39
jasonbcox added inline comments.
src/rpc/server.cpp
72 ↗(On Diff #7493)

Good catch. Fixed here: D2632

jasonbcox marked an inline comment as done.

Fixed according to feedback

Currently there is a grand total of zero commands that are handled by that framework. I'm not even sure this is required or not, but I'm fairly confident that it's better to have something that handle many the RPC calls rather than have something that could handle them all but handle none in practice. Beside priorities, it is also very possible that other limitation in the current design will be uncovered when actually using it, and any complication of the design will get in the way rather than help.

The reason zero commands exist on this framework is because I'd like to implement the help RPC command before implementing any others (as it's a direct dependency that encourages poor practices in the current RPC command set such as inconsistent help message formatting across commands). The unfortunate part about migrating help is that the old help message functions depend on the entire JSONRPCRequest context. This can be replaced in the future, but as of now, this change is needed so that we can begin migrating away from the old help implementation without introducing those same poor practices in our own code.

deadalnix requested changes to this revision.Mar 4 2019, 22:02
deadalnix added inline comments.
src/rpc/commandwithargscontext.h
29 ↗(On Diff #7577)

You can just move that in an implementation file and be done with it instead of putting it int he header, especially since you expect that no user of this class will actually use JSONRPCRequest (it's the whole point of the class to begin with).

src/rpc/commandwithrequestcontext.h
22 ↗(On Diff #7577)

As the base of the hierarchy, it cannot be an RPCCommandWIthSomething as that imply it's a special kind of RPCCommand, and therefore not the base class.

Most of software design is about listening to what the code is telling us.

This revision now requires changes to proceed.Mar 4 2019, 22:02
src/rpc/server.cpp
70 ↗(On Diff #7577)

Another thing that the code is telling us is that the RPCServer is dealing with only one kind of command. It is therefore likely not a special kind of command.

Added command.cpp and renamed RPCCommandWithRequestContext -> RPCCommand

deadalnix added inline comments.
src/rpc/command.cpp
1 ↗(On Diff #7587)

Change

8 ↗(On Diff #7587)

Remove

src/rpc/command.h
55 ↗(On Diff #7587)

You want to take const std::string& instead of copying and destructing all over the place.

This revision is now accepted and ready to land.Mar 5 2019, 15:41
This revision was automatically updated to reflect the committed changes.