feat(server/v2): init the indexer in server/v2 #22218
Conversation
cool-develope
requested review from
facundomedica,
sontrinh16,
a team,
julienrbrt and
hieuvubk
as code owners
📝 Walkthrough📝 Walkthrough📝 WalkthroughWalkthroughThe changes introduce a new method Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
github-actions
bot
added
C:server/v2
|
@cool-develope your pull request is missing a changelog! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
server/v2/types.go (1)
21-22: LGTM: New method correctly added to AppI interface.The
GetSchemaDecoderResolvermethod is a good addition that aligns with the PR objectives. The method signature follows Go naming conventions, and the return type matches the newly importeddecodingpackage.Consider slightly rewording the comment for clarity:
- // It is used to get the module schema resolver for the indexer. + // GetSchemaDecoderResolver returns the module schema resolver for the indexer.This change makes the comment more directly describe the method's action.
runtime/v2/app.go (1)
101-108: LGTM: New method enhances schema decoding functionality.The
GetSchemaDecoderResolvermethod is well-implemented and aligns with the PR objectives. It efficiently creates a map of module names to modules and uses it to initialize aModuleSetDecoderResolver.A minor optimization suggestion:
Consider pre-allocating the
moduleSetmap with the known size:-moduleSet := map[string]any{} +moduleSet := make(map[string]any, len(a.moduleManager.Modules()))This can slightly improve performance by avoiding potential map resizing.
server/v2/cometbft/server.go (1)
23-28: Organize import statements according to the Uber Go Style GuideThe import statements at lines 23, 27, and 28 are not grouped correctly. According to the Uber Go Style Guide, imports should be grouped in the following order with blank lines separating them:
- Standard library packages
- Third-party packages
- Local packages
This improves code readability and maintainability.
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (4)
- runtime/v2/app.go (2 hunks)
- server/v2/cometbft/server.go (2 hunks)
- server/v2/config.go (1 hunks)
- server/v2/types.go (2 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
runtime/v2/app.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.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/config.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.
🔇 Additional comments (4)
server/v2/types.go (1)
10-10: LGTM: Import statement is correctly added and used.The new import for the
decodingpackage is properly placed and follows the Uber Golang style guide. It's grouped correctly with other external imports and is necessary for the new method signature.server/v2/config.go (1)
47-48: Excellent simplification and efficiency improvement!The changes to the
UnmarshalSubConfigfunction are well-implemented:
- The use of the comma-ok idiom (
if val, ok := cfg[subName]; ok) is idiomatic Go and aligns with the Uber Golang Style Guide.- This change simplifies the code by removing the unnecessary loop, making it more readable and efficient.
- The new implementation directly checks for the existence of the key in the map, which is more performant, especially for large configurations.
Great job on improving both the clarity and efficiency of this function!
runtime/v2/app.go (1)
11-11: LGTM: Import statement is correctly placed and necessary.The new import for the
decodingpackage is properly placed and follows the Uber Go Style Guide for import grouping. It's required for the newly addedGetSchemaDecoderResolvermethod.server/v2/cometbft/server.go (1)
155-155: VerifyGetSchemaDecoderResolverdoes not return nilThe
Resolverfield inindexer.IndexingOptionsis set usingappI.GetSchemaDecoderResolver(). Ensure that this method does not returnnilto avoid a potential nil pointer dereference during indexer initialization.Run the following script to check if
GetSchemaDecoderResolvercould returnnil:
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| for moduleName, module := range a.moduleManager.Modules() { | ||
| moduleSet[moduleName] = module | ||
| } |
Check warning
Code scanning / CodeQL
Iteration over map Warning
julienrbrt
changed the title
server/v2/cometbft/server.go
Outdated
| @@ -145,6 +148,19 @@ func (s *CometBFTServer[T]) Init(appI serverv2.AppI[T], cfg map[string]any, logg | |||
| } | |||
| consensus.snapshotManager = snapshots.NewManager(snapshotStore, s.serverOptions.SnapshotOptions(cfg), sc, ss, nil, s.logger) | |||
|
|
|||
| // initialize the indexer | |||
| if indexerCfg, ok := cfg[IndexerConfigKey]; ok { | |||
There was a problem hiding this comment.
where is the config created? should it be part of the comet config?
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
⛔ Files ignored due to path filters (3)
server/v2/cometbft/go.sumis excluded by!**/*.sumserver/v2/go.sumis excluded by!**/*.sumsimapp/v2/go.sumis excluded by!**/*.sum
📒 Files selected for processing (7)
- server/v2/cometbft/abci.go (0 hunks)
- server/v2/cometbft/config.go (3 hunks)
- server/v2/cometbft/go.mod (1 hunks)
- server/v2/cometbft/server.go (2 hunks)
- server/v2/go.mod (1 hunks)
- simapp/v2/go.mod (1 hunks)
- tools/confix/data/v2-app.toml (2 hunks)
💤 Files with no reviewable changes (1)
- server/v2/cometbft/abci.go
✅ Files skipped from review due to trivial changes (1)
- simapp/v2/go.mod
🚧 Files skipped from review as they are similar to previous changes (1)
- server/v2/go.mod
🧰 Additional context used
📓 Path-based instructions (2)
server/v2/cometbft/config.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.server/v2/cometbft/server.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
🔇 Additional comments (8)
server/v2/cometbft/config.go (3)
6-6: LGTM: New import added correctly.The new import for the
indexerpackage is correctly placed and necessary for the newIndexerfield in theAppTomlConfigstruct.
47-47: LGTM: Indexer field added to AppTomlConfig struct.The
Indexerfield is correctly added to theAppTomlConfigstruct with appropriate type, struct tags, and comment. This addition aligns well with the existing structure and style of the config.
27-31: LGTM: Indexer field added with default values.The
Indexerfield is correctly added to theDefaultAppTomlConfigfunction. However, could you clarify the reasoning behind the empty string forTypeand empty map forConfig? Are these intended as placeholders for user configuration?✅ Verification successful
Verified: Indexer field addition is appropriate and consistent.
All instances of
indexer.Configinitialization are confined toserver/v2/cometbft/config.goand relevant test files. The emptyTypeandConfigfields serve as intended default placeholders for user configuration.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify if there are any other occurrences of indexer.Config initialization rg --type go 'indexer\.Config\s*{'Length of output: 138
Script:
#!/bin/bash # Verify if there are any other occurrences of indexer.Config initialization with corrected regex rg --type go 'indexer\.Config\s*\{'Length of output: 252
tools/confix/data/v2-app.toml (3)
24-27: Clarify the indexer type configurationThe
typefield in the[comet.indexer]section is currently empty. This might lead to issues if a default indexer type is not automatically applied.Could you please clarify if this is intentional or if a default value should be specified? If a default value is expected, consider adding it to the configuration.
93-93: Verify telemetry address changeThe telemetry server address has been changed from 'localhost:1338' to 'localhost:1318'.
Could you please confirm if this change is intentional? If so, ensure that all relevant documentation and any dependent systems are updated to reflect this new address. If not, please revert to the original address.
Line range hint
24-93: Summary of configuration changesThe changes to
v2-app.tomlintroduce new indexer configuration options and update the telemetry address. These modifications align with the PR objectives of initializing the indexer in server/v2. However, there are a few points that require attention:
- The indexer type is not specified.
- An unsupported indexer filter configuration is included.
- The telemetry address has been changed.
Please address the comments above to improve the clarity and reliability of the configuration file.
server/v2/cometbft/go.mod (1)
25-25: Dependency update looks good, but verify compatibility.The update of
cosmossdk.io/schemato versionv0.3.1-0.20241010102526-3a03804c148dis in line with the PR objectives. However, as this appears to be a pre-release version, please ensure that it's compatible with the rest of the codebase and doesn't introduce any breaking changes.To verify compatibility, please run the following commands:
server/v2/cometbft/server.go (1)
148-160: Initialization of the indexer is implemented correctlyThe code correctly initializes the indexer when the
Indexerconfiguration is provided and itsTypeis not empty. The logic integrates the indexer listener with the consensus, and error handling is appropriately implemented.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (3)
server/v2/cometbft/config.go (1)
46-46: LGTM: Indexer field added correctly to AppTomlConfig struct.The
Indexerfield is properly added to theAppTomlConfigstruct with appropriate tags and a descriptive comment. Its placement at the end of the struct follows the Uber Golang style guide recommendations for adding new fields.Consider expanding the comment to provide more context about the indexer's purpose and its relationship to the SDK. For example:
Indexer indexer.IndexingConfig `mapstructure:"indexer" toml:"indexer" comment:"indexer defines the configuration for the SDK built-in indexer implementation, which is responsible for indexing application events and enabling efficient querying of blockchain data."`server/v2/cometbft/server.go (2)
Line range hint
69-81: Avoid usingpanicfor error handlingUsing
panicfor error handling is discouraged, especially in libraries or servers, as it can cause the program to exit unexpectedly. Instead, return errors to allow the caller to handle them gracefully.Apply this diff to handle errors without panicking:
if chainID == "" { // fallback to genesis chain-id reader, err := os.Open(filepath.Join(home, "config", "genesis.json")) if err != nil { - panic(err) + return fmt.Errorf("failed to open genesis file: %w", err) } defer reader.Close() chainID, err = genutiltypes.ParseChainIDFromGenesis(reader) if err != nil { - panic(fmt.Errorf("failed to parse chain-id from genesis file: %w", err)) + return fmt.Errorf("failed to parse chain-id from genesis file: %w", err) } }
23-28: Organize import statements according to Go conventionsAlthough import sorting is generally handled by tools, ensure that the import statements are grouped and ordered correctly as per Go conventions. Standard library imports should be grouped separately from third-party and project-specific imports.
Consider organizing imports like this:
import ( "context" "crypto/sha256" "encoding/json" "fmt" "os" "path/filepath" + // Standard library imports + abciserver "github.com/cometbft/cometbft/abci/server" cmtcmd "github.com/cometbft/cometbft/cmd/cometbft/commands" cmtcfg "github.com/cometbft/cometbft/config" "github.com/cometbft/cometbft/node" "github.com/cometbft/cometbft/p2p" "github.com/cometbft/cometbft/proxy" "github.com/spf13/cobra" "github.com/spf13/pflag" + // Third-party imports + "cosmossdk.io/core/transaction" "cosmossdk.io/log" "cosmossdk.io/schema/indexer" serverv2 "cosmossdk.io/server/v2" cometlog "cosmossdk.io/server/v2/cometbft/log" "cosmossdk.io/server/v2/cometbft/mempool" "cosmossdk.io/server/v2/store" "cosmossdk.io/store/v2/root" "cosmossdk.io/store/v2/snapshots" + // Project-specific imports + genutiltypes "github.com/cosmos/cosmos-sdk/x/genutil/types" )Note: This ensures better readability and maintains consistency with the Go community standards.
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
⛔ Files ignored due to path filters (3)
server/v2/cometbft/go.sumis excluded by!**/*.sumserver/v2/go.sumis excluded by!**/*.sumsimapp/v2/go.sumis excluded by!**/*.sum
📒 Files selected for processing (8)
- schema/indexer/start.go (2 hunks)
- server/v2/cometbft/config.go (3 hunks)
- server/v2/cometbft/go.mod (1 hunks)
- server/v2/cometbft/server.go (2 hunks)
- server/v2/go.mod (1 hunks)
- server/v2/types.go (2 hunks)
- simapp/v2/go.mod (1 hunks)
- tools/confix/data/v2-app.toml (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- server/v2/cometbft/go.mod
- server/v2/go.mod
- server/v2/types.go
- simapp/v2/go.mod
- tools/confix/data/v2-app.toml
🧰 Additional context used
📓 Path-based instructions (3)
schema/indexer/start.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.server/v2/cometbft/config.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.server/v2/cometbft/server.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
🔇 Additional comments (4)
server/v2/cometbft/config.go (2)
6-6: LGTM: Import statement added correctly.The new import for the
indexerpackage is placed correctly and follows the Uber Golang style guide recommendations for import grouping and ordering.
27-30: LGTM: Indexer field initialized correctly in DefaultAppTomlConfig.The
Indexerfield is properly initialized with default values. The use ofmakefor theTargetmap and the defaultChannelBufferSizeof 1024 are appropriate. The initialization follows the struct field order, maintaining consistency with theAppTomlConfigstruct definition.schema/indexer/start.go (2)
54-54: LGTM: Enhanced field annotations and documentation.The addition of
mapstructure,toml, andjsontags to theTargetfield improves its serialization capabilities. The added comment clearly describes the field's purpose, enhancing code readability and maintainability.
58-58: LGTM: Improved field type and annotations.The
ChannelBufferSizefield has been improved by:
- Changing the type from
*inttoint, which simplifies usage and removes the need for nil checks.- Adding
mapstructure,toml, andjsontags for better serialization support.- Including a descriptive comment that explains the field's purpose and default value.
These changes enhance code clarity and usability.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (2)
server/v2/cometbft/config.go (1)
27-30: LGTM with a minor suggestionThe initialization of the new
Indexerfield in theDefaultAppTomlConfigfunction is well-structured and follows the Uber Golang style guide. The use of explicit field names enhances readability.Consider replacing the magic number 1024 for
ChannelBufferSizewith a named constant or adding a comment to explain its significance. This would improve code maintainability and clarity.const defaultIndexerChannelBufferSize = 1024 // ... Indexer: indexer.IndexingConfig{ Target: make(map[string]indexer.Config), ChannelBufferSize: defaultIndexerChannelBufferSize, // Default buffer size for indexer channels },schema/indexer/start.go (1)
58-58: Improved field type and tags forChannelBufferSize.The changes to
ChannelBufferSizeare well-considered:
- Changing from
*inttointsimplifies usage.- Added tags improve configuration parsing and serialization.
- The comment clearly explains the field's purpose.
Consider adding a default value to the comment for completeness:
-comment:"Buffer size of the channels used for buffering data sent to indexer go routines."` +comment:"Buffer size of the channels used for buffering data sent to indexer go routines. Default is 1024."`
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
⛔ Files ignored due to path filters (3)
server/v2/cometbft/go.sumis excluded by!**/*.sumserver/v2/go.sumis excluded by!**/*.sumsimapp/v2/go.sumis excluded by!**/*.sum
📒 Files selected for processing (8)
- schema/indexer/start.go (2 hunks)
- server/v2/cometbft/config.go (3 hunks)
- server/v2/cometbft/go.mod (1 hunks)
- server/v2/cometbft/server.go (2 hunks)
- server/v2/go.mod (1 hunks)
- server/v2/types.go (2 hunks)
- simapp/v2/go.mod (1 hunks)
- tools/confix/data/v2-app.toml (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- server/v2/cometbft/go.mod
- server/v2/go.mod
- server/v2/types.go
- simapp/v2/go.mod
- tools/confix/data/v2-app.toml
🧰 Additional context used
📓 Path-based instructions (3)
schema/indexer/start.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.server/v2/cometbft/config.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.server/v2/cometbft/server.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
🔇 Additional comments (3)
server/v2/cometbft/config.go (2)
6-6: LGTM: Import statement correctly addedThe new import for the
indexerpackage is correctly placed and follows the Uber Golang style guide for import grouping. This import is necessary for the newIndexerfield in theAppTomlConfigstructure.
45-46: LGTM: AppTomlConfig struct updated correctlyThe addition of the
Indexerfield to theAppTomlConfigstruct is well-implemented:
- It follows the Uber Golang style guide for struct field declarations.
- The comment clearly explains the purpose of the field.
- The new field is correctly placed at the end of the struct, which is a good practice for maintaining backward compatibility.
schema/indexer/start.go (1)
54-54: Improved struct field tags and documentation.The addition of
mapstructureandtomltags, along with the descriptive comment, enhances theTargetfield's usability and documentation. These changes improve configuration parsing and provide clear information about the field's purpose.
|
Noticed while fixing conflicts in #22323 that this should have been backported. Will do once the above pr is merged. |
|
@Mergifyio backport release/v0.52.x |
✅ Backports have been created
|
Co-authored-by: Julien Robert <[email protected]> (cherry picked from commit e84c0eb) # Conflicts: # runtime/v2/app.go # runtime/v2/go.mod # schema/indexer/start.go # server/v2/cometbft/go.mod # server/v2/cometbft/go.sum # server/v2/cometbft/server.go # server/v2/config.go # server/v2/go.mod # server/v2/go.sum # server/v2/types.go # simapp/v2/go.mod # simapp/v2/go.sum
…) (#22324) Co-authored-by: cool-developer <[email protected]> Co-authored-by: Julien Robert <[email protected]>
Description
Closes: #22217
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!in the type prefix if API or client breaking changeCHANGELOG.mdReviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
Please see Pull Request Reviewer section in the contributing guide for more information on how to review a pull request.
I have...
Summary by CodeRabbit
New Features
Bug Fixes
Chores