Review: Check ALL Usage of `unwrap()`

[hackfisher]

Unwrap panic could cause node hanging.

Like #151.

[clearloop]

The panic part currently exists at chainrelay/eth/relay, map_err into Receipt,

let receipt = rlp::decode(&value).map_err(|_| "Deserialize Receipt - FAILED")?;

Below are two viewpoints

It might be safe now?

If we handler the Err on the caller of the function which contains this line, it is safe.

Probably need more tests for the decode method?

Or we can force the decode method to return Ok, and there will be no more panic about this method at all, but we have to do more checks

This force Ok solution is not a good practice, because it's too complex.

[aurexav]

Move to #353.

[hackfisher]

The panic part currently exists at chainrelay/eth/relay, map_err into Receipt,

let receipt = rlp::decode(&value).map_err(|_| "Deserialize Receipt - FAILED")?;

It might be safe now? or probably need more tests for the decode method?

Didn't get it, do you mean it could panic in decode?

map_error is used to avoid no handling of Error?

[aurexav]

No, this just map decode err into &str err.
https://doc.rust-lang.org/std/result/enum.Result.html#method.map_err

[clearloop]

Didn't get it, do you mean it could panic in decode?

map_error is used to avoid no handling of Error?

Sorry for that, I forget what I was thinking about at that time, but map_err only can modify err but not stop panic, ref to rust doc:

fn stringify(x: u32) -> String { 
  format!("error code: {}", x) 
}

let x: Result<u32, u32> = Err(13);
assert_eq!(x.map_err(stringify), Err("error code: 13".to_string()));

It's still an Err after we map it, and ? will pass it to the caller function, so we should check if the caller function handles the Err well.

I used to bubble Errs by Result till they reach the user action and handle them together in one function in my past projects, but darwinia is different from them, the safety problem is focused on runtime call.

[aurexav]

No, we just need to raise it up. Substrate will handle this.

[hackfisher]

The key idea here is that runtime cannot panic! but error handling/mapping is acceptable, so be careful about functions which can cause panic, such as unwrap()

[hackfisher]

https://github.com/darwinia-network/darwinia/wiki/Best-Practice-for-Darwinia-Security-Review