feat(server/v2/cometbft,stf): Listener integration in server/v2 #21917
Conversation
cool-develope
requested review from
a team,
facundomedica,
sontrinh16,
testinginprod and
kocubinski
as code owners
📝 Walkthrough📝 WalkthroughWalkthroughThe pull request introduces a direct dependency on Changes
Possibly related issues
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
|
github-actions
bot
added
C:server/v2
|
@cool-develope your pull request is missing a changelog! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (9)
server/v2/cometbft/abci.go (3)
43-43: New field added correctly, consider adding documentation.The
listenerfield is appropriately added to theConsensusstruct. However, it would be beneficial to add a comment explaining the purpose and usage of this field.Consider adding a comment above the
listenerfield to explain its purpose, for example:// listener is used to stream block-related data changes. listener *appdata.Listener
108-111: New method added correctly, consider improvements.The
SetListenermethod is appropriately implemented. However, consider the following suggestions:
- Add a comment to explain the purpose of this method.
- Consider adding a nil check to handle cases where a nil listener is passed.
Here's a suggested improvement:
// SetListener sets the listener for the consensus module. // It panics if a nil listener is provided. func (c *Consensus[T]) SetListener(l *appdata.Listener) { if l == nil { panic("cannot set nil listener") } c.listener = l }This change adds documentation and ensures that a nil listener cannot be set, which could lead to runtime errors if not handled properly.
22-22: Overall, changes look good but documentation could be improved.The additions of the
appdataimport,listenerfield, andSetListenermethod are well-implemented and integrate smoothly with the existing code. They follow Go conventions and best practices.To enhance code maintainability and clarity, consider adding documentation comments for the new
listenerfield andSetListenermethod. This will help other developers understand the purpose and usage of these new additions.Also applies to: 43-43, 108-111
server/v2/stf/stf.go (4)
200-200: Offer assistance with handlingmsgIndexandeventIndexin transaction processingThere are TODO comments at lines 200 and 284 indicating uncertainty on how to handle
msgIndexandeventIndex. I can help implement the logic to properly track and handle these indices during transaction execution.Would you like me to provide a proposed implementation or open a GitHub issue to track this task?
Also applies to: 284-284
209-209: Offer assistance with integratingexecCtx.txIndexwith eventsThe TODO comment at line 209 suggests handling
execCtx.txIndexwith events. I can help implement the necessary changes to ensure thattxIndexis correctly associated with events.Would you like me to provide a proposed implementation or open a GitHub issue to track this task?
325-325: Offer assistance with handlingexecCtx.msgIndexin event handlingThe TODO comment at line 325 indicates that
execCtx.msgIndexshould be handled with events. I can help implement the logic to associatemsgIndexwith events during message execution.Would you like me to provide a proposed implementation or open a GitHub issue to track this task?
500-502: Offer assistance with passing index fields to handlersThe
executionContextstruct now includestxIndex,msgIndex, andeventIndex. There's a TODO comment at line 502 questioning how to passeventIndexto the handlers. I can help design and implement a solution to pass these indices appropriately.Would you like me to provide a proposed implementation or open a GitHub issue to track this task?
server/v2/cometbft/streaming.go (2)
63-64: Address TODOs: ProvideHeaderBytesandHeaderJSONThe
HeaderBytesandHeaderJSONfields inappdata.StartBlockDataare currently set tonilwith TODO comments. Providing the actual header bytes and JSON is important for the listener to receive complete block information.Would you like assistance in implementing the serialization of the header data? I can help generate the necessary code or open a GitHub issue to track this task.
75-75: Address TODO: Provide transaction JSON representationThe
JSONfield inappdata.TxDatais set tonilwith a TODO comment. Supplying the JSON representation of transactions is crucial for the listener to process transaction data effectively.Would you like assistance in implementing the JSON serialization of transactions? I can help generate the necessary code or open a GitHub issue to track this task.
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (5)
- server/v2/cometbft/abci.go (3 hunks)
- server/v2/cometbft/go.mod (1 hunks)
- server/v2/cometbft/streaming.go (2 hunks)
- server/v2/stf/stf.go (8 hunks)
- server/v2/stf/stf_router.go (1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
server/v2/cometbft/abci.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.server/v2/cometbft/streaming.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.server/v2/stf/stf.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.server/v2/stf/stf_router.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
🔇 Additional comments not posted (2)
server/v2/cometbft/go.mod (1)
24-24: LGTM: Addition of direct dependency aligns with PR objectives.The addition of
cosmossdk.io/schema v0.3.0as a direct dependency is appropriate and aligns with the PR objectives of utilizingcosmossdk.io/schema/appdataevents. This change suggests that the codebase now explicitly uses features from this package.To ensure this dependency is being used effectively in the codebase, please run the following command:
This will help verify that the newly added dependency is actually being imported and used in the code.
✅ Verification successful
Dependency Usage Verified and Approved
The added direct dependency on
cosmossdk.io/schema v0.3.0is utilized across multiple files, includingcore/event/event.gowherecosmossdk.io/schema/appdatais imported. This verifies that the dependency is effectively used in the codebase.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for imports of cosmossdk.io/schema in the codebase rg --type go 'import.*cosmossdk.io/schema'Length of output: 505
server/v2/cometbft/abci.go (1)
22-22: Import statement looks good.The new import for
appdatais correctly placed and follows Go conventions for import grouping.
server/v2/stf/stf_router.go
Outdated
| @@ -165,6 +165,6 @@ func (r coreRouterImpl) Invoke(ctx context.Context, req transaction.Msg) (res tr | |||
| if !exists { | |||
| return nil, fmt.Errorf("%w: %s", ErrNoHandler, typeName) | |||
| } | |||
|
|
|||
| // TODO: should handle ctx.eventIndex, eventIndex should be embedded as a value in the context??? | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider creating an issue for the suggested enhancement instead of using a TODO comment.
The TODO comment suggests an important enhancement for handling ctx.eventIndex by embedding it as a value in the context. This could potentially improve event tracking and processing throughout the system. However, inline TODO comments are generally discouraged in production code.
Instead of keeping this TODO comment:
- Create a GitHub issue to track this enhancement, providing more details about the proposed change and its implications.
- Remove the TODO comment from the code.
- If necessary, add a comment referencing the created issue for context.
This approach aligns better with best practices for code maintenance and ensures the proposed enhancement is properly tracked and addressed.
server/v2/stf/stf.go
Outdated
| @@ -111,6 +111,7 @@ func (s STF[T]) DeliverBlock( | |||
|
|
|||
| // reset events | |||
| exCtx.events = make([]event.Event, 0) | |||
| exCtx.txIndex = 0 | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider consolidating exCtx.txIndex initialization
exCtx.txIndex is reset to 0 at lines 114, 127, and 145. If possible, consider initializing or resetting txIndex once to reduce code duplication and improve maintainability.
Also applies to: 127-127, 145-145
| if completionCallback, err := c.listener.Commit(appdata.CommitData{}); err != nil { | ||
| return err | ||
| } else if completionCallback != nil { | ||
| if err := completionCallback(); err != nil { | ||
| return err | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor to simplify control flow and enhance readability
The else block after a return statement is unnecessary and can be refactored for clarity. According to the Uber Go Style Guide, it's preferable to avoid else statements when the if block ends with a return.
Apply this diff to refactor the code:
- if completionCallback, err := c.listener.Commit(appdata.CommitData{}); err != nil {
- return err
- } else if completionCallback != nil {
- if err := completionCallback(); err != nil {
- return err
- }
- }
+ completionCallback, err := c.listener.Commit(appdata.CommitData{})
+ if err != nil {
+ return err
+ }
+ if completionCallback != nil {
+ if err := completionCallback(); err != nil {
+ return err
+ }
+ }This refactoring removes the unnecessary else and enhances the readability of the control flow.
📝 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.
| if completionCallback, err := c.listener.Commit(appdata.CommitData{}); err != nil { | |
| return err | |
| } else if completionCallback != nil { | |
| if err := completionCallback(); err != nil { | |
| return err | |
| } | |
| } | |
| } | |
| completionCallback, err := c.listener.Commit(appdata.CommitData{}) | |
| if err != nil { | |
| return err | |
| } | |
| if completionCallback != nil { | |
| if err := completionCallback(); err != nil { | |
| return err | |
| } | |
| } | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (5)
server/v2/stf/go.mod (1)
Go Version Mismatch in go.mod
The specified Go version
1.23.1inserver/v2/stf/go.modis not a recognized or released version as of April 2024. This may lead to compatibility issues or prevent the module from building with currently supported Go versions.
- File:
server/v2/stf/go.mod- Line: 6
Please update the Go version to a currently available version (e.g.,
1.21or1.22).🔗 Analysis chain
Line range hint
3-3: Verify the Go version specificationThe Go version is set to 1.23, which is not yet released. This might cause compatibility issues or prevent the module from being built with current Go versions.
Please confirm if this is intentional or if it should be changed to a currently available Go version (e.g., 1.21 or 1.22).
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check the Go version used in the project and compare with the one specified in go.mod # Get the Go version used in the project PROJECT_GO_VERSION=$(go version | awk '{print $3}' | sed 's/go//') # Get the Go version specified in go.mod GOMOD_VERSION=$(grep "^go " go.mod | awk '{print $2}') echo "Project Go version: $PROJECT_GO_VERSION" echo "go.mod specified version: $GOMOD_VERSION" if [ "$PROJECT_GO_VERSION" != "$GOMOD_VERSION" ]; then echo "Warning: Go version mismatch between project and go.mod" fiLength of output: 51554
Script:
#!/bin/bash # Description: Check the Go version used in the project and compare with the one specified in go.mod # Get the Go version used in the project PROJECT_GO_VERSION=$(go version | awk '{print $3}' | sed 's/go//') # Get the Go version specified in go.mod GOMOD_VERSION=$(grep "^go " go.mod | awk '{print $2}') echo "Project Go version: $PROJECT_GO_VERSION" echo "go.mod specified version: $GOMOD_VERSION" if [ "$PROJECT_GO_VERSION" != "$GOMOD_VERSION" ]; then echo "Warning: Go version mismatch between project and go.mod" fiLength of output: 476
Script:
#!/bin/bash # Description: Extract the Go version specified in go.mod and verify its validity # Extract the Go version specified in go.mod GOMOD_VERSION=$(grep "^go " go.mod | awk '{print $2}') echo "go.mod specified Go version: $GOMOD_VERSION" # List of known released Go versions (up to 1.22 as of April 2024) KNOWN_GO_VERSIONS=("1.20" "1.21" "1.22") # Check if the specified version is in the list of known versions if [[ " ${KNOWN_GO_VERSIONS[@]} " =~ " ${GOMOD_VERSION} " ]]; then echo "The specified Go version ${GOMOD_VERSION} is a recognized and released version." else echo "Warning: The specified Go version ${GOMOD_VERSION} is not a recognized or released version." fiLength of output: 400
server/v2/stf/stf.go (4)
204-221: LGTM: Enhanced event handling with improved granularityThe changes to event handling in the
deliverTxmethod significantly improve the granularity of event tracking during transaction processing. The code now sets appropriate block stage, transaction index, message index, and event index for each event, which aligns well with the PR objectives.Consider pre-allocating the
eventsslice to reduce potential memory reallocations:-events := make([]event.Event, 0) +events := make([]event.Event, 0, len(validationEvents)+len(execEvents))This minor optimization could slightly improve performance, especially for transactions with many events.
345-362: LGTM: Improved event handling in runTxMsgsThe updates to the
runTxMsgsmethod enhance event handling by including message and event indices. Returning events from this method allows for better event management in the calling method (execTx). These changes align well with the PR objectives for improved event handling.Consider pre-allocating the
eventsslice to potentially improve performance:-events := make([]event.Event, 0) +events := make([]event.Event, 0, len(msgs)*estimatedEventsPerMsg)Where
estimatedEventsPerMsgcould be a constant based on the average number of events typically generated per message. This could reduce memory reallocations, especially for transactions with many messages.
377-377: LGTM: Consistent event handling across block stagesThe addition of event index setting in
preBlock,beginBlock, andendBlockmethods ensures consistent event handling across all block stages. This is a positive change that aligns with the overall improvements in event management.In the
endBlockmethod, consider adding a comment to explain why events are reset:events := ctx.events -ctx.events = make([]event.Event, 0) // reset events +// Reset events to prevent mixing with validator update events +ctx.events = make([]event.Event, 0)This clarification would help future developers understand the reasoning behind this operation.
Also applies to: 394-394, 408-420
446-446: LGTM: Enhanced Simulate method return valuesThe modification to return
server.TxResultandstore.WriterMapfrom theSimulatemethod provides a more comprehensive and structured output from the simulation. This change aligns well with the improvements made to transaction processing and event handling throughout the file.Consider adding a comment to explain the use of 0 as the transaction index:
-txr := s.deliverTx(ctx, simulationState, tx, internal.ExecModeSimulate, hi, 0) +// Use 0 as txIndex for simulation as it's not part of an actual block +txr := s.deliverTx(ctx, simulationState, tx, internal.ExecModeSimulate, hi, 0)This clarification would help future developers understand the reasoning behind using 0 as the transaction index in simulations.
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
⛔ Files ignored due to path filters (1)
server/v2/stf/go.sumis excluded by!**/*.sum
📒 Files selected for processing (7)
- server/v2/cometbft/abci.go (3 hunks)
- server/v2/cometbft/go.mod (1 hunks)
- server/v2/cometbft/streaming.go (2 hunks)
- server/v2/stf/go.mod (1 hunks)
- server/v2/stf/stf.go (10 hunks)
- server/v2/stf/stf_router.go (0 hunks)
- server/v2/stf/stf_test.go (4 hunks)
💤 Files with no reviewable changes (1)
- server/v2/stf/stf_router.go
🚧 Files skipped from review as they are similar to previous changes (3)
- server/v2/cometbft/abci.go
- server/v2/cometbft/go.mod
- server/v2/cometbft/streaming.go
🧰 Additional context used
📓 Path-based instructions (2)
server/v2/stf/stf.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.server/v2/stf/stf_test.go (2)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"
🔇 Additional comments (5)
server/v2/stf/go.mod (1)
9-9: New dependencies addedThe following new dependencies have been added:
- Direct:
github.com/stretchr/testify v1.8.4- Indirect:
github.com/davecgh/go-spew v1.1.1,github.com/pmezard/go-difflib v1.0.0,gopkg.in/yaml.v3 v3.0.1The addition of
testifysuggests an enhancement in testing capabilities. The indirect dependencies are likely brought in bytestify.To ensure these dependencies are being used, let's check for their imports in the codebase:
#!/bin/bash # Description: Verify the usage of new dependencies in the codebase # Check for testify usage echo "Checking for testify usage:" rg --type go '"github.com/stretchr/testify' # Check for indirect dependencies usage (although they shouldn't be directly imported) echo "Checking for indirect dependencies usage (for completeness):" rg --type go '"github.com/davecgh/go-spew' rg --type go '"github.com/pmezard/go-difflib' rg --type go '"gopkg.in/yaml.v3'Also applies to: 14-14, 16-16, 18-18
server/v2/stf/stf.go (3)
149-149: LGTM: Addition of txIndex parameter enhances transaction trackingThe inclusion of
txIndex int32in thedeliverTxmethod signature is a positive change. It allows for precise tracking of transaction indices within a block, which aligns with the PR's objective to improve event handling capabilities.Also applies to: 175-175
223-223: LGTM: Comprehensive event inclusion in TxResultThe modification to include all events (both validation and execution) in the
TxResultstruct is a positive change. It ensures that the transaction result contains a complete record of all events associated with the transaction processing.
426-431: LGTM: Simplified validatorUpdates methodThe simplification of the
validatorUpdatesmethod to focus solely on retrieving validator updates is a positive change. It provides better separation of concerns, with event processing for validator updates now handled in the callingendBlockmethod. This change enhances the modularity and clarity of the code.server/v2/stf/stf_test.go (1)
176-227: Test cases provide comprehensive coverage of event generationThe test cases thoroughly verify the correctness of the events emitted during transaction processing stages. Assertions check event types, stages, indices, and attributes, ensuring that events are generated and structured as expected. This enhances the reliability of the event handling implementation.
| doPreBlock: func(ctx context.Context, txs []mock.Tx) error { | ||
| ctx.(*executionContext).events = append(ctx.(*executionContext).events, event.NewEvent("pre-block")) | ||
| return nil | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider safely asserting the context type to prevent potential panics
In the doPreBlock function, the code performs a direct type assertion ctx.(*executionContext) without checking if the assertion succeeds. If ctx is not of type *executionContext, this could lead to a panic. It's a good practice to handle type assertions safely to prevent unexpected panics.
Apply this change to safely assert the type:
execCtx, ok := ctx.(*executionContext)
if !ok {
return errors.New("unexpected context type in doPreBlock")
}
execCtx.events = append(execCtx.events, event.NewEvent("pre-block"))| doBeginBlock: func(ctx context.Context) error { | ||
| kvSet(t, ctx, "begin-block") | ||
| ctx.(*executionContext).events = append(ctx.(*executionContext).events, event.NewEvent("begin-block")) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ensure safe type assertion in doBeginBlock
Similar to doPreBlock, the doBeginBlock function performs an unchecked type assertion. Safely asserting the context type will enhance code robustness.
Modify the code as follows:
execCtx, ok := ctx.(*executionContext)
if !ok {
return errors.New("unexpected context type in doBeginBlock")
}
execCtx.events = append(execCtx.events, event.NewEvent("begin-block"))| return nil | ||
| }, | ||
| doEndBlock: func(ctx context.Context) error { | ||
| kvSet(t, ctx, "end-block") | ||
| ctx.(*executionContext).events = append(ctx.(*executionContext).events, event.NewEvent("end-block")) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add type assertion check in doEndBlock
In the doEndBlock function, consider adding a check to ensure the context is of the expected type to prevent potential panics.
Update the code to:
execCtx, ok := ctx.(*executionContext)
if !ok {
return errors.New("unexpected context type in doEndBlock")
}
execCtx.events = append(execCtx.events, event.NewEvent("end-block"))| return nil | ||
| }, | ||
| doValidatorUpdate: func(ctx context.Context) ([]appmodulev2.ValidatorUpdate, error) { return nil, nil }, | ||
| doValidatorUpdate: func(ctx context.Context) ([]appmodulev2.ValidatorUpdate, error) { | ||
| ctx.(*executionContext).events = append(ctx.(*executionContext).events, event.NewEvent("validator-update")) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Safely assert context type in doValidatorUpdate
For consistency and safety, add a type assertion check in the doValidatorUpdate function.
Implement the safe type assertion:
execCtx, ok := ctx.(*executionContext)
if !ok {
return errors.New("unexpected context type in doValidatorUpdate")
}
execCtx.events = append(execCtx.events, event.NewEvent("validator-update"))
server/v2/stf/stf_test.go
Outdated
| ctx.(*executionContext).events = append( | ||
| ctx.(*executionContext).events, | ||
| event.NewEvent("validate-tx", event.NewAttribute("sender", string(tx.Sender))), | ||
| event.NewEvent( | ||
| "validate-tx", | ||
| event.NewAttribute("sender", string(tx.Sender)), | ||
| event.NewAttribute("index", "2"), | ||
| ), | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Extract repeated code into a helper function to enhance maintainability
The pattern of asserting the context type and appending events is repeated across multiple functions. Consider creating a helper function to reduce code duplication and improve readability.
Create a helper function:
func appendEvent(ctx context.Context, evt *event.Event) error {
execCtx, ok := ctx.(*executionContext)
if !ok {
return errors.New("unexpected context type")
}
execCtx.events = append(execCtx.events, evt)
return nil
}Then modify the functions to use this helper:
// In doTxValidation:
err := appendEvent(ctx, event.NewEvent("validate-tx", event.NewAttribute("sender", string(tx.Sender))))
if err != nil {
return err
}
// Add additional events as needed.
server/v2/cometbft/streaming.go
Outdated
| if c.listener.StartBlock != nil { | ||
| if err := c.listener.StartBlock(appdata.StartBlockData{ | ||
| Height: uint64(height), | ||
| HeaderBytes: nil, // TODO: need to pass header bytes |
server/v2/cometbft/streaming.go
Outdated
| if err := c.listener.OnTx(appdata.TxData{ | ||
| TxIndex: int32(i), | ||
| Bytes: func() ([]byte, error) { return tx, nil }, | ||
| JSON: nil, // TODO: need to pass tx json |
server/v2/stf/stf_test.go
Outdated
| @@ -135,6 +173,57 @@ func TestSTF(t *testing.T) { | |||
| if txResult.GasWanted != mockTx.GasLimit { | |||
| t.Errorf("Expected GasWanted to be %d, got %d", mockTx.GasLimit, txResult.GasWanted) | |||
| } | |||
|
|
|||
| // check PreBlockEvents | |||
| require.Len(t, result.PreBlockEvents, 1) | |||
There was a problem hiding this comment.
since we have been avoiding pulling in stf already can it be removed here
There was a problem hiding this comment.
what do you mean by pulling?
| @@ -104,6 +106,11 @@ func (c *Consensus[T]) SetStreamingManager(sm streaming.Manager) { | |||
| c.streaming = sm | |||
| } | |||
|
|
|||
| // SetListener sets the listener for the consensus module. | |||
There was a problem hiding this comment.
I don't like to add setters tbh, I feel like it pollutes the server instantiation in root.go
Additionally, Consensus is wired from the cometbft server and not manually.
Let's instead add it to the Consensus contructor if possible and have the CometBFT ServerOptions take a listener.
There was a problem hiding this comment.
The idea of the separated setters is to make the listener a nil default since all nodes are not open to an off-chain indexer.
There was a problem hiding this comment.
That makes sense, the option can be nil by default then. But a user won't have to call NewConsensus manually, so putting it in a setter isn't useful
There was a problem hiding this comment.
When init the consensus from the config, if the indexer is set as on, then call the setter API.
I think it is more flexible.
There was a problem hiding this comment.
works too, could you wire that in cometbft server.go?
There was a problem hiding this comment.
can I do it in the next PR after baseapp integration? it seems not trivial, and have no idea of the indexer config, not exist???
There was a problem hiding this comment.
Can you add a TODO in cometbft/server.go after NewConsensus then?
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (1)
server/v2/stf/stf_test.go (1)
226-287: LGTM: Comprehensive test cases for TxEvents with a suggestion for improvementThe new test cases for TxEvents are exceptionally thorough and well-structured. They cover all important aspects including the number of events, event types, block stage, tx index, event indices, and attributes for different event types. The assertions are clear, specific, and cover different event types (validate-tx, handle-msg, post-tx-exec). It's particularly commendable that the tests verify the correct ordering and indexing of events, and the attribute checks are thorough.
To improve readability, consider extracting the event type checks into a separate function. For example:
func checkEventType(t *testing.T, event appdata.Event, expectedType string, expectedMsgIndex int32) { t.Helper() if event.Type != expectedType { t.Errorf("Expected event type '%s', got %s", expectedType, event.Type) } if event.MsgIndex != expectedMsgIndex { t.Errorf("Expected MsgIndex %d, got %d", expectedMsgIndex, event.MsgIndex) } } // Usage in the test: checkEventType(t, event, "validate-tx", 0)This would make the main test body more concise and easier to read.
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (3)
- server/v2/cometbft/go.mod (1 hunks)
- server/v2/stf/stf.go (10 hunks)
- server/v2/stf/stf_test.go (4 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- server/v2/cometbft/go.mod
- server/v2/stf/stf.go
🧰 Additional context used
📓 Path-based instructions (1)
server/v2/stf/stf_test.go (2)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"
🔇 Additional comments (7)
server/v2/stf/stf_test.go (7)
25-26: LGTM: Good use of constant for sender addressThe introduction of the
senderAddrconstant improves code maintainability and readability. It's consistently used throughout the test file, which is a good practice.
67-67: LGTM: Consistent use of senderAddr constantThe use of
senderAddrconstant inmockTxinitialization is consistent with the earlier change and improves maintainability.
178-191: LGTM: Comprehensive test cases for PreBlockEventsThe new test cases for PreBlockEvents are well-structured and comprehensive. They cover important aspects such as the number of events, event type, block stage, and event index. The assertions are clear and specific, which will help catch any regressions in the PreBlockEvents implementation.
192-205: LGTM: Comprehensive test cases for BeginBlockEventsThe new test cases for BeginBlockEvents are well-structured and comprehensive. They cover important aspects such as the number of events, event type, block stage, and event index. The assertions are clear and specific, and the structure is consistent with the PreBlockEvents tests. This consistency in testing approach is commendable.
206-225: LGTM: Comprehensive test cases for EndBlockEventsThe new test cases for EndBlockEvents are well-structured and comprehensive. They cover important aspects such as the number of events, event types, block stage, and event indices. The assertions are clear and specific, and the structure is consistent with the previous event tests. It's particularly good that the tests account for multiple events (end-block and validator-update) in the EndBlockEvents, showing attention to detail in the testing process.
294-294: LGTM: Consistent use of senderAddr constant in mock transactionThe use of the
senderAddrconstant in the mock transaction creation is consistent with earlier changes and improves maintainability by centralizing the sender address value.
Line range hint
1-487: Overall assessment: Significant improvements with minor suggestions for enhancementThis update to the STF test file introduces comprehensive event handling and testing, which greatly enhances the robustness of the STF implementation. The consistent use of constants (like
senderAddr) and the addition of thorough test cases for various event types demonstrate good coding practices and attention to detail.Key strengths:
- Comprehensive event handling across different block stages.
- Detailed and well-structured test cases for all event types.
- Consistent use of constants for improved maintainability.
Areas for improvement:
- Consider implementing safe type assertions throughout the file to prevent potential panics.
- Minor refactoring suggestions to improve code readability in test cases.
These changes significantly contribute to the reliability and testability of the STF implementation. Addressing the type assertion issue will further enhance the robustness of the code.
| doPreBlock: func(ctx context.Context, txs []mock.Tx) error { | ||
| ctx.(*executionContext).events = append(ctx.(*executionContext).events, event.NewEvent("pre-block")) | ||
| return nil | ||
| }, | ||
| doBeginBlock: func(ctx context.Context) error { | ||
| kvSet(t, ctx, "begin-block") | ||
| ctx.(*executionContext).events = append(ctx.(*executionContext).events, event.NewEvent("begin-block")) | ||
| return nil | ||
| }, | ||
| doEndBlock: func(ctx context.Context) error { | ||
| kvSet(t, ctx, "end-block") | ||
| ctx.(*executionContext).events = append(ctx.(*executionContext).events, event.NewEvent("end-block")) | ||
| return nil | ||
| }, | ||
| doValidatorUpdate: func(ctx context.Context) ([]appmodulev2.ValidatorUpdate, error) { return nil, nil }, | ||
| doValidatorUpdate: func(ctx context.Context) ([]appmodulev2.ValidatorUpdate, error) { | ||
| ctx.(*executionContext).events = append(ctx.(*executionContext).events, event.NewEvent("validator-update")) | ||
| return nil, nil | ||
| }, |
There was a problem hiding this comment.
Consider using safe type assertions for context
The event handling implementation is consistent across different block stages, which is good. However, the current implementation uses unsafe type assertions that could lead to panics if the context is of an unexpected type.
Consider using a safe type assertion pattern:
execCtx, ok := ctx.(*executionContext)
if !ok {
return fmt.Errorf("unexpected context type in doPreBlock")
}
execCtx.events = append(execCtx.events, event.NewEvent("pre-block"))Apply this pattern to doPreBlock, doBeginBlock, doEndBlock, and doValidatorUpdate functions.
| ctx.(*executionContext).events = append( | ||
| ctx.(*executionContext).events, | ||
| event.NewEvent("validate-tx", event.NewAttribute(senderAddr, string(tx.Sender))), | ||
| event.NewEvent( | ||
| "validate-tx", | ||
| event.NewAttribute(senderAddr, string(tx.Sender)), | ||
| event.NewAttribute("index", "2"), | ||
| ), | ||
| ) |
There was a problem hiding this comment.
Consistent event handling, but consider safe type assertion
The implementation of event handling in doTxValidation is consistent with the approach used in other functions and provides detailed event information. The use of senderAddr constant is also consistent.
However, the same unsafe type assertion issue is present. Consider using a safe type assertion pattern as suggested earlier:
execCtx, ok := ctx.(*executionContext)
if !ok {
return fmt.Errorf("unexpected context type in doTxValidation")
}
execCtx.events = append(
execCtx.events,
event.NewEvent("validate-tx", event.NewAttribute(senderAddr, string(tx.Sender))),
event.NewEvent(
"validate-tx",
event.NewAttribute(senderAddr, string(tx.Sender)),
event.NewAttribute("index", "2"),
),
)| ctx.(*executionContext).events = append( | ||
| ctx.(*executionContext).events, | ||
| event.NewEvent("post-tx-exec", event.NewAttribute(senderAddr, string(tx.Sender))), | ||
| event.NewEvent( | ||
| "post-tx-exec", | ||
| event.NewAttribute(senderAddr, string(tx.Sender)), | ||
| event.NewAttribute("index", "2"), | ||
| ), | ||
| ) |
There was a problem hiding this comment.
Consistent event handling in postTxExec, but consider safe type assertion
The event handling implementation in postTxExec is consistent with the approach used in other functions and provides detailed event information. The use of senderAddr constant is also consistent.
However, the same unsafe type assertion issue is present. Consider using a safe type assertion pattern as suggested earlier:
execCtx, ok := ctx.(*executionContext)
if !ok {
return fmt.Errorf("unexpected context type in postTxExec")
}
execCtx.events = append(
execCtx.events,
event.NewEvent("post-tx-exec", event.NewAttribute(senderAddr, string(tx.Sender))),
event.NewEvent(
"post-tx-exec",
event.NewAttribute(senderAddr, string(tx.Sender)),
event.NewAttribute("index", "2"),
),
)| ctx.(*executionContext).events = append( | ||
| ctx.(*executionContext).events, | ||
| event.NewEvent("handle-msg", event.NewAttribute("msg", msg.String())), | ||
| event.NewEvent( | ||
| "handle-msg", | ||
| event.NewAttribute("msg", msg.String()), | ||
| event.NewAttribute("index", "2"), | ||
| ), | ||
| ) |
There was a problem hiding this comment.
Consistent event handling in message handler, but consider safe type assertion
The event handling implementation in the message handler is consistent with the approach used in other functions and provides detailed event information.
However, the same unsafe type assertion issue is present. Consider using a safe type assertion pattern as suggested earlier:
execCtx, ok := ctx.(*executionContext)
if !ok {
return nil, fmt.Errorf("unexpected context type in message handler")
}
execCtx.events = append(
execCtx.events,
event.NewEvent("handle-msg", event.NewAttribute("msg", msg.String())),
event.NewEvent(
"handle-msg",
event.NewAttribute("msg", msg.String()),
event.NewAttribute("index", "2"),
),
)
julienrbrt
added
the
backport/v0.52.x
julienrbrt
changed the title
(cherry picked from commit bf95c81) # Conflicts: # server/v2/cometbft/go.mod # server/v2/stf/stf.go # server/v2/stf/stf_test.go
…port #21917) (#22051) Co-authored-by: cool-developer <[email protected]> Co-authored-by: Julien Robert <[email protected]>
Description
ref: #21312
ref: #21527
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
Summary by CodeRabbit
New Features
deliverTxmethod.Improvements
STFstruct to support new functionalities.Dependencies
cosmossdk.io/schema v0.3.1.