From 5ebc2b8cb5b1aad6959a0caecc15e3a502303e59 Mon Sep 17 00:00:00 2001 From: "mergify[bot]" <37929162+mergify[bot]@users.noreply.github.com> Date: Tue, 31 Aug 2021 19:00:30 +0200 Subject: [PATCH] refactor: Move some methods inside TX Factory (backport #9421) (#10039) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * refactor: Move some methods inside TX Factory (#9421) ## Description Putting some things inside the factory (which was very anemic struct) has helped me to understand the flow. Feel free to merge if you see some benefit. closes: #XXXX --- Before we can merge this PR, please make sure that all the following items have been checked off. If any of the checklist items are not applicable, please leave them but write a little note why. - [ ] Targeted PR against correct branch (see [CONTRIBUTING.md](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#pr-targeting)) - [ ] Linked to Github issue with discussion and accepted design OR link to spec that describes this work. - [ ] Code follows the [module structure standards](https://github.com/cosmos/cosmos-sdk/blob/master/docs/building-modules/structure.md). - [ ] Wrote unit and integration [tests](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#testing) - [ ] Updated relevant documentation (`docs/`) or specification (`x//spec/`) - [ ] Added relevant `godoc` [comments](https://blog.golang.org/godoc-documenting-go-code). - [ ] Added a relevant changelog entry to the `Unreleased` section in `CHANGELOG.md` - [ ] Re-reviewed `Files changed` in the Github PR explorer - [ ] Review `Codecov Report` in the comment section below once CI passes (cherry picked from commit e17be874bb0e3a246b74752e9b8894855cab9b03) # Conflicts: # CHANGELOG.md * Update CHANGELOG.md Co-authored-by: Jonathan Gimeno Co-authored-by: Robert Zaremba --- CHANGELOG.md | 12 +-- client/tx/factory.go | 198 ++++------------------------------ client/tx/tx.go | 83 +++++++++++--- client/tx/tx_test.go | 29 +---- x/genutil/client/cli/gentx.go | 6 -- 5 files changed, 98 insertions(+), 230 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 52145443c4da..844481c09bbf 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -53,7 +53,11 @@ Ref: https://keepachangelog.com/en/1.0.0/ ### Bug Fixes + [\#9965](https://github.com/cosmos/cosmos-sdk/pull/9965) Fixed `simd version` command output to report the right release tag. - ++ +### API Breaking Changes +* (client/tx) [\#9421](https://github.com/cosmos/cosmos-sdk/pull/9421/) `BuildUnsignedTx`, `BuildSimTx`, `PrintUnsignedStdTx` functions are moved to + the Tx Factory as methods. + ## [v0.43.0](https://github.com/cosmos/cosmos-sdk/releases/tag/v0.43.0) - 2021-08-10 ### Features @@ -148,11 +152,7 @@ Ref: https://keepachangelog.com/en/1.0.0/ * (x/capability) [\#9836](https://github.com/cosmos/cosmos-sdk/pull/9836) Removed `InitializeAndSeal(ctx sdk.Context)` and replaced with `Seal()`. App must add x/capability module to the begin blockers which will assure that the x/capability keeper is properly initialized. The x/capability begin blocker must be run before any other module which uses x/capability. -### Deprecated - -## [v0.52.0](https://github.com/cosmos/cosmos-sdk/releases/tag/v0.52.0) - 2024-XX-XX - -Every module contains its own CHANGELOG.md. Please refer to the module you are interested in. +### State Machine Breaking ### Features diff --git a/client/tx/factory.go b/client/tx/factory.go index 4864cacaa877..eef7c34b0207 100644 --- a/client/tx/factory.go +++ b/client/tx/factory.go @@ -3,12 +3,8 @@ package tx import ( "errors" "fmt" - "math/big" "os" - "strings" - "time" - "github.com/cosmos/go-bip39" "github.com/spf13/pflag" "cosmossdk.io/math" @@ -17,9 +13,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" "github.com/cosmos/cosmos-sdk/types/tx/signing" ) @@ -253,84 +247,11 @@ func (f Factory) WithTimeoutHeight(height uint64) Factory { return f } -// WithTimeoutTimestamp returns a copy of the Factory with an updated timeout timestamp. -func (f Factory) WithTimeoutTimestamp(timestamp time.Time) Factory { - f.timeoutTimestamp = timestamp - return f -} - -// WithUnordered returns a copy of the Factory with an updated unordered field. -func (f Factory) WithUnordered(v bool) Factory { - f.unordered = v - return f -} - -// WithFeeGranter returns a copy of the Factory with an updated fee granter. -func (f Factory) WithFeeGranter(fg sdk.AccAddress) Factory { - f.feeGranter = fg - return f -} - -// WithFeePayer returns a copy of the Factory with an updated fee granter. -func (f Factory) WithFeePayer(fp sdk.AccAddress) Factory { - f.feePayer = fp - return f -} - -// WithPreprocessTxHook returns a copy of the Factory with an updated preprocess tx function, -// allows for preprocessing of transaction data using the TxBuilder. -func (f Factory) WithPreprocessTxHook(preprocessFn client.PreprocessTxFn) Factory { - f.preprocessTxHook = preprocessFn - return f -} - -// PreprocessTx calls the preprocessing hook with the factory parameters and -// returns the result. -func (f Factory) PreprocessTx(keyname string, builder client.TxBuilder) error { - if f.preprocessTxHook == nil { - // Allow pass-through - return nil - } - - key, err := f.Keybase().Key(keyname) - if err != nil { - return fmt.Errorf("error retrieving key from keyring: %w", err) - } - - return f.preprocessTxHook(f.chainID, key.GetType(), builder) -} - -// WithExtensionOptions returns a Factory with given extension options added to the existing options, -// Example to add dynamic fee extension options: -// -// extOpt := ethermint.ExtensionOptionDynamicFeeTx{ -// MaxPriorityPrice: math.NewInt(1000000), -// } -// -// extBytes, _ := extOpt.Marshal() -// -// extOpts := []*types.Any{ -// { -// TypeUrl: "/ethermint.types.v1.ExtensionOptionDynamicFeeTx", -// Value: extBytes, -// }, -// } -// -// txf.WithExtensionOptions(extOpts...) -func (f Factory) WithExtensionOptions(extOpts ...*codectypes.Any) Factory { - f.extOptions = extOpts - return f -} - // BuildUnsignedTx builds a transaction to be signed given a set of messages. // Once created, the fee, memo, and messages are set. func (f Factory) BuildUnsignedTx(msgs ...sdk.Msg) (client.TxBuilder, error) { - if f.offline && f.generateOnly { - if f.chainID != "" { - return nil, errors.New("chain ID cannot be used when offline and generate-only flags are set") - } - } else if f.chainID == "" { - return nil, errors.New("chain ID required but not specified") + if f.chainID == "" { + return nil, fmt.Errorf("chain ID required but not specified") } fees := f.fees @@ -340,9 +261,7 @@ func (f Factory) BuildUnsignedTx(msgs ...sdk.Msg) (client.TxBuilder, error) { return nil, errors.New("cannot provide both fees and gas prices") } - // f.gas is a uint64 and we should convert to LegacyDec - // without the risk of under/overflow via uint64->int64. - glDec := math.LegacyNewDecFromBigInt(new(big.Int).SetUint64(f.gas)) + glDec := sdk.NewDec(int64(f.gas)) // Derive the fees based on the provided gas prices, where // fee = ceil(gasPrice * gasLimit). @@ -354,11 +273,6 @@ func (f Factory) BuildUnsignedTx(msgs ...sdk.Msg) (client.TxBuilder, error) { } } - // Prevent simple inclusion of a valid mnemonic in the memo field - if f.memo != "" && bip39.IsMnemonicValid(strings.ToLower(f.memo)) { - return nil, errors.New("cannot provide a valid mnemonic seed in the memo field") - } - tx := f.txConfig.NewTxBuilder() if err := tx.SetMsgs(msgs...); err != nil { @@ -368,15 +282,7 @@ func (f Factory) BuildUnsignedTx(msgs ...sdk.Msg) (client.TxBuilder, error) { tx.SetMemo(f.memo) tx.SetFeeAmount(fees) tx.SetGasLimit(f.gas) - tx.SetFeeGranter(f.feeGranter) - tx.SetFeePayer(f.feePayer) tx.SetTimeoutHeight(f.TimeoutHeight()) - tx.SetTimeoutTimestamp(f.TimeoutTimestamp()) - tx.SetUnordered(f.Unordered()) - - if etx, ok := tx.(client.ExtendedTxBuilder); ok { - etx.SetExtensionOptions(f.extOptions...) - } return tx, nil } @@ -391,14 +297,7 @@ func (f Factory) PrintUnsignedTx(clientCtx client.Context, msgs ...sdk.Msg) erro return errors.New("cannot estimate gas in offline mode") } - // Prepare TxFactory with acc & seq numbers as CalculateGas requires - // account and sequence numbers to be set - preparedTxf, err := f.Prepare(clientCtx) - if err != nil { - return err - } - - _, adjusted, err := CalculateGas(clientCtx, preparedTxf, msgs...) + _, adjusted, err := CalculateGas(clientCtx, f, msgs...) if err != nil { return err } @@ -407,17 +306,12 @@ func (f Factory) PrintUnsignedTx(clientCtx client.Context, msgs ...sdk.Msg) erro _, _ = fmt.Fprintf(os.Stderr, "%s\n", GasEstimateResponse{GasEstimate: f.Gas()}) } - unsignedTx, err := f.BuildUnsignedTx(msgs...) + tx, err := f.BuildUnsignedTx(msgs...) if err != nil { return err } - encoder := f.txConfig.TxJSONEncoder() - if encoder == nil { - return errors.New("cannot print unsigned tx: tx json encoder is nil") - } - - json, err := encoder(unsignedTx.GetTx()) + json, err := clientCtx.TxConfig.TxJSONEncoder()(tx.GetTx()) if err != nil { return err } @@ -434,88 +328,34 @@ func (f Factory) BuildSimTx(msgs ...sdk.Msg) ([]byte, error) { return nil, err } - pk, err := f.getSimPK() - if err != nil { - return nil, err - } - // Create an empty signature literal as the ante handler will populate with a // sentinel pubkey. sig := signing.SignatureV2{ - PubKey: pk, - Data: f.getSimSignatureData(pk), + PubKey: &secp256k1.PubKey{}, + Data: &signing.SingleSignatureData{ + SignMode: f.signMode, + }, Sequence: f.Sequence(), } if err := txb.SetSignatures(sig); err != nil { return nil, err } - encoder := f.txConfig.TxEncoder() - if encoder == nil { - return nil, errors.New("cannot simulate tx: tx encoder is nil") - } - - return encoder(txb.GetTx()) -} - -// getSimPK gets the public key to use for building a simulation tx. -// Note, we should only check for keys in the keybase if we are in simulate and execute mode, -// e.g. when using --gas=auto. -// When using --dry-run, we are in simulation mode only and should not check the keybase. -// Ref: https://github.com/cosmos/cosmos-sdk/issues/11283 -func (f Factory) getSimPK() (cryptotypes.PubKey, error) { - var ( - ok bool - pk cryptotypes.PubKey = &secp256k1.PubKey{} // use default public key type - ) - - if f.simulateAndExecute && f.keybase != nil { - 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") - } - } - - 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, - } + return f.txConfig.TxEncoder()(txb.GetTx()) } // 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. -// A new Factory with the updated fields will be returned. -// Note: When in offline mode, the Prepare does nothing and returns the original factory. +// they will be queried for and set on the provided Factory. A new Factory with +// the updated fields will be returned. func (f Factory) Prepare(clientCtx client.Context) (Factory, error) { - if clientCtx.Offline { - return f, nil - } - fc := f - from := clientCtx.FromAddress + + from := clientCtx.GetFromAddress() + + if err := fc.accountRetriever.EnsureExists(clientCtx, from); err != nil { + return fc, err + } initNum, initSeq := fc.accountNumber, fc.sequence if initNum == 0 || initSeq == 0 { diff --git a/client/tx/tx.go b/client/tx/tx.go index 4aa34b0a931c..6605833eee5a 100644 --- a/client/tx/tx.go +++ b/client/tx/tx.go @@ -43,20 +43,6 @@ func GenerateOrBroadcastTxWithFactory(clientCtx client.Context, txf Factory, msg if !ok { continue } - - if err := m.ValidateBasic(); err != nil { - return err - } - } - - // If the --aux flag is set, we simply generate and print the AuxSignerData. - if clientCtx.IsAux { - auxSignerData, err := makeAuxSignerData(clientCtx, txf, msgs...) - if err != nil { - return err - } - - return clientCtx.PrintProto(&auxSignerData) } if clientCtx.GenerateOnly { @@ -143,6 +129,75 @@ func BroadcastTx(clientCtx client.Context, txf Factory, msgs ...sdk.Msg) error { return clientCtx.PrintProto(res) } +// WriteGeneratedTxResponse writes a generated unsigned transaction to the +// provided http.ResponseWriter. It will simulate gas costs if requested by the +// BaseReq. Upon any error, the error will be written to the http.ResponseWriter. +// Note that this function returns the legacy StdTx Amino JSON format for compatibility +// with legacy clients. +// Deprecated: We are removing Amino soon. +func WriteGeneratedTxResponse( + clientCtx client.Context, w http.ResponseWriter, br rest.BaseReq, msgs ...sdk.Msg, +) { + gasAdj, ok := rest.ParseFloat64OrReturnBadRequest(w, br.GasAdjustment, flags.DefaultGasAdjustment) + if !ok { + return + } + + gasSetting, err := flags.ParseGasSetting(br.Gas) + if rest.CheckBadRequestError(w, err) { + return + } + + txf := Factory{fees: br.Fees, gasPrices: br.GasPrices}. + WithAccountNumber(br.AccountNumber). + WithSequence(br.Sequence). + WithGas(gasSetting.Gas). + WithGasAdjustment(gasAdj). + WithMemo(br.Memo). + WithChainID(br.ChainID). + WithSimulateAndExecute(br.Simulate). + WithTxConfig(clientCtx.TxConfig). + WithTimeoutHeight(br.TimeoutHeight) + + if br.Simulate || gasSetting.Simulate { + if gasAdj < 0 { + rest.WriteErrorResponse(w, http.StatusBadRequest, sdkerrors.ErrorInvalidGasAdjustment.Error()) + return + } + + _, adjusted, err := CalculateGas(clientCtx, txf, msgs...) + if rest.CheckInternalServerError(w, err) { + return + } + + txf = txf.WithGas(adjusted) + + if br.Simulate { + rest.WriteSimulationResponse(w, clientCtx.LegacyAmino, txf.Gas()) + return + } + } + + tx, err := txf.BuildUnsignedTx(msgs...) + if rest.CheckBadRequestError(w, err) { + return + } + + stdTx, err := ConvertTxToStdTx(clientCtx.LegacyAmino, tx.GetTx()) + if rest.CheckInternalServerError(w, err) { + return + } + + output, err := clientCtx.LegacyAmino.MarshalJSON(stdTx) + if rest.CheckInternalServerError(w, err) { + return + } + + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusOK) + _, _ = w.Write(output) +} + // CalculateGas simulates the execution of a transaction and returns the // simulation response obtained by the query and the adjusted gas amount. func CalculateGas( diff --git a/client/tx/tx_test.go b/client/tx/tx_test.go index 21bfff6070b8..ed6e9e22b895 100644 --- a/client/tx/tx_test.go +++ b/client/tx/tx_test.go @@ -126,15 +126,7 @@ func TestBuildSimTx(t *testing.T) { kb, err := keyring.New(t.Name(), "test", t.TempDir(), nil, cdc) require.NoError(t, err) - path := hd.CreateHDPath(118, 0, 0).String() - _, _, err = kb.NewMnemonic("test_key1", keyring.English, path, keyring.DefaultBIP39Passphrase, hd.Secp256k1) - require.NoError(t, err) - - fromAddr, err := ac.BytesToString(sdk.AccAddress("from")) - require.NoError(t, err) - - txf := mockTxFactory(txCfg).WithSignMode(defaultSignMode).WithKeybase(kb) - msg := &countertypes.MsgIncreaseCounter{Signer: fromAddr, Count: 1} + msg := banktypes.NewMsgSend(sdk.AccAddress("from"), sdk.AccAddress("to"), nil) bz, err := txf.BuildSimTx(msg) require.NoError(t, err) require.NotNil(t, bz) @@ -147,12 +139,7 @@ func TestBuildUnsignedTx(t *testing.T) { path := hd.CreateHDPath(118, 0, 0).String() - _, _, err = kb.NewMnemonic("test_key1", keyring.English, path, keyring.DefaultBIP39Passphrase, hd.Secp256k1) - require.NoError(t, err) - fromAddr, err := ac.BytesToString(sdk.AccAddress("from")) - require.NoError(t, err) - txf := mockTxFactory(txConfig).WithKeybase(kb) - msg := &countertypes.MsgIncreaseCounter{Signer: fromAddr, Count: 1} + msg := banktypes.NewMsgSend(sdk.AccAddress("from"), sdk.AccAddress("to"), nil) tx, err := txf.BuildUnsignedTx(msg) require.NoError(t, err) require.NotNil(t, tx) @@ -266,16 +253,8 @@ func TestSign(t *testing.T) { WithSignMode(signingtypes.SignMode_SIGN_MODE_DIRECT) txfAmino := txfDirect. WithSignMode(signingtypes.SignMode_SIGN_MODE_LEGACY_AMINO_JSON) - addr1, err := k1.GetAddress() - requireT.NoError(err) - addr2, err := k2.GetAddress() - requireT.NoError(err) - addr1Str, err := ac.BytesToString(addr1) - require.NoError(t, err) - addr2Str, err := ac.BytesToString(addr2) - require.NoError(t, err) - msg1 := &countertypes.MsgIncreaseCounter{Signer: addr1Str, Count: 1} - msg2 := &countertypes.MsgIncreaseCounter{Signer: addr2Str, Count: 1} + msg1 := banktypes.NewMsgSend(info1.GetAddress(), sdk.AccAddress("to"), nil) + msg2 := banktypes.NewMsgSend(info2.GetAddress(), sdk.AccAddress("to"), nil) txb, err := txfNoKeybase.BuildUnsignedTx(msg1, msg2) requireT.NoError(err) txb2, err := txfNoKeybase.BuildUnsignedTx(msg1, msg2) diff --git a/x/genutil/client/cli/gentx.go b/x/genutil/client/cli/gentx.go index 26872a359821..df3ff3edd0ba 100644 --- a/x/genutil/client/cli/gentx.go +++ b/x/genutil/client/cli/gentx.go @@ -168,12 +168,6 @@ $ %s gentx my-key-name 1000000stake --home=/path/to/home/dir --keyring-backend=o w := bytes.NewBuffer([]byte{}) clientCtx = clientCtx.WithOutput(w) - if m, ok := msg.(sdk.HasValidateBasic); ok { - if err := m.ValidateBasic(); err != nil { - return err - } - } - if err = txBldr.PrintUnsignedTx(clientCtx, msg); err != nil { return errors.Wrap(err, "failed to print unsigned std tx") }