-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
Updates to negative stake fix #3037
Updates to negative stake fix #3037
Conversation
Codecov Report
@@ Coverage Diff @@
## jae/removevalidator_negstake_fix #3037 +/- ##
====================================================================
- Coverage 52.18% 52.16% -0.03%
====================================================================
Files 140 140
Lines 9768 9768
====================================================================
- Hits 5097 5095 -2
- Misses 4330 4332 +2
Partials 341 341 |
defer proxyApp.Stop() | ||
defer func() { | ||
_ = proxyApp.Stop() | ||
}() |
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.
confused as to why this is necessary?
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.
Probably a linting error
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.
just double checked, it definitely was a linting error (errcheck).
should we not be checking this error still though?
defer func() {
err = proxyApp.Stop()
if err != nil {
panic(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.
I guess not actually because the error is only returned if the proxyApp is already stopped, which wouldn't be an issue in this case
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.
changes lgtm 👌
* Fix negative stake & invariance bug * Merge PR #3037: Updates to negative stake fix * Update invariant; fix lint * Fix linter * Add comment & TODO
cc @jaekwon
For Admin Use: