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.

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

fix: use keyring in config for add-genesis-account cmd #9969

Merged
merged 12 commits into from
Sep 2, 2021
Merged
30 changes: 15 additions & 15 deletions simapp/simd/cmd/genaccounts.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@ import (

"github.com/cosmos/cosmos-sdk/client"
"github.com/cosmos/cosmos-sdk/client/flags"
"github.com/cosmos/cosmos-sdk/codec"
"github.com/cosmos/cosmos-sdk/crypto/keyring"
"github.com/cosmos/cosmos-sdk/server"
sdk "github.com/cosmos/cosmos-sdk/types"
Expand Down Expand Up @@ -40,30 +39,31 @@ contain valid denominations. Accounts may optionally be supplied with vesting pa
Args: cobra.ExactArgs(2),
RunE: func(cmd *cobra.Command, args []string) error {
clientCtx := client.GetClientContextFromCmd(cmd)
depCdc := clientCtx.Codec
cdc := depCdc.(codec.Codec)

serverCtx := server.GetServerContextFromCmd(cmd)
config := serverCtx.Config

config.SetRoot(clientCtx.HomeDir)

var kr keyring.Keyring
technicallyty marked this conversation as resolved.
Show resolved Hide resolved
addr, err := sdk.AccAddressFromBech32(args[0])
if err != nil {
inBuf := bufio.NewReader(cmd.InOrStdin())
keyringBackend, _ := cmd.Flags().GetString(flags.FlagKeyringBackend)

// attempt to lookup address from Keybase if no address was provided
kb, err := keyring.New(sdk.KeyringServiceName(), keyringBackend, clientCtx.HomeDir, inBuf)
if err != nil {
return err
if keyringBackend != "" && clientCtx.Keyring == nil {
var err error
kr, err = keyring.New(sdk.KeyringServiceName(), keyringBackend, clientCtx.HomeDir, inBuf)
if err != nil {
return err
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

shouldn't we set:

clientCtx.Keyring = kr

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hmm im not sure i like the idea of overwriting clientCtx values

Copy link
Collaborator

Choose a reason for hiding this comment

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

What if for some reason we will use clientCtx.Keyring down the line?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the keyring in clientCtx comes from the user setting it in their config - if it needs to be used down the line i think it should be explicitly set by the user

} else {
kr = clientCtx.Keyring
}

info, err := kb.Key(args[0])
info, err := kr.Key(args[0])
if err != nil {
return fmt.Errorf("failed to get address from Keybase: %w", err)
return fmt.Errorf("failed to get address from Keyring: %w", err)
}

addr = info.GetAddress()
}

Expand Down Expand Up @@ -119,7 +119,7 @@ contain valid denominations. Accounts may optionally be supplied with vesting pa
return fmt.Errorf("failed to unmarshal genesis state: %w", err)
}

authGenState := authtypes.GetGenesisStateFromAppState(cdc, appState)
authGenState := authtypes.GetGenesisStateFromAppState(clientCtx.Codec, appState)

accs, err := authtypes.UnpackAccounts(authGenState.Accounts)
if err != nil {
Expand All @@ -141,19 +141,19 @@ contain valid denominations. Accounts may optionally be supplied with vesting pa
}
authGenState.Accounts = genAccs

authGenStateBz, err := cdc.MarshalJSON(&authGenState)
authGenStateBz, err := clientCtx.Codec.MarshalJSON(&authGenState)
if err != nil {
return fmt.Errorf("failed to marshal auth genesis state: %w", err)
}

appState[authtypes.ModuleName] = authGenStateBz

bankGenState := banktypes.GetGenesisStateFromAppState(depCdc, appState)
bankGenState := banktypes.GetGenesisStateFromAppState(clientCtx.Codec, appState)
bankGenState.Balances = append(bankGenState.Balances, balances)
bankGenState.Balances = banktypes.SanitizeGenesisBalances(bankGenState.Balances)
bankGenState.Supply = bankGenState.Supply.Add(balances.Coins...)

bankGenStateBz, err := cdc.MarshalJSON(bankGenState)
bankGenStateBz, err := clientCtx.Codec.MarshalJSON(bankGenState)
if err != nil {
return fmt.Errorf("failed to marshal bank genesis state: %w", err)
}
Expand Down
55 changes: 39 additions & 16 deletions simapp/simd/cmd/genaccounts_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,9 @@ package cmd_test
import (
"context"
"fmt"
"github.com/cosmos/cosmos-sdk/crypto/hd"
"github.com/cosmos/cosmos-sdk/crypto/keyring"
sdk "github.com/cosmos/cosmos-sdk/types"
"testing"

"github.com/spf13/viper"
Expand All @@ -25,28 +28,39 @@ var testMbm = module.NewBasicManager(genutil.AppModuleBasic{})
func TestAddGenesisAccountCmd(t *testing.T) {
_, _, addr1 := testdata.KeyTestPubAddr()
tests := []struct {
name string
addr string
denom string
expectErr bool
name string
addr string
denom string
withKeyring bool
Copy link
Collaborator

Choose a reason for hiding this comment

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

can we always setup the keyring in the context and remove the parameter from here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no - it would cause the invalid address case to succeed when we want failure

expectErr bool
}{
{
name: "invalid address",
addr: "",
denom: "1000atom",
expectErr: true,
name: "invalid address",
addr: "",
denom: "1000atom",
withKeyring: false,
expectErr: true,
},
{
name: "valid address",
addr: addr1.String(),
denom: "1000atom",
expectErr: false,
name: "valid address",
addr: addr1.String(),
denom: "1000atom",
withKeyring: false,
expectErr: false,
},
{
name: "multiple denoms",
addr: addr1.String(),
denom: "1000atom, 2000stake",
expectErr: false,
name: "multiple denoms",
addr: addr1.String(),
denom: "1000atom, 2000stake",
withKeyring: false,
expectErr: false,
},
{
name: "with keyring",
addr: "ser",
denom: "1000atom",
withKeyring: true,
expectErr: false,
},
}

Expand All @@ -65,6 +79,15 @@ func TestAddGenesisAccountCmd(t *testing.T) {
serverCtx := server.NewContext(viper.New(), cfg, logger)
clientCtx := client.Context{}.WithCodec(appCodec).WithHomeDir(home)

if tc.withKeyring {
path := hd.CreateHDPath(118, 0, 0).String()
kr, err := keyring.New(sdk.KeyringServiceName(), keyring.BackendMemory, home, nil)
require.NoError(t, err)
_, _, err = kr.NewMnemonic(tc.addr, keyring.English, path, keyring.DefaultBIP39Passphrase, hd.Secp256k1)
require.NoError(t, err)
clientCtx = clientCtx.WithKeyring(kr)
}

ctx := context.Background()
ctx = context.WithValue(ctx, client.ClientContextKey, &clientCtx)
ctx = context.WithValue(ctx, server.ServerContextKey, serverCtx)
Expand Down