Use structures for object configuration rather then configuration methods

[robert-zaremba]

Summary

Currently we are using configuration methods for setting optional arguments to constructor, example:

return simapp.NewSimApp(
                ...
		baseapp.SetPruning(pruningOpts),
		baseapp.SetMinGasPrices(cast.ToString(appOpts.Get(server.FlagMinGasPrices))),
		baseapp.SetHaltHeight(cast.ToUint64(appOpts.Get(server.FlagHaltHeight))),
		baseapp.SetHaltTime(cast.ToUint64(appOpts.Get(server.FlagHaltTime))),
		baseapp.SetInterBlockCache(cache),

This approach is more verbose and less explicit about available options. Also, it doesn't detect duplicates.

Problem Definition

When designing a constructor for objects, very often we need to pass some configuration parameters. Some of them are optional, and some of them are more complex. Moreover we need to design for future extensibility.

Proposal

Recently, in recent post at blog.golang.com provides a great background about interface extensibility and maintenance: Keeping Your Modules Compatible. It also discuss the configuration methods vs configuration structure.

Any change to a function’s signature is a breaking change. The situation is much better with structs. If you have an exported struct type, you can almost always add a field or remove an unexported field without breaking compatibility. When adding a field, make sure that its zero value is meaningful and preserves the old behavior, so that existing code that doesn’t set the field continues to work.

Instead of having a terribly lengthy function parameters:

func NewSimApp(
	logger log.Logger, db dbm.DB, traceStore io.Writer, loadLatest bool, skipUpgradeHeights map[int64]bool,
	homePath string, invCheckPeriod uint, encodingConfig simappparams.EncodingConfig, baseAppOptions ...func(*baseapp.BaseApp),
)

we can do:

type AppCfg struct {
   // all high leve configuration goups represented by structures
   Logger  LoggerCfg
   Sim SimCfg
   GRPC GRPCCfg
   /// ...
}
...

func NewSimApp(logger log.Logger, db dbm.DB, simcfg SimCfg, baseAppCfg BaseAppCfg)

TODO:

• Check if we can reuse appOpts somehow Update x/banking and x/crisis InitChain re slow Gaia startup #7764 (comment)
• Update x/banking and x/crisis InitChain re slow Gaia startup #7764 (comment)

---

For Admin Use

• Not duplicate issue
• Appropriate labels applied
• Appropriate contributors tagged
• Contributor assigned/self-assigned

[alexanderbez]

This is specific to the application at hand. While we could modify SimApp, it does nothing really other than providing an "example" for other devs. Personally, I'd prefer to keep the flow as-is and use variadic option funcs: https://dave.cheney.net/2014/10/17/functional-options-for-friendly-apis.

We could however, group the non-optional args into an options struct like you're suggesting, that I'm in favor of 👍

/cc @erikgrinaker

[robert-zaremba]

I think it's good to use best practice for examples.
Struct Configs are more readable, and has other advantages I mentioned above. I didn't grep the code, but maybe there are other places where the config methods are used.

[robert-zaremba]

So, if you will find it useful, I can handle that task.

[alexanderbez]

Sure. Please keep the variadic option funcs, but you you can turn all other args into a struct.

[robert-zaremba]

Sure. Please keep the variadic option funcs, but you you can turn all other args into a struct.

But the whole goal of this proposal is to use Struct Config instead of variadic option funcs.

[alexanderbez]

No, I am not in favor of that. Please read that link I posted above.

[robert-zaremba]

I know the Dave Chaney post. I read it when it was out. In enterprise projects we were more in favor to grouping functions arguments. The same motivation is also nicely discussed in the recent golang.org blog post: https://blog.golang.org/module-compatibility (also linked in the OP).

[tac0turtle]

interestingly this is the approach we are looking into for runtime to make the system more modular, stay tuned for a adr/rfc in the coming week