feat: indexer base types #20629
feat: indexer base types #20629
Conversation
|
Warning Rate limit exceeded@aaronc has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 31 minutes and 2 seconds before requesting another review. How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughThis update introduces a new Changes
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
| @@ -0,0 +1,6 @@ | |||
| module cosmossdk.io/indexer/base | |||
There was a problem hiding this comment.
Should we maybe just call this cosmossdk.io/indexer? I'm not sure what package would be simply cosmossdk.io/indexer if it's not this one, although maybe good to leave as base to communicate the intent of a base package clearly.
There was a problem hiding this comment.
this works, no need to change
|
@tac0turtle @fdymylja I still have to do a bit more work on tests here, but wondering if you two could take a look now so we can start reviewing the API and design. The tests are easy but a bit tedious so not worth it unless otherwise things look good. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| // DurationKind is a duration type and values of this type must be of the go type time.Duration. | ||
| DurationKind | ||
|
|
||
| // Float32Kind is a float32 type and values of this type must be of the go type float32. |
There was a problem hiding this comment.
wondering if we want to support this day0 considering they're not used in the state machine
There was a problem hiding this comment.
I guess I'm thinking better to be thorough, but I don't have a strong opinion
There was a problem hiding this comment.
in the case of cosmwasm i belive they have this type and would want to add it if they were to write a decoder
indexer/base/kind.go
Outdated
| return fmt.Errorf("expected uint64, got %T", value) | ||
| } | ||
| case IntegerKind: | ||
| _, ok := value.(string) |
There was a problem hiding this comment.
minor perf nit to exit earlier in case one of the conditions is satisfied
indexer/base/entity.go
Outdated
| // Iterate iterates over the columns and values in the entity update. The function should return | ||
| // true to continue iteration or false to stop iteration. Each column value should conform | ||
| // to the requirements of that column's type in the schema. | ||
| Iterate(func(col string, value any) bool) |
This comment has been minimized.
This comment has been minimized.
|
I have moved all executable code into #20665 and marked this R4R. So this PR just contains the core indexer types. If we can get alignment on these, then other pieces related to postgres, collections and baseapp integration can move along more independently. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files ignored due to path filters (1)
indexer/base/go.sumis excluded by!**/*.sum
Files selected for processing (9)
- indexer/base/README.md (1 hunks)
- indexer/base/enum.go (1 hunks)
- indexer/base/field.go (1 hunks)
- indexer/base/go.mod (1 hunks)
- indexer/base/kind.go (1 hunks)
- indexer/base/listener.go (1 hunks)
- indexer/base/module_schema.go (1 hunks)
- indexer/base/object_descriptor.go (1 hunks)
- indexer/base/object_update.go (1 hunks)
Files skipped from review due to trivial changes (2)
- indexer/base/enum.go
- indexer/base/go.mod
Additional context used
Path-based instructions (7)
indexer/base/module_schema.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.indexer/base/field.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.indexer/base/README.md (1)
Pattern
**/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"indexer/base/object_descriptor.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.indexer/base/object_update.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.indexer/base/kind.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.indexer/base/listener.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Markdownlint
indexer/base/README.md
5-5: null (MD047, single-trailing-newline)
Files should end with a single newline character
Additional comments not posted (6)
indexer/base/field.go (1)
14-19: Great additions to theFieldstruct. Consider adding comments forAddressPrefixandEnumDefinitionto explain their specific roles and when they should be used.// AddressPrefix is the address prefix of the field's kind, currently only used for Bech32AddressKind. + // This prefix helps in validating and formatting the addresses according to specific network standards. // EnumDefinition is the definition of the enum type and is only valid when Kind is EnumKind. + // This definition includes the name of the enum and its possible values, facilitating validation and indexing of enum fields.Likely invalid or redundant comment.
indexer/base/object_update.go (2)
3-28: TheObjectUpdatestruct is well-documented and the fields are clearly defined, aligning with the PR's objective to enhance indexing capabilities.
31-41: TheValueUpdatesinterface is well-defined, with a clear method for iterating over updates. It supports flexible and efficient handling of object value updates.indexer/base/kind.go (1)
3-82: TheKindtype and its constants are clearly defined, covering a wide range of data types appropriate for the indexer's needs. The documentation for each constant is thorough, ensuring clarity in their usage.indexer/base/listener.go (2)
7-59: TheListenerstruct is comprehensive, with well-defined methods for handling various blockchain events. The documentation provides clear guidance on the purpose and usage of each method.
62-127: The additional data structures (InitializationData,BlockHeaderData,TxData,EventData,ToBytes,ToJSON) are well-designed to support the functionality of theListenerstruct. The use of function types for lazy data processing is particularly noteworthy.
indexer/base/module_schema.go
Outdated
| // ModuleSchema represents the logical schema of a module for purposes of indexing and querying. | ||
| type ModuleSchema struct { | ||
|
|
||
| // Objects is a list of objects that are part of the schema for the module. | ||
| Objects []ObjectDescriptor | ||
| } |
There was a problem hiding this comment.
Consider adding a more detailed comment for the Objects field to explain its usage in the context of indexing.
// Objects is a list of objects that are part of the schema for the module.
+ // Each object defines the structure and fields that will be indexed and queried.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.
| // ModuleSchema represents the logical schema of a module for purposes of indexing and querying. | |
| type ModuleSchema struct { | |
| // Objects is a list of objects that are part of the schema for the module. | |
| Objects []ObjectDescriptor | |
| } | |
| // ModuleSchema represents the logical schema of a module for purposes of indexing and querying. | |
| type ModuleSchema struct { | |
| // Objects is a list of objects that are part of the schema for the module. | |
| // Each object defines the structure and fields that will be indexed and queried. | |
| Objects []ObjectDescriptor | |
| } |
indexer/base/object_descriptor.go
Outdated
| // RetainDeletions is a flag that indicates whether the indexer should retain | ||
| // deleted rows in the database and flag them as deleted rather than actually | ||
| // deleting the row. For many types of data in state, the data is deleted even | ||
| // though it is still valid in order to save space. Indexers will want to have | ||
| // the option of retaining such data and distinguishing from other "true" deletions. | ||
| RetainDeletions bool |
There was a problem hiding this comment.
The addition of the RetainDeletions flag is a thoughtful inclusion for handling deletions. Consider expanding the comment to explain potential use cases where retaining deletions might be beneficial, such as auditing or historical data analysis.
// RetainDeletions is a flag that indicates whether the indexer should retain
+ // deleted rows for auditing or historical analysis purposes, marking them as deleted instead of removing them entirely.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.
| // RetainDeletions is a flag that indicates whether the indexer should retain | |
| // deleted rows in the database and flag them as deleted rather than actually | |
| // deleting the row. For many types of data in state, the data is deleted even | |
| // though it is still valid in order to save space. Indexers will want to have | |
| // the option of retaining such data and distinguishing from other "true" deletions. | |
| RetainDeletions bool | |
| // RetainDeletions is a flag that indicates whether the indexer should retain | |
| // deleted rows in the database and flag them as deleted rather than actually | |
| // deleting the row. For many types of data in state, the data is deleted even | |
| // though it is still valid in order to save space. Indexers will want to have | |
| // the option of retaining such data and distinguishing from other "true" deletions. | |
| // deleted rows for auditing or historical analysis purposes, marking them as deleted instead of removing them entirely. | |
| RetainDeletions bool |
There was a problem hiding this comment.
Actionable comments posted: 2
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (6)
- indexer/base/CHANGELOG.md (1 hunks)
- indexer/base/field.go (1 hunks)
- indexer/base/kind.go (1 hunks)
- indexer/base/module_schema.go (1 hunks)
- indexer/base/object_type.go (1 hunks)
- indexer/base/object_update.go (1 hunks)
Files skipped from review due to trivial changes (1)
- indexer/base/CHANGELOG.md
Files skipped from review as they are similar to previous changes (3)
- indexer/base/field.go
- indexer/base/kind.go
- indexer/base/object_update.go
Additional context used
Path-based instructions (2)
indexer/base/module_schema.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.indexer/base/object_type.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
indexer/base/module_schema.go
Outdated
| // ModuleSchema represents the logical schema of a module for purposes of indexing and querying. | ||
| type ModuleSchema struct { | ||
|
|
||
| // ObjectTypes describe the types of objects that are part of the module's schema. | ||
| ObjectTypes []ObjectType | ||
| } |
There was a problem hiding this comment.
Refine the comment for ObjectTypes to enhance clarity.
- // ObjectTypes describe the types of objects that are part of the module's schema.
+ // ObjectTypes is an array of ObjectType that defines the different object types managed by the module. Each type describes the structure and fields that will be indexed and queried.This change provides more context about what ObjectTypes represents and how it's used, aligning with the previous suggestion made by coderabbitai[bot].
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.
| // ModuleSchema represents the logical schema of a module for purposes of indexing and querying. | |
| type ModuleSchema struct { | |
| // ObjectTypes describe the types of objects that are part of the module's schema. | |
| ObjectTypes []ObjectType | |
| } | |
| // ModuleSchema represents the logical schema of a module for purposes of indexing and querying. | |
| type ModuleSchema struct { | |
| // ObjectTypes is an array of ObjectType that defines the different object types managed by the module. Each type describes the structure and fields that will be indexed and queried. | |
| ObjectTypes []ObjectType | |
| } |
indexer/base/decoder.go
Outdated
| // DecodableModule is an interface that modules can implement to provide a ModuleDecoder. | ||
| // Usually these modules would also implement appmodule.AppModule, but that is not included | ||
| // to keep this package free of any dependencies. | ||
| type DecodableModule interface { | ||
|
|
||
| // ModuleDecoder returns a ModuleDecoder for the module. | ||
| ModuleDecoder() (ModuleDecoder, error) | ||
| } | ||
|
|
||
| // ModuleDecoder is a struct that contains the schema and a KVDecoder for a module. | ||
| type ModuleDecoder struct { | ||
| // Schema is the schema for the module. | ||
| Schema ModuleSchema | ||
|
|
||
| // KVDecoder is a function that decodes a key-value pair into an ObjectUpdate. | ||
| // If modules pass logical updates directly to the engine and don't require logical decoding of raw bytes, | ||
| // then this function should be nil. | ||
| KVDecoder KVDecoder | ||
| } | ||
|
|
||
| // KVDecoder is a function that decodes a key-value pair into an ObjectUpdate. | ||
| // If the KV-pair doesn't represent an object update, the function should return false | ||
| // as the second return value. Error should only be non-nil when the decoder expected | ||
| // to parse a valid update and was unable to. | ||
| type KVDecoder = func(key, value []byte) (ObjectUpdate, bool, error) |
There was a problem hiding this comment.
I have added the DecodableModule interface to this PR so that we can see how everything will link up to modules and what collections will need to provide. So we can consider this replacing #20537.
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range and nitpick comments (1)
indexer/base/decoder.go (1)
3-10: Ensure clear documentation forDecodableModuleinterface.Consider adding more detailed documentation for the
DecodableModuleinterface, explaining when and why a module should implement this interface, especially in the context of the indexing system. This could help developers understand its usage better.
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (1)
- indexer/base/decoder.go (1 hunks)
Additional context used
Path-based instructions (1)
indexer/base/decoder.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Additional comments not posted (1)
indexer/base/decoder.go (1)
12-21: Review theModuleDecoderstruct for clarity and functionality.The
ModuleDecoderstruct is well-defined with clear fields forSchemaandKVDecoder. However, consider adding a brief example or more detailed comments on howKVDecoderis typically implemented, especially in cases where it might be set tonil.
| // KVDecoder is a function that decodes a key-value pair into an ObjectUpdate. | ||
| // If the KV-pair doesn't represent an object update, the function should return false | ||
| // as the second return value. Error should only be non-nil when the decoder expected | ||
| // to parse a valid update and was unable to. | ||
| type KVDecoder = func(key, value []byte) (ObjectUpdate, bool, error) |
There was a problem hiding this comment.
Clarify behavior and error handling in KVDecoder function.
The KVDecoder function type's documentation could be improved by explicitly stating what constitutes a "valid update" and under what conditions error should be non-nil. This will enhance understanding and correct usage of this function across different modules.
// KVDecoder is a function that decodes a key-value pair into an ObjectUpdate.
// If the KV-pair doesn't represent an object update, the function should return false
// as the second return value. Error should only be non-nil when the decoder expected
// to parse a valid update and was unable to.
+ // A "valid update" refers to a key-value pair that conforms to the expected schema and data format for the module.
+ // Errors should be returned if the key-value pair is malformed or if essential data is missing, making decoding impossible.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.
| // KVDecoder is a function that decodes a key-value pair into an ObjectUpdate. | |
| // If the KV-pair doesn't represent an object update, the function should return false | |
| // as the second return value. Error should only be non-nil when the decoder expected | |
| // to parse a valid update and was unable to. | |
| type KVDecoder = func(key, value []byte) (ObjectUpdate, bool, error) | |
| // KVDecoder is a function that decodes a key-value pair into an ObjectUpdate. | |
| // If the KV-pair doesn't represent an object update, the function should return false | |
| // as the second return value. Error should only be non-nil when the decoder expected | |
| // to parse a valid update and was unable to. | |
| // A "valid update" refers to a key-value pair that conforms to the expected schema and data format for the module. | |
| // Errors should be returned if the key-value pair is malformed or if essential data is missing, making decoding impossible. | |
| type KVDecoder = func(key, value []byte) (ObjectUpdate, bool, error) |
indexer/base/go.mod
Outdated
| // NOTE: this go.mod should have zero dependencies and remain on an older version of Go | ||
| // to be compatible with legacy codebases. | ||
|
|
||
| go 1.19 |
There was a problem hiding this comment.
One comment from our call, should we target an even earlier go release? One excellent outcome here be that if all else fails, i.e. I (as streaming developer) can't logically decode state updates from previous versions in the latest app binary, at least I can patch prior versions and use those binaries to stream from genesis.
There was a problem hiding this comment.
If we're going all the way back to the beginning (basically Gaia 1.0) that means Go v1.12. I think the main change is converting all anys to interface{}. Any other changes you can see needed?
There was a problem hiding this comment.
Made this update. Is there any easy way we could check this in CI? I guess we could create a GitHub action with an older version of go just for this module?
There was a problem hiding this comment.
Would we also want to make sure that the Postgres indexer module is compatible with earlier versions of Go? I guess I'm wondering if there would be any issue building legacy codebases with a newer version of Go. For instance, would Gaia 1.0 compile with Go 1.22? Maybe I'll do a quick check
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (3)
- indexer/base/go.mod (1 hunks)
- indexer/base/listener.go (1 hunks)
- indexer/base/object_update.go (1 hunks)
Files skipped from review as they are similar to previous changes (3)
- indexer/base/go.mod
- indexer/base/listener.go
- indexer/base/object_update.go
indexer/base/README.md
Outdated
|
|
||
| The indexer base module is designed to provide a stable, zero-dependency base layer for the built-in indexer functionality. Packages that integrate with the indexer should feel free to depend on this package without fear of any external dependencies being pulled in. | ||
|
|
||
| The basic types for specifying index sources, targets and decoders are provided here along with a basic engine that ties these together. A package wishing to be an indexing source could accept an instance of `Engine` directly to be compatible with indexing. A package wishing to be a decoder can use the `Entity` and `Table` types. A package defining an indexing target should implement the `Indexer` interface. |
There was a problem hiding this comment.
can we add an architecture diagram using mermaid here to help explain the overall design
There was a problem hiding this comment.
Sure, just noticing this README text is out of date too. Will update
There was a problem hiding this comment.
I've updated the README and added a sequence diagram around Listener calls. Let me know how that looks @tac0turtle.
I could also add a class diagram around the schema and object updates, but not sure that's too useful.
| @@ -0,0 +1,6 @@ | |||
| module cosmossdk.io/indexer/base | |||
There was a problem hiding this comment.
this works, no need to change
| // DurationKind is a duration type and values of this type must be of the go type time.Duration. | ||
| DurationKind | ||
|
|
||
| // Float32Kind is a float32 type and values of this type must be of the go type float32. |
There was a problem hiding this comment.
in the case of cosmwasm i belive they have this type and would want to add it if they were to write a decoder
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (1)
- indexer/base/README.md (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- indexer/base/README.md
There was a problem hiding this comment.
Actionable comments posted: 3
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (1)
- indexer/base/README.md (1 hunks)
Additional context used
Path-based instructions (1)
indexer/base/README.md (1)
Pattern
**/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"
LanguageTool
indexer/base/README.md
[style] ~28-~28: To form a complete sentence, be sure to include a subject. (MISSING_IT_THERE)
Context: ...dexer: Commit end ```Initializeshould be called before anything else and get ...
[uncategorized] ~32-~32: Loose punctuation mark. (UNLIKELY_OPENING_PUNCTUATION)
Context: ...leModuleimplementations.StartBlock,OnBlockHeader` should be called only o...
[uncategorized] ~32-~32: Possible missing comma found. (AI_HYDRA_LEO_MISSING_COMMA)
Context: ... called only once at the beginning of a block andCommitshould be called only once...
Markdownlint
indexer/base/README.md
32-32: Expected: 0 or 2; Actual: 1 (MD009, no-trailing-spaces)
Trailing spaces
34-34: Expected: 1; Actual: 2 (MD012, no-multiple-blanks)
Multiple consecutive blank lines
35-35: Expected: 1; Actual: 3 (MD012, no-multiple-blanks)
Multiple consecutive blank lines
Additional comments not posted (1)
indexer/base/README.md (1)
11-26: The sequence diagram is well-constructed and provides a clear visual representation of the callback flow. This is an excellent addition for understanding the operational flow of theListenerinterface.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
|
I'd like to move forward with merging this so other PRs can move forward. We can iterate and improve docs and/or make API changes in follow-ups if needed |
Description
Ref: #20352 #20532
This PR creates a new go module
cosmossdk.io/indexer/baseand introduces basic types related to logical decoding and blockchain data listeners. It is the first small PR extracted from #20547. See #20547 for more details on other pieces intended forindexer/basesuch as the indexingEngineand how this module fits in with the postgres indexer and logical decoding of collections.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
Indexer Basemodule with a stable, zero-dependency foundation for built-in indexer functionality.