fix(indexer): the issues during simapp v1 integration #22413
Conversation
cool-develope
requested review from
a team,
julienrbrt,
sontrinh16,
facundomedica,
aaronc and
testinginprod
as code owners
📝 Walkthrough📝 WalkthroughWalkthroughThe changes in this pull request involve modifications across several files related to schema codec handling, secondary indexing, and dependency management. Key updates include the introduction of new functions for schema type conversion, enhancements to existing codec functionalities, and the addition of PostgreSQL support in the application. The changes also streamline certain methods and improve flexibility in key management and indexing options, while maintaining overall logic and error handling consistency. Changes
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yml ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
|
@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 (10)
indexer/postgres/params.go (2)
11-14: LGTM! Consider enhancing the documentation.The interface is well-designed with a clear single responsibility.
Consider expanding the documentation to include an example:
type keyCollection interface { - // Keys returns the key fields for the collection. + // Keys returns the key fields for the collection. + // For example, for a composite key (a, b), Keys() would return []interface{}{a, b} Keys() []interface{} }
25-27: Add nil check and improve error handling.While the implementation is correct, consider these improvements for better robustness:
if kc, ok := key.(keyCollection); ok { + keys := kc.Keys() + if keys == nil { + return nil, nil, errors.New("keyCollection.Keys() returned nil") + } - return tm.bindParams(tm.typ.KeyFields, kc.Keys()) + return tm.bindParams(tm.typ.KeyFields, keys) }Also, consider making the slice error message more specific:
-return nil, nil, errors.New("expected key to be a slice") +return nil, nil, fmt.Errorf("expected key to be []interface{} for composite key with %d fields, got %T", n, key)collections/codec/indexing.go (1)
93-97: Consider enhancing the error messageWhile the error handling is correct, the error message could be more descriptive.
Consider this improvement:
- return t, fmt.Errorf("expected string, got %T", a) + return t, fmt.Errorf("expected string, got %T with value: %v", a, a)This would provide more context for debugging type mismatches.
collections/indexing.go (1)
136-138: LGTM! Consider adding a comment for clarity.The nil check for
keyDecoder.ToSchemaTypeis a good addition that provides flexibility in key decoding by allowing direct value returns when no schema transformation is needed.Consider adding a comment to explain this bypass behavior:
+ // If no schema type transformation is defined, return the decoded value directly if keyDecoder.ToSchemaType == nil { return x, nil }collections/keyset.go (1)
24-29: Enhance documentation for secondary index behavior.While the function is well-implemented, the documentation could be more detailed about:
- What it means for a KeySet to be a secondary index
- The implications and use cases
- Any performance considerations
Consider expanding the documentation like this:
-// WithKeySetSecondaryIndex changes the behavior of the KeySet to be a secondary index. +// WithKeySetSecondaryIndex changes the behavior of the KeySet to be a secondary index. +// When enabled, the KeySet acts as a secondary index, allowing efficient lookups +// based on alternative keys. This is useful when you need to maintain additional +// access patterns to your data without duplicating the primary data structure. +// Note that secondary indexes may have additional storage and performance overhead +// compared to primary indexes.collections/map.go (1)
37-39: Add documentation for the struct and fieldConsider adding documentation to explain:
- The purpose of the
mapOptionsstruct- The impact of the
isSecondaryIndexfield on Map behavior+// mapOptions configures the behavior of a Map collection. type mapOptions struct { + // isSecondaryIndex indicates whether this Map serves as a secondary index + // for another collection. When true, the Map is excluded from user-facing schemas. isSecondaryIndex bool }collections/pair.go (1)
36-39: Consider enhancing type safety and documentation.While the implementation is functional, there are several areas for improvement:
- The use of
interface{}loses type safety. Consider using a more type-safe approach or documenting whyinterface{}is necessary for the PostgreSQL indexer integration.- The method could benefit from documentation explaining:
- The handling of nil values
- The order of returned keys
- The relationship with PostgreSQL indexer integration
Add documentation and consider a more type-safe implementation:
+// Keys returns both keys of the pair as a slice of interface{}. +// The keys are returned in order (key1, key2) regardless of whether they are nil. +// This method is primarily used for PostgreSQL indexer integration. func (p Pair[K1, K2]) Keys() []interface{} { return []interface{}{p.K1(), p.K2()} }simapp/app_di.go (1)
10-18: Consider adding error handling for database initialization.While the imports are correct, the application might benefit from explicit error handling during database initialization, especially since PostgreSQL connections can fail.
Consider adding a connection health check in the
NewSimAppfunction and proper connection cleanup in case of failures. I can help implement this if needed.simapp/go.mod (2)
Line range hint
3-3: Invalid Go version specifiedThe Go version
1.23.1does not exist. The latest stable version is1.22.1.Apply this diff to fix the version:
-go 1.23.1 +go 1.22.1
53-53: Consider updating pgx to the latest versionThe current version
v5.7.1is outdated. Consider updating to the latest stable versionv5.5.4for potential bug fixes and improvements.Apply this diff to update the version:
-require github.com/jackc/pgx/v5 v5.7.1 +require github.com/jackc/pgx/v5 v5.5.4
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
⛔ Files ignored due to path filters (1)
simapp/go.sumis excluded by!**/*.sum
📒 Files selected for processing (11)
collections/codec/indexing.go(1 hunks)collections/indexes/reverse_pair.go(1 hunks)collections/indexing.go(1 hunks)collections/keyset.go(2 hunks)collections/map.go(2 hunks)collections/pair.go(1 hunks)indexer/postgres/params.go(2 hunks)simapp/app_di.go(1 hunks)simapp/go.mod(4 hunks)x/bank/keeper/view.go(0 hunks)x/bank/module.go(2 hunks)
💤 Files with no reviewable changes (1)
- x/bank/keeper/view.go
🧰 Additional context used
📓 Path-based instructions (9)
collections/codec/indexing.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
collections/indexes/reverse_pair.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
collections/indexing.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
collections/keyset.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
collections/map.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
collections/pair.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
indexer/postgres/params.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
simapp/app_di.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
x/bank/module.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
🔇 Additional comments (14)
indexer/postgres/params.go (1)
Line range hint 11-27: Verify keyCollection interface implementations.
Let's ensure all intended types properly implement the keyCollection interface.
✅ Verification successful
Based on the code inspection, I can now provide a final response:
The keyCollection interface is properly implemented
The Pair type correctly implements the keyCollection interface by providing the required Keys() method that returns []interface{}. The implementation in collections/pair.go is complete and returns both parts of the pair key as expected:
func (p Pair[K1, K2]) Keys() []interface{} {
return []interface{}{p.K1(), p.K2()}
}This implementation matches the interface requirements and is used correctly in the indexer's bindKeyParams function.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for types implementing Keys() []interface{}
ast-grep --pattern 'type $TYPE struct {
$$$
}
func ($VAR $TYPE) Keys() []interface{} {
$$$
}'
Length of output: 100
Script:
#!/bin/bash
# Let's try a broader search for Keys() method implementations
ast-grep --pattern 'func ($VAR $_) Keys() []interface{} {
$$$
}'
# Also search for any references to keyCollection interface
rg -l "keyCollection"
# Search for any Keys() method calls
rg "Keys\(\)"
Length of output: 32914
Script:
#!/bin/bash
# Let's search for the specific keyCollection interface implementation in collections/pair.go
cat collections/pair.go
# Also search for any other potential implementations
rg -A 5 "type.*struct.*{" collections/
Length of output: 8848
collections/codec/indexing.go (2)
89-89: LGTM: JSON marshaling change is correct
The conversion of JSON bytes to string is safe and consistent with the new StringKind schema.
84-86: LGTM: Schema kind change looks good
The change from JSONKind to StringKind for default encoding provides better universal compatibility.
Let's verify the impact on existing types:
✅ Verification successful
Change from JSONKind to StringKind is safe to proceed
Based on the verification results:
- FallbackSchemaCodec is only used internally in KeySchemaCodec and ValueSchemaCodec functions
- The change to StringKind is isolated to the fallback case when no explicit schema codec is provided
- No direct dependencies on JSONKind were found in the codebase
- Existing tests in collections/naming_test.go verify the schema codec functionality but don't depend on the specific kind
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for types using the default codec
rg -l "FallbackSchemaCodec" --type go
Length of output: 67
Script:
#!/bin/bash
# Search for usages of FallbackSchemaCodec to understand its impact
rg "FallbackSchemaCodec" -A 5 -B 5 --type go
Length of output: 1909
Script:
#!/bin/bash
# Search for any tests that might be affected by this change
rg "FallbackSchemaCodec" --type go "*_test.go"
Length of output: 96
Script:
#!/bin/bash
# Look for any custom types being used with this codec
ast-grep --pattern 'FallbackSchemaCodec[T]'
Length of output: 45
Script:
#!/bin/bash
# Look for test files in the collections directory
fd "_test.go" collections/
# Then search for schema codec related tests
rg "SchemaCodec|schema.StringKind|schema.JSONKind" collections/*_test.go
Length of output: 1621
Script:
#!/bin/bash
# Look for any direct usage of schema kinds
rg "schema\.(String|JSON)Kind" collections/
Length of output: 129
collections/keyset.go (2)
31-34: LGTM!
The struct modification is clean and follows Go conventions. The new field is well-named and properly aligned with the existing structure.
57-57: LGTM!
The secondary index option is correctly propagated to the underlying Map implementation.
collections/indexes/reverse_pair.go (2)
58-58: LGTM: Secondary index option added correctly.
The addition of collections.WithKeySetSecondaryIndex() aligns with the PR's goal of improving indexing capabilities while maintaining compatibility with unchecked values.
63-63: LGTM: Secondary index option consistently applied.
The addition of collections.WithKeySetSecondaryIndex() maintains consistency with the unchecked value case and completes the secondary indexing support.
Let's verify that this pattern is consistently applied across other similar index implementations:
✅ Verification successful
Secondary index option correctly and consistently implemented
The verification confirms that WithKeySetSecondaryIndex() is:
- Properly defined in
collections/keyset.go - Consistently used in both branches of
reverse_pair.go(checked and unchecked value cases) - Applied appropriately for the reverse pair index implementation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other NewKeySet calls in index implementations
ast-grep --pattern 'collections.NewKeySet($$$)'
# Search for WithKeySetSecondaryIndex usage
rg 'WithKeySetSecondaryIndex'
Length of output: 687
x/bank/module.go (2)
12-15: LGTM: Import statements are properly organized.
The new imports for collections and schema packages are necessary for implementing the ModuleCodec functionality.
181-185: 🛠️ Refactor suggestion
Consider improving error handling and documentation.
While the implementation is functional, consider these improvements:
- Add error handling for the type assertion to prevent potential panics:
func (am AppModule) ModuleCodec() (schema.ModuleCodec, error) {
+ baseKeeper, ok := am.keeper.(keeper.BaseKeeper)
+ if !ok {
+ return nil, fmt.Errorf("keeper does not implement keeper.BaseKeeper")
+ }
- return am.keeper.(keeper.BaseKeeper).Schema.ModuleCodec(collections.IndexingOptions{})
+ return baseKeeper.Schema.ModuleCodec(collections.IndexingOptions{})
}- Enhance the documentation to explain:
- The purpose of IndexingOptions
- The expected return values
- Any potential error conditions
✅ Verification successful
Let me gather more information about the keeper implementation to verify the type assertion safety.
Let me gather more information about the keeper initialization and usage to verify the type assertion safety.
Based on the gathered information, I can now provide a final response about the type assertion safety in the ModuleCodec implementation.
Type assertion is safe, but documentation can be improved
The type assertion am.keeper.(keeper.BaseKeeper) is safe because:
- The bank module's
NewAppModulefunction explicitly takes akeeper.Keeperinterface which is implemented byBaseKeeper - The keeper is properly initialized and passed through the module's constructor
- The
ModuleCodecimplementation is only used in the bank module where we have full control over the keeper type
However, to improve code clarity and maintainability:
- Consider adding a comment explaining why the type assertion is safe
- Document the expected return values and any potential error conditions
- Add more details about the purpose of IndexingOptions in the documentation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that keeper always implements BaseKeeper to ensure type assertion safety
# Search for keeper initialization to verify BaseKeeper implementation
rg -l "keeper\." | xargs ast-grep --pattern 'NewKeeper($$$)'
Length of output: 182
Script:
#!/bin/bash
# Check the keeper implementation and BaseKeeper type
ast-grep --pattern 'type BaseKeeper struct {
$$$
}'
# Check NewKeeper implementation
ast-grep --pattern 'func NewKeeper($$$) (BaseKeeper, error) {
$$$
}'
# Search for other keeper types or interfaces
rg "type.*Keeper" -A 2
Length of output: 102834
Script:
#!/bin/bash
# Check the AppModule implementation and keeper initialization
ast-grep --pattern 'func NewAppModule($$$) AppModule {
$$$
}'
# Check where the keeper is used in ModuleCodec
rg -A 3 "ModuleCodec\(\)"
Length of output: 11990
collections/map.go (2)
30-35: LGTM: Well-implemented options pattern
The function follows Go idioms for options pattern and has clear, descriptive naming.
49-63: LGTM: Verify backward compatibility
The changes correctly implement the options pattern while maintaining backward compatibility. The implementation follows Go best practices.
Let's verify that existing Map usages aren't affected:
✅ Verification successful
Backward compatibility verified - no issues found
The search results show that all existing NewMap calls in the codebase are compatible with the new implementation. The only exception is in collections/keyset.go which already uses the new options pattern, as expected since it's part of the same package.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for NewMap calls without options to ensure backward compatibility
rg "NewMap\([^)]*\)" --type go
Length of output: 10108
collections/pair.go (1)
36-39: Verify PostgreSQL indexer integration requirements.
Please confirm that this implementation satisfies all PostgreSQL indexer requirements, particularly:
- The order of keys in the returned slice matches the expected column order in PostgreSQL.
- The handling of nil values is compatible with the indexer's null value handling.
✅ Verification successful
Let me try another approach to verify the PostgreSQL indexer integration requirements.
Based on the search results, I can now provide a definitive response about the PostgreSQL indexer integration.
Implementation aligns with indexer requirements
The Pair type's Keys() method implementation is correct and compatible with PostgreSQL indexer requirements because:
- The order of keys is preserved as defined in the struct (K1, K2) which matches the column order in PostgreSQL tables as shown by the indexer implementation in
indexer/postgres/params.go. - The
interface{}return type is compatible with the PostgreSQL indexer's parameter binding system, which handles null values appropriately through thebindParammethod inindexer/postgres/params.go.
The implementation is minimal but complete for the indexer's needs, as the PostgreSQL indexer handles type conversion and null value cases internally through its object indexer implementation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for PostgreSQL indexer usage of the Keys() method
rg -l "Keys\(\)" --type go | xargs rg "postgres"
# Look for any special handling of null values in the indexer
rg "interface\{\}" --type go | xargs rg "null.*postgres"
Length of output: 523754
Script:
#!/bin/bash
# Search for PostgreSQL indexer implementation and usage
rg -A 5 "type.*Indexer.*struct" --type go
rg -A 5 "func.*Indexer" --type go
rg -A 5 "indexer.*postgres" --type go
Length of output: 28000
simapp/app_di.go (1)
10-11: LGTM: PostgreSQL driver import is correctly added.
The import of the PostgreSQL driver with the blank identifier is the correct pattern for SQL driver registration.
simapp/go.mod (1)
12-12: PostgreSQL integration changes look good
The addition of PostgreSQL-related dependencies and replace directive aligns well with the PR objective of integrating simapp/v1 with the PostgreSQL indexer.
Also applies to: 153-155, 249-249
| clienthelpers "cosmossdk.io/client/v2/helpers" | ||
| "cosmossdk.io/core/address" | ||
| "cosmossdk.io/core/appmodule" | ||
| "cosmossdk.io/core/registry" | ||
| corestore "cosmossdk.io/core/store" | ||
| "cosmossdk.io/depinject" | ||
| _ "cosmossdk.io/indexer/postgres" // register the postgres indexer |
There was a problem hiding this comment.
💡 Codebase verification
Missing required PostgreSQL indexer dependency in go.mod
The PostgreSQL indexer is imported and used in the code, but its dependency is not properly declared in go.mod. This could lead to build issues or version inconsistencies.
- Add
cosmossdk.io/indexer/postgresto the required dependencies in go.mod
🔗 Analysis chain
LGTM: PostgreSQL indexer import is correctly added.
The import of the Postgres indexer module with the blank identifier follows the correct pattern for registering the indexer.
Let's verify that the required dependencies are properly declared in go.mod:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that the required PostgreSQL dependencies are declared in go.mod
# Test: Check for pgx dependency
echo "Checking for pgx dependency..."
rg "github.com/jackc/pgx/v5" go.mod
# Test: Check for postgres indexer dependency
echo "Checking for postgres indexer dependency..."
rg "cosmossdk.io/indexer/postgres" go.mod
Length of output: 245
Script:
#!/bin/bash
# Let's try a different approach to check the dependencies
# Check the entire go.mod content
echo "=== Checking go.mod content ==="
cat go.mod
# Check if there are any indirect dependencies related to postgres
echo -e "\n=== Checking for any postgres-related dependencies ==="
rg -i "postgres" go.mod
# Check if the indexer package is imported anywhere else
echo -e "\n=== Checking for indexer usage across the codebase ==="
rg "cosmossdk.io/indexer" --type go
Length of output: 10219
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (8)
runtime/builder.go (1)
102-109: Consider optimizing the override lookup performance.The current implementation iterates through all override configurations for each module. For better performance, consider pre-processing the overrides into a map.
Here's a suggested optimization:
func (a *AppBuilder) registerIndexer() error { if indexerOpts := a.appOptions.Get("indexer"); indexerOpts != nil { + // Pre-process overrides into a map for O(1) lookup + overrideMap := make(map[string]string) + for _, cfg := range a.app.config.OverrideStoreKeys { + overrideMap[cfg.ModuleName] = cfg.KvStoreKey + } + moduleSet := map[string]any{} for modName, mod := range a.app.ModuleManager.Modules { - storeKey := modName - for _, cfg := range a.app.config.OverrideStoreKeys { - if cfg.ModuleName == modName { - storeKey = cfg.KvStoreKey - break - } - } + storeKey, exists := overrideMap[modName] + if !exists { + storeKey = modName + } moduleSet[storeKey] = mod }collections/keyset.go (2)
24-29: Enhance documentation for secondary index behavior.While the function is well-implemented, the documentation could be more detailed about:
- What it means for a KeySet to be a secondary index
- The implications of enabling this option
- When developers should use this option
Consider expanding the documentation like this:
-// WithKeySetSecondaryIndex changes the behavior of the KeySet to be a secondary index. +// WithKeySetSecondaryIndex modifies the KeySet to behave as a secondary index, +// enabling efficient lookups through alternative keys. This is useful when you need +// to maintain additional access patterns to your data. When enabled, the KeySet +// participates in referential integrity checks and cascading operations.
31-34: Add documentation for struct fields.The struct fields lack documentation explaining their purpose and impact.
Consider adding field documentation:
type keySetOptions struct { - uncheckedValue bool - isSecondaryIndex bool + // uncheckedValue when true, allows values different from []byte{} during migrations + uncheckedValue bool + // isSecondaryIndex when true, indicates this KeySet maintains secondary indexes + isSecondaryIndex bool }collections/map.go (3)
30-35: Add godoc documentation for thewithMapSecondaryIndexfunction.While the function implementation is correct, it would benefit from comprehensive godoc documentation explaining:
- When to use this option
- The implications of setting a map as a secondary index
- Example usage
Add documentation like:
// withMapSecondaryIndex marks a Map as a secondary index. Secondary indexes are used for... // Example usage: // // myMap := NewMap[string, int]( // builder, prefix, "mymap", // codec.StringKey, codec.IntValue, // withMapSecondaryIndex(true), // )
37-39: Add documentation for themapOptionsstruct.The struct and its field would benefit from documentation explaining their purpose and usage.
Add documentation like:
// mapOptions contains configuration options for Map instances. type mapOptions struct { // isSecondaryIndex indicates whether this Map serves as a secondary index. // When true, the Map will be excluded from user-facing schema generation. isSecondaryIndex bool }
49-63: Update NewMap documentation to include options parameter.The implementation looks good and follows Go best practices. However, the function's documentation should be updated to explain the new options parameter.
Update the documentation to include:
// NewMap returns a Map given a StoreKey, a Prefix, human-readable name and the relative value and key encoders. // Name and prefix must be unique within the schema and name must match the format specified by NameRegex, or // else this method will panic. // // Optional configuration can be provided through functional options: // - withMapSecondaryIndex: marks the Map as a secondary index // // Example: // // myMap := NewMap[string, int]( // builder, prefix, "mymap", // codec.StringKey, codec.IntValue, // withMapSecondaryIndex(true), // )collections/triple.go (1)
48-51: Consider enhancing type safety of the Keys() method.The implementation looks good and aligns with the PR objectives for PostgreSQL indexer integration. However, consider a future enhancement to maintain type safety:
// Alternative type-safe approach func (t Triple[K1, K2, K3]) TypedKeys() (K1, K2, K3) { return t.K1(), t.K2(), t.K3() }collections/quad.go (1)
57-60: Enhance method documentationThe implementation looks good, but the documentation could be more descriptive. Consider expanding it to explain that the method returns all four keys in order, and that nil values are handled by returning their zero values.
Consider updating the comment to:
-// Keys returns key1, key2, key3 and key4 as a slice. +// Keys returns all four keys as a slice of interfaces, in order (key1, key2, key3, key4). +// If any key is nil, its zero value is returned in the slice.
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
⛔ Files ignored due to path filters (2)
simapp/go.sumis excluded by!**/*.sumtests/go.sumis excluded by!**/*.sum
📒 Files selected for processing (8)
collections/codec/indexing.go(2 hunks)collections/keyset.go(2 hunks)collections/map.go(2 hunks)collections/quad.go(1 hunks)collections/triple.go(1 hunks)runtime/builder.go(1 hunks)simapp/go.mod(4 hunks)tests/go.mod(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- collections/codec/indexing.go
- simapp/go.mod
🧰 Additional context used
📓 Path-based instructions (6)
collections/keyset.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
collections/map.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
collections/quad.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
collections/triple.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
runtime/builder.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
tests/go.mod (1)
Pattern tests/**/*: "Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull request"
🔇 Additional comments (5)
runtime/builder.go (1)
102-109: LGTM! The store key override logic is well-implemented.
The implementation correctly handles store key overrides while maintaining backward compatibility.
collections/keyset.go (1)
57-57: LGTM: Secondary index option properly integrated.
The secondary index option is correctly propagated to the underlying Map implementation.
collections/triple.go (1)
48-51: Verify consistent Keys() method implementation across collection types.
The implementation aligns with the AI summary mentioning similar Keys() methods added to Pair and Quad types.
✅ Verification successful
Keys() method implementation is consistent across collection types
The verification confirms that the Keys() method is consistently implemented across all collection types:
- Pair:
func (p Pair[K1, K2]) Keys() []interface{} { return []interface{}{p.K1(), p.K2()} } - Triple:
func (t Triple[K1, K2, K3]) Keys() []interface{} { return []interface{}{t.K1(), t.K2(), t.K3()} } - Quad:
func (t Quad[K1, K2, K3, K4]) Keys() []interface{} { return []interface{}{t.K1(), t.K2(), t.K3(), t.K4()} }
All implementations follow the same pattern:
- Return type is
[]interface{} - Return all keys in order using their respective accessor methods
- Consistent naming and documentation style
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify consistent Keys() method implementation across collection types
# Expected: Similar Keys() method implementations returning []interface{} in Pair and Quad
# Check Pair implementation
echo "Checking Pair implementation:"
rg -A 4 "func \(.*Pair.*\) Keys\(\) \[\]interface{}" --type go
echo "Checking Quad implementation:"
rg -A 4 "func \(.*Quad.*\) Keys\(\) \[\]interface{}" --type go
Length of output: 599
Script:
#!/bin/bash
# Description: Verify consistent Keys() method implementation across collection types
# Let's fix the regex and search for Keys() method
echo "Checking Pair implementation:"
rg -A 4 "Keys\(\) \[]interface{}" collections/pair.go
echo "Checking Quad implementation:"
rg -A 4 "Keys\(\) \[]interface{}" collections/quad.go
Length of output: 505
Script:
#!/bin/bash
# Description: Verify consistent Keys() method implementation across collection types
# Let's use ast-grep to find the Keys() method implementations
echo "Checking Pair implementation:"
ast-grep --pattern 'func ($_) Keys() []interface{} { $_$ }' collections/pair.go
echo "Checking Quad implementation:"
ast-grep --pattern 'func ($_) Keys() []interface{} { $_$ }' collections/quad.go
Length of output: 301
Script:
#!/bin/bash
# Let's try a simpler approach - just look at the content of these files
echo "Content of collections/pair.go:"
cat collections/pair.go
echo -e "\nContent of collections/quad.go:"
cat collections/quad.go
Length of output: 20984
collections/quad.go (1)
57-60: Verify consistency with similar implementations
Let's verify that the implementation aligns with similar Keys methods in related types.
✅ Verification successful
Implementation aligns perfectly with existing patterns
The Keys() method in Quad follows the exact same pattern as its counterparts in Pair and Triple:
- Returns
[]interface{} - Returns keys in order using respective getter methods
- Consistent implementation across all collection types
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for similar Keys() method implementations in related types
# Expected: Find similar implementations in Pair and other collection types
# Search for Keys method implementations
echo "Searching for Keys method implementations..."
ast-grep --pattern 'func ($_ $_) Keys() []interface{} {
$$$
}'
# Search for related collection types
echo "Searching for related collection types..."
rg "type (Pair|Triple|Quad)\[" -A 5
Length of output: 1412
tests/go.mod (1)
68-68: LGTM! Dependencies are properly configured for PostgreSQL integration.
The addition of PostgreSQL-related dependencies and their configuration appears correct. Note that cosmossdk.io/indexer/postgres is at version v0.1.0, indicating it's in early development. The local development setup using the replace directive is consistent with the module's development practices.
Let's verify the version compatibility:
Also applies to: 155-158, 245-245
✅ Verification successful
PostgreSQL dependencies are properly configured and compatible
Looking at both indexer/postgres/go.mod and tests/go.mod, the setup is correct:
- The indexer module is intentionally minimal, using only standard library's
database/sqland schema dependency - The test module correctly includes the required PostgreSQL dependencies (
jackc/pgx/v5ecosystem) as indirect dependencies - The replace directive points to the local development path, which is the expected configuration
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that the PostgreSQL driver version is compatible with the indexer
# Test: Check if there are any version constraints in the indexer module
rg -A 5 "github.com/jackc/pgx/v5" ../indexer/postgres/go.mod
Length of output: 127
Script:
#!/bin/bash
# Let's first check if the file exists in the repository
fd "go.mod" --type f
# Then let's check for any PostgreSQL-related dependencies in all go.mod files
rg "github.com/jackc/pgx" --type go-mod
# Also check for the cosmos indexer postgres module
rg "cosmossdk.io/indexer/postgres" --type go-mod
Length of output: 1206
Script:
#!/bin/bash
# Check the content of the indexer/postgres/go.mod file
cat indexer/postgres/go.mod
# Check the content of tests/go.mod file
cat tests/go.mod
Length of output: 14031
collections/codec/indexing.go
Outdated
| @@ -81,20 +81,20 @@ func FallbackSchemaCodec[T any]() SchemaCodec[T] { | |||
| FromSchemaType: nil, | |||
| } | |||
| } else { | |||
| // we default to encoding everything to JSON | |||
| // we default to encoding everything to String | |||
| @@ -21,7 +21,17 @@ func WithKeySetUncheckedValue() func(opt *keySetOptions) { | |||
| } | |||
| } | |||
|
|
|||
| type keySetOptions struct{ uncheckedValue bool } | |||
| // WithKeySetSecondaryIndex changes the behavior of the KeySet to be a secondary index. | |||
| func WithKeySetSecondaryIndex() func(opt *keySetOptions) { | |||
There was a problem hiding this comment.
yes, used in indexes/reverse_pair.go
indexer/postgres/params.go
Outdated
| @@ -17,6 +22,9 @@ func (tm *objectIndexer) bindKeyParams(key interface{}) ([]interface{}, []string | |||
| } else if n == 1 { | |||
| return tm.bindParams(tm.typ.KeyFields, []interface{}{key}) | |||
| } else { | |||
| if kc, ok := key.(keyCollection); ok { | |||
| return tm.bindParams(tm.typ.KeyFields, kc.Keys()) | |||
| } | |||
There was a problem hiding this comment.
We shouldn't need to change anything in indexer here. This breaks the schema specification. Instead we need to make sure that pair, triple, etc in collections decode correctly to what schema expects. This would be part of the codec implementation
There was a problem hiding this comment.
The SchemaCodec method of PairKeyCodec, etc is what needs to be fixed. I didn't complete the encoding/decoding there properly. Apologies that this wasn't clear.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
collections/indexes/multi.go (1)
67-73: LGTM! Consider documenting the secondary index behavior.The addition of
WithKeySetSecondaryIndex()in the default initialization path is correct and consistent with the changes above.Consider adding a comment explaining the significance of
WithKeySetSecondaryIndex()and its impact on theMultiindex behavior, especially in the context of PostgreSQL integration.
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (6)
collections/codec/indexing.go(2 hunks)collections/indexes/multi.go(1 hunks)collections/indexes/reverse_pair.go(1 hunks)collections/pair.go(1 hunks)collections/quad.go(1 hunks)collections/triple.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- collections/codec/indexing.go
- collections/indexes/reverse_pair.go
- collections/pair.go
- collections/quad.go
🧰 Additional context used
📓 Path-based instructions (2)
collections/indexes/multi.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
collections/triple.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
🔇 Additional comments (2)
collections/indexes/multi.go (1)
54-61: LGTM! Verify secondary index behavior with unchecked values.
The addition of WithKeySetSecondaryIndex() alongside WithKeySetUncheckedValue() looks correct. The order of options is logical.
Let's verify the usage of these options together:
✅ Verification successful
Secondary index option is correctly implemented and consistently used
The verification shows that WithKeySetSecondaryIndex() is:
- Properly implemented in
collections/keyset.goto mark a KeySet as a secondary index - Consistently used in similar index implementations (
ReversePairandMulti) - Appropriately combined with
WithKeySetUncheckedValue()in index implementations where needed
The implementation pattern in the reviewed code follows the established pattern in the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other instances where both options are used together
# to ensure consistent behavior across the codebase
rg -l 'WithKeySetUncheckedValue.*WithKeySetSecondaryIndex|WithKeySetSecondaryIndex.*WithKeySetUncheckedValue'
Length of output: 111
Script:
#!/bin/bash
# Let's first check the implementation of WithKeySetSecondaryIndex
ast-grep --pattern 'func WithKeySetSecondaryIndex'
# Then check for any tests or usage patterns of these options
rg -A 5 'WithKeySetSecondaryIndex\(\)'
# Also check for any related documentation or comments
rg -B 2 'WithKeySetSecondaryIndex'
Length of output: 2801
collections/triple.go (1)
314-316: LGTM: Clean and efficient implementation of ToSchemaType
The implementation correctly uses the existing K1(), K2(), K3() methods which already handle nil cases, making it robust and maintainable.
| FromSchemaType: func(a any) (Triple[K1, K2, K3], error) { | ||
| aSlice, ok := a.([]interface{}) | ||
| if !ok || len(aSlice) != 3 { | ||
| return Triple[K1, K2, K3]{}, fmt.Errorf("expected slice of length 3, got %T", a) | ||
| } | ||
| return Join3(aSlice[0].(K1), aSlice[1].(K2), aSlice[2].(K3)), nil | ||
| }, |
There was a problem hiding this comment.
Add type assertion safety checks to prevent panics
The current implementation has unsafe type assertions that could cause runtime panics. Consider adding proper type validation and error handling.
Here's a safer implementation:
FromSchemaType: func(a any) (Triple[K1, K2, K3], error) {
aSlice, ok := a.([]interface{})
if !ok || len(aSlice) != 3 {
return Triple[K1, K2, K3]{}, fmt.Errorf("expected slice of length 3, got %T", a)
}
+ k1, ok := aSlice[0].(K1)
+ if !ok {
+ return Triple[K1, K2, K3]{}, fmt.Errorf("invalid type for k1: expected %T, got %T", *new(K1), aSlice[0])
+ }
+ k2, ok := aSlice[1].(K2)
+ if !ok {
+ return Triple[K1, K2, K3]{}, fmt.Errorf("invalid type for k2: expected %T, got %T", *new(K2), aSlice[1])
+ }
+ k3, ok := aSlice[2].(K3)
+ if !ok {
+ return Triple[K1, K2, K3]{}, fmt.Errorf("invalid type for k3: expected %T, got %T", *new(K3), aSlice[2])
+ }
- return Join3(aSlice[0].(K1), aSlice[1].(K2), aSlice[2].(K3)), nil
+ return Join3(k1, k2, k3), nil
},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| FromSchemaType: func(a any) (Triple[K1, K2, K3], error) { | |
| aSlice, ok := a.([]interface{}) | |
| if !ok || len(aSlice) != 3 { | |
| return Triple[K1, K2, K3]{}, fmt.Errorf("expected slice of length 3, got %T", a) | |
| } | |
| return Join3(aSlice[0].(K1), aSlice[1].(K2), aSlice[2].(K3)), nil | |
| }, | |
| FromSchemaType: func(a any) (Triple[K1, K2, K3], error) { | |
| aSlice, ok := a.([]interface{}) | |
| if !ok || len(aSlice) != 3 { | |
| return Triple[K1, K2, K3]{}, fmt.Errorf("expected slice of length 3, got %T", a) | |
| } | |
| k1, ok := aSlice[0].(K1) | |
| if !ok { | |
| return Triple[K1, K2, K3]{}, fmt.Errorf("invalid type for k1: expected %T, got %T", *new(K1), aSlice[0]) | |
| } | |
| k2, ok := aSlice[1].(K2) | |
| if !ok { | |
| return Triple[K1, K2, K3]{}, fmt.Errorf("invalid type for k2: expected %T, got %T", *new(K2), aSlice[1]) | |
| } | |
| k3, ok := aSlice[2].(K3) | |
| if !ok { | |
| return Triple[K1, K2, K3]{}, fmt.Errorf("invalid type for k3: expected %T, got %T", *new(K3), aSlice[2]) | |
| } | |
| return Join3(k1, k2, k3), nil | |
| }, |
julienrbrt
added
the
backport/v0.52.x
|
adding backport label for simapp and module changes |
(cherry picked from commit 2290c5e) # Conflicts: # collections/codec/indexing.go # collections/indexes/multi.go # collections/indexes/reverse_pair.go # collections/indexing.go # collections/keyset.go # collections/map.go # collections/pair.go # collections/quad.go # collections/triple.go # simapp/go.mod # tests/go.mod
… (#22461) Co-authored-by: cool-developer <[email protected]> Co-authored-by: Julien Robert <[email protected]>
Description
simapp/v1integration with the postgres indexerbankmodule integrationindexer/postgresto support thePairschemaAuthor 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
Pair,Quad, andTripleinstances to and from schema representations, improving serialization and deserialization.NewMultimethod to include secondary indexing options for improved reference management.Bug Fixes
Documentation