Remove all usages of .unwrap() in non-test code

kevinlewi commented 1 year ago

_Originally posted by @kevinlewi in https://github.com/novifinancial/akd/pull/292#discussion_r1052487581_

slawlor commented 1 year ago

Ah we actually want this! Returning a "Result" from test functions does not give you the line or stack of what failed in the test. We really want the unwrap() or ideally expect() statements to show us exactly where a test failed. Ideally we shouldn't have any tests which actually return a Result<_,_> type and unwrap everything that should not fail.

kevinlewi commented 1 year ago

@slawlor: To double-check, this issue is for removing usages of unwrap in non-test (production) code, because they could cause un-catchable panics that abort the server. Having unwraps in test code sounds fine by me!

slawlor commented 1 year ago

@slawlor: To double-check, this issue is for removing usages of unwrap in non-test (production) code, because they could cause un-catchable panics that abort the server. Having unwraps in test code sounds fine by me!

Ah wow.. My bad, I misread that!

slawlor commented 1 year ago

A precursory search found a few

a. (1) in akd/src/append_only_zks.rs b. (2) in akd/src/ecvrf_impl.rs c. (4) in akd_core/src/types/node_label/mod.rs d. (2) in akd_mysql/src/mysql_storables.rs e. (3) in akd_mysql/src/mysql.rs

The rest I think are in tests from the looks of it. Looking at the usages a few are kind of logical, so i'm not sure we want to get rid of all of them. For example

let bin = St::get_full_binary_key_id(key);
// Since these are constructed from a safe key, they should never fail
// so we'll leave the unwrap to simplify
let back: NodeKey =
        TreeNodeWithPreviousValue::key_from_full_binary(&bin).unwrap();

so like unless we screwed up our conversions, it shouldn't be possible... which if we screwed them up we probably want to panic here :p. I'll put up a PR cleaning up some of them however.

facebook / akd

Remove all usages of .unwrap() in non-test code #294