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

feat(server/v2/cometbft): Implement necessary handlers and helpers for vote extensions #22830

Merged
merged 18 commits into from
Dec 19, 2024

Conversation

facundomedica
Copy link
Member

@facundomedica facundomedica commented Dec 11, 2024

Description

Closes: #XXXX


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 custom pre-blocking behavior in the application build process.
    • Added new functions for enhanced transaction handling and vote extension validation.
    • Implemented new message handlers for governance-related messages.
    • Enhanced integration testing capabilities with refined service implementations.
  • Bug Fixes

    • Corrected naming conventions for handler types to ensure consistency.
  • Documentation

    • Updated comments for configuration functions to clarify security implications and settings.

Copy link
Contributor

coderabbitai bot commented Dec 11, 2024

📝 Walkthrough
📝 Walkthrough

Walkthrough

This pull request introduces several enhancements to the application builder, server, and testing infrastructure across multiple files. The primary changes include adding a new preblocker field to the AppBuilder, updating handler signatures to include chain ID, correcting type names, and introducing new utility functions for transaction and consensus handling. The modifications aim to increase flexibility in transaction processing, improve error handling, and provide more robust testing capabilities for the application framework.

Changes

File Change Summary
runtime/v2/builder.go Added preblocker field to AppBuilder, introduced AppBuilderWithPreblocker function
server/v2/cometbft/abci.go Updated method signatures, corrected type names, modified consensus parameter retrieval
server/v2/cometbft/handlers/defaults.go Added chainID parameter to handler methods, corrected type names
server/v2/cometbft/handlers/handlers.go Updated function type signatures, renamed parameters
server/v2/cometbft/options.go Corrected VerifyVoteExtensionHandler type
server/v2/cometbft/utils.go Added new functions for consensus parameters, transaction handling, and vote extension validation
tests/integration/v2/services.go Introduced new service structs for testing, including BranchService, RouterService
tests/integration/v2/gov/common_test.go Added handler for MsgExecLegacyContent message type
tests/integration/v2/gov/keeper/fixture_test.go Registered custom message handler for MsgExecLegacyContent
x/auth/tx/gogotx.go Modified fee handling in newWrapperFromDecodedTx function

Possibly related PRs

Suggested Labels

C:x/group

Suggested Reviewers

  • testinginprod
  • akhilkumarpilli
  • sontrinh16
  • kocubinski

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.

@julienrbrt julienrbrt added the backport/v0.52.x PR scheduled for inclusion in the v0.52's next stable release label Dec 16, 2024
@facundomedica facundomedica marked this pull request as ready for review December 18, 2024 09:25
@facundomedica facundomedica changed the title feat(server/v2): Ensure vote extensions work as expected feat(server/v2/cometbft): Implement necessary handlers and helpers for vote extensions Dec 18, 2024
Copy link
Contributor

@facundomedica your pull request is missing a changelog!

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: 2

🔭 Outside diff range comments (2)
tests/integration/v2/services.go (2)

Line range hint 235-248: Avoid re-initializing the gas meter if originalGasMeter is nil.

The code unconditionally invokes Consume on the originalGasMeter. If the original context had no gas meter, this can panic. A nil-check or a default fallback could prevent unexpected runtime errors.

-originalGasMeter.Consume(gasUsed, "execute-with-gas-limit")
+if originalGasMeter != nil {
+    originalGasMeter.Consume(gasUsed, "execute-with-gas-limit")
+}

The handlers map in RouterService needs mutex protection for concurrent access

The code analysis reveals that RouterService is used in a concurrent environment:

  1. The handlers map is modified during registration via RegisterHandler method
  2. The map is accessed during Invoke and CanInvoke operations
  3. These operations are used across multiple modules (gov, authz, accounts) that may run concurrently

To prevent potential race conditions, the handlers map should be protected by a mutex. Here's what needs to be addressed:

  • Add a mutex field to RouterService struct
  • Lock the mutex during RegisterHandler calls
  • Use RLock during Invoke and CanInvoke operations
🔗 Analysis chain

Line range hint 291-291: Consider map accesses under concurrent usage.

The handlers map in RouterService is not protected by any mutex. If there's a possibility of concurrent registration or invocation, this could cause race conditions.


Let me analyze the code to verify the concern about concurrent access to the handlers map in RouterService.


Let me analyze the RouterService implementation more closely to verify the concurrent access concern.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Checking usage patterns for `RouterService` to ensure no concurrent writes
rg -A 5 "RouterService"

Length of output: 46162


Script:

#!/bin/bash
# Let's check if there are any concurrent write operations to RouterService
ast-grep --pattern 'go func() {
  $$$
  $router.$$$
  $$$
}'

# Also check for goroutines that might access RouterService
rg -A 3 "go \w*\(" 

Length of output: 28352


Script:

#!/bin/bash
# Check the implementation of RouterService and its usage patterns
ast-grep --pattern 'type RouterService struct {
  $$$
}'

# Check for any concurrent invocations of RegisterHandler
rg -A 5 "RegisterHandler.*RouterService"

# Check if there are any concurrent invocations of Invoke or CanInvoke
rg -A 5 "(\w+\s*:=\s*)?.*RouterService.*\.(Invoke|CanInvoke)"

Length of output: 6407

🧹 Nitpick comments (6)
tests/integration/v2/services.go (2)

173-173: Consider returning an error instead of panicking.

The code panics if the context fails type assertion, which terminates test execution abruptly. Converting panics into returned errors can provide a cleaner failure mode.

-func (e *eventService) EventManager(ctx context.Context) event.Manager {
-	iCtx, ok := ctx.Value(contextKey).(*integrationContext)
-	if !ok {
-		panic("context is not an integration context")
-	}
-	return &eventManager{ctx: iCtx}
+func (e *eventService) EventManager(ctx context.Context) (event.Manager, error) {
+	iCtx, ok := ctx.Value(contextKey).(*integrationContext)
+	if !ok {
+		return nil, errors.New("context is not an integration context")
+	}
+	return &eventManager{ctx: iCtx}, nil
}

Line range hint 339-346: Enhance gas configuration granularity.

The returned GasConfig is empty, which may limit the ability to fine-tune gas usage during tests or expansions. Consider returning a test-specific configuration to exercise gas logic more thoroughly in integration tests.

server/v2/cometbft/utils.go (2)

456-479: Consider returning actual values in InjectedTx methods.
The methods GetGasLimit, GetMessages, and GetSenders all return hardcoded or empty values. If your use cases need fully functional transaction data, it might help to populate these with real values or handle them carefully at higher layers. Otherwise, consider documenting these method stubs clearly.


480-608: Looks good—recommend thorough testing for vote extensions.
ValidateVoteExtensions contains robust checks for vote extension signatures. Ensure there's comprehensive test coverage, especially around edge cases and error paths.

Would you like me to help generate some baseline tests for this function?

simapp/v2/simdv2/cmd/config.go (1)

95-98: Consider clarifying the commented handler code.
These lines suggest a plan to implement custom vote extension handlers. If they are meant to be examples, consider adding a short comment explaining that they are placeholders. If they are a TODO, you might track it in a separate issue to make it more visible.

x/auth/tx/gogotx.go (1)

Line range hint 35-50: LGTM! Consider adding more detailed comments

The changes improve robustness by properly handling nil fee amounts and using the Add method for fee accumulation. The inline comment about nil amounts is helpful, but consider expanding it to explain the rationale for using Add.

Consider adding a more detailed comment:

-  fees = sdk.Coins{} // decodedTx.Tx.AuthInfo.Fee.Amount might be nil
+  // Initialize empty fees since decodedTx.Tx.AuthInfo.Fee.Amount might be nil
+  // Use Add method for proper coin addition and validation
+  fees = sdk.Coins{}
📜 Review details

Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 0a89178 and ad2801e.

📒 Files selected for processing (12)
  • runtime/v2/builder.go (3 hunks)
  • server/v2/cometbft/abci.go (9 hunks)
  • server/v2/cometbft/handlers/defaults.go (4 hunks)
  • server/v2/cometbft/handlers/handlers.go (1 hunks)
  • server/v2/cometbft/options.go (1 hunks)
  • server/v2/cometbft/utils.go (4 hunks)
  • simapp/v2/go.mod (1 hunks)
  • simapp/v2/simdv2/cmd/config.go (1 hunks)
  • tests/integration/v2/gov/common_test.go (0 hunks)
  • tests/integration/v2/gov/keeper/fixture_test.go (0 hunks)
  • tests/integration/v2/services.go (1 hunks)
  • x/auth/tx/gogotx.go (2 hunks)
💤 Files with no reviewable changes (2)
  • tests/integration/v2/gov/common_test.go
  • tests/integration/v2/gov/keeper/fixture_test.go
🧰 Additional context used
📓 Path-based instructions (9)
server/v2/cometbft/options.go (1)

Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.

x/auth/tx/gogotx.go (1)

Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.

runtime/v2/builder.go (1)

Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.

simapp/v2/simdv2/cmd/config.go (1)

Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.

server/v2/cometbft/handlers/handlers.go (1)

Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.

server/v2/cometbft/utils.go (1)

Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.

server/v2/cometbft/handlers/defaults.go (1)

Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.

tests/integration/v2/services.go (2)

Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.


Pattern tests/**/*: "Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull request"

server/v2/cometbft/abci.go (1)

Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.

🪛 golangci-lint (1.62.2)
server/v2/cometbft/utils.go

18-18: ST1019: package "github.com/cosmos/gogoproto/proto" is being imported more than once

(stylecheck)

🔇 Additional comments (17)
tests/integration/v2/services.go (1)

Line range hint 205-231: Verify concurrency safety when updating parent state from branched state.

Merging state changes from a branched context into the parent context could introduce race conditions if multiple goroutines branch and merge concurrently. Reviewing usage and ensuring that merges are protected by proper synchronization (or that execution remains single-threaded during merges) is advisable.

server/v2/cometbft/utils.go (2)

450-454: Function name matches functionality—no issues found.
RawTx returns an InjectedTx instance and is suitably named.


356-357: Verify usage of version 0 for consensus parameters.
The function GetConsensusParams hardcodes the height version to 0 when querying the consensus module. Please confirm that this is the intended behavior and that the latest version isn't required.

✅ Verification successful

Let me check the implementation of the function and its usage context to better understand the height parameter.


Let me check the QueryParamsRequest implementation and related code to understand if height parameter has any special meaning.


Let me check the consensus module's query implementation to understand if height parameter has any special meaning.


Let me check how the consensus parameters are used in the codebase, particularly around the height parameter.


Based on the code analysis, I can now provide a final response about the usage of height 0 in GetConsensusParams.

Using height 0 for consensus parameters query is the correct behavior

The height parameter of 0 is used to query the latest consensus parameters from the current state. This is evident from:

  1. The consensus module's QueryParamsRequest doesn't require a height parameter in its protobuf definition
  2. The function is used in critical paths like ABCI interface where it needs to get the latest consensus parameters
  3. The pattern of using height 0 to get latest state is a common pattern in Cosmos SDK
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# This script searches for all invocations of GetConsensusParams to see whether
# any call site expects a non-zero version.

ast-grep --pattern $'GetConsensusParams($_, $_)' 

Length of output: 660


Script:

#!/bin/bash
# Check the implementation of GetConsensusParams and related code
rg -A 5 "func GetConsensusParams" 

Length of output: 759


Script:

#!/bin/bash
# Search for QueryParamsRequest definition and usage
rg -A 5 "QueryParamsRequest" 

# Also check if there are any comments or documentation about height parameter in query
ast-grep --pattern 'Query($_, $height, $_)'

Length of output: 75002


Script:

#!/bin/bash
# Search for consensus module's query implementation
rg -A 5 "func.*Query.*consensus" 

# Also check if there are any comments about height in the consensus module
rg -B 2 -A 2 "height.*consensus"

Length of output: 46441


Script:

#!/bin/bash
# Search for usage of consensus parameters in the codebase
rg -B 2 -A 2 "GetConsensusParams.*height"

# Also check the implementation of the consensus module's Query method
rg -A 5 "func.*Query.*consensus.*Params"

Length of output: 555

server/v2/cometbft/handlers/handlers.go (1)

16-20: Name all arguments for clarity.
According to a past review comment, chainID was added as a parameter. It may help to name all parameters consistently for readability (rather than relying solely on types).

server/v2/cometbft/options.go (1)

23-23: Type name correction looks good.
The change from VerifyVoteExtensionhandler to VerifyVoteExtensionHandler resolves the naming inconsistency.

server/v2/cometbft/handlers/defaults.go (4)

177-179: LGTM!

The addition of the chainID parameter maintains consistency with the interface while preserving the no-op behavior.


185-187: LGTM!

The addition of the chainID parameter maintains consistency with the interface while preserving the no-op behavior.


Line range hint 200-204: LGTM!

The type name has been corrected to follow proper naming conventions.


35-35: Verify the usage of chainID parameter

The chainID parameter has been added to the function signature but is not used within the function body. This might indicate incomplete implementation or missing functionality.

runtime/v2/builder.go (2)

33-33: LGTM!

The preblocker implementation correctly chains the default module manager's preblocker with any custom preblocker provided. The error handling is appropriate, and the execution order ensures that the default preblocker runs before any custom implementation.

Also applies to: 99-109


236-244: LGTM!

The AppBuilderWithPreblocker function follows the established pattern of other AppBuilderOption functions and provides a clean way to set custom pre-blocking logic.

simapp/v2/go.mod (1)

283-283: LGTM!

The new replace directive for cosmossdk.io/x/auth follows the established pattern in the file and correctly points to the local directory.

server/v2/cometbft/abci.go (5)

71-71: LGTM: Fixed type name capitalization.

The type name correction follows Go naming conventions for proper capitalization of type names.


Line range hint 145-153: LGTM: Improved error handling for consensus parameters.

The code now properly handles the case where consensus parameters are not found by explicitly checking for collections.ErrNotFound and setting appVersion to 0 in such cases.


390-390: LGTM: Added chain ID to proposal handlers.

The change properly passes the chain ID to both prepare and process proposal handlers, ensuring proper validation context is available.

Also applies to: 436-436


536-536: LGTM: Standardized consensus parameters retrieval.

The code now consistently uses GetConsensusParams across all methods, improving code consistency and maintainability.

Also applies to: 603-603, 622-622, 661-661


705-706: Verify type assertion safety.

The type assertion RawTx(rawTx).(T) could potentially panic if type T doesn't implement the required interface. Please ensure that all possible types that can be used as T are compatible with RawTx.

✅ Verification successful

Type assertion is safe in this context.

The type assertion RawTx(rawTx).(T) is safe because:

  1. RawTx returns a transaction.Tx interface implementation (specifically InjectedTx)
  2. The generic type parameter T is constrained to transaction.Tx in the function signature decodeTxs[T transaction.Tx]
  3. The type assertion is used as a fallback mechanism when decoding fails, allowing access to raw transaction bytes while maintaining the expected interface

The implementation ensures type safety through Go's generic constraints, making it impossible for T to be anything that doesn't implement transaction.Tx.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Search for all types that could be used as T in this context
# to verify they implement the necessary interface.

# Find all type declarations that could be used as T
ast-grep --pattern 'type $name struct { $$$ }'

# Find all interface implementations
ast-grep --pattern 'func ($receiver $type) $method($$$) $$$'

Length of output: 111


Script:

#!/bin/bash
# Let's first find the RawTx type definition and its usage context
rg -A 5 "type RawTx" 

# Find where RawTx is used in type parameters or generic constraints
rg "RawTx"

# Find the decodeTxs function definition and its context
rg -A 10 "func decodeTxs"

# Find any interface definitions that might be related to transactions
rg -A 5 "type.*interface"

Length of output: 82585


Script:

#!/bin/bash
# Let's find the transaction.Tx interface definition and its implementations
rg -A 5 "type.*Tx.*interface"

# Find the RawTx function implementation and its context
rg -A 10 "func RawTx"

# Find where type T is used in the decodeTxs function context
rg -B 5 -A 10 "func decodeTxs\[T"

Length of output: 19006

"strings"
"time"

abci "github.com/cometbft/cometbft/api/cometbft/abci/v1"
cmtproto "github.com/cometbft/cometbft/api/cometbft/types/v1"
cryptoenc "github.com/cometbft/cometbft/crypto/encoding"
protoio "github.com/cosmos/gogoproto/io"
"github.com/cosmos/gogoproto/proto"
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

Remove duplicate import to adhere to style guidelines.
The package "github.com/cosmos/gogoproto/proto" is imported twice (lines 18 and 19). Please remove or consolidate one of these imports in order to comply with ST1019 (no duplicate imports).

Suggested diff:

Committable suggestion skipped: line range outside the PR's diff.

🧰 Tools
🪛 golangci-lint (1.62.2)

18-18: ST1019: package "github.com/cosmos/gogoproto/proto" is being imported more than once

(stylecheck)

@@ -98,7 +98,7 @@ func (h *DefaultProposalHandler[T]) PrepareHandler() PrepareHandler[T] {
}

func (h *DefaultProposalHandler[T]) ProcessHandler() ProcessHandler[T] {
return func(ctx context.Context, app AppManager[T], codec transaction.Codec[T], req *abci.ProcessProposalRequest) error {
return func(ctx context.Context, app AppManager[T], codec transaction.Codec[T], req *abci.ProcessProposalRequest, chainID string) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

💡 Codebase verification

Remove unused chainID parameter from ProcessHandler

The chainID parameter is indeed not used within the ProcessHandler implementation. The function body only deals with mempool operations and doesn't utilize the chainID parameter anywhere.

  • Remove the chainID parameter from the ProcessHandler function signature in server/v2/cometbft/handlers/defaults.go
🔗 Analysis chain

Verify the usage of chainID parameter

Similar to PrepareHandler, the chainID parameter has been added but is not utilized within the function body.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for chainID usage in process proposal handlers
rg -A 5 "chainID.*ProcessProposal"

Length of output: 36


Script:

#!/bin/bash
# Let's search for the ProcessHandler function implementation and check chainID usage
ast-grep --pattern 'func ProcessHandler[T any]($$$) func($$$) error {
  $$$
}'

# Also search for any chainID usage in the file
rg -A 5 "chainID" server/v2/cometbft/handlers/defaults.go

Length of output: 1064

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.

utACK! One q and nit

simapp/v2/go.mod Outdated Show resolved Hide resolved
@@ -219,3 +232,13 @@ func AppBuilderWithPostTxExec[T transaction.Tx](
a.postTxExec = postTxExec
}
}

func AppBuilderWithPreblocker[T transaction.Tx](
Copy link
Member

Choose a reason for hiding this comment

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

Can we add go docs here?

Copy link
Member

@julienrbrt julienrbrt Dec 18, 2024

Choose a reason for hiding this comment

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

We should precise as well that the module preblocker are ran before any logic there.
Which makes me think, do you think we should always run pre blocker before their logic?
What if we give them a callback to call preblocker themselves if needed.

Like

preblocker func(
		ctx context.Context, txs []T, mmPreblocker func()
	) error,

And then above we do smth like

	preblockerFn := func(ctx context.Context, txs []T, mmPreblocker func()) error {
		if a.preblocker != nil {
			return a.preblocker(ctx, txs, func() { if err := a.app.moduleManager.PreBlocker()(ctx, txs); err != nil {
			panic(err)
		} })
		}

                 return .app.moduleManager.PreBlocker()(ctx, txs)
	}

Not sure if needed however.

Copy link
Member Author

Choose a reason for hiding this comment

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

I like this, adding it

}

res, err := c.app.Query(ctx, latestVersion, &types.QueryParamsRequest{})
func GetConsensusParams[T transaction.Tx](ctx context.Context, app handlers.AppManager[T]) (*cmtproto.ConsensusParams, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why did this need to be refactored?

The use of c.GetConsensusParams is a lot clearer than the new syntax introduced

Copy link
Member

Choose a reason for hiding this comment

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

I think because it is used in ValidateVoteExtensions and that is something you'd be calling in the app directly.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, it's also a nice helper to have. I don't think it's much more complicated than what we had

@julienrbrt julienrbrt linked an issue Dec 18, 2024 that may be closed by this pull request
Copy link
Contributor

@hieuvubk hieuvubk left a comment

Choose a reason for hiding this comment

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

lgtm!

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: 1

🧹 Nitpick comments (1)
runtime/v2/builder.go (1)

241-253: Enhance documentation for AppBuilderWithPreblocker.

While the basic documentation is present, it would be beneficial to add:

  • Example usage showing common patterns
  • Error handling expectations
  • Threading model and concurrency considerations
  • Impact on block processing flow

Example documentation:

// AppBuilderWithPreblocker sets logic that will be executed before each block.
// The preblocker function is called with the current context, pending transactions,
// and a callback to execute the module manager's preblocker.
//
// The mmPreblocker callback can be used to call module manager's preblocker at any
// point during custom preblocker execution, allowing flexible integration with
// vote extensions and other pre-block processing requirements.
//
// Example usage:
//
//	builder.Build(
//		AppBuilderWithPreblocker(func(ctx context.Context, txs []T, mmPreblocker func()) error {
//			// Custom pre-block processing
//			if err := processVoteExtensions(ctx); err != nil {
//				return err
//			}
//			
//			// Call module manager's preblocker
//			mmPreblocker()
//			
//			return nil
//		}),
//	)
//
// Note: This function is executed synchronously during block processing.
// Any errors returned will prevent block creation.
📜 Review details

Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 28beb94 and f620313.

📒 Files selected for processing (1)
  • runtime/v2/builder.go (3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
runtime/v2/builder.go (1)

Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.

🔇 Additional comments (2)
runtime/v2/builder.go (2)

33-33: Add documentation for the preblocker field.

The preblocker field needs documentation explaining its purpose, usage, and the significance of the mmPreblocker callback parameter.


120-120: LGTM!

The integration of preblockerFn with the STF constructor is clean and correct.

runtime/v2/builder.go Outdated Show resolved Hide resolved
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

🧹 Nitpick comments (2)
runtime/v2/builder.go (2)

99-108: Simplify preblockerFn implementation using early return pattern.

The current implementation can be made more concise while maintaining the same logic.

Consider this refactoring:

 preblockerFn := func(ctx context.Context, txs []T) error {
-    if a.preblocker != nil {
-        return a.preblocker(ctx, txs, func() error {
-            return a.app.moduleManager.PreBlocker()(ctx, txs)
-        })
-    }
-
-    // if there is no preblocker set, call the module manager's preblocker directly
-    return a.app.moduleManager.PreBlocker()(ctx, txs)
+    if a.preblocker == nil {
+        return a.app.moduleManager.PreBlocker()(ctx, txs)
+    }
+    return a.preblocker(ctx, txs, func() error {
+        return a.app.moduleManager.PreBlocker()(ctx, txs)
+    })
 }

235-239: Enhance function documentation with examples and usage guidelines.

While the current documentation provides basic information, it would be helpful to add:

  1. Example usage showing how to implement vote extensions
  2. Clear explanation of execution order and its impact
  3. Best practices for implementing the preblocker function

Example documentation:

// AppBuilderWithPreblocker sets logic that will be executed before each block.
// The provided preblocker function receives a callback (mmPreblocker) that can be used
// to execute the module manager's preblocker at any point during custom preprocessing.
//
// This is especially useful when implementing vote extensions. For example:
//
//	app.Build(
//		AppBuilderWithPreblocker(func(ctx context.Context, txs []T, mmPreblocker func() error) error {
//			// Custom preprocessing logic
//			if err := processVoteExtensions(ctx); err != nil {
//				return err
//			}
//			// Execute module manager's preblocker
//			if err := mmPreblocker(); err != nil {
//				return err
//			}
//			// Additional preprocessing if needed
//			return nil
//		}),
//	)
📜 Review details

Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between f620313 and ca5d8f4.

📒 Files selected for processing (1)
  • runtime/v2/builder.go (3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
runtime/v2/builder.go (1)

Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.

🔇 Additional comments (2)
runtime/v2/builder.go (2)

33-33: Add field documentation for preblocker.

Please add godoc for the preblocker field explaining its purpose, execution timing, and relationship with the module manager's preblocker.


114-114: LGTM!

The preblockerFn is correctly integrated into the stf.New call.

@facundomedica facundomedica added this pull request to the merge queue Dec 19, 2024
Merged via the queue into main with commit 6d12b1d Dec 19, 2024
76 of 77 checks passed
@facundomedica facundomedica deleted the facu/v2-voteexts branch December 19, 2024 12:48
mergify bot pushed a commit that referenced this pull request Dec 19, 2024
…r vote extensions (#22830)

(cherry picked from commit 6d12b1d)

# Conflicts:
#	runtime/v2/builder.go
julienrbrt added a commit that referenced this pull request Dec 19, 2024
…r vote extensions (backport #22830) (#23006)

Co-authored-by: Facundo Medica <[email protected]>
Co-authored-by: Julien Robert <[email protected]>
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 C:server/v2 cometbft C:server/v2 Issues related to server/v2 C:x/auth
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support Vote Extensions in CometBFT server
8 participants