Fix error types #89
Fix error types #89
Conversation
idky137
added
bug
| @@ -126,15 +126,15 @@ impl JsonRpcConnector { | |||
| if body_str.contains("Work queue depth exceeded") { | |||
| if attempts >= max_attempts { | |||
| return Err(JsonRpcConnectorError::new( | |||
| "Work queue depth exceeded after multiple attempts", | |||
| "Error: The node's rpc queue depth was exceeded after multiple attempts", | |||
There was a problem hiding this comment.
im not sure if we should consider this blocking but I think we should try to avoid repeated Error: Error: Error status xxxx in the format display of error types. Im not sure what the standard practice is but my preferred method is to not have "Error:" in the format display of error types (or this case strings which we should change to error types and implement fmt::display at some point soon). The idea here is all the different error types can navigate many different levels and enums without "collecting" more "Error:"s at the start. then finally, at the top level where it will be called directly by the user/consumer we put "Error: {}" in the final fmt:display. I have seen you have done this later in the PR but i havent checked this is the top level fmt::display.
There was a problem hiding this comment.
the final error printed to user should preferably look like this:
"Error: Serialisation error. 2nd level error. 2nd level thing failed." for example. IMO
There was a problem hiding this comment.
this would be made up of the top level error message for example in an eprintln:
match result {
Ok(_) => (),
Err(top_level_error_enum) => eprintln!"Error: {}", top_level_error_enum)
}
then the top level error enum with thiserror macro:
pub enum TopLevelError {
#error("Serialisation error. {0}")
SerialisationFailed(SerialisationError),
#error("Some other error. {0}")
SomeOtherError,
}
then serialisation error enum:
enum SerialisationError {
#error("2nd level thing failed. {0}")
2ndLevelThingFailed(2ndLevelThing),
#error("2nd level other thing failed. {0}")
2ndLevelOtherThing,
}
where 2ndLevelthing might be some struct with useful info like if its a block height for example it would produce:
"Error: Serialisation error. 2nd level error. 2nd level thing failed. Blockheight: 10"
| { | ||
| break; | ||
| if height > chain_height { | ||
| let _ = channel_tx |
There was a problem hiding this comment.
sometimes the compliler asks to put "let _ = " but i think this is bad advice. sometimes its useful to use this to keep something in scope but i still prefer "let _unused_name_of_thing =" in stead of just underscore for that. in this case i believe you need to actually handle your result with a match and error handling or atleast an expect if it cant fail
| { | ||
| break; | ||
| if height > chain_height { | ||
| let _ = channel_tx |
There was a problem hiding this comment.
see comment above about "let _ ="
Fixes the errors returned for get_block, get_block_range, get_nullifiers, get_nullifiers_range RPCs.