From 966c93c9f96b02c1246022f79d06f4f0468023fc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Juli=C3=A1n=20Toledano?= Date: Fri, 17 Nov 2023 22:22:16 +0100 Subject: [PATCH] fix(client/tx): simulate with correct pk (#18472) (cherry picked from commit 80e0c631ccc6917ede49f47d07069eda0aa9aa5a) # Conflicts: # client/tx/tx_test.go --- CHANGELOG.md | 1 + client/tx/aux_builder_test.go | 7 ++- client/tx/factory.go | 40 +++++++++++---- client/tx/factory_test.go | 96 ++++++++++++++++++++++++++++++++--- client/tx/legacy_test.go | 2 +- client/tx/tx_test.go | 26 ++++++---- 6 files changed, 140 insertions(+), 32 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f4df1c9c048f..c0def70fe770 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -51,6 +51,7 @@ Ref: https://keepachangelog.com/en/1.0.0/ ### Bug Fixes +* (client/tx) [#18472](https://github.com/cosmos/cosmos-sdk/pull/18472) Utilizes the correct Pubkey when simulating a transaction. * (baseapp) [#18486](https://github.com/cosmos/cosmos-sdk/pull/18486) Fixed FinalizeBlock calls not being passed to ABCIListeners ## [v0.50.1](https://github.com/cosmos/cosmos-sdk/releases/tag/v0.50.1) - 2023-11-07 diff --git a/client/tx/aux_builder_test.go b/client/tx/aux_builder_test.go index af61fc098187..f475ee5f0649 100644 --- a/client/tx/aux_builder_test.go +++ b/client/tx/aux_builder_test.go @@ -1,11 +1,10 @@ -package tx_test +package tx import ( "testing" "github.com/stretchr/testify/require" - "github.com/cosmos/cosmos-sdk/client/tx" "github.com/cosmos/cosmos-sdk/codec" codectypes "github.com/cosmos/cosmos-sdk/codec/types" cryptotypes "github.com/cosmos/cosmos-sdk/crypto/types" @@ -26,7 +25,7 @@ func TestAuxTxBuilder(t *testing.T) { // required for test case: "GetAuxSignerData works for DIRECT_AUX" bankModule.RegisterInterfaces(reg) - var b tx.AuxTxBuilder + var b AuxTxBuilder testcases := []struct { name string @@ -200,7 +199,7 @@ func TestAuxTxBuilder(t *testing.T) { for _, tc := range testcases { tc := tc t.Run(tc.name, func(t *testing.T) { - b = tx.NewAuxTxBuilder() + b = NewAuxTxBuilder() err := tc.malleate() if tc.expErr { diff --git a/client/tx/factory.go b/client/tx/factory.go index ab73a550d245..7ab4fe5cbb84 100644 --- a/client/tx/factory.go +++ b/client/tx/factory.go @@ -15,6 +15,7 @@ import ( "github.com/cosmos/cosmos-sdk/client/flags" codectypes "github.com/cosmos/cosmos-sdk/codec/types" "github.com/cosmos/cosmos-sdk/crypto/keyring" + "github.com/cosmos/cosmos-sdk/crypto/keys/multisig" "github.com/cosmos/cosmos-sdk/crypto/keys/secp256k1" cryptotypes "github.com/cosmos/cosmos-sdk/crypto/types" sdk "github.com/cosmos/cosmos-sdk/types" @@ -33,6 +34,7 @@ type Factory struct { timeoutHeight uint64 gasAdjustment float64 chainID string + fromName string offline bool generateOnly bool memo string @@ -84,6 +86,7 @@ func NewFactoryCLI(clientCtx client.Context, flagSet *pflag.FlagSet) (Factory, e accountRetriever: clientCtx.AccountRetriever, keybase: clientCtx.Keyring, chainID: clientCtx.ChainID, + fromName: clientCtx.FromName, offline: clientCtx.Offline, generateOnly: clientCtx.GenerateOnly, gas: gasSetting.Gas, @@ -398,10 +401,8 @@ func (f Factory) BuildSimTx(msgs ...sdk.Msg) ([]byte, error) { // Create an empty signature literal as the ante handler will populate with a // sentinel pubkey. sig := signing.SignatureV2{ - PubKey: pk, - Data: &signing.SingleSignatureData{ - SignMode: f.signMode, - }, + PubKey: pk, + Data: f.getSimSignatureData(pk), Sequence: f.Sequence(), } if err := txb.SetSignatures(sig); err != nil { @@ -427,16 +428,13 @@ func (f Factory) getSimPK() (cryptotypes.PubKey, error) { pk cryptotypes.PubKey = &secp256k1.PubKey{} // use default public key type ) - // Use the first element from the list of keys in order to generate a valid - // pubkey that supports multiple algorithms. if f.simulateAndExecute && f.keybase != nil { - records, _ := f.keybase.List() - if len(records) == 0 { - return nil, errors.New("cannot build signature for simulation, key records slice is empty") + record, err := f.keybase.Key(f.fromName) + if err != nil { + return nil, err } - // take the first record just for simulation purposes - pk, ok = records[0].PubKey.GetCachedValue().(cryptotypes.PubKey) + pk, ok = record.PubKey.GetCachedValue().(cryptotypes.PubKey) if !ok { return nil, errors.New("cannot build signature for simulation, failed to convert proto Any to public key") } @@ -445,6 +443,26 @@ func (f Factory) getSimPK() (cryptotypes.PubKey, error) { return pk, nil } +// getSimSignatureData based on the pubKey type gets the correct SignatureData type +// to use for building a simulation tx. +func (f Factory) getSimSignatureData(pk cryptotypes.PubKey) signing.SignatureData { + multisigPubKey, ok := pk.(*multisig.LegacyAminoPubKey) + if !ok { + return &signing.SingleSignatureData{SignMode: f.signMode} + } + + multiSignatureData := make([]signing.SignatureData, 0, multisigPubKey.Threshold) + for i := uint32(0); i < multisigPubKey.Threshold; i++ { + multiSignatureData = append(multiSignatureData, &signing.SingleSignatureData{ + SignMode: f.SignMode(), + }) + } + + return &signing.MultiSignatureData{ + Signatures: multiSignatureData, + } +} + // Prepare ensures the account defined by ctx.GetFromAddress() exists and // if the account number and/or the account sequence number are zero (not set), // they will be queried for and set on the provided Factory. diff --git a/client/tx/factory_test.go b/client/tx/factory_test.go index 3b055781bd0b..402d178a98dc 100644 --- a/client/tx/factory_test.go +++ b/client/tx/factory_test.go @@ -1,4 +1,4 @@ -package tx_test +package tx import ( "testing" @@ -6,30 +6,114 @@ import ( "github.com/stretchr/testify/require" "github.com/cosmos/cosmos-sdk/client" - "github.com/cosmos/cosmos-sdk/client/tx" + "github.com/cosmos/cosmos-sdk/codec" + codectypes "github.com/cosmos/cosmos-sdk/codec/types" + cryptocodec "github.com/cosmos/cosmos-sdk/crypto/codec" + "github.com/cosmos/cosmos-sdk/crypto/hd" + "github.com/cosmos/cosmos-sdk/crypto/keyring" + "github.com/cosmos/cosmos-sdk/crypto/keys/multisig" + "github.com/cosmos/cosmos-sdk/crypto/keys/secp256k1" + "github.com/cosmos/cosmos-sdk/crypto/types" + "github.com/cosmos/cosmos-sdk/testutil/testdata" + "github.com/cosmos/cosmos-sdk/types/tx/signing" ) -func TestFactoryPrepate(t *testing.T) { +func TestFactoryPrepare(t *testing.T) { t.Parallel() - factory := tx.Factory{} + factory := Factory{} clientCtx := client.Context{} output, err := factory.Prepare(clientCtx.WithOffline(true)) require.NoError(t, err) require.Equal(t, output, factory) - factory = tx.Factory{}.WithAccountRetriever(client.MockAccountRetriever{ReturnAccNum: 10, ReturnAccSeq: 1}).WithAccountNumber(5) + factory = Factory{}.WithAccountRetriever(client.MockAccountRetriever{ReturnAccNum: 10, ReturnAccSeq: 1}).WithAccountNumber(5) output, err = factory.Prepare(clientCtx.WithFrom("foo")) require.NoError(t, err) require.NotEqual(t, output, factory) require.Equal(t, output.AccountNumber(), uint64(5)) require.Equal(t, output.Sequence(), uint64(1)) - factory = tx.Factory{}.WithAccountRetriever(client.MockAccountRetriever{ReturnAccNum: 10, ReturnAccSeq: 1}) + factory = Factory{}.WithAccountRetriever(client.MockAccountRetriever{ReturnAccNum: 10, ReturnAccSeq: 1}) output, err = factory.Prepare(clientCtx.WithFrom("foo")) require.NoError(t, err) require.NotEqual(t, output, factory) require.Equal(t, output.AccountNumber(), uint64(10)) require.Equal(t, output.Sequence(), uint64(1)) } + +func TestFactory_getSimPKType(t *testing.T) { + // setup keyring + registry := codectypes.NewInterfaceRegistry() + cryptocodec.RegisterInterfaces(registry) + k := keyring.NewInMemory(codec.NewProtoCodec(registry)) + + tests := []struct { + name string + fromName string + genKey func(fromName string, k keyring.Keyring) error + wantType types.PubKey + }{ + { + name: "simple key", + fromName: "testKey", + genKey: func(fromName string, k keyring.Keyring) error { + _, err := k.NewAccount(fromName, testdata.TestMnemonic, "", "", hd.Secp256k1) + return err + }, + wantType: (*secp256k1.PubKey)(nil), + }, + { + name: "multisig key", + fromName: "multiKey", + genKey: func(fromName string, k keyring.Keyring) error { + pk := multisig.NewLegacyAminoPubKey(1, []types.PubKey{&multisig.LegacyAminoPubKey{}}) + _, err := k.SaveMultisig(fromName, pk) + return err + }, + wantType: (*multisig.LegacyAminoPubKey)(nil), + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := tt.genKey(tt.fromName, k) + require.NoError(t, err) + f := Factory{ + keybase: k, + fromName: tt.fromName, + simulateAndExecute: true, + } + got, err := f.getSimPK() + require.NoError(t, err) + require.IsType(t, tt.wantType, got) + }) + } +} + +func TestFactory_getSimSignatureData(t *testing.T) { + tests := []struct { + name string + pk types.PubKey + wantType any + }{ + { + name: "simple pubkey", + pk: &secp256k1.PubKey{}, + wantType: (*signing.SingleSignatureData)(nil), + }, + { + name: "multisig pubkey", + pk: &multisig.LegacyAminoPubKey{}, + wantType: (*signing.MultiSignatureData)(nil), + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := Factory{}.getSimSignatureData(tt.pk) + require.IsType(t, tt.wantType, got) + }) + } +} diff --git a/client/tx/legacy_test.go b/client/tx/legacy_test.go index 53751d4d90da..9492a3ed18eb 100644 --- a/client/tx/legacy_test.go +++ b/client/tx/legacy_test.go @@ -1,4 +1,4 @@ -package tx_test +package tx import ( "github.com/cosmos/cosmos-sdk/testutil/testdata" diff --git a/client/tx/tx_test.go b/client/tx/tx_test.go index 36ece26b92de..cfc828966ef7 100644 --- a/client/tx/tx_test.go +++ b/client/tx/tx_test.go @@ -1,4 +1,4 @@ -package tx_test +package tx import ( "context" @@ -9,8 +9,14 @@ import ( "github.com/stretchr/testify/require" "google.golang.org/grpc" +<<<<<<< HEAD +======= + "cosmossdk.io/x/auth/ante" + "cosmossdk.io/x/auth/signing" + authtx "cosmossdk.io/x/auth/tx" + +>>>>>>> 80e0c631c (fix(client/tx): simulate with correct pk (#18472)) "github.com/cosmos/cosmos-sdk/client" - "github.com/cosmos/cosmos-sdk/client/tx" "github.com/cosmos/cosmos-sdk/codec" codectypes "github.com/cosmos/cosmos-sdk/codec/types" "github.com/cosmos/cosmos-sdk/crypto/hd" @@ -80,7 +86,7 @@ func TestCalculateGas(t *testing.T) { defaultSignMode, err := signing.APISignModeToInternal(txCfg.SignModeHandler().DefaultMode()) require.NoError(t, err) - txf := tx.Factory{}. + txf := Factory{}. WithChainID("test-chain"). WithTxConfig(txCfg).WithSignMode(defaultSignMode) @@ -89,7 +95,7 @@ func TestCalculateGas(t *testing.T) { gasUsed: tc.args.mockGasUsed, wantErr: tc.args.mockWantErr, } - simRes, gotAdjusted, err := tx.CalculateGas(mockClientCtx, txf.WithGasAdjustment(stc.args.adjustment)) + simRes, gotAdjusted, err := CalculateGas(mockClientCtx, txf.WithGasAdjustment(stc.args.adjustment)) if stc.expPass { require.NoError(t, err) require.Equal(t, simRes.GasInfo.GasUsed, stc.wantEstimate) @@ -103,8 +109,8 @@ func TestCalculateGas(t *testing.T) { } } -func mockTxFactory(txCfg client.TxConfig) tx.Factory { - return tx.Factory{}. +func mockTxFactory(txCfg client.TxConfig) Factory { + return Factory{}. WithTxConfig(txCfg). WithAccountNumber(50). WithSequence(23). @@ -195,7 +201,7 @@ func TestMnemonicInMemo(t *testing.T) { for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { - txf := tx.Factory{}. + txf := Factory{}. WithTxConfig(txConfig). WithAccountNumber(50). WithSequence(23). @@ -266,7 +272,7 @@ func TestSign(t *testing.T) { testCases := []struct { name string - txf tx.Factory + txf Factory txb client.TxBuilder from string overwrite bool @@ -353,7 +359,7 @@ func TestSign(t *testing.T) { var prevSigs []signingtypes.SignatureV2 for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { - err = tx.Sign(context.TODO(), tc.txf, tc.from, tc.txb, tc.overwrite) + err = Sign(context.TODO(), tc.txf, tc.from, tc.txb, tc.overwrite) if len(tc.expectedPKs) == 0 { requireT.Error(err) } else { @@ -408,7 +414,7 @@ func TestPreprocessHook(t *testing.T) { txb, err := txfDirect.BuildUnsignedTx(msg1, msg2) requireT.NoError(err) - err = tx.Sign(context.TODO(), txfDirect, from, txb, false) + err = Sign(context.TODO(), txfDirect, from, txb, false) requireT.NoError(err) // Run preprocessing