Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from
Draft

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
21785c0
feat: Migrate networks to yml file
activescott Mar 11, 2024
cef7336
fix: review feedback: Addresses substruct and added support for extra…
activescott Mar 12, 2024
7bc1aba
Update shared/services/config/smartnode-config.go
activescott Mar 13, 2024
782ff8f
chore: add network lookup map and rename loadNetworksFile per review …
activescott Mar 13, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion rocketpool-cli/rocketpool-cli.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,14 +145,15 @@ ______ _ _ ______ _
if cfg.Smartnode.GetRplFaucetAddress() != "" {
faucet.RegisterCommands(app, "faucet", []string{"f"})
}

service.RegisterCommands(app, "service", []string{"s"}, cfg)
}

minipool.RegisterCommands(app, "minipool", []string{"m"})
network.RegisterCommands(app, "network", []string{"e"})
node.RegisterCommands(app, "node", []string{"n"})
odao.RegisterCommands(app, "odao", []string{"o"})
queue.RegisterCommands(app, "queue", []string{"q"})
service.RegisterCommands(app, "service", []string{"s"})
activescott marked this conversation as resolved.
Show resolved Hide resolved
wallet.RegisterCommands(app, "wallet", []string{"w"})

app.Before = func(c *cli.Context) error {
Expand Down
9 changes: 4 additions & 5 deletions rocketpool-cli/service/commands.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,17 +80,16 @@ func createFlagsFromConfigParams(sectionName string, params []*cfgtypes.Paramete
}

// Register commands
func RegisterCommands(app *cli.App, name string, aliases []string) {
func RegisterCommands(app *cli.App, name string, aliases []string, cfg *config.RocketPoolConfig) {

configFlags := []cli.Flag{}
cfgTemplate := config.NewRocketPoolConfig("", false)
network := cfgTemplate.Smartnode.Network.Value.(cfgtypes.Network)
network := cfg.Smartnode.Network.Value.(cfgtypes.Network)

// Root params
configFlags = createFlagsFromConfigParams("", cfgTemplate.GetParameters(), configFlags, network)
configFlags = createFlagsFromConfigParams("", cfg.GetParameters(), configFlags, network)

// Subconfigs
for sectionName, subconfig := range cfgTemplate.GetSubconfigs() {
for sectionName, subconfig := range cfg.GetSubconfigs() {
configFlags = createFlagsFromConfigParams(sectionName, subconfig.GetParameters(), configFlags, network)
}

Expand Down
2 changes: 1 addition & 1 deletion rocketpool-cli/service/service.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1628,7 +1628,7 @@ func resyncEth2(c *cli.Context) error {

// Generate a YAML file that shows the current configuration schema, including all of the parameters and their descriptions
func getConfigYaml(c *cli.Context) error {
cfg := config.NewRocketPoolConfig("", false)
cfg := config.NewRocketPoolConfig("", false, config.NewNetworksConfig())
bytes, err := yaml.Marshal(cfg)
if err != nil {
return fmt.Errorf("error serializing configuration schema: %w", err)
Expand Down
65 changes: 65 additions & 0 deletions shared/services/config/networks-config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,65 @@
package config

import (
"fmt"
"os"
"path"

"github.com/rocket-pool/smartnode/shared/types/config"
"gopkg.in/yaml.v2"
)

const defaultNetworksConfigPath = "networks-default.yml"

type NetworksConfig struct {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Networks []*config.NetworkInfo `yaml:"networks"`
}

func NewNetworksConfig() *NetworksConfig {
return &NetworksConfig{
Networks: make([]*config.NetworkInfo, 0),
}
}

func LoadNetworksFromFile(configPath string) (*NetworksConfig, error) {
_, err := os.Stat(configPath)
if os.IsNotExist(err) {
return nil, fmt.Errorf("config path does not exist: %s", configPath)
}

filePath := path.Join(configPath, defaultNetworksConfigPath)
if os.IsNotExist(err) {
return nil, fmt.Errorf("default networks file does not exist: %s", filePath)
}

bytes, err := os.ReadFile(filePath)
if err != nil {
return nil, fmt.Errorf("could not read default networks file: %w", err)
}

networks := NetworksConfig{}
err = yaml.Unmarshal(bytes, &networks)
if err != nil {
return nil, fmt.Errorf("could not unmarshal default networks file: %w", err)
}

return &networks, nil
}

func (nc *NetworksConfig) GetNetworks() []*config.NetworkInfo {
return nc.Networks
}

func (nc *NetworksConfig) SaveToFile(filePath string) error {
// TODO: this signature doesn't support saving to default/extras files
d, err := yaml.Marshal(nc)
if err != nil {
return fmt.Errorf("could not marshal networks config: %w", err)
}

err = os.WriteFile(filePath, d, 0644)
if err != nil {
return fmt.Errorf("could not write networks config to file to %s: %w", filePath, err)
}
return nil
}
15 changes: 11 additions & 4 deletions shared/services/config/rocket-pool-config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,8 @@ type RocketPoolConfig struct {

IsNativeMode bool `yaml:"-"`

networks *NetworksConfig `yaml:"-"`

// Execution client settings
ExecutionClientMode config.Parameter `yaml:"executionClientMode,omitempty"`
ExecutionClient config.Parameter `yaml:"executionClient,omitempty"`
Expand Down Expand Up @@ -165,9 +167,13 @@ func LoadFromFile(path string) (*RocketPoolConfig, error) {
if err := yaml.Unmarshal(configBytes, &settings); err != nil {
return nil, fmt.Errorf("could not parse settings file: %w", err)
}

// now load the networks:
networks, err := LoadNetworksFromFile(filepath.Dir(path))
if err != nil {
return nil, fmt.Errorf("could not load networks file: %w", err)
}
Comment on lines +170 to +174
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@jshufro jshufro Mar 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

// Deserialize it into a config object
cfg := NewRocketPoolConfig(filepath.Dir(path), false)
cfg := NewRocketPoolConfig(filepath.Dir(path), false, networks)
err = cfg.Deserialize(settings)
if err != nil {
return nil, fmt.Errorf("could not deserialize settings file: %w", err)
Expand All @@ -178,7 +184,7 @@ func LoadFromFile(path string) (*RocketPoolConfig, error) {
}

// Creates a new Rocket Pool configuration instance
func NewRocketPoolConfig(rpDir string, isNativeMode bool) *RocketPoolConfig {
func NewRocketPoolConfig(rpDir string, isNativeMode bool, networks *NetworksConfig) *RocketPoolConfig {

clientModes := []config.ParameterOption{{
Name: "Locally Managed",
Expand All @@ -194,6 +200,7 @@ func NewRocketPoolConfig(rpDir string, isNativeMode bool) *RocketPoolConfig {
Title: "Top-level Settings",
RocketPoolDirectory: rpDir,
IsNativeMode: isNativeMode,
networks: networks,

ExecutionClientMode: config.Parameter{
ID: "executionClientMode",
Expand Down Expand Up @@ -503,7 +510,7 @@ func getAugmentedEcDescription(client config.ExecutionClient, originalDescriptio

// Create a copy of this configuration.
func (cfg *RocketPoolConfig) CreateCopy() *RocketPoolConfig {
newConfig := NewRocketPoolConfig(cfg.RocketPoolDirectory, cfg.IsNativeMode)
newConfig := NewRocketPoolConfig(cfg.RocketPoolDirectory, cfg.IsNativeMode, cfg.networks)

// Set the network
network := cfg.Smartnode.Network.Value.(config.Network)
Expand Down
Loading