Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor: Small optimizations #320

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 27 additions & 17 deletions app/ante/accnum/account_number.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,47 +2,57 @@ package accnum

import (
storetypes "cosmossdk.io/store/types"

sdk "github.com/cosmos/cosmos-sdk/types"
cosmosante "github.com/cosmos/cosmos-sdk/x/auth/ante"
authkeeper "github.com/cosmos/cosmos-sdk/x/auth/keeper"
)

// AccountNumberDecorator is a custom ante handler that increments the account number depending on
// the execution mode (Simulate, CheckTx, Finalize).
//
// This is to avoid account number conflicts when running concurrent Simulate, CheckTx, and Finalize.
// AccountNumberDecorator is a custom AnteHandler that increments the account number
// to avoid conflicts when running concurrent Simulate, CheckTx, and Finalize operations.
type AccountNumberDecorator struct {
ak cosmosante.AccountKeeper
accountKeeper cosmosante.AccountKeeper
}

// NewAccountNumberDecorator creates a new instance of AccountNumberDecorator.
func NewAccountNumberDecorator(ak cosmosante.AccountKeeper) AccountNumberDecorator {
return AccountNumberDecorator{ak}
// NewAccountNumberDecorator creates a new AccountNumberDecorator.
func NewAccountNumberDecorator(accountKeeper cosmosante.AccountKeeper) AccountNumberDecorator {
return AccountNumberDecorator{accountKeeper: accountKeeper}
}

// AnteHandle is the AnteHandler implementation for AccountNumberDecorator.
// AnteHandle implements the AnteHandler interface.
// It increments the account number depending on the execution mode (Simulate, CheckTx).
func (and AccountNumberDecorator) AnteHandle(ctx sdk.Context, tx sdk.Tx, simulate bool, next sdk.AnteHandler) (sdk.Context, error) {
// Skip account number modification for FinalizeTx or DeliverTx.
if !ctx.IsCheckTx() && !ctx.IsReCheckTx() && !simulate {
return next(ctx, tx, simulate)
}

ak := and.ak.(*authkeeper.AccountKeeper)
// Safely cast to the concrete implementation of AccountKeeper.
authKeeper, ok := and.accountKeeper.(*authkeeper.AccountKeeper)
if !ok {
return ctx, sdk.ErrInvalidRequest.Wrap("invalid AccountKeeper type")
}
beer-1 marked this conversation as resolved.
Show resolved Hide resolved

// Create a gas-free context to interact with the account number storage.
gasFreeCtx := ctx.WithGasMeter(storetypes.NewInfiniteGasMeter())
num, err := ak.AccountNumber.Peek(gasFreeCtx)

// Peek at the current account number.
currentAccountNum, err := authKeeper.AccountNumber.Peek(gasFreeCtx)
if err != nil {
return ctx, err
return ctx, sdk.ErrInternal.Wrapf("failed to peek account number: %v", err)
}

accountNumAddition := uint64(1_000_000)
// Determine the increment value based on the execution mode.
accountNumIncrement := uint64(1_000_000)
if simulate {
accountNumAddition += 1_000_000
accountNumIncrement += 1_000_000
}

if err := ak.AccountNumber.Set(gasFreeCtx, num+accountNumAddition); err != nil {
return ctx, err
// Increment and set the account number.
newAccountNum := currentAccountNum + accountNumIncrement
if err := authKeeper.AccountNumber.Set(gasFreeCtx, newAccountNum); err != nil {
return ctx, sdk.ErrInternal.Wrapf("failed to set account number: %v", err)
}

// Proceed to the next AnteHandler.
return next(ctx, tx, simulate)
}
78 changes: 43 additions & 35 deletions app/ante/ante.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,7 @@ import (
auctionkeeper "github.com/skip-mev/block-sdk/v2/x/auction/keeper"
)

// HandlerOptions extends the SDK's AnteHandler options by requiring the IBC
// channel keeper.
// HandlerOptions extends the SDK's AnteHandler options by including IBC channel keeper and custom handlers.
type HandlerOptions struct {
ante.HandlerOptions
Codec codec.BinaryCodec
Expand All @@ -33,66 +32,75 @@ type HandlerOptions struct {
FreeLane block.Lane
}

// NewAnteHandler returns an AnteHandler that checks and increments sequence
// numbers, checks signatures & account numbers, and deducts fees from the first
// signer.
// NewAnteHandler creates a custom AnteHandler pipeline for transaction processing.
func NewAnteHandler(options HandlerOptions) (sdk.AnteHandler, error) {
if options.AccountKeeper == nil {
return nil, errors.Wrap(sdkerrors.ErrLogic, "account keeper is required for ante builder")
}

if options.BankKeeper == nil {
return nil, errors.Wrap(sdkerrors.ErrLogic, "bank keeper is required for ante builder")
// Validate mandatory dependencies.
if err := validateHandlerOptions(options); err != nil {
return nil, err
}
Comment on lines +38 to 40
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

⚠️ Potential issue

Validate all required dependencies in validateHandlerOptions

Currently, validateHandlerOptions checks for AccountKeeper, BankKeeper, and SignModeHandler. However, additional dependencies like ExtensionOptionChecker, FeegrantKeeper, IBCkeeper, AuctionKeeper, TxEncoder, MoveKeeper, MevLane, and FreeLane are used later in the code without prior validation. To prevent potential nil pointer dereferences, consider adding validation for these dependencies.


if options.SignModeHandler == nil {
return nil, errors.Wrap(sdkerrors.ErrLogic, "sign mode handler is required for ante builder")
}

sigGasConsumer := options.SigGasConsumer
if sigGasConsumer == nil {
sigGasConsumer = sigverify.DefaultSigVerificationGasConsumer
}

txFeeChecker := options.TxFeeChecker
if txFeeChecker == nil {
txFeeChecker = moveante.NewMempoolFeeChecker(options.MoveKeeper).CheckTxFeeWithMinGasPrices
}
// Default to provided or custom fee checker.
txFeeChecker := getTxFeeChecker(options)

// Define a custom free lane fee checker.
freeLaneFeeChecker := func(ctx sdk.Context, tx sdk.Tx) (sdk.Coins, int64, error) {
// skip fee checker if the tx is free lane tx.
if !options.FreeLane.Match(ctx, tx) {
return txFeeChecker(ctx, tx)
}

// return fee without fee check
feeTx, ok := tx.(sdk.FeeTx)
if !ok {
return nil, 0, errors.Wrap(sdkerrors.ErrTxDecode, "Tx must be a FeeTx")
}

return feeTx.GetFee(), 1 /* FIFO */, nil
}

anteDecorators := []sdk.AnteDecorator{
// Create the AnteDecorators sequence.
anteDecorators := buildAnteDecorators(options, freeLaneFeeChecker)

// Chain the AnteDecorators to construct the AnteHandler.
return sdk.ChainAnteDecorators(anteDecorators...), nil
}

// validateHandlerOptions ensures all mandatory dependencies are provided.
func validateHandlerOptions(options HandlerOptions) error {
if options.AccountKeeper == nil {
return errors.Wrap(sdkerrors.ErrLogic, "account keeper is required for ante builder")
}
if options.BankKeeper == nil {
return errors.Wrap(sdkerrors.ErrLogic, "bank keeper is required for ante builder")
}
if options.SignModeHandler == nil {
return errors.Wrap(sdkerrors.ErrLogic, "sign mode handler is required for ante builder")
}
return nil
}

// getTxFeeChecker returns the appropriate TxFeeChecker function.
func getTxFeeChecker(options HandlerOptions) func(sdk.Context, sdk.Tx) (sdk.Coins, int64, error) {
if options.TxFeeChecker != nil {
return options.TxFeeChecker
}
return moveante.NewMempoolFeeChecker(options.MoveKeeper).CheckTxFeeWithMinGasPrices
}
Comment on lines +83 to +84
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

⚠️ Potential issue

Check for nil MoveKeeper in getTxFeeChecker

In getTxFeeChecker, options.MoveKeeper is used without checking if it's nil. If MoveKeeper is nil, this will cause a nil pointer dereference at runtime. Ensure that MoveKeeper is validated in validateHandlerOptions.


// buildAnteDecorators constructs the list of AnteDecorators in the correct order.
func buildAnteDecorators(options HandlerOptions, freeLaneFeeChecker func(sdk.Context, sdk.Tx) (sdk.Coins, int64, error)) []sdk.AnteDecorator {
return []sdk.AnteDecorator{
accnum.NewAccountNumberDecorator(options.AccountKeeper),
ante.NewSetUpContextDecorator(), // outermost AnteDecorator. SetUpContext must be called first
ante.NewSetUpContextDecorator(), // Must be the first decorator.
ante.NewExtensionOptionsDecorator(options.ExtensionOptionChecker),
moveante.NewGasPricesDecorator(),
ante.NewValidateBasicDecorator(),
ante.NewTxTimeoutHeightDecorator(),
ante.NewValidateMemoDecorator(options.AccountKeeper),
ante.NewConsumeGasForTxSizeDecorator(options.AccountKeeper),
ante.NewDeductFeeDecorator(options.AccountKeeper, options.BankKeeper, options.FeegrantKeeper, freeLaneFeeChecker),
Comment on lines 96 to 97
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

⚠️ Potential issue

Validate FeegrantKeeper before usage

In line 97, NewDeductFeeDecorator is called with options.FeegrantKeeper. If FeegrantKeeper is nil, this could result in a runtime error. Add a check in validateHandlerOptions to ensure FeegrantKeeper is not nil.

// SetPubKeyDecorator must be called before all signature verification decorators
ante.NewSetPubKeyDecorator(options.AccountKeeper),
ante.NewSetPubKeyDecorator(options.AccountKeeper), // Must be called before signature verification.
ante.NewValidateSigCountDecorator(options.AccountKeeper),
ante.NewSigGasConsumeDecorator(options.AccountKeeper, sigGasConsumer),
ante.NewSigGasConsumeDecorator(options.AccountKeeper, sigverify.DefaultSigVerificationGasConsumer),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you change sigGasConsumer to retrieve from options first?

sigverify.NewSigVerificationDecorator(options.AccountKeeper, options.SignModeHandler),
ante.NewIncrementSequenceDecorator(options.AccountKeeper),
ibcante.NewRedundantRelayDecorator(options.IBCkeeper),
auctionante.NewAuctionDecorator(options.AuctionKeeper, options.TxEncoder, options.MevLane),
}

return sdk.ChainAnteDecorators(anteDecorators...), nil
}
Loading