This is necessary for migration of wallet and help RPC commands that depend on
the entire JSONRPCRequest context.
Details
Details
- Reviewers
deadalnix Fabien - Group Reviewers
Restricted Project - Commits
- rSTAGING48eef2eed953: Added Execute(JSONRPCRequest) overload in RPCCommandBase for wallet and help…
rABC48eef2eed953: Added Execute(JSONRPCRequest) overload in RPCCommandBase for wallet and help…
ninja check
Diff Detail
Diff Detail
- Repository
- rABC Bitcoin ABC
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
Comment Actions
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. |
Comment Actions
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.
| 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. |
| 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. |