refactor: simplify key command config handling

link/cmd/ibc/config.go

@@ -4,7 +4,6 @@ package main
 
 import (
 	"fmt"
-	"log/slog"
 	"os"
 
 	"github.com/pkg/errors"
@@ -170,43 +169,12 @@ func setupHomeWithConfig() (config.Config, error) {
 		return config.Config{}, err
 	}
 
-	return cfg.OverrideFromFlags(globalFlags)
-}
-
-// setupHomeWithOptionalConfig changes process directory to `--home` and parses the config
-// if the config file doesn't exist, returns the default config
-func setupHomeWithOptionalConfig() (config.Config, error) {
-	logger := slog.Default().With("module", "config")
-
-	home, err := config.ExpandHome(globalFlags.Home)
-	if err != nil {
-		return config.Config{}, errors.Wrap(err, "home")
-	}
-
-	configPath, err := globalFlags.ConfigPath()
-	if err != nil {
-		return config.Config{}, errors.Wrap(err, "unable to get config path")
-	}
-
-	// ephemeral config
-	defaultConfig := config.DefaultConfig()
-
-	// if directory doesn't exist, return default config
-	if err = config.EnsureDirectory(configPath); err != nil {
-		logger.Debug("Config directory does not exist, returning default config", "path", configPath)
-		return defaultConfig, nil
-	}
-
-	if err = os.Chdir(home); err != nil {
-		return config.Config{}, errors.Wrapf(err, "unable to change working directory to %s", home)
-	}
-
-	// if file doesn't exist, return default config
-	cfg, err := config.LoadFromFile(configPath, globalFlags.ValidateConfig(), flagConfigValidateStrict)
-	if err != nil {
-		logger.Debug("Error loading config file, returning default config", "error", err, "path", configPath)
-		return defaultConfig, nil
+	if globalFlags.DB != "" {
+		cfg.DB, err = config.DBConfigFromURL(globalFlags.DB)
+		if err != nil {
+			return config.Config{}, errors.Wrap(err, "invalid --db")
+		}
 	}
 
-	return cfg.OverrideFromFlags(globalFlags)
+	return cfg, nil
 }

link/cmd/ibc/keys.go

@@ -5,6 +5,7 @@ package main
 import (
 	"encoding/base64"
 	"encoding/hex"
+	"errors"
 	"fmt"
 	"os"
 	"path/filepath"
@@ -26,9 +27,6 @@ var (
 	cmdKeys = &cobra.Command{
 		Use:   "keys",
 		Short: "Key management commands",
-		PersistentPreRun: func(_ *cobra.Command, _ []string) {
-			globalFlags.SkipConfigValidation()
-		},
 	}
 
 	cmdKeysNew = &cobra.Command{
@@ -65,11 +63,6 @@ var (
 
 //nolint:goconst // cli usage
 func keysNew(_ *cobra.Command, args []string) error {
-	cfg, err := setupHomeWithOptionalConfig()
-	if err != nil {
-		return err
-	}
-
 	keyType, err := signer.ParseKeyType(args[0])
 	if err != nil {
 		return err
@@ -92,37 +85,18 @@ func keysNew(_ *cobra.Command, args []string) error {
 	}
 
 	keyName := args[1]
-	keyPath, err := signer.KeyFilePath(globalFlags.Home, keyName)
+	keyPath, err := saveNamedKey(key, keyName, flagKeysPopulateConfig)
 	if err != nil {
 		return err
 	}
 
-	if err := key.StoreToFile(keyPath); err != nil {
-		if !os.IsExist(err) {
-			return err
-		}
-
-		return fmt.Errorf("key already exists: %s", keyPath)
-	}
-
-	if flagKeysPopulateConfig {
-		if err := addSignerToConfig(cfg, keyName); err != nil {
-			return err
-		}
-	}
-
 	// note that we don't print the private key here to avoid leaking it to the user
 	return printKey(key, false, map[string]any{
 		"path": keyPath,
 	})
 }
 
 func keysShow(_ *cobra.Command, args []string) error {
-	_, err := setupHomeWithOptionalConfig()
-	if err != nil {
-		return err
-	}
-
 	keyPath, err := signer.KeyFilePath(globalFlags.Home, args[0])
 	if err != nil {
 		return err
@@ -139,11 +113,6 @@ func keysShow(_ *cobra.Command, args []string) error {
 }
 
 func keysList(_ *cobra.Command, _ []string) error {
-	_, err := setupHomeWithOptionalConfig()
-	if err != nil {
-		return err
-	}
-
 	keyPath, err := config.ExpandHome(filepath.Join(globalFlags.Home, "keys"))
 	if err != nil {
 		return err
@@ -168,15 +137,6 @@ func keysList(_ *cobra.Command, _ []string) error {
 }
 
 func keysImport(_ *cobra.Command, args []string) error {
-	cfg, err := setupHomeWithOptionalConfig()
-	if err != nil {
-		return err
-	}
-
-	if flagKeysImportPrivateKey == "" {
-		return fmt.Errorf("--private-key is required")
-	}
-
 	keyType, err := signer.ParseKeyType(args[0])
 	switch {
 	case err != nil:
@@ -186,15 +146,6 @@ func keysImport(_ *cobra.Command, args []string) error {
 	}
 
 	keyName := args[1]
-	keyPath, err := signer.KeyFilePath(globalFlags.Home, keyName)
-	if err != nil {
-		return err
-	}
-
-	if _, err = os.Stat(keyPath); err == nil {
-		return fmt.Errorf("key already exists: %s", keyPath)
-	}
-
 	privateKey, err := signer.DecodeHex(flagKeysImportPrivateKey)
 	if err != nil {
 		return fmt.Errorf("decode private key: %w", err)
@@ -205,47 +156,66 @@ func keysImport(_ *cobra.Command, args []string) error {
 		return fmt.Errorf("create ecdsa key: %w", err)
 	}
 
-	if err := key.StoreToFile(keyPath); err != nil {
-		if os.IsExist(err) {
-			return fmt.Errorf("key already exists: %s", keyPath)
-		}
+	keyPath, err := saveNamedKey(key, keyName, flagKeysPopulateConfig)
+	if err != nil {
 		return err
 	}
 
-	if flagKeysPopulateConfig {
-		if err := addSignerToConfig(cfg, keyName); err != nil {
-			return err
-		}
-	}
-
 	return printKey(key, false, map[string]any{
 		"path": keyPath,
 	})
 }
 
-// addSignerToConfig appends a local signer entry for the given key to the
-// config file, so a freshly created/imported key is immediately usable
-// as a `signers:` alias without a manual edit.
-// note: cfg argument is NOT modified, be careful.
-func addSignerToConfig(cfg config.Config, alias string) error {
-	signer := config.SignerConfig{
-		Alias: alias,
-		Type:  config.SignerLocal,
-		File:  alias,
+func saveNamedKey(key signer.LocalKey, name string, populateConfig bool) (string, error) {
+	keyPath, err := signer.KeyFilePath(globalFlags.Home, name)
+	if err != nil {
+		return "", err
 	}
 
-	cfg, added := cfg.AddSigner(signer)
-	if !added {
-		// already exists, no need to add
-		return nil
+	var (
+		cfg        config.Config
+		configPath string
+	)
+	if populateConfig {
+		configPath, err = globalFlags.ConfigPath()
+		if err != nil {
+			return "", err
+		}
+
+		cfg, err = config.LoadFromFile(configPath, false, false)
+		if os.IsNotExist(err) {
+			cfg = config.DefaultConfig()
+		} else if err != nil {
+			return "", err
+		}
+
+		if _, exists := cfg.Signer(name); exists {
+			return "", fmt.Errorf("signer alias already exists: %q", name)
+		}
 	}
 
-	configPath, err := globalFlags.ConfigPath()
-	if err != nil {
-		return err
+	if err := key.StoreToFile(keyPath); err != nil {
+		if os.IsExist(err) {
+			return "", fmt.Errorf("key already exists: %s", keyPath)
+		}
+		return "", err
+	}
+
+	if !populateConfig {
+		return keyPath, nil
+	}
+
+	cfg.Signers = append(cfg.Signers, config.SignerConfig{
+		Alias: name,
+		Type:  config.SignerLocal,
+		File:  name,
+	})
+
+	if err := cfg.StoreToFileWithComments(configPath); err != nil {
+		return "", errors.Join(err, os.Remove(keyPath))
 	}
 
-	return cfg.StoreToFileWithComments(configPath)
+	return keyPath, nil
 }
 
 func printKey(key signer.LocalKey, showPrivate bool, extra map[string]any) error {

link/cmd/ibc/keys_test.go

@@ -0,0 +1,113 @@
+// SPDX-License-Identifier: Apache-2.0
+
+package main
+
+import (
+	"os"
+	"path/filepath"
+	"testing"
+
+	"github.com/stretchr/testify/require"
+
+	"github.com/cosmos/ibc/link/internal/config"
+	"github.com/cosmos/ibc/link/internal/service/signer"
+)
+
+func TestSaveNamedKeyPopulatesConfig(t *testing.T) {
+	useKeyTestHome(t)
+
+	key, err := signer.GenerateLocalSecp256k1Signer()
+	require.NoError(t, err)
+	keyPath, err := saveNamedKey(key, "alice", true)
+	require.NoError(t, err)
+	require.FileExists(t, keyPath)
+
+	configPath, err := globalFlags.ConfigPath()
+	require.NoError(t, err)
+	cfg, err := config.LoadFromFile(configPath, false, false)
+	require.NoError(t, err)
+	require.Equal(t, config.Signers{{
+		Alias: "alice",
+		Type:  config.SignerLocal,
+		File:  "alice",
+	}}, cfg.Signers)
+}
+
+func TestSaveNamedKeyRejectsExistingSigner(t *testing.T) {
+	useKeyTestHome(t)
+
+	cfg := config.DefaultConfig()
+	cfg.Signers = config.Signers{{
+		Alias:       "alice",
+		Type:        config.SignerRemote,
+		GRPC:        "localhost:3000",
+		RemoteKeyID: "alice",
+	}}
+	configPath, err := globalFlags.ConfigPath()
+	require.NoError(t, err)
+	require.NoError(t, cfg.StoreToFile(configPath))
+
+	key, err := signer.GenerateLocalSecp256k1Signer()
+	require.NoError(t, err)
+	_, err = saveNamedKey(key, "alice", true)
+	require.ErrorContains(t, err, "signer alias already exists")
+
+	keyPath, err := signer.KeyFilePath(globalFlags.Home, "alice")
+	require.NoError(t, err)
+	require.NoFileExists(t, keyPath)
+}
+
+func TestSaveNamedKeyRollsBackAfterConfigWriteFailure(t *testing.T) {
+	home := useKeyTestHome(t)
+
+	configPath, err := globalFlags.ConfigPath()
+	require.NoError(t, err)
+	require.NoError(t, config.DefaultConfig().StoreToFile(configPath))
+	require.NoError(t, os.MkdirAll(filepath.Join(home, "keys"), 0o700))
+	require.NoError(t, os.Chmod(home, 0o500))
+	t.Cleanup(func() { require.NoError(t, os.Chmod(home, 0o700)) })
+
+	if probe, probeErr := os.CreateTemp(home, "probe"); probeErr == nil {
+		_ = probe.Close()
+		_ = os.Remove(probe.Name())
+		t.Skip("filesystem permits writes to a read-only directory")
+	}
+
+	key, err := signer.GenerateLocalSecp256k1Signer()
+	require.NoError(t, err)
+	_, err = saveNamedKey(key, "alice", true)
+	require.Error(t, err)
+
+	keyPath, err := signer.KeyFilePath(home, "alice")
+	require.NoError(t, err)
+	require.NoFileExists(t, keyPath)
+}
+
+func TestSaveNamedKeyRejectsInvalidConfig(t *testing.T) {
+	useKeyTestHome(t)
+
+	configPath, err := globalFlags.ConfigPath()
+	require.NoError(t, err)
+	require.NoError(t, os.WriteFile(configPath, []byte("\tinvalid"), 0o600))
+
+	key, err := signer.GenerateLocalSecp256k1Signer()
+	require.NoError(t, err)
+	_, err = saveNamedKey(key, "alice", true)
+	require.Error(t, err)
+
+	keyPath, err := signer.KeyFilePath(globalFlags.Home, "alice")
+	require.NoError(t, err)
+	require.NoFileExists(t, keyPath)
+}
+
+func useKeyTestHome(t *testing.T) string {
+	t.Helper()
+
+	previousFlags := globalFlags
+	t.Cleanup(func() { globalFlags = previousFlags })
+
+	home := t.TempDir()
+	globalFlags = config.DefaultFlagSet()
+	globalFlags.Home = home
+	return home
+}

link/cmd/ibc/main.go

@@ -82,6 +82,7 @@ func init() {
 	cmdKeys.AddCommand(cmdKeysNew, cmdKeysShow, cmdKeysImport, cmdKeysList)
 	cmdKeysShow.Flags().BoolVarP(&flagKeysShowPrivate, "private", "", false, "show private key")
 	cmdKeysImport.Flags().StringVar(&flagKeysImportPrivateKey, "private-key", "", "hex-encoded private key")
+	_ = cmdKeysImport.MarkFlagRequired("private-key")
 	for _, c := range []*cobra.Command{cmdKeysNew, cmdKeysImport} {
 		c.Flags().BoolVarP(&flagKeysPopulateConfig, "populate-config", "p", false, "write key reference to the config")
 	}