From 712d4a7b7d6a0a1a5fbd74e075f421944dfd5ac9 Mon Sep 17 00:00:00 2001 From: Julian Toledano Date: Wed, 15 Nov 2023 12:14:13 +0100 Subject: [PATCH 1/8] fix: simulate with correct pk --- client/tx/factory.go | 13 ++++--------- 1 file changed, 4 insertions(+), 9 deletions(-) diff --git a/client/tx/factory.go b/client/tx/factory.go index 02f6a41e159d..f8e4b045d79c 100644 --- a/client/tx/factory.go +++ b/client/tx/factory.go @@ -33,6 +33,7 @@ type Factory struct { timeoutHeight uint64 gasAdjustment float64 chainID string + fromName string offline bool generateOnly bool memo string @@ -92,6 +93,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, @@ -435,16 +437,9 @@ 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") - } - - // take the first record just for simulation purposes - pk, ok = records[0].PubKey.GetCachedValue().(cryptotypes.PubKey) + record, _ := f.keybase.Key(f.fromName) + 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") } From 6cc1febd9d0ffec674b18262ef2d9803a4d1c828 Mon Sep 17 00:00:00 2001 From: Julian Toledano Date: Wed, 15 Nov 2023 12:20:09 +0100 Subject: [PATCH 2/8] add: check err --- client/tx/factory.go | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/client/tx/factory.go b/client/tx/factory.go index f8e4b045d79c..f9fbfcf56786 100644 --- a/client/tx/factory.go +++ b/client/tx/factory.go @@ -438,7 +438,11 @@ func (f Factory) getSimPK() (cryptotypes.PubKey, error) { ) if f.simulateAndExecute && f.keybase != nil { - record, _ := f.keybase.Key(f.fromName) + record, err := f.keybase.Key(f.fromName) + if err != nil { + return nil, err + } + 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") From 6fdc50a7e29fb198c863fcc209453e631612df46 Mon Sep 17 00:00:00 2001 From: Julian Toledano Date: Wed, 15 Nov 2023 12:24:48 +0100 Subject: [PATCH 3/8] add: CHANGELOG --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2803cd8ed97b..98bfe32d56cd 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -65,6 +65,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) [#18383](https://github.com/cosmos/cosmos-sdk/pull/18383) Fixed a data race inside BaseApp.getContext, found by end-to-end (e2e) tests. * (client/server) [#18345](https://github.com/cosmos/cosmos-sdk/pull/18345) Consistently set viper prefix in client and server. It defaults for the binary name for both client and server. * (simulation) [#17911](https://github.com/cosmos/cosmos-sdk/pull/17911) Fix all problems with executing command `make test-sim-custom-genesis-fast` for simulation test. From 058d220ff204adf2d40a4b5ec7c267b54c49f8ea Mon Sep 17 00:00:00 2001 From: Julian Toledano Date: Thu, 16 Nov 2023 11:44:02 +0100 Subject: [PATCH 4/8] fix: use correct SignatureData --- client/tx/factory.go | 26 ++++++++++++++++++++++---- 1 file changed, 22 insertions(+), 4 deletions(-) diff --git a/client/tx/factory.go b/client/tx/factory.go index f9fbfcf56786..aaadb50c3ca2 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" @@ -408,10 +409,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 { @@ -452,6 +451,25 @@ 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 { + if multisigPubKey, ok := pk.(*multisig.LegacyAminoPubKey); ok { + 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, + } + } + return &signing.SingleSignatureData{ + SignMode: f.signMode, + } +} + // 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. From 7217fba621ae38ca417a6050d939879c757673d9 Mon Sep 17 00:00:00 2001 From: Julian Toledano Date: Thu, 16 Nov 2023 16:38:25 +0100 Subject: [PATCH 5/8] add: tests --- client/tx/factory_test.go | 92 ++++++++++++++++++++++++++++++++++++--- 1 file changed, 87 insertions(+), 5 deletions(-) diff --git a/client/tx/factory_test.go b/client/tx/factory_test.go index 3b055781bd0b..9dcc61cd5bb5 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,112 @@ 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) { 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_getSimPK(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 + want 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 + }, + want: &secp256k1.PubKey{}, + }, + { + 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 + }, + want: &multisig.LegacyAminoPubKey{}, + }, + } + 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.want, got) + }) + } +} + +func TestFactory_getSimSignatureData(t *testing.T) { + tests := []struct { + name string + pk types.PubKey + want signing.SignatureData + }{ + { + name: "simple pubkey", + pk: &secp256k1.PubKey{}, + want: &signing.SingleSignatureData{}, + }, + { + name: "multisig pubkey", + pk: &multisig.LegacyAminoPubKey{}, + want: &signing.MultiSignatureData{}, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := Factory{}.getSimSignatureData(tt.pk) + require.IsType(t, tt.want, got) + }) + } +} From 8d3e6930929de37b779420816b98f724c899744f Mon Sep 17 00:00:00 2001 From: Julian Toledano Date: Thu, 16 Nov 2023 19:08:15 +0100 Subject: [PATCH 6/8] update: change test package name --- client/tx/aux_builder_test.go | 7 +++---- client/tx/factory_test.go | 2 +- client/tx/legacy_test.go | 2 +- client/tx/tx_test.go | 21 ++++++++++----------- 4 files changed, 15 insertions(+), 17 deletions(-) diff --git a/client/tx/aux_builder_test.go b/client/tx/aux_builder_test.go index 4758247f9e86..aa2bf7751d20 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" counterModule.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_test.go b/client/tx/factory_test.go index 9dcc61cd5bb5..cec28d05a149 100644 --- a/client/tx/factory_test.go +++ b/client/tx/factory_test.go @@ -18,7 +18,7 @@ import ( "github.com/cosmos/cosmos-sdk/types/tx/signing" ) -func TestFactoryPrepate(t *testing.T) { +func TestFactoryPrepare(t *testing.T) { t.Parallel() factory := Factory{} diff --git a/client/tx/legacy_test.go b/client/tx/legacy_test.go index 15aac031523e..cd7a64776fa4 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 340fbc804349..384c2177d650 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,12 +9,11 @@ import ( "github.com/stretchr/testify/require" "google.golang.org/grpc" - ante "cosmossdk.io/x/auth/ante" + "cosmossdk.io/x/auth/ante" "cosmossdk.io/x/auth/signing" authtx "cosmossdk.io/x/auth/tx" "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" @@ -81,7 +80,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) @@ -90,7 +89,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) @@ -104,8 +103,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). @@ -196,7 +195,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). @@ -267,7 +266,7 @@ func TestSign(t *testing.T) { testCases := []struct { name string - txf tx.Factory + txf Factory txb client.TxBuilder from string overwrite bool @@ -354,7 +353,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 { @@ -409,7 +408,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 From efb92eb197249afb007edec518b0e62b0f03f314 Mon Sep 17 00:00:00 2001 From: Julian Toledano Date: Fri, 17 Nov 2023 16:39:20 +0100 Subject: [PATCH 7/8] update: tests --- client/tx/factory.go | 23 +++++++++++------------ client/tx/factory_test.go | 32 +++++++++++++++++--------------- 2 files changed, 28 insertions(+), 27 deletions(-) diff --git a/client/tx/factory.go b/client/tx/factory.go index aaadb50c3ca2..7c5f9ae264e8 100644 --- a/client/tx/factory.go +++ b/client/tx/factory.go @@ -454,19 +454,18 @@ func (f Factory) getSimPK() (cryptotypes.PubKey, error) { // 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 { - if multisigPubKey, ok := pk.(*multisig.LegacyAminoPubKey); ok { - 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, - } + 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.SingleSignatureData{ - SignMode: f.signMode, + return &signing.MultiSignatureData{ + Signatures: multiSignatureData, } } diff --git a/client/tx/factory_test.go b/client/tx/factory_test.go index cec28d05a149..402d178a98dc 100644 --- a/client/tx/factory_test.go +++ b/client/tx/factory_test.go @@ -43,7 +43,7 @@ func TestFactoryPrepare(t *testing.T) { require.Equal(t, output.Sequence(), uint64(1)) } -func TestFactory_getSimPK(t *testing.T) { +func TestFactory_getSimPKType(t *testing.T) { // setup keyring registry := codectypes.NewInterfaceRegistry() cryptocodec.RegisterInterfaces(registry) @@ -53,7 +53,7 @@ func TestFactory_getSimPK(t *testing.T) { name string fromName string genKey func(fromName string, k keyring.Keyring) error - want types.PubKey + wantType types.PubKey }{ { name: "simple key", @@ -62,7 +62,7 @@ func TestFactory_getSimPK(t *testing.T) { _, err := k.NewAccount(fromName, testdata.TestMnemonic, "", "", hd.Secp256k1) return err }, - want: &secp256k1.PubKey{}, + wantType: (*secp256k1.PubKey)(nil), }, { name: "multisig key", @@ -72,9 +72,10 @@ func TestFactory_getSimPK(t *testing.T) { _, err := k.SaveMultisig(fromName, pk) return err }, - want: &multisig.LegacyAminoPubKey{}, + wantType: (*multisig.LegacyAminoPubKey)(nil), }, } + for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { err := tt.genKey(tt.fromName, k) @@ -86,32 +87,33 @@ func TestFactory_getSimPK(t *testing.T) { } got, err := f.getSimPK() require.NoError(t, err) - require.IsType(t, tt.want, got) + require.IsType(t, tt.wantType, got) }) } } func TestFactory_getSimSignatureData(t *testing.T) { tests := []struct { - name string - pk types.PubKey - want signing.SignatureData + name string + pk types.PubKey + wantType any }{ { - name: "simple pubkey", - pk: &secp256k1.PubKey{}, - want: &signing.SingleSignatureData{}, + name: "simple pubkey", + pk: &secp256k1.PubKey{}, + wantType: (*signing.SingleSignatureData)(nil), }, { - name: "multisig pubkey", - pk: &multisig.LegacyAminoPubKey{}, - want: &signing.MultiSignatureData{}, + 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.want, got) + require.IsType(t, tt.wantType, got) }) } } From 9169d8316414c0ae789e1c0e2b90e9dd96235081 Mon Sep 17 00:00:00 2001 From: Julian Toledano Date: Fri, 17 Nov 2023 16:40:48 +0100 Subject: [PATCH 8/8] style --- client/tx/factory.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/client/tx/factory.go b/client/tx/factory.go index 7c5f9ae264e8..e3ca6041d9e0 100644 --- a/client/tx/factory.go +++ b/client/tx/factory.go @@ -458,12 +458,14 @@ func (f Factory) getSimSignatureData(pk cryptotypes.PubKey) signing.SignatureDat 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, }