fix: allow up to one direct sign in multi sign tx (#708)

* fix: TXT01 - allow one direct sign in multi sign

* docs: add changelog

CHANGELOG.md

@@ -77,7 +77,8 @@ Ref: https://keepachangelog.com/en/1.0.0/
 * (x/wasm,distribution) [\#696](https://github.com/line/lbm-sdk/pull/696) x/wasm,distribution - add checking a file size before reading it
 * (x/foundation) [\#698](https://github.com/line/lbm-sdk/pull/698) update x/group relevant logic in x/foundation
 * (x/auth,bank,foundation,wasm) [\#691](https://github.com/line/lbm-sdk/pull/691) change AccAddressFromBech32 to MustAccAddressFromBech32
-* (x/wasm)  [\#690](https://github.com/line/lbm-sdk/pull/690) fix to prevent accepting file name
+* (x/wasm) [\#690](https://github.com/line/lbm-sdk/pull/690) fix to prevent accepting file name
+* (cli) [\#708](https://github.com/line/lbm-sdk/pull/708) In CLI, allow 1 SIGN_MODE_DIRECT signer in transactions with multiple signers.
 
 ### Bug Fixes
 * (x/wasm) [\#453](https://github.com/line/lbm-sdk/pull/453) modify wasm grpc query api path

client/tx/tx.go

@@ -317,11 +317,41 @@ 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
+	default:
+		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 sdkerrors.ErrNotSupported.Wrap("txs signed with CLI can have maximum 1 DIRECT signer")
+		}
 	}
+
 	return nil
 }
 
@@ -341,9 +371,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
-	}
 
 	key, err := txf.keybase.Key(name)
 	if err != nil {
@@ -373,14 +400,26 @@ 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()
 		if err != nil {
 			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
 	}
 

client/tx/tx_test.go

@@ -306,25 +306,39 @@ func TestSign(t *testing.T) {
 		},
 
 		/**** test double sign Direct mode
-		  signing transaction with more than 2 signers should fail in DIRECT mode ****/
+		  signing transaction with 2 or more DIRECT signers should fail in DIRECT mode ****/
 		{
-			"direct: should fail to append a signature with different mode",
+			"direct: should append a DIRECT signature with existing AMINO",
+			// txb already has 1 AMINO signature
 			txfDirect, txb, from1, false,
-			[]cryptotypes.PubKey{},
+			[]cryptotypes.PubKey{pubKey2, pubKey1},
 			nil,
 		},
 		{
-			"direct: should fail to sign multi-signers tx",
+			"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 fail to overwrite multi-signers tx",
-			txfDirect, txb2, from1, true,
+			"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,
+		},
 	}
 	var prevSigs []signingtypes.SignatureV2
 	for _, tc := range testCases {

x/auth/client/testutil/suite.go

@@ -33,6 +33,7 @@ import (
 	authtypes "github.com/line/lbm-sdk/x/auth/types"
 	bankcli "github.com/line/lbm-sdk/x/bank/client/testutil"
 	banktypes "github.com/line/lbm-sdk/x/bank/types"
+	"github.com/line/lbm-sdk/x/genutil/client/cli"
 )
 
 type IntegrationTestSuite struct {
@@ -1247,7 +1248,7 @@ func (s *IntegrationTestSuite) TestTxWithoutPublicKey() {
 	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.