Refactor Error Handling - II (Modules)

[alexanderbez]

Part II PR of #5421 where all modules and tests are updated.

Not sure how to best recommend review on this one as it's...pretty large. Simulation and unit tests are passing.

---

For contributor use:

• Targeted PR against correct branch (see CONTRIBUTING.md)
• Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
• Code follows the module structure standards.
• Wrote unit and integration tests
• Updated relevant documentation (docs/) or specification (x/<module>/spec/)
• Added relevant godoc comments.
• Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
• Re-reviewed Files changed in the Github PR explorer

---

For admin use:

• Added appropriate labels to PR (ex. WIP, R4R, docs, etc)
• Reviewers assigned
• Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)

[lgtm-com]

This pull request fixes 7 alerts when merging 2bd6fb9 into 6bee805 - view on LGTM.com

fixed alerts:

• 7 for Impossible interface nil check

[lgtm-com]

This pull request fixes 7 alerts when merging 0ce6f62 into ff229ac - view on LGTM.com

fixed alerts:

• 7 for Impossible interface nil check

[fedekunze]

ACK. Minor nits that can be addressed in other PRs.

I also noticed that the error registration has a big gap between the error codes. For eg: x/distribution module errors that reserve codes 300-399, but only uses 300-311. Then comes x/evidence (sorted alphabetically) that reserves codes 400-499 (again, it uses only 400-403). Imo we shouldn't assign codes to the sdkerrors.Register function as it should somehow maintain a variable of the latest registered error code.

In the same context, what do we do with the error codes if we introduce another module in-between x/distribution and x/evidence? Should we assign it another range (say 800-899)? Or should we push the error codes so that it's assigned 400-499?

[fedekunze]

do we need to update legacy types too?

[alexanderbez]

I doubt it since the build passes?

[fedekunze]

nit, this should be ValidateBasic

[alexanderbez]

Agree -- outside the scope of the PR though.

[fedekunze]

ditto ValidateBasic

[alexanderbez]

Agree -- outside the scope of the PR though.

[lgtm-com]

This pull request fixes 7 alerts when merging ba73422 into ff229ac - view on LGTM.com

fixed alerts:

• 7 for Impossible interface nil check

[alexanderbez]

ACK. Minor nits that can be addressed in other PRs.

I also noticed that the error registration has a big gap between the error codes. For eg: x/distribution module errors that reserve codes 300-399, but only uses 300-311. Then comes x/evidence (sorted alphabetically) that reserves codes 400-499 (again, it uses only 400-403).

There are no gaps. Each module "reserves" a range of error codes, but by no means does it have to utilize every single code. e.g. x/evidence only has a handful of errors, but it reserves 400-499. It can add and register new errors in the future.

Imo we shouldn't assign codes to the sdkerrors.Register function as it should somehow maintain a variable of the latest registered error code.

I'd counter your suggestion by stating that abstracting error codes in the registration process defeats the entire purpose of errors codes (assuming this is what you're suggesting). The entire point is that if I see an error with code C and codespace S, I know exactly where to look -- module S and error with code C. If the Register function abstracts this, I no longer have the ability to know.

Keep in mind, this PR does not change existing behavior -- it just makes it more strict.

In the same context, what do we do with the error codes if we introduce another module in-between x/distribution and x/evidence? Should we assign it another range (say 800-899)? Or should we push the error codes so that it's assigned 400-499?

Ok, the distribution of error code ranges was not meant to be alphabetic -- I just did it that way to keep mental tabs. When you introduce a new module (regardless of where it fits alphabetically), you simply assign it an error code range that isn't used. It can be 800-899, 1000-1099, etc.

However, I do agree that In the future, when a boundless set of modules exist, we'll need to introduce the ability for these ranges to be modified, because we cannot expect developers to know ever module's range out there. e.g.

var StartingErrCode uint32 = 400

var (
  ErrEmptyValidatorAddr  = sdkerrors.Register(ModuleName, StartingErrCode, "empty validator address")
  ErrBadValidatorAddr    = sdkerrors.Register(ModuleName, StartingErrCode+1, "validator address is invalid")
  // ...
)

If I have my own module and it ends up conflicting with this, I simply update StartingErrCode. Maybe this isn't the ideal solution, but we can address this when the time comes.

[fedekunze]

There are no gaps. Each module "reserves" a range of error codes, but by no means does it have to utilize every single code. e.g. x/evidence only has a handful of errors, but it reserves 400-499

Is that enforced? For example, an app that imports x/evidence can actually use sdkerrors.Register to register the code 450 (i.e set that code as used) if it's not being used by the evidence module.

However, I do agree that In the future, when a boundless set of modules exist, we'll need to introduce the ability for these ranges to be modified, because we cannot expect developers to know ever module's range out there

++

[alexanderbez]

Actually, my entire argument is moot. Codespaces only have to be unique per codespace/code, this means all modules can use any codes they like -- they just have to be unique per "codespace" (i.e. module). I'll go ahead and update all codes to be 0+

[lgtm-com]

This pull request fixes 7 alerts when merging de33669 into ff229ac - view on LGTM.com

fixed alerts:

• 7 for Impossible interface nil check