feat(node): add node_reward_type to config tool #2641
Conversation
rs/ic_os/config/src/types.rs
Outdated
| /// The node reward type determines node rewards | ||
| pub node_reward_type: String, |
There was a problem hiding this comment.
node_reward_type exists as an enum in the registry protobuf:
Rather than maintain the enum in two locations, we can leave it as a string here and just do a basic formatting check in the CreateSetuposConfig command
There was a problem hiding this comment.
This may be a slippery slope, but why can't we rely on these types directly?
We will need to do this mapping at some point when nodes use this config value to communicate with the registry - why not now? If the enum is in flux, it might be another good story to think about for config compatibility.
There was a problem hiding this comment.
Okay I actually looked at the enum this time 🤦♀️.
We will still need some parsing to map the "human" names to these enum values, and at that point it would be good to use the types directly.
Until that mapping exists, though, this basic check in SetupOS looks fine. We just need to be wary that every variant that makes it past parsing needs to eventually be handled. (The same applies to the check from #1787.)
| @@ -61,6 +61,8 @@ pub struct GuestOSConfig { | |||
|
|
|||
| #[derive(Serialize, Deserialize, Debug, PartialEq, Eq, Clone)] | |||
| pub struct ICOSSettings { | |||
| /// The node reward type determines node rewards | |||
| pub node_reward_type: Option<String>, | |||
There was a problem hiding this comment.
node_reward_type is Optional in config, even though it's required for SetupOS installation, because nodes on mainnet currently don't have the field, so this way, the update-config.service can leave the value as optional
NODE-1474
node_reward_type was just added to config.ini. This PR adds it to the new config tool.