Cleanup params in Babylon custom modules. #334
Conversation
There was a problem hiding this comment.
Thanks for the effort and this indeed cuts a lot of code. However, I have some concern that removing boilerplate code for params might make it hard for us to add new parameters to these modules in the future. It could be possible that we want some new parameters in a certain module, and may even want to update their values upon gov prop. Given these reasons I'm inclined to keep the code as is. No strong objection, but would like to hear feedback from more ppl
| import "babylon/checkpointing/v1/bls_key.proto"; | ||
|
|
||
| option go_package = "github.com/babylonchain/babylon/x/checkpointing/types"; | ||
|
|
||
| // GenesisState defines the checkpointing module's genesis state. | ||
| message GenesisState { | ||
| // params defines all the paramaters of related to checkpointing | ||
| Params params = 1 [ (gogoproto.nullable) = false ]; |
There was a problem hiding this comment.
It it worth keeping the Params message and field here in case of any new parameters in the future?
| import "babylon/btclightclient/v1/btclightclient.proto"; | ||
|
|
||
| option go_package = "github.com/babylonchain/babylon/x/btclightclient/types"; | ||
|
|
||
| // GenesisState defines the btclightclient module's genesis state. | ||
| message GenesisState { | ||
| Params params = 1 [ (gogoproto.nullable) = false ]; |
There was a problem hiding this comment.
same here, is it worth keeping params?
|
|
||
| option go_package = "github.com/babylonchain/babylon/x/monitor/types"; | ||
|
|
||
| // GenesisState defines the monitor module's genesis state. | ||
| message GenesisState { Params params = 1 [ (gogoproto.nullable) = false ]; } |
|
|
||
| option go_package = "github.com/babylonchain/babylon/x/zoneconcierge/types"; | ||
|
|
||
| // GenesisState defines the zoneconcierge module's genesis state. | ||
| message GenesisState { | ||
| Params params = 1 [ (gogoproto.nullable) = false ]; |
There was a problem hiding this comment.
Same here. Notably, a param that can be upgraded by gov prop might be useful when we add support to phase 3 when at phase 2.
There was a problem hiding this comment.
Agree with @SebastianElvis here. Do we get anything else other than less boilerplate code if we remove them? From what I understand from #325 , we are already compliant with the change (at least with the changes related to this PR), as we store module parameters in the module.
So currently we store params in x/params module which is now deprecated and from cosmos sdk 0.48 will be removed. Given the fact that we are currently always on newest version, I assume we will also want to switch to 0.48, so we will want to make a switch to a new version. It makes sense to do it before mainnet as this way we will avoid any upgrade handlers etc.
So the effort to add params later from scratch versus add new param to params proto will be roughly the same. You will need upgrade handler which will update required proto files or create v2 of proto file, and probably gov proposal to introduce them. Also current situation is mostly a result of copy/pasting modules at the beginning of our project which is not really good design. In general carrying code which may/or may not become useful in some not specified time in the future is a smell that something is not right. In general, we have still some time till mainnet, so we have time to decide if any of those modules will require params. If we happen to decide that this is the case, it will be done in new way which is much different from current one. So in summary imo it makes sense to:
Keeping 4k lines of code just in case just sounds like a bad deal. |
A bit of preparation before fixing - #325
A lot of babylon modules used empty
Paramsprotos. This makes little sense as it just generate a lot unnecessary code. This pr removes thoseParamsprotos and all related code.