From 49c5d8b8b1f13377112cfe1495ed99db8680a39d Mon Sep 17 00:00:00 2001 From: Amaury M <1293565+amaurym@users.noreply.github.com> Date: Mon, 6 Dec 2021 15:51:15 +0100 Subject: [PATCH 1/8] fix(cli): Allow max 1 DIRECT signer per tx --- client/tx/tx.go | 61 +++++++++++++++++++++++++++++++++++++------- client/tx/tx_test.go | 17 +++++++----- 2 files changed, 62 insertions(+), 16 deletions(-) diff --git a/client/tx/tx.go b/client/tx/tx.go index db787a774c51..f950a166bbea 100644 --- a/client/tx/tx.go +++ b/client/tx/tx.go @@ -14,7 +14,6 @@ import ( "github.com/cosmos/cosmos-sdk/client/input" cryptotypes "github.com/cosmos/cosmos-sdk/crypto/types" sdk "github.com/cosmos/cosmos-sdk/types" - sdkerrors "github.com/cosmos/cosmos-sdk/types/errors" "github.com/cosmos/cosmos-sdk/types/tx" "github.com/cosmos/cosmos-sdk/types/tx/signing" authsigning "github.com/cosmos/cosmos-sdk/x/auth/signing" @@ -170,11 +169,46 @@ func SignWithPrivKey( return sigV2, nil } -func checkMultipleSigners(mode signing.SignMode, tx authsigning.Tx) error { - if mode == signing.SignMode_SIGN_MODE_DIRECT && - len(tx.GetSigners()) > 1 { - return sdkerrors.Wrap(sdkerrors.ErrNotSupported, "Signing in DIRECT mode is only supported for transactions with one signer only") +// countDirectSigners counts the number of DIRECT signers in a signature data. +func countDirectSigners(data signing.SignatureData) int { + switch data := data.(type) { + case *signing.SingleSignatureData: + { + if data.SignMode == signing.SignMode_SIGN_MODE_DIRECT { + return 1 + } + + return 0 + } + case *signing.MultiSignatureData: + { + directSigners := 0 + for _, d := range data.Signatures { + directSigners += countDirectSigners(d) + } + + return directSigners + } + } + + panic("unreachable case") +} + +// checkMultipleSigners checks that there can be maximum one DIRECT signer in +// a tx. +func checkMultipleSigners(tx authsigning.Tx) error { + directSigners := 0 + sigsV2, err := tx.GetSignaturesV2() + if err != nil { + return err + } + for _, sig := range sigsV2 { + directSigners += countDirectSigners(sig.Data) + if directSigners > 1 { + return fmt.Errorf("txs signed with CLI can have maximum 1 DIRECT signer") + } } + return nil } @@ -194,9 +228,6 @@ func Sign(txf Factory, name string, txBuilder client.TxBuilder, overwriteSig boo // use the SignModeHandler's default mode if unspecified signMode = txf.txConfig.SignModeHandler().DefaultMode() } - if err := checkMultipleSigners(signMode, txBuilder.GetTx()); err != nil { - return err - } k, err := txf.keybase.Key(name) if err != nil { @@ -246,6 +277,7 @@ func Sign(txf Factory, name string, txBuilder client.TxBuilder, overwriteSig boo Data: &sigData, Sequence: txf.Sequence(), } + var prevSignatures []signing.SignatureV2 if !overwriteSig { prevSignatures, err = txBuilder.GetTx().GetSignaturesV2() @@ -253,7 +285,18 @@ func Sign(txf Factory, name string, txBuilder client.TxBuilder, overwriteSig boo return err } } - if err := txBuilder.SetSignatures(sig); err != nil { + // Overwrite or append signer infos. + var sigs []signing.SignatureV2 + if overwriteSig { + sigs = []signing.SignatureV2{sig} + } else { + sigs = append(prevSignatures, sig) + } + if err := txBuilder.SetSignatures(sigs...); err != nil { + return err + } + + if err := checkMultipleSigners(txBuilder.GetTx()); err != nil { return err } diff --git a/client/tx/tx_test.go b/client/tx/tx_test.go index 8c3ed0ddf07a..ae1e73c3bbb5 100644 --- a/client/tx/tx_test.go +++ b/client/tx/tx_test.go @@ -210,13 +210,16 @@ func TestSign(t *testing.T) { txfAmino, txb, from2, true, []cryptotypes.PubKey{pubKey2}, []int{1, 0}}, /**** test double sign Direct mode - signing transaction with more than 2 signers should fail in DIRECT mode ****/ - {"direct: should fail to append a signature with different mode", - txfDirect, txb, from1, false, []cryptotypes.PubKey{}, nil}, - {"direct: should fail to sign multi-signers tx", - txfDirect, txb2, from1, false, []cryptotypes.PubKey{}, nil}, - {"direct: should fail to overwrite multi-signers tx", - txfDirect, txb2, from1, true, []cryptotypes.PubKey{}, nil}, + signing transaction with 2 or mjore DIRECT signers should fail in DIRECT mode ****/ + {"direct: should append a DIRECT signature with existing AMINO", + // Note: txb already has 1 AMINO signature + txfDirect, txb, from1, false, []cryptotypes.PubKey{pubKey2, pubKey1}, nil}, + {"direct: should add single DIRECT sig in multi-signers tx", + txfDirect, txb2, from1, false, []cryptotypes.PubKey{pubKey1}, nil}, + {"direct: should fail to append 2nd DIRECT sig in multi-signers tx", + txfDirect, txb2, from2, false, []cryptotypes.PubKey{}, nil}, + {"direct: should overwrite multi-signers tx with DIRECT sig", + txfDirect, txb2, from1, true, []cryptotypes.PubKey{pubKey1}, nil}, } var prevSigs []signingtypes.SignatureV2 for _, tc := range testCases { From b71da46a754818a25815c16f6d80fe07f14c00ca Mon Sep 17 00:00:00 2001 From: Amaury M <1293565+amaurym@users.noreply.github.com> Date: Mon, 6 Dec 2021 15:54:13 +0100 Subject: [PATCH 2/8] Use sdkerrors --- client/tx/tx.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/client/tx/tx.go b/client/tx/tx.go index f950a166bbea..ae3c5e2f6a18 100644 --- a/client/tx/tx.go +++ b/client/tx/tx.go @@ -14,6 +14,7 @@ import ( "github.com/cosmos/cosmos-sdk/client/input" cryptotypes "github.com/cosmos/cosmos-sdk/crypto/types" sdk "github.com/cosmos/cosmos-sdk/types" + sdkerrors "github.com/cosmos/cosmos-sdk/types/errors" "github.com/cosmos/cosmos-sdk/types/tx" "github.com/cosmos/cosmos-sdk/types/tx/signing" authsigning "github.com/cosmos/cosmos-sdk/x/auth/signing" @@ -205,7 +206,7 @@ func checkMultipleSigners(tx authsigning.Tx) error { for _, sig := range sigsV2 { directSigners += countDirectSigners(sig.Data) if directSigners > 1 { - return fmt.Errorf("txs signed with CLI can have maximum 1 DIRECT signer") + return sdkerrors.ErrNotSupported.Wrap("txs signed with CLI can have maximum 1 DIRECT signer") } } From 15ecf8536e8ad5f6b4684c9452888803edaccee6 Mon Sep 17 00:00:00 2001 From: Amaury <1293565+amaurym@users.noreply.github.com> Date: Mon, 6 Dec 2021 15:56:12 +0100 Subject: [PATCH 3/8] Update client/tx/tx_test.go --- client/tx/tx_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/client/tx/tx_test.go b/client/tx/tx_test.go index ae1e73c3bbb5..37a8b0849887 100644 --- a/client/tx/tx_test.go +++ b/client/tx/tx_test.go @@ -210,7 +210,7 @@ func TestSign(t *testing.T) { txfAmino, txb, from2, true, []cryptotypes.PubKey{pubKey2}, []int{1, 0}}, /**** test double sign Direct mode - signing transaction with 2 or mjore DIRECT signers should fail in DIRECT mode ****/ + signing transaction with 2 or more DIRECT signers should fail in DIRECT mode ****/ {"direct: should append a DIRECT signature with existing AMINO", // Note: txb already has 1 AMINO signature txfDirect, txb, from1, false, []cryptotypes.PubKey{pubKey2, pubKey1}, nil}, From 710bee577250f281d1da31eb6968a10a5355d38b Mon Sep 17 00:00:00 2001 From: Amaury M <1293565+amaurym@users.noreply.github.com> Date: Mon, 6 Dec 2021 15:58:08 +0100 Subject: [PATCH 4/8] add more test --- client/tx/tx_test.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/client/tx/tx_test.go b/client/tx/tx_test.go index 37a8b0849887..a05c9310bd48 100644 --- a/client/tx/tx_test.go +++ b/client/tx/tx_test.go @@ -212,12 +212,15 @@ func TestSign(t *testing.T) { /**** test double sign Direct mode signing transaction with 2 or more DIRECT signers should fail in DIRECT mode ****/ {"direct: should append a DIRECT signature with existing AMINO", - // Note: txb already has 1 AMINO signature + // txb already has 1 AMINO signature txfDirect, txb, from1, false, []cryptotypes.PubKey{pubKey2, pubKey1}, nil}, {"direct: should add single DIRECT sig in multi-signers tx", txfDirect, txb2, from1, false, []cryptotypes.PubKey{pubKey1}, nil}, {"direct: should fail to append 2nd DIRECT sig in multi-signers tx", txfDirect, txb2, from2, false, []cryptotypes.PubKey{}, nil}, + {"amino: should append 2nd AMINO sig in multi-signers tx with 1 DIRECT sig", + // txb2 already has 1 DIRECT signature + txfAmino, txb2, from2, false, []cryptotypes.PubKey{}, nil}, {"direct: should overwrite multi-signers tx with DIRECT sig", txfDirect, txb2, from1, true, []cryptotypes.PubKey{pubKey1}, nil}, } From 409d0f8614893c25c1435579af9b777d316002ec Mon Sep 17 00:00:00 2001 From: Amaury M <1293565+amaurym@users.noreply.github.com> Date: Mon, 6 Dec 2021 16:05:56 +0100 Subject: [PATCH 5/8] cl --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 90673485c90a..7294176ad9c2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -141,6 +141,7 @@ Ref: https://keepachangelog.com/en/1.0.0/ * [\#10341](https://github.com/cosmos/cosmos-sdk/pull/10341) Move from `io/ioutil` to `io` and `os` packages. * [\#10468](https://github.com/cosmos/cosmos-sdk/pull/10468) Allow futureOps to queue additional operations in simulations * [\#10625](https://github.com/cosmos/cosmos-sdk/pull/10625) Add `--fee-payer` CLI flag +* (cli) [\#10683](https://github.com/cosmos/cosmos-sdk/pull/10683) In CLI, allow 1 SIGN_MODE_DIRECT signer in transactions with multiple signers. ### Bug Fixes From ec391dd55e5acaaff1037742a95580c13126c168 Mon Sep 17 00:00:00 2001 From: Amaury M <1293565+amaurym@users.noreply.github.com> Date: Mon, 6 Dec 2021 16:18:50 +0100 Subject: [PATCH 6/8] Fix test --- x/auth/client/testutil/suite.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/x/auth/client/testutil/suite.go b/x/auth/client/testutil/suite.go index b7e527271bbe..1e48d1c8d232 100644 --- a/x/auth/client/testutil/suite.go +++ b/x/auth/client/testutil/suite.go @@ -33,6 +33,7 @@ import ( bank "github.com/cosmos/cosmos-sdk/x/bank/client/cli" bankcli "github.com/cosmos/cosmos-sdk/x/bank/client/testutil" banktypes "github.com/cosmos/cosmos-sdk/x/bank/types" + "github.com/cosmos/cosmos-sdk/x/genutil/client/cli" ) type IntegrationTestSuite struct { @@ -1360,11 +1361,12 @@ func (s *IntegrationTestSuite) TestTxWithoutPublicKey() { // Create a file with the unsigned tx. txJSON, err := txCfg.TxJSONEncoder()(txBuilder.GetTx()) + fmt.Println(string(txJSON)) s.Require().NoError(err) unsignedTxFile := testutil.WriteToNewTempFile(s.T(), string(txJSON)) // Sign the file with the unsignedTx. - signedTx, err := TxSignExec(val1.ClientCtx, val1.Address, unsignedTxFile.Name()) + signedTx, err := TxSignExec(val1.ClientCtx, val1.Address, unsignedTxFile.Name(), fmt.Sprintf("--%s=true", cli.FlagOverwrite)) s.Require().NoError(err) // Remove the signerInfo's `public_key` field manually from the signedTx. From a1844eee9caf5fea8050bbe80c6f3e3abfad7636 Mon Sep 17 00:00:00 2001 From: Amaury <1293565+amaurym@users.noreply.github.com> Date: Mon, 6 Dec 2021 16:25:06 +0100 Subject: [PATCH 7/8] Update x/auth/client/testutil/suite.go --- x/auth/client/testutil/suite.go | 1 - 1 file changed, 1 deletion(-) diff --git a/x/auth/client/testutil/suite.go b/x/auth/client/testutil/suite.go index 1e48d1c8d232..9fbb3b31d7f6 100644 --- a/x/auth/client/testutil/suite.go +++ b/x/auth/client/testutil/suite.go @@ -1361,7 +1361,6 @@ func (s *IntegrationTestSuite) TestTxWithoutPublicKey() { // Create a file with the unsigned tx. txJSON, err := txCfg.TxJSONEncoder()(txBuilder.GetTx()) - fmt.Println(string(txJSON)) s.Require().NoError(err) unsignedTxFile := testutil.WriteToNewTempFile(s.T(), string(txJSON)) From 8673ea04d4e5895ecd23a2d135b6a73f46463d71 Mon Sep 17 00:00:00 2001 From: Amaury M <1293565+amaurym@users.noreply.github.com> Date: Mon, 6 Dec 2021 17:25:32 +0100 Subject: [PATCH 8/8] Address nits --- client/tx/tx.go | 26 +++++++++++--------------- 1 file changed, 11 insertions(+), 15 deletions(-) diff --git a/client/tx/tx.go b/client/tx/tx.go index ae3c5e2f6a18..1d3fcb3af013 100644 --- a/client/tx/tx.go +++ b/client/tx/tx.go @@ -174,25 +174,21 @@ func SignWithPrivKey( func countDirectSigners(data signing.SignatureData) int { switch data := data.(type) { case *signing.SingleSignatureData: - { - if data.SignMode == signing.SignMode_SIGN_MODE_DIRECT { - return 1 - } - - return 0 + if data.SignMode == signing.SignMode_SIGN_MODE_DIRECT { + return 1 } - case *signing.MultiSignatureData: - { - directSigners := 0 - for _, d := range data.Signatures { - directSigners += countDirectSigners(d) - } - return directSigners + return 0 + case *signing.MultiSignatureData: + directSigners := 0 + for _, d := range data.Signatures { + directSigners += countDirectSigners(d) } - } - panic("unreachable case") + return directSigners + default: + panic("unreachable case") + } } // checkMultipleSigners checks that there can be maximum one DIRECT signer in