refactor(runtime/v2): genesis event service bindings #22192
Conversation
kocubinski
requested review from
julienrbrt,
hieuvubk and
a team
as code owners
|
@kocubinski your pull request is missing a changelog! |
| @@ -129,14 +143,34 @@ func NewGenesisHeaderService(executionService header.Service) *GenesisHeaderServ | |||
| } | |||
| } | |||
|
|
|||
| type readonlyKVStore struct { | |||
| store.Reader | |||
| type GenesisEventService struct { | |||
There was a problem hiding this comment.
go doc on why it exists please
|
|
||
| // blackHoleEventManager is an event.Manager that does nothing. | ||
| // It is used during genesis initialization, genesis events are not emitted. | ||
| type blackHoleEventManager struct{} |
kocubinski
requested review from
auricom,
tac0turtle,
cool-develope,
alpe,
facundomedica and
testinginprod
as code owners
github-prbot
assigned alpe, auricom, cool-develope, facundomedica, tac0turtle and testinginprod
kocubinski
changed the title
📝 Walkthrough📝 WalkthroughWalkthroughThe pull request introduces changes to the Changes
Assessment against linked 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:Simulations
C:Store
C:server/v2
kocubinski
changed the title
julienrbrt
changed the title
kocubinski
force-pushed
the
kocu/sims2_v2_dev
branch
from
9f6f889 to
4bb71dc
Compare
github-actions
bot
removed
C:Simulations
C:Store
C:server/v2
kocubinski
force-pushed
the
kocu/sims2_v2_dev
branch
from
4bb71dc to
10fcb68
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
runtime/v2/services/genesis.go (1)
162-168: Add GoDoc comments for exported methodsThe exported methods
EventManager,Emit, andEmitKVlack GoDoc comments. The Uber Go Style Guide recommends adding comments to all exported functions and methods, starting with the function name, to enhance code readability.Consider adding comments like the following:
// EventManager returns an event.Manager for the given context. // During genesis initialization, it returns a blackHoleEventManager. func (g *GenesisEventService) EventManager(ctx context.Context) event.Manager { ... } // Emit does nothing as this is a black hole event manager. func (b *blackHoleEventManager) Emit(_ transaction.Msg) error { ... } // EmitKV does nothing as this is a black hole event manager. func (b *blackHoleEventManager) EmitKV(_ string, _ ...event.Attribute) error { ... }Also applies to: 176-181
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (2)
- runtime/v2/module.go (1 hunks)
- runtime/v2/services/genesis.go (3 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
runtime/v2/module.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.runtime/v2/services/genesis.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
🔇 Additional comments (1)
runtime/v2/module.go (1)
246-246: LGTM! Verify impact on genesis process.The change to wrap
stf.NewEventService()withservices.NewGenesisEventService()looks good. This modification aligns with the PR objectives and may help address the issues related to genesis service event bindings.To ensure this change resolves the panic error mentioned in issue #21924, please run the following verification steps:
These tests will help ensure that the new
GenesisEventServiceis properly implemented and used throughout the codebase, potentially preventing the panic error during node startup from a state dump.✅ Verification successful
Verified!
All checks pass, and the wrapping of
stf.NewEventService()withservices.NewGenesisEventService()is correctly implemented without introducing new issues.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the impact of the new GenesisEventService on the genesis process # Test 1: Check for any remaining direct usage of stf.NewEventService() echo "Checking for any remaining direct usage of stf.NewEventService():" rg --type go "stf\.NewEventService\(\)" # Test 2: Verify the implementation of NewGenesisEventService echo "Checking the implementation of NewGenesisEventService:" rg --type go -A 10 "func NewGenesisEventService" # Test 3: Look for any potential issues or TODOs in the GenesisEventService implementation echo "Checking for potential issues or TODOs in GenesisEventService:" rg --type go -i "todo|fixme|xxx" -g "*genesis*event*.go"Length of output: 1474
Script:
#!/bin/bash # Description: Re-verify potential issues or TODOs in GenesisEventService with corrected file pattern # Test 3: Look for any potential issues or TODOs in the GenesisEventService implementation echo "Re-checking for potential issues or TODOs in GenesisEventService:" rg --type go -i "todo|fixme|xxx" -g "*genesis.go"Length of output: 672
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (4)
runtime/v2/services/genesis.go (3)
147-160: LGTM: Well-structured new GenesisEventServiceThe new
GenesisEventServicestruct andNewGenesisEventServicefunction are well-implemented and follow Go conventions. The purpose is clear from the comments, and the function correctly returns anevent.Serviceinterface.Consider adding a brief comment to the
executionServicefield in theGenesisEventServicestruct to explain its purpose, similar to other structs in this file.
171-182: LGTM: Correct implementation of blackHoleEventManagerThe
blackHoleEventManagerand its methodsEmitandEmitKVare correctly implemented as no-op versions of theevent.Managerinterface. This is appropriate for discarding events during genesis initialization.Consider renaming
blackHoleEventManagerto something more conventional, such asnoopEventManagerordiscardEventManager, to align with common Go naming practices while still conveying its purpose.
147-151: Add Go doc comment for GenesisEventServiceTo address the previous review comment and improve documentation, consider adding a proper Go doc comment for the
GenesisEventServicestruct. This will provide better documentation that can be picked up by Go tools. For example:// GenesisEventService is an event.Service implementation used during // genesis initialization. It wraps an inner execution context event.Service // and returns a blackHoleEventManager into which events enter and disappear // completely during genesis initialization. type GenesisEventService struct { executionService event.Service }runtime/v2/module.go (1)
244-246: LGTM! Consider applying consistent wrapping to other services.The change to
eventServiceinitialization looks good. It wraps thestf.NewEventService()withservices.NewGenesisEventService(), which aligns with the PR objectives of refactoring the genesis event service bindings.For consistency, you might want to consider applying similar wrapping to
headerServiceand potentiallycometServiceif they also require genesis-specific functionality. This would make the initialization pattern uniform across all services.
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (2)
- runtime/v2/module.go (1 hunks)
- runtime/v2/services/genesis.go (3 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
runtime/v2/module.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.runtime/v2/services/genesis.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
🔇 Additional comments (4)
runtime/v2/services/genesis.go (4)
5-5: LGTM: New imports added correctlyThe new imports for "errors" and "cosmossdk.io/core/transaction" are correctly added and align with the new functionality introduced in the file.
Also applies to: 11-11
115-121: LGTM: Improved error handling in Set and Delete methodsThe changes to the
SetandDeletemethods are good. Returning errors instead of panicking is a better practice, allowing callers to handle the errors gracefully. The error messages are clear and descriptive.
141-144: LGTM: Improved function signatureThe change in the return type from
*GenesisHeaderServicetoheader.Serviceis a good improvement. It promotes loose coupling and aligns with Go's interface-based design principles. This change will make the code more flexible and easier to test.
163-169: LGTM: Well-implemented EventManager methodThe
EventManagermethod is correctly implemented. It appropriately checks for the genesis context and returns the correct event manager based on the context. The logic is consistent with other similar methods in the file, maintaining a coherent design.
| type readonlyKVStore struct { | ||
| store.Reader | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using an explicit field instead of embedding the interface
Embedding the store.Reader interface directly in the readonlyKVStore struct can lead to unintended method implementations. It's generally better to use an explicit field for better control and clarity.
Consider modifying the struct as follows:
type readonlyKVStore struct {
reader store.Reader
}This change would require updating the methods to use r.reader instead of directly calling the methods.
Description
Closes: #21924
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
GenesisEventServicefor improved event handling during genesis initialization.blackHoleEventManagerto suppress event emissions during genesis.Enhancements
These changes aim to improve the application's event management capabilities, especially during the initialization phase, ensuring a smoother and more efficient user experience.