Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(indexer/postgres)!: add basic support for header, txs and events #22695

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

facundomedica
Copy link
Member

@facundomedica facundomedica commented Nov 29, 2024

Description

Related to epic #20352

  • Added failure message if the indexer fails on block 1, as we can't rollback from that, the user will have to reset the node before restarting, otherwise the indexer will have missed that block.
  • Added txDecoder to baseapp listenerWrapper, which will then be used to get the JSON of the decoded tx.
  • Added BlockNumber to event and TxData
  • Added some fixes for v2

Missing:

  • tx hash. For this we need to use the decoded tx's Hash method, which is not accessible from the postgres indexer side, and if we want to access it from the baseapp side then we'd need to decode it before.

Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

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

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.

Please see Pull Request Reviewer section in the contributing guide for more information on how to review a pull request.

I have...

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

Summary by CodeRabbit

  • New Features

    • Enhanced error handling for commit failures at block height 1.
    • Added block height context to transaction and event handling.
    • Introduced new methods for processing transactions and events in the listener.
  • Improvements

    • Improved transaction decoding and event processing capabilities.
    • Updated SQL schema for better data flexibility in transaction and event tables.
    • Enhanced logging for better error management.
  • Dependency Updates

    • Added new dependencies and updated existing ones for improved functionality and integration.

Copy link
Contributor

coderabbitai bot commented Nov 29, 2024

📝 Walkthrough

Walkthrough

The pull request introduces significant changes across multiple files, primarily focusing on enhancing error handling, event processing, and dependency management. Key modifications include improvements to the Commit method in baseapp/abci.go, which now includes specific error handling for block height scenarios. The streaming.go file has been updated to improve transaction decoding and event context. Additionally, various go.mod files have been modified to include new dependencies and replace directives, reflecting a restructuring of module paths. Overall, the changes aim to enhance the functionality and robustness of the application.

Changes

File Change Summary
baseapp/abci.go Modified Commit method to enhance error handling for block height 1, including rollback logic and improved logging.
baseapp/streaming.go Updated listenerWrapper struct to include txDecoder, modified EnableIndexer and eventToAppDataEvent to handle block height, and enhanced OnTx method to include block number in transaction data.
codec/collections.go Updated protoCol function to treat BytesKind fields as nullable and to accurately represent google.protobuf.Timestamp and google.protobuf.Duration in schema generation.
go.mod Added new dependencies and updated existing ones, including cosmossdk.io/schema and local path replacements.
indexer/postgres/base_sql.go Modified SQL schema for tx and event tables, allowing null values for certain columns and adding new columns for better data handling.
indexer/postgres/listener.go Introduced OnTx and OnEvent methods for processing transaction and event data, with error handling improvements.
runtime/v2/app.go Enhanced SchemaDecoderResolver to handle OverrideStoreKeys from the App struct's config.
runtime/v2/go.mod Added replace directive for cosmossdk.io/schema to point to a local path.
schema/appdata/data.go Added BlockNumber field to TxData and Event structs for better event tracking.
schema/decoding/middleware.go Modified error handling logic in Middleware function to continue processing on codec errors instead of returning immediately.
server/v2/cometbft/abci.go Updated FinalizeBlock and internalFinalizeBlock methods for improved transaction handling and error logging.
server/v2/cometbft/abci_test.go Adjusted import statements and improved formatting of generic function signatures without changing logic.
server/v2/cometbft/go.mod Added replace directive for cosmossdk.io/schema to point to a local path.
server/v2/cometbft/oe/optimistic_execution_test.go Reorganized imports and structured tests for OptimisticExecution without altering core functionality.
server/v2/cometbft/server.go Modified import statements for better organization without changing logic.
server/v2/cometbft/streaming.go Updated streamDeliverBlockChanges method to include decoded transaction data and enhanced error handling.
server/v2/go.mod Added replace directive and updated require statement for cosmossdk.io/schema.
server/v2/stf/go.mod Added replace directive for cosmossdk.io/schema to point to a local path.
server/v2/stf/stf.go Enhanced event handling in various methods by adding BlockNumber field for improved traceability.
simapp/go.mod Added new dependencies and updated existing ones, including local path replacements for cosmossdk.io/schema and cosmossdk.io/indexer/postgres.
simapp/v2/app_di.go Added imports for PostgreSQL database handling without altering existing methods.
simapp/v2/go.mod Updated dependencies for PostgreSQL support and local path replacements.
tests/go.mod Added cosmossdk.io/schema with a replace directive and updated indirect dependencies.
x/accounts/go.mod Updated dependencies and added a new replace directive for cosmossdk.io/schema.
x/accounts/keeper.go Enhanced NewKeeper function to improve clarity in collections.Map key structures without altering core logic.
x/accounts/module.go Added ModuleCodec method to AppModule for improved codec handling and added necessary imports.
x/distribution/go.mod Updated dependencies and added a replace directive for cosmossdk.io/schema.

Possibly related PRs

Suggested labels

C:collections

Suggested reviewers

  • kocubinski
  • sontrinh16
  • testinginprod
  • julienrbrt
  • tac0turtle

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

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

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

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

Other keywords and placeholders

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

Documentation and Community

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

Comment on lines 997 to +1003
app.logger.Error("Commit listening hook failed", "height", blockHeight, "err", err)
if app.streamingManager.StopNodeOnErr {
err = fmt.Errorf("Commit listening hook failed: %w", err)
if blockHeight == 1 {
// can't rollback to height 0, so just return the error
return nil, fmt.Errorf("failed to commit block 1, can't automatically rollback: %w", err)
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change potentially affects state.

Call sequence:

(*github.com/cosmos/cosmos-sdk/baseapp.BaseApp).Commit (baseapp/abci.go:970)

@github-actions github-actions bot added the C:x/distribution distribution module related label Dec 5, 2024
Copy link
Contributor

github-actions bot commented Dec 5, 2024

@facundomedica your pull request is missing a changelog!

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 6

🧹 Outside diff range and nitpick comments (7)
baseapp/streaming.go (2)

5-5: Remove unnecessary import of encoding/json

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

The eventToAppDataEvent function now accepts an additional height parameter. 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 convention

Group 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 execution

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

While 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)
+ }
+ continue
runtime/v2/go.mod (1)

Line range hint 3-3: Update Go version to a released version

The 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.22
x/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

📥 Commits

Reviewing files that changed from the base of the PR and between 6fb8299 and 689c530.

⛔ Files ignored due to path filters (3)
  • go.sum is excluded by !**/*.sum
  • server/v2/stf/go.sum is excluded by !**/*.sum
  • simapp/v2/go.sum is 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: ⚠️ Potential issue

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

Comment on lines +139 to +142
attrsMap := map[string]interface{}{}
for _, attr := range attrs {
attrsMap[attr.Key] = attr.Value
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.

Comment on lines +578 to 581
decodedTxs, err := decodeTxs(c.logger, req.Txs, c.txCodec)
if err != nil {
return nil, nil, nil, err
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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
 }

Comment on lines +97 to +100

for _, overrideKey := range a.config.OverrideStoreKeys {
moduleSet[overrideKey.KvStoreKey] = moduleSet[overrideKey.ModuleName]
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Suggested change
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
}

Comment on lines +131 to +135
// 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{})
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment on lines +81 to +86
BlockNumber: uint64(height),
TxIndex: int32(i),
Bytes: func() ([]byte, error) { return tx, nil },
JSON: func() (json.RawMessage, error) {
return json.Marshal(decodedTxs[i])
},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.

Suggested change
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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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/v5 from 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/v5 v5.7.1
  • github.com/jackc/pgpassfile v1.0.0
  • github.com/jackc/pgservicefile v0.0.0-20240606120523-5a60cdf6a761
  • github.com/jackc/puddle/v2 v2.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

event_index BIGINT NOT NULL,
type TEXT NULL,
data JSONB NULL,
attributes JSONB NULL
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So basically I think we just need one data column, otherwise it's a lot of duplicated data in most cases

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants