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.

Sign up for GitHub

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
+339 −319
Open

refactor: Small optimizations #320

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
7819c47
Update account_number.go
tudorpintea999 Dec 15, 2024
d1d2925
Update sigverify.go
tudorpintea999 Dec 15, 2024
6d07c41
Update ante.go
tudorpintea999 Dec 15, 2024
52f3d1c
Update mempool.go
tudorpintea999 Dec 15, 2024
1687893
Update proposals.go
tudorpintea999 Dec 15, 2024
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
View file
Open in desktop
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")
}
Comment on lines +30 to +33
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Avoid type assertion to concrete AccountKeeper implementation

Casting accountKeeper to *authkeeper.AccountKeeper ties your code to a specific implementation, reducing flexibility and making testing harder. It's better to rely on the AccountKeeper interface or extend it if necessary.

Consider modifying the AccountKeeper interface to include the required methods or using an interface that provides access to AccountNumber.


// 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
View file
Open in desktop
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),
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