KOL-5 | feat: Enhance error handling with detailed KolmeError variants

[anakinzhed]

No description provided.

[cloudflare-workers-and-pages]

Deploying kolme with    Cloudflare Pages

Latest commit:	aab096d
Status:	✅  Deploy successful!
Preview URL:	https://e2411421.kolme.pages.dev
Branch Preview URL:	https://kol-5.kolme.pages.dev

View logs

[snoyberg]

Left some comments for a change in general direction. When this is ready for another review, let's get @MichaelNelo on this too so I'm not a bottleneck.

[anakinzhed]

@borsboom, Michael asked me to work with you in this PR.

[borsboom]

@anakinzhed I'm still familiarizing myself with the Kolme codebase, and this is a big PR. I'm working to understand the changes and figure out if there's a way we can avoid the stringy errors that are hard to avoid because of the traits that KolmeError derives. It might take me a little while to respond further, but I am working on it.

[borsboom]

Not easy to retrofit proper errors into a codebase like this, especially given the need for some errors to be serializable.

Aside from the comments I added to specific changes, I noticed there are still many uses of anyhow::Result which end up being "hidden" by error types implementing From<anyhow::Error> and stringifying them. Removing those, and any imports of anyhow::Result should help find them (as well as just searching the codebase anyhow).

[anakinzhed]

@borsboom Sorry for the delay. I believe I’ve addressed all of your comments. Please take a look.

[borsboom]

Great progress!

I notice there are still some places that are returning anyhow::Result (e.g. in packages/kolme/src/core/execute.rs). Where concrete error types are returned, they are being converted to anyhow::Error (e.g. return Err(KolmeExecuteError::…).into();).

Better would be for the functions to return a concrete error (e.g. fn …(…) -> Result<(), KolmeExecuteError>). Any upstream errors should be embedded in KolmeExecuteError, which is made easier using thiserror's #[from] annotation so that the ? operator still works. E.g.

pub enum KolmeExecuteError {
    …

    #[error("Public key error: {0}")]
    PublicKeyError(#[from] shared::cryptography::PublicKeyError),
}

If at all possible, anyhow should be removed from the dependencies entirely.

[anakinzhed]

@borsboom I made several changes. Please take a look and let me know your thoughts.

[borsboom]

This is getting really close. Most of these comments are nitpicks which you can disregard if you don't think they're worth addressing.

My biggest question is about TransactionError::StoreError and ::CoreError and why they take Strings, because if feels like that may be due to some kind of structural issue. If you can explain the reason, I might be able to help find another approach.

I pushed a commit that removes the anyhow dependency from kolme and kolme-store's Cargo.tomls, and removed a few remaining places that referenced it (one of which was in code I added recently, which is why I figured it made sense for me to take care of it).

[borsboom]

Why are StoreError and CoreError both strings instead of concrete types? Is there a way to avoid this? I'd like to understand so maybe we can find another approach.

[anakinzhed]

The problem here is that TransactionError must be Clone, serde::Serialize, and serde::Deserialize in order to be sent, but KolmeCoreError and KolmeStoreError do not derive Clone, serde::Serialize, or serde::Deserialize.

[anakinzhed]

@borsboom This is the only remaining open point. The reason is explained in the message above. I don’t currently see another way to handle this, so please if you have a better approach in mind, please let me know. Thanks!