feat: Migrate networks to yml file (1 of 2)

[activescott]

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:

• Not super clear what would be an effective set of tests as much of that data is used in operations that I'm not sure how to easily/quickly exercise. If you want to suggest some test cases I'm happy to run them. I was able to use rocketpool service config and restart my holesky node and it seems fine.
• Am I missing some other places where network info is needed?

[jshufro]

Directionally, looks good. Might be neater to move all the Address fields into a substruct of NetworkInfo

[activescott]

@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.

[jshufro]

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

[activescott]

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.

[jshufro]

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.

[activescott]

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! 😅

[jshufro]

Seems like a good option as well. I think this one is ready for @0xfornax but i'll give it one last pass

[jshufro]

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.

[activescott]

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.

[jshufro]

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...

[activescott]

:) 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.

[jshufro]

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

[activescott]

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?

[jshufro]

I think the best thing would be to rebase onto the v2 branch. A lot of the work unfortunately won't apply, but the good and bad news is that this was already incomplete- #459 (review) I do think for this to be meaningful we need to be able to add networks simply by adding yaml