simapp: rename MakeEncodingConfig to MakeTestEncodingConfig #7681

robert-zaremba · 2020-10-26T17:43:08Z

Description

Temporal solution for #7642

Refactoring MakeEncodingConfig is a bigger work. For the Stargate release we want to make it clear to the users that this method is rather internal and deprecated to use in their apps.

Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

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.
Code follows the module structure standards.
Wrote unit and integration tests
Updated relevant documentation (docs/) or specification (x/<module>/spec/)
Added relevant godoc comments.
Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
Re-reviewed Files changed in the Github PR explorer
Review Codecov Report in the comment section below once CI passes

+ Adding DEPRECATED attribute.

robert-zaremba · 2020-10-26T17:48:05Z

server/README.md

@@ -34,7 +34,7 @@ var (
 		},
 	}

-	encodingConfig = simapp.MakeEncodingConfig()
+	encodingConfig = simapp.MakeTestEncodingConfig()


Any idea how we should update this README file?

It seams that the MakeTestEncodingConfig is needed to users who want to use cli.

No it shouldn't be. Not sure why this is here.

That example code needs to be rewritten to not use EncodingConfig

No it shouldn't be. Not sure why this is here.

Another point that this task is useful ;)

codecov · 2020-10-26T17:52:05Z

Codecov Report

Merging #7681 into master will not change coverage.
The diff coverage is 73.07%.

@@           Coverage Diff           @@
##           master    #7681   +/-   ##
=======================================
  Coverage   54.14%   54.14%           
=======================================
  Files         611      611           
  Lines       38558    38558           
=======================================
  Hits        20876    20876           
  Misses      15550    15550           
  Partials     2132     2132

clevinson · 2020-10-26T23:00:04Z

simapp/simd/cmd/root.go

@@ -38,7 +38,7 @@ import (
 // NewRootCmd creates a new root command for simd. It is called once in the
 // main function.
 func NewRootCmd() (*cobra.Command, params.EncodingConfig) {
-	encodingConfig := simapp.MakeEncodingConfig()
+	encodingConfig := simapp.MakeTestEncodingConfig()


This line is hte line being referenced in the documentation at server/README.md. I know that "simapp" in generally is mainly for testing purposes, but it seems strange to call this MakeTestEncodingConfig here, when from what I understand users will mostly be copying a lot of simapp/simd code into their own application when building their own chains.

Perhaps its fine if we have a better strategy for users than looking at simapp/ code when bootstrapping their own app.

@aaronc when you said that we shouldint depend on EncodingConfig at all in the server/README.md, what do you suggest we do here instead? explicitly do all the interface registry stuff here in this function to make it super clear ?

MakeTestEncodingConfig is just the wrong name for this function I'd say. What was the rationale in the first place?

It's just for switching between amino and protobuf builds.

I don't think simapp is intended to be a reference for apps. The tutorial is for that. Simd was only added during stargate work for testing it was never intended as a reference.

If we want to make simapp a reference for apps we should pull out all the legacy stuff and fully deprecate amino support which will allow EncodingConfig to go away.

Apps should just create a codec and register interfaces as needed. There is a reason why EncodingConfig is in simapp and not part of the SDK proper.

It makes sense to me, thanks. Yet IMHO the MakeTestEncodingConfig name does not suggest anything like that and MakeEncodingConfig seems still the best name?

@alessio - please check the referenced issue: #7642

I'm open for a better name suggestions. As stated in this PR, it's a temporal solution to make it clear for the users that they shouldn't use this method.

amaury1093 · 2020-10-27T09:49:53Z

simapp/app.go

@@ -432,7 +432,7 @@ func NewSimApp(
 // simapp. It is useful for tests and clients who do not want to construct the
 // full simapp
 func MakeCodecs() (codec.Marshaler, *codec.LegacyAmino) {
-	config := MakeEncodingConfig()
+	config := MakeTestEncodingConfig()


Suggested change

config := MakeTestEncodingConfig()

config := simappparams.MakeEncodingConfig()

std.RegisterLegacyAminoCodec(config.Amino)

std.RegisterInterfaces(config.InterfaceRegistry)

ModuleBasics.RegisterLegacyAminoCodec(config.Amino)

ModuleBasics.RegisterInterfaces(config.InterfaceRegistry)

How about:

Move simapp/encoding.go to testutil/encoding.go (That's where it really belongs)

Make the above change (as per simapp: rename MakeEncodingConfig to MakeTestEncodingConfig #7681 (comment), "Apps should just create a codec and register interfaces as needed.")

Advantages:

MakeTestEncodingConfig lives in testutil, which makes sense.

For users looking at simapp as a reference implementation, they will do registration directly in NewSimApp as above. This imports MakeEncodingConfig from ./params, which is okay.

Disadvantages:

We write twice the registration (once for tests, once for reference). It's only executed once though).

I like this approach! 👍

This is what we want to do in #7642. This PR, as noted in the description, is a temporal solution for that.

In fact what we want to do in #7642 is to hide the MakeTestEncodingConfig in a private method, and use app.AppCodecs outside.

amaury1093

I think this change is still an improvement. It's not perfect, but "perfect" requires a way bigger refactoring. So I'm okay to merge this now.

* simapp: rename MakeEncodingConfig to MakeTestEncodingConfig * Updating the Changelog + Adding DEPRECATED attribute. Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>

) * simapp: rename MakeEncodingConfig to MakeTestEncodingConfig * Updating the Changelog + Adding DEPRECATED attribute. Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>

simapp: rename MakeEncodingConfig to MakeTestEncodingConfig

c56d097

robert-zaremba requested review from aaronc, alessio and alexanderbez as code owners October 26, 2020 17:43

robert-zaremba requested review from amaury1093 and blushi October 26, 2020 17:43

Updating the Changelog

b4244b3

+ Adding DEPRECATED attribute.

robert-zaremba commented Oct 26, 2020

View reviewed changes

clevinson mentioned this pull request Oct 26, 2020

[docs]: Update server/README.md with alternative to simapp.MakeEncodingConfig() #7685

Closed

clevinson reviewed Oct 26, 2020

View reviewed changes

amaury1093 reviewed Oct 27, 2020

View reviewed changes

amaury1093 approved these changes Oct 27, 2020

View reviewed changes

alessio approved these changes Oct 27, 2020

View reviewed changes

robert-zaremba added T: API Breaking Breaking changes that impact APIs and the SDK only (not state machine). A:automerge Automatically merge PR once all prerequisites pass. labels Oct 27, 2020

Merge branch 'master' into robert/makeencodingconfig

03c4829

robert-zaremba added the backport/0.40.x (Stargate) label Oct 27, 2020

mergify bot merged commit 9bd42ac into master Oct 27, 2020

mergify bot deleted the robert/makeencodingconfig branch October 27, 2020 11:33

clevinson mentioned this pull request Oct 29, 2020

[backports]: v0.40.0-rc2 Cherry-picks #7734

Closed

45 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

simapp: rename MakeEncodingConfig to MakeTestEncodingConfig #7681

simapp: rename MakeEncodingConfig to MakeTestEncodingConfig #7681

robert-zaremba commented Oct 26, 2020 •

edited

Loading

robert-zaremba Oct 26, 2020

robert-zaremba Oct 26, 2020

aaronc Oct 26, 2020

aaronc Oct 26, 2020

robert-zaremba Oct 26, 2020

codecov bot commented Oct 26, 2020 •

edited

Loading

clevinson Oct 26, 2020

alessio Oct 26, 2020

aaronc Oct 27, 2020 •

edited

Loading

alessio Oct 27, 2020

robert-zaremba Oct 27, 2020

amaury1093 Oct 27, 2020 •

edited

Loading

alessio Oct 27, 2020

robert-zaremba Oct 27, 2020

robert-zaremba Oct 27, 2020

amaury1093 left a comment

-	config := MakeTestEncodingConfig()
+	config := simappparams.MakeEncodingConfig()
+	std.RegisterLegacyAminoCodec(config.Amino)
+	std.RegisterInterfaces(config.InterfaceRegistry)
+	ModuleBasics.RegisterLegacyAminoCodec(config.Amino)
+	ModuleBasics.RegisterInterfaces(config.InterfaceRegistry)

simapp: rename MakeEncodingConfig to MakeTestEncodingConfig #7681

simapp: rename MakeEncodingConfig to MakeTestEncodingConfig #7681

Conversation

robert-zaremba commented Oct 26, 2020 • edited Loading

Description

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

codecov bot commented Oct 26, 2020 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aaronc Oct 27, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amaury1093 Oct 27, 2020 • 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

amaury1093 left a comment

Choose a reason for hiding this comment

robert-zaremba commented Oct 26, 2020 •

edited

Loading

codecov bot commented Oct 26, 2020 •

edited

Loading

aaronc Oct 27, 2020 •

edited

Loading

amaury1093 Oct 27, 2020 •

edited

Loading