From b6f1972ebc1ac74402280d9844c479212ea130d9 Mon Sep 17 00:00:00 2001 From: Amaury <1293565+amaurym@users.noreply.github.com> Date: Wed, 8 Dec 2021 12:43:15 +0100 Subject: [PATCH] refactor: Change SignerData.SignerIndex to PubKey (#10692) ## Description Closes: #10691 --- ### Author Checklist *All items are required. Please add a note to the item if the item is not applicable and please add links to any relevant follow up issues.* I have... - [ ] included the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title - [ ] added `!` to the type prefix if API or client breaking change - [ ] targeted the correct branch (see [PR Targeting](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#pr-targeting)) - [ ] provided a link to the relevant issue or specification - [ ] followed the guidelines for [building modules](https://github.com/cosmos/cosmos-sdk/blob/master/docs/building-modules) - [ ] included the necessary unit and integration [tests](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#testing) - [ ] added a changelog entry to `CHANGELOG.md` - [ ] included comments for [documenting Go code](https://blog.golang.org/godoc) - [ ] updated the relevant documentation or specification - [ ] reviewed "Files changed" and left comments if necessary - [ ] confirmed all CI checks have passed ### Reviewers Checklist *All items are required. Please add a note if the item is not applicable and please add your handle next to the items reviewed if you only reviewed selected items.* I have... - [ ] confirmed the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title - [ ] confirmed `!` in the type prefix if API or client breaking change - [ ] confirmed all author checklist items have been addressed - [ ] reviewed state machine logic - [ ] reviewed API design and naming - [ ] reviewed documentation is accurate - [ ] reviewed tests and test coverage - [ ] manually tested (if applicable) --- CHANGELOG.md | 1 + client/tx/tx.go | 15 +-------------- server/rosetta/converter.go | 2 +- simapp/helpers/test_helpers.go | 2 +- x/auth/client/cli/tx_multisign.go | 6 +++--- x/auth/client/cli/validate_sigs.go | 2 +- x/auth/middleware/feegrant_test.go | 2 +- x/auth/middleware/sigverify.go | 2 +- x/auth/middleware/testutil_test.go | 2 +- x/auth/middleware/tips_test.go | 2 +- x/auth/migrations/legacytx/amino_signing_test.go | 2 +- x/auth/signing/handler_map_test.go | 2 +- x/auth/signing/sign_mode_handler.go | 16 +++++++++++++--- x/auth/signing/verify_test.go | 2 +- x/auth/testutil/suite.go | 4 ++-- x/auth/tx/aux_test.go | 8 +++++++- x/auth/tx/direct_aux.go | 11 ++++++----- x/auth/tx/direct_aux_test.go | 6 ++++-- x/auth/tx/direct_test.go | 2 +- x/auth/tx/legacy_amino_json_test.go | 6 +++--- 20 files changed, 51 insertions(+), 44 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 074265f3c8..e1a210fd06 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -114,6 +114,7 @@ Ref: https://keepachangelog.com/en/1.0.0/ * [\#10348](https://github.com/cosmos/cosmos-sdk/pull/10348) StdSignBytes takes a new argument of type `*tx.Tip` for signing over tips using LEGACY_AMINO_JSON. * [\#10208](https://github.com/cosmos/cosmos-sdk/pull/10208) The `x/auth/signing.Tx` interface now also includes a new `GetTip() *tx.Tip` method for verifying tipped transactions. The `x/auth/types` expected BankKeeper interface now expects the `SendCoins` method too. * [\#10612](https://github.com/cosmos/cosmos-sdk/pull/10612) `baseapp.NewBaseApp` constructor function doesn't take the `sdk.TxDecoder` anymore. This logic has been moved into the TxDecoderMiddleware. +* [\#10692](https://github.com/cosmos/cosmos-sdk/pull/10612) `SignerData` takes 2 new fields, `Address` and `PubKey`, which need to get populated when using SIGN_MODE_DIRECT_AUX. ### Client Breaking Changes diff --git a/client/tx/tx.go b/client/tx/tx.go index b1b7c3c181..b90da0b84b 100644 --- a/client/tx/tx.go +++ b/client/tx/tx.go @@ -247,24 +247,11 @@ func Sign(txf Factory, name string, txBuilder client.TxBuilder, overwriteSig boo return err } - pubkeys, err := txBuilder.GetTx().GetPubKeys() - if err != nil { - return err - } - - signerIndex := 0 - for i, p := range pubkeys { - if p.Equals(pubKey) { - signerIndex = i - break - } - } - signerData := authsigning.SignerData{ ChainID: txf.chainID, AccountNumber: txf.accountNumber, Sequence: txf.sequence, - SignerIndex: signerIndex, + PubKey: pubKey, Address: sdk.AccAddress(pubKey.Address()).String(), } diff --git a/server/rosetta/converter.go b/server/rosetta/converter.go index 765d7fdeaf..6782073aaf 100644 --- a/server/rosetta/converter.go +++ b/server/rosetta/converter.go @@ -720,7 +720,7 @@ func (c converter) SigningComponents(tx authsigning.Tx, metadata *ConstructionMe ChainID: metadata.ChainID, AccountNumber: metadata.SignersData[i].AccountNumber, Sequence: metadata.SignersData[i].Sequence, - SignerIndex: i, + PubKey: pubKey, } // get signature bytes diff --git a/simapp/helpers/test_helpers.go b/simapp/helpers/test_helpers.go index 887bc8d3da..f0be36ad42 100644 --- a/simapp/helpers/test_helpers.go +++ b/simapp/helpers/test_helpers.go @@ -61,7 +61,7 @@ func GenTx(gen client.TxConfig, msgs []sdk.Msg, feeAmt sdk.Coins, gas uint64, ch ChainID: chainID, AccountNumber: accNums[i], Sequence: accSeqs[i], - SignerIndex: i, + PubKey: p.PubKey(), } signBytes, err := gen.SignModeHandler().GetSignBytes(signMode, signerData, tx.GetTx()) if err != nil { diff --git a/x/auth/client/cli/tx_multisign.go b/x/auth/client/cli/tx_multisign.go index 5f88834125..9c0432df79 100644 --- a/x/auth/client/cli/tx_multisign.go +++ b/x/auth/client/cli/tx_multisign.go @@ -127,13 +127,13 @@ func makeMultiSignCmd() func(cmd *cobra.Command, args []string) (err error) { return fmt.Errorf("set the chain id with either the --chain-id flag or config file") } - for j, sig := range sigs { + for _, sig := range sigs { signingData := signing.SignerData{ Address: sdk.AccAddress(sig.PubKey.Address()).String(), ChainID: txFactory.ChainID(), AccountNumber: txFactory.AccountNumber(), Sequence: txFactory.Sequence(), - SignerIndex: j, + PubKey: sig.PubKey, } err = signing.VerifySignature(sig.PubKey, signingData, sig.Data, txCfg.SignModeHandler(), txBuilder.GetTx()) @@ -328,7 +328,7 @@ func makeBatchMultisignCmd() func(cmd *cobra.Command, args []string) error { ChainID: txFactory.ChainID(), AccountNumber: txFactory.AccountNumber(), Sequence: txFactory.Sequence(), - SignerIndex: i, + PubKey: pubKey, } for _, sig := range signatureBatch { diff --git a/x/auth/client/cli/validate_sigs.go b/x/auth/client/cli/validate_sigs.go index 9a0911d3a8..7d4da92bac 100644 --- a/x/auth/client/cli/validate_sigs.go +++ b/x/auth/client/cli/validate_sigs.go @@ -110,7 +110,7 @@ func printAndValidateSigs( ChainID: chainID, AccountNumber: accNum, Sequence: accSeq, - SignerIndex: i, + PubKey: pubKey, } err = authsigning.VerifySignature(pubKey, signingData, sig.Data, signModeHandler, sigTx) if err != nil { diff --git a/x/auth/middleware/feegrant_test.go b/x/auth/middleware/feegrant_test.go index c4c429f32f..f2c36ad5ef 100644 --- a/x/auth/middleware/feegrant_test.go +++ b/x/auth/middleware/feegrant_test.go @@ -216,7 +216,7 @@ func genTxWithFeeGranter(gen client.TxConfig, msgs []sdk.Msg, feeAmt sdk.Coins, ChainID: chainID, AccountNumber: accNums[i], Sequence: accSeqs[i], - SignerIndex: i, + PubKey: p.PubKey(), } signBytes, err := gen.SignModeHandler().GetSignBytes(signMode, signerData, tx.GetTx()) if err != nil { diff --git a/x/auth/middleware/sigverify.go b/x/auth/middleware/sigverify.go index 0e31efa528..6189546df0 100644 --- a/x/auth/middleware/sigverify.go +++ b/x/auth/middleware/sigverify.go @@ -487,7 +487,7 @@ func (svd sigVerificationTxHandler) sigVerify(ctx context.Context, req tx.Reques ChainID: chainID, AccountNumber: accNum, Sequence: acc.GetSequence(), - SignerIndex: i, + PubKey: pubKey, } if !simulate { diff --git a/x/auth/middleware/testutil_test.go b/x/auth/middleware/testutil_test.go index 241615c488..f493a463fa 100644 --- a/x/auth/middleware/testutil_test.go +++ b/x/auth/middleware/testutil_test.go @@ -151,7 +151,7 @@ func (s *MWTestSuite) createTestTx(txBuilder client.TxBuilder, privs []cryptotyp ChainID: chainID, AccountNumber: accNums[i], Sequence: accSeqs[i], - SignerIndex: i, + PubKey: priv.PubKey(), } sigV2, err := tx.SignWithPrivKey( s.clientCtx.TxConfig.SignModeHandler().DefaultMode(), signerData, diff --git a/x/auth/middleware/tips_test.go b/x/auth/middleware/tips_test.go index b7f3c9ae88..d0b987ad6f 100644 --- a/x/auth/middleware/tips_test.go +++ b/x/auth/middleware/tips_test.go @@ -191,7 +191,7 @@ func (s *MWTestSuite) mkFeePayerTxBuilder( ChainID: chainID, AccountNumber: accNum, Sequence: accSeq, - SignerIndex: 1, + PubKey: feePayerPriv.PubKey(), } feePayerSigV2, err = clienttx.SignWithPrivKey( signMode, signerData, diff --git a/x/auth/migrations/legacytx/amino_signing_test.go b/x/auth/migrations/legacytx/amino_signing_test.go index 3342c2dba0..568ea8b7c2 100644 --- a/x/auth/migrations/legacytx/amino_signing_test.go +++ b/x/auth/migrations/legacytx/amino_signing_test.go @@ -50,7 +50,7 @@ func TestLegacyAminoJSONHandler_GetSignBytes(t *testing.T) { ChainID: chainId, AccountNumber: accNum, Sequence: seqNum, - SignerIndex: 0, + PubKey: priv1.PubKey(), } signBz, err := handler.GetSignBytes(signingtypes.SignMode_SIGN_MODE_LEGACY_AMINO_JSON, signingData, tx) require.NoError(t, err) diff --git a/x/auth/signing/handler_map_test.go b/x/auth/signing/handler_map_test.go index 88559c8c32..c672ff37ae 100644 --- a/x/auth/signing/handler_map_test.go +++ b/x/auth/signing/handler_map_test.go @@ -64,7 +64,7 @@ func TestHandlerMap_GetSignBytes(t *testing.T) { ChainID: chainId, AccountNumber: accNum, Sequence: seqNum, - SignerIndex: 0, + PubKey: priv1.PubKey(), } signBz, err := handler.GetSignBytes(signingtypes.SignMode_SIGN_MODE_LEGACY_AMINO_JSON, signingData, tx) require.NoError(t, err) diff --git a/x/auth/signing/sign_mode_handler.go b/x/auth/signing/sign_mode_handler.go index d66dedebbd..57d201dbde 100644 --- a/x/auth/signing/sign_mode_handler.go +++ b/x/auth/signing/sign_mode_handler.go @@ -1,6 +1,7 @@ package signing import ( + cryptotypes "github.com/cosmos/cosmos-sdk/crypto/types" sdk "github.com/cosmos/cosmos-sdk/types" "github.com/cosmos/cosmos-sdk/types/tx/signing" ) @@ -24,20 +25,29 @@ type SignModeHandler interface { // isn't included in the transaction body itself type SignerData struct { // The address of the signer. + // + // In case of multisigs, this should be the multisig's address. Address string // ChainID is the chain that this transaction is targeted ChainID string - // AccountNumber is the account number of the signer + // AccountNumber is the account number of the signer. + // + // In case of multisigs, this should be the multisig account number. AccountNumber uint64 // Sequence is the account sequence number of the signer that is used // for replay protection. This field is only useful for Legacy Amino signing, // since in SIGN_MODE_DIRECT the account sequence is already in the signer // info. + // + // In case of multisigs, this should be the multisig account number. Sequence uint64 - // SignerIndex index of signer in the signer_infos array. - SignerIndex int + // PubKey is the public key of the signer. + // + // In case of multisigs, this should be the pubkey of the member of the + // multisig that is signing the current sign doc. + PubKey cryptotypes.PubKey } diff --git a/x/auth/signing/verify_test.go b/x/auth/signing/verify_test.go index f67590491d..3d138e1f77 100644 --- a/x/auth/signing/verify_test.go +++ b/x/auth/signing/verify_test.go @@ -53,7 +53,7 @@ func TestVerifySignature(t *testing.T) { ChainID: chainId, AccountNumber: acc.GetAccountNumber(), Sequence: acc.GetSequence(), - SignerIndex: 0, + PubKey: pubKey, } signBytes := legacytx.StdSignBytes(signerData.ChainID, signerData.AccountNumber, signerData.Sequence, 10, fee, msgs, memo, nil) signature, err := priv.Sign(signBytes) diff --git a/x/auth/testutil/suite.go b/x/auth/testutil/suite.go index e1080f067a..d1ef1ba5f7 100644 --- a/x/auth/testutil/suite.go +++ b/x/auth/testutil/suite.go @@ -132,7 +132,7 @@ func (s *TxConfigTestSuite) TestTxBuilderSetSignatures() { ChainID: "test", AccountNumber: 1, Sequence: seq1, - SignerIndex: 0, + PubKey: pubkey, } signBytes, err := signModeHandler.GetSignBytes(signModeHandler.DefaultMode(), signerData, sigTx) s.Require().NoError(err) @@ -144,7 +144,7 @@ func (s *TxConfigTestSuite) TestTxBuilderSetSignatures() { ChainID: "test", AccountNumber: 3, Sequence: mseq, - SignerIndex: 0, + PubKey: multisigPk, } mSignBytes, err := signModeHandler.GetSignBytes(signModeHandler.DefaultMode(), signerData, sigTx) s.Require().NoError(err) diff --git a/x/auth/tx/aux_test.go b/x/auth/tx/aux_test.go index 1f7308dba5..7e60300ed1 100644 --- a/x/auth/tx/aux_test.go +++ b/x/auth/tx/aux_test.go @@ -130,7 +130,13 @@ func TestBuilderWithAux(t *testing.T) { }) signBz, err = encCfg.TxConfig.SignModeHandler().GetSignBytes( signing.SignMode_SIGN_MODE_DIRECT, - authsigning.SignerData{Address: feepayerAddr.String(), ChainID: chainID, AccountNumber: 11, Sequence: 15, SignerIndex: 1}, + authsigning.SignerData{ + Address: feepayerAddr.String(), + ChainID: chainID, + AccountNumber: 11, + Sequence: 15, + PubKey: feepayerPk, + }, w.GetTx(), ) require.NoError(t, err) diff --git a/x/auth/tx/direct_aux.go b/x/auth/tx/direct_aux.go index 63aba24918..475b4e2e35 100644 --- a/x/auth/tx/direct_aux.go +++ b/x/auth/tx/direct_aux.go @@ -3,6 +3,7 @@ package tx import ( "fmt" + codectypes "github.com/cosmos/cosmos-sdk/codec/types" sdk "github.com/cosmos/cosmos-sdk/types" sdkerrors "github.com/cosmos/cosmos-sdk/types/errors" types "github.com/cosmos/cosmos-sdk/types/tx" @@ -39,9 +40,9 @@ func (signModeDirectAuxHandler) GetSignBytes( return nil, fmt.Errorf("can only handle a protobuf Tx, got %T", tx) } - signerInfo := protoTx.tx.AuthInfo.SignerInfos[data.SignerIndex] - if signerInfo == nil || signerInfo.PublicKey == nil { - return nil, sdkerrors.ErrInvalidRequest.Wrapf("got empty pubkey for signer #%d in %s handler", data.SignerIndex, signingtypes.SignMode_SIGN_MODE_DIRECT_AUX) + pkAny, err := codectypes.NewAnyWithValue(data.PubKey) + if err != nil { + return nil, err } addr := data.Address @@ -61,9 +62,9 @@ func (signModeDirectAuxHandler) GetSignBytes( BodyBytes: protoTx.getBodyBytes(), ChainId: data.ChainID, AccountNumber: data.AccountNumber, - Sequence: signerInfo.Sequence, + Sequence: data.Sequence, Tip: protoTx.tx.AuthInfo.Tip, - PublicKey: signerInfo.PublicKey, + PublicKey: pkAny, } return signDocDirectAux.Marshal() diff --git a/x/auth/tx/direct_aux_test.go b/x/auth/tx/direct_aux_test.go index 6bd67024a2..27a5764dbe 100644 --- a/x/auth/tx/direct_aux_test.go +++ b/x/auth/tx/direct_aux_test.go @@ -66,13 +66,15 @@ func TestDirectAuxHandler(t *testing.T) { Address: addr.String(), ChainID: chainID, AccountNumber: accNum, - SignerIndex: 0, + Sequence: accSeq, + PubKey: pubkey, } feePayerSigningData := signing.SignerData{ Address: feePayerAddr.String(), ChainID: chainID, AccountNumber: accNum, - SignerIndex: 1, + Sequence: accSeq, + PubKey: feePayerPubKey, } modeHandler := signModeDirectAuxHandler{} diff --git a/x/auth/tx/direct_test.go b/x/auth/tx/direct_test.go index 418ab56e11..2230963965 100644 --- a/x/auth/tx/direct_test.go +++ b/x/auth/tx/direct_test.go @@ -72,7 +72,7 @@ func TestDirectModeHandler(t *testing.T) { Address: addr.String(), ChainID: "test-chain", AccountNumber: 1, - SignerIndex: 0, + PubKey: pubkey, } signBytes, err := modeHandler.GetSignBytes(signingtypes.SignMode_SIGN_MODE_DIRECT, signingData, txBuilder.GetTx()) diff --git a/x/auth/tx/legacy_amino_json_test.go b/x/auth/tx/legacy_amino_json_test.go index 7bb4770fa6..7df42f92b1 100644 --- a/x/auth/tx/legacy_amino_json_test.go +++ b/x/auth/tx/legacy_amino_json_test.go @@ -15,8 +15,8 @@ import ( ) var ( - _, _, addr1 = testdata.KeyTestPubAddr() - _, _, addr2 = testdata.KeyTestPubAddr() + _, pubkey1, addr1 = testdata.KeyTestPubAddr() + _, _, addr2 = testdata.KeyTestPubAddr() coins = sdk.Coins{sdk.NewInt64Coin("foocoin", 10)} gas = uint64(10000) @@ -112,7 +112,7 @@ func TestLegacyAminoJSONHandler_GetSignBytes(t *testing.T) { ChainID: chainId, AccountNumber: accNum, Sequence: seqNum, - SignerIndex: 0, + PubKey: pubkey1, } // expect error with wrong sign mode