From 8800d2e477b2e6913262b3c7f43d19529aaad171 Mon Sep 17 00:00:00 2001 From: Amaury <1293565+amaurym@users.noreply.github.com> Date: Wed, 6 Apr 2022 13:56:01 +0200 Subject: [PATCH] refactor: Set defaults in middlewares instead of `NewDefaultTxHandler` (#11557) ## Description If tx handler `options.{ExtensionOptionChecker, TxFeeChecker}` is not set, we fallback to a default option provided by the SDK. These default are set in 2 places, which is redundant: - once in `NewDefaultTxHandler` - the second time in the middleware constructor Proposing here to only set the default once, inside each middleware constructor --- ### 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) --- x/auth/middleware/middleware.go | 21 +++------------------ x/auth/middleware/sigverify.go | 4 ++++ 2 files changed, 7 insertions(+), 18 deletions(-) diff --git a/x/auth/middleware/middleware.go b/x/auth/middleware/middleware.go index a3fb30b0d6..812bd96038 100644 --- a/x/auth/middleware/middleware.go +++ b/x/auth/middleware/middleware.go @@ -71,21 +71,6 @@ func NewDefaultTxHandler(options TxHandlerOptions) (tx.Handler, error) { return nil, sdkerrors.Wrap(sdkerrors.ErrLogic, "sign mode handler is required for middlewares") } - var sigGasConsumer = options.SigGasConsumer - if sigGasConsumer == nil { - sigGasConsumer = DefaultSigVerificationGasConsumer - } - - var extensionOptionChecker = options.ExtensionOptionChecker - if extensionOptionChecker == nil { - extensionOptionChecker = rejectExtensionOption - } - - var txFeeChecker = options.TxFeeChecker - if txFeeChecker == nil { - txFeeChecker = checkTxFeeWithValidatorMinGasPrices - } - return ComposeMiddlewares( NewRunMsgsTxHandler(options.MsgServiceRouter, options.LegacyRouter), NewTxDecoderMiddleware(options.TxDecoder), @@ -102,7 +87,7 @@ func NewDefaultTxHandler(options TxHandlerOptions) (tx.Handler, error) { // emitted outside of this middleware. NewIndexEventsTxMiddleware(options.IndexEvents), // Reject all extension options other than the ones needed by the feemarket. - NewExtensionOptionsMiddleware(extensionOptionChecker), + NewExtensionOptionsMiddleware(options.ExtensionOptionChecker), ValidateBasicMiddleware, TxTimeoutHeightMiddleware, ValidateMemoMiddleware(options.AccountKeeper), @@ -111,10 +96,10 @@ func NewDefaultTxHandler(options TxHandlerOptions) (tx.Handler, error) { // ComposeMiddlewares godoc for details. // `DeductFeeMiddleware` and `IncrementSequenceMiddleware` should be put outside of `WithBranchedStore` middleware, // so their storage writes are not discarded when tx fails. - DeductFeeMiddleware(options.AccountKeeper, options.BankKeeper, options.FeegrantKeeper, txFeeChecker), + DeductFeeMiddleware(options.AccountKeeper, options.BankKeeper, options.FeegrantKeeper, options.TxFeeChecker), SetPubKeyMiddleware(options.AccountKeeper), ValidateSigCountMiddleware(options.AccountKeeper), - SigGasConsumeMiddleware(options.AccountKeeper, sigGasConsumer), + SigGasConsumeMiddleware(options.AccountKeeper, options.SigGasConsumer), SigVerificationMiddleware(options.AccountKeeper, options.SignModeHandler), IncrementSequenceMiddleware(options.AccountKeeper), // Creates a new MultiStore branch, discards downstream writes if the downstream returns error. diff --git a/x/auth/middleware/sigverify.go b/x/auth/middleware/sigverify.go index 6189546df0..0e13b4142a 100644 --- a/x/auth/middleware/sigverify.go +++ b/x/auth/middleware/sigverify.go @@ -299,6 +299,10 @@ type sigGasConsumeTxHandler struct { // CONTRACT: Pubkeys are set in context for all signers before this middleware runs // CONTRACT: Tx must implement SigVerifiableTx interface func SigGasConsumeMiddleware(ak AccountKeeper, sigGasConsumer SignatureVerificationGasConsumer) tx.Middleware { + if sigGasConsumer == nil { + sigGasConsumer = DefaultSigVerificationGasConsumer + } + return func(h tx.Handler) tx.Handler { return sigGasConsumeTxHandler{ ak: ak,