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

chore(consensus): add cometInfo to consensus #20615

Merged
merged 13 commits into from
Jun 13, 2024
Merged

Conversation

tac0turtle
Copy link
Member

@tac0turtle tac0turtle commented Jun 10, 2024

Description

Upstream changes from server/v2. This pr focuses on the consensus module and getting the changes needed for it to work with v2


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

    • Introduced comet information management, allowing users to set and retrieve comet details.
    • Added new RPC methods to query and set comet information.
  • Tests

    • Implemented test cases to validate comet information handling, ensuring robust error handling and validation.
  • Compatibility

    • Updated protocol files to be compatible with cosmos-sdk version 0.52.

Copy link
Contributor

coderabbitai bot commented Jun 10, 2024

Walkthrough

Walkthrough

Recent changes in the x/consensus module introduce a mechanism for storing and querying comet information. This includes updates to the Keeper struct in the Go code and new protobuf definitions for RPC functions and message types pertaining to comet information. The modifications facilitate both setting and retrieving comet details within the consensus protocol.

Changes

File(s) Change Summary
x/consensus/keeper/keeper.go Added cometInfo field and methods SetCometInfo and CometInfo to manage comet data.
x/consensus/proto/cosmos/consensus/v1/query.proto Introduced new RPC functions and message types for querying comet information.

Sequence Diagram(s)

sequenceDiagram
    participant Client
    participant ConsensusModule
    participant Keeper

    Client->>+ConsensusModule: QueryCometInfo(QueryCometInfoRequest)
    ConsensusModule->>+Keeper: CometInfo()
    Keeper-->>-ConsensusModule: QueryCometInfoResponse
    ConsensusModule-->>-Client: CometInfo(comet_info)
    
    Client->>+ConsensusModule: GetCometInfo(QueryGetCometInfoRequest)
    ConsensusModule->>+Keeper: CometInfo()
    Keeper-->>-ConsensusModule: QueryGetCometInfoResponse
    ConsensusModule-->>-Client: CometInfo(comet_info)
Loading

This sequence diagram visualizes the interaction for querying comet information. The client sends requests to the ConsensusModule, which then interacts with the Keeper to fetch the required data before returning it back to the client.


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>.
    • 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 generate interesting stats about this repository and render them as a table.
    • @coderabbitai show all the console.log statements in this repository.
    • @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 as 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.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

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.

api/cosmos/consensus/v1/consensus.pulsar.go Dismissed Show dismissed Hide dismissed
api/cosmos/consensus/v1/consensus.pulsar.go Dismissed Show dismissed Hide dismissed
api/cosmos/consensus/v1/tx.pulsar.go Fixed Show fixed Hide fixed
api/cosmos/consensus/v1/tx.pulsar.go Fixed Show fixed Hide fixed
api/cosmos/consensus/v1/tx.pulsar.go Fixed Show fixed Hide fixed
api/cosmos/consensus/v1/tx.pulsar.go Fixed Show fixed Hide fixed
@tac0turtle tac0turtle marked this pull request as ready for review June 13, 2024 11:01
@tac0turtle tac0turtle requested a review from a team as a code owner June 13, 2024 11:01
api/cosmos/consensus/v1/query.pulsar.go Fixed Show fixed Hide fixed
api/cosmos/consensus/v1/query.pulsar.go Fixed Show fixed Hide fixed
api/cosmos/consensus/v1/query.pulsar.go Fixed Show fixed Hide fixed
api/cosmos/consensus/v1/query.pulsar.go Fixed Show fixed Hide fixed
api/cosmos/consensus/v1/tx.pulsar.go Dismissed Show dismissed Hide dismissed
api/cosmos/consensus/v1/tx.pulsar.go Dismissed Show dismissed Hide dismissed
api/cosmos/consensus/v1/tx.pulsar.go Dismissed Show dismissed Hide dismissed
api/cosmos/consensus/v1/tx.pulsar.go Dismissed Show dismissed Hide dismissed
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: 5

Outside diff range and nitpick comments (1)
api/cosmos/consensus/v1/consensus.pulsar.go (1)

Line range hint 86-133: Ensure that methods in fastReflection_CometInfo handle null cases appropriately, especially in methods like ProtoReflect and New, to prevent runtime panics.

- return (*fastReflection_CometInfo)(x)
+ if x == nil { return nil }
+ return (*fastReflection_CometInfo)(x)
Review details

Configuration used: .coderabbit.yml
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 8d205af and 445c177.

Files ignored due to path filters (10)
  • api/cosmos/consensus/v1/query_grpc.pb.go is excluded by !**/*.pb.go
  • api/cosmos/consensus/v1/tx_grpc.pb.go is excluded by !**/*.pb.go
  • cosmossdk.io/server/v2/streaming/grpc.pb.go is excluded by !**/*.pb.go
  • cosmossdk.io/store/snapshots/types/snapshot.pb.go is excluded by !**/*.pb.go
  • cosmossdk.io/store/streaming/abci/grpc.pb.go is excluded by !**/*.pb.go
  • cosmossdk.io/store/types/commit_info.pb.go is excluded by !**/*.pb.go
  • cosmossdk.io/store/types/listening.pb.go is excluded by !**/*.pb.go
  • x/consensus/types/consensus.pb.go is excluded by !**/*.pb.go
  • x/consensus/types/query.pb.go is excluded by !**/*.pb.go
  • x/consensus/types/tx.pb.go is excluded by !**/*.pb.go
Files selected for processing (9)
  • api/cosmos/consensus/v1/consensus.pulsar.go (25 hunks)
  • api/cosmos/consensus/v1/query.pulsar.go (12 hunks)
  • api/cosmos/consensus/v1/tx.pulsar.go (6 hunks)
  • x/consensus/CHANGELOG.md (1 hunks)
  • x/consensus/keeper/keeper.go (5 hunks)
  • x/consensus/keeper/keeper_test.go (2 hunks)
  • x/consensus/proto/cosmos/consensus/v1/consensus.proto (1 hunks)
  • x/consensus/proto/cosmos/consensus/v1/query.proto (2 hunks)
  • x/consensus/proto/cosmos/consensus/v1/tx.proto (3 hunks)
Files not summarized due to errors (3)
  • api/cosmos/consensus/v1/consensus.pulsar.go: Error: Message exceeds token limit
  • api/cosmos/consensus/v1/query.pulsar.go: Error: Message exceeds token limit
  • api/cosmos/consensus/v1/tx.pulsar.go: Error: Message exceeds token limit
Files skipped from review due to trivial changes (1)
  • x/consensus/CHANGELOG.md
Additional context used
Path-based instructions (5)
x/consensus/keeper/keeper.go (1)

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

x/consensus/keeper/keeper_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"

api/cosmos/consensus/v1/consensus.pulsar.go (1)

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

api/cosmos/consensus/v1/query.pulsar.go (1)

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

api/cosmos/consensus/v1/tx.pulsar.go (1)

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

Additional comments not posted (19)
x/consensus/proto/cosmos/consensus/v1/consensus.proto (1)

9-14: The CometInfo message structure appears well-defined for handling comet-related data.

x/consensus/proto/cosmos/consensus/v1/query.proto (2)

18-28: The new CometInfo RPC method and associated request/response messages are correctly defined and follow the expected structure for querying comet information.


7-7: The import for consensus.proto is necessary for the new RPC method. Ensure it is correctly specified.

Verification successful

The import path for consensus.proto in query.proto is correctly specified as cosmos/consensus/v1/consensus.proto.

Scripts executed

The following scripts were executed for the analysis:


Script:

#!/bin/bash
# Description: Verify the import path for consensus.proto in the query.proto file.

# Test: Search for the import path in the repository. Expect: Path exists.
fd --type file 'consensus.proto' | grep 'cosmos/consensus/v1'

Length of output: 113

x/consensus/proto/cosmos/consensus/v1/tx.proto (1)

Line range hint 23-73: The SetCometInfo RPC method and associated messages are correctly defined and follow the expected structure for setting comet information.

x/consensus/keeper/keeper.go (3)

4-4: The import of the bytes package is essential for handling byte comparisons in the keeper methods.


30-31: The cometInfo field addition and the SetCometInfo method are correctly implemented to handle comet information. Ensure the method handles all edge cases and error conditions adequately.

Also applies to: 108-125


127-134: The CometInfo method for querying comet information is correctly implemented. It appears robust and follows the expected error handling protocols.

x/consensus/keeper/keeper_test.go (2)

Line range hint 7-656: The test suite is comprehensive and well-structured, covering a variety of scenarios including edge cases.


608-656: LGTM! Ensure the test environment is correctly set up for all scenarios.

Verification successful

The SetupTest method correctly initializes all necessary components for the tests in TestSetCometInfo.

  • The method sets up the key, context, encoding configuration, environment, authority, keeper, and query client.
  • It also configures the consensus parameters based on the enabledFeatures flag.
Scripts executed

The following scripts were executed for the analysis:


Script:

#!/bin/bash
# Description: Verify that the test environment setup in `SetupTest` correctly initializes all necessary components for the `TestSetCometInfo`.

# Test: Search for the `SetupTest` method usage. Expect: Proper initialization for all test cases.
rg --type go $'SetupTest' x/consensus/keeper/keeper_test.go

Length of output: 219



Script:

#!/bin/bash
# Description: Locate and analyze the definition of the `SetupTest` method to verify proper initialization.

# Find the definition of the `SetupTest` method in the `keeper_test.go` file.
rg --type go --pcre2 --context 5 $'func \\(s \\*KeeperTestSuite\\) SetupTest\\(enabledFeatures bool\\)' x/consensus/keeper/keeper_test.go

Length of output: 660



Script:

#!/bin/bash
# Description: Retrieve the full implementation of the `SetupTest` method to verify proper initialization.

# Get the complete `SetupTest` method implementation from `keeper_test.go`.
rg --type go --pcre2 --context 50 $'func \\(s \\*KeeperTestSuite\\) SetupTest\\(enabledFeatures bool\\)' x/consensus/keeper/keeper_test.go

Length of output: 3386

api/cosmos/consensus/v1/consensus.pulsar.go (1)

77-81: Validate that the field names used in md_CometInfo match the actual protobuf definitions to prevent runtime errors.

Verification successful

The field names used in md_CometInfo match the actual protobuf definitions in the CometInfo message.

  • evidence
  • validators_hash
  • proposer_address
  • last_commit

These fields are correctly defined in x/consensus/proto/cosmos/consensus/v1/consensus.proto.

Scripts executed

The following scripts were executed for the analysis:


Script:

#!/bin/bash
# Description: Ensure field names in `md_CometInfo` match protobuf definitions.

# Test: Search for field names in the protobuf file. Expect: Match with the names used here.
rg --type proto "field name" cosmos/consensus/v1/consensus.proto

Length of output: 96



Script:

#!/bin/bash
# Description: Ensure field names in `md_CometInfo` match protobuf definitions.

# Search for the CometInfo message definition and its fields in the protobuf file.
rg --pcre2 --context 5 'message CometInfo' cosmos/consensus/v1/consensus.proto

Length of output: 156



Script:

#!/bin/bash
# Description: Locate the correct protobuf file and ensure field names in `md_CometInfo` match protobuf definitions.

# Locate the .proto file
proto_file=$(fd --type f --extension proto | grep 'consensus')

# Search for the CometInfo message definition and its fields in the located protobuf file.
if [ -n "$proto_file" ]; then
  rg --pcre2 --context 5 'message CometInfo' "$proto_file"
else
  echo "Protobuf file for consensus not found."
fi

Length of output: 1003



Script:

#!/bin/bash
# Description: Locate the correct protobuf file and ensure field names in `md_CometInfo` match protobuf definitions.

# Locate the .proto files
proto_files=$(fd --type f --extension proto | grep 'consensus')

# Iterate over each located protobuf file and search for the CometInfo message and its fields.
for proto_file in $proto_files; do
  echo "Searching in $proto_file"
  rg --pcre2 --context 5 'message CometInfo' "$proto_file"
done

Length of output: 1794

api/cosmos/consensus/v1/query.pulsar.go (6)

17-24: Ensure proper initialization of message descriptors.

The initialization of md_QueryCometInfoRequest within the init function is correctly handled, ensuring that the message descriptor is properly set up for use in the protobuf reflection.


26-72: Review the implementation of fastReflection_QueryCometInfoRequest for message reflection.

The fastReflection_QueryCometInfoRequest struct correctly implements the protoreflect.Message interface, providing optimized reflection methods for the QueryCometInfoRequest type. This is crucial for efficient handling of protobuf messages in a high-performance environment like blockchain consensus operations.


374-382: Check the initialization of QueryCometInfoResponse message descriptor.

Similar to the request counterpart, the initialization of md_QueryCometInfoResponse is correctly done in the init function, ensuring that the message descriptor for QueryCometInfoResponse is ready for use.


384-430: Evaluate the implementation of fastReflection_QueryCometInfoResponse.

The fastReflection_QueryCometInfoResponse struct, like its request counterpart, properly implements the protoreflect.Message interface. This implementation ensures that the response type can be efficiently handled in protobuf operations, which is essential for the performance of the consensus module.


Line range hint 807-834: Review the initialization of QueryParamsRequest and QueryParamsResponse.

The initialization logic for QueryParamsRequest and QueryParamsResponse is correctly placed in the init function. This setup ensures that these types are properly registered and available for protobuf operations, aligning with the standard practices for protobuf services in Go.


Line range hint 836-1202: Assess the completeness and correctness of message handling for QueryParamsRequest and QueryParamsResponse.

Both QueryParamsRequest and QueryParamsResponse are implemented with comprehensive handling of their respective message states, size caches, and unknown fields. This thorough implementation is crucial for the robust processing of consensus parameters queries within the Cosmos SDK.

api/cosmos/consensus/v1/tx.pulsar.go (3)

5-5: Imports and package declaration are correct and standard for protobuf-generated files.

Also applies to: 1270-1270


2532-2532: Message type definitions are correctly structured according to protobuf standards.

Also applies to: 2538-2548


2706-2706: Service definitions are correctly implemented with appropriate request and response types.

Comment on lines +18 to +65
type _CometInfo_1_list struct {
list *[]*v1.Misbehavior
}

func (x *_CometInfo_1_list) Len() int {
if x.list == nil {
return 0
}
return len(*x.list)
}

func (x *_CometInfo_1_list) Get(i int) protoreflect.Value {
return protoreflect.ValueOfMessage((*x.list)[i].ProtoReflect())
}

func (x *_CometInfo_1_list) Set(i int, value protoreflect.Value) {
valueUnwrapped := value.Message()
concreteValue := valueUnwrapped.Interface().(*v1.Misbehavior)
(*x.list)[i] = concreteValue
}

func (x *_CometInfo_1_list) Append(value protoreflect.Value) {
valueUnwrapped := value.Message()
concreteValue := valueUnwrapped.Interface().(*v1.Misbehavior)
*x.list = append(*x.list, concreteValue)
}

func (x *_CometInfo_1_list) AppendMutable() protoreflect.Value {
v := new(v1.Misbehavior)
*x.list = append(*x.list, v)
return protoreflect.ValueOfMessage(v.ProtoReflect())
}

func (x *_CometInfo_1_list) Truncate(n int) {
for i := n; i < len(*x.list); i++ {
(*x.list)[i] = nil
}
*x.list = (*x.list)[:n]
}

func (x *_CometInfo_1_list) NewElement() protoreflect.Value {
v := new(v1.Misbehavior)
return protoreflect.ValueOfMessage(v.ProtoReflect())
}

func (x *_CometInfo_1_list) IsValid() bool {
return x.list != nil
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Ensure that the _CometInfo_1_list type and its methods are well-documented, especially since this is a generated file and clarity is crucial for future maintainers.

default:
if fd.IsExtension() {
panic(fmt.Errorf("proto3 declared messages do not support extensions: cosmos.consensus.v1.ConsensusMsgParams"))
panic(fmt.Errorf("proto3 declared messages do not support extensions: cosmos.consensus.v1.CometInfo"))
Copy link
Contributor

Choose a reason for hiding this comment

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

Replace panic with proper error handling to improve the robustness of the code. Crashing the application due to a non-critical error should be avoided.

- panic(fmt.Errorf("proto3 declared messages do not support extensions: cosmos.consensus.v1.CometInfo"))
+ return fmt.Errorf("proto3 declared messages do not support extensions: cosmos.consensus.v1.CometInfo")

Also applies to: 222-222, 224-224, 318-318, 320-320, 341-341, 343-343, 353-353

@@ -2,7 +2,7 @@
package consensusv1

import (
v1 "buf.build/gen/go/cometbft/cometbft/protocolbuffers/go/cometbft/types/v1"
v1 "buf.build/gen/go/cometbft/cometbft/protocolbuffers/go/cometbft/abci/v1"
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider organizing imports to adhere to the Go style guide; group standard library imports separately from third-party imports.

syntax = "proto3";
package cosmos.consensus.v1;

import "cometbft/types/v1/params.proto";
import "cometbft/abci/v1/types.proto";
Copy link
Contributor

Choose a reason for hiding this comment

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

Tip

Codebase Verification

The import path cometbft/abci/v1/types.proto in consensus.proto is incorrect or the file is missing from the repository. Please verify the correct path or ensure that the required file is added to the repository.

  • File: x/consensus/proto/cosmos/consensus/v1/consensus.proto
  • Line: 5
Analysis chain

Ensure the import path is correctly specified and accessible.

Scripts executed

The following scripts were executed for the analysis:


Script:

#!/bin/bash
# Description: Verify the import path for types.proto in the consensus.proto file.

# Test: Search for the import path in the repository. Expect: Path exists.
fd --type file 'types.proto' | grep 'cometbft/abci/v1'

Length of output: 52



Script:

#!/bin/bash
# Description: Search for types.proto in the entire repository to determine its location.

# Test: Search for the types.proto file in the repository. Expect: Path exists.
fd --type file 'types.proto'

Length of output: 221



Script:

#!/bin/bash
# Description: Search for directories or files related to cometbft in the repository.

# Test: Search for any directories or files related to cometbft. Expect: Path exists.
fd --type d 'cometbft' || fd --type f 'cometbft'

Length of output: 42

@@ -6,6 +6,7 @@ import "amino/amino.proto";
import "cosmos_proto/cosmos.proto";
import "cosmos/msg/v1/msg.proto";
import "cometbft/types/v1/params.proto";
import "cometbft/abci/v1/types.proto";
Copy link
Contributor

Choose a reason for hiding this comment

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

Tip

Codebase Verification

The types.proto file is not located in the cometbft/abci/v1/ directory as specified in the import statement. Instead, the types.proto files are found in different directories, none of which match the expected path. This indicates that the import path in tx.proto is incorrect and needs to be updated to reflect the correct location of the required types.proto.

  • File: x/consensus/proto/cosmos/consensus/v1/tx.proto
  • Line: 9
  • Current Import: import "cometbft/abci/v1/types.proto";

Please update the import path to the correct location based on the actual file structure.

Analysis chain

Ensure the import path for types.proto is correctly specified and accessible.

Scripts executed

The following scripts were executed for the analysis:


Script:

#!/bin/bash
# Description: Verify the import path for types.proto in the tx.proto file.

# Test: Search for the import path in the repository. Expect: Path exists.
fd --type file 'types.proto' | grep 'cometbft/abci/v1'

Length of output: 52



Script:

#!/bin/bash
# Description: Verify the existence of types.proto in the repository and identify its correct path.

# Test: Search for any occurrences of types.proto in the repository.
fd --type file 'types.proto'

Length of output: 221



Script:

#!/bin/bash
# Description: Verify the content of the identified types.proto files to check for the correct import path.

# Test: Display the content of the identified types.proto files.
cat proto/cosmos/base/tendermint/v1beta1/types.proto
cat x/circuit/proto/cosmos/circuit/v1/types.proto
cat x/group/proto/cosmos/group/v1/types.proto
cat x/protocolpool/proto/cosmos/protocolpool/v1/types.proto

Length of output: 19813

Copy link
Contributor

@hieuvubk hieuvubk left a comment

Choose a reason for hiding this comment

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

LGTM

return &types.MsgSetCometInfoResponse{}, nil
}

func (k Keeper) CometInfo(ctx context.Context, _ *types.QueryCometInfoRequest) (*types.QueryCometInfoResponse, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

would love to rename it to GetCometInfo

Copy link
Member Author

Choose a reason for hiding this comment

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

on it

@tac0turtle tac0turtle added this pull request to the merge queue Jun 13, 2024
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)
x/consensus/proto/cosmos/consensus/v1/query.proto (2)

18-20: The addition of the GetCometInfo RPC method enhances the module's functionality by allowing querying of comet info. Ensure this new method is documented and tested.


22-28: The message types for the new RPC methods are well-defined. Consider adding more detailed comments to the fields within the QueryGetCometInfoResponse message for better clarity.

x/consensus/keeper/keeper.go (1)

127-134: The GetCometInfo method is well implemented. Consider adding error handling specifics in the documentation to inform users about possible internal errors.

Review details

Configuration used: .coderabbit.yml
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 445c177 and 314c3a4.

Files ignored due to path filters (2)
  • api/cosmos/consensus/v1/query_grpc.pb.go is excluded by !**/*.pb.go
  • x/consensus/types/query.pb.go is excluded by !**/*.pb.go
Files selected for processing (3)
  • api/cosmos/consensus/v1/query.pulsar.go (12 hunks)
  • x/consensus/keeper/keeper.go (5 hunks)
  • x/consensus/proto/cosmos/consensus/v1/query.proto (2 hunks)
Files not summarized due to errors (1)
  • api/cosmos/consensus/v1/query.pulsar.go: Error: Message exceeds token limit
Additional context used
Path-based instructions (2)
x/consensus/keeper/keeper.go (1)

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

api/cosmos/consensus/v1/query.pulsar.go (1)

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

Additional comments not posted (18)
x/consensus/proto/cosmos/consensus/v1/query.proto (1)

7-7: Ensure the import is correctly updated to reflect the new dependencies.

x/consensus/keeper/keeper.go (4)

4-4: Ensure that all new imports are necessary and correctly placed according to the Uber Golang style guide.


30-31: The addition of the cometInfo field in the Keeper struct is crucial for storing comet information. Ensure that the field is properly initialized and used across the module.


42-42: Proper initialization of cometInfo in the NewKeeper function is essential for correct module operation. Ensure that the initialization aligns with the module's data handling strategies.


108-125: The implementation of the SetCometInfo method appears robust. However, ensure that the authority validation logic aligns with the system's security requirements.

api/cosmos/consensus/v1/query.pulsar.go (13)

17-24: Ensure proper initialization of md_QueryGetCometInfoRequest.

The initialization of md_QueryGetCometInfoRequest using File_cosmos_consensus_v1_query_proto.Messages().ByName() ensures that the message descriptor is correctly set up during the package initialization phase. This is crucial for the proper functioning of the protobuf messages in runtime.


26-33: Check the implementation of ProtoReflect() in QueryGetCometInfoRequest.

The implementation of ProtoReflect() method for QueryGetCometInfoRequest is standard for protobuf message types in Go. It correctly casts the receiver to its fast reflection type, facilitating efficient operations on the message.


61-65: Review the Descriptor() method in fastReflection_QueryGetCometInfoRequest.

The Descriptor() method correctly returns the message descriptor from the global variable, which is initialized in the init() function. This is a standard practice in protobuf-generated Go code to interact with message descriptors.


67-77: Evaluate the New() and related methods in fastReflection_QueryGetCometInfoRequest.

The New() method and its related functionality are correctly implemented to provide new instances of the QueryGetCometInfoRequest message. This is essential for creating new message instances in protobuf.


85-91: Check the implementation of Range() in fastReflection_QueryGetCometInfoRequest.

The Range() method is implemented as an empty method, which is acceptable here since QueryGetCometInfoRequest does not contain any populated fields directly. This is typical for simple query request types.


114-120: Analyze the Clear() method in fastReflection_QueryGetCometInfoRequest.

The Clear() method provides robust error handling for attempts to clear fields that do not exist or are not supported. This ensures that the message state remains consistent and predictable.


130-136: Inspect the Get() method for retrieving field values.

The Get() method is correctly implemented to panic when trying to access unsupported fields or extensions. This behavior is consistent with the protobuf specification for proto3, where only explicitly defined fields should be accessible.


146-156: Evaluate the Set() method for setting field values.

The Set() method is correctly designed to panic for unsupported fields, ensuring that only valid fields can be modified. This maintains the integrity of the message data structure.


166-176: Check the Mutable() method implementation.

The Mutable() method is implemented to panic for fields that do not support mutability, which is a good practice to enforce immutability where required. This method is crucial for modifying message fields safely.


186-197: Review the NewField() method in fastReflection_QueryGetCometInfoRequest.

The NewField() method is correctly implemented to provide new instances of fields or panic for unsupported fields. This method is essential for working with fields in a type-safe manner.


199-208: Analyze the WhichOneof() method functionality.

The WhichOneof() method is correctly implemented to panic for non-oneof fields, ensuring that the method is used correctly according to the protobuf specifications.


210-226: Inspect the handling of unknown fields in GetUnknown() and SetUnknown().

The methods for getting and setting unknown fields are implemented according to the protobuf best practices, allowing for the handling of unknown data while maintaining the overall message integrity.


240-371: Review the ProtoMethods() implementation.

The ProtoMethods() implementation provides custom methods for size calculation, marshaling, and unmarshaling, which are optimized for performance. This is crucial for efficient serialization and deserialization of protobuf messages.

Merged via the queue into main with commit f413679 Jun 13, 2024
70 checks passed
@tac0turtle tac0turtle deleted the marko/upstream_more_v2_4 branch June 13, 2024 15:01
api/cosmos/consensus/v1/query.pulsar.go Dismissed Show dismissed Hide dismissed
api/cosmos/consensus/v1/query.pulsar.go Dismissed Show dismissed Hide dismissed
api/cosmos/consensus/v1/query.pulsar.go Dismissed Show dismissed Hide dismissed
api/cosmos/consensus/v1/query.pulsar.go Dismissed Show dismissed Hide dismissed
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: 4

Outside diff range and nitpick comments (1)
api/cosmos/consensus/v1/query.pulsar.go (1)

1761-1764: Correct documentation for fields to align with protobuf standards.

Consider revising the documentation to follow protobuf comment standards, ensuring clarity and consistency.

Review details

Configuration used: .coderabbit.yml
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 445c177 and 314c3a4.

Files ignored due to path filters (2)
  • api/cosmos/consensus/v1/query_grpc.pb.go is excluded by !**/*.pb.go
  • x/consensus/types/query.pb.go is excluded by !**/*.pb.go
Files selected for processing (3)
  • api/cosmos/consensus/v1/query.pulsar.go (12 hunks)
  • x/consensus/keeper/keeper.go (5 hunks)
  • x/consensus/proto/cosmos/consensus/v1/query.proto (2 hunks)
Files not summarized due to errors (1)
  • api/cosmos/consensus/v1/query.pulsar.go: Error: Message exceeds token limit
Files skipped from review due to trivial changes (1)
  • x/consensus/keeper/keeper.go
Files skipped from review as they are similar to previous changes (1)
  • x/consensus/proto/cosmos/consensus/v1/query.proto
Additional context used
Path-based instructions (1)
api/cosmos/consensus/v1/query.pulsar.go (1)

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

Additional comments not posted (2)
api/cosmos/consensus/v1/query.pulsar.go (2)

1641-1676: Ensure the newly added CometInfo field in the response is handled correctly across all usages.

Verification successful

The CometInfo field in the QueryGetCometInfoResponse struct is handled correctly across all usages.

  • The field is properly managed in methods such as Reset, String, ProtoReflect, and GetCometInfo.
  • Fast reflection handling includes appropriate checks for the field's existence, getting, setting, and clearing the field.
  • Serialization and deserialization processes correctly handle the CometInfo field.
Scripts executed

The following scripts were executed for the analysis:


Script:

#!/bin/bash
# Check for correct handling of the `CometInfo` field in response handling.
rg --type go 'CometInfo' api/cosmos/consensus/v1/query.pulsar.go

Length of output: 13528


61-65: Verify the correctness of the descriptor retrieval.

Verification successful

The Descriptor function in fastReflection_QueryGetCometInfoRequest correctly returns md_QueryGetCometInfoRequest, which is a protoreflect.MessageDescriptor. This descriptor is set up through the protoimpl.TypeBuilder, which initializes the File_cosmos_consensus_v1_query_proto with the correct RawDescriptor, GoTypes, and DependencyIndexes.

  • md_QueryGetCometInfoRequest is correctly defined and assigned.
  • The protoimpl.TypeBuilder is properly initialized with the necessary components.
Scripts executed

The following scripts were executed for the analysis:


Script:

#!/bin/bash
# Verify the descriptor's integrity and correctness.
ast-grep --lang go --pattern 'func ($_ *fastReflection_QueryGetCometInfoRequest) Descriptor() protoreflect.MessageDescriptor { return $_ }'

Length of output: 410



Script:

#!/bin/bash
# Find the definition of md_QueryGetCometInfoRequest to ensure it is set up correctly.
rg 'md_QueryGetCometInfoRequest' --type go

Length of output: 448



Script:

#!/bin/bash
# Find the definition and initialization of File_cosmos_consensus_v1_query_proto to ensure it is correctly set up.
rg 'File_cosmos_consensus_v1_query_proto' --type go

Length of output: 935



Script:

#!/bin/bash
# Find the definition and initialization of out.File to ensure it is correctly set up.
rg 'out.File' --type go

Length of output: 16540



Script:

#!/bin/bash
# Find the initialization of 'out' to trace how 'out.File' is set up.
rg 'var out ' --type go

Length of output: 264



Script:

#!/bin/bash
# Find the usage of 'out' in the relevant file to trace its initialization.
rg '\bout\b' api/cosmos/consensus/v1/query.pulsar.go

Length of output: 134



Script:

#!/bin/bash
# Find the initialization details of 'protoimpl.TypeBuilder' within the relevant file.
rg 'protoimpl.TypeBuilder' api/cosmos/consensus/v1/query.pulsar.go

Length of output: 97



Script:

#!/bin/bash
# Extract the complete initialization block of 'protoimpl.TypeBuilder' within the relevant file.
rg -A 10 'protoimpl.TypeBuilder' api/cosmos/consensus/v1/query.pulsar.go

Length of output: 470



Script:

#!/bin/bash
# Find the definitions of 'file_cosmos_consensus_v1_query_proto_rawDesc', 'file_cosmos_consensus_v1_query_proto_goTypes', and 'file_cosmos_consensus_v1_query_proto_depIdxs'.
rg 'file_cosmos_consensus_v1_query_proto_(rawDesc|goTypes|depIdxs)' api/cosmos/consensus/v1/query.pulsar.go

Length of output: 1393

Comment on lines +17 to +24
var (
md_QueryGetCometInfoRequest protoreflect.MessageDescriptor
)

func init() {
file_cosmos_consensus_v1_query_proto_init()
md_QueryGetCometInfoRequest = File_cosmos_consensus_v1_query_proto.Messages().ByName("QueryGetCometInfoRequest")
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Initialization of md_QueryGetCometInfoRequest should be thread-safe.

Consider protecting the initialization logic in init() with a mutex to avoid potential race conditions in a concurrent environment.

Comment on lines +1634 to +1639
func (*QueryGetCometInfoRequest) ProtoMessage() {}

// Deprecated: Use QueryGetCometInfoRequest.ProtoReflect.Descriptor instead.
func (*QueryGetCometInfoRequest) Descriptor() ([]byte, []int) {
return file_cosmos_consensus_v1_query_proto_rawDescGZIP(), []int{0}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Deprecated method usage should be avoided.

Consider updating the Descriptor method usage to the recommended ProtoReflect.Descriptor as indicated by the deprecation warning.

Comment on lines +1791 to 1800
0x73, 0x75, 0x73, 0x2f, 0x76, 0x31, 0x3b, 0x63, 0x6f, 0x6e, 0x73, 0x65, 0x6e, 0x73, 0x75, 0x73,
0x76, 0x31, 0xa2, 0x02, 0x03, 0x43, 0x43, 0x58, 0xaa, 0x02, 0x13, 0x43, 0x6f, 0x73, 0x6d, 0x6f,
0x73, 0x2e, 0x43, 0x6f, 0x6e, 0x73, 0x65, 0x6e, 0x73, 0x75, 0x73, 0x2e, 0x56, 0x31, 0xca, 0x02,
0x13, 0x43, 0x6f, 0x73, 0x6d, 0x6f, 0x73, 0x5c, 0x43, 0x6f, 0x6e, 0x73, 0x65, 0x6e, 0x73, 0x75,
0x73, 0x5c, 0x56, 0x31, 0xe2, 0x02, 0x1f, 0x43, 0x6f, 0x73, 0x6d, 0x6f, 0x73, 0x5c, 0x43, 0x6f,
0x6e, 0x73, 0x65, 0x6e, 0x73, 0x75, 0x73, 0x5c, 0x56, 0x31, 0x5c, 0x47, 0x50, 0x42, 0x4d, 0x65,
0x74, 0x61, 0x64, 0x61, 0x74, 0x61, 0xea, 0x02, 0x15, 0x43, 0x6f, 0x73, 0x6d, 0x6f, 0x73, 0x3a,
0x3a, 0x43, 0x6f, 0x6e, 0x73, 0x65, 0x6e, 0x73, 0x75, 0x73, 0x3a, 0x3a, 0x56, 0x31, 0x62, 0x06,
0x70, 0x72, 0x6f, 0x74, 0x6f, 0x33,
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Ensure the File_cosmos_consensus_v1_query_proto initialization is thread-safe.

Similar to previous comments, consider using synchronization techniques to ensure thread safety during the initialization of File_cosmos_consensus_v1_query_proto.

Comment on lines +26 to +27
var _ protoreflect.Message = (*fastReflection_QueryGetCometInfoRequest)(nil)

Copy link
Contributor

Choose a reason for hiding this comment

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

Tip

Codebase Verification

The manual inspection of the file api/cosmos/consensus/v1/query.pulsar.go reveals the following:

  • The QueryGetCometInfoRequest type has the Reset method that dereferences x without a nil check.
  • The ProtoReflect, slowProtoReflect, and String methods for QueryGetCometInfoRequest do not contain any nil pointer dereferences.

Points to Address:

  • Ensure a nil check before dereferencing x in the Reset method.

Locations that need attention:

  • api/cosmos/consensus/v1/query.pulsar.go, line 26-27:
    func (x *QueryGetCometInfoRequest) Reset() {
    	*x = QueryGetCometInfoRequest{}
    }

Suggested Fix:

Add a nil check in the Reset method:

func (x *QueryGetCometInfoRequest) Reset() {
	if x == nil {
		return
	}
	*x = QueryGetCometInfoRequest{}
}
Analysis chain

Ensure proper handling of nil pointer dereferences.

Also applies to: 30-44

Scripts executed

The following scripts were executed for the analysis:


Script:

#!/bin/bash
# Check all usages of `x` to ensure it's not nil before dereferencing.
rg --type go 'x \*QueryGetCometInfoRequest' api/cosmos/consensus/v1/query.pulsar.go

Length of output: 333



Script:

#!/bin/bash
# Extract the function bodies and check for nil pointer dereferences within them.
rg --type go --pcre2 --multiline 'func \(x \*QueryGetCometInfoRequest\) \w+\(\) \{[^}]+\}' api/cosmos/consensus/v1/query.pulsar.go

Length of output: 210



Script:

#!/bin/bash
# Extract the function bodies of the remaining methods and check for nil pointer dereferences within them.
rg --type go --pcre2 --multiline 'func \(x \*QueryGetCometInfoRequest\) (ProtoReflect|slowProtoReflect|String)\(\) \{[^}]+\}' api/cosmos/consensus/v1/query.pulsar.go

Length of output: 167



Script:

#!/bin/bash
# Extract the function bodies of the remaining methods and check for nil pointer dereferences within them.
rg --type go --pcre2 --multiline 'func \(x \*QueryGetCometInfoRequest\) \w+\(\) \{([^}]*\n)*?[^}]*\}' api/cosmos/consensus/v1/query.pulsar.go

Length of output: 221



Script:

#!/bin/bash
# Output the entire file content for manual inspection.
cat api/cosmos/consensus/v1/query.pulsar.go

Length of output: 75880

alpe added a commit that referenced this pull request Jun 17, 2024
* main:
  feat(tx): port simappv2 changes (#20648)
  build(deps): Bump github.com/spf13/cobra from 1.8.0 to 1.8.1 (#20682)
  chore(proto): fix comments (#20679)
  (serverv2/cometbft) Commands support both json and yaml output (#20670)
  fix(x/staking,x/auth):  regenerate mock to fix test (#20684)
  docs: ADR 074: Msg v2 (#20618)
  fix: nested multisig signatures using CLI (#20438)
  chore: fix spelling errors (#20674)
  fix: align Dockerfile for build-push-action (#20672)
  fix: avoid build fail when make localnet-build-env (#20671)
  build(deps): Bump bufbuild/buf-setup-action from 1.32.2 to 1.33.0 (#20669)
  chore: make function comment match function names (#20666)
  chore(consensus): add cometInfo to consensus  (#20615)
  chore: fix typos (#20662)
  fix: Properly parse json in the wait-tx command. (#20631)
  fix(sims): check before sending RotateConsPubKey (#20659)
  test(types/address): add unit tests for the file types/address.go  (#20237)
@coderabbitai coderabbitai bot mentioned this pull request Aug 7, 2024
12 tasks
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.

5 participants