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): add to modules and implement proto fields #22544

Merged
merged 29 commits into from
Nov 28, 2024

Conversation

facundomedica
Copy link
Member

@facundomedica facundomedica commented Nov 18, 2024

Description

Implement necessary methods and named keys in:

  • x/staking
  • x/mint
  • x/epochs
  • x/distribution
  • x/consensus
  • x/circuit
  • x/bank
  • x/auth

Changed some default key codecs to be NameableKeyCodec (TimeKey, LengthPrefixedBytesKey, LengthPrefixedAddressKey)

Improve error messages in schema
Fixed AsyncListener in schema to avoid hanging on unprocessed packets

In baseapp, check for app.streamingManager.StopNodeOnErr so the indexer can crash the node if needed. Also the call to the store's Commit() was moved to after the streaming listener, so in case that something fails in the indexer we can stop the node before it commits. Otherwise when restarting the indexer would have skipped a block.

In codec:

  • collValue now implements SchemaCodec, which goes through the fields and returns the correct types for each + does the conversion if needed.
  • collInterfaceValue also implements SchemaCodec but always use JSON, as there's no way to know the necessary fields beforehand.

The following is a bit hacky:
In collections, for ModuleCodec, we use the value decoder to obtain an empty message (so this assumes that value decoders will return an empty struct and not a nil when passing an empty byte slice). This is used to optionally get a proto message which is then used to register any existing enums in the used messages.
We also adapt the full enum name to not use periods, otherwise it will fail somewhere else (Name: strings.ReplaceAll(string(enum.FullName()), ".", "_"), // make it compatible with schema).
The goal of this is so then the indexer can register the enum in the DB, which need to be registered before starting to write data.


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

Release Notes

  • New Features

    • Enhanced error handling for streaming listeners and application state management.
    • Introduced new codec functionalities for improved protobuf message handling.
    • Added ModuleCodec methods in multiple modules to support schema management and indexing.
    • New collection types and value codecs introduced in the collections module.
    • Implemented a RouterService for structured message handling in integration tests.
  • Bug Fixes

    • Improved error messages for validation checks in various modules.
  • Dependencies

    • Updated Go version to 1.23.3 across multiple modules.
    • Various dependencies updated to ensure compatibility and functionality.
  • Chores

    • Cleaned up and organized module paths and replace directives in Go module files.

Copy link
Contributor

coderabbitai bot commented Nov 18, 2024

Caution

Review failed

The head commit changed during the review from d94bde6 to dc20c04.

📝 Walkthrough
📝 Walkthrough

Walkthrough

The pull request introduces several updates across multiple files, primarily focusing on enhancing error handling, updating dependencies, and implementing new functionalities related to codec and schema management. Key changes include improved error handling in baseapp/abci.go, updates to Go versioning in various go.mod files, and the addition of new methods in several modules to support schema and codec functionalities. These modifications aim to enhance the robustness and maintainability of the codebase while ensuring compatibility with the latest Go versions.

Changes

File Change Summary
baseapp/abci.go Updated FinalizeBlock and Commit methods to include error handling for streaming listeners, checking the StopNodeOnErr flag.
client/v2/go.mod Updated Go version to 1.23.2, added toolchain go1.23.3, and modified dependencies with specific commit hashes.
codec/collections.go Introduced protoCollValueCodec interface, updated CollValue function, and added methods for schema codec retrieval and error handling.
collections/go.mod Updated Go version to 1.23.2, added toolchain go1.23.3, and introduced new dependencies.
collections/indexing.go Enhanced ModuleCodec method to handle conversions to proto.Message types and added error handling.
collections/protocodec/collections.go Restructured package from codec to protocodec, added collValue and collValue2 types for handling protobuf messages.
collections/protocodec/collections_test.go Renamed package from codec_test to protocodec_test, maintaining existing test functions.
go.mod Updated Go version to 1.23.2, added toolchain go1.23.3, modified dependencies and added retract block for certain versions.
indexer/postgres/create_table.go Simplified logging statement in createTable method.
indexer/postgres/enum.go Added a comment for future enhancement regarding enum type checks.
indexer/postgres/listener.go Expanded StartBlock function to include conditional checks for HeaderJSON and HeaderBytes, enhancing error handling.
schema/appdata/async.go Added cleanup process for goroutines in AsyncListener function using a defer statement.
schema/field.go Added validation check for empty Name field in Validate method.
schema/state_object.go Enhanced error messages in Validate and ValidateObjectUpdate methods to include object type names for clarity.
server/v2/cometbft/go.mod Updated Go version to 1.23.2, added toolchain go1.23.3, and maintained existing dependencies.
simapp/go.mod Updated Go version to 1.23.2, added toolchain go1.23.3, and reorganized dependencies.
tests/go.mod Updated Go version to 1.23.2, added toolchain go1.23.3, and modified dependencies.
types/collections.go Updated key codecs to NameableKeyCodec for better clarity and added WithName methods for named instances.
x/accounts/defaults/base/go.mod Updated Go version to 1.23.2, added toolchain go1.23.3, and maintained existing dependencies.
x/accounts/defaults/lockup/go.mod Updated Go version to 1.23.2, added toolchain go1.23.3, and introduced a new indirect dependency.
x/accounts/defaults/multisig/go.mod Updated Go version to 1.23.2, added toolchain go1.23.3, and maintained existing dependencies.
x/accounts/go.mod Updated Go version to 1.23.2, added toolchain go1.23.3, and introduced a new indirect dependency.
x/auth/module.go Added ModuleCodec method to AppModule struct for schema management.
x/authz/go.mod Updated Go version to 1.23.2, added toolchain go1.23.3, and maintained existing dependencies.
x/bank/go.mod Updated Go version to 1.23.2, added toolchain go1.23.3, and added new indirect dependencies.
x/bank/keeper/view.go Updated key names in BaseViewKeeper initialization to enhance clarity.
x/bank/module.go Implemented schema.HasModuleCodec interface in AppModule struct and added ModuleCodec method.
x/circuit/go.mod Updated Go version to 1.23.2, added toolchain go1.23.3, and modified dependency management.
x/circuit/module.go Added ModuleCodec method to AppModule struct for enhanced codec handling.
x/consensus/go.mod Updated Go version to 1.23.2, added toolchain go1.23.3, and modified dependency management.
x/consensus/keeper/keeper.go Introduced Schema field in Keeper struct for schema management and enhanced error handling during initialization.
x/consensus/module.go Added ModuleCodec method to AppModule struct for improved indexing capabilities.
x/distribution/go.mod Updated Go version to 1.23.2, added toolchain go1.23.3, and modified dependency management.
x/distribution/keeper/keeper.go Replaced key codecs with NamedPairKeyCodec and NamedTripleKeyCodec for improved clarity in collections.
x/distribution/module.go Added ModuleCodec method to AppModule struct for enhanced indexing capabilities.
x/epochs/go.mod Updated Go version to 1.23.2, added toolchain go1.23.3, and modified dependency management.
x/epochs/module.go Added ModuleCodec method to AppModule struct for improved codec handling.
x/evidence/go.mod Updated Go version to 1.23.2, added toolchain go1.23.3, and maintained existing dependencies.
x/feegrant/go.mod Updated Go version to 1.23.2, added toolchain go1.23.3, and modified dependency management.
x/gov/go.mod Updated Go version to 1.23.2, added toolchain go1.23.3, and modified dependency management.
x/group/go.mod Updated Go version to 1.23.2, added toolchain go1.23.3, and modified dependency management.
x/mint/go.mod Updated Go version to 1.23.2, added toolchain go1.23.3, and modified dependency management.
x/mint/module.go Added ModuleCodec method to AppModule struct for enhanced codec handling.
x/nft/go.mod Updated Go version to 1.23.2, added toolchain go1.23.3, and maintained existing dependencies.
x/params/go.mod Updated Go version to 1.23.2, added toolchain go1.23.3, and modified dependency management.
x/protocolpool/go.mod Updated Go version to 1.23.2, added toolchain go1.23.3, and modified dependency management.
x/slashing/go.mod Updated Go version to 1.23.2, added toolchain go1.23.3, and modified dependency management.
x/staking/go.mod Updated Go version to 1.23.2, added toolchain go1.23.3, and modified dependency management, including moving cosmossdk.io/schema to direct requirement.
x/staking/keeper/keeper.go Updated key codecs for clarity in Keeper struct initialization.
x/staking/module.go Added ModuleCodec method to AppModule struct for enhanced codec handling.
x/upgrade/go.mod Updated Go version to 1.23.2, added toolchain go1.23.3, and modified dependency management.

Sequence Diagram(s)

sequenceDiagram
    participant A as Application
    participant B as Streaming Manager
    participant C as Listener

    A->>C: Start Listener
    C->>A: Execute Listener
    alt Error Occurs
        C->>B: Check StopNodeOnErr
        alt StopNodeOnErr is true
            B->>A: Shutdown Application
        end
    else No Error
        A->>C: Continue Processing
    end
Loading

Possibly related PRs

Suggested reviewers

  • kocubinski
  • sontrinh16
  • julienrbrt
  • testinginprod
  • akhilkumarpilli

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.

Copy link
Member

@aaronc aaronc left a comment

Choose a reason for hiding this comment

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

Looks like a good direction generally

@@ -77,7 +77,9 @@ func FallbackSchemaCodec[T any]() SchemaCodec[T] {
Kind: kind,
}},
// these can be nil because T maps directly to a schema value for this kind
ToSchemaType: nil,
ToSchemaType: func(t T) (any, error) {
Copy link
Member

Choose a reason for hiding this comment

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

Instead can we just have the nil case be handled properly?

Copy link
Member Author

Choose a reason for hiding this comment

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

Done, it was already there for keys

if c.Name == "" {
return fmt.Errorf("field name cannot be empty, might be missing the named key codec")
}

if !ValidateName(c.Name) {
Copy link
Member

Choose a reason for hiding this comment

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

Does ValidateName not handle this case?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, it does handle it but I thought it was nice to have a more descriptive error for the empty case which would be the most common while implementing

@@ -3,9 +3,12 @@
import (
"bytes"
"fmt"
"reflect"

Check notice

Code scanning / CodeQL

Sensitive package import Note

Certain system packages contain functions which may be a possible source of non-determinism
@facundomedica
Copy link
Member Author

The linter is ignoring nolint for some reason

Copy link
Member

@julienrbrt julienrbrt left a comment

Choose a reason for hiding this comment

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

lgtm but two comments on the error handling in baseapp but approving already
Additionally, could you do the follow-up PR about the replace removal?

baseapp/abci.go Outdated Show resolved Hide resolved
app.logger.Error("ListenFinalizeBlock listening hook failed", "height", req.Height, "err", err)
if app.streamingManager.StopNodeOnErr {
// if StopNodeOnErr is set, we should return the streamErr in order to stop the node
err = streamErr
Copy link
Member

Choose a reason for hiding this comment

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

doesn't this get override line 937? Shouldn't we return the error as the comment says?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is inside a defer, so it will replace the one we are returning

Copy link
Member

Choose a reason for hiding this comment

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

I see yeah, so I suppose we should join it too right?

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: 0

🧹 Outside diff range and nitpick comments (3)
tests/integration/v2/services.go (3)

131-131: Consider adding documentation for BranchService

The BranchService struct would benefit from godoc comments explaining its purpose in integration tests and any specific behaviors or requirements.

Add documentation like:

+// BranchService is a custom implementation of branch.Service for integration tests.
+// It provides execution isolation and gas metering capabilities.
 type BranchService struct{}

Line range hint 132-190: Consider thread safety and error handling improvements

A few suggestions to enhance the router implementation:

  1. The handlers map is not thread-safe. Consider using sync.Map or adding mutex protection if concurrent access is possible.
  2. Consider adding a method to unregister handlers to prevent memory leaks.
  3. The error messages in CanInvoke and Invoke are duplicated.

Here's a suggested improvement:

 type RouterService struct {
+    sync.RWMutex
     handlers map[string]routerHandler
 }

+var errNoHandler = func(typeUrl string) error {
+    return fmt.Errorf("no handler for typeURL %s", typeUrl)
+}

 func (rs *RouterService) RegisterHandler(handler routerHandler, typeUrl string) {
+    rs.Lock()
+    defer rs.Unlock()
     rs.handlers[typeUrl] = handler
 }

+func (rs *RouterService) UnregisterHandler(typeUrl string) {
+    rs.Lock()
+    defer rs.Unlock()
+    delete(rs.handlers, typeUrl)
+}

 func (rs RouterService) CanInvoke(ctx context.Context, typeUrl string) error {
+    rs.RLock()
+    defer rs.RUnlock()
     if rs.handlers[typeUrl] == nil {
-        return fmt.Errorf("no handler for typeURL %s", typeUrl)
+        return errNoHandler(typeUrl)
     }
     return nil
 }

Missing test coverage for RouterService implementation

The RouterService implementation lacks dedicated test coverage. While the service is used in the test setup (auth/app_test.go), there are no direct tests that verify:

  • Handler registration functionality
  • CanInvoke method behavior
  • Error handling for missing handlers
  • Invoke method functionality

Required test cases:

  • Test handler registration and retrieval
  • Test CanInvoke with registered and unregistered handlers
  • Test Invoke with valid and invalid message types
  • Test error scenarios
🔗 Analysis chain

Line range hint 132-190: Verify test coverage for RouterService

Since this is in the integration test package, we should ensure comprehensive test coverage for the RouterService implementation.


Let me check the actual test coverage by examining the test file content.


Let me check for actual test cases that exercise the RouterService functionality.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check for test files covering RouterService
rg -l "RouterService" tests/integration/v2/ | grep "_test.go"

# Check for specific test coverage
ast-grep --pattern 'func Test$_($_, $_) {
  $$$
  RouterService
  $$$
}'

Length of output: 172


Script:

#!/bin/bash
# Check the content of the test file that mentions RouterService
cat tests/integration/v2/auth/app_test.go

# Search for any other tests that might use RouterService
rg -A 5 "RouterService" tests/integration/v2/

Length of output: 9603


Script:

#!/bin/bash
# Look for test functions in auth/app_test.go that might exercise RouterService
rg -A 10 "func Test" tests/integration/v2/auth/app_test.go

# Look for actual usage of bank send handler and query handler
rg -A 5 "MsgSend|AccountNumberRequest" tests/integration/v2/auth/app_test.go

Length of output: 908

📜 Review details

Configuration used: .coderabbit.yml
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between f3cd58f and e320f48.

⛔ Files ignored due to path filters (12)
  • server/v2/cometbft/go.sum is excluded by !**/*.sum
  • simapp/go.sum is excluded by !**/*.sum
  • x/circuit/go.sum is excluded by !**/*.sum
  • x/consensus/go.sum is excluded by !**/*.sum
  • x/epochs/go.sum is excluded by !**/*.sum
  • x/evidence/go.sum is excluded by !**/*.sum
  • x/mint/go.sum is excluded by !**/*.sum
  • x/nft/go.sum is excluded by !**/*.sum
  • x/params/go.sum is excluded by !**/*.sum
  • x/protocolpool/go.sum is excluded by !**/*.sum
  • x/slashing/go.sum is excluded by !**/*.sum
  • x/upgrade/go.sum is excluded by !**/*.sum
📒 Files selected for processing (31)
  • client/v2/go.mod (1 hunks)
  • collections/CHANGELOG.md (1 hunks)
  • go.mod (1 hunks)
  • server/v2/cometbft/go.mod (1 hunks)
  • simapp/go.mod (2 hunks)
  • simapp/v2/go.mod (1 hunks)
  • tests/go.mod (1 hunks)
  • tests/integration/v2/auth/app_test.go (1 hunks)
  • tests/integration/v2/services.go (1 hunks)
  • x/accounts/defaults/base/go.mod (1 hunks)
  • x/accounts/defaults/lockup/go.mod (1 hunks)
  • x/accounts/defaults/multisig/go.mod (1 hunks)
  • x/accounts/go.mod (1 hunks)
  • x/authz/go.mod (1 hunks)
  • x/bank/go.mod (1 hunks)
  • x/circuit/go.mod (3 hunks)
  • x/consensus/go.mod (2 hunks)
  • x/distribution/go.mod (2 hunks)
  • x/distribution/keeper/keeper.go (2 hunks)
  • x/epochs/go.mod (3 hunks)
  • x/evidence/go.mod (2 hunks)
  • x/feegrant/go.mod (1 hunks)
  • x/gov/go.mod (1 hunks)
  • x/group/go.mod (1 hunks)
  • x/mint/go.mod (3 hunks)
  • x/nft/go.mod (2 hunks)
  • x/params/go.mod (2 hunks)
  • x/protocolpool/go.mod (2 hunks)
  • x/slashing/go.mod (2 hunks)
  • x/staking/go.mod (2 hunks)
  • x/upgrade/go.mod (2 hunks)
✅ Files skipped from review due to trivial changes (1)
  • tests/integration/v2/auth/app_test.go
🚧 Files skipped from review as they are similar to previous changes (29)
  • client/v2/go.mod
  • collections/CHANGELOG.md
  • go.mod
  • server/v2/cometbft/go.mod
  • simapp/go.mod
  • simapp/v2/go.mod
  • tests/go.mod
  • x/accounts/defaults/base/go.mod
  • x/accounts/defaults/lockup/go.mod
  • x/accounts/defaults/multisig/go.mod
  • x/accounts/go.mod
  • x/authz/go.mod
  • x/bank/go.mod
  • x/circuit/go.mod
  • x/consensus/go.mod
  • x/distribution/go.mod
  • x/distribution/keeper/keeper.go
  • x/epochs/go.mod
  • x/evidence/go.mod
  • x/feegrant/go.mod
  • x/gov/go.mod
  • x/group/go.mod
  • x/mint/go.mod
  • x/nft/go.mod
  • x/params/go.mod
  • x/protocolpool/go.mod
  • x/slashing/go.mod
  • x/staking/go.mod
  • x/upgrade/go.mod
🧰 Additional context used
📓 Path-based instructions (1)
tests/integration/v2/services.go (2)

Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.


Pattern tests/**/*: "Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull request"

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: 0

🧹 Outside diff range and nitpick comments (4)
tests/integration/v2/services.go (4)

Line range hint 131-163: Enhance error messages and gas consumption description

The implementation is solid, but could benefit from more descriptive error messages and gas consumption reason:

Consider applying these improvements:

-       return errors.New("context is not an integration context")
+       return errors.New("invalid context type: expected integrationContext")

-       return 0, errors.New("context is not an integration context")
+       return 0, errors.New("invalid context type: expected integrationContext")

-       _ = iCtx.gasMeter.Consume(gasUsed, "execute-with-gas-limit")
+       _ = iCtx.gasMeter.Consume(gasUsed, "branch-execution-overhead")

Line range hint 182-186: Add concurrent access protection

The handlers map could be accessed concurrently by multiple goroutines during tests, which might lead to race conditions.

Consider adding mutex protection:

 type RouterService struct {
+       mu       sync.RWMutex
        handlers map[string]routerHandler
 }

Line range hint 188-192: Validate handler function during registration

The RegisterHandler method should validate that the handler function is not nil to prevent potential panics.

 func (rs *RouterService) RegisterHandler(handler routerHandler, typeUrl string) {
+       if handler == nil {
+               panic("handler function cannot be nil")
+       }
        rs.handlers[typeUrl] = handler
 }

Line range hint 194-207: Reduce code duplication in error handling

The error message for missing handlers is duplicated in both CanInvoke and Invoke methods.

Consider extracting the error message to a constant:

+const errNoHandler = "no handler for typeURL %s"

 func (rs RouterService) CanInvoke(ctx context.Context, typeUrl string) error {
        if rs.handlers[typeUrl] == nil {
-               return fmt.Errorf("no handler for typeURL %s", typeUrl)
+               return fmt.Errorf(errNoHandler, typeUrl)
        }
        return nil
 }

 func (rs RouterService) Invoke(ctx context.Context, req transaction.Msg) (transaction.Msg, error) {
        typeUrl := msgTypeURL(req)
        if rs.handlers[typeUrl] == nil {
-               return nil, fmt.Errorf("no handler for typeURL %s", typeUrl)
+               return nil, fmt.Errorf(errNoHandler, typeUrl)
        }
        return rs.handlers[typeUrl](ctx, req)
 }
📜 Review details

Configuration used: .coderabbit.yml
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between f3cd58f and e320f48.

⛔ Files ignored due to path filters (12)
  • server/v2/cometbft/go.sum is excluded by !**/*.sum
  • simapp/go.sum is excluded by !**/*.sum
  • x/circuit/go.sum is excluded by !**/*.sum
  • x/consensus/go.sum is excluded by !**/*.sum
  • x/epochs/go.sum is excluded by !**/*.sum
  • x/evidence/go.sum is excluded by !**/*.sum
  • x/mint/go.sum is excluded by !**/*.sum
  • x/nft/go.sum is excluded by !**/*.sum
  • x/params/go.sum is excluded by !**/*.sum
  • x/protocolpool/go.sum is excluded by !**/*.sum
  • x/slashing/go.sum is excluded by !**/*.sum
  • x/upgrade/go.sum is excluded by !**/*.sum
📒 Files selected for processing (31)
  • client/v2/go.mod (1 hunks)
  • collections/CHANGELOG.md (1 hunks)
  • go.mod (1 hunks)
  • server/v2/cometbft/go.mod (1 hunks)
  • simapp/go.mod (2 hunks)
  • simapp/v2/go.mod (1 hunks)
  • tests/go.mod (1 hunks)
  • tests/integration/v2/auth/app_test.go (1 hunks)
  • tests/integration/v2/services.go (1 hunks)
  • x/accounts/defaults/base/go.mod (1 hunks)
  • x/accounts/defaults/lockup/go.mod (1 hunks)
  • x/accounts/defaults/multisig/go.mod (1 hunks)
  • x/accounts/go.mod (1 hunks)
  • x/authz/go.mod (1 hunks)
  • x/bank/go.mod (1 hunks)
  • x/circuit/go.mod (3 hunks)
  • x/consensus/go.mod (2 hunks)
  • x/distribution/go.mod (2 hunks)
  • x/distribution/keeper/keeper.go (2 hunks)
  • x/epochs/go.mod (3 hunks)
  • x/evidence/go.mod (2 hunks)
  • x/feegrant/go.mod (1 hunks)
  • x/gov/go.mod (1 hunks)
  • x/group/go.mod (1 hunks)
  • x/mint/go.mod (3 hunks)
  • x/nft/go.mod (2 hunks)
  • x/params/go.mod (2 hunks)
  • x/protocolpool/go.mod (2 hunks)
  • x/slashing/go.mod (2 hunks)
  • x/staking/go.mod (2 hunks)
  • x/upgrade/go.mod (2 hunks)
✅ Files skipped from review due to trivial changes (1)
  • tests/integration/v2/auth/app_test.go
🚧 Files skipped from review as they are similar to previous changes (29)
  • client/v2/go.mod
  • collections/CHANGELOG.md
  • go.mod
  • server/v2/cometbft/go.mod
  • simapp/go.mod
  • simapp/v2/go.mod
  • tests/go.mod
  • x/accounts/defaults/base/go.mod
  • x/accounts/defaults/lockup/go.mod
  • x/accounts/defaults/multisig/go.mod
  • x/accounts/go.mod
  • x/authz/go.mod
  • x/bank/go.mod
  • x/circuit/go.mod
  • x/consensus/go.mod
  • x/distribution/go.mod
  • x/distribution/keeper/keeper.go
  • x/epochs/go.mod
  • x/evidence/go.mod
  • x/feegrant/go.mod
  • x/gov/go.mod
  • x/group/go.mod
  • x/mint/go.mod
  • x/nft/go.mod
  • x/params/go.mod
  • x/protocolpool/go.mod
  • x/slashing/go.mod
  • x/staking/go.mod
  • x/upgrade/go.mod
🧰 Additional context used
📓 Path-based instructions (1)
tests/integration/v2/services.go (2)

Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.


Pattern tests/**/*: "Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull request"

🔇 Additional comments (1)
tests/integration/v2/services.go (1)

Line range hint 1-207: Implementation provides good test coverage with room for improvement

The integration test services are well-implemented with good coverage of router and branch functionality. The suggested improvements around concurrent access protection, error handling, and validation will enhance the robustness of the test infrastructure.

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: 0

🧹 Outside diff range and nitpick comments (5)
tests/integration/v2/services.go (5)

131-131: LGTM! Consider enhancing error messages

The BranchService implementation is well-structured with proper context validation. However, the error message could be more descriptive to aid in debugging.

Consider enhancing the error message:

-       return errors.New("context is not an integration context")
+       return errors.New("invalid context type: expected *integrationContext")

Also applies to: 132-146


Line range hint 147-166: Consider documenting gas consumption reason

The ExecuteWithGasLimit implementation correctly handles gas metering, but the hardcoded gas consumption reason could be more meaningful.

Consider making the gas consumption reason more descriptive:

-       _ = iCtx.gasMeter.Consume(gasUsed, "execute-with-gas-limit")
+       _ = iCtx.gasMeter.Consume(gasUsed, "branch-execution-overhead")

Line range hint 176-182: Consider adding thread safety to handler registration

The RouterService implementation is clean and well-structured. However, the handlers map could face race conditions in concurrent scenarios.

Consider adding mutex protection:

 type RouterService struct {
+       mu       sync.RWMutex
        handlers map[string]routerHandler
 }

 func (rs *RouterService) RegisterHandler(handler routerHandler, typeUrl string) {
+       rs.mu.Lock()
+       defer rs.mu.Unlock()
        rs.handlers[typeUrl] = handler
 }

Also applies to: 183-187


Line range hint 189-204: Enhance error handling consistency

The error handling in CanInvoke and Invoke methods is good but contains duplicate error message construction.

Consider extracting the error message to a constant:

+const errNoHandler = "no handler for typeURL %s"

 func (rs RouterService) CanInvoke(ctx context.Context, typeUrl string) error {
        if rs.handlers[typeUrl] == nil {
-               return fmt.Errorf("no handler for typeURL %s", typeUrl)
+               return fmt.Errorf(errNoHandler, typeUrl)
        }
        return nil
 }

 func (rs RouterService) Invoke(ctx context.Context, req transaction.Msg) (transaction.Msg, error) {
        typeUrl := msgTypeURL(req)
        if rs.handlers[typeUrl] == nil {
-               return nil, fmt.Errorf("no handler for typeURL %s", typeUrl)
+               return nil, fmt.Errorf(errNoHandler, typeUrl)
        }
        return rs.handlers[typeUrl](ctx, req)
 }

Missing test coverage for integration services

The BranchService and RouterService in the integration test package lack direct test coverage. While these services are used as dependencies in other test scenarios (e.g., in auth/app_test.go), they should have their own dedicated test cases to verify their behavior:

  • BranchService.Execute and ExecuteWithGasLimit methods need test coverage
  • RouterService.RegisterHandler, CanInvoke, and Invoke methods need test coverage
  • Edge cases like invalid contexts and handler registration should be tested

Please add test cases for these services in tests/integration/v2/services_test.go.

🔗 Analysis chain

Line range hint 131-204: Verify test coverage for router and branch services

As this is an integration test file, we should ensure proper test coverage for the new services.

Let's check for existing tests:

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for test coverage of the router and branch services
# Look for test files that might cover these services

# Search for test files that might cover these services
echo "Searching for relevant test files..."
fd -e go -e _test.go . | grep -i "router\|branch"

# Search for specific test cases
echo "Searching for router service tests..."
rg -l "TestRouter" -g "*.go"

echo "Searching for branch service tests..."
rg -l "TestBranch" -g "*.go"

Length of output: 1568


Script:

#!/bin/bash
# Let's examine the content of the test files to verify coverage

# Check core_branch_service_test.go for BranchService test coverage
echo "=== Checking BranchService test coverage ==="
rg "BranchService" "server/v2/stf/core_branch_service_test.go" -C 5

echo -e "\n=== Checking RouterService test coverage ==="
# Check stf_router_test.go for RouterService test coverage
rg "RouterService" "server/v2/stf/stf_router_test.go" -C 5

Length of output: 947


Script:

#!/bin/bash
# Let's check for test coverage in integration tests directory specifically

echo "=== Checking integration tests for RouterService ==="
rg -A 10 "func Test.*Router" "tests/integration/v2/"

echo -e "\n=== Checking integration tests for BranchService ==="
rg -A 10 "func Test.*Branch" "tests/integration/v2/"

# Let's also check if these services are used in test scenarios
echo -e "\n=== Checking usage of services in tests ==="
rg "NewRouterService|RouterService|BranchService" "tests/integration/v2/" -C 2

Length of output: 6556

📜 Review details

Configuration used: .coderabbit.yml
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between f3cd58f and e320f48.

⛔ Files ignored due to path filters (12)
  • server/v2/cometbft/go.sum is excluded by !**/*.sum
  • simapp/go.sum is excluded by !**/*.sum
  • x/circuit/go.sum is excluded by !**/*.sum
  • x/consensus/go.sum is excluded by !**/*.sum
  • x/epochs/go.sum is excluded by !**/*.sum
  • x/evidence/go.sum is excluded by !**/*.sum
  • x/mint/go.sum is excluded by !**/*.sum
  • x/nft/go.sum is excluded by !**/*.sum
  • x/params/go.sum is excluded by !**/*.sum
  • x/protocolpool/go.sum is excluded by !**/*.sum
  • x/slashing/go.sum is excluded by !**/*.sum
  • x/upgrade/go.sum is excluded by !**/*.sum
📒 Files selected for processing (31)
  • client/v2/go.mod (1 hunks)
  • collections/CHANGELOG.md (1 hunks)
  • go.mod (1 hunks)
  • server/v2/cometbft/go.mod (1 hunks)
  • simapp/go.mod (2 hunks)
  • simapp/v2/go.mod (1 hunks)
  • tests/go.mod (1 hunks)
  • tests/integration/v2/auth/app_test.go (1 hunks)
  • tests/integration/v2/services.go (1 hunks)
  • x/accounts/defaults/base/go.mod (1 hunks)
  • x/accounts/defaults/lockup/go.mod (1 hunks)
  • x/accounts/defaults/multisig/go.mod (1 hunks)
  • x/accounts/go.mod (1 hunks)
  • x/authz/go.mod (1 hunks)
  • x/bank/go.mod (1 hunks)
  • x/circuit/go.mod (3 hunks)
  • x/consensus/go.mod (2 hunks)
  • x/distribution/go.mod (2 hunks)
  • x/distribution/keeper/keeper.go (2 hunks)
  • x/epochs/go.mod (3 hunks)
  • x/evidence/go.mod (2 hunks)
  • x/feegrant/go.mod (1 hunks)
  • x/gov/go.mod (1 hunks)
  • x/group/go.mod (1 hunks)
  • x/mint/go.mod (3 hunks)
  • x/nft/go.mod (2 hunks)
  • x/params/go.mod (2 hunks)
  • x/protocolpool/go.mod (2 hunks)
  • x/slashing/go.mod (2 hunks)
  • x/staking/go.mod (2 hunks)
  • x/upgrade/go.mod (2 hunks)
✅ Files skipped from review due to trivial changes (1)
  • tests/integration/v2/auth/app_test.go
🚧 Files skipped from review as they are similar to previous changes (28)
  • client/v2/go.mod
  • collections/CHANGELOG.md
  • go.mod
  • server/v2/cometbft/go.mod
  • simapp/v2/go.mod
  • tests/go.mod
  • x/accounts/defaults/base/go.mod
  • x/accounts/defaults/lockup/go.mod
  • x/accounts/defaults/multisig/go.mod
  • x/accounts/go.mod
  • x/authz/go.mod
  • x/bank/go.mod
  • x/circuit/go.mod
  • x/consensus/go.mod
  • x/distribution/go.mod
  • x/distribution/keeper/keeper.go
  • x/epochs/go.mod
  • x/evidence/go.mod
  • x/feegrant/go.mod
  • x/gov/go.mod
  • x/group/go.mod
  • x/mint/go.mod
  • x/nft/go.mod
  • x/params/go.mod
  • x/protocolpool/go.mod
  • x/slashing/go.mod
  • x/staking/go.mod
  • x/upgrade/go.mod
🧰 Additional context used
📓 Path-based instructions (1)
tests/integration/v2/services.go (2)

Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.


Pattern tests/**/*: "Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull request"

🔇 Additional comments (2)
simapp/go.mod (2)

283-284: Local module paths specified in replace directives are not found

The verification revealed that both referenced local module paths are missing:

  • ../indexer/postgres directory does not exist
  • ../schema directory does not exist

This indicates a setup issue that needs to be addressed before the changes can work properly.


3-3: ⚠️ Potential issue

Invalid Go version specified

The Go version 1.23.3 specified in the file does not exist yet. The latest stable version of Go is 1.22.1 (as of March 2024).

Apply this diff to fix the version:

-go 1.23.3
+go 1.22.1

Likely invalid or redundant comment.

@facundomedica facundomedica added this pull request to the merge queue Nov 28, 2024
Merged via the queue into main with commit bd76b47 Nov 28, 2024
93 of 95 checks passed
@facundomedica facundomedica deleted the facu/staking-indexer1 branch November 28, 2024 09:52
mergify bot pushed a commit that referenced this pull request Nov 28, 2024
(cherry picked from commit bd76b47)

# Conflicts:
#	collections/CHANGELOG.md
#	collections/go.mod
#	collections/go.sum
#	collections/indexing.go
#	collections/protocodec/collections.go
#	collections/protocodec/collections_test.go
#	indexer/postgres/create_table.go
#	indexer/postgres/enum.go
#	indexer/postgres/listener.go
#	indexer/postgres/tests/testdata/init_schema.txt
#	indexer/postgres/tests/testdata/init_schema_no_retain_delete.txt
#	math/legacy_dec_test.go
#	schema/appdata/async.go
#	schema/field.go
#	schema/field_test.go
#	schema/state_object.go
#	schema/state_object_test.go
#	server/v2/cometbft/go.mod
#	server/v2/cometbft/go.sum
#	simapp/go.mod
#	simapp/go.sum
#	store/cachekv/branch_bench_test.go
#	tests/integration/v2/auth/app_test.go
#	tests/integration/v2/services.go
#	x/bank/keeper/view.go
#	x/circuit/go.mod
#	x/circuit/go.sum
#	x/consensus/go.mod
#	x/consensus/go.sum
#	x/distribution/go.mod
#	x/epochs/go.mod
#	x/epochs/go.sum
#	x/evidence/go.mod
#	x/evidence/go.sum
#	x/mint/go.mod
#	x/mint/go.sum
#	x/nft/go.mod
#	x/nft/go.sum
#	x/params/go.mod
#	x/params/go.sum
#	x/protocolpool/go.mod
#	x/protocolpool/go.sum
#	x/slashing/go.mod
#	x/slashing/go.sum
#	x/staking/go.mod
#	x/upgrade/go.mod
#	x/upgrade/go.sum
github-merge-queue bot pushed a commit that referenced this pull request Nov 28, 2024
julienrbrt added a commit that referenced this pull request Nov 28, 2024
…2544) (#22676)

Co-authored-by: Facundo Medica <14063057+facundomedica@users.noreply.github.com>
Co-authored-by: Facundo <facundomedica@gmail.com>
Co-authored-by: Julien Robert <julien@rbrt.fr>
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.