search

cosmos / cosmos-sdk

:chains: A Framework for Building High Value Public Blockchains :sparkles:
https://cosmos.network/
Apache License 2.0
6.27k stars 3.63k forks source link

Refactor: remove viper in server context #22388

Open kocubinski opened 2 weeks ago

kocubinski commented 2 weeks ago

After the merge of #22267 it will be possible to remove the viper.Viper instance from server command context and replace it with server.ConfigMap. This small bit of refactoring abstracts all config loading away from Viper, since that is handled early in application life cycle with ParseCommand. It will allow for:

1) swapping out config parsing for something else (e.g. viper) 2) swapping out our CLI lib for another one (e.g. cobra) 3) more easily provide custom config to tests calling CLI handlers since a viper instance is not needed, which could decrease our dependency on system tests for nearly everything.

julienrbrt commented 1 week ago

If we do that, we should remove the ViperContextKey from core then, as it will be a v1 thing only. Maybe we should store the whole config is context instead then.

Additionally, ReadConfig returns a viper, and viper really helps for binding the flags, would you want to do that manually?

kocubinski commented 1 week ago

Agree that ViperContextKey should be renamed to ConfigContextKey and return a server.ConfigMap if we go this route. Also ClientCtx should not have a viper.Viper either. I'm not sure how breaking this, maybe we can PoC it a bit and see if it's worth it?

julienrbrt commented 1 week ago

Yes, I agree with the above. To not be breaking it should be a DynamicConfig stored in the context however and not a ConfigMap.