-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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
R4R: BaseApp Peer Review #3526
R4R: BaseApp Peer Review #3526
Conversation
Should we open issues here? |
I think we should address them in this PR. |
Codecov Report
@@ Coverage Diff @@
## develop #3526 +/- ##
===========================================
+ Coverage 59.63% 59.64% +<.01%
===========================================
Files 131 131
Lines 9682 9691 +9
===========================================
+ Hits 5774 5780 +6
- Misses 3568 3571 +3
Partials 340 340 |
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.
Agree with @jackzampolin; otherwise LGTM.
baseapp/baseapp.go
Outdated
// NOTE: this changed in tendermint and we didn't notice... | ||
return app.FilterPeerByPubKey(path[3]) | ||
cmd, typ, arg := path[1], path[2], path[3] | ||
if cmd == "filter" { |
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.
switch cmd
instead?
@@ -805,12 +804,13 @@ func (app *BaseApp) runTx(mode runTxMode, txBytes []byte, tx sdk.Tx) (result sdk | |||
ctx = newCtx.WithMultiStore(ms) | |||
} | |||
|
|||
gasWanted = result.GasWanted |
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.
Reason for this change? Seems like this ordering might be significant...
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.
One small question, but otherwise LGTM 👍
cc @alexanderbez
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.
Wrote tests
Updated relevant documentation (
docs/
)Added entries in
PENDING.md
with issue #rereviewed
Files changed
in the github PR explorerFor Admin Use: