pokt-network / poktroll

The official Shannon upgrade implementation of the Pocket Network Protocol implemented using the Cosmos SDK
MIT License
15 stars 8 forks source link

[Code Health] Formalize "error variable convention" #348

Open bryanchriswhite opened 7 months ago

bryanchriswhite commented 7 months ago

Objective

Formalize and document an "error variable convention" that sufficiently removes ambiguity around:

Origin Document

We seem to have a few degrees of freedom with respect to our "error variable convention" and I'm concerned that we're not using or thinking about them consistently across the team (and/or perhaps even, individually, over time :sweat_smile:).

IIRC we initially carved out a convention to the effect of:

Error variables MUST:

  1. Begin with Err<owning module name> Where <owning module name> would match the module directory in which the error is defined and primarily used.
  2. Be defined in an x/<module name>/types/error.go file (according to 1.).

However it seems to me that the conventional intended usage scope of the error message itself has yet to be bounded, and in some cases (see the example below) has been made too specific for the error to be generally applicable. This leads to two possible outcomes (each time):

  1. The error is made more general and the specific aspects are pushed "down" into the message/format string that is passed to #Wrap()/#Wrapf(). This may also potentially require/involve renaming the error to match the more general level of specificity.
  2. A new error is added to the same module error set (likely with similar specificity) to apply in the new case.

I would argue that we should prefer 1. over 2. and that this should be formally captured in an "error variable convention" document that extends the existing considerations (above).

Additionally, the lack of a more complete convention leaves an open question around when it is and is not appropriate to use errors from a different module.

E.g. In a scenario where an application address is "invalid" in some supplier module business logic, should we use app.ErrAppInvalidAddress or supplier.ErrSupplierInvalidAddress](https://github.com/pokt-network/poktroll/blob/main/x/supplier/types/errors.go#L12)). The confusion comes from the fact that (currently) ErrAppInvalidAddress's message is more specific: "invalid application address", whereas ErrSupplierAddress is more general: "invalid address". In the later case, it seems like the expectation is that the specifics of which actor's address is invalid is to be included in the message passed to #Wrap().

Goals

Deliverables

Non-goals / Non-deliverables

General deliverables


Creator: @bryanchriswhite Co-Owners: ❓

Olshansk commented 7 months ago

@bryanchriswhite Updated the deliverables. PTAL