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

Use SignModeLegacyAminoJSON when signer is ledger key #8136

Merged
merged 9 commits into from
Dec 11, 2020
16 changes: 15 additions & 1 deletion client/cmd.go
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import (
rpchttp "github.com/tendermint/tendermint/rpc/client/http"

"github.com/cosmos/cosmos-sdk/client/flags"
"github.com/cosmos/cosmos-sdk/crypto/keyring"
sdk "github.com/cosmos/cosmos-sdk/types"
)

Expand Down Expand Up @@ -215,14 +216,27 @@ func ReadTxCommandFlags(clientCtx Context, flagSet *pflag.FlagSet) (Context, err
clientCtx = clientCtx.WithSkipConfirmation(skipConfirm)
}

if clientCtx.SignModeStr == "" || flagSet.Changed(flags.FlagSignMode) {
signModeStr, _ := flagSet.GetString(flags.FlagSignMode)
clientCtx = clientCtx.WithSignModeStr(signModeStr)
}

if clientCtx.From == "" || flagSet.Changed(flags.FlagFrom) {
from, _ := flagSet.GetString(flags.FlagFrom)
fromAddr, fromName, err := GetFromFields(clientCtx.Keyring, from, clientCtx.GenerateOnly)
fromAddr, fromName, keyType, err := GetFromFields(clientCtx.Keyring, from, clientCtx.GenerateOnly)
if err != nil {
return clientCtx, err
}

clientCtx = clientCtx.WithFrom(from).WithFromAddress(fromAddr).WithFromName(fromName)

// If the `from` signer account is a ledger key, we need to use
// SIGN_MODE_AMINO_JSON, because ledger doesn't support proto yet.
// ref: https://github.com/cosmos/cosmos-sdk/issues/8109
if keyType == keyring.TypeLedger && clientCtx.SignModeStr != flags.SignModeLegacyAminoJSON {
amaury1093 marked this conversation as resolved.
Show resolved Hide resolved
fmt.Println("Default sign-mode 'direct' not supported by Ledger, using sign-mode 'amino-json'.")
clientCtx = clientCtx.WithSignModeStr(flags.SignModeLegacyAminoJSON)
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 could have just flagSet.AddFlag("--sign-mode=amino-json") here, but it feels ugly, so I went with the clientCtx.WithSignModeStr dance.

Copy link
Contributor

Choose a reason for hiding this comment

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

This looks preferred anyway

}
}

return clientCtx, nil
Expand Down
24 changes: 16 additions & 8 deletions client/context.go
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ type Context struct {
From string
BroadcastMode string
FromName string
SignModeStr string
UseLedger bool
Simulate bool
GenerateOnly bool
Expand Down Expand Up @@ -172,6 +173,13 @@ func (ctx Context) WithBroadcastMode(mode string) Context {
return ctx
}

// WithSignModeStr returns a copy of the context with an updated SignMode
// value.
func (ctx Context) WithSignModeStr(signModeStr string) Context {
ctx.SignModeStr = signModeStr
return ctx
}

// WithSkipConfirmation returns a copy of the context with an updated SkipConfirm
// value.
func (ctx Context) WithSkipConfirmation(skip bool) Context {
Expand Down Expand Up @@ -268,37 +276,37 @@ func (ctx Context) printOutput(out []byte) error {
return nil
}

// GetFromFields returns a from account address and Keybase name given either
// GetFromFields returns a from account address, account name and keyring type, given either
// an address or key name. If genOnly is true, only a valid Bech32 cosmos
// address is returned.
func GetFromFields(kr keyring.Keyring, from string, genOnly bool) (sdk.AccAddress, string, error) {
func GetFromFields(kr keyring.Keyring, from string, genOnly bool) (sdk.AccAddress, string, keyring.KeyType, error) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

probably would be better to return (info, error) here. All the return values are info attributes.

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 first reflex too, but not possible: "If genOnly is true, only a valid Bech32 cosmos address is returned.", that case doesn't return the keyring.

if from == "" {
return nil, "", nil
return nil, "", 0, nil
}

if genOnly {
addr, err := sdk.AccAddressFromBech32(from)
if err != nil {
return nil, "", errors.Wrap(err, "must provide a valid Bech32 address in generate-only mode")
return nil, "", 0, errors.Wrap(err, "must provide a valid Bech32 address in generate-only mode")
}

return addr, "", nil
return addr, "", 0, nil
}

var info keyring.Info
if addr, err := sdk.AccAddressFromBech32(from); err == nil {
info, err = kr.KeyByAddress(addr)
if err != nil {
return nil, "", err
return nil, "", 0, err
}
} else {
info, err = kr.Key(from)
if err != nil {
return nil, "", err
return nil, "", 0, err
}
}

return info.GetAddress(), info.GetName(), nil
return info.GetAddress(), info.GetName(), info.GetType(), nil
}

func newKeyringFromFlags(ctx Context, backend string) (keyring.Keyring, error) {
Expand Down
5 changes: 5 additions & 0 deletions client/flags/flags.go
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,11 @@ const (
// BroadcastAsync defines a tx broadcasting mode where the client returns
// immediately.
BroadcastAsync = "async"

// SignModeDirect is the value of the --sign-mode flag for SIGN_MODE_DIRECT
SignModeDirect = "direct"
// SignModeLegacyAminoJSON is the value of the --sign-mode flag for SIGN_MODE_LEGACY_AMINO_JSON
SignModeLegacyAminoJSON = "amino-json"
)

// List of CLI flags
Expand Down
12 changes: 4 additions & 8 deletions client/tx/factory.go
Original file line number Diff line number Diff line change
Expand Up @@ -29,19 +29,15 @@ type Factory struct {
simulateAndExecute bool
}

const (
signModeDirect = "direct"
signModeAminoJSON = "amino-json"
)

// NewFactoryCLI creates a new Factory.
func NewFactoryCLI(clientCtx client.Context, flagSet *pflag.FlagSet) Factory {
signModeStr, _ := flagSet.GetString(flags.FlagSignMode)
signModeStr := clientCtx.SignModeStr

signMode := signing.SignMode_SIGN_MODE_UNSPECIFIED
switch signModeStr {
case signModeDirect:
case flags.SignModeDirect:
signMode = signing.SignMode_SIGN_MODE_DIRECT
case signModeAminoJSON:
case flags.SignModeLegacyAminoJSON:
signMode = signing.SignMode_SIGN_MODE_LEGACY_AMINO_JSON
}

Expand Down
4 changes: 2 additions & 2 deletions x/auth/client/cli/tx_sign.go
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,7 @@ func makeSignBatchCmd() func(cmd *cobra.Command, args []string) error {
}
if multisigAddr.Empty() {
from, _ := cmd.Flags().GetString(flags.FlagFrom)
_, fromName, err := client.GetFromFields(txFactory.Keybase(), from, clientCtx.GenerateOnly)
_, fromName, _, err := client.GetFromFields(txFactory.Keybase(), from, clientCtx.GenerateOnly)
if err != nil {
return fmt.Errorf("error getting account from keybase: %w", err)
}
Expand Down Expand Up @@ -235,7 +235,7 @@ func makeSignCmd() func(cmd *cobra.Command, args []string) error {
multisigAddrStr, _ := cmd.Flags().GetString(flagMultisig)

from, _ := cmd.Flags().GetString(flags.FlagFrom)
_, fromName, err := client.GetFromFields(txFactory.Keybase(), from, clientCtx.GenerateOnly)
_, fromName, _, err := client.GetFromFields(txFactory.Keybase(), from, clientCtx.GenerateOnly)
if err != nil {
return fmt.Errorf("error getting account from keybase: %w", err)
}
Expand Down