-
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
simapp: rename MakeEncodingConfig to MakeTestEncodingConfig #7681
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -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() | ||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
How about:
Advantages:
Disadvantages:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I like this approach! 👍 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is what we want to do in #7642. This PR, as noted in the description, is a temporal solution for that. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In fact what we want to do in #7642 is to hide the |
||||||||||||||
return config.Marshaler, config.Amino | ||||||||||||||
} | ||||||||||||||
|
||||||||||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This line is hte line being referenced in the documentation at Perhaps its fine if we have a better strategy for users than looking at @aaronc when you said that we shouldint depend on There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It makes sense to me, thanks. Yet IMHO the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
initClientCtx := client.Context{}. | ||
WithJSONMarshaler(encodingConfig.Marshaler). | ||
WithInterfaceRegistry(encodingConfig.InterfaceRegistry). | ||
|
@@ -191,7 +191,7 @@ func newApp(logger log.Logger, db dbm.DB, traceStore io.Writer, appOpts serverty | |
logger, db, traceStore, true, skipUpgradeHeights, | ||
cast.ToString(appOpts.Get(flags.FlagHome)), | ||
cast.ToUint(appOpts.Get(server.FlagInvCheckPeriod)), | ||
simapp.MakeEncodingConfig(), // Ideally, we would reuse the one created by NewRootCmd. | ||
simapp.MakeTestEncodingConfig(), // Ideally, we would reuse the one created by NewRootCmd. | ||
baseapp.SetPruning(pruningOpts), | ||
baseapp.SetMinGasPrices(cast.ToString(appOpts.Get(server.FlagMinGasPrices))), | ||
baseapp.SetHaltHeight(cast.ToUint64(appOpts.Get(server.FlagHaltHeight))), | ||
|
@@ -211,7 +211,7 @@ func newApp(logger log.Logger, db dbm.DB, traceStore io.Writer, appOpts serverty | |
func createSimappAndExport( | ||
logger log.Logger, db dbm.DB, traceStore io.Writer, height int64, forZeroHeight bool, jailAllowedAddrs []string, | ||
) (servertypes.ExportedApp, error) { | ||
encCfg := simapp.MakeEncodingConfig() // Ideally, we would reuse the one created by NewRootCmd. | ||
encCfg := simapp.MakeTestEncodingConfig() // Ideally, we would reuse the one created by NewRootCmd. | ||
encCfg.Marshaler = codec.NewProtoCodec(encCfg.InterfaceRegistry) | ||
var simApp *simapp.SimApp | ||
if height != -1 { | ||
|
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.
Any idea how we should update this README file?
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.
It seams that the
MakeTestEncodingConfig
is needed to users who want to usecli
.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.
No it shouldn't be. Not sure why this is here.
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.
That example code needs to be rewritten to not use EncodingConfig
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.
Another point that this task is useful ;)