-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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(runtime/v2): simplify app manager #22300
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThe pull request introduces significant modifications to the Changes
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
|
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: 9
🧹 Outside diff range and nitpick comments (12)
server/v2/types.go (1)
20-23
: Summary of interface changes and their impact.The changes to the
AppI[T transaction.Tx]
interface consistently remove the "Get" prefix from four method names:AppManager()
,QueryHandlers()
,Store()
, andSchemaDecoderResolver()
. These changes align well with Go naming conventions and improve the overall readability of the interface.While these changes enhance code quality, they may have a widespread impact on the codebase. Ensure that all implementations of this interface and all calls to these methods throughout the project are updated accordingly.
Consider running a project-wide search and replace operation to update all occurrences of these method names. Additionally, if this is a public API, make sure to document these changes clearly in the changelog and update any relevant documentation or examples.
runtime/v2/manager.go (2)
Line range hint
844-844
: Consider documenting the new error handling approach.The addition of the
error
field tostfRouterWrapper
suggests a shift in error handling strategy. This allows for accumulating multiple errors during the registration process, which can be beneficial for comprehensive error reporting.Consider adding a comment explaining this new error handling approach and how it should be used. Also, ensure that all methods using this struct are updated to handle the accumulated errors appropriately.
Line range hint
855-863
: LGTM! Consider a small optimization.The changes in the
RegisterHandler
method are consistent with the new error accumulation approach. Storing successfully registered handlers in thehandlers
map is a good practice for later retrieval or validation.Consider initializing the
handlers
map in the struct's constructor or when the struct is first created, rather than checking and initializing it in every call toRegisterHandler
. This would slightly improve performance and simplify the method:type stfRouterWrapper struct { stfRouter *stf.MsgRouterBuilder error error - handlers map[string]appmodulev2.Handler + handlers map[string]appmodulev2.Handler = make(map[string]appmodulev2.Handler) } func (s *stfRouterWrapper) RegisterHandler(handler appmodulev2.Handler) { // ... existing code ... - if s.error == nil { - s.handlers = map[string]appmodulev2.Handler{} - } s.handlers[requestName] = handler }runtime/v2/app.go (2)
148-148
: Correct the typo in the method comment forAppManager
The comment for the
AppManager
method contains a typo: "appamanger" should be "AppManager". Additionally, ensuring the correctness of comments enhances code readability and maintainability.Apply this diff to correct the typo:
-// AppManager returns the app's appamanger +// AppManager returns the app's AppManager
153-153
: Update the method comment to match the method nameQueryHandlers
The comment for the
QueryHandlers
method still references the old method nameGetQueryHandlers
. To maintain consistency and clarity, please update the comment to reflect the current method name.Apply this diff to update the comment:
-// GetQueryHandlers returns the query handlers. +// QueryHandlers returns the query handlers.server/v2/appmanager/appmanager.go (7)
43-49
: Reorder parameters in the constructor for clarityConsider reordering the parameters in the
NewAppManager
function to group related parameters together. PlacinginitGenesisImpl
andexportGenesisImpl
afterstf
improves readability by grouping configuration parameters followed by function implementations.Apply this diff to reorder the parameters:
func NewAppManager[T transaction.Tx]( config Config, db Store, stf StateTransitionFunction[T], + initGenesisImpl InitGenesis, + exportGenesisImpl ExportGenesis, - initGenesisImpl InitGenesis, - exportGenesisImpl ExportGenesis, ) (*AppManager[T], error) {
31-36
: Simplify struct field comments following Go conventionsPer the Uber Golang style guide, comments for struct fields should be concise and begin with the field name. The current comments are verbose and include parameter details, which are more suitable for function documentation. Consider simplifying these comments to make them more concise.
Apply the following changes:
-// InitGenesis is a function that initializes the application state from a genesis file. -// It takes a context, a source reader for the genesis file, and a transaction handler function. +// initGenesis initializes the application state from a genesis file. initGenesis InitGenesis -// ExportGenesis is a function that exports the application state to a genesis file. -// It takes a context and a version number for the genesis file. +// exportGenesis exports the application state to a genesis file. exportGenesis ExportGenesis
Line range hint
71-208
: Use pointer receivers for methods to avoid unnecessary copyingThe methods
InitGenesis
,ExportGenesis
,DeliverBlock
,ValidateTx
,Simulate
,Query
, andQueryWithState
have value receivers(a AppManager[T])
. SinceAppManager
is a struct with multiple fields, using value receivers may result in unnecessary copying on each method call. To optimize performance, consider changing the receivers to pointers.Apply the following diff:
-func (a AppManager[T]) InitGenesis( +func (a *AppManager[T]) InitGenesis( -func (a AppManager[T]) ExportGenesis(ctx context.Context, version uint64) ([]byte, error) { +func (a *AppManager[T]) ExportGenesis(ctx context.Context, version uint64) ([]byte, error) { -func (a AppManager[T]) DeliverBlock( +func (a *AppManager[T]) DeliverBlock( -func (a AppManager[T]) ValidateTx(ctx context.Context, tx T) (server.TxResult, error) { +func (a *AppManager[T]) ValidateTx(ctx context.Context, tx T) (server.TxResult, error) { -func (a AppManager[T]) Simulate(ctx context.Context, tx T) (server.TxResult, corestore.WriterMap, error) { +func (a *AppManager[T]) Simulate(ctx context.Context, tx T) (server.TxResult, corestore.WriterMap, error) { -func (a AppManager[T]) Query(ctx context.Context, version uint64, request transaction.Msg) (transaction.Msg, error) { +func (a *AppManager[T]) Query(ctx context.Context, version uint64, request transaction.Msg) (transaction.Msg, error) { -func (a AppManager[T]) QueryWithState(ctx context.Context, state corestore.ReaderMap, request transaction.Msg) (transaction.Msg, error) { +func (a *AppManager[T]) QueryWithState(ctx context.Context, state corestore.ReaderMap, request transaction.Msg) (transaction.Msg, error) {
Line range hint
95-102
: Add nil check forinitGenesis
inInitGenesis
methodIn the
InitGenesis
method,a.initGenesis
is used without checking if it is nil. This could lead to anil
pointer dereference ifinitGenesis
was not properly initialized. Similar to theExportGenesis
method, consider adding a nil check before usinga.initGenesis
.Apply the following diff:
func (a *AppManager[T]) InitGenesis( ctx context.Context, blockRequest *server.BlockRequest[T], initGenesisJSON []byte, txDecoder transaction.Codec[T], ) (*server.BlockResponse, corestore.WriterMap, error) { + if a.initGenesis == nil { + return nil, nil, errors.New("init genesis function not set") + } var genTxs []T
Line range hint
112-115
: Handle potential error when applying state changesAfter applying the state changes in the
InitGenesis
method, the returned errorerr
is used directly in the return statement. IfApplyStateChanges
returns an error,blockResponse
will benil
, potentially causing issues downstream. Ensure that the error is handled appropriately, and consider returningblockResponse
only when no error has occurred.Apply the following diff:
err = genesisState.ApplyStateChanges(stateChanges) if err != nil { return nil, nil, fmt.Errorf("failed to apply block zero state changes to genesis state: %w", err) } -return blockResponse, genesisState, err +return blockResponse, genesisState, nil
Line range hint
153-160
: Simplify version check inDeliverBlock
methodIn the
DeliverBlock
method, the version check can be simplified for clarity. Instead of checkinglatestVersion+1 != block.Height
, consider using an explicit error message whenblock.Height
is not the next expected height.Apply the following diff:
if latestVersion+1 != block.Height { - return nil, nil, fmt.Errorf("invalid DeliverBlock height wanted %d, got %d", latestVersion+1, block.Height) + return nil, nil, fmt.Errorf("expected block height %d, but got %d", latestVersion+1, block.Height) }
31-41
: Group related struct fields togetherFor better readability, consider grouping related fields in the
AppManager
struct. Place the function fields (initGenesis
,exportGenesis
,stf
) together, and the data fields (config
,db
) together.Apply the following diff:
// AppManager is a coordinator for all things related to an application type AppManager[T transaction.Tx] struct { // Gas limits for validating, querying, and simulating transactions. config Config + // The database for storing application data. + db Store // InitGenesis initializes the application state from a genesis file. initGenesis InitGenesis // ExportGenesis exports the application state to a genesis file. exportGenesis ExportGenesis // The state transition function for processing transactions. stf StateTransitionFunction[T] - // The database for storing application data. - db Store }
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (13)
- runtime/v2/app.go (3 hunks)
- runtime/v2/builder.go (3 hunks)
- runtime/v2/manager.go (2 hunks)
- runtime/v2/module.go (1 hunks)
- server/v2/api/grpc/server.go (1 hunks)
- server/v2/api/rest/server.go (1 hunks)
- server/v2/appmanager/appmanager.go (1 hunks)
- server/v2/appmanager/appmanager_builder.go (0 hunks)
- server/v2/appmanager/config.go (0 hunks)
- server/v2/appmanager/genesis.go (1 hunks)
- server/v2/store/server.go (1 hunks)
- server/v2/types.go (1 hunks)
- simapp/v2/app_di.go (1 hunks)
💤 Files with no reviewable changes (2)
- server/v2/appmanager/appmanager_builder.go
- server/v2/appmanager/config.go
✅ Files skipped from review due to trivial changes (1)
- server/v2/appmanager/genesis.go
🧰 Additional context used
📓 Path-based instructions (10)
runtime/v2/app.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.runtime/v2/manager.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.server/v2/api/grpc/server.go (1)
Pattern
**/*.go
: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.server/v2/api/rest/server.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.server/v2/store/server.go (1)
Pattern
**/*.go
: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.server/v2/types.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.
🔇 Additional comments (12)
server/v2/types.go (4)
22-22
: Approve method renaming and suggest impact verification.The renaming of
GetStore()
toStore()
follows the same pattern as the previous changes, maintaining consistency within the interface and aligning with Go naming conventions. This change contributes to a more idiomatic and readable codebase.To ensure this change doesn't break existing code, please run the following command to check for any remaining uses of
GetStore()
:#!/bin/bash # Search for any remaining uses of GetStore() rg --type go 'GetStore\(\)'
21-21
: Approve method renaming and suggest impact verification.The renaming of
GetQueryHandlers()
toQueryHandlers()
is consistent with the previous change and aligns well with Go naming conventions. This change enhances the overall readability of the interface.To ensure this change doesn't break existing code, please run the following command to check for any remaining uses of
GetQueryHandlers()
:#!/bin/bash # Search for any remaining uses of GetQueryHandlers() rg --type go 'GetQueryHandlers\(\)'
23-23
: Approve method renaming and suggest impact verification.The renaming of
GetSchemaDecoderResolver()
toSchemaDecoderResolver()
completes the set of changes in this interface, maintaining consistency and adhering to Go naming conventions. This change, along with the others, significantly improves the interface's readability and alignment with idiomatic Go code.To ensure this change doesn't break existing code, please run the following command to check for any remaining uses of
GetSchemaDecoderResolver()
:#!/bin/bash # Search for any remaining uses of GetSchemaDecoderResolver() rg --type go 'GetSchemaDecoderResolver\(\)'
20-20
: Approve method renaming and suggest impact verification.The renaming of
GetAppManager()
toAppManager()
aligns well with Go naming conventions. This change improves readability and follows idiomatic Go practices for getter methods.To ensure this change doesn't break existing code, please run the following command to check for any remaining uses of
GetAppManager()
:✅ Verification successful
Verified: No remaining uses of
GetAppManager()
found.The renaming has been successfully verified, and there are no instances of
GetAppManager()
remaining in the codebase.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining uses of GetAppManager() rg --type go 'GetAppManager\(\)'Length of output: 120
server/v2/api/grpc/server.go (2)
60-60
: LGTM: Method name change aligns with Go conventions.The change from
GetQueryHandlers()
toQueryHandlers()
is consistent with Go naming conventions, which discourage the use of "Get" prefixes for getter methods. This change improves code clarity and maintainability.
67-67
: LGTM: Consistent method name change, verify interface update.The change from
GetAppManager()
toAppManager()
is consistent with Go naming conventions and aligns with the previous modification. This suggests a systematic update across the codebase to improve naming consistency.To ensure the interface has been updated correctly, please run the following command:
This will help confirm that the
appI
interface has been updated to reflect these method name changes.runtime/v2/builder.go (6)
70-71
: Properly setting default branch whena.app.branch
isnil
Setting a default branch using
branch.DefaultNewWriterMap
ensures that the application has a valid branch function if none is provided. This prevents potentialnil
dereference errors.
107-107
: Includinga.app.branch
instf.NewSTF
parametersPassing
a.app.branch
tostf.NewSTF
ensures that the state transition function is aware of the branch mechanism, which is essential for managing state writes correctly.
114-124
: RefactoringAppManager
initialization for clarityThe introduction of
appmanager.NewAppManager
with the provided configuration simplifies the app manager's initialization. This change enhances code readability and modularity by consolidating the setup into a single method.
126-126
: Consistent error handling with%w
verb infmt.Errorf
Using
fmt.Errorf
with the%w
verb correctly wraps the original error, allowing for error unwrapping and preserving the error chain. This aligns with Go's best practices for error handling.
128-128
: AssigningappManager
toa.app.appm
Assigning the newly created
appManager
toa.app.appm
integrates the app manager into the application structure effectively, ensuring that the app has access to all app manager functionalities.
139-139
: Setting custom branch inAppBuilderWithBranch
By assigning the custom
branch
function toa.app.branch
, the application can override the default branch behavior. This provides flexibility for customizing how state writes are managed.
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: 0
🧹 Outside diff range and nitpick comments (5)
simapp/v2/export.go (1)
Line range hint
32-41
: LGTM with a minor suggestion for error handlingThe changes look good and align with the PR objectives of simplifying the app manager. The use of
app.Store()
instead ofapp.GetStore()
is consistent with the refactoring goals.A minor suggestion for improvement:
Consider combining the error check for
StateAt
with the one forRead
to reduce nesting and improve readability:readerMap, err := app.Store().StateAt(latestHeight) if err != nil { return exportedApp, err } genesisCtx := services.NewGenesisContext(readerMap) if err := genesisCtx.Read(ctx, func(ctx context.Context) error { exportedApp.Validators, err = staking.WriteValidators(ctx, app.StakingKeeper) return err }); err != nil { return exportedApp, err }This change would flatten the error handling structure slightly.
server/v2/server_test.go (1)
38-40
: LGTM! Consider adding a comment for theQueryHandlers
method.The renaming of
GetQueryHandlers
toQueryHandlers
aligns well with Go naming conventions for getter methods. The removal ofGetAppManager
andGetStore
methods successfully simplifies themockApp
interface, which is in line with the PR objective.Consider adding a brief comment to explain the purpose of the
QueryHandlers
method:// QueryHandlers returns a map of query handlers for the mock app. func (*mockApp[T]) QueryHandlers() map[string]appmodulev2.Handler { return map[string]appmodulev2.Handler{} }simapp/v2/app_test.go (1)
Line range hint
1-158
: Consider adding specific tests for Store() methodWhile the existing tests cover the basic functionality and indirectly test the changes made, it might be beneficial to add specific tests for the
Store()
method. This would ensure that the refactored method behaves correctly in various scenarios.Would you like assistance in drafting additional test cases for the
Store()
method?runtime/v2/builder.go (2)
128-157
: Well-structured initGenesis method with comprehensive error handlingThe new
initGenesis
method is well-implemented with proper error handling and state checks. It effectively manages the initialization of the app's genesis state.Consider adding a comment explaining the purpose of the
TODO
on line 144:// TODO: genesis state may be > 0, we need to set version on store if v != 0 { return nil, errors.New("cannot init genesis on non-zero state") }This will help future developers understand the reasoning behind this check and the potential need for modification.
159-181
: Well-implemented exportGenesis method with proper error handlingThe new
exportGenesis
method is well-structured and effectively manages the export of the app's genesis data. The error handling is comprehensive, ensuring robust operation.For consistency with the rest of the file, consider using
fmt.Errorf
for the error on line 173:if err != nil { return nil, fmt.Errorf("failed to export genesis: %w", err) }This change would make the error handling style consistent throughout the file.
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (14)
- runtime/v2/app.go (2 hunks)
- runtime/v2/builder.go (2 hunks)
- server/v2/api/grpc/server.go (1 hunks)
- server/v2/api/rest/handler.go (1 hunks)
- server/v2/api/rest/server.go (1 hunks)
- server/v2/appmanager/appmanager.go (7 hunks)
- server/v2/cometbft/abci.go (2 hunks)
- server/v2/cometbft/abci_test.go (2 hunks)
- server/v2/cometbft/server.go (2 hunks)
- server/v2/server_test.go (1 hunks)
- server/v2/stf/stf.go (1 hunks)
- server/v2/types.go (1 hunks)
- simapp/v2/app_test.go (2 hunks)
- simapp/v2/export.go (1 hunks)
✅ Files skipped from review due to trivial changes (1)
- server/v2/stf/stf.go
🚧 Files skipped from review as they are similar to previous changes (4)
- runtime/v2/app.go
- server/v2/api/grpc/server.go
- server/v2/api/rest/server.go
- server/v2/types.go
🧰 Additional context used
📓 Path-based instructions (9)
runtime/v2/builder.go (1)
Pattern
**/*.go
: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.server/v2/api/rest/handler.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.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/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"server/v2/cometbft/server.go (1)
Pattern
**/*.go
: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.server/v2/server_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"simapp/v2/app_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"simapp/v2/export.go (1)
Pattern
**/*.go
: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
🔇 Additional comments (24)
server/v2/api/rest/handler.go (2)
26-29
: Evaluate the impact of changingappManager
field to value type.The
appManager
field in theDefaultHandler
struct has been changed from a pointer type to a value type. This change aligns with the modification in theNewDefaultHandler
function, but it's crucial to consider the following:
- Memory usage: If
AppManager
is a large struct, storing it by value in eachDefaultHandler
instance could increase memory consumption.- Performance: Operations that modify the
AppManager
might now create copies instead of modifying the original, potentially affecting performance.- Concurrency: This change might affect how
AppManager
is shared across goroutines if that was a consideration in the original design.To assess the impact of this change, you can run the following script:
#!/bin/bash # Description: Analyze AppManager usage within DefaultHandler methods # Search for DefaultHandler methods echo "Searching for DefaultHandler methods:" ast-grep --lang go --pattern $'type DefaultHandler[T transaction.Tx] struct { $$$ } func (h *DefaultHandler[T]) $$$($$$) $$$' # Search for appManager usage within DefaultHandler methods echo "\nSearching for appManager usage within DefaultHandler methods:" rg --type go "func \(h \*DefaultHandler\[T\]\)" -A 10 | rg "h\.appManager"This will help identify how
appManager
is used withinDefaultHandler
methods and potential areas that might need adjustment due to this change.
23-25
: Consider the implications of changingappManager
from pointer to value type.The function signature has been modified to accept
appManager
as a value instead of a pointer. While this change is consistent with the modification in theDefaultHandler
struct, it's important to consider the following:
- Performance: If
AppManager
is a large struct, passing it by value might impact performance.- Consistency: Ensure this change is reflected consistently across the codebase.
- Intended behavior: Confirm that passing
AppManager
by value aligns with the intended usage and doesn't introduce any unexpected side effects.To verify the impact of this change, you can run the following script:
This will help identify other areas of the code that might be affected by this change.
server/v2/server_test.go (1)
Line range hint
45-88
: Consider adding tests for the modifiedmockApp
struct.While the existing
TestServer
function provides good coverage for server initialization and configuration, it doesn't directly test the changes made to themockApp
struct. To ensure comprehensive test coverage:
- Add unit tests for the
QueryHandlers
method ofmockApp
.- Verify that the removal of
GetAppManager
andGetStore
methods doesn't negatively impact the test scenarios.To check the current test coverage, you can run:
This will help identify if the
QueryHandlers
method is being adequately tested.simapp/v2/app_test.go (2)
111-111
: LGTM! Consistent with previous change.The change from
app.GetStore()
toapp.Store()
is consistent with the previous modification, maintaining uniformity throughout the codebase. This refactoring improves code readability without altering functionality.
75-75
: LGTM! Verify consistency across the codebase.The change from
app.GetStore()
toapp.Store()
improves code readability and follows Go naming conventions. This refactoring appears to be part of a larger effort to simplify the API.To ensure consistency, let's verify if this change has been applied throughout the codebase:
✅ Verification successful
Consistent Renaming Confirmed
The change from
app.GetStore()
toapp.Store()
has been successfully verified across the codebase. The remainingGetStore()
methods are located in auto-generated files and do not affect this refactoring.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining instances of GetStore() method rg --type go 'GetStore\(\)'Length of output: 424
runtime/v2/builder.go (3)
Line range hint
96-126
: Improved app manager creation and initializationThe restructuring of the
Build
method significantly improves code clarity and maintainability. The use ofappmanager.New
consolidates the app manager creation and configuration, making the code more concise and easier to understand. This change aligns well with the PR's objective of simplifying the application manager.
132-134
: Improved error handling and loggingThe updates to error handling throughout the file are commendable. The error messages are now more descriptive and consistent, making debugging easier. The consistent use of
fmt.Errorf
with error wrapping (%w
) is a good practice that preserves the error chain.Also applies to: 136-138, 140-143, 150-152, 162-164, 172-174, 177-179
Line range hint
1-181
: Improved overall structure and adherence to Go best practicesThe restructuring of this file has significantly improved its modularity and separation of concerns. The addition of
initGenesis
andexportGenesis
as separate methods enhances readability and maintainability. The code style consistently adheres to Go best practices and the Uber Go Style Guide.These changes align well with the PR's objective of simplifying the application manager and improve the overall quality of the codebase.
server/v2/appmanager/appmanager.go (7)
15-52
: Well-defined AppManager interfaceThe new
AppManager
interface is well-structured and provides a clear contract for implementing types. The method signatures are consistent, follow Go naming conventions, and include helpful comments for each method, enhancing readability and maintainability.
66-80
: Well-structured appManager structThe
appManager
struct is well-organized with fields covering all necessary components. The use of generics with[T transaction.Tx]
allows for flexibility in transaction types. Each field is accompanied by a descriptive comment, enhancing code readability.
82-96
: Well-implemented New constructorThe
New
function is correctly implemented as a constructor forAppManager
. It uses generics consistently and includes all necessary parameters. All fields of theappManager
struct are properly initialized, addressing the previous issue of uninitialized fields.
Line range hint
99-143
: Comprehensive InitGenesis implementationThe
InitGenesis
method is well-implemented, matching the interface definition. It correctly handles genesis transactions and state initialization. The error handling is thorough, with descriptive error messages that will aid in debugging.
144-151
: Correct ExportGenesis implementationThe
ExportGenesis
method is correctly implemented, matching the interface definition. The nil check for theexportGenesis
function is a good practice to prevent nil pointer dereferences. The implementation appropriately delegates to theexportGenesis
function.
Line range hint
152-175
: Well-implemented DeliverBlock methodThe
DeliverBlock
method is correctly implemented, matching the interface definition. It includes a proper check for the correct block height before processing, which is crucial for maintaining blockchain integrity. The error handling is thorough with descriptive messages, and the method correctly utilizes the state transition function (stf) to deliver the block.
Line range hint
177-222
: Correctly implemented transaction and query methodsThe
ValidateTx
,Simulate
,Query
, andQueryWithState
methods are all correctly implemented, matching their respective interface definitions. The implementations are concise and efficiently delegate to the appropriate components (stf, db). Gas limits from the configuration are correctly applied, ensuring proper resource management. Error handling is present where necessary, contributing to the robustness of the code.server/v2/cometbft/server.go (4)
105-113
: LGTM: Simplified app manager accessThe changes in this segment improve code clarity and align with the PR objective of simplifying the app manager. The direct use of
appI
and the more concise method calls (Store()
andQueryHandlers()
) enhance readability and maintain consistency with the overall refactoring effort.
140-140
: LGTM: Consistent simplification of app property accessThe change from
appI.GetSchemaDecoderResolver()
toappI.SchemaDecoderResolver()
is consistent with the other simplifications in this file. It maintains the pattern of direct access to app properties, improving code readability and consistency.
109-109
: LGTM: Consistent simplification in Start methodThe removal of
appI.GetAppManager()
in favor of directly usingappI
is consistent with the changes made in theInit
method. This modification further simplifies the code and aligns well with the PR's objective of streamlining the app manager interface.
Line range hint
1-324
: LGTM: Successful refactoring of CometBFTServerThe changes in this file successfully refactor the
CometBFTServer
to use a simplified app manager interface. The modifications consistently replace getter methods with direct property access, improving code clarity and readability. These changes align well with the PR's objective of simplifying the app manager.The refactored code maintains adherence to the Uber Golang style guide and introduces no new issues or inconsistencies. Overall, this refactoring effort enhances the maintainability of the
CometBFTServer
implementation.server/v2/cometbft/abci.go (3)
Line range hint
110-110
: Clarify the purpose and impact of the newgetProtoRegistry
field.A new field
getProtoRegistry
has been added to theConsensus
struct initialization:getProtoRegistry: sync.OnceValues(gogoproto.MergedRegistry),This addition introduces new functionality related to protocol buffers registry. The use of
sync.OnceValues
suggests that the merged registry will be computed only once and then cached for subsequent uses.Please provide more context on:
- The purpose of this new field and how it will be used within the
Consensus
struct.- Any performance considerations or benefits this caching mechanism provides.
- Whether this change requires any additional error handling or affects the behavior of other methods in the struct.
#!/bin/bash # Check for usages of getProtoRegistry in the codebase rg --type go 'getProtoRegistry'
80-80
: Update all calls toNewConsensus
function.The
app
parameter in theNewConsensus
function has been changed from a pointer type to a value type. This change is consistent with the modification in theConsensus
struct. However, it requires updating all calls toNewConsensus
throughout the codebase to passAppManager
by value instead of by pointer.Please ensure that all instances where
NewConsensus
is called have been updated to reflect this change.#!/bin/bash # Check for any remaining calls to NewConsensus with a pointer AppManager rg --type go 'NewConsensus\([^)]*\*appmanager\.AppManager[^)]*\)'
45-45
: Consider the implications of changingapp
field to value type.The
app
field type has been changed from a pointer (*appmanager.AppManager[T]
) to a value type (appmanager.AppManager[T]
). This change may have several implications:
- Performance: If
AppManager
is a large struct, this could lead to increased memory usage and potential performance impacts when theConsensus
struct is copied.- Behavior: This change ensures that
app
is always initialized, potentially preventing nil pointer dereferences. However, it may also change how theapp
field is used throughout the codebase.- Mutability: With a value type, any modifications to
app
will be local to the method unless the field is reassigned.Please ensure that this change is intentional and that all usages of
app
throughout the codebase have been updated accordingly.✅ Verification successful
Verification Successful: No remaining pointer usages of
AppManager
found.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for any remaining pointer usages of AppManager in the codebase rg --type go 'app\s+\*appmanager\.AppManager'Length of output: 67
Script:
#!/bin/bash # Re-check for any remaining pointer usages of AppManager in the codebase with the correct pattern rg --type go 'app\s+\*appmanager\.AppManager'Length of output: 47
server/v2/cometbft/abci_test.go (2)
Line range hint
649-664
: Initialization of 'stf' instance is correctly updatedThe
stf.New
function is now called directly with the appropriate parameters, and error handling is properly implemented usingrequire.NoError(t, err)
. This change simplifies the code and aligns with best practices.
675-687
: Simplification of 'appmanager' initialization enhances readabilityThe direct initialization of
appmanager.New
with the necessary parameters removes the previous builder pattern, leading to clearer and more maintainable code. The configuration and dependencies are correctly set up, and error handling is appropriately managed.
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 selected for processing (3)
- server/v2/api/grpc/server.go (2 hunks)
- server/v2/cometbft/abci_test.go (2 hunks)
- server/v2/server_test.go (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- server/v2/cometbft/abci_test.go
- server/v2/server_test.go
🧰 Additional context used
📓 Path-based instructions (1)
server/v2/api/grpc/server.go (1)
Pattern
**/*.go
: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
🔇 Additional comments (2)
server/v2/api/grpc/server.go (2)
60-60
: Improved method naming: Omitted 'Get' prefixThe method
GetQueryHandlers()
has been renamed toQueryHandlers()
, which aligns with Go conventions by omitting the 'Get' prefix in accessor methods. According to the Uber Go Style Guide, getter methods should avoid using the 'Get' prefix unless necessary for disambiguation. This change enhances code readability and consistency.
88-88
: Type change inQuery
method may affect implementationsThe
Query
method'smsg
parameter type has changed fromgogoproto.Message
totransaction.Msg
. This narrows the accepted message types and may impact existing implementations that rely on the broadergogoproto.Message
interface. Ensure that all implementations of theQuery
method are updated to accommodate this change and that no required functionality is lost.To confirm that all implementations have been updated, you can run the following script:
This script searches for all
Query
method implementations and displays their signatures, helping you verify that they now usetransaction.Msg
as themsg
parameter.✅ Verification successful
All
Query
method implementations have been successfully updated to usetransaction.Msg
. No instances ofgogoproto.Message
were found, ensuring that existing implementations are consistent with the type change.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find all implementations of the Query method and check their signatures. # Search for all implementations of the Query method. rg --type go 'func\s+\(\w+\s+\*?\w+\)\s+Query\s*\(\s*ctx\s+context\.Context,\s*version\s+uint64,\s*msg\s+(gogoproto\.Message|transaction\.Msg)\s*\)' -A 2Length of output: 155
Script:
#!/bin/bash # Description: Find all implementations of the Query method and list their signatures to verify parameter types. # Search for all Query method implementations and display their signatures. rg --type go 'func\s+\(\w+\s+\*?\w+\)\s+Query\s*\(' -A 2Length of output: 2469
// AppManager is a coordinator for all things related to an application | ||
// It is responsible for interacting with stf and store. | ||
// Runtime/v2 is an extension of this interface. | ||
type AppManager[T transaction.Tx] interface { |
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.
if this is moved to cometbft we drop the dependency from cometbft on appamanger. This follows the design in which appmanager does not depend on stf as well
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.
we cannot really move it, but I can copy it to cometbft, sure!
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.
left one comment, otherwise looks good
(cherry picked from commit 681366e) # Conflicts: # runtime/v2/app.go # runtime/v2/builder.go # runtime/v2/manager.go # runtime/v2/module.go # server/v2/api/grpc/server.go # server/v2/api/rest/handler.go # server/v2/api/rest/server.go # server/v2/appmanager/appmanager.go # server/v2/appmanager/config.go # server/v2/appmanager/genesis.go # server/v2/appmanager/stf.go # server/v2/cometbft/server.go # server/v2/server_test.go # server/v2/stf/stf.go # server/v2/store/server.go # server/v2/types.go
Description
Simplify runtime/v2 and appmanager by removing builder for consistency.
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
AppManager
with new operational capabilities.Bug Fixes
Documentation
Refactor
Build
method for simplified app manager creation.Chores