Correct error handling in EndBlocker#839
Conversation
spooktheducks
force-pushed
the
spook/cleanup-logging
branch
from
3ac264a to
142e49d
Compare
amimart
left a comment
amimart
left a comment
There was a problem hiding this comment.
This seems like a good standard to adopt
However I think through this initiative we also need to carefully hunt down all the false-errors that are expected situations and non state breaking, as returning an error in an ABCI method will halt the chain.
Also as a side not, I think we should have less error types as most of them denotes the same type but in different context
| logger := sdkCtx.Logger().With( | ||
| "abci_step", "EndBlock", | ||
| "height", sdkCtx.BlockHeight(), | ||
| ) | ||
| sdkCtx = sdkCtx.WithLogger(logger) |
There was a problem hiding this comment.
This is great, I think we should also systematically set the module logger field to allow proper filtering through configuration as it is done here
There was a problem hiding this comment.
We actually do it in mint module :D
and in the tests, although here we use "submodules"
| return types.ErrNoQualifiedInferers | ||
| return errors.Wrap(err, "failed: fetch active inferrers for topic") | ||
| } else if len(activeInfererAddresses) == 0 { | ||
| return errors.Wrap(types.ErrNoQualifiedInferers, "no qualified inferrers") |
There was a problem hiding this comment.
I think this should not be an error as it seems to be a situation expected to happen, by returning an error for a normal behaviour we'll halt the chain
| if err != nil { | ||
| sdkCtx.Logger().Info("Error closing worker nonce", "error", err) | ||
| } | ||
| return errors.WrapWithFields(err, "failed: close worker nonce", "topic_id", topicId) |
There was a problem hiding this comment.
If we want to return the error here, on any case of any error, then we need to carefully review all of the errors we currently have, and either classify them as unexpected (then halt) or recoverable, or just do not return and propagate such recoverable errors on those functions if they are expected to be handled.
This change would need careful review of our error handling within all Begin/EndBlocker code, so it'd be better done in a separate effort than the current fixes on BeginBlocker/EndBlocker.
There was a problem hiding this comment.
And I think we would need to do that, for safety; because we are treating equally (= ignoring) both a business-as-usual error like "no inferers in this epoch" or a critical one like "no access to store".
|
|
||
| // EndBlock returns the end blocker for the emissions module. | ||
| func (am AppModule) EndBlock(ctx context.Context) error { | ||
| func (am AppModule) EndBlock(ctx context.Context) (err error) { |
There was a problem hiding this comment.
Good one, I'll move this pattern to our current Begin/EndBlocker.
spooktheducks
force-pushed
the
spook/cleanup-logging
branch
from
142e49d to
7ba7ff9
Compare
3 participants
Purpose of Changes and their Description
Link(s) to Ticket(s) or Issue(s) resolved by this PR
Are these changes tested and documented?
Unreleasedsection ofCHANGELOG.md?Still Left Todo
Fill this out if this is a Draft PR so others can help.