[#24] Better error handling

[DK318]

Description

Problem

In Backend.Vault.Kv error handling is very bad. We lose a lot of info about thrown exceptions.

Solution

Added more CofferError constructors (e.g. RequestFailed and DecodeFailed) which stores all necessary information and wrote Buildable instance for it.

Related issue(s)

• Fixes #24

  Related changes (conditional)

  • Tests

    • [ ] If I added new functionality, I added tests covering it.
    • [ ] If I fixed a bug, I added a regression test to prevent the bug from silently reappearing again.

  • Documentation

    • [x] I checked whether I should update the docs and did so if necessary:
    • README
    • Haddock

  • Public contracts

    • [x] Any modifications of public contracts comply with the Evolution of Public Contracts policy.
    • [x] I added an entry to the changelog if my changes are visible to the users and
    • [x] provided a migration guide for breaking changes if possible

  Stylistic guide (mandatory)

  • [x] My commits comply with the policy used in Serokell.
  • [x] My code complies with the style guide.

[MagicRB]

The issue with a concrete error type is that a Vault backend requires different errors than a pass backend which is different to a bitwarden backend or keepassxc backend. We can't possibly have just one error type for all of them. IMO the same way Backend is polymorphic over a, BackendError a should somehow also be polymorphic, i've yet to figure out why, thats why its in sucha sorry state right now. But the PR itself LGTM

[dcastro]

BackendError a should somehow also be polymorphic

@MagicRB does it need to be?

Backend is polymorphic because we need to abstract over it. But I don't see the need to abstract over the type of exception, we just need to bubble it up to main and then print it.

[MagicRB]

BackendError a should somehow also be polymorphic

@MagicRB does it need to be?

Backend is polymorphic because we need to abstract over it. But I don't see the need to abstract over the type of exception, we just need to bubble it up to main and then print it.

Because the frontend should be decoupled from the backend, that's the point of this architecture, if we add errors for all the backend we'll end up with a tight coupling between the two parts and a pattern matching mess somewhere.

[MagicRB]

The lowest common denominators for errors are showing them in some prettified sense and serializing into some unified data structure for human consumption. I also figured out another reason why this is a bad idea, as it stands now, the coffer client and server can have different backends available when running in #56 but if we add these backend specific errors we'll loose this decoupledness.

[dcastro]

Because the frontend should be decoupled from the backend, that's the point of this architecture, if we add errors for all the backend we'll end up with a tight coupling between the two parts and a pattern matching mess somewhere

Yeah I get that, what I'm saying is that CofferError doesn't need to be polymorphic (i.e. CofferError a) (unless I misunderstood your suggestion).

We could have a constructor that takes any printable error (basically, what you just said "The lowest common denominators for errors are showing them in some prettified sense and serializing into some unified data structure for human consumption")

data CofferError where
  ...
  BackendError :: Buildable err => err -> CofferError

Then you can have multiple backend-specific error types (VaultError, PassError, etc) and wrap them all in BackendError.

[MagicRB]

data CofferError where
  ...
  BackendError :: Buildable err => err -> CofferError

Then you can have multiple backend-specific error types (VaultError, PassError, etc) and wrap them all in BackendError. Sounds reasonable

[dcastro]

@MagicRB : I've created an issue for this: https://github.com/serokell/coffer/issues/72