-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
refactor(stf): remove RunWithCtx #21739
Conversation
WalkthroughWalkthroughThe changes involve modifying the Changes
Possibly related PRs
Recent review detailsConfiguration used: .coderabbit.yml Files ignored due to path filters (2)
Files selected for processing (3)
Files not reviewed due to no reviewable changes (1)
Files skipped from review as they are similar to previous changes (2)
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
|
runtime/v2/services/genesis.go
Outdated
return g.state, nil | ||
} | ||
|
||
type GenesisKVStoreServie struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo.
There was a problem hiding this 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
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files ignored due to path filters (2)
runtime/v2/go.sum
is excluded by!**/*.sum
simapp/v2/go.sum
is excluded by!**/*.sum
Files selected for processing (13)
- core/header/service.go (3 hunks)
- core/store/service.go (2 hunks)
- runtime/v2/builder.go (3 hunks)
- runtime/v2/go.mod (1 hunks)
- runtime/v2/module.go (5 hunks)
- runtime/v2/services/genesis.go (1 hunks)
- server/v2/appmanager/appmanager.go (3 hunks)
- server/v2/appmanager/genesis.go (1 hunks)
- server/v2/appmanager/types.go (0 hunks)
- server/v2/stf/stf.go (0 hunks)
- simapp/v2/app_di.go (1 hunks)
- simapp/v2/go.mod (1 hunks)
- simapp/v2/simdv2/cmd/root_di.go (1 hunks)
Files not reviewed due to no reviewable changes (2)
- server/v2/appmanager/types.go
- server/v2/stf/stf.go
Files skipped from review due to trivial changes (1)
- core/header/service.go
Additional context used
Path-based instructions (8)
server/v2/appmanager/genesis.go (1)
Pattern
**/*.go
: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.core/store/service.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.simapp/v2/simdv2/cmd/root_di.go (1)
Pattern
**/*.go
: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.server/v2/appmanager/appmanager.go (1)
Pattern
**/*.go
: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.simapp/v2/app_di.go (1)
Pattern
**/*.go
: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.runtime/v2/module.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.
Additional comments not posted (26)
server/v2/appmanager/genesis.go (1)
14-27
: LGTM!The changes to the
InitGenesis
function signature and comments are well-aligned with the Uber Go Style Guide and improve the functionality and documentation of the function.Please ensure that all callers of the
InitGenesis
function are updated to handle the returnedstore.WriterMap
and error. You can use the following script to verify the usage ofInitGenesis
:Verification successful
To properly verify the changes and their impact, we need to gather more specific information about the usage of this
InitGenesis
function. Let's start by searching for its direct callers.
Let's broaden our search to gather more information about the usage of
InitGenesis
and related concepts.
To further verify the changes and their impact, let's examine the AppManager's InitGenesis method more closely.
LGTM! The changes to InitGenesis are well-implemented and consistent.
The modifications to the
InitGenesis
function inserver/v2/appmanager/genesis.go
have been correctly implemented. The function now returns astore.WriterMap
along with the*server.BlockResponse
anderror
. This change is consistently applied in theAppManager
'sInitGenesis
method.Key points:
- The new signature is correctly used and propagated.
- Error handling is comprehensive throughout the implementation.
- The
store.WriterMap
(genesisState) is properly utilized to represent the genesis state.- While there are no direct callers of
appmanager.InitGenesis
, it's being indirectly called through the CometBFT ABCI interface, maintaining compatibility.The changes appear to be part of a larger refactoring to improve state management during genesis initialization. The implementation is sound and should integrate well with the rest of the system.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all callers of `InitGenesis` handle the returned `store.WriterMap` and error. # Test: Search for the function usage. Expect: Callers should handle the returned values. rg --type go -A 5 $'InitGenesis'Length of output: 102909
Script:
#!/bin/bash # Search for direct callers of InitGenesis in the appmanager package rg --type go -A 5 "appmanager\.InitGenesis"Length of output: 45
Script:
#!/bin/bash # Search for any usage of InitGenesis in the appmanager package echo "Searching for InitGenesis in appmanager package:" rg --type go -A 5 "InitGenesis" server/v2/appmanager/ # Search for store.WriterMap usage echo "\nSearching for store.WriterMap usage:" rg --type go -A 5 "store\.WriterMap" # Look for recent changes in error handling related to genesis initialization echo "\nSearching for error handling changes in genesis initialization:" rg --type go -A 5 "InitGenesis.*error"Length of output: 35830
Script:
#!/bin/bash # Check the AppManager's InitGenesis method echo "Examining AppManager's InitGenesis method:" rg --type go -A 20 "func \(a AppManager\[T\]\) InitGenesis" server/v2/appmanager/appmanager.go # Look for potential indirect callers of InitGenesis echo "\nSearching for potential indirect callers of InitGenesis:" rg --type go -A 5 "InitGenesis\(" --glob !server/v2/appmanager/genesis.goLength of output: 61017
core/store/service.go (2)
13-13
: LGTM!The new
KVStoreServiceFactory
function type provides a flexible way to create instances ofKVStoreService
based on a byte slice input. The naming and syntax are correct.
23-23
: LGTM!The new
MemoryStoreServiceFactory
function type provides a flexible way to create instances ofMemoryStoreService
based on a byte slice input. The naming and syntax are correct.runtime/v2/services/genesis.go (5)
21-25
: LGTM!The
NewGenesisContext
function correctly initializes thegenesisContext
struct with the provided state.
27-37
: LGTM!The
Run
method correctly executes the provided function within the context of thegenesisContext
and handles the return values appropriately.
44-52
: LGTM!The
NewGenesisKVService
function correctly initializes theGenesisKVStoreServie
struct with the provided actor and execution service.
76-82
: LGTM!The
HeaderInfo
method correctly returns header information based on the provided context. If thegenesisContext
is found, it returns an emptyheader.Info
struct, otherwise, it delegates the call to the underlyingexecutionService
.
84-88
: LGTM!The
NewGenesisHeaderService
function correctly initializes theGenesisHeaderService
struct with the provided execution service.runtime/v2/go.mod (1)
8-8
: LGTM!The addition of the replacement directive for
cosmossdk.io/core
is a good change. It allows the module to be sourced from a local directory, which can facilitate local development and testing.The change is unlikely to have any negative impact on the module's functionality and may improve the development workflow.
simapp/v2/simdv2/cmd/root_di.go (1)
40-40
: LGTM! The addition ofruntime.DefaultServiceBindings()
enhances the dependency injection setup.This change integrates default service bindings into the dependency injection configuration, which may streamline service management and improve the modularity and maintainability of the codebase. It aligns with the PR objective of refactoring the codebase.
Potential benefits:
- Standardizes service instantiation and resolution
- Improves modularity and maintainability
- Simplifies the dependency injection setup
server/v2/appmanager/appmanager.go (4)
47-58
: LGTM!The changes to the
InitGenesis
method simplify the initialization process by directly callinginitGenesis
with the necessary parameters. The error handling for decoding the genesis transaction remains intact, ensuring that any issues during this process are still reported accurately. The changes enhance clarity and reduce complexity.
86-87
: LGTM!The changes to the
ExportGenesis
method reduce unnecessary complexity by directly checking if theexportGenesis
function is set before attempting to call it. The error handling is straightforward, as it directly returns an error if the function is not defined. The changes improve the readability of the code.
90-90
: LGTM!The changes to the
ExportGenesis
method simplify the export process by directly returning the result of theexportGenesis
call if the function is defined. The error handling is handled by the calling function. The changes enhance the maintainability and readability of the code.
159-161
: LGTM!The
QueryWithState
method provides a way to process a query with a temporary or uncommitted state, which can be useful in certain scenarios. The method uses theQuery
method of thestf
field to execute the query, ensuring consistency with other query methods. The method is well-documented, explaining its purpose and use case.simapp/v2/app_di.go (1)
74-74
: LGTM!The addition of
runtime.DefaultServiceBindings()
to theappConfig
initialization process is a good change. It integrates default service bindings into the application's dependency injection configuration, which may enhance the application's ability to manage service dependencies more effectively.runtime/v2/module.go (6)
179-181
: LGTM!The introduction of factory parameters for service instantiation enhances the modularity and flexibility of the
ProvideEnvironment
function. This change allows for easier testing and promotes better separation of concerns.
203-203
: LGTM!Instantiating the
kvService
using thekvFactory
aligns with the factory-based approach and promotes flexibility and testability.
207-207
: LGTM!Instantiating the
memKvService
using thememFactory
aligns with the factory-based approach and promotes flexibility and testability.
215-215
: LGTM!Instantiating the
HeaderService
using theheaderFactory
aligns with the factory-based approach and promotes flexibility and testability.
226-227
: LGTM!Renaming the
wrapper
parameter tobuilder
improves the clarity of the parameter's purpose and enhances code readability.
240-259
: LGTM!The introduction of the
DefaultServiceBindings
function streamlines the service binding process and enhances the clarity of service dependencies within the application. This change aligns with the overall refactoring goal of improving modularity and configurability.runtime/v2/builder.go (4)
162-193
: Excellent enhancements to theInitGenesis
function!The changes significantly improve the genesis initialization process:
- The state check ensures genesis is only initialized on a zero state, preventing inconsistencies.
- The use of
GenesisContext
provides a more structured approach to handling the genesis state.- The improved error handling provides clearer error reporting for various failure points.
Great work on enhancing the robustness and clarity of the genesis initialization process!
196-199
: Good addition of the state check in theExportGenesis
function.The state check ensures that the latest state is retrieved before attempting to export the genesis data. This change aligns well with the modifications made to the
InitGenesis
function and enhances the consistency and reliability of the genesis export process.
200-206
: Good use ofGenesisContext
in theExportGenesis
function.The utilization of
GenesisContext
allows for a more organized execution of the export logic. This change aligns well with the modifications made to theInitGenesis
function and provides a consistent and structured approach to handling the genesis state during both initialization and export processes.
6-6
: LGTM!The import of the
errors
package aligns with the improved error handling in theInitGenesis
function and does not introduce any issues.simapp/v2/go.mod (1)
289-289
: LGTM! The new replacement directive forcosmossdk.io/core
facilitates local development.The addition of the replacement directive for
cosmossdk.io/core
allows thesimapp/v2
module to utilize a local version of thecore
package instead of fetching it from the original repository. This change facilitates local development and testing by enabling developers to work with a modified version of thecore
package without needing to publish changes to a remote repository.The other existing replacements remain unchanged, indicating that the integration with other parts of the Cosmos SDK ecosystem continues as before.
There was a problem hiding this 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
Outside diff range and nitpick comments (1)
server/v2/cometbft/abci_test.go (1)
689-690
: Add a TODO comment to track the missing implementation.Since the actual genesis initialization logic is missing, consider adding a TODO comment to track this pending work:
InitGenesis: func(ctx context.Context, src io.Reader, txHandler func(json.RawMessage) error) (store.WriterMap, error) { // TODO: Implement the genesis initialization logic return nil, nil },
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (1)
- server/v2/cometbft/abci_test.go (1 hunks)
Additional context used
Path-based instructions (1)
server/v2/cometbft/abci_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"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK!
Can we check if we can make genesis export concurrent with this method? (Basically checking if the module manager changes from #21554 are working out of the box with this)
Because of the CometBFT server changes and simapp/v2 changes adding the backport label |
}, | ||
ExportGenesis: func(ctx context.Context, version uint64) ([]byte, error) { | ||
genesisJson, err := a.app.moduleManager.ExportGenesisForModules(ctx) | ||
_, state, err := a.app.db.StateLatest() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
_, state, err := a.app.db.StateLatest() | |
state, err := a.app.db.StateAt(version) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looked valid @kocubinski btw.
@kocubinski, I attempted to use |
Can you share a link to the code you ran and tested? I see #21554 is still using |
(cherry picked from commit d6364b8) # Conflicts: # core/header/service.go # core/store/service.go # runtime/v2/builder.go # runtime/v2/go.mod # runtime/v2/go.sum # runtime/v2/module.go # server/v2/appmanager/appmanager.go # server/v2/appmanager/genesis.go # server/v2/appmanager/types.go # server/v2/stf/stf.go # simapp/v2/go.mod # simapp/v2/go.sum
Co-authored-by: Matt Kocubinski <[email protected]> Co-authored-by: Julien Robert <[email protected]>
Description
This resolves a TODO by removing stf.RunWithCtx. Instead some indirection with services is used to create a bespoke genesis context (with support for fewer services).
Tested so far with:
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.md
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...
Summary by CodeRabbit
New Features
InitGenesis
function to provide a detailed state representation alongside error reporting.Bug Fixes
RunWithCtx
function to simplify theSTF[T]
struct interface and reduce potential confusion.Chores