feat(schema): update event index definitions #21565
Conversation
WalkthroughWalkthroughThe changes introduce a new field, Changes
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
|
|
@aaronc your pull request is missing a changelog! |
…es' into aaronc/aaronc-schema-event-updates
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (1)
- schema/appdata/data.go (2 hunks)
Additional context used
Path-based instructions (1)
schema/appdata/data.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Additional comments not posted (2)
schema/appdata/data.go (2)
52-68: Review of theEventstruct changes:The changes to the
Eventstruct introduce a new fieldBlockStageand redefineTxIndex,MsgIndex, andEventIndexto be 1-based, with zero indicating unknown values. This aligns with the PR objectives to enhance clarity and functionality.
- BlockStage Field: The addition of the
BlockStagefield (lines 52-54) is well-documented and clearly specifies its purpose and default value. This is a positive change that enhances the semantic understanding of the event's context within the block processing lifecycle.- Index Fields: The changes to the index fields (lines 56-68) are crucial as they now clearly define the starting index as 1, and zero as an indicator of unknown values. This is a significant improvement in clarity for developers interacting with these fields.
Overall, these changes are well-aligned with the PR objectives and should improve the usability and understanding of the
Eventstruct.
81-99: Review of theBlockStagetype and constants:The introduction of the
BlockStagetype (lines 81-82) and its associated constants (lines 84-99) is a structured approach to defining various stages of block processing. This addition is beneficial for several reasons:
- Clear Definitions: Each stage of block processing is clearly defined with its own constant, making it easier for developers to reference and understand the specific stages of an event within the block lifecycle.
- Use of
iota: The use ofiotafor initializing constants ensures that these values are unique and incremented correctly, which is a good practice in Go for defining related constants.This enhancement aids in the clarity and functionality of event indexing within the SDK, as outlined in the PR objectives.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (1)
- schema/appdata/data.go (2 hunks)
Additional context used
Path-based instructions (1)
schema/appdata/data.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Additional comments not posted (2)
schema/appdata/data.go (2)
52-68: Review of theEventstruct modifications:The addition of the
BlockStagefield and the updates to the index fields (TxIndex,MsgIndex,EventIndex) are well-documented and align with the PR objectives to enhance clarity and functionality. The comments clearly specify the 1-based indexing and conditions for unknown indices, which improves the semantic understanding of these fields.
- BlockStage Field: The addition of this field with a clear comment about its purpose and the default value (
UnknownBlockStage) is a good practice. It helps in understanding the context of the event within the block processing stages.- Index Fields: The clarification that these indices are 1-based unless explicitly set to zero (indicating unknown) is crucial for avoiding off-by-one errors and makes the data structure more intuitive.
Overall, these changes are well-aligned with the PR's goals and should enhance the usability and maintenance of the event indexing system.
81-99: Review of theBlockStagetype and constants:The definition of the
BlockStagetype and its associated constants (UnknownBlockStage,PreBlockStage,BeginBlockStage,TxProcessingStage,EndBlockStage) is a significant improvement in terms of type safety and code readability. Using an enumerated type (int32) for these constants ensures that they are lightweight and efficient for comparisons, which is ideal for performance-sensitive applications like blockchain technologies.
- Type Safety: Defining
BlockStageas a distinct type rather than using a generic integer enhances type safety, making the code less prone to errors.- Readability and Maintenance: The use of enumerated constants makes the code more readable and easier to maintain. It clearly delineates the stages of block processing, which is crucial for developers working with this part of the Cosmos SDK.
This approach follows best practices in Go programming and aligns well with the Uber Golang style guide by promoting clear type definitions and the use of constants for known values.
|
Thanks for the reviews @julienrbrt and @tac0turtle. @cool-develope does this look good enough for you or is there anything you would change or add? |
I still think we should add attributes for those: https://github.com/cosmos/cosmos-sdk/blob/v0.50.9/baseapp/baseapp.go#L1075-L1100 (Which I believe was going to be done in a follow-up already). |
So this code from BaseApp is just adding a new event for a message. There is nothing that prevents such an event from being created using this event model. It's basically the same as far as event producers are concerned and we don't need new attributes here. But in appdata we could also add an OnMessage callback specifically for that. I think sender attribute will make more sense in the context of RFC 003 and x/accounts when that model is more first class. |
Description
This updates the
appdata.EventTxIndex,MsgIndexaEventIndexdocumentation to precisely specify their use and to handle the case where we do not know the index, making these indexes 1-based. Special tx index constants for pre-block, begin-block and end-block processing are also added.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
BlockStagefield in event data for better clarity on block processing stages.Documentation