[RPC] Cleanup startmasternode command #2833
Conversation
- Remove 'local' set support. Long-time NonOp. - Remove 'many' set support. Always NonOp. - General cleanup of the command's logic.
Adds a method to start multiple MNs with a single command, adding code coverage to the more 'heavy-lifter' portion of the startmasternode command.
Fuzzbawls
added
RPC
Needs Release Notes
| if (request.fHelp || request.params.size() < 2 || request.params.size() > 4 || | ||
| (request.params.size() == 2 && (strCommand != "local" && strCommand != "all" && strCommand != "many" && strCommand != "missing" && strCommand != "disabled")) || | ||
| ( (request.params.size() == 3 || request.params.size() == 4) && strCommand != "alias")) | ||
| (strCommand == "alias" && request.params.size() < 3)) |
There was a problem hiding this comment.
Improve this if:
if the strCommand is "alias" you must use either 3 or 4 params
if the strCommand is valid but not "alias" you must use 2 params
if the strCommand is not valid you should throw error anyways
There was a problem hiding this comment.
if the strCommand is "alias" you must use either 3 or 4 params
"alias" requires at least 3 arguments, this is specifically covered in this conditional check.
if the strCommand is valid but not "alias" you must use 2 params
This is no longer the case, as shown in the changes to the functional tests that use named arguments with the "all" command whilst reloading the masternode.conf file (here and here).
if the strCommand is not valid you should throw error anyways
This does that, and is tested in the functional tests (here) by returning a RPC_INVALID_PARAMETER error code. This is done towards the end of the RPC function call and not at the beginning intentionally (here).
There was a problem hiding this comment.
if the strCommand is not valid you should throw error anyways
This does that, and is tested in the functional tests (here) by returning a
RPC_INVALID_PARAMETERerror code. This is done towards the end of the RPC function call and not at the beginning intentionally (here).
Yep I saw that my main point was that (as a user) I would prefer that if I inserted an invalid strCommand the output would be error + how to use that rpc command (as it was done before) instead of a simple "Invalid strCommand" and same for the number of parameters
There was a problem hiding this comment.
I moved to this approach for a few reasons:
- It allows greater code coverage and testability.
- A user's natural instinct upon getting an
RPC_INVALID_PARAMETERerror (SHOULD) be to then check thehelpoutput. - This new behavior is much much more inline with the rest of our RPC commands, and greatly simplifies the logic checks being done for when to show the actual help output.
There was a problem hiding this comment.
ok still don't totally agree but I'm not blocking the PR for this small thing
Fuzzbawls
removed
the
Needs Release Notes
General cleanup of the
startmasternodeRPC command. This has been a long time coming, and rectifies some long-standing issues with the design and operability of the command.Adds code coverage for new failure cases as well as code coverage for multi-MN start (ie, not "alias"-only) start commands.