tidy up usage and flags (#149)

* tidy up usage and flags * Fix sign mode flag description * nits * add --bare flag to config init to disable validation * update test data
strangelove-ventures · May 15, 2023 · cf9a0ce · cf9a0ce
1 parent c466a8c
commit cf9a0ce
Show file tree

Hide file tree

Showing 8 changed files with 75 additions and 46 deletions.
diff --git a/cmd/horcrux/cmd/address.go b/cmd/horcrux/cmd/address.go
@@ -31,21 +31,23 @@ func addressCmd() *cobra.Command {
 
 			var pubKey crypto.PubKey
 
+			chainID := args[0]
+
 			switch config.Config.SignMode {
 			case signer.SignModeThreshold:
 				err := config.Config.ValidateThresholdModeConfig()
 				if err != nil {
 					return err
 				}
 
-				keyFile, err := config.KeyFileExistsCosigner(args[0])
+				keyFile, err := config.KeyFileExistsCosigner(chainID)
 				if err != nil {
 					return err
 				}
 
 				key, err := signer.LoadCosignerEd25519Key(keyFile)
 				if err != nil {
-					return fmt.Errorf("error reading cosigner key: %s", err)
+					return fmt.Errorf("error reading cosigner key: %w, check that key is present for chain ID: %s", err, chainID)
 				}
 
 				pubKey = key.PubKey
@@ -54,9 +56,9 @@ func addressCmd() *cobra.Command {
 				if err != nil {
 					return err
 				}
-				keyFile, err := config.KeyFileExistsSingleSigner(args[0])
+				keyFile, err := config.KeyFileExistsSingleSigner(chainID)
 				if err != nil {
-					return err
+					return fmt.Errorf("error reading priv-validator key: %w, check that key is present for chain ID: %s", err, chainID)
 				}
 
 				filePV := cometprivval.LoadFilePVEmptyState(keyFile, "")

diff --git a/cmd/horcrux/cmd/config.go b/cmd/horcrux/cmd/config.go
@@ -17,6 +17,7 @@ const (
 	flagRaftTimeout = "raft-timeout"
 	flagGRPCTimeout = "grpc-timeout"
 	flagOverwrite   = "overwrite"
+	flagBare        = "bare"
 )
 
 func configCmd() *cobra.Command {
@@ -33,16 +34,17 @@ func configCmd() *cobra.Command {
 
 func initCmd() *cobra.Command {
 	cmd := &cobra.Command{
-		Use:     "init [chain-nodes]",
+		Use:     "init",
 		Aliases: []string{"i"},
 		Short:   "initialize configuration file and home directory if one doesn't already exist",
-		Long: "initialize configuration file, use flags for cosigner configuration.\n\n" +
-			"[chain-nodes] is a comma separated array of chain node addresses i.e.\n" +
-			"tcp://chain-node-1:1234,tcp://chain-node-2:1234",
+		Long: `initialize configuration file.
+for threshold signer mode, --cosigner flags and --threshold flag are required.
+		`,
 		Args: cobra.NoArgs,
 		RunE: func(cmd *cobra.Command, args []string) (err error) {
 			cmdFlags := cmd.Flags()
 
+			bare, _ := cmdFlags.GetBool(flagBare)
 			nodes, _ := cmdFlags.GetStringSlice(flagNode)
 
 			cn, err := signer.ChainNodesFromFlag(nodes)
@@ -89,8 +91,11 @@ func initCmd() *cobra.Command {
 					ChainNodes: cn,
 					DebugAddr:  debugAddr,
 				}
-				if err = cfg.ValidateThresholdModeConfig(); err != nil {
-					return err
+
+				if !bare {
+					if err = cfg.ValidateThresholdModeConfig(); err != nil {
+						return err
+					}
 				}
 			} else {
 				// Single Signer Config
@@ -100,8 +105,10 @@ func initCmd() *cobra.Command {
 					ChainNodes:    cn,
 					DebugAddr:     debugAddr,
 				}
-				if err = cfg.ValidateSingleSignerConfig(); err != nil {
-					return err
+				if !bare {
+					if err = cfg.ValidateSingleSignerConfig(); err != nil {
+						return err
+					}
 				}
 			}
 
@@ -122,23 +129,34 @@ func initCmd() *cobra.Command {
 			return nil
 		},
 	}
-	cmd.Flags().StringP(flagSignMode, "m", string(signer.SignModeThreshold),
-		`sign mode, "threshold" (recommended) or "single" (unsupported). threshold mode requires --cosigners and --threshold`,
+
+	f := cmd.Flags()
+	f.StringP(flagSignMode, "m", string(signer.SignModeThreshold),
+		`sign mode, "threshold" (recommended) or "single" (unsupported). threshold mode requires --cosigner (multiple) and --threshold`, //nolint
 	)
-	cmd.Flags().StringSliceP(flagNode, "n", []string{}, "chain nodes in format tcp://{p2p-addr}:{port}")
-	cmd.Flags().StringSliceP(flagCosigner, "c", []string{},
-		"cosigners in format tcp://{p2p-addr}:{port}|{shard-id} \n"+
-			"(i.e. \"tcp://node-1:2222|1,tcp://node-2:2222|2,tcp://node-3:2222|3\")")
-	cmd.Flags().IntP(flagThreshold, "t", 0, "number of shards required for threshold signature")
-	cmd.Flags().StringP(
+	f.StringSliceP(flagNode, "n", []string{}, "chain nodes in format tcp://{node-addr}:{privval-port} \n"+
+		"(e.g. --node tcp://sentry-1:1234 --node tcp://sentry-2:1234 --node tcp://sentry-3:1234 )")
+
+	f.StringSliceP(flagCosigner, "c", []string{},
+		`cosigners in format tcp://{cosigner-addr}:{p2p-port}
+(e.g. --cosigner tcp://horcrux-1:2222 --cosigner tcp://horcrux-2:2222 --cosigner tcp://horcrux-3:2222)`)
+
+	f.IntP(flagThreshold, "t", 0, "number of shards required for threshold signature")
+
+	f.StringP(
 		flagDebugAddr, "d", "",
 		"listen address for debug server and prometheus metrics in format localhost:8543",
 	)
-	cmd.Flags().StringP(flagKeyDir, "k", "", "key directory if other than home directory")
-	cmd.Flags().String(flagRaftTimeout, "1500ms", "cosigner raft timeout value, \n"+
+	f.StringP(flagKeyDir, "k", "", "key directory if other than home directory")
+	f.String(flagRaftTimeout, "1500ms", "cosigner raft timeout value, \n"+
 		"accepts valid duration strings for Go's time.ParseDuration() e.g. 1s, 1000ms, 1.5m")
-	cmd.Flags().String(flagGRPCTimeout, "1500ms", "cosigner grpc timeout value, \n"+
+	f.String(flagGRPCTimeout, "1500ms", "cosigner grpc timeout value, \n"+
 		"accepts valid duration strings for Go's time.ParseDuration() e.g. 1s, 1000ms, 1.5m")
-	cmd.Flags().BoolP(flagOverwrite, "o", false, "overwrite an existing config.yaml")
+	f.BoolP(flagOverwrite, "o", false, "overwrite an existing config.yaml")
+	f.Bool(
+		flagBare,
+		false,
+		"allows initialization without providing any flags. If flags are provided, will not perform final validation",
+	)
 	return cmd
 }
diff --git a/cmd/horcrux/cmd/config_test.go b/cmd/horcrux/cmd/config_test.go
@@ -46,6 +46,7 @@ thresholdMode:
 chainNodes:
 - privValAddr: tcp://10.168.0.1:1234
 - privValAddr: tcp://10.168.0.2:1234
+debugAddr: ""
 `,
 		},
 		{
@@ -60,6 +61,7 @@ chainNodes:
 chainNodes:
 - privValAddr: tcp://10.168.0.1:1234
 - privValAddr: tcp://10.168.0.2:1234
+debugAddr: ""
 `,
 		},
 		{

diff --git a/cmd/horcrux/cmd/shards.go b/cmd/horcrux/cmd/shards.go
@@ -55,15 +55,9 @@ func addOutputDirFlag(cmd *cobra.Command) {
 	cmd.Flags().StringP(flagOutputDir, "", "", "output directory")
 }
 
-func addShardsFlag(cmd *cobra.Command) {
+func addTotalShardsFlag(cmd *cobra.Command) {
 	cmd.Flags().Uint8(flagShards, 0, "total key shards")
-}
-
-func addShardFlags(cmd *cobra.Command) {
-	addShardsFlag(cmd)
-	cmd.Flags().Uint8(flagThreshold, 0, "threshold number of shards required to successfully sign")
-	cmd.Flags().String(flagKeyFile, "", "priv_validator_key.json file to shard")
-	cmd.Flags().String(flagChainID, "", "key shards will sign for this chain ID")
+	_ = cmd.MarkFlagRequired(flagShards)
 }
 
 // createCosignerEd25519ShardsCmd is a cobra command for creating
@@ -84,19 +78,19 @@ func createCosignerEd25519ShardsCmd() *cobra.Command {
 			var errs []error
 
 			if keyFile == "" {
-				errs = append(errs, fmt.Errorf("key-file flag must be provided and non-empty"))
+				errs = append(errs, fmt.Errorf("key-file flag must not be empty"))
 			}
 
 			if chainID == "" {
-				errs = append(errs, fmt.Errorf("chain-id flag must be provided and non-empty"))
+				errs = append(errs, fmt.Errorf("chain-id flag must not be empty"))
 			}
 
 			if threshold == 0 {
-				errs = append(errs, fmt.Errorf("threshold flag must be provided and non-zero"))
+				errs = append(errs, fmt.Errorf("threshold flag must be > 0, <= --shards, and > --shards/2"))
 			}
 
 			if shards == 0 {
-				errs = append(errs, fmt.Errorf("shards flag must be provided and non-zero"))
+				errs = append(errs, fmt.Errorf("shards flag must be greater than zero"))
 			}
 
 			if _, err := os.Stat(keyFile); err != nil {
@@ -148,15 +142,25 @@ func createCosignerEd25519ShardsCmd() *cobra.Command {
 			return nil
 		},
 	}
-	addShardFlags(cmd)
+
 	addOutputDirFlag(cmd)
+	addTotalShardsFlag(cmd)
+
+	f := cmd.Flags()
+	f.Uint8(flagThreshold, 0, "threshold number of shards required to successfully sign")
+	_ = cmd.MarkFlagRequired(flagThreshold)
+	f.String(flagKeyFile, "", "priv_validator_key.json file to shard")
+	_ = cmd.MarkFlagRequired(flagKeyFile)
+	f.String(flagChainID, "", "key shards will sign for this chain ID")
+	_ = cmd.MarkFlagRequired(flagChainID)
+
 	return cmd
 }
 
 // createCosignerRSAShardsCmd is a cobra command for creating cosigner shards from a priv validator
 func createCosignerRSAShardsCmd() *cobra.Command {
 	cmd := &cobra.Command{
-		Use:   "create-rsa-shards shards",
+		Use:   "create-rsa-shards",
 		Args:  cobra.NoArgs,
 		Short: "Create cosigner RSA shards",
 
@@ -196,7 +200,7 @@ func createCosignerRSAShardsCmd() *cobra.Command {
 			return nil
 		},
 	}
-	addShardsFlag(cmd)
+	addTotalShardsFlag(cmd)
 	addOutputDirFlag(cmd)
 	return cmd
 }
diff --git a/cmd/horcrux/cmd/state.go b/cmd/horcrux/cmd/state.go
@@ -74,7 +74,7 @@ func showStateCmd() *cobra.Command {
 
 func setStateCmd() *cobra.Command {
 	return &cobra.Command{
-		Use:          "set [chain-id] [height]",
+		Use:          "set chain-id height",
 		Aliases:      []string{"s"},
 		Short:        "Set the height for the sign state of a specific chain-id",
 		Args:         cobra.ExactArgs(2),
@@ -136,7 +136,7 @@ func setStateCmd() *cobra.Command {
 
 func importStateCmd() *cobra.Command {
 	return &cobra.Command{
-		Use:     "import [chain-id]",
+		Use:     "import chain-id",
 		Aliases: []string{"i"},
 		Short: "Read the old priv_validator_state.json and set the height, round and step" +
 			"(good for migrations but NOT shared state update)",

diff --git a/cmd/horcrux/cmd/testdata/config-migrated.yaml b/cmd/horcrux/cmd/testdata/config-migrated.yaml
@@ -14,3 +14,4 @@ chainNodes:
 - privValAddr: tcp://127.0.0.1:1234
 - privValAddr: tcp://127.0.0.1:2345
 - privValAddr: tcp://127.0.0.1:3456
+debugAddr: ""
diff --git a/signer/config.go b/signer/config.go
@@ -33,8 +33,8 @@ type Config struct {
 	PrivValKeyDir       *string              `yaml:"keyDir,omitempty"`
 	SignMode            SignMode             `yaml:"signMode"`
 	ThresholdModeConfig *ThresholdModeConfig `yaml:"thresholdMode,omitempty"`
-	ChainNodes          ChainNodes           `yaml:"chainNodes,omitempty"`
-	DebugAddr           string               `yaml:"debugAddr,omitempty"`
+	ChainNodes          ChainNodes           `yaml:"chainNodes"`
+	DebugAddr           string               `yaml:"debugAddr"`
 }
 
 func (c *Config) Nodes() (out []string) {
@@ -304,10 +304,11 @@ func (cns ChainNodes) Validate() error {
 	return errors.Join(errs...)
 }
 
-func ChainNodesFromFlag(nodes []string) (out ChainNodes, err error) {
-	for _, n := range nodes {
+func ChainNodesFromFlag(nodes []string) (ChainNodes, error) {
+	out := make(ChainNodes, len(nodes))
+	for i, n := range nodes {
 		cn := ChainNode{PrivValAddr: n}
-		out = append(out, cn)
+		out[i] = cn
 	}
 	if err := out.Validate(); err != nil {
 		return nil, err

diff --git a/signer/config_test.go b/signer/config_test.go
@@ -435,6 +435,7 @@ chainNodes:
 - privValAddr: tcp://127.0.0.1:1234
 - privValAddr: tcp://127.0.0.1:2345
 - privValAddr: tcp://127.0.0.1:3456
+debugAddr: ""
 `, string(configYamlBz))
 }