feat: Migrate networks to yml file (1 of 2) #459
Conversation
|
@jshufro Added substruct. I also noticed I forgot to add support for extra-networks.yml and did that and tested it. Please let me know what else needs done here to get this one merged. |
| extraNetworksConfigPath = "networks-extra.yml" | ||
| ) | ||
|
|
||
| type NetworksConfig struct { |
There was a problem hiding this comment.
seems like a great opportunity to use type NetworksConfig []*config.NetworkInfo.
You'll have to remove the networks: piece from the smartnode-install .yml file and just have the list be top-level.
but then, instead of:
defaultNetworks.Networks = append(defaultNetworks.Networks, extraNetworks.Networks...) below you can ergonmically return append(defaultNetworks, extraNetworks...)
and you can kill off func (nc *NetworksConfig) GetNetworks() []*config.NetworkInfo
There was a problem hiding this comment.
My thought here was future proofing: We may want other information about that config file rather than just an array of NetworkInfo at some point.
I don't feel strongly though. Let me know if you prefer though the flat array and I can change it.
There was a problem hiding this comment.
Good point, it's easier to extend if the root level is an object. I kind of am leaning towards:
type NetworksConfigs []*config.NetworkInfo
type NetworksConfigFile struct {
Networks []NetworkConfigs `yaml:"networks"`
}now... that way we get the extensibility of the file format and can simplify the ABI here to remove GetNetworks().
If we ever have to add more fields in memory and change the NetworkConfigs type down the line, the compiler will yell at us about the surface area, so it's any easy upgrade path.
There was a problem hiding this comment.
Based on another discussion I've now added func (nc *NetworksConfig) GetNetwork(name config.Network) *config.NetworkInfo to do the lookup of the network in the map. This adds more utility to NetworksConfig. I renamed the old GetNetworks() to AllNetworks() and it is only used in one spot.
So with this in mind I think it's okay. What about you?
Sorry to be going back and forth, I promise I'm not trying to be argumentative! 😅
There was a problem hiding this comment.
Seems like a good option as well. I think this one is ready for @0xfornax but i'll give it one last pass
| // now load the networks: | ||
| networks, err := LoadNetworksFromFile(filepath.Dir(path)) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("could not load networks file: %w", err) | ||
| } |
There was a problem hiding this comment.
kinda seems like NewRocketPoolConfig should handle this. filepath.Dir(path) gets stored in cfg.RocketPoolDirectory on alloc, and then in cfg.Deserialize you can parse the new yml.
There was a problem hiding this comment.
I've looked into this again and I prefer passing it into New for two reasons... NewRocketPoolConfig calls NewSmartnodeConfig which loads a list of network options so it requires networks to be available already.
There is also a place in the client creates a fresh config without deserializing that would require some patching after it is created to even have a valid config. So I think it's more correct to have networks passed in.
Let me know if I'm missing something.
There was a problem hiding this comment.
Can we meet in the middle and have NewRocketPoolConfig parse the files just before calling NewSmartnodeConfig?
In both places NewRocketPoolConfig is called it uses the same path as the previous LoadNetworksFromeFile call
edit: you will, of course, have to turn NewRocketPoolConfig into a function that returns an error, though. Maybe parsing the networks first is not so bad...
There was a problem hiding this comment.
:) I agree with your edit.
There is a third place where that NewRocketPoolConfig that actually doesn't require reading the networks: https://github.com/activescott/smartnode/blob/cef7336fc59d22944c1a5de56fbec89b862a18c7/shared/services/rocketpool/client.go#L230
After reviewing this, I prefer the way it is now. Let me know what you prefer though. I'm open.
Co-authored-by: Jacob Shufro <[email protected]>
There was a problem hiding this comment.
Can you do a brief bit of research to see how hard it is to delete:
// Enum to describe which network the system is on
const (
Network_Unknown Network = ""
Network_All Network = "all"
Network_Mainnet Network = "mainnet"
Network_Devnet Network = "devnet"
Network_Holesky Network = "holesky"
)
The TUI will have to generate settings according to the parsed network, but beyond that, there isn't a ton of value in moving network-related settings to yml when the list of networks itself remains hardcoded.
Network_All of course can stay
|
Hey @jshufro. You mentioned holding off on this. Should I pick up those changes from comments and see this through or is the consensus that we should no longer do this? |
|
I think the best thing would be to rebase onto the |
Submission for milestone B of bounty BA0902402 to move networks to yaml.
The networks-default.yml file is in rocket-pool/smartnode-install#122
fixes #438
@jshufro Can you please review and make sure this is on the right track. I think this is complete. The things that are on my mind though:
rocketpool service configand restart my holesky node and it seems fine.