refactor(baseapp): create checktx handler #21979
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThe changes involve significant updates to the Cosmos SDK, focusing on transaction handling and validation. Key modifications include the introduction of new methods and type aliases for transaction processing, updates to existing method signatures for improved context handling, and enhancements to the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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
Documentation and Community
|
tac0turtle
force-pushed
the
marko/checktx_handler
branch
from
7f5209c
to
c71365c
Compare
|
there is not a clean way to do this in order to achieve it for cometmempool lanes. |
tac0turtle
requested review from
a team,
facundomedica,
aaronc,
testinginprod and
kocubinski
as code owners
julienrbrt
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (20)
docs/build/abci/04-checktx.md (2)
17-19: Consider adding a brief description to the code referenceWhile the GitHub link is helpful, it would be more informative to include a brief description of what the linked code contains. This would give readers a better understanding of what to expect when they follow the link.
Consider adding a line like:
The following link points to the CheckTx implementation in the Cosmos SDK BaseApp:
33-50: Enhance the example usage sectionWhile the example is helpful, it could be improved in the following ways:
- Provide more context about where in the
NewSimAppfunction this code should be placed.- Explain what arguments should be passed to
abci.NewCustomCheckTxHandler(...).- Consider adding a brief comment explaining the purpose of
SetCheckTxHandler.These additions would make the example more comprehensive and easier to understand for developers implementing a custom CheckTxHandler.
Consider updating the example as follows:
func NewSimApp( logger log.Logger, db corestore.KVStoreWithBatch, traceStore io.Writer, loadLatest bool, appOpts servertypes.AppOptions, baseAppOptions ...func(*baseapp.BaseApp), ) *SimApp { // ... (other initialization code) // Create ChecktxHandler // Replace the ellipsis with appropriate arguments based on your application's needs checktxHandler := abci.NewCustomCheckTxHandler(app.txConfig, app.GetAnteHandler(), app.GetPostHandler()) // Set the custom CheckTxHandler for the application app.SetCheckTxHandler(checktxHandler) // ... (remaining initialization code) }baseapp/test_helpers.go (4)
22-22: LGTM! Consider enhancing error handling.The addition of the
txparameter to therunTxcall aligns well with the PR objectives of simplifying the checktx handler design. This change allows for more contextual information to be passed to therunTxmethod, which can be beneficial for custom checktx logic implementation.Consider wrapping the error returned from
runTxwith additional context:gasInfo, result, _, err := app.runTx(execModeCheck, bz, tx) if err != nil { return sdk.GasInfo{}, nil, errorsmod.Wrap(err, "failed to run transaction in check mode") }This would provide more informative error messages for debugging purposes.
28-28: LGTM! Consider adding a comment for clarity.The modification to pass
nilas the third argument torunTxis consistent with the function's signature and maintains alignment with the changes in other functions.Consider adding a brief comment to explain why
nilis passed:// Pass nil as the tx parameter since Simulate operates on raw transaction bytes gasInfo, result, _, err := app.runTx(execModeSimulate, txBytes, nil)This would improve code readability and make the intention behind passing
nilmore explicit.
39-39: LGTM! Consider enhancing error handling.The addition of the
txparameter to therunTxcall is consistent with the changes inSimCheckand aligns with the PR objectives. This modification allows for more contextual information to be passed to therunTxmethod during transaction delivery simulation.Similar to the suggestion for
SimCheck, consider wrapping the error returned fromrunTxwith additional context:gasInfo, result, _, err := app.runTx(execModeFinalize, bz, tx) if err != nil { return sdk.GasInfo{}, nil, errorsmod.Wrap(err, "failed to run transaction in finalize mode") }This would provide more informative error messages for debugging purposes.
Line range hint
1-73: Overall assessment: Changes align well with PR objectivesThe modifications to
SimCheck,Simulate, andSimDeliverfunctions consistently implement the addition of thetxparameter to therunTxmethod calls. These changes align well with the PR objectives of simplifying the checktx handler design and enhancing its extensibility.The refactoring allows for more contextual information to be passed during transaction processing, which should facilitate the implementation of custom checktx logic and potentially improve integration with Comet mempool lanes.
While the changes themselves look good, it's worth noting the concern raised in the PR comments about the complexity of implementing the desired changes for Comet mempool lanes. This might require further discussion or refinement in future iterations.
To address the complexity concerns mentioned in the PR comments, consider the following steps:
- Document the new design and its integration points clearly.
- Create examples or templates for common use cases of custom checktx logic.
- Consider creating a separate issue to track and address the complexities related to Comet mempool lanes integration.
types/abci.go (2)
26-28: LGTM with a minor suggestion for the commentThe
CheckTxHandlertype definition looks good and follows the Uber Golang style guide. The function type alias enhances readability and maintainability.Consider slightly modifying the comment to provide more context:
-// CheckTxHandler defines a function type alias for executing logic before transactions are executed. +// CheckTxHandler defines a function type alias for handling CheckTx requests and executing logic before transactions are added to the mempool.This change clarifies the specific stage in the transaction lifecycle where this handler is used.
30-31: LGTM with a suggestion to enhance the commentThe
RunTxtype definition is well-structured and follows the Uber Golang style guide. The function type alias improves code readability and maintainability.Consider expanding the comment to provide more specific information about the function's role and its return values:
-// RunTx defines a function type alias for executing logic before transactions are executed. +// RunTx defines a function type alias for executing pre-processing logic on transactions. +// It returns gas information, execution result, events, and any error encountered during processing.This change offers more context about the function's purpose and its return values, which can be helpful for developers using this type.
docs/build/building-apps/02-app-mempool.md (3)
Line range hint
40-48: LGTM: Clear explanation of No-op Mempool with a suggestionThe explanation of the No-op Mempool is clear and concise. The note about PrepareProposal and ProcessProposal awareness is crucial for developers to understand potential implications.
Consider adding a brief example or use case where a No-op Mempool might be preferred to help developers better understand when to use this option.
Line range hint
50-71: LGTM: Comprehensive explanation of Sender Nonce MempoolThe explanation of the Sender Nonce Mempool is clear and provides good detail on its functionality and configurable parameters (MaxTxs and Seed).
Consider adding a brief explanation of the benefits or use cases for the Sender Nonce Mempool to help developers understand when to choose this option over others.
Line range hint
73-101: LGTM: Detailed explanation of Priority Nonce Mempool with a suggestionThe explanation of the Priority Nonce Mempool is detailed and provides good information on its implementation and configuration. The link to more detailed documentation is helpful for developers seeking additional information.
Consider adding a brief comparison between the Sender Nonce Mempool and the Priority Nonce Mempool to help developers understand the key differences and when to choose one over the other.
baseapp/options.go (1)
370-377: LGTM with a minor suggestion.The new
SetCheckTxHandlermethod is well-implemented and follows the consistent pattern of other setter methods in the file. It correctly checks if the BaseApp is sealed before setting the handler.Consider updating the panic message for consistency with other methods:
- panic("SetCheckTx() on sealed BaseApp") + panic("SetCheckTxHandler() on sealed BaseApp")This change would make the error message more accurate and consistent with the method name.
CHANGELOG.md (7)
Line range hint
5-8: Consider adding more context to breaking changesThe breaking changes section provides a good overview, but it might be helpful to add more context or examples for some of the changes. For instance, the impact of removing
LegacyMsginterface could be explained further.
Line range hint
10-14: Highlight significant API changesThe API Breaking Changes section is comprehensive, but it might be beneficial to highlight the most significant changes that are likely to affect a large number of users. Consider adding a brief summary at the beginning of this section.
Line range hint
22-27: Consider grouping related CLI changesThe CLI Breaking Changes section is informative, but it might be more readable if related changes are grouped together. For example, changes related to the same command or module could be listed consecutively.
Line range hint
36-41: Consider prioritizing improvementsThe Improvements section is extensive, which is great. However, consider highlighting or prioritizing the most impactful improvements at the beginning of the section to draw attention to them.
Line range hint
50-55: Consider adding migration guides for major changesFor some of the more significant changes, especially in the State Machine Breaking section, consider adding links to migration guides or more detailed documentation to help users update their applications.
Line range hint
64-69: Consider adding examples for new featuresIn the Features section, consider adding brief examples or use cases for some of the more complex new features. This could help users understand how to implement these features in their own applications.
Line range hint
78-83: Consider adding links to related issues or PRsFor some of the bug fixes and improvements, consider adding links to the related GitHub issues or pull requests. This can provide additional context for users who want to understand the changes in more detail.
baseapp/baseapp.go (1)
825-825: UpdaterunTxfunction comment to include the newtxparameterThe function comment for
runTxshould be updated to describe the newtx sdk.Txparameter and its purpose, ensuring the documentation remains accurate.
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (8)
- CHANGELOG.md (1 hunks)
- baseapp/abci.go (1 hunks)
- baseapp/baseapp.go (7 hunks)
- baseapp/options.go (1 hunks)
- baseapp/test_helpers.go (2 hunks)
- docs/build/abci/04-checktx.md (1 hunks)
- docs/build/building-apps/02-app-mempool.md (1 hunks)
- types/abci.go (1 hunks)
🧰 Additional context used
📓 Path-based instructions (8)
CHANGELOG.md (1)
Pattern
**/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"baseapp/abci.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.baseapp/baseapp.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.baseapp/options.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.baseapp/test_helpers.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.docs/build/abci/04-checktx.md (1)
Pattern
**/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"docs/build/building-apps/02-app-mempool.md (1)
Pattern
**/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"types/abci.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
🔇 Additional comments (14)
docs/build/abci/04-checktx.md (2)
1-15: LGTM: Clear introduction and informative flowchartThe introduction effectively explains the purpose of CheckTx, and the flowchart provides a clear visual representation of the process. The content is well-structured and easy to understand.
21-31: LGTM: Clear explanation of CheckTxHandlerThis section effectively introduces the CheckTxHandler, its purpose, and provides a helpful note about performance optimization. The type definition is correctly presented, and the explanation is clear and concise.
docs/build/building-apps/02-app-mempool.md (2)
Line range hint
25-38: LGTM: Clear introduction and overview of mempool conceptsThe introduction effectively provides context about the changes in v0.47 and the introduction of ABCI 1.0. The overview of the three mempool types (No-op, Sender Nonce, and Priority Nonce) is concise and informative, giving readers a good starting point for understanding the available options.
Line range hint
1-101: LGTM: Comprehensive and well-structured documentation on application mempoolThis document provides a clear and comprehensive overview of the application mempool in Cosmos SDK. It effectively covers the three types of mempools (No-op, Sender Nonce, and Priority Nonce) with detailed explanations and configuration options for each. The content is well-structured, free of grammatical or spelling errors, and aligns well with the PR objectives of simplifying the design of the checktx handler and enhancing its extensibility.
The document successfully achieves its goal of providing developers with a better understanding of the available mempool options and their configurations. This will greatly assist in implementing custom checktx logic and working with mempool lanes in Comet.
baseapp/options.go (1)
370-377: Summary: New SetCheckTxHandler method enhances BaseApp configurability.The addition of the
SetCheckTxHandlermethod aligns well with the PR objectives to simplify the design of the checktx handler. This new method follows the established patterns in the file and enhances the configurability of the BaseApp by allowing custom transaction checking logic to be set.This change contributes to making it easier for developers to implement custom checktx logic, which was one of the main goals of this PR. It also maintains consistency with other setter methods in the file, ensuring that the codebase remains clean and maintainable.
CHANGELOG.md (6)
Line range hint
1-3: LGTM: Clear version header and descriptionThe header for the unreleased v0.47.0 version is clear and follows the standard format for changelogs.
Line range hint
16-20: LGTM: Clear description of client breaking changesThe Client Breaking Changes section is well-documented and provides clear information about the changes affecting client implementations.
Line range hint
29-34: LGTM: Comprehensive list of featuresThe Features section provides a detailed list of new functionalities added to various components of the SDK. This is very helpful for users looking to understand what's new in this version.
Line range hint
43-48: LGTM: Clear description of bug fixesThe Bug Fixes section provides clear descriptions of the issues that have been resolved. This is very helpful for users who may have encountered these problems in previous versions.
Line range hint
57-62: LGTM: Comprehensive documentation of API changesThe API Breaking Changes section provides a detailed list of changes that affect the API. This is crucial information for developers using the SDK.
Line range hint
71-76: LGTM: Clear documentation of improvementsThe Improvements section provides a comprehensive list of enhancements made to various components of the SDK. This is valuable information for users looking to understand how the SDK has been optimized.
baseapp/baseapp.go (2)
764-764: Verify that passingnilastxtorunTxis safeWhen calling
app.runTx(execModeFinalize, tx, nil),nilis passed as thetxparameter. Ensure thatrunTxgracefully handles aniltxwithout causing unexpected behavior or panics.
873-878: LGTM!The code correctly decodes
txwhen it isniland handles potential decoding errors properly.baseapp/abci.go (1)
385-390: Function closure captures execution mode correctlyThe wrapper function
runTxis appropriately defined to prevent users from overriding the execution mode. It ensures that the execution context remains consistent whenapp.checkTxHandleris called.
| _, _, _, err = app.runTx(execModePrepareProposal, bz, tx) | ||
| if err != nil { | ||
| return nil, err |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider refactoring to eliminate code duplication
Both PrepareProposalVerifyTx and ProcessProposalVerifyTx perform similar transaction verification by calling runTx and handling the error in the same way. Refactoring this common logic into a shared helper function could improve maintainability.
Also applies to: 1185-1187
baseapp/baseapp.go
Outdated
| @@ -37,7 +37,7 @@ | |||
| ) | |||
|
|
|||
| type ( | |||
| execMode uint8 | |||
| execMode = uint8 | |||
There was a problem hiding this comment.
Avoid using type aliases; prefer type definitions
According to the Uber Go Style Guide, type aliases should be avoided. Consider defining execMode as a new type instead of an alias for better type safety.
-type (
- execMode = uint8
+type (
+ execMode uint8📝 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.
| execMode = uint8 | |
| execMode uint8 |
| @@ -91,6 +91,7 @@ | |||
| prepareCheckStater sdk.PrepareCheckStater // logic to run during commit using the checkState | |||
| precommiter sdk.Precommiter // logic to run during commit using the deliverState | |||
| versionModifier server.VersionModifier // interface to get and set the app version | |||
| checkTxHandler sdk.CheckTxHandler | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Group checkTxHandler with related handler fields for consistency
Per the Uber Go Style Guide, related struct fields should be grouped together. Consider moving checkTxHandler next to other handler fields to improve readability.
postHandler sdk.PostHandler // post handler, optional
+ checkTxHandler sdk.CheckTxHandler // ABCI CheckTx handler
initChainer sdk.InitChainer // ABCI InitChain handler
preBlocker sdk.PreBlocker // logic to run before BeginBlocker
beginBlocker sdk.BeginBlocker // (legacy ABCI) BeginBlock handler
endBlocker sdk.EndBlocker // (legacy ABCI) EndBlock handler
- checkTxHandler sdk.CheckTxHandler📝 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.
| checkTxHandler sdk.CheckTxHandler | |
| postHandler sdk.PostHandler // post handler, optional | |
| checkTxHandler sdk.CheckTxHandler // ABCI CheckTx handler | |
| initChainer sdk.InitChainer // ABCI InitChain handler | |
| preBlocker sdk.PreBlocker // logic to run before BeginBlocker | |
| beginBlocker sdk.BeginBlocker // (legacy ABCI) BeginBlock handler | |
| endBlocker sdk.EndBlocker // (legacy ABCI) EndBlock handler |
There was a problem hiding this comment.
+1 for adding a comment and moving it
There was a problem hiding this comment.
@julienrbrt, understood! Adding a comment to checkTxHandler will improve clarity. Here's the updated code:
checkTxHandler sdk.CheckTxHandler // ABCI CheckTx handler😊🐰
| if app.checkTxHandler == nil { | ||
| gInfo, result, anteEvents, err := app.runTx(mode, req.Tx, nil) | ||
| if err != nil { | ||
| return responseCheckTxWithEvents(err, gInfo.GasWanted, gInfo.GasUsed, anteEvents, app.trace), nil | ||
| } | ||
|
|
||
| return &abci.CheckTxResponse{ | ||
| GasWanted: int64(gInfo.GasWanted), // TODO: Should type accept unsigned ints? | ||
| GasUsed: int64(gInfo.GasUsed), // TODO: Should type accept unsigned ints? | ||
| Log: result.Log, | ||
| Data: result.Data, | ||
| Events: sdk.MarkEventsToIndex(result.Events, app.indexEvents), | ||
| }, nil |
There was a problem hiding this comment.
Resolve TODO comments regarding gas value types
The code includes TODO comments questioning whether GasWanted and GasUsed should accept unsigned integers. Since gas values are inherently non-negative, consider using uint64 instead of int64 to accurately represent these values and prevent potential issues with negative values.
There was a problem hiding this comment.
ACK. Just missing some go docs and one nit.
Are we going to do the same for v2 for feature parity? https://github.com/cosmos/cosmos-sdk/blob/main/server/v2/cometbft/options.go#L19-L23
yes, was thinking of doing it in a follow up, but can do it here too |
A follow up makes sense yeah. |
julienrbrt
added
the
backport/v0.52.x
|
Adding the backport label as anything touching baseapp needs to go in 0.52 or it'll be removed. |
tac0turtle
force-pushed
the
marko/checktx_handler
branch
from
6aae348
to
76c5587
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
types/abci.go (1)
26-29: Consider separating RunTx type definition and improving documentationThe new
CheckTxHandlertype and its documentation look good overall. However, there are a few suggestions for improvement:
Consider separating the
RunTxfunction type into its own type definition with a separate comment. This would improve clarity and make it easier to referenceRunTxelsewhere if needed.The comment could provide more context about why
RunTxis passed as a parameter toCheckTxHandler. This would address the question raised in a previous review comment.Good job addressing the execution mode concern in the comment. This clarifies the intended usage and limitations of the
RunTxfunction.As suggested in a previous review comment, consider if this type could be defined inline where it's used. If it's only used in one place, inline definition might be more appropriate.
Here's a suggested refactor to address points 1 and 2:
// RunTx is a function type alias for executing logic before transactions are executed. // It does not override antehandlers, and the execution mode is not passed to avoid overriding. type RunTx func(txBytes []byte, tx Tx) (gInfo GasInfo, result *Result, anteEvents []abci.Event, err error) // CheckTxHandler defines a function type alias for executing logic before transactions are processed. // It takes a RunTx function as a parameter to allow custom pre-execution logic while maintaining // consistent handling of antehandlers and execution mode. type CheckTxHandler func(RunTx, *abci.CheckTxRequest) (*abci.CheckTxResponse, error)
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📥 Commits
Files that changed from the base of the PR and between 6aae3484fc891fe5f3daa23b404c0453596d6daf and 76c5587.
📒 Files selected for processing (1)
- types/abci.go (1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
types/abci.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
(cherry picked from commit 0ea2445)
Description
this pr is a follow up of #21960. While trying to align the design of mempool lanes I realized blocksdk is somewhat hard to follow for custom checktx code. This aims to simplify the design and allow people to extend checktx in more ways. This will also be used for mempool lanes by comet
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...
!in the type prefix if API or client breaking changeCHANGELOG.mdReviewers 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.
Please see Pull Request Reviewer section in the contributing guide for more information on how to review a pull request.
I have...
Summary by CodeRabbit
New Features
x/validatemodule for transaction validation.Bug Fixes
Documentation
CheckTxfunctionality, providing clearer guidance on usage.