BaseApp cleanup #1663

AdityaSripal · 2018-07-13T01:59:15Z

Addresses #1592

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)
Added appropriate labels to PR (ex. wip, ready-for-review, docs)

codecov · 2018-07-13T02:22:25Z

Codecov Report

Merging #1663 into develop will decrease coverage by <.01%.
The diff coverage is 71.05%.

@@             Coverage Diff             @@
##           develop    #1663      +/-   ##
===========================================
- Coverage    62.77%   62.77%   -0.01%     
===========================================
  Files          122      122              
  Lines         7125     7133       +8     
===========================================
+ Hits          4473     4478       +5     
- Misses        2390     2394       +4     
+ Partials       262      261       -1

alexanderbez · 2018-07-13T18:33:43Z

@AdityaSripal what's the plan for moving towards the breaking changes? Post Gaia 7000 launch?

AdityaSripal · 2018-07-13T18:36:10Z

@alexanderbez Definitely think it should be post Gaia7000. Probably want to get the other stability stuff done first and then make the breaking changes.

AdityaSripal · 2018-07-13T18:38:12Z

cmd/gaia/app/app.go

@@ -60,7 +60,7 @@ type GaiaApp struct {
 func NewGaiaApp(logger log.Logger, db dbm.DB, traceStore io.Writer, baseAppOptions ...func(*bam.BaseApp)) *GaiaApp {
 	cdc := MakeCodec()

-	bApp := bam.NewBaseApp(appName, cdc, logger, db, baseAppOptions...)
+	bApp := bam.NewBaseAppNoCodec(appName, logger, db, auth.DefaultTxDecoder(cdc), baseAppOptions...)


We may want to revert this back to using deprecated NewBaseApp. Otherwise QueryResult will be encoded with json rather than amino. May be a breaking change even though tests pass.

AdityaSripal · 2018-07-13T18:39:06Z

x/auth/ante.go

@@ -17,32 +17,52 @@ const (
 // NewAnteHandler returns an AnteHandler that checks
 // and increments sequence numbers, checks signatures & account numbers,
 // and deducts fees from the first signer.
+// nolint: gocyclo


Current cyclomatic complexity of 12, greater than max 11

AdityaSripal · 2018-07-13T18:42:49Z

docs/core/examples/app1.go

@@ -18,17 +17,12 @@ const (

 func NewApp1(logger log.Logger, db dbm.DB) *bapp.BaseApp {


Not sure if we want to update docs now or after making the breaking changes.

AdityaSripal · 2018-07-13T18:46:22Z

Addresses #1592

…itya/cleanup

cwgoes · 2018-07-14T00:14:08Z

Is this R4R? Looks like there are file conflicts.

…itya/cleanup

AdityaSripal · 2018-07-14T00:59:46Z

@cwgoes fixed

rigelrozanski · 2018-07-18T03:26:24Z

@AdityaSripal could you link to an issue (if there is one) as well as describe the high level changes being made here - as I recall this is based out of some conversations that we want to be passing a txDecoder to bassapp instead of a ctx -> This type of information should be added to the PR description (aka the header comment of the PR), it's good context for reviewers

edit: saw your later comment "Addresses #1592" this should be editted into the header comment probably

cwgoes · 2018-07-18T22:59:46Z

baseapp/baseapp.go

+// Create and name new BaseApp
+// Does not set cdc and instead takes a user-defined txDecoder.
+// TODO: Rename to NewBaseApp and remove above constructor once auth, wire dependencies removed
+func NewBaseAppNoCodec(name string, logger log.Logger, db dbm.DB, txDecoder sdk.TxDecoder, options ...func(*BaseApp)) *BaseApp {


I don't think deprecating this API is worth it; adds cruft we have to clean up later. When we promise a stable baseapp API, we can do this, but until then I think we should just refactor all calls.

Should we punt this to next iteration then or... ?

No, let's go ahead and merge.

…ditya/cleanup

cwgoes · 2018-07-19T01:23:31Z

baseapp/baseapp.go

-		value := app.cdc.MustMarshalBinary(result)
+
+		// Encode with json
+		value, err := json.Marshal(result)


Hmm - won't this marshal e.g. numbers different than Amino JSON? Maybe we should use an empty Amino codec (just for the custom JSON) to avoid returning different JSON formats in different places?

cwgoes · 2018-07-19T01:23:52Z

baseapp/baseapp.go

 // Accepts variable number of option functions, which act on the BaseApp to set configuration choices
-func NewBaseApp(name string, cdc *wire.Codec, logger log.Logger, db dbm.DB, options ...func(*BaseApp)) *BaseApp {
+// TODO: Rename to NewBaseApp and remove above constructor once auth, wire dependencies removed


Comment can be removed?

cwgoes · 2018-07-19T01:24:25Z

Gopkg.lock

@@ -2,76 +2,57 @@


 [[projects]]


Try running make update_deps, looks like you're on an old dep version

cwgoes

utACK

rigelrozanski

Looks good

AdityaSripal added 2 commits July 12, 2018 18:32

Refactor BaseApp without breaking change

fa31165

merge

98c1951

AdityaSripal requested review from cwgoes, ebuchman, jbibla, rigelrozanski and zramsay as code owners July 13, 2018 01:59

AdityaSripal added 3 commits July 12, 2018 19:06

minor cleanup

ade7186

fmt

4751727

remove false promise

cb9c7fe

AdityaSripal added the wip label Jul 13, 2018

AdityaSripal added 3 commits July 12, 2018 19:38

Added recover to standard antehandler

f6d5c85

Test recover in ante

aa3a64d

ignore linter

ddd4ded

AdityaSripal commented Jul 13, 2018

View reviewed changes

AdityaSripal changed the title ~~WIP: BaseApp cleanup non-breaking~~ BaseApp cleanup non-breaking Jul 13, 2018

AdityaSripal added API and removed wip labels Jul 13, 2018

AdityaSripal commented Jul 13, 2018

View reviewed changes

Merge branch 'master' of https://github.com/cosmos/cosmos-sdk into ad…

cd23f8b

…itya/cleanup

Merge branch 'master' of https://github.com/cosmos/cosmos-sdk into ad…

5b98a3d

…itya/cleanup

ebuchman changed the base branch from master to develop July 16, 2018 15:36

revert changes to gaia to ensure non-breaking

198aef2

cwgoes reviewed Jul 18, 2018

View reviewed changes

AdityaSripal added 7 commits July 18, 2018 16:24

make breaking changes

81f3e79

update documentation

ca5e159

fmt

46b8766

fix baseapp docs

8cdbc4b

Merge branch 'develop' of https://github.com/cosmos/cosmos-sdk into a…

218c9b3

…ditya/cleanup

add to pending changelog

971cfc6

update pending

20ce656

AdityaSripal changed the title ~~BaseApp cleanup non-breaking~~ BaseApp cleanup breaking Jul 19, 2018

AdityaSripal changed the title ~~BaseApp cleanup breaking~~ BaseApp cleanup Jul 19, 2018

cwgoes reviewed Jul 19, 2018

View reviewed changes

Gopkg.lock Outdated

@@ -2,76 +2,57 @@

[[projects]]

Copy link

Contributor

cwgoes Jul 19, 2018

Choose a reason for hiding this comment

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

Try running make update_deps, looks like you're on an old dep version

AdityaSripal and others added 6 commits July 18, 2018 18:44

revert change to gopkg.lock

7c41e8c

revert change to queryApp encoding

5015c46

remove old comment

1e87b5e

merge

48885b4

Merge branch 'develop' into aditya/cleanup

28cbca7

Remove duplicate changelog entry

6198c20

cwgoes approved these changes Jul 21, 2018

View reviewed changes

rigelrozanski approved these changes Jul 23, 2018

View reviewed changes

rigelrozanski merged commit fae728f into develop Jul 23, 2018

rigelrozanski deleted the aditya/cleanup branch July 23, 2018 18:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BaseApp cleanup #1663

BaseApp cleanup #1663

AdityaSripal commented Jul 13, 2018 •

edited

Loading

codecov bot commented Jul 13, 2018 •

edited

Loading

alexanderbez commented Jul 13, 2018

AdityaSripal commented Jul 13, 2018

AdityaSripal Jul 13, 2018

AdityaSripal Jul 13, 2018

AdityaSripal Jul 13, 2018

AdityaSripal commented Jul 13, 2018

cwgoes commented Jul 14, 2018

AdityaSripal commented Jul 14, 2018

rigelrozanski commented Jul 18, 2018 •

edited

Loading

cwgoes Jul 18, 2018

AdityaSripal Jul 18, 2018

cwgoes Jul 18, 2018

cwgoes Jul 19, 2018

cwgoes Jul 19, 2018

cwgoes Jul 19, 2018

cwgoes left a comment

rigelrozanski left a comment

		@@ -18,17 +17,12 @@ const (

		func NewApp1(logger log.Logger, db dbm.DB) *bapp.BaseApp {

BaseApp cleanup #1663

BaseApp cleanup #1663

Conversation

AdityaSripal commented Jul 13, 2018 • edited Loading

codecov bot commented Jul 13, 2018 • edited Loading

Codecov Report

alexanderbez commented Jul 13, 2018

AdityaSripal commented Jul 13, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AdityaSripal commented Jul 13, 2018

cwgoes commented Jul 14, 2018

AdityaSripal commented Jul 14, 2018

rigelrozanski commented Jul 18, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cwgoes left a comment

Choose a reason for hiding this comment

rigelrozanski left a comment

Choose a reason for hiding this comment

AdityaSripal commented Jul 13, 2018 •

edited

Loading

codecov bot commented Jul 13, 2018 •

edited

Loading

rigelrozanski commented Jul 18, 2018 •

edited

Loading