Replacing unwraps with Errors in application code #492
Conversation
|
The linting won't pass until all unwraps have been removed |
|
So, you really are going to convert ALL of the unwraps. Damn.
|
|
Resolved conflicts |
|
Tests are fine, github actions has a problem right now. Will try again later. |
There was a problem hiding this comment.
Pretty nice overall.
I'd say we need ways to avoid developers pushing the usage of except, too. By maybe utilizing error handling to it's fullest (mentioned in #481), check scripts for PR's (reports the new except usages) or stricter reviews.
core/src/rpc/aggregator.rs
Outdated
| let movetx_agg_nonce = nonce_agg_handle.await.unwrap()?; | ||
| let movetx_agg_nonce = nonce_agg_handle | ||
| .await | ||
| .expect("cancelled task or panic in task")?; |
There was a problem hiding this comment.
Can't we return Status instead?
There was a problem hiding this comment.
i think an inner panic should be propagated to an outer panic. We're now making it so that panics are unrecoverable errors, we shouldn't try to recover from them.
for cancelled task, I'm not sure what it actually means. I'll check
There was a problem hiding this comment.
This should be fine I think:
let movetx_agg_nonce = nonce_agg_handle
.await.map_err(|e| Status::aborted(format!("Failed to aggregate nonces: {:?}", e)))??;There was a problem hiding this comment.
Just checked but the unwrap will only panic if the underlying task panics in our case. Check JoinError for more info.
Panics should happen with unrecoverable errors (like memory corruption, invariant violation, etc). In such cases, terminating the program is a better option than attempting to return an error IMO.
There was a problem hiding this comment.
Ok but if that happens and except panics, doesn't aggregator server will stop responding instead of just returning an error code to the caller? Why would we want to aggregator to terminate? This could happen if one of the params is incorrect. This will punish an honest and correct aggregator.
There was a problem hiding this comment.
We shouldn't panic on an incorrect parameter, but I'll still return an internal error here.
|
There are some |
| url, | ||
| auth, | ||
| client: rpc, | ||
| } | ||
| client: Arc::new(rpc), |
There was a problem hiding this comment.
Can you please explain why the changes here?
There was a problem hiding this comment.
Client is not Clone but Arc<Client> is, since Arc has an infallible clone impl. The difference is, we're not constructing a new Client on every clone but sharing a reference to the same client. If the inner Client needs to be cloned, we now have a new clone_inner function that's fallible.
All client RPC functions take the client by reference, so we don't need anything mutable anyway.
mmtftr
added
code quality
mmtftr
changed the title
Description
This PR aims to resolve the usage of unwraps throughout the codebase. The aim is to add insightful error messages to all unwrap() callsites.
Linked Issues
Testing
Added a
unwrap_usedlint that will error whenunwrapis used outside of tests.