Skip to content

Commit

Permalink
[Backport] Add ledger/multisig detection in SignTx functions (#9026) (#…
Browse files Browse the repository at this point in the history 
…9041)

Co-authored-by: Alessio Treglia <alessio@tendermint.com>
  • Loading branch information
@amaury1093
amaury1093 and Alessio Treglia authored Apr 1, 2021
1 parent 7648bfc commit 1e03826
Show file tree
Hide file tree
Showing 3 changed files with 31 additions and 22 deletions.
22 changes: 12 additions & 10 deletions x/auth/client/cli/cli_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -132,14 +132,15 @@ func (s *IntegrationTestSuite) TestCLISignBatch() {
s.Require().Error(err)
}

func (s *IntegrationTestSuite) TestCLISign() {
func (s *IntegrationTestSuite) TestCLISign_AminoJSON() {
require := s.Require()
val1 := s.network.Validators[0]
txCfg := val1.ClientCtx.TxConfig
txBz := s.createBankMsg(val1, val1.Address)
fileUnsigned := testutil.WriteToNewTempFile(s.T(), txBz.String())
chainFlag := fmt.Sprintf("--%s=%s", flags.FlagChainID, val1.ClientCtx.ChainID)
sigOnlyFlag := "--signature-only"
signModeAminoFlag := "--sign-mode=amino-json"

// SIC! validators have same key names and same adddresses as those registered in the keyring,
// BUT the keys are different!
Expand All @@ -154,7 +155,7 @@ func (s *IntegrationTestSuite) TestCLISign() {

/**** test signature-only ****/
res, err := authtest.TxSignExec(val1.ClientCtx, val1.Address, fileUnsigned.Name(), chainFlag,
sigOnlyFlag)
sigOnlyFlag, signModeAminoFlag)
require.NoError(err)
checkSignatures(require, txCfg, res.Bytes(), valInfo.GetPubKey())
sigs, err := txCfg.UnmarshalSignatureJSON(res.Bytes())
Expand All @@ -163,7 +164,7 @@ func (s *IntegrationTestSuite) TestCLISign() {
require.Equal(account.GetSequence(), sigs[0].Sequence)

/**** test full output ****/
res, err = authtest.TxSignExec(val1.ClientCtx, val1.Address, fileUnsigned.Name(), chainFlag)
res, err = authtest.TxSignExec(val1.ClientCtx, val1.Address, fileUnsigned.Name(), chainFlag, signModeAminoFlag)
require.NoError(err)

// txCfg.UnmarshalSignatureJSON can't unmarshal a fragment of the signature, so we create this structure.
Expand All @@ -178,15 +179,15 @@ func (s *IntegrationTestSuite) TestCLISign() {
/**** test file output ****/
filenameSigned := filepath.Join(s.T().TempDir(), "test_sign_out.json")
fileFlag := fmt.Sprintf("--%s=%s", flags.FlagOutputDocument, filenameSigned)
_, err = authtest.TxSignExec(val1.ClientCtx, val1.Address, fileUnsigned.Name(), chainFlag, fileFlag)
_, err = authtest.TxSignExec(val1.ClientCtx, val1.Address, fileUnsigned.Name(), chainFlag, fileFlag, signModeAminoFlag)
require.NoError(err)
fContent, err := ioutil.ReadFile(filenameSigned)
require.NoError(err)
require.Equal(res.String(), string(fContent))

/**** try to append to the previously signed transaction ****/
res, err = authtest.TxSignExec(val1.ClientCtx, val1.Address, filenameSigned, chainFlag,
sigOnlyFlag)
sigOnlyFlag, signModeAminoFlag)
require.NoError(err)
checkSignatures(require, txCfg, res.Bytes(), valInfo.GetPubKey(), valInfo.GetPubKey())

Expand All @@ -197,12 +198,12 @@ func (s *IntegrationTestSuite) TestCLISign() {
// provide functionality to check / manage `auth_info`.
// Cases with different keys are are covered in unit tests of `tx.Sign`.
res, err = authtest.TxSignExec(val1.ClientCtx, val1.Address, filenameSigned, chainFlag,
sigOnlyFlag, "--overwrite")
sigOnlyFlag, "--overwrite", signModeAminoFlag)
checkSignatures(require, txCfg, res.Bytes(), valInfo.GetPubKey())

/**** test flagAmino ****/
res, err = authtest.TxSignExec(val1.ClientCtx, val1.Address, filenameSigned, chainFlag,
"--amino=true")
"--amino=true", signModeAminoFlag)
require.NoError(err)

var txAmino authrest.BroadcastReq
Expand Down Expand Up @@ -1087,7 +1088,8 @@ func (s *IntegrationTestSuite) TestSignWithMultiSigners_AminoJSON() {
_, _, addr1 := testdata.KeyTestPubAddr()

// Creating a tx with 2 msgs from 2 signers: val0 and val1.
// The validators sign with SIGN_MODE_LEGACY_AMINO_JSON by default.
// The validators need to sign with SIGN_MODE_LEGACY_AMINO_JSON,
// because DIRECT doesn't support multi signers via the CLI.
// Since we we amino, we don't need to pre-populate signer_infos.
txBuilder := val0.ClientCtx.TxConfig.NewTxBuilder()
txBuilder.SetMsgs(
Expand All @@ -1104,7 +1106,7 @@ func (s *IntegrationTestSuite) TestSignWithMultiSigners_AminoJSON() {
unsignedTxFile := testutil.WriteToNewTempFile(s.T(), string(txJSON))

// Let val0 sign first the file with the unsignedTx.
signedByVal0, err := authtest.TxSignExec(val0.ClientCtx, val0.Address, unsignedTxFile.Name(), "--overwrite")
signedByVal0, err := authtest.TxSignExec(val0.ClientCtx, val0.Address, unsignedTxFile.Name(), "--overwrite", "--sign-mode=amino-json")
require.NoError(err)
signedByVal0File := testutil.WriteToNewTempFile(s.T(), signedByVal0.String())

Expand All @@ -1113,7 +1115,7 @@ func (s *IntegrationTestSuite) TestSignWithMultiSigners_AminoJSON() {
require.NoError(err)
signedTx, err := authtest.TxSignExec(
val1.ClientCtx, val1.Address, signedByVal0File.Name(),
"--offline", fmt.Sprintf("--account-number=%d", val1AccNum), fmt.Sprintf("--sequence=%d", val1Seq),
"--offline", fmt.Sprintf("--account-number=%d", val1AccNum), fmt.Sprintf("--sequence=%d", val1Seq), "--sign-mode=amino-json",
)
require.NoError(err)
signedTxFile := testutil.WriteToNewTempFile(s.T(), signedTx.String())
Expand Down
10 changes: 1 addition & 9 deletions x/auth/client/cli/tx_sign.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@ import (
"github.com/cosmos/cosmos-sdk/client/flags"
"github.com/cosmos/cosmos-sdk/client/tx"
sdk "github.com/cosmos/cosmos-sdk/types"
"github.com/cosmos/cosmos-sdk/types/tx/signing"
authclient "github.com/cosmos/cosmos-sdk/x/auth/client"
"github.com/cosmos/cosmos-sdk/x/auth/client/rest"
)
Expand Down Expand Up @@ -114,9 +113,6 @@ func makeSignBatchCmd() func(cmd *cobra.Command, args []string) error {
return err
}
} else {
if txFactory.SignMode() == signing.SignMode_SIGN_MODE_UNSPECIFIED {
txFactory = txFactory.WithSignMode(signing.SignMode_SIGN_MODE_LEGACY_AMINO_JSON)
}
err = authclient.SignTxWithSignerAddress(
txFactory, clientCtx, multisigAddr, clientCtx.GetFromName(), txBuilder, clientCtx.Offline, true)
}
Expand Down Expand Up @@ -212,15 +208,11 @@ func makeSignCmd() func(cmd *cobra.Command, args []string) error {
return err
}
f := cmd.Flags()
txFactory := tx.NewFactoryCLI(clientCtx, f)

clientCtx, txF, newTx, err := readTxAndInitContexts(clientCtx, cmd, args[0])
if err != nil {
return err
}
if txF.SignMode() == signing.SignMode_SIGN_MODE_UNSPECIFIED {
txF = txF.WithSignMode(signing.SignMode_SIGN_MODE_LEGACY_AMINO_JSON)
}
txCfg := clientCtx.TxConfig
txBuilder, err := txCfg.WrapTxBuilder(newTx)
if err != nil {
Expand All @@ -230,7 +222,7 @@ func makeSignCmd() func(cmd *cobra.Command, args []string) error {
printSignatureOnly, _ := cmd.Flags().GetBool(flagSigOnly)
multisigAddrStr, _ := cmd.Flags().GetString(flagMultisig)
from, _ := cmd.Flags().GetString(flags.FlagFrom)
_, fromName, _, err := client.GetFromFields(txFactory.Keybase(), from, clientCtx.GenerateOnly)
_, fromName, _, err := client.GetFromFields(txF.Keybase(), from, clientCtx.GenerateOnly)
if err != nil {
return fmt.Errorf("error getting account from keybase: %w", err)
}
Expand Down
21 changes: 18 additions & 3 deletions x/auth/client/tx.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,10 @@ import (
"github.com/cosmos/cosmos-sdk/client"
"github.com/cosmos/cosmos-sdk/client/tx"
"github.com/cosmos/cosmos-sdk/codec"
"github.com/cosmos/cosmos-sdk/crypto/keyring"
sdk "github.com/cosmos/cosmos-sdk/types"
sdkerrors "github.com/cosmos/cosmos-sdk/types/errors"
"github.com/cosmos/cosmos-sdk/types/tx/signing"
"github.com/cosmos/cosmos-sdk/x/auth/legacy/legacytx"
)

Expand Down Expand Up @@ -46,13 +48,20 @@ func PrintUnsignedStdTx(txBldr tx.Factory, clientCtx client.Context, msgs []sdk.
// SignTx signs a transaction managed by the TxBuilder using a `name` key stored in Keybase.
// The new signature is appended to the TxBuilder when overwrite=false or overwritten otherwise.
// Don't perform online validation or lookups if offline is true.
func SignTx(txFactory tx.Factory, clientCtx client.Context, name string, stdTx client.TxBuilder, offline, overwriteSig bool) error {
func SignTx(txFactory tx.Factory, clientCtx client.Context, name string, txBuilder client.TxBuilder, offline, overwriteSig bool) error {
info, err := txFactory.Keybase().Key(name)
if err != nil {
return err
}

// Ledger and Multisigs only support LEGACY_AMINO_JSON signing.
if txFactory.SignMode() == signing.SignMode_SIGN_MODE_UNSPECIFIED &&
(info.GetType() == keyring.TypeLedger || info.GetType() == keyring.TypeMulti) {
txFactory = txFactory.WithSignMode(signing.SignMode_SIGN_MODE_LEGACY_AMINO_JSON)
}

addr := sdk.AccAddress(info.GetPubKey().Address())
if !isTxSigner(addr, stdTx.GetTx().GetSigners()) {
if !isTxSigner(addr, txBuilder.GetTx().GetSigners()) {
return fmt.Errorf("%s: %s", sdkerrors.ErrorInvalidSigner, name)
}
if !offline {
Expand All @@ -62,14 +71,20 @@ func SignTx(txFactory tx.Factory, clientCtx client.Context, name string, stdTx c
}
}

return tx.Sign(txFactory, name, stdTx, overwriteSig)
return tx.Sign(txFactory, name, txBuilder, overwriteSig)
}

// SignTxWithSignerAddress attaches a signature to a transaction.
// Don't perform online validation or lookups if offline is true, else
// populate account and sequence numbers from a foreign account.
// This function should only be used when signing with a multisig. For
// normal keys, please use SignTx directly.
func SignTxWithSignerAddress(txFactory tx.Factory, clientCtx client.Context, addr sdk.AccAddress,
name string, txBuilder client.TxBuilder, offline, overwrite bool) (err error) {
// Multisigs only support LEGACY_AMINO_JSON signing.
if txFactory.SignMode() == signing.SignMode_SIGN_MODE_UNSPECIFIED {
txFactory = txFactory.WithSignMode(signing.SignMode_SIGN_MODE_LEGACY_AMINO_JSON)
}

// check whether the address is a signer
if !isTxSigner(addr, txBuilder.GetTx().GetSigners()) {
Expand Down

0 comments on commit 1e03826

Please sign in to comment.