feat(indexer/postgres)!: add basic support for header, txs and events #22695
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThe changes in this pull request involve multiple modifications across various files, primarily focusing on enhancing error handling, logging, and event processing capabilities. Key updates include improvements to the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yml ⛔ Files ignored due to path filters (7)
📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (5)
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]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
github-actions
bot
added
C:server/v2
facundomedica
requested review from
a team,
testinginprod,
sontrinh16,
kocubinski,
julienrbrt,
hieuvubk and
cool-develope
as code owners
|
@facundomedica 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: 6
🧹 Outside diff range and nitpick comments (7)
baseapp/streaming.go (2)
5-5: Remove unnecessary import ofencoding/jsonThe import of
"encoding/json"is redundant if not used elsewhere in the code. Ensure that all imported packages are utilized to maintain a clean codebase.Apply this diff to remove the unused import:
import ( - "encoding/json" "fmt" "sort"
Line range hint
149-162: Update function documentation to reflect new parameterThe
eventToAppDataEventfunction now accepts an additionalheightparameter. Update the function's documentation comment to reflect this change for better clarity.Apply this diff to update the function documentation:
-func eventToAppDataEvent(event abci.Event) (appdata.Event, error) { +// eventToAppDataEvent converts an abci.Event to an appdata.Event, including the block height. +func eventToAppDataEvent(event abci.Event, height int64) (appdata.Event, error) {server/v2/cometbft/oe/optimistic_execution_test.go (2)
8-10: Organize imports according to Go conventionGroup standard library imports separately from third-party imports, and sort them alphabetically within their groups to enhance readability, as per Go conventions.
Apply this diff to reorganize imports:
import ( "context" "errors" "testing" + "github.com/stretchr/testify/assert" + + abci "github.com/cometbft/cometbft/api/cometbft/abci/v1" - abci "github.com/cometbft/cometbft/api/cometbft/abci/v1" - "github.com/stretchr/testify/assert" "cosmossdk.io/core/server" "cosmossdk.io/core/store" "cosmossdk.io/core/transaction" "cosmossdk.io/log" )
Line range hint
25-27: Check for potential data races in test executionThe test may not be thread-safe if accessed concurrently. Ensure that shared resources are properly synchronized to avoid data races during parallel test runs.
Consider using sync mechanisms like mutexes if the test suite allows for concurrent execution.
schema/decoding/middleware.go (1)
43-44: Consider logging when skipping modules without codecsWhile silently continuing when a codec is not found makes the system more resilient, it might hide potential issues from operators. Consider adding debug-level logging to inform about skipped modules.
- // we don't have a codec for this module so continue - continue + // we don't have a codec for this module so skip it + if a.logger.IsDebug() { + a.logger.Debug("skipping module without codec", "module", moduleName) + } + continueruntime/v2/go.mod (1)
Line range hint
3-3: Update Go version to a released versionThe module specifies Go 1.23 which hasn't been released yet. This could cause compatibility issues.
Update to the latest stable version:
-go 1.23 +go 1.22x/accounts/keeper.go (1)
65-72: LGTM: Enhanced map key naming improves code clarity.The addition of named keys for collections.Map instances improves code readability and maintainability. Consider adding a comment block explaining the purpose of each named key for better documentation.
Add documentation like this:
+// AccountsByType maps account addresses to their implementation types: +// - address: the account's address in bytes +// - type: the account's implementation type as a string AccountsByType: collections.NewMap(sb, AccountTypeKeyPrefix, "accounts_by_type", collections.BytesKey.WithName("address"), collections.StringValue.WithName("type")),
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
⛔ Files ignored due to path filters (3)
go.sumis excluded by!**/*.sumserver/v2/stf/go.sumis excluded by!**/*.sumsimapp/v2/go.sumis excluded by!**/*.sum
📒 Files selected for processing (27)
baseapp/abci.go(1 hunks)baseapp/streaming.go(7 hunks)codec/collections.go(1 hunks)go.mod(1 hunks)indexer/postgres/base_sql.go(1 hunks)indexer/postgres/listener.go(2 hunks)runtime/v2/app.go(1 hunks)runtime/v2/go.mod(1 hunks)schema/appdata/data.go(2 hunks)schema/decoding/middleware.go(1 hunks)server/v2/cometbft/abci.go(3 hunks)server/v2/cometbft/abci_test.go(3 hunks)server/v2/cometbft/go.mod(1 hunks)server/v2/cometbft/oe/optimistic_execution_test.go(1 hunks)server/v2/cometbft/server.go(1 hunks)server/v2/cometbft/streaming.go(3 hunks)server/v2/go.mod(1 hunks)server/v2/stf/go.mod(1 hunks)server/v2/stf/stf.go(5 hunks)simapp/go.mod(1 hunks)simapp/v2/app_di.go(1 hunks)simapp/v2/go.mod(3 hunks)tests/go.mod(1 hunks)x/accounts/go.mod(1 hunks)x/accounts/keeper.go(1 hunks)x/accounts/module.go(2 hunks)x/distribution/go.mod(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- server/v2/cometbft/server.go
- server/v2/stf/go.mod
🧰 Additional context used
📓 Path-based instructions (17)
runtime/v2/app.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
schema/decoding/middleware.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
x/accounts/module.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"
codec/collections.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
server/v2/stf/stf.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
server/v2/cometbft/oe/optimistic_execution_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"
schema/appdata/data.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
baseapp/abci.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
server/v2/cometbft/streaming.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/abci.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
indexer/postgres/base_sql.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
indexer/postgres/listener.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
x/accounts/keeper.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
baseapp/streaming.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 (24)
indexer/postgres/listener.go (1)
85-87: Consider splitting listener methods into separate files
To enhance code organization and maintainability, consider breaking out the OnTx and OnEvent implementations into separate files like tx.go and event.go.
baseapp/streaming.go (1)
232-243: Handle transaction decoding errors appropriately
When decoding transactions, logging the error and proceeding might mask underlying issues. Consider handling the error explicitly, possibly by recording failed transactions separately or by failing the process if decoding is critical.
Provide a verification script to check how often transaction decoding fails:
server/v2/cometbft/abci.go (1)
541-543: Pass all relevant transaction data to streamDeliverBlockChanges
Ensure that the decodedTxs variable is correctly populated and passed to streamDeliverBlockChanges to prevent potential nil pointer dereferences or missing transaction data in downstream processing.
Review the code to confirm that decodedTxs is not nil and contains all necessary transaction information.
indexer/postgres/base_sql.go (2)
20-21: Ensure consistency of NOT NULL constraints in tx table
The data column is now NULL, but it might be essential for certain queries. Verify if allowing NULL values is intentional and won't impact data integrity.
32-34: Clarify the necessity of allowing NULL values in type and data columns
Allowing NULL values in the type and data columns of the event table may lead to incomplete event records. Confirm if this change is necessary and that it won't affect the integrity of event data.
x/accounts/module.go (1)
10-10: LGTM: Required imports for schema handling
The new imports are necessary for implementing the schema.HasModuleCodec interface.
Also applies to: 14-14
server/v2/cometbft/streaming.go (1)
20-20:
Verify length consistency between txs and decodedTxs
The function assumes that txs and decodedTxs slices have the same length, but this isn't validated.
Add length validation at the start of the function:
+if len(txs) != len(decodedTxs) {
+ return fmt.Errorf("mismatched lengths: txs=%d, decodedTxs=%d", len(txs), len(decodedTxs))
+}schema/appdata/data.go (1)
34-35: LGTM! Well-documented field additions
The BlockNumber field additions to both TxData and Event structs are well-documented and use appropriate types.
Also applies to: 59-60
server/v2/go.mod (1)
7-7: LGTM: Schema module path correctly configured
The replace directive for the schema module is properly configured to use the local path.
simapp/v2/app_di.go (2)
7-7: LGTM: PostgreSQL driver properly imported
The pgx driver is imported with a blank identifier, which is the correct pattern for SQL drivers.
13-13: LGTM: PostgreSQL indexer registration
The postgres indexer is properly imported with a blank identifier for side-effect registration.
x/distribution/go.mod (1)
178-178: LGTM: Schema module path correctly configured
The replace directive for the schema module is properly configured to use the local path, maintaining consistency with other modules.
server/v2/cometbft/go.mod (1)
6-6: LGTM: Schema module replacement is correctly configured
The addition of cosmossdk.io/schema => ../../../schema uses the correct relative path and maintains consistency with the module organization pattern.
go.mod (1)
189-189: LGTM: Schema module replacement is properly configured at root level
The addition of cosmossdk.io/schema => ./schema is correctly configured. However, note that this is part of the temporary replacements that will be removed once all modules have their own go.mods, as indicated by the TODO comment.
Let's verify the schema module's independence status:
✅ Verification successful
Schema module replacement is temporary but necessary
The schema module already has its own go.mod file, but it's intentionally kept dependency-free and on Go 1.12 for backward compatibility with all Cosmos SDK production releases. The root-level replacement is still needed temporarily to maintain build consistency while ensuring the module remains accessible for historical app patching.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the schema module has its own go.mod file
# Expected: Presence of go.mod would indicate progress toward module independence
fd -t f "^go.mod$" schema/
Length of output: 64
Script:
#!/bin/bash
# Let's check the content of schema/go.mod to verify if it's properly configured
cat schema/go.mod
Length of output: 300
codec/collections.go (1)
401-401: LGTM: Setting BytesKind fields as nullable is correct.
This change ensures proper handling of byte fields in protobuf messages by marking them as nullable, aligning with protobuf's behavior where bytes can be nil/empty.
tests/go.mod (1)
290-290: LGTM: Appropriate dependency management updates.
The addition of the schema module replace directive ensures consistent local development, while the keyring replacement aligns with the project's security practices.
simapp/go.mod (1)
255-256: LGTM: Required dependencies for postgres indexer feature.
The addition of schema and postgres indexer replace directives properly supports the PR's objective of adding basic postgres indexer support.
Let's verify the postgres dependencies are properly configured:
✅ Verification successful
PostgreSQL dependencies are properly configured
The verification confirms that all required PostgreSQL dependencies are correctly set up:
- Core PostgreSQL driver (
github.com/jackc/pgx/v5) is present in go.mod files - Required support packages (pgpassfile, pgservicefile) are properly included
- Driver is correctly imported with
_ "github.com/jackc/pgx/v5/stdlib"in relevant files - Dependency versions are consistent across the codebase
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify postgres-related dependencies
# Test: Check if all required postgres dependencies are present
rg -A 1 'github.com/jackc/pg'
Length of output: 5028
simapp/v2/go.mod (1)
303-303: LGTM: Replace directives are correctly configured.
The replace directives for indexer/postgres and schema are properly set up to use local development paths, which is the expected configuration for development work.
Also applies to: 305-305
server/v2/stf/stf.go (2)
Line range hint 207-211: LGTM: Consistent event enrichment in deliverTx.
The addition of BlockNumber and proper event staging in both validation and execution events improves traceability and debugging capabilities.
Also applies to: 218-221
378-381: LGTM: Consistent block stage event handling.
The changes maintain consistency in setting BlockNumber and BlockStage across preBlock, beginBlock, and endBlock events, which enhances event tracking throughout the block lifecycle.
Also applies to: 396-399, 420-423
baseapp/abci.go (1)
1000-1003: LGTM: Important safety check for block 1 commit failures
The added check prevents attempting to rollback to height 0 when a commit fails at block 1, which would be an invalid operation. This is a critical safety feature that ensures proper error handling during chain initialization.
x/accounts/go.mod (1)
181-181: LGTM: Schema module replacement is correctly configured.
The replace directive for cosmossdk.io/schema follows the established pattern for local module replacements.
server/v2/cometbft/abci_test.go (2)
14-14: LGTM: Import organization is clean and clear.
The import statements are well-organized, with the new abci import properly aliased and core imports appropriately positioned.
Also applies to: 22-22, 30-30
63-66: LGTM: Generic type constraints are properly formatted.
The indentation of generic type constraints improves readability while maintaining the same functionality.
Also applies to: 93-96
| attrsMap := map[string]interface{}{} | ||
| for _, attr := range attrs { | ||
| attrsMap[attr.Key] = attr.Value | ||
| } |
There was a problem hiding this comment.
Preserve all event attributes by handling duplicate keys appropriately
Currently, attributes with duplicate keys may overwrite previous entries due to the use of a map. This can lead to loss of data for events that have multiple attributes with the same key. To ensure all attributes are preserved, store them as a slice of key-value pairs instead.
Apply this diff to modify the handling of attributes:
-func (e *eventData) Attributes() ([]EventAttribute, error) {
- attrsMap := map[string]interface{}{}
- for _, attr := range attrs {
- attrsMap[attr.Key] = attr.Value
- }
- jsonAttributes, err = json.Marshal(attrsMap)
+func (e *eventData) Attributes() ([]EventAttribute, error) {
+ attrsSlice := []map[string]interface{}{}
+ for _, attr := range attrs {
+ attrsSlice = append(attrsSlice, map[string]interface{}{
+ "key": attr.Key,
+ "value": attr.Value,
+ })
+ }
+ jsonAttributes, err = json.Marshal(attrsSlice)
if err != nil {
return fmt.Errorf("failed to marshal event attributes: %w", err)
}
}Committable suggestion skipped: line range outside the PR's diff.
| decodedTxs, err := decodeTxs(c.logger, req.Txs, c.txCodec) | ||
| if err != nil { | ||
| return nil, nil, nil, err | ||
| } |
There was a problem hiding this comment.
Ineffective error handling in decodeTxs function
The decodeTxs function logs decoding errors but continues processing, which may lead to nil transactions being included in further processing, potentially causing panics. Ensure that the error handling either skips invalid transactions or fails gracefully.
Apply this diff to modify the error handling:
func decodeTxs[T transaction.Tx](logger log.Logger, rawTxs [][]byte, codec transaction.Codec[T]) ([]T, error) {
txs := make([]T, 0, len(rawTxs))
for _, rawTx := range rawTxs {
tx, err := codec.Decode(rawTx)
if err != nil {
- // do not return an error here, as we want to deliver the block even if some txs are invalid
- logger.Debug("failed to decode tx", "err", err)
+ // skip invalid transactions
+ logger.Error("failed to decode tx", "err", err)
+ continue
}
- txs[i] = tx
+ txs = append(txs, tx)
}
return txs, nil
}|
|
||
| for _, overrideKey := range a.config.OverrideStoreKeys { | ||
| moduleSet[overrideKey.KvStoreKey] = moduleSet[overrideKey.ModuleName] | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Validate module existence before applying overrides
The current implementation assumes that the module specified in overrideKey.ModuleName exists in the moduleSet. We should validate this to prevent nil pointer dereferences.
for _, overrideKey := range a.config.OverrideStoreKeys {
+ module, exists := moduleSet[overrideKey.ModuleName]
+ if !exists {
+ return nil, fmt.Errorf("module %s not found for store key override %s",
+ overrideKey.ModuleName, overrideKey.KvStoreKey)
+ }
- moduleSet[overrideKey.KvStoreKey] = moduleSet[overrideKey.ModuleName]
+ moduleSet[overrideKey.KvStoreKey] = module
}📝 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.
| for _, overrideKey := range a.config.OverrideStoreKeys { | |
| moduleSet[overrideKey.KvStoreKey] = moduleSet[overrideKey.ModuleName] | |
| } | |
| for _, overrideKey := range a.config.OverrideStoreKeys { | |
| module, exists := moduleSet[overrideKey.ModuleName] | |
| if !exists { | |
| return nil, fmt.Errorf("module %s not found for store key override %s", | |
| overrideKey.ModuleName, overrideKey.KvStoreKey) | |
| } | |
| moduleSet[overrideKey.KvStoreKey] = module | |
| } |
| // ModuleCodec implements `schema.HasModuleCodec` interface. | ||
| // It allows the indexer to decode the module's KVPairUpdate. | ||
| func (am AppModule) ModuleCodec() (schema.ModuleCodec, error) { | ||
| return am.k.Schema.ModuleCodec(collections.IndexingOptions{}) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding error handling for nil Schema
The current implementation assumes that the keeper's Schema is properly initialized. Consider adding a nil check to prevent potential panics.
func (am AppModule) ModuleCodec() (schema.ModuleCodec, error) {
+ if am.k.Schema == nil {
+ return schema.ModuleCodec{}, fmt.Errorf("schema not initialized for accounts module")
+ }
return am.k.Schema.ModuleCodec(collections.IndexingOptions{})
}Committable suggestion skipped: line range outside the PR's diff.
| BlockNumber: uint64(height), | ||
| TxIndex: int32(i), | ||
| Bytes: func() ([]byte, error) { return tx, nil }, | ||
| JSON: func() (json.RawMessage, error) { | ||
| return json.Marshal(decodedTxs[i]) | ||
| }, |
There was a problem hiding this comment.
Add error handling for JSON marshaling
The JSON marshaling operation could fail, but the error is not being handled properly. This could lead to silent failures.
Apply this fix:
JSON: func() (json.RawMessage, error) {
- return json.Marshal(decodedTxs[i])
+ if i >= len(decodedTxs) {
+ return nil, fmt.Errorf("index %d out of bounds for decodedTxs (len=%d)", i, len(decodedTxs))
+ }
+ return json.Marshal(decodedTxs[i])
},📝 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.
| BlockNumber: uint64(height), | |
| TxIndex: int32(i), | |
| Bytes: func() ([]byte, error) { return tx, nil }, | |
| JSON: func() (json.RawMessage, error) { | |
| return json.Marshal(decodedTxs[i]) | |
| }, | |
| BlockNumber: uint64(height), | |
| TxIndex: int32(i), | |
| Bytes: func() ([]byte, error) { return tx, nil }, | |
| JSON: func() (json.RawMessage, error) { | |
| if i >= len(decodedTxs) { | |
| return nil, fmt.Errorf("index %d out of bounds for decodedTxs (len=%d)", i, len(decodedTxs)) | |
| } | |
| return json.Marshal(decodedTxs[i]) | |
| }, |
| @@ -43,9 +43,11 @@ require ( | |||
| ) | |||
|
|
|||
| require ( | |||
| cosmossdk.io/indexer/postgres v0.0.0-20241204160609-556102cfa046 | |||
There was a problem hiding this comment.
💡 Codebase verification
Critical: Update pgx dependency to patch security vulnerabilities
The codebase is using pgx v5.7.1 which is vulnerable to SQL injection attacks (CVE published on March 4, 2024). The current version is within the vulnerable range ">= 5.0.0, < 5.5.4".
- Update
github.com/jackc/pgx/v5from v5.7.1 to at least v5.5.4 in:- simapp/go.mod
- simapp/v2/go.mod
- indexer/postgres/tests/go.mod
- tests/go.mod
This addresses two HIGH severity vulnerabilities:
- SQL Injection via Protocol Message Size Overflow
- SQL Injection via Line Comment Creation
🔗 Analysis chain
Verify PostgreSQL dependency versions for security vulnerabilities.
The PostgreSQL-related dependencies have been added:
github.com/jackc/pgx/v5v5.7.1github.com/jackc/pgpassfilev1.0.0github.com/jackc/pgservicefilev0.0.0-20240606120523-5a60cdf6a761github.com/jackc/puddle/v2v2.2.2
Also applies to: 50-50, 161-163
Let me verify the exact version of pgx being used.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for security advisories in PostgreSQL-related dependencies
gh api graphql -f query='
{
securityVulnerabilities(first: 5, ecosystem: GO, package: "github.com/jackc/pgx") {
nodes {
advisory {
summary
severity
publishedAt
}
vulnerableVersionRange
firstPatchedVersion {
identifier
}
}
}
}'
Length of output: 1001
Script:
#!/bin/bash
# Check the exact version of pgx in go.mod files
rg "github.com/jackc/pgx/v5" -A 2
Length of output: 3044
There was a problem hiding this comment.
coderabbit doesn't understand semantic versioning?
There was a problem hiding this comment.
@facundomedica, thank you for pointing that out. You are correct; I apologize for the misunderstanding. Version v5.7.1 of github.com/jackc/pgx/v5 is not within the vulnerable range and is secure. I appreciate your correction.
✏️ Learnings added
Learnt from: facundomedica
PR: cosmos/cosmos-sdk#22695
File: simapp/v2/go.mod:46-46
Timestamp: 2024-12-06T14:38:24.256Z
Learning: Versions of `github.com/jackc/pgx/v5` starting from `v5.5.4` and above are secure and not affected by the SQL injection vulnerabilities present in earlier versions.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
indexer/postgres/base_sql.go
Outdated
| event_index BIGINT NOT NULL, | ||
| type TEXT NULL, | ||
| data JSONB NULL, | ||
| attributes JSONB NULL |
There was a problem hiding this comment.
I don't think we should store both data and attributes. I think if an event has Data, we should use that. If it doesn't have data and just has Attributes we should store that in data as JSON
There was a problem hiding this comment.
So basically I think we just need one data column, otherwise it's a lot of duplicated data in most cases
There was a problem hiding this comment.
Can we make sure it's documented on the Event struct that Data and Attributes should represent the same thing and that Data is preferred by indexers if it is present.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
indexer/postgres/listener.go (2)
90-115: Add function documentation and improve error messagesThe transaction listener implementation would benefit from:
- GoDoc comments explaining the function's purpose and parameters
- More descriptive error messages that include context about the transaction
Apply this diff to add documentation:
+// txListener returns a function that processes and stores transaction data in PostgreSQL. +// It handles both raw transaction bytes and JSON representations. func txListener(i *indexerImpl) func(data appdata.TxData) error {Also, enhance error messages:
- return err + return fmt.Errorf("failed to get transaction bytes at height %d, index %d: %w", + td.BlockNumber, td.TxIndex, err)
145-149: Enhance error context in SQL operationsThe error message could provide more context about the event being processed.
Apply this diff to improve error handling:
_, err := i.tx.Exec("INSERT INTO event (block_number, block_stage, tx_index, msg_index, event_index, type, data) VALUES ($1, $2, $3, $4, $5, $6, $7)", e.BlockNumber, e.BlockStage, e.TxIndex, e.MsgIndex, e.EventIndex, e.Type, jsonData) if err != nil { - return fmt.Errorf("failed to index event: %w", err) + return fmt.Errorf("failed to index event (block: %d, type: %s, tx_index: %d): %w", + e.BlockNumber, e.Type, e.TxIndex, err) }
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (2)
indexer/postgres/base_sql.go(1 hunks)indexer/postgres/listener.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- indexer/postgres/base_sql.go
🧰 Additional context used
📓 Path-based instructions (1)
indexer/postgres/listener.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
🔇 Additional comments (3)
indexer/postgres/listener.go (3)
4-4: LGTM!
The added json package import is necessary for the new JSON handling functionality.
85-88: Consider splitting the code into separate files
As suggested in a previous review, consider breaking out the transaction and event handling code into separate files (tx.go and event.go) to improve code organization and maintainability.
134-143: Preserve all event attributes by handling duplicate keys appropriately
The current implementation may lose data when events have multiple attributes with the same key.
julienrbrt
added
backport/v0.52.x
|
Adding backport label for SDK changes. The modules tagged from main can be deleted from the backport. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
baseapp/abci.go (1)
1002-1005: LGTM! Consider enhancing the error message.The special handling for block height 1 failures is a good addition that prevents invalid rollbacks and provides clear guidance.
Consider enhancing the error message to provide more guidance:
- return nil, fmt.Errorf("failed to commit block 1, can't automatically rollback: %w", err) + return nil, fmt.Errorf("failed to commit block 1, manual node reset required (can't automatically rollback to height 0): %w", err)
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
⛔ Files ignored due to path filters (2)
go.sumis excluded by!**/*.sumsimapp/v2/go.sumis excluded by!**/*.sum
📒 Files selected for processing (12)
baseapp/abci.go(1 hunks)go.mod(1 hunks)runtime/v2/go.mod(1 hunks)server/v2/cometbft/abci.go(3 hunks)server/v2/cometbft/go.mod(1 hunks)server/v2/go.mod(1 hunks)simapp/go.mod(1 hunks)simapp/v2/go.mod(3 hunks)tests/go.mod(1 hunks)x/accounts/go.mod(1 hunks)x/accounts/module.go(2 hunks)x/distribution/go.mod(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (11)
- server/v2/cometbft/go.mod
- x/accounts/module.go
- server/v2/go.mod
- runtime/v2/go.mod
- go.mod
- tests/go.mod
- x/accounts/go.mod
- x/distribution/go.mod
- server/v2/cometbft/abci.go
- simapp/go.mod
- simapp/v2/go.mod
🧰 Additional context used
📓 Path-based instructions (1)
baseapp/abci.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
…s-sdk into facu/indexer-events
…#22695) (cherry picked from commit 332d0b1) # Conflicts: # go.mod # go.sum # indexer/postgres/base_sql.go # indexer/postgres/listener.go # runtime/v2/app.go # runtime/v2/go.mod # runtime/v2/go.sum # schema/appdata/data.go # schema/decoding/middleware.go # server/v2/cometbft/go.mod # server/v2/cometbft/go.sum # server/v2/go.mod # server/v2/go.sum # server/v2/stf/go.mod # server/v2/stf/go.sum # server/v2/stf/stf.go # simapp/go.mod # simapp/go.sum # simapp/v2/go.mod # simapp/v2/go.sum # tests/go.sum # x/accounts/go.mod # x/accounts/go.sum # x/distribution/go.mod # x/distribution/go.sum
… (backport #22695) (#22808) Co-authored-by: Facundo Medica <[email protected]> Co-authored-by: Julien Robert <[email protected]>
Description
Related to epic #20352
Missing:
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
Documentation
Chores