consistent JSON return value for failed vote #1001
Conversation
* result should be either 'success' or 'failed' * error message belongs in 'errorMessage' field
|
Looking through the rpc interface code it looks like the code is designed to throw a JSONRPCError exception when an error occurs. Unfortunately there are a number of places in rpcgovernance.cpp where this practice wasn't followed but I think this is the better fix, rather than returning a JSON object directly here. |
|
I thought about that, but after looking thru the other places where it throws a JSONRPC error, it seems like that's only when a user enters the command incorrectly. In these cases it's not technically a JSONRPC error, esp. in cases where the vote is rejected because "time between votes is too soon". But in these places the commands are entered correctly, but the vote is simply failing for some reason or other (e.g. the above example). @UdjinM6 @schinzelh Your thoughts? |
|
Here's one example where JSONRPCError is thrown for an error that is not an incorrect input. I think this is bitcoin code, so probably more likely to follow the original design intent. https://github.com/dashpay/dash/blob/v0.12.1.x/src/wallet/rpcwallet.cpp#L249 |
|
Yep, there are lots of different rpc errors, see rpcprotocol.h I think there is a case where throwing rpc errors is useful: there are some typical errors and you need standard, deterministic way to identify them. For example you need rpc to throw some specific predefined error code if there is some additional logic in another software using this rpc and you need to notify such software what happened exactly. Throwing such errors also indicate complete termination of command execution which is also a strong signal that smth went terribly wrong. But I think it's ok to have error strings as a regular json output and no error thrown if execution can/should continue or no further action is needed in case of such error. For example there is no reason to throw an error for "masternode start-all" if say 1 masternode out of 10 failed to start. I have no strong opinion about this specific case but it looks like "vote-*" commands are user-only so probably ok to have json reported errors? I don't quite understand the reason for "vote-conf" and "vote-alias" to count failures and to have "overall" though - both of them should either fail or succeed once - multiple results could only happen for "vote-many" (but we don't have such command at all?). |
|
I don't really like the idea of constructing ad hoc error objects like this though. It seems error prone (ie. easy to accidentally construct a malformed object that clients won't see as an error, and I think this kind of bug may be relatively hard to spot in code review) and the client has to do more work to determine if an error occurred. I think calling JSONRPCError ensures that the standard error information is returned but it has the disadvantage of not allowing the code to include additional details that may be useful to the caller (more likely for human debugging purposes than for automated action). Maybe we should add an optional JSON parameter to JSONRPCError so that additional information can be easily returned. A simpler option would be call write() on some JSON object to produce a string and pack that into the string parameter of the exception, but I don't much like that either, especially if there is any possibility the client may need to parse the additional information (as opposed to just displaying it for debugging purposes). |
|
I'll merge this for now as it actually does fix inconsistency. We can come up with better system for rpc calls in general (if needed) in another PR. |
|
oot@iZ2zeeeup1v8x4tt6rzmr6Z:/var/www/html/mpos/cronjobs# ./findblock.php -f root@iZ2zeeeup1v8x4tt6rzmr6Z:~# dash-cli getblockhash Returns hash of block in best-block-chain at index provided. Arguments: index (numeric, required) The block index Examples: dash-cli getblockhash 1000 |
No description provided.