Conversation
| // Validate performs basic genesis state validation returning an error upon any | ||
| // failure. | ||
| func (gs GenesisState) Validate() error { | ||
| if gs.LastLockId < 0 { |
There was a problem hiding this comment.
This is a uint so it cannot be less than 0
x/epochs/handler.go
Outdated
| // NewHandler returns a handler for epochs module messages. | ||
| func NewHandler(k keeper.Keeper) sdk.Handler { | ||
| return func(ctx sdk.Context, msg sdk.Msg) (*sdk.Result, error) { | ||
| ctx = ctx.WithEventManager(sdk.NewEventManager()) |
There was a problem hiding this comment.
IMO we shouldn't even have these handler.go files -- it's all legacy code.
There was a problem hiding this comment.
I'm guessing this: cosmos/cosmos-sdk#9650 would be the right way to do it?
cmd/osmosisd/cmd/forceprune.go
Outdated
| // N.B: We duplicate the call to db.Close() on top of | ||
| // the call in defer statement above to make sure that the resources | ||
| // are properly released and any potential error from Close() | ||
| // is handled. Close() should be idempotent so this is acceptable. | ||
| if err := db.Close(); err != nil { | ||
| return err | ||
| } |
There was a problem hiding this comment.
I don't get why are we doing this? The defer handles this more cleanly?
There was a problem hiding this comment.
If we do defer db.Close(), it does not do error handling.
The new change validates the error while keeping defer db.Close() as well. The code in defer runs even if the code panics, making sure that we always close the resources. Calling this twice should not be a problem because Close() is idempotent
There was a problem hiding this comment.
Can we instead do a named return error, (e.g. func compactBlockStore(dbPath string) (rerr error), and then do defer rerr = db.Close(). This is the general pattern for handling errors in a panic.
Encouraging db closing being isolated scope / not handled multiple times is ideal imo.
There was a problem hiding this comment.
Sweet, yeah followed your advice
ValarDragon
left a comment
ValarDragon
left a comment
There was a problem hiding this comment.
LGTM after we resolve the db.Close
|
Merging since db.Close addressed + 2 approvals |
3 participants
Closes: #XXX
What is the purpose of the change
Follow-up to: #1897
Please note that this is a
staticcheckset of rules (a subset of rules fromstaticcheckbinaryBrief Changelog
defer Close()GenerateSaveCoinKeyuintsTesting and Verifying
This change is a trivial rework / code cleanup without any test coverage.
Documentation and Release Note
Unreleasedsection inCHANGELOG.md? no