Skip to content

Suggestions to refactor KDF errors

Jump to bottom
dimxy edited this page Aug 30, 2024 · 23 revisions

(inspired by meteroid project)

What's not perfect with KDF errors?

Today in the KDF codebase we have too many errors, which apparently are created without a single strategy.
Many error enums contain same duplicate variants (example CoinFindError).
Number of error enums itself is too big. In many cases we do not process error enum variants in code (in match) so we basically do not need them, but only convert them to upper error enums. That is, we have a lot of code just to convert errors, like this:

impl From<PaymentStatusErr> for ValidatePaymentError {
    fn from(err: PaymentStatusErr) -> Self {
        match err {
            PaymentStatusErr::Transport(e) => Self::Transport(e),
            PaymentStatusErr::ABIError(e) | PaymentStatusErr::Internal(e) => Self::InternalError(e),
            PaymentStatusErr::InvalidData(e) => Self::InvalidData(e),
        }
    }
}

For this case I think PaymentStatusErr was added when the status_and_htlc_params_from_tx_data() function was created.
But do we really need to create a new error when some new function or struct with methods is added? This leads to excess number of errors and duplications and a lot of conversion code.

Instead of a one-time-use specific 'PaymentStatusErr' type we could have a more general error like 'SwapDataError', for use in many more cases (functions and methods). Also, instead of mapping each PaymentStatusErr variant to ValidatePaymentError variants we could use new SwapDataError as one variant in the upper error, like this:

enum ValidatePaymentError {
    #[from_stringify("SwapDataError")]
    SwapDataError(String),
}

How to refactor errors

So the idea is to have less quantity of errors and make them more general and reusable.
Basically, let us define an hierarchy with general errors and try to make it more or less stable. When we need an error in new code try to use an existing error, extend it if needed, instead of creating too many new error types.
Here is a possible hierarchy of general errors: KDF Errors So we basically define one general error for each logical group of code. There are 3 layers of errors: API, domain and infrastructure:

  • API layer (named RpcErrors or ApiErrors) is errors returned to the user by our rpc. I think we need a similar json representation for all of them. We already add HTTP code serialised. We also may serialise other fields identically, for e.g.: { "status": .., "title": .., "detail": .., "source": ..}. When a new RPC is added I suggest not to create a new API error but try to find a group of similar RPCs (like on the picture) and use the existing error.
  • Domain errors are where business logic is processed. It's not easy to define this structure once and for all and should be done iterations. I only think we should to limit the number of such errors (or it would be hard to remember and use them, and also make them less reusable in API errors).
  • Infrastructure errors contain error or statuses from downstream services (DB, electrum, web3 etc).

Again, I suggest for any domain error try not to map each its variants into an upper API error variants, but simply make it a variant in API error enum (we can use macro to generate 'from' code) - then we will get all possible lower error variants.
Same for infrastructure and domain errors.
Ofc, any API domain or infrastructure errors could be extended with simple variants whenever needed, like InvalidParam:

enum ValidatePaymentError {
    #[from_stringify("SwapDataError")]
    SwapDataError(String),
    InvalidParam(String),
}

Consider this:
If we share same API error within same group of API methods (RPCs) we will not have duplications like CoinNotSupported than if we have many error enums (for this group of RPCs).
And even better, if instead of adding CoinNotSupported and CoinIsNotActive variants in each API Error we add one variant as a whole domain error like CoinConfigError (having both those variants) we will avoid such duplication in all API errors.

More suggestions

Follow naming conventions:

  • Now we have both '..Error' and '..Err' ending (like 'WithdrawError' and 'PaymentStatusErr') - let's always use '..Error'.
  • Let's always use 'RpcError' (or 'ApiError') in the names of top-level errors returned by our rpc functions. (I think ApiError may be better because we have lower level rpc error names when interacting with electrum, but the other option would work well too if used consistently).

Try to use less entities or defaults:

  • Let's try not to use 'InternalError' variant as, I guess, this is used when we could not pick a better name (as a default). I believe if we have consistent stable error structure, we won't need this.
  • Maybe we sometimes could reduce extra errors like PlatformCoinIsNotActivated and CoinIsNotActive to only CoinIsNotActive.

An idea from shamardy:

  • We can use the error hierarchy above as starting point for refactoring our module/crate structure. In this case each error on the diagram will be a single pub error for each module. That means each module may create more private error objects for internal use (but I guess we still should prevent error quantity to grow in uncontrollable way).
Clone this wiki locally