Skip to content
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

Merged
merged 5 commits into from
Oct 21, 2024
Merged

Conversation

julienrbrt
Copy link
Member

@julienrbrt julienrbrt commented Oct 17, 2024

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...

  • included the correct type prefix in the PR title, you can find examples of the prefixes below:
  • confirmed ! in the type prefix if API or client breaking change
  • targeted the correct branch (see PR Targeting)
  • provided a link to the relevant issue or specification
  • reviewed "Files changed" and left comments if necessary
  • included the necessary unit and integration tests
  • added a changelog entry to CHANGELOG.md
  • updated the relevant documentation or specification, including comments for documenting Go code
  • confirmed all CI checks have passed

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...

  • confirmed the correct type prefix in the PR title
  • confirmed all author checklist items have been addressed
  • reviewed state machine logic, API design and naming, documentation is accurate, tests and test coverage

Summary by CodeRabbit

  • New Features

    • Introduced new methods for initializing and exporting application state.
    • Enhanced AppManager with new operational capabilities.
    • Added methods for closing the application and managing genesis state.
  • Bug Fixes

    • Improved error handling in query handler registration.
  • Documentation

    • Updated comments for clarity throughout the codebase.
  • Refactor

    • Renamed methods for consistency and clarity, removing unnecessary prefixes.
    • Streamlined method calls and variable access in various components.
    • Restructured Build method for simplified app manager creation.
  • Chores

    • Removed deprecated methods and fields to enhance code cleanliness.

Copy link
Contributor

coderabbitai bot commented Oct 17, 2024

📝 Walkthrough
📝 Walkthrough

Walkthrough

The pull request introduces significant modifications to the App struct and its associated methods in the runtime/v2 package. Key changes include the addition of new fields and methods, renaming existing methods for clarity, and restructuring how components interact, particularly regarding the application manager and query handlers. Additionally, several related files have been updated to reflect these changes, ensuring consistency across the codebase.

Changes

File Path Change Summary
runtime/v2/app.go Added fields (appm, storeLoader), added methods (initGenesis, exportGenesis, Close), renamed methods (GetAppManager to AppManager, etc.), and updated comments.
runtime/v2/builder.go Removed branch field, restructured Build method to use appmanager.New, removed DefaultGenesis method, and added initGenesis and exportGenesis methods.
server/v2/api/grpc/server.go Renamed method calls from GetQueryHandlers() to QueryHandlers() and updated related calls.
server/v2/api/rest/server.go Updated Init method to use appI directly instead of appI.GetAppManager().
server/v2/appmanager/appmanager.go Defined AppManager interface with new methods, changed implementation from struct to interface, and added New constructor.
server/v2/cometbft/abci.go Changed app field from pointer to value type and updated NewConsensus constructor accordingly.
server/v2/cometbft/server.go Streamlined method calls to access properties of appI.
server/v2/types.go Updated AppI[T] interface to inherit methods from appmanager.AppManager[T] and renamed methods to remove "Get" prefix.
simapp/v2/app_test.go Updated store access in NewTestApp and MoveNextBlock functions.
simapp/v2/export.go Updated store access in ExportAppStateAndValidators method.

Possibly related PRs

  • test(server/v2/cometbft): Add abci unit tests #21020: The changes in the main PR involve significant modifications to the App struct and its methods, which may relate to the testing framework introduced in this PR that validates various aspects of the consensus process, including initialization and block finalization.
  • ci: actually enable v2 system test #21539: The main PR's updates to method signatures and the introduction of new methods in the App struct may relate to the CI changes that ensure system tests are executed correctly, as they both involve structural changes to the application.
  • chore: remove duplicate proto files for the same proto file #21648: The refactoring of the App struct and its methods in the main PR may connect with the changes in this PR that simplify the interaction with blockchain nodes, as both involve modifications to how components communicate and function.
  • chore: remove consensus authority  #21734: The removal of the consensusAuthority field in the main PR aligns with the simplification of the Consensus struct, which is also a focus of this PR that aims to streamline the consensus engine.
  • feat: wire v2 handlers #22112: The introduction of new handlers in the main PR may relate to the wiring of version 2 handlers in this PR, as both involve enhancements to the modularity and functionality of the application.
  • refactor(runtime/v2): use AppI.GetStore() to fetch an initialized RootStore #22205: The main PR's focus on using AppI.GetStore() to fetch an initialized RootStore connects with the refactoring efforts in this PR, which also aims to simplify the store management processes.
  • fix(server/v2/cometbft): wire app closer #22240: The addition of the appCloser function pointer in the main PR enhances lifecycle management, which may relate to the changes in this PR that wire the application closer to the server components.
  • fix(server/v2): use only one logger for app and server #22241: The consolidation of logging mechanisms in the main PR aligns with the objective of this PR to use a single logger for both the application and server, enhancing consistency in logging practices.

Suggested labels

backport/v0.52.x

Suggested reviewers

  • facundomedica
  • kocubinski
  • sontrinh16
  • hieuvubk
  • tac0turtle

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?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@julienrbrt julienrbrt marked this pull request as draft October 17, 2024 09:47
Copy link
Contributor

@coderabbitai coderabbitai bot left a 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(), and SchemaDecoderResolver(). 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 to stfRouterWrapper 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 the handlers 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 to RegisterHandler. 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 for AppManager

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 name QueryHandlers

The comment for the QueryHandlers method still references the old method name GetQueryHandlers. 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 clarity

Consider reordering the parameters in the NewAppManager function to group related parameters together. Placing initGenesisImpl and exportGenesisImpl after stf 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 conventions

Per 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 copying

The methods InitGenesis, ExportGenesis, DeliverBlock, ValidateTx, Simulate, Query, and QueryWithState have value receivers (a AppManager[T]). Since AppManager 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 for initGenesis in InitGenesis method

In the InitGenesis method, a.initGenesis is used without checking if it is nil. This could lead to a nil pointer dereference if initGenesis was not properly initialized. Similar to the ExportGenesis method, consider adding a nil check before using a.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 changes

After applying the state changes in the InitGenesis method, the returned error err is used directly in the return statement. If ApplyStateChanges returns an error, blockResponse will be nil, potentially causing issues downstream. Ensure that the error is handled appropriately, and consider returning blockResponse 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 in DeliverBlock method

In the DeliverBlock method, the version check can be simplified for clarity. Instead of checking latestVersion+1 != block.Height, consider using an explicit error message when block.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 together

For 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

📥 Commits

Files that changed from the base of the PR and between f01baf3 and 232a23a.

📒 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() to Store() 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() to QueryHandlers() 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() to SchemaDecoderResolver() 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() to AppManager() 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() to QueryHandlers() 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() to AppManager() 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 when a.app.branch is nil

Setting a default branch using branch.DefaultNewWriterMap ensures that the application has a valid branch function if none is provided. This prevents potential nil dereference errors.


107-107: Including a.app.branch in stf.NewSTF parameters

Passing a.app.branch to stf.NewSTF ensures that the state transition function is aware of the branch mechanism, which is essential for managing state writes correctly.


114-124: Refactoring AppManager initialization for clarity

The 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 in fmt.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: Assigning appManager to a.app.appm

Assigning the newly created appManager to a.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 in AppBuilderWithBranch

By assigning the custom branch function to a.app.branch, the application can override the default branch behavior. This provides flexibility for customizing how state writes are managed.

server/v2/api/rest/server.go Outdated Show resolved Hide resolved
server/v2/store/server.go Show resolved Hide resolved
simapp/v2/app_di.go Show resolved Hide resolved
runtime/v2/module.go Show resolved Hide resolved
runtime/v2/manager.go Show resolved Hide resolved
runtime/v2/app.go Outdated Show resolved Hide resolved
runtime/v2/app.go Outdated Show resolved Hide resolved
runtime/v2/app.go Outdated Show resolved Hide resolved
server/v2/appmanager/appmanager.go Outdated Show resolved Hide resolved
@julienrbrt julienrbrt marked this pull request as ready for review October 17, 2024 10:10
Copy link
Contributor

@coderabbitai coderabbitai bot left a 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 handling

The changes look good and align with the PR objectives of simplifying the app manager. The use of app.Store() instead of app.GetStore() is consistent with the refactoring goals.

A minor suggestion for improvement:

Consider combining the error check for StateAt with the one for Read 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 the QueryHandlers method.

The renaming of GetQueryHandlers to QueryHandlers aligns well with Go naming conventions for getter methods. The removal of GetAppManager and GetStore methods successfully simplifies the mockApp 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() method

While 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 handling

The 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 handling

The 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

📥 Commits

Files that changed from the base of the PR and between 232a23a and 61f7539.

📒 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 changing appManager field to value type.

The appManager field in the DefaultHandler struct has been changed from a pointer type to a value type. This change aligns with the modification in the NewDefaultHandler function, but it's crucial to consider the following:

  1. Memory usage: If AppManager is a large struct, storing it by value in each DefaultHandler instance could increase memory consumption.
  2. Performance: Operations that modify the AppManager might now create copies instead of modifying the original, potentially affecting performance.
  3. 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 within DefaultHandler methods and potential areas that might need adjustment due to this change.


23-25: Consider the implications of changing appManager 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 the DefaultHandler struct, it's important to consider the following:

  1. Performance: If AppManager is a large struct, passing it by value might impact performance.
  2. Consistency: Ensure this change is reflected consistently across the codebase.
  3. 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 modified mockApp struct.

While the existing TestServer function provides good coverage for server initialization and configuration, it doesn't directly test the changes made to the mockApp struct. To ensure comprehensive test coverage:

  1. Add unit tests for the QueryHandlers method of mockApp.
  2. Verify that the removal of GetAppManager and GetStore 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() to app.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() to app.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() to app.Store() has been successfully verified across the codebase. The remaining GetStore() 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 initialization

The restructuring of the Build method significantly improves code clarity and maintainability. The use of appmanager.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 logging

The 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 practices

The restructuring of this file has significantly improved its modularity and separation of concerns. The addition of initGenesis and exportGenesis 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 interface

The 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 struct

The 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 constructor

The New function is correctly implemented as a constructor for AppManager. It uses generics consistently and includes all necessary parameters. All fields of the appManager struct are properly initialized, addressing the previous issue of uninitialized fields.


Line range hint 99-143: Comprehensive InitGenesis implementation

The 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 implementation

The ExportGenesis method is correctly implemented, matching the interface definition. The nil check for the exportGenesis function is a good practice to prevent nil pointer dereferences. The implementation appropriately delegates to the exportGenesis function.


Line range hint 152-175: Well-implemented DeliverBlock method

The 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 methods

The ValidateTx, Simulate, Query, and QueryWithState 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 access

The 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() and QueryHandlers()) enhance readability and maintain consistency with the overall refactoring effort.


140-140: LGTM: Consistent simplification of app property access

The change from appI.GetSchemaDecoderResolver() to appI.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 method

The removal of appI.GetAppManager() in favor of directly using appI is consistent with the changes made in the Init 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 CometBFTServer

The 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 new getProtoRegistry field.

A new field getProtoRegistry has been added to the Consensus 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:

  1. The purpose of this new field and how it will be used within the Consensus struct.
  2. Any performance considerations or benefits this caching mechanism provides.
  3. 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 to NewConsensus function.

The app parameter in the NewConsensus function has been changed from a pointer type to a value type. This change is consistent with the modification in the Consensus struct. However, it requires updating all calls to NewConsensus throughout the codebase to pass AppManager 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 changing app 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:

  1. Performance: If AppManager is a large struct, this could lead to increased memory usage and potential performance impacts when the Consensus struct is copied.
  2. Behavior: This change ensures that app is always initialized, potentially preventing nil pointer dereferences. However, it may also change how the app field is used throughout the codebase.
  3. 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 updated

The stf.New function is now called directly with the appropriate parameters, and error handling is properly implemented using require.NoError(t, err). This change simplifies the code and aligns with best practices.


675-687: Simplification of 'appmanager' initialization enhances readability

The 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.

@julienrbrt julienrbrt added the backport/v0.52.x PR scheduled for inclusion in the v0.52's next stable release label Oct 17, 2024
Copy link
Contributor

@coderabbitai coderabbitai bot left a 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

📥 Commits

Files that changed from the base of the PR and between 61f7539 and d6cd414.

📒 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' prefix

The method GetQueryHandlers() has been renamed to QueryHandlers(), 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 in Query method may affect implementations

The Query method's msg parameter type has changed from gogoproto.Message to transaction.Msg. This narrows the accepted message types and may impact existing implementations that rely on the broader gogoproto.Message interface. Ensure that all implementations of the Query 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 use transaction.Msg as the msg parameter.

✅ Verification successful

All Query method implementations have been successfully updated to use transaction.Msg. No instances of gogoproto.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 2

Length 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 2

Length of output: 2469

server/v2/api/grpc/server.go Show resolved Hide resolved
// 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 {
Copy link
Member

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

Copy link
Member Author

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!

Copy link
Member

@tac0turtle tac0turtle left a 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

@julienrbrt julienrbrt added this pull request to the merge queue Oct 21, 2024
Merged via the queue into main with commit 681366e Oct 21, 2024
72 of 73 checks passed
@julienrbrt julienrbrt deleted the julien/refactor-app branch October 21, 2024 13:50
mergify bot pushed a commit that referenced this pull request Oct 21, 2024
(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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport/v0.52.x PR scheduled for inclusion in the v0.52's next stable release C:server/v2 api C:server/v2 appmanager C:server/v2 cometbft C:server/v2 stf C:server/v2 Issues related to server/v2
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants