Details
- Reviewers
deadalnix schancel Mengerian - Group Reviewers
Restricted Project - Commits
- rSTAGINGeeede0fd2a21: Added RPCServer and defined its API, wrapping the old implementation
rABCeeede0fd2a21: Added RPCServer and defined its API, wrapping the old implementation
make check && test_runner.py
Diff Detail
- Repository
- rABC Bitcoin ABC
- Branch
- server-rpc
- Lint
Lint Passed - Unit
No Test Coverage - Build Status
Buildable 3365 Build 4811: Bitcoin ABC Buildbot (legacy) Build 4810: arc lint + arc unit
Event Timeline
| 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. |
| 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. |
| 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. |
| 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. |
| 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. |