Make handleValidatorExitCommand work with REST API
#3318
Conversation
| @@ -333,37 +333,37 @@ proc check_voluntary_exit*( | |||
|
|
|||
| # Not in spec. Check that validator_index is in range | |||
| if voluntary_exit.validator_index >= state.validators.lenu64: | |||
| return err("Exit: invalid validator index") | |||
| return err("Invalid validator index") | |||
There was a problem hiding this comment.
the state transition has a convention to include where the error is coming from - voluntary exit processing shoud not deviate from it, this needs to be changed back
There was a problem hiding this comment.
I think this was done to provide a more reasonable message as an error output of the REST API, but it can be handled locally there (which would be admittedly a bit hacky). A more proper engineering solution would be to replace these strings with an enum.
There was a problem hiding this comment.
well, I'd prefer that the state transition stays clean - the rest api error outputs aren't hurt by an extra "exit", but the we use these strings in other places as well where it makes sense in their context that they're formatted the way they are (for example when dealing with invalid blocks) - this is more important than rest error messages.
There was a problem hiding this comment.
The broader point here is that the convention exists for a reason (it wasn't put there frivolously): when starting to use the functions in a new context, that reason must be taken into account so that the old functionality doesn't regress: there are two ways to approach that:
- adapt the new functionality to the current status quo
- change the current status quo to be fully consistent with both the new use cases and the old use cases
Either is fine, but leaving the code half-way broken is not.
|
This change needs a corresponding update to the manual: previously we required running the node with json-rpc enabled - now it will be REST instead. |
| @@ -542,16 +542,16 @@ type | |||
| name: "validator" | |||
| desc: "Validator index or a public key of the exited validator" }: string | |||
|
|
|||
| rpcUrlForExit* {. | |||
There was a problem hiding this comment.
this command line parameter should remain: when someone uses it, a warning should be printed pointing to the new option.
No description provided.