kindelia / Kindelia

An efficient, secure cryptocomputer
https://kindelia.org/
609 stars 38 forks source link

refactor(error): improve error handling #252

Closed dan-da closed 1 year ago

dan-da commented 1 year ago

Addresses issue #247. (part a: more to come later)

I started small by just fixing 4 unwrap() calls in kindelia_core/src/net.rs.

Unfortunately one of these involved a trait, which complicated things a bit and required small changes in other files.

In kindelia main.rs I could have just mapped the errors from net.rs into a string and submitted a smaller PR, but instead I decided it would be cleaner to handle them as anyhow::Result.

I ended up changing most fn in main.rs and config.rs to return anyhow::Result instead of Result<_, String>. This required some calls to map_err(|e| anyhow!(e) to deal with String errors from format!() or from lower level fn calls. However, as the lower level calls get fixed, then the map_err goes away and we will just have fn_call()?. very clean.

note: the map_err() is necessary because anyhow::Result accepts anyhow::Error which is a boxed dynamic error type that accepts any error type derived from std::error::Error. However String is not so derived and thus must be mapped (or preferably return anyhow::Result or a thiserror type instead).

So this is very much a preliminary / intermediate PR to introduce anyhow and show the direction.

I understand there is a big refactor happening in another branch. I am ok with rebasing onto it when merged. Or this could be merged first, whichever works.


kindelia_core:

kindelia:

dan-da commented 1 year ago

Something seems wonky with CI. afaict, checks are only running when the PR is first created, and not for subsequent push(es). or at least it does not run for some subsequent pushes. is this intentional?

dan-da commented 1 year ago

ok, so I was getting some emails from github that CI was failing. Here's an example: https://github.com/dan-da/Kindelia/actions/runs/3562238811

It was some kind of cargo bench issue. I found a Cargo.toml workaround, and pushed changes for it to this branch. After which, I get another kind of failure: https://github.com/dan-da/Kindelia/actions/runs/3562393608

Run racs4/github-action-benchmark@v1
/usr/bin/git -c user.name=github-action-benchmark -c user.email=github@users.noreply.github.com -c http.https://github.com/.extraheader= fetch ***github.com/dan-da/Kindelia.git gh-pages:gh-pages
fatal: couldn't find remote ref gh-pages
Error: Command 'git' failed with args '-c user.name=github-action-benchmark -c user.email=github@users.noreply.github.com -c http.https://github.com/.extraheader= fetch ***github.com/dan-da/Kindelia.git gh-pages:gh-pages': fatal: couldn't find remote ref gh-pages
: Error: The process '/usr/bin/git' failed with exit code 128

maybe it wants me to have a gh-pages branch for some reason?

racs4 commented 1 year ago

Something seems wonky with CI

I think this is solved in ab32905, but you can talk if you have any problems.

then the map_err goes away and we will just have fn_call()?. very clean

This seems very good.

Or this could be merged first

I think this is better, since this is smaller.

dan-da commented 1 year ago

I just force pushed with these changes:

  1. removed Cargo.toml bench sections to disable benches
  2. added a comment to Node::new() as requested