-
Notifications
You must be signed in to change notification settings - Fork 28
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: revise to call Begin/EndRecheck even though mem.Size()
is 0
#162
Conversation
Would you explain why we should call |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
|
||
// Update metrics | ||
mem.metrics.Size.Set(float64(mem.Size())) | ||
|
||
return nil | ||
return err |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you can omit err
because you define variable in return type. so it can be just return
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm trying to avoid naked return
. cc: @wetcod
https://github.com/golang/go/wiki/CodeReviewComments#naked-returns
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Related with: https://github.com/line/link/issues/1151, #160
Description
W/ #160
CONTRACT
, we need to callBegin/EndRecheck
even thoughmem.Size()
is 0. IMHO, it's conceptually expected behavior to callBegin/EndRecheck
even thoughmem.Size()
is 0. We could compare it w/Begine/EndBlock
that are called even though there is nodeliverTx
.What I've got from the team
mem.config.Recheck
istrue
, callBegin/EndRecheck
.mem.config.Recheck
isfalse
butmem.config.Recheck
isfalse
.mem.config.Recheck
is configuredtrue
for all phase and all chain.false
and also it seems to be almost impossible.mem.config.Recheck
or not w/ other issue/PR.Need to discussHow about removingmem.config.Recheck
to restrictmisuse
it?cosmos-sdk
assumes thatrecheck
is working in terms of managing accountsequence
. ~~If we disablerecheck
, we might get problems related with accountsequence
.The PR could look like weird because it callsBegin/EndRecheck
even whenmem.config.Recheck
isfalse
.We actually need to call these even whenmem.config.Recheck
isfalse
.I'd like to hear the team's opinion.For contributor use:
docs/
) and code commentsFiles changed
in the Github PR explorer