refactor: improve Error handling (clean up `unwrap`s)

[plebhash]

as @jakubtrnka pointed out in https://github.com/stratum-mining/stratum/issues/723#issuecomment-1904233521, we should improve error handling across SRI codebase

while I do believe and understand that there are responsible ways to use unwrap, as the codebase grows and becomes more complex for newcomers to understand, these unwraps can quickly become footguns with long-term consequences on the maintenability and overall code quality of the project

[Fi3]

I think that this is too generic can you be more specific

[Fi3]

in particular we have to distinguish between low-level library and application. Now considering that we are going to have mid level library we have 3 "zone"

• low level libraries
• mid level library
• application (mostly used for testing and as reference implementation)

[Fi3]

For the libraries every unwrap should be documented if it is not we have to open issues for it. For the application we have handle_result that is underused cause is a little bit complex to use. Not sure if we want to improve it (considering that will be used only by application zone), improve it and put it in the mid level library zone, or just leave at it is now.

[Fi3]

whatever we decide we should add a code reviews police doc or something like that. For example we can say that every result in the a application zone that is unhandled must be put in a handle_result macro and every exception have to be documented. Or we can decide that we do not care an unwrap in application zone are acceptable cause they are not (meant to be) used in production ecc ecc

[plebhash]

For the libraries every unwrap should be documented if it is not we have to open issues for it.

this is a good start, but at some point I believe that some kind of refactoring introducing some enum-based error handling could be an interesting direction

it will not be a trivial/small effort, however

For the application we have handle_result that is underused cause is a little bit complex to use.

usually I tend to avoid macros unless absolutely necessary

in my past job, I worked for around ~2y on the go-to-market vertical of @paritytech, where our main product was polkadot-sdk (fka substrate), a Rust framework for blockchains which makes heavy usage of macros.

our overall consensus was that our heavy usage of macros on the codebase was slowing down developer adoption because macros are always harder to understand.

while returning std::result::Result on the majority of functions does make the code bigger and uglier, it is way easier to understand (and more importantly, to contribute to)

I think that this is too generic can you be more specific

apologies for the lack of clarity here, I will take some time to elaborate more concrete examples across the codebase along with suggestions on what to do about them

I will write more comments on this issue in the future

---

overall, my personal opinion is just that excessive usage of unwraps is like the code author telling a code reviewer/auditor: "hey, trust me, I know what I'm doing here", while it's usually better to have some match arms saying: "here's proof I know what I'm doing here because I'm explicitly handling every possible case"

[plebhash]

just found this Discussion started by @xraid , which I think is going in the same direction of this issue so I'm linking here

https://github.com/stratum-mining/stratum/issues/428

[plebhash]

also related: https://github.com/stratum-mining/stratum/issues/773

[plebhash]

one idea here is to start using the anyhow crate on the roles implementations

it makes error handling smooth, and we can basically return a anyhow::Result on every function and use ? instead of unwrap