Update keys add cmd patch (#477)

* update patch

* add tests for patch

cli_test/cli_test.go

@@ -92,6 +92,18 @@ func TestKavaCLIKeysAddRecoverHDPath(t *testing.T) {
 	f.KeysAddRecoverHDPath("test-recoverH5", "dentist task convince chimney quality leave banana trade firm crawl eternal easily", 2, 17, "--legacy-hd-path")
 	require.Equal(t, "kava1v9plmhvyhgxk3th9ydacm7j4z357s3nhhmy0tv", f.KeyAddress("test-recoverH5").String())
 
+	exitSuccess, _, _ := f.KeysAddRecover("test-recover-fail", "dentist task convince chimney quality leave banana trade firm crawl eternal easily", "--legacy-hd-path --hd-path 44'/459'/0'/0/0")
+	require.False(t, exitSuccess)
+
+	// test -hd-path flag
+	exitSuccess, _, _ = f.KeysAddRecover("test-recoverH6", "dentist task convince chimney quality leave banana trade firm crawl eternal easily", "--hd-path 44'/459'/0'/0/0")
+	require.True(t, exitSuccess)
+	require.Equal(t, "kava1rsjxn2e4dfl3a2qzuzzjvvgjmmate383g9q4cz", f.KeyAddress("test-recoverH6").String())
+
+	exitSuccess, _, _ = f.KeysAddRecover("test-recoverH7", "dentist task convince chimney quality leave banana trade firm crawl eternal easily", "--hd-path 44'/459'/2'/0/17")
+	require.True(t, exitSuccess)
+	require.Equal(t, "kava1xvsfnksmhr887skcfrm4pe3va54tkmrtw7wyer", f.KeyAddress("test-recoverH7").String())
+
 	// Cleanup testing directories
 	f.Cleanup()
 }

cmd/kvcli/cmd_keys.go

@@ -17,18 +17,28 @@ import (
 NOTE TO FUTURE IMPLEMENTERS
 This monkey patches the sdk `keys` command, therefore needs to be reviewed on any sdk updates.
 
-Where a bip44 coin type is used (cosmos-sdk 18de630d):
+The patch adds support for using kava's legacy bip44 coin type to the cli.
-- adding local keys
+Coin types are used to create a bip44 derivation path, which is used as a mapping from mnemonic to private key.
-	- global variable `sdk.Config.CoinType` is used to derive the key from a mnemonic (supplied by user or generated), but only the private key is stored
+
-- adding ledger keys
+In cosmos-sdk v0.38.3, all private keys are stored without reference to the mnemonic or bip44 derivation path, except ledger keys.
-	- global variable `sdk.Config.CoinType` is used to reference a key on a ledger device, bip44 path (not private key) is stored locally
+Ledger keys are just references to a private key on a ledger device. They contain the bip44 derivation path.
-- signing txs with local keys
+To patch the cli, we only need to modify:
+- when new ledger references are created
+- anything to do with converting a mnemonic to a private key.
+
+These only happen in `kvcli keys add` cmd.
+For private key generation, use a --legacy-hd-path flag to enable old coin type.
+The current cosmos ledger app (v1.5.3) only supports the legacy coin type. So we only need to ensure ledger reference creation doesn't use the new coin type.
+
+Signing txs:
+- with local keys
 	- the stored the priv key is used to sign, mnemonics or bip44 paths not involved
-- signing txs with ledger
+- with ledger
 	- the stored bip44 path is used to instruct the ledger which key to sign with
 */
 
 const flagLegacyHDPath = "legacy-hd-path"
+const flagHDPath = "hd-path" // this is copied from keys add cmd because it's not exported
 
 // getModifiedKeysCmd returns the standard cosmos-sdk/client/keys cmd but modified to support new and old bip44 coin types supported by kava.
 func getModifiedKeysCmd() *cobra.Command {
@@ -70,18 +80,20 @@ func monkeyPatchCmdKeysAdd(keysAddCmd *cobra.Command) {
 	// replace the run function with a wrapped version that sets the old coin type in the global config
 	oldRun := keysAddCmd.RunE
 	keysAddCmd.RunE = func(cmd *cobra.Command, args []string) error {
-		preExistingCoinType := sdk.GetConfig().GetCoinType()
 
+		if !viper.GetBool(flagLegacyHDPath) && viper.GetBool(flags.FlagUseLedger) {
+			return fmt.Errorf("cosmos ledger app only supports legacy bip44 coin type, must use --%s flag when adding ledger key", flagLegacyHDPath)
+		}
+		if viper.GetBool(flagLegacyHDPath) && viper.IsSet(flagHDPath) {
+			return fmt.Errorf("cannot use a custom hd path (--%s) and legacy bip44 coin type (--%s) at the same time", flagHDPath, flagLegacyHDPath)
+		}
 		if viper.GetBool(flagLegacyHDPath) {
+			preExistingCoinType := sdk.GetConfig().GetCoinType()
 			sdk.GetConfig().SetCoinType(sdk.CoinType) // set old coin type
 			err := oldRun(cmd, args)
-			sdk.GetConfig().SetCoinType(preExistingCoinType) // revert to preexisting coin type
+			sdk.GetConfig().SetCoinType(preExistingCoinType) // revert to pre-existing coin type
 			return err
-		} else {
-			if viper.GetBool(flags.FlagUseLedger) {
-				return fmt.Errorf("cosmos ledger app only supports legacy bip44 coin type, must use --%s flag when adding ledger key", flagLegacyHDPath)
 		}
 		return oldRun(cmd, args)
 	}
-	}
 }