feat(indexer/server-v2): Listener integration in server/v2 #21917
Conversation
cool-develope
requested review from
a team,
facundomedica,
sontrinh16,
testinginprod and
kocubinski
as code owners
📝 WalkthroughWalkthroughThe pull request introduces enhancements to the Changes
Assessment against linked issues
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.
| @@ -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.
| @@ -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 | |
| } | |
| } | |
| } |
Description
Closes: #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
New Features
Consensusstruct to support a listener for streaming various block-related data.SetListenerfor setting the listener in theConsensusstruct.Improvements
DeliverBlockprocess.Dependencies
cosmossdk.io/schema v0.3.0.