refactor: Small optimizations #320
Conversation
Renamed variables for clarity (e.g., ak → authKeeper, num → currentAccountNum). • Added more descriptive comments for each step of the process. • Provided more informative error messages using sdk.ErrInternal and sdk.ErrInvalidRequest. • Added a type assertion with a fallback to handle potential incorrect types for AccountKeeper. • Highlighted the rationale for using a gas-free context when interacting with the account number. • Clearly defined the increment value (1_000_000) and its variation during simulation to make the logic easier to understand. • Retained flexibility to handle additional modes by centralizing the execution mode check.
Broke down large functions (AnteHandle) into smaller, focused helper methods (verifySignature, performSignatureVerification, etc.). • Clearer variable names (sig → signature, acc → account). • Simplified nested logic by early returns and helpers. • Added context-specific error messages for better debugging. • Ensured consistency in gas calculation logic. • Centralized repeated operations into helper functions (e.g., performSignatureVerification). • Modularized logic for easier testing and extension.
validateHandlerOptions: Validates that all required dependencies are provided. • getTxFeeChecker: Retrieves the appropriate fee checker. • buildAnteDecorators: Constructs the AnteDecorator chain. • Grouped related operations together for easier understanding. • Enhanced the readability of the free lane fee checker by clarifying its logic. • Kept the design flexible by accommodating custom fee checkers (TxFeeChecker) and defaulting to a predefined one if not provided.
Added validateMempoolConfig to encapsulate configuration validation. • Centralized validation logic improves readability and reuse. • Replaced nested if-else statements with if checks and early returns. • Encapsulated logic in modular helper functions (getTxKey, AssertLaneLimits) to reduce code duplication. • Provided hooks to customize transaction priority (Priority) and validation logic. • Optimized Contains and Remove to avoid redundant checks by using getTxKey.
Created helper functions like validateTxEligibility, validateTxPriority, and handleNonMatchingTransactions to reduce code duplication and enhance clarity. • Encapsulated repeated logging and transaction removal logic into logAndAppend and logTxLimitExceeded. • Used descriptive variable names (txsToInclude, txsToRemove) for better understanding.
📝 WalkthroughWalkthroughThis pull request introduces comprehensive improvements across several core modules of the blockchain application, focusing on code readability, error handling, and modular design. The changes span the ante handler, account number management, signature verification, mempool, and proposal handling components. Key modifications include renaming variables for clarity, extracting complex logic into separate methods, enhancing error messages, and refactoring validation processes. The overall goal appears to be improving code maintainability and providing more robust transaction processing mechanisms. Changes
Sequence DiagramsequenceDiagram
participant TX as Transaction
participant Ante as AnteHandler
participant AccNum as AccountNumberDecorator
participant SigVerify as SigVerificationDecorator
participant Mempool as Mempool
TX->>Ante: Submit Transaction
Ante->>AccNum: Validate Account Number
AccNum-->>Ante: Account Number Validated
Ante->>SigVerify: Verify Signatures
SigVerify-->>Ante: Signatures Verified
Ante->>Mempool: Insert Transaction
Mempool-->>Ante: Transaction Inserted
Possibly Related PRs
Suggested Reviewers
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🔭 Outside diff range comments (1)
app/lanes/mempool.go (1)
Line range hint
33-70: Consider adding validation for the extractor parameter.While the validation logic is well-organized and comprehensive for ratio and txEncoder, the extractor parameter should also be validated to ensure it's not nil, as it's a critical component for transaction processing.
func validateMempoolConfig(ratio math.LegacyDec, txEncoder sdk.TxEncoder) error { if !ratio.IsPositive() { return errors.New("mempool creation: ratio must be positive") } if ratio.GT(math.LegacyOneDec()) { return errors.New("mempool creation: ratio must be less than or equal to 1") } if txEncoder == nil { return errors.New("mempool creation: tx encoder is nil") } return nil } func NewMempool[C comparable]( txPriority blockbase.TxPriority[C], extractor signer_extraction.Adapter, maxTx int, ratio math.LegacyDec, txEncoder sdk.TxEncoder, ) (*Mempool[C], error) { + if extractor == nil { + return nil, errors.New("mempool creation: signer extractor is nil") + } if err := validateMempoolConfig(ratio, txEncoder); err != nil { return nil, err }
🧹 Nitpick comments (5)
app/lanes/proposals.go (3)
165-167: Avoid loggingnilerrors inlogAndAppendIn
logAndAppend, the error is logged even whenerrisnil, which could lead to misleading log entries. Check iferris notnilbefore including it in the log.Apply this diff to conditionally log the error:
func (h *DefaultProposalHandler) logAndAppend(message string, tx sdk.Tx, err error, txsToRemove *[]sdk.Tx) { - h.lane.Logger().Info(message, "err", err) + if err != nil { + h.lane.Logger().Info(message, "err", err) + } else { + h.lane.Logger().Info(message) + } *txsToRemove = append(*txsToRemove, tx) }
142-144: Correct the transaction priority error messageThe error message in
validateTxPrioritymight be misleading. Whenpriority == -1, it means the current transaction has lower priority than the previous one. The message should reflect that to avoid confusion.Update the error message as follows:
return fmt.Errorf("tx at index %d has lower priority than tx at index %d", index, index-1)
122-124: Ensure transactions exceeding size limits are handled consistentlyIn
validateTxEligibility, when a transaction causesupdatedSizeto exceedlimit.MaxTxBytes, it logs the issue but does not append the transaction totxsToRemove. This could result in the transaction being reconsidered in future proposals. Consider appending the transaction totxsToRemovefor consistency.Apply this diff:
return false + *txsToRemove = append(*txsToRemove, tx)app/ante/accnum/account_number.go (1)
51-53: Handle potential errors when setting the new account numberIn
AnteHandle, when setting the new account number usingauthKeeper.AccountNumber.Set, you wrap the error withsdk.ErrInternal. Sincesdk.ErrInternalis deprecated, consider usingsdkerrors.ErrInvalidRequestor another appropriate error type fromsdkerrors.Update the error handling as follows:
if err := authKeeper.AccountNumber.Set(gasFreeCtx, newAccountNum); err != nil { - return ctx, sdk.ErrInternal.Wrapf("failed to set account number: %v", err) + return ctx, sdkerrors.Wrapf(sdkerrors.ErrInvalidRequest, "failed to set account number: %v", err) }app/lanes/mempool.go (1)
Line range hint
155-182: Consider extracting constants and error messages.The validation logic is solid, but would benefit from better organization:
- Define error messages as constants
- Consider creating a separate type for lane limits configuration
+const ( + ErrTxSizeExceeded = "transaction size %d exceeds max lane size %d" + ErrTxGasLimitExceeded = "transaction gas limit %d exceeds max lane gas limit %d" + ErrNotFeeTx = "transaction does not implement FeeTx interface" +) +type LaneLimits struct { + MaxTxSize int64 + MaxGasLimit uint64 +} func (cm *Mempool[C]) AssertLaneLimits(ctx sdk.Context, tx sdk.Tx) error { maxBlockSize, maxGasLimit := proposals.GetBlockLimits(ctx) - maxLaneTxSize := cm.ratio.MulInt64(maxBlockSize).TruncateInt64() - maxLaneGasLimit := cm.ratio.MulInt(math.NewIntFromUint64(maxGasLimit)).TruncateInt().Uint64() + limits := LaneLimits{ + MaxTxSize: cm.ratio.MulInt64(maxBlockSize).TruncateInt64(), + MaxGasLimit: cm.ratio.MulInt(math.NewIntFromUint64(maxGasLimit)).TruncateInt().Uint64(), + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
app/ante/accnum/account_number.go(1 hunks)app/ante/ante.go(2 hunks)app/ante/sigverify/sigverify.go(2 hunks)app/lanes/mempool.go(3 hunks)app/lanes/proposals.go(3 hunks)
🔇 Additional comments (4)
app/ante/ante.go (1)
90-90: 🛠️ Refactor suggestion
Ensure NewSetUpContextDecorator is the first decorator
According to Cosmos SDK best practices, NewSetUpContextDecorator must be the first decorator in the AnteHandler chain. Currently, it's placed after accnum.NewAccountNumberDecorator. To maintain the correct execution order, NewSetUpContextDecorator should be the first decorator.
Apply this diff to reorder the decorators:
func buildAnteDecorators(options HandlerOptions, freeLaneFeeChecker func(sdk.Context, sdk.Tx) (sdk.Coins, int64, error)) []sdk.AnteDecorator {
return []sdk.AnteDecorator{
+ ante.NewSetUpContextDecorator(), // Must be the first decorator.
accnum.NewAccountNumberDecorator(options.AccountKeeper),
- ante.NewSetUpContextDecorator(), // Must be the first decorator.
ante.NewExtensionOptionsDecorator(options.ExtensionOptionChecker),
moveante.NewGasPricesDecorator(),
// ... remaining decorators
}
}Likely invalid or redundant comment.
app/ante/sigverify/sigverify.go (1)
90-98: Handle missing pubKey in simulation and ReCheckTx
In simulation or ReCheckTx modes, if pubKey is nil, signature verification is skipped. Ensure that this behavior is intended and that it doesn't lead to security issues by allowing transactions without public keys in certain modes.
app/lanes/mempool.go (2)
23-29: LGTM! Well-structured struct definition with clear documentation.
The Mempool struct is well-organized with appropriate field types and clear documentation for each component. The use of generics with the comparable constraint is a good choice for type safety.
143-152: 🛠️ Refactor suggestion
Reconsider single-signer assumption in getTxKey.
The current implementation only uses the first signer for indexing, which might not be sufficient for multi-signer transactions. This could lead to unexpected behavior when transactions have multiple signers with different sequences.
Let's verify if multi-signer transactions are common in the codebase:
Consider either:
- Creating composite keys that include all signers
- Documenting the limitation and rationale for using only the first signer
✅ Verification successful
Based on the search results, I can now provide a final response:
Using first signer for indexing is appropriate in this context.
The current implementation's approach of using the first signer for indexing is actually correct and consistent with the codebase's design:
- The mempool's role is transaction ordering and basic validation, not signature verification
- Full signature verification happens in the ante handler (
app/ante/sigverify/), which properly validates all signers - The mempool key is used for deduplication and ordering, where the first signer's sequence number is sufficient
The code shows a clear separation of concerns:
- Mempool: Basic indexing and ordering using first signer
- Ante handler: Complete signature verification for all signers
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for multi-signer transaction patterns
ast-grep --pattern 'GetSigners($$$)' | grep -B 2 -A 2 'Signers'
Length of output: 63
Script:
#!/bin/bash
# Let's try a different approach to find multi-signer related code
# First, let's look for GetSigners method implementations
rg -A 5 "func.*GetSigners"
# Also search for any mentions of multiple signers in comments or code
rg -i "multi.*signer|multiple.*signer"
# Look for test cases that might involve multiple signers
rg "test.*signer" -A 5
Length of output: 65851
Script:
#!/bin/bash
# Let's search for any code or comments related to transaction signers
# Search for files containing GetSigners implementation
rg -p "func.*GetSigners"
# Look for any test files or code related to transaction signing
rg -p "type.*Tx.*interface" -A 5
# Search for any comments or documentation about transaction signing
rg -i "transaction.*sign|tx.*sign"
Length of output: 65634
Script:
#!/bin/bash
# Let's search for transaction signing related code in the app directory
rg -p "type.*Signer" app/
rg -p "interface.*Signer" app/
rg -p "struct.*Signer" app/
# Look for any documentation or comments about transaction signing
rg -i "transaction.*signer|tx.*signer" app/ -A 3
Length of output: 4176
| if err := validateHandlerOptions(options); err != nil { | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
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.
| return moveante.NewMempoolFeeChecker(options.MoveKeeper).CheckTxFeeWithMinGasPrices | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
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.
| ante.NewConsumeGasForTxSizeDecorator(options.AccountKeeper), | ||
| ante.NewDeductFeeDecorator(options.AccountKeeper, options.BankKeeper, options.FeegrantKeeper, freeLaneFeeChecker), |
There was a problem hiding this comment.
🛠️ Refactor suggestion
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.
| return consumeMultisignatureVerificationGas(meter, sig.Data.(*signing.MultiSignatureData), pubkey, params, sig.Sequence) | ||
|
|
||
| default: |
There was a problem hiding this comment.
Ensure safe type assertion for multisignature data
In DefaultSigVerificationGasConsumer, when handling multisignature public keys, you assert that sig.Data is of type *signing.MultiSignatureData without checking. To prevent potential panics, ensure the type assertion is safe or handle the error appropriately.
Apply this diff to safely handle the type assertion:
case multisig.PubKey:
- return consumeMultisignatureVerificationGas(meter, sig.Data.(*signing.MultiSignatureData), pubkey, params, sig.Sequence)
+ multiSigData, ok := sig.Data.(*signing.MultiSignatureData)
+ if !ok {
+ return errorsmod.Wrap(sdkerrors.ErrInvalidType, "expected MultiSignatureData for multisig public key")
+ }
+ return consumeMultisignatureVerificationGas(meter, multiSigData, pubkey, params, sig.Sequence)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| return consumeMultisignatureVerificationGas(meter, sig.Data.(*signing.MultiSignatureData), pubkey, params, sig.Sequence) | |
| default: | |
| multiSigData, ok := sig.Data.(*signing.MultiSignatureData) | |
| if !ok { | |
| return errorsmod.Wrap(sdkerrors.ErrInvalidType, "expected MultiSignatureData for multisig public key") | |
| } | |
| return consumeMultisignatureVerificationGas(meter, multiSigData, pubkey, params, sig.Sequence) | |
| default: |
| authKeeper, ok := and.accountKeeper.(*authkeeper.AccountKeeper) | ||
| if !ok { | ||
| return ctx, sdk.ErrInvalidRequest.Wrap("invalid AccountKeeper type") | ||
| } |
There was a problem hiding this comment.
🛠️ 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.
| // Priority returns the transaction priority. | ||
| func (cm *Mempool[C]) Priority(ctx sdk.Context, tx sdk.Tx) any { | ||
| return 1 | ||
| return 1 // Fixed priority for now; extend as needed. |
There was a problem hiding this comment.
Priority implementation needs improvement.
The current implementation returns a constant value of 1, which may not provide optimal transaction ordering. Consider implementing a more sophisticated priority mechanism based on factors like:
- Transaction fees
- Gas prices
- Account sequence
- Transaction type
Would you like help implementing a more comprehensive priority calculation?
| // Contains checks whether a transaction exists in the mempool. | ||
| func (cm *Mempool[C]) Contains(tx sdk.Tx) bool { | ||
| if key, err := cm.getTxKey(tx); err != nil { | ||
| key, err := cm.getTxKey(tx) | ||
| if err != nil { | ||
| return false | ||
| } else { | ||
| if _, ok := cm.txCache[key]; ok { | ||
| return true | ||
| } else { | ||
| return false | ||
| } | ||
| } | ||
| _, exists := cm.txCache[key] | ||
| return exists | ||
| } | ||
|
|
||
| // Insert inserts a transaction into the mempool. | ||
| // Insert adds a transaction to the mempool after validating lane limits. | ||
| func (cm *Mempool[C]) Insert(ctx context.Context, tx sdk.Tx) error { | ||
| if err := cm.AssertLaneLimits(sdk.UnwrapSDKContext(ctx), tx); err != nil { | ||
| sdkCtx := sdk.UnwrapSDKContext(ctx) | ||
|
|
||
| // Validate lane limits. | ||
| if err := cm.AssertLaneLimits(sdkCtx, tx); err != nil { | ||
| return err | ||
| } | ||
|
|
||
| // Insert into the underlying priority mempool. | ||
| if err := cm.index.Insert(ctx, tx); err != nil { | ||
| return fmt.Errorf("failed to insert tx into auction index: %w", err) | ||
| } | ||
|
|
||
| if key, err := cm.getTxKey(tx); err != nil { | ||
| // Cache the transaction. | ||
| key, err := cm.getTxKey(tx) | ||
| if err != nil { | ||
| return err | ||
| } else { | ||
| cm.txCache[key] = struct{}{} | ||
| } | ||
| cm.txCache[key] = struct{}{} | ||
|
|
||
| return nil | ||
| } | ||
|
|
||
| // Remove removes a transaction from the mempool. | ||
| // Remove deletes a transaction from the mempool and its cache. | ||
| func (cm *Mempool[C]) Remove(tx sdk.Tx) error { | ||
| // Remove from the priority mempool. | ||
| if err := cm.index.Remove(tx); err != nil && !errors.Is(err, sdkmempool.ErrTxNotFound) { | ||
| return fmt.Errorf("failed to remove transaction from the mempool: %w", err) | ||
| } | ||
|
|
||
| if key, err := cm.getTxKey(tx); err != nil { | ||
| // Remove from the cache. | ||
| key, err := cm.getTxKey(tx) | ||
| if err != nil { | ||
| return err | ||
| } else { | ||
| delete(cm.txCache, key) | ||
| } | ||
| delete(cm.txCache, key) | ||
|
|
||
| return nil | ||
| } |
There was a problem hiding this comment.
Add mutex protection for thread-safe cache operations.
The txCache map is accessed from multiple methods (Contains, Insert, Remove) without synchronization. This could lead to race conditions in a concurrent environment.
type Mempool[C comparable] struct {
index sdkmempool.Mempool
extractor signer_extraction.Adapter
txCache map[txKey]struct{}
ratio math.LegacyDec
txEncoder sdk.TxEncoder
+ mu sync.RWMutex // Protects txCache
}
func (cm *Mempool[C]) Contains(tx sdk.Tx) bool {
key, err := cm.getTxKey(tx)
if err != nil {
return false
}
+ cm.mu.RLock()
+ defer cm.mu.RUnlock()
_, exists := cm.txCache[key]
return exists
}Committable suggestion skipped: line range outside the PR's diff.
Hello Initia team!
I have refactored a small part of your code
Not much but it s honest work and hope it helps you
Br,
Tudor.