Skip to content
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

Cleanup Baseapp #1400

Merged
merged 1 commit into from
Jul 10, 2018
Merged

Cleanup Baseapp #1400

merged 1 commit into from
Jul 10, 2018

Conversation

mossid
Copy link
Contributor

@mossid mossid commented Jun 27, 2018

  • Updated all relevant documentation in docs
  • Updated all code comments where relevant
  • Wrote tests
  • Updated CHANGELOG.md
  • Updated Gaia/Examples
  • Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)

Closes: #1329

@mossid mossid requested a review from ebuchman as a code owner June 27, 2018 01:55
@mossid mossid added the wip label Jun 27, 2018
@mossid mossid force-pushed the joon/1329-cleanup-baseapp branch from d5e3d54 to 2101459 Compare June 27, 2018 01:55
@mossid mossid force-pushed the joon/1329-cleanup-baseapp branch from 2101459 to 7c23cef Compare June 27, 2018 01:56
mossid added a commit that referenced this pull request Jun 27, 2018
@codecov
Copy link

codecov bot commented Jun 27, 2018

Codecov Report

Merging #1400 into develop will increase coverage by 0.01%.
The diff coverage is 90.9%.

@@             Coverage Diff             @@
##           develop    #1400      +/-   ##
===========================================
+ Coverage    63.66%   63.67%   +0.01%     
===========================================
  Files          122      122              
  Lines         6764     6766       +2     
===========================================
+ Hits          4306     4308       +2     
  Misses        2213     2213              
  Partials       245      245

@mossid
Copy link
Contributor Author

mossid commented Jun 29, 2018

Dependent on tendermint/tendermint#1803

@mossid mossid added the S:blocked Status: Blocked label Jun 29, 2018
@cwgoes cwgoes changed the base branch from develop to unstable July 3, 2018 21:12
@cwgoes cwgoes changed the base branch from unstable to develop July 4, 2018 19:26
Copy link
Member

@ebuchman ebuchman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice! Except possibly the context passed to the ante handler

@@ -505,17 +447,17 @@ func (app *BaseApp) runTx(mode runTxMode, txBytes []byte, tx sdk.Tx) (result sdk

// Run the ante handler.
if app.anteHandler != nil {
newCtx, result, abort := app.anteHandler(ctx, tx)
var newCtx sdk.Context
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should the ante handler really get an empty context? why not the ctx we already populated above?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tried to modify it, and the test failed :( I don't know why does it affect to the execution but we cannot change it.

@mossid mossid force-pushed the joon/1329-cleanup-baseapp branch from 0264619 to fca7277 Compare July 5, 2018 22:51
@mossid mossid force-pushed the joon/1329-cleanup-baseapp branch from fca7277 to ea9ca6f Compare July 5, 2018 23:45
mossid added a commit that referenced this pull request Jul 5, 2018
fix lint

apply requests

revert removing newCtx
@mossid mossid added ready-for-review and removed S:blocked Status: Blocked wip labels Jul 9, 2018
@mossid
Copy link
Contributor Author

mossid commented Jul 9, 2018

Decided to skip codespace removing

@mossid mossid force-pushed the joon/1329-cleanup-baseapp branch from a1e6bb8 to 31b9007 Compare July 9, 2018 22:59
mossid added a commit that referenced this pull request Jul 9, 2018
fix lint

apply requests

revert removing newCtx

refactor

fix errors
@mossid mossid changed the title WIP: Cleanup Baseapp Cleanup Baseapp Jul 9, 2018
@ebuchman
Copy link
Member

I'll fix and merge this thanks jooN!

@ebuchman ebuchman force-pushed the joon/1329-cleanup-baseapp branch from 31b9007 to 4761612 Compare July 10, 2018 03:50
fix lint

apply requests

revert removing newCtx

refactor

fix errors
@ebuchman ebuchman merged commit 9531466 into develop Jul 10, 2018
@ebuchman ebuchman deleted the joon/1329-cleanup-baseapp branch July 10, 2018 04:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants