Page MenuHomePhabricator

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

Authored by jasonbcox on Tue, Feb 26, 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
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.Tue, Feb 26, 23:40
Herald added a reviewer: Restricted Project. · View Herald TranscriptTue, Feb 26, 23:40
Herald added a subscriber: schancel. · View Herald Transcript
deadalnix requested changes to this revision.Thu, Feb 28, 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.Thu, Feb 28, 17:39
jasonbcox marked an inline comment as done.Fri, Mar 1, 00:56
jasonbcox added inline comments.
src/rpc/server.cpp
72 ↗(On Diff #7493)

Good catch. Fixed here: D2632

jasonbcox updated this revision to Diff 7577.Mon, Mar 4, 20:41
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.Mon, Mar 4, 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.Mon, Mar 4, 22:02
deadalnix added inline comments.Mon, Mar 4, 22:03
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.

jasonbcox updated this revision to Diff 7587.Mon, Mar 4, 22:47

Added command.cpp and renamed RPCCommandWithRequestContext -> RPCCommand

deadalnix accepted this revision.Tue, Mar 5, 15:41
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.Tue, Mar 5, 15:41
jasonbcox updated this revision to Diff 7605.Tue, Mar 5, 17:18

Feedback

This revision was automatically updated to reflect the committed changes.