From 2eb6fd9a81a3b3eaf606cccf4b512dfc4aa9d4c5 Mon Sep 17 00:00:00 2001 From: Sujong Lee Date: Mon, 17 Oct 2022 06:31:36 +0900 Subject: [PATCH] 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 | 3 +- client/tx/tx.go | 55 ++++++++++++++++++++++++++++----- client/tx/tx_test.go | 26 ++++++++++++---- x/auth/client/testutil/suite.go | 3 +- 4 files changed, 71 insertions(+), 16 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6018d4fd0c..dcf04b11c2 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/client/tx/tx.go b/client/tx/tx.go index e9e94dfdad..a27ee09915 100644 --- a/client/tx/tx.go +++ b/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,6 +400,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() @@ -380,7 +408,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 edee55de2f..911d8e391e 100644 --- a/client/tx/tx_test.go +++ b/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 { diff --git a/x/auth/client/testutil/suite.go b/x/auth/client/testutil/suite.go index 9bb89fcef8..4545ba9619 100644 --- a/x/auth/client/testutil/suite.go +++ b/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.