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(baseapp): create checktx handler #21979

Merged
merged 13 commits into from
Oct 2, 2024
Merged

Conversation

tac0turtle
Copy link
Member

@tac0turtle tac0turtle commented Sep 30, 2024

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...

  • included the correct type prefix in the PR title, you can find examples of the prefixes below:
  • confirmed ! in the type prefix if API or client breaking change
  • targeted the correct branch (see PR Targeting)
  • provided a link to the relevant issue or specification
  • reviewed "Files changed" and left comments if necessary
  • included the necessary unit and integration tests
  • added a changelog entry to CHANGELOG.md
  • updated the relevant documentation or specification, including comments for documenting Go code
  • 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.

Please see Pull Request Reviewer section in the contributing guide for more information on how to review a pull request.

I have...

  • confirmed the correct type prefix in the PR title
  • confirmed all author checklist items have been addressed
  • reviewed state machine logic, API design and naming, documentation is accurate, tests and test coverage

Summary by CodeRabbit

  • New Features

    • Introduced the x/validate module for transaction validation.
    • Added a new method for setting custom transaction checking logic.
    • Enhanced CLI commands and added new flags for improved user experience.
  • Bug Fixes

    • Resolved various issues related to transaction handling and state management.
  • Documentation

    • Updated documentation for CheckTx functionality, providing clearer guidance on usage.
    • Enhanced explanations of new mempool implementations and their configurations.

Copy link
Contributor

coderabbitai bot commented Sep 30, 2024

📝 Walkthrough
📝 Walkthrough

Walkthrough

The 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 CheckTx functionality. The CHANGELOG.md has been updated to reflect these changes, detailing new features, improvements, and bug fixes across various modules.

Changes

File Path Change Summary
CHANGELOG.md Updated to include new features, improvements, bug fixes, and breaking changes, including the introduction of the x/validate module and updates to CLI commands.
baseapp/... Modifications to CheckTx, FinalizeBlock, and runTx methods to enhance transaction processing and introduce a custom checkTxHandler.
baseapp/options.go Added SetCheckTxHandler method for assigning custom transaction checking logic.
baseapp/test_helpers.go Updated method calls in SimCheck, Simulate, and SimDeliver to include transaction parameters for improved processing during simulation.
docs/build/abci/04-checktx.md New documentation detailing the CheckTx functionality and custom handler usage.
docs/build/building-apps/02-app-mempool.md Updated to clarify mempool concepts and new implementations since version v0.47.
types/abci.go Introduced new function type aliases CheckTxHandler and RunTx to enhance transaction handling capabilities.

Possibly related PRs

Suggested labels

C:server/v2, C:server/v2 cometbft

Suggested reviewers

  • sontrinh16
  • akhilkumarpilli
  • julienrbrt
  • kocubinski
  • aaronc

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?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@tac0turtle
Copy link
Member Author

there is not a clean way to do this in order to achieve it for cometmempool lanes.

@tac0turtle tac0turtle closed this Sep 30, 2024
@tac0turtle tac0turtle reopened this Sep 30, 2024
@tac0turtle tac0turtle marked this pull request as ready for review September 30, 2024 11:03
@julienrbrt julienrbrt changed the title refactor: create checktx handler refactor(baseapp): create checktx handler Sep 30, 2024
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

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 reference

While 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 section

While the example is helpful, it could be improved in the following ways:

  1. Provide more context about where in the NewSimApp function this code should be placed.
  2. Explain what arguments should be passed to abci.NewCustomCheckTxHandler(...).
  3. 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 tx parameter to the runTx call aligns well with the PR objectives of simplifying the checktx handler design. This change allows for more contextual information to be passed to the runTx method, which can be beneficial for custom checktx logic implementation.

Consider wrapping the error returned from runTx with 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 nil as the third argument to runTx is consistent with the function's signature and maintains alignment with the changes in other functions.

Consider adding a brief comment to explain why nil is 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 nil more explicit.


39-39: LGTM! Consider enhancing error handling.

The addition of the tx parameter to the runTx call is consistent with the changes in SimCheck and aligns with the PR objectives. This modification allows for more contextual information to be passed to the runTx method during transaction delivery simulation.

Similar to the suggestion for SimCheck, consider wrapping the error returned from runTx with 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 objectives

The modifications to SimCheck, Simulate, and SimDeliver functions consistently implement the addition of the tx parameter to the runTx method 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:

  1. Document the new design and its integration points clearly.
  2. Create examples or templates for common use cases of custom checktx logic.
  3. 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 comment

The CheckTxHandler type 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 comment

The RunTx type 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 suggestion

The 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 Mempool

The 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 suggestion

The 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 SetCheckTxHandler method 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 changes

The 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 LegacyMsg interface could be explained further.


Line range hint 10-14: Highlight significant API changes

The 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 changes

The 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 improvements

The 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 changes

For 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 features

In 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 PRs

For 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: Update runTx function comment to include the new tx parameter

The function comment for runTx should be updated to describe the new tx sdk.Tx parameter and its purpose, ensuring the documentation remains accurate.

📜 Review details

Configuration used: .coderabbit.yml
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between 968bc7c and 20e9fc0.

📒 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 flowchart

The 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 CheckTxHandler

This 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 concepts

The 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 mempool

This 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 SetCheckTxHandler method 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 description

The 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 changes

The 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 features

The 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 fixes

The 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 changes

The 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 improvements

The 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 passing nil as tx to runTx is safe

When calling app.runTx(execModeFinalize, tx, nil), nil is passed as the tx parameter. Ensure that runTx gracefully handles a nil tx without causing unexpected behavior or panics.


873-878: LGTM!

The code correctly decodes tx when it is nil and handles potential decoding errors properly.

baseapp/abci.go (1)

385-390: Function closure captures execution mode correctly

The wrapper function runTx is appropriately defined to prevent users from overriding the execution mode. It ensures that the execution context remains consistent when app.checkTxHandler is called.

Comment on lines +1166 to 1168
_, _, _, err = app.runTx(execModePrepareProposal, bz, tx)
if err != nil {
return nil, err
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ 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

@@ -37,7 +37,7 @@
)

type (
execMode uint8
execMode = uint8
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

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.

Suggested change
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
Copy link
Contributor

@coderabbitai coderabbitai bot Sep 30, 2024

Choose a reason for hiding this comment

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

🛠️ 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.

Suggested change
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

Copy link
Member

@julienrbrt julienrbrt Sep 30, 2024

Choose a reason for hiding this comment

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

+1 for adding a comment and moving it

Copy link
Contributor

Choose a reason for hiding this comment

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

@julienrbrt, understood! Adding a comment to checkTxHandler will improve clarity. Here's the updated code:

	checkTxHandler     sdk.CheckTxHandler             // ABCI CheckTx handler

😊🐰

Comment on lines +370 to +382
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
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

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.

Copy link
Member

@julienrbrt julienrbrt left a comment

Choose a reason for hiding this comment

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

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

types/abci.go Outdated Show resolved Hide resolved
baseapp/baseapp.go Show resolved Hide resolved
baseapp/baseapp.go Outdated Show resolved Hide resolved
@tac0turtle
Copy link
Member Author

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

@julienrbrt
Copy link
Member

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 julienrbrt added the backport/v0.52.x PR scheduled for inclusion in the v0.52's next stable release label Sep 30, 2024
@julienrbrt
Copy link
Member

Adding the backport label as anything touching baseapp needs to go in 0.52 or it'll be removed.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Outside diff range and nitpick comments (1)
types/abci.go (1)

26-29: Consider separating RunTx type definition and improving documentation

The new CheckTxHandler type and its documentation look good overall. However, there are a few suggestions for improvement:

  1. Consider separating the RunTx function type into its own type definition with a separate comment. This would improve clarity and make it easier to reference RunTx elsewhere if needed.

  2. The comment could provide more context about why RunTx is passed as a parameter to CheckTxHandler. This would address the question raised in a previous review comment.

  3. Good job addressing the execution mode concern in the comment. This clarifies the intended usage and limitations of the RunTx function.

  4. 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 6aae348 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.

Copy link
Contributor

@testinginprod testinginprod left a comment

Choose a reason for hiding this comment

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

LGTM

@tac0turtle tac0turtle added this pull request to the merge queue Oct 2, 2024
Merged via the queue into main with commit 0ea2445 Oct 2, 2024
75 of 78 checks passed
@tac0turtle tac0turtle deleted the marko/checktx_handler branch October 2, 2024 11:45
mergify bot pushed a commit that referenced this pull request Oct 2, 2024
tac0turtle added a commit that referenced this pull request Oct 2, 2024
@coderabbitai coderabbitai bot mentioned this pull request Oct 4, 2024
12 tasks
@coderabbitai coderabbitai bot mentioned this pull request Oct 25, 2024
12 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport/v0.52.x PR scheduled for inclusion in the v0.52's next stable release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants