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(Validator endpoints): Implement exocore validator endpoints #235

Merged
merged 13 commits into from
Nov 11, 2024

Conversation

trestinlsd
Copy link
Contributor

@trestinlsd trestinlsd commented Nov 6, 2024

Description

This PR implements the Exocore Validator API endpoint .

Added gRPC and REST endpoints
1./exocore/operator/v1/validator/{validator_addr}/{chain}
2./exocore/operator/v1/validators/{chain}

Support common query params like chainID, pagination

Summary by CodeRabbit

Summary by CodeRabbit

  • New Features

    • Added new RPC methods for querying all validators and specific validator information, enhancing the ability to retrieve validator data.
    • Introduced new message types to support the new RPC methods, including Validator and DelegatorInfo.
    • Implemented a new method for managing validators based on consensus addresses and chain IDs.
  • Bug Fixes

    • Improved error handling for validator queries to ensure robust responses.
  • Refactor

    • Enhanced formatting consistency across proto message definitions for improved readability.
    • Streamlined event emission logic to reduce redundancy and enhance structure.
    • Adjusted field declaration styles for better consistency across various proto files.
    • Standardized formatting of repeated fields and options in proto definitions for clarity.
    • Improved overall control flow and error handling in various methods related to validators and operators.

Copy link
Contributor

coderabbitai bot commented Nov 6, 2024

Walkthrough

This pull request introduces several modifications across multiple proto files within the exocore.operator.v1 package. Key changes include formatting adjustments for consistency and readability in genesis.proto, the addition of new RPC methods and message types in query.proto, and syntax modifications in tx.proto to streamline field declarations. Additionally, the grpc_query.go file in the keeper package has been updated to include new methods for querying validators. These changes enhance the structure and functionality of the proto definitions without altering their underlying logic.

Changes

File Path Change Summary
proto/exocore/operator/v1/genesis.proto Formatting adjustments to repeated fields and options for consistency and readability.
proto/exocore/operator/v1/query.proto Added new import statements, updated message definitions, and introduced new RPC methods and message types.
proto/exocore/operator/v1/tx.proto Syntax modifications to field declarations for improved readability; options moved inline.
x/operator/keeper/grpc_query.go Added new methods Validators and Validator for querying validators, with improved error handling and reporting.
proto/exocore/operator/v1/validator.proto Introduced new messages Validator and DelegatorInfo for blockchain validator attributes and delegation details.
x/operator/types/validator.go Added functionality for managing validators, including a NewValidator function and string representation methods.
go.mod Updated dependencies and added new replacements to align with specific forks and versions.
x/operator/keeper/consensus_keys.go Added method GetValidatorByConsAddrForChainID for retrieving validators based on consensus address and chain ID.

Possibly related PRs

Suggested labels

C:CLI, enhancement

Suggested reviewers

  • cloud8little
  • leonz789
  • mikebraver
  • TimmyExogenous
  • adu-web3
  • bwhour

🐇 In the land of proto, where fields align,
Formatting changes make code so fine.
New queries for validators, oh what a treat,
With clearer structures, our work’s now complete!
So hop along, let’s celebrate this day,
For tidy code makes all bunnies play! 🥕


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.

CodeRabbit Configuration 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.

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 (7)
proto/exocore/operator/v1/tx.proto (2)

Line range hint 215-217: Address TODO comment regarding BLS key support

The TODO comment indicates missing BLS key support which could be important for certain AVS implementations.

Would you like me to help create a GitHub issue to track the BLS key support implementation?


Type consistency verified with one minor inconsistency in customtype path

The verification reveals consistent usage of types across the codebase with:

  • cosmos.Int scalar type consistently used for amount fields
  • cosmos.Dec scalar type consistently used for decimal/value fields

However, there is one inconsistency to note:

  • In proto/exocore/feedistribution/v1/params.proto, the customtype path uses cosmossdk.io/math.LegacyDec while all other files use github.com/cosmos/cosmos-sdk/types.Dec
🔗 Analysis chain

Line range hint 96-164: Verify type consistency across asset and slash-related fields

The field definitions look correct with appropriate types:

  • cosmos.Int for amount fields
  • cosmos.Dec for value fields
  • Non-nullable repeated fields for slash records

Let's verify type consistency across related files:

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check for consistent use of cosmos.Int and cosmos.Dec types
rg -A 1 "cosmos.Int|cosmos.Dec" proto/

Length of output: 5812

x/operator/keeper/grpc_query.go (3)

262-300: Consider adding unit tests for the Validators method

To ensure the correctness and reliability of the new Validators method, please consider adding unit tests that cover various scenarios, including:

  • Requests with and without the ChainId field populated.
  • Pagination over multiple pages.
  • Handling of invalid consensus keys.

Would you like assistance in generating unit test code or opening a GitHub issue to track this task?


303-340: Consider adding unit tests for the Validator method

To ensure the Validator method works correctly, please consider adding unit tests that cover:

  • Valid and invalid validator addresses.
  • Non-existent validators.
  • Error handling paths.

Would you like assistance in generating unit test code or opening a GitHub issue to track this task?


262-300: Optimize slice allocation for validators

When initializing the vals slice, you can optimize memory allocation by providing a capacity hint if you have an estimate of the number of validators. This can improve performance by reducing the number of allocations.

Consider modifying the slice initialization:

- vals := make([]stakingtypes.Validator, 0)
+ vals := make([]stakingtypes.Validator, 0, expectedNumberOfValidators)

Replace expectedNumberOfValidators with an appropriate estimate if available.

proto/exocore/operator/v1/query.proto (2)

267-267: Consistency in API Endpoint Naming

The HTTP GET endpoint /exocore/operator/v1/QueryAVSUSDValue uses uppercase letters, which is unconventional in RESTful API design. Consider updating the endpoint to use lowercase letters for consistency and adherence to standard naming conventions.

Apply the following diff to update the endpoint:

 rpc QueryAVSUSDValue(QueryAVSUSDValueRequest) returns (DecValueField) {
-  option (google.api.http).get = "/exocore/operator/v1/QueryAVSUSDValue";
+  option (google.api.http).get = "/exocore/operator/v1/query_avs_usd_value";
 }

341-344: Add Pagination to 'QueryValidatorRequest' for Consistency

While the QueryValidatorsRequest includes pagination options, the QueryValidatorRequest does not. If there is a possibility of multiple validators being returned or if consistency with other query messages is desired, consider adding pagination to this request.

Apply the following diff to include pagination:

 message QueryValidatorRequest {
   // validator_addr defines the validator address to query for.
   string validator_addr = 1 [(cosmos_proto.scalar) = "cosmos.AddressString"];
+  // pagination defines an optional pagination for the request.
+  cosmos.base.query.v1beta1.PageRequest pagination = 2;
 }
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between b4a331e and 9c8bf1a.

⛔ Files ignored due to path filters (2)
  • x/operator/types/query.pb.go is excluded by !**/*.pb.go
  • x/operator/types/query.pb.gw.go is excluded by !**/*.pb.gw.go
📒 Files selected for processing (4)
  • proto/exocore/operator/v1/genesis.proto (5 hunks)
  • proto/exocore/operator/v1/query.proto (5 hunks)
  • proto/exocore/operator/v1/tx.proto (11 hunks)
  • x/operator/keeper/grpc_query.go (3 hunks)
✅ Files skipped from review due to trivial changes (1)
  • proto/exocore/operator/v1/genesis.proto
🧰 Additional context used
🪛 buf
proto/exocore/operator/v1/query.proto

4-4: import "google/api/annotations.proto": file does not exist

(COMPILE)

🔇 Additional comments (8)
proto/exocore/operator/v1/tx.proto (3)

Line range hint 16-48: LGTM: Well-structured decimal field definitions

The decimal field definitions are properly configured with:

  • Appropriate use of cosmos.Dec scalar type
  • Correct mapping to cosmos-sdk/types.Dec
  • Non-nullable fields for safety
  • Consistent naming conventions for USD value fields

61-63: LGTM: Proper chain ID field configuration

The chain ID field is correctly defined with:

  • Appropriate uint64 type for chain identifiers
  • Proper customname option for Go field naming

Line range hint 196-252: LGTM: Well-defined request messages with proper address handling

The request message definitions are properly configured with:

  • Correct use of cosmos.AddressString scalar type for address validation
  • Appropriate signer options for authentication
x/operator/keeper/grpc_query.go (2)

Line range hint 9-18: Approved: Added necessary imports for gRPC error handling

The addition of "google.golang.org/grpc/codes" and "google.golang.org/grpc/status" imports is appropriate for handling gRPC status codes and errors in the new methods.


277-295: ⚠️ Potential issue

Handle potential errors when appending validators

In the pagination function within the Validators method, if ValidatorByConsAddrForChainID returns an error, it's currently ignored. Consider checking for errors and handling them appropriately to avoid unexpected behavior.

Apply this diff to handle errors returned by ValidatorByConsAddrForChainID:

 val, found, err := k.ValidatorByConsAddrForChainID(
     ctx, wrappedKey.ToConsAddr(), chainIDWithoutRevision,
 )
+ if err != nil {
+     return status.Errorf(codes.Internal, "failed to get validator by consensus address: %v", err)
+ }
 if found {
     vals = append(vals, val)
 }

Likely invalid or redundant comment.

proto/exocore/operator/v1/query.proto (3)

273-274: Ensure Correct Field Access in HTTP Options

In the QueryOperatorSlashInfo method, the HTTP GET option references operator_and_avs.operator_addr and operator_and_avs.avs_address. Since OperatorAVSAddress is embedded using [(gogoproto.embed) = true], ensure that the field paths in the URL are correctly referencing the embedded fields.

Please verify that the field references in the URL are correct and will properly map to the request parameters.


349-349: Verify Options for 'validator' Field

The validator field includes the options (gogoproto.nullable) = false and (amino.dont_omitempty) = true. As with the earlier validators field, please verify that (amino.dont_omitempty) is valid and intended.

Refer to the verification script provided earlier to check for the usage of (amino.dont_omitempty) in the codebase.


332-334: Verify Options for 'validators' Field

The validators field includes the options (gogoproto.nullable) = false and (amino.dont_omitempty) = true. Please verify that (amino.dont_omitempty) is a valid option and correctly applied here. Incorrect or unsupported options may lead to issues during code generation.

To confirm whether (amino.dont_omitempty) is a valid option, you can check for its usage in the codebase:

✅ Verification successful

amino.dont_omitempty Option is Valid and Consistently Used

The option (amino.dont_omitempty) = true is valid and consistently used throughout the codebase's protobuf files, particularly in conjunction with (gogoproto.nullable) = false. This pattern appears in multiple modules including feedistribution, oracle, operator, and appchain, indicating it's a standard and supported option in the project.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Search for the usage of 'amino.dont_omitempty' in proto files.

# Test: Find occurrences of 'amino.dont_omitempty'. Expect: Valid usages or none if unsupported.
rg 'amino\.dont_omitempty' --glob '*.proto'

Length of output: 1570

x/operator/keeper/grpc_query.go Outdated Show resolved Hide resolved
x/operator/keeper/grpc_query.go Outdated Show resolved Hide resolved
x/operator/keeper/grpc_query.go Outdated Show resolved Hide resolved
proto/exocore/operator/v1/query.proto Outdated Show resolved Hide resolved
proto/exocore/operator/v1/query.proto Outdated Show resolved Hide resolved
x/operator/keeper/grpc_query.go Fixed Show fixed Hide fixed
x/operator/keeper/grpc_query.go Fixed Show fixed Hide fixed
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: 8

🧹 Outside diff range and nitpick comments (6)
x/operator/types/validator.go (1)

17-37: Consider adding input validation and documentation for default values.

The constructor function could benefit from additional validation and documentation:

  1. Add validation for the operator address format
  2. Document the reasoning behind default values (e.g., why Unbonded status, ZeroInt for voting power)
  3. Consider pre-allocating capacity for DelegatorTokens slice if you have an expected size
 func NewValidator(operator sdk.AccAddress, pubKey cryptotypes.PubKey, description stakingtypes.Description) (Validator, error) {
+    // Validate operator address
+    if operator.Empty() {
+        return Validator{}, fmt.Errorf("operator address cannot be empty")
+    }
+
     pkAny, err := codectypes.NewAnyWithValue(pubKey)
     if err != nil {
         return Validator{}, err
     }

+    // Initialize with default capacity if known
+    delegatorTokens := make([]DelegatorInfo, 0, 10) // adjust capacity based on expected size
+
     return Validator{
         OperatorAddress:         operator.String(),
         ConsensusPubkey:         pkAny,
         Jailed:                  false,
         Status:                  stakingtypes.Unbonded,
         VotingPower:             math.ZeroInt(),
         DelegatorShares:         math.LegacyZeroDec(),
         Description:             description,
         UnbondingHeight:         int64(0),
         UnbondingTime:           time.Unix(0, 0).UTC(),
         Commission:              stakingtypes.NewCommission(math.LegacyZeroDec(), math.LegacyZeroDec(), math.LegacyZeroDec()),
         UnbondingOnHoldRefCount: 0,
-        DelegatorTokens:         []DelegatorInfo{},
+        DelegatorTokens:         delegatorTokens,
     }, nil
 }
proto/exocore/operator/v1/query.proto (1)

7-7: Remove unused import

The import cosmos/staking/v1beta1/staking.proto is not used in this file.

-import "cosmos/staking/v1beta1/staking.proto";
🧰 Tools
🪛 GitHub Check: lint

[failure] 7-7:
Import "cosmos/staking/v1beta1/staking.proto" is unused.

proto/exocore/operator/v1/validator.proto (4)

28-28: Correct term to 'Bech32 encoded'

In the comment:

// operator_address defines the address of the validator's operator; bech encoded in JSON.

The term 'bech encoded' should be 'Bech32 encoded' to accurately reflect the encoding format.

Apply this diff to correct the comment:

-// operator_address defines the address of the validator's operator; bech encoded in JSON.
+// operator_address defines the address of the validator's operator; Bech32 encoded in JSON.

61-61: Fix typo in comment: 'identifing' → 'identifying'

There's a typographical error in the comment. 'identifing' should be 'identifying'.

Apply this diff to fix the typo:

-// list of unbonding ids, each uniquely identifing an unbonding of this validator
+// list of unbonding ids, each uniquely identifying an unbonding of this validator

68-68: Clarify the abbreviation 'AVS' in the comment

The term 'AVS' is used without definition in the comment:

// DelegatorInfo records the self and total opted-in USD value for the specified operator and AVS

Please provide a definition or expand the abbreviation to improve clarity.

Apply this diff to clarify:

-// DelegatorInfo records the self and total opted-in USD value for the specified operator and AVS
+// DelegatorInfo records the self and total opted-in USD value for the specified operator and [expand 'AVS']

Replace [expand 'AVS'] with the full term or a brief explanation.


74-74: Improve comment clarity for 'amount' field

The comment:

// amount is the total amount of the asset which delegation.

is unclear and contains grammatical errors. Consider rephrasing it for clarity.

Apply this diff to correct the comment:

-// amount is the total amount of the asset which delegation.
+// amount is the total amount of the asset being delegated.
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 9c8bf1a and c1ef399.

⛔ Files ignored due to path filters (4)
  • go.sum is excluded by !**/*.sum
  • x/feedistribution/types/genesis.pb.go is excluded by !**/*.pb.go
  • x/operator/types/query.pb.go is excluded by !**/*.pb.go
  • x/operator/types/validator.pb.go is excluded by !**/*.pb.go
📒 Files selected for processing (5)
  • go.mod (1 hunks)
  • proto/exocore/operator/v1/query.proto (5 hunks)
  • proto/exocore/operator/v1/validator.proto (1 hunks)
  • x/operator/keeper/grpc_query.go (2 hunks)
  • x/operator/types/validator.go (1 hunks)
🧰 Additional context used
🪛 buf
proto/exocore/operator/v1/query.proto

4-4: import "google/api/annotations.proto": file does not exist

(COMPILE)

proto/exocore/operator/v1/validator.proto

4-4: import "gogoproto/gogo.proto": file does not exist

(COMPILE)

🪛 GitHub Check: lint
proto/exocore/operator/v1/query.proto

[failure] 7-7:
Import "cosmos/staking/v1beta1/staking.proto" is unused.

🪛 GitHub Check: Run golangci-lint
x/operator/keeper/grpc_query.go

[failure] 7-7:
File is not gofumpt-ed (gofumpt)


[failure] 20-20:
File is not gofumpt-ed (gofumpt)


[failure] 369-369:
ineffectual assignment to err (ineffassign)


[failure] 415-415:
ineffectual assignment to err (ineffassign)

🪛 GitHub Check: CodeQL
x/operator/keeper/grpc_query.go

[warning] 369-369: Useless assignment to local variable
This definition of err is never used.


[warning] 415-415: Useless assignment to local variable
This definition of err is never used.

🔇 Additional comments (9)
x/operator/types/validator.go (1)

1-63: Verify integration with validator endpoints.

Let's verify that this implementation aligns with the validator endpoints mentioned in the PR objectives (/exocore/operator/v1/validators and /exocore/operator/v1/validators/{validator_addr}).

✅ Verification successful

Validator implementation aligns with proto definitions and endpoints

The implementation in validator.go perfectly matches the proto definition and integrates correctly with the gRPC endpoints:

  • The Validator struct fields in the Go implementation match all fields defined in validator.proto, including:

    • operator_address
    • consensus_pubkey
    • jailed
    • status
    • voting_power
    • delegator_shares
    • description
    • unbonding_height
    • unbonding_time
    • commission
    • unbonding_on_hold_ref_count
    • delegator_tokens
  • The gRPC endpoints are properly defined in query.proto:

    • /exocore/operator/v1/validators for querying all validators
    • /exocore/operator/v1/validators/{validator_addr} for querying specific validator
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify integration with validator endpoints and proto definitions

# Test 1: Check if the Validator type matches the proto definition
echo "Checking validator.proto definition..."
rg -l "message Validator" "proto/"

# Test 2: Verify gRPC endpoint implementation
echo "Checking gRPC endpoint implementation..."
rg -l "service Operator" "proto/"

Length of output: 515


Script:

#!/bin/bash
# Let's examine the validator.proto definition and service endpoints in detail

# Check the validator.proto content
echo "=== Validator Proto Definition ==="
cat proto/exocore/operator/v1/validator.proto

# Look for gRPC service definitions related to validators
echo -e "\n=== Searching for Validator Service Endpoints ==="
rg "rpc.*Validator" proto/exocore/operator/v1/ -A 2

# Check for any validator query implementations
echo -e "\n=== Searching for Validator Query Implementations ==="
fd -e go -E "vendor/*" -E "*_test.go" | xargs rg "func.*Validator.*\(ctx context\.Context"

Length of output: 8618

go.mod (1)

279-280: LGTM! Security update for websocket dependency.

The websocket dependency has been updated to a specific version (v1.8.17) which is a good practice for security and stability. The empty line maintains consistent formatting.

x/operator/keeper/grpc_query.go (3)

289-291: Use consistent chainIDWithoutRevision in ValidatorByConsAddrForChainID call.

The method uses chainIDWithoutRevision for querying but inconsistently uses avstypes.ChainIDWithoutRevision(ctx.ChainID()) in the ValidatorByConsAddrForChainID call.


325-330: Simplify error handling and return nil for empty responses.


332-334: Use consistent chainIDWithoutRevision in ValidatorByConsAddrForChainID call.

proto/exocore/operator/v1/query.proto (4)

4-4: Critical: Fix Missing Import 'google/api/annotations.proto'

🧰 Tools
🪛 buf

4-4: import "google/api/annotations.proto": file does not exist

(COMPILE)


20-20: LGTM! Field options improvements

The changes to field options enhance consistency and improve protobuf code quality:

  • Added proper scalar type for operator_addr
  • Added embedding for operator_and_avs
  • Added appropriate customname options

Also applies to: 48-48, 54-54, 60-60, 82-82


324-327: Inconsistent Field Name: Use 'chain' Instead of 'chain_id'


308-314: Verify REST path conventions for validator endpoints

The new validator endpoints follow standard REST conventions:

  • List endpoint: GET /exocore/operator/v1/validators
  • Single resource endpoint: GET /exocore/operator/v1/validators/{validator_addr}

The implementation aligns with the PR objectives to add validator endpoints.

Also applies to: 316-320

✅ Verification successful

REST path conventions for validator endpoints are consistent and follow best practices

The verification confirms that the validator endpoints follow RESTful conventions consistently:

  • Collection endpoint: GET /exocore/operator/v1/validators
  • Single resource endpoint: GET /exocore/operator/v1/validators/{validator_addr}

The paths maintain proper hierarchy, use plural nouns, and follow standard REST patterns for collection and individual resource access.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Verify consistency of REST path conventions across other endpoints
rg -A 1 'option \(google\.api\.http\).*get.*validators'

Length of output: 377

x/operator/types/validator.go Show resolved Hide resolved
x/operator/types/validator.go Show resolved Hide resolved
go.mod Outdated Show resolved Hide resolved
x/operator/keeper/grpc_query.go Outdated Show resolved Hide resolved
x/operator/keeper/grpc_query.go Outdated Show resolved Hide resolved
proto/exocore/operator/v1/validator.proto Show resolved Hide resolved
proto/exocore/operator/v1/validator.proto Outdated Show resolved Hide resolved
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 (5)
proto/exocore/operator/v1/validator.proto (3)

23-65: Consider reordering fields for better memory alignment.

The current field ordering could be optimized. Consider reordering fields to minimize padding:

  1. Group fixed-size fields first (bool, int64, etc.)
  2. Follow with string and message fields
  3. Finally, place repeated fields
message Validator {
  option (gogoproto.equal) = false;
  option (gogoproto.goproto_stringer) = false;
  option (gogoproto.goproto_getters) = false;

  // Fixed-size fields first
  bool jailed = 3;
  int64 unbonding_height = 8;
  int64 unbonding_on_hold_ref_count = 11;
  cosmos.staking.v1beta1.BondStatus status = 4;

  // String and message fields
  string operator_address = 1 [(cosmos_proto.scalar) = "cosmos.AddressString"];
  google.protobuf.Any consensus_pubkey = 2;
  string voting_power = 5;
  string delegator_shares = 6;
  cosmos.staking.v1beta1.Description description = 7;
  google.protobuf.Timestamp unbonding_time = 9;
  cosmos.staking.v1beta1.Commission commission = 10;

  // Repeated fields last
  repeated uint64 unbonding_ids = 12;
  repeated DelegatorInfo delegator_tokens = 13;
}

58-59: Improve documentation for unbonding reference count.

The comment "strictly positive if this validator's unbonding has been stopped by external modules" could be more descriptive. Consider explaining:

  1. Which external modules can stop unbonding
  2. Why unbonding might be stopped
  3. The significance of the reference count
-    // strictly positive if this validator's unbonding has been stopped by external modules
+    // Reference count tracking the number of external module holds on this validator's unbonding process.
+    // Each external module can increment this counter to prevent the validator from completing unbonding.
+    // The validator can only complete unbonding when this count returns to zero.

67-89: Enhance field documentation for DelegatorInfo message.

Consider improving the documentation:

  1. Add validation constraints (if any) for asset_id, symbol, and name
  2. Clarify the precision/decimal places for amount and total_usd_value
  3. Make comment style consistent (all should end with periods)
 message DelegatorInfo {
-  // asset_id is the asset for which the query is made.
+  // asset_id is the unique identifier for the asset, must be non-empty and follow [a-zA-Z0-9_-]+ format.
   string asset_id = 1 [(gogoproto.customname) = "AssetID"];
-  // symbol of the asset, like "USDT"
+  // symbol of the asset (e.g., "USDT"), must be uppercase and 2-12 characters.
   string symbol = 2;
-  // name of the asset, like "Tether USD"
+  // name of the asset (e.g., "Tether USD"), must be non-empty.
   string name = 3;
-  // amount is the total amount of the asset which delegation.
+  // amount is the total delegated asset amount, stored as a string representation of cosmos.Int.
   string amount = 4
   [
     (cosmos_proto.scalar) = "cosmos.Int",
     (gogoproto.customtype) = "github.com/cosmos/cosmos-sdk/types.Int",
     (gogoproto.nullable) = false
   ];
-  // total_usd_value is the total delegation USD value for the validator
+  // total_usd_value is the total delegation USD value for the validator, stored as a string representation of cosmos.Dec with 18 decimal places.
   string total_usd_value = 5 [
x/operator/keeper/consensus_keys.go (2)

578-607: Consider refactoring repetitive error handling blocks

Multiple error checks and logging statements are repeated at lines 578-582, 589-592, 594-597, and 605-607. Refactoring these into a helper function can reduce code duplication and improve readability.

Consider creating a helper function for error handling to streamline the code.


630-638: Check for nil maps before usage

Before accessing decimals and prices maps, there are checks to ensure they are not nil. However, additional checks after fetching data might be redundant.

Review the necessity of the if prices == nil check at line 651, given that an error would have been returned previously if prices were nil.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between c1ef399 and f6f3486.

⛔ Files ignored due to path filters (2)
  • x/operator/types/query.pb.go is excluded by !**/*.pb.go
  • x/operator/types/validator.pb.go is excluded by !**/*.pb.go
📒 Files selected for processing (5)
  • proto/exocore/operator/v1/query.proto (5 hunks)
  • proto/exocore/operator/v1/validator.proto (1 hunks)
  • x/operator/keeper/consensus_keys.go (2 hunks)
  • x/operator/keeper/grpc_query.go (2 hunks)
  • x/operator/types/validator.go (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • x/operator/types/validator.go
🧰 Additional context used
🪛 buf
proto/exocore/operator/v1/query.proto

4-4: import "amino/amino.proto": file does not exist

(COMPILE)

proto/exocore/operator/v1/validator.proto

4-4: import "gogoproto/gogo.proto": file does not exist

(COMPILE)

🔇 Additional comments (7)
proto/exocore/operator/v1/query.proto (2)

324-353: LGTM: Well-structured validator messages

The new message definitions for validator queries are well-structured with proper use of protobuf options and clear documentation.

Note: The previous review comment about renaming chain_id to chain for consistency is still applicable.


4-4: ⚠️ Potential issue

Critical: Fix Missing Import

The import amino/amino.proto is missing from the project dependencies. This import is required for the amino.dont_omitempty option used in the validator messages. Please ensure this dependency is properly installed and accessible.

🧰 Tools
🪛 buf

4-4: import "amino/amino.proto": file does not exist

(COMPILE)

x/operator/keeper/consensus_keys.go (5)

6-10: Approved: Necessary imports for new functionality

The added imports are appropriate and required for the new methods and types used in the code.


689-690: Verify type compatibility for 'DelegatorShares' assignment

At line 689, val.DelegatorShares is assigned an sdk.Int value after truncation. Ensure that this matches the expected type, as DelegatorShares is typically an sdk.Dec to represent fractional shares accurately.

Please confirm that val.DelegatorShares is intended to be an sdk.Int, or adjust the assignment to match the field's expected type.


689-691: Ensure correct assignment to 'DelegatorTokens'

At line 690, val.DelegatorTokens is assigned delegatorTokens, which is of type []types.DelegatorInfo. Verify that val.DelegatorTokens is intended to hold a slice of DelegatorInfo and that this assignment aligns with the types.Validator definition.

Please confirm that val.DelegatorTokens accepts []types.DelegatorInfo as its type.


615-619: Approved: Setting validator commission from operator info

Retrieving operator commission info and assigning it to the validator is correctly implemented.


610-613: ⚠️ Potential issue

Redundant error check after setting 'val.Jailed'

The err variable is not assigned between lines 608 and 610. The error check at lines 610-613 is redundant and can be removed.

Apply this diff to fix the issue:

     val.Jailed = k.IsOperatorJailedForChainID(ctx, consAddr, chainIDWithoutRevision)

-    if err != nil {
-        ctx.Logger().Error("new validator error", "err", err)
-        return types.Validator{}, false
-    }

Likely invalid or redundant comment.

x/operator/keeper/grpc_query.go Outdated Show resolved Hide resolved
x/operator/keeper/grpc_query.go Outdated Show resolved Hide resolved
proto/exocore/operator/v1/query.proto Outdated Show resolved Hide resolved
x/operator/keeper/consensus_keys.go Outdated Show resolved Hide resolved
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: 3

🧹 Outside diff range and nitpick comments (1)
x/operator/keeper/consensus_keys.go (1)

574-699: Consider refactoring for better maintainability

The function has high cyclomatic complexity with multiple early returns and nested conditions. Consider breaking it down into smaller, more focused functions:

  1. Asset price and decimal retrieval
  2. Staking calculations
  3. Validator construction

This would improve readability and maintainability.

Example structure:

func (k Keeper) NewValidatorByConsAddrForChainID(ctx sdk.Context, consAddr sdk.ConsAddress, chainIDWithoutRevision string) (types.Validator, bool) {
    validator, ok := k.getBaseValidator(ctx, consAddr, chainIDWithoutRevision)
    if !ok {
        return types.Validator{}, false
    }
    
    if err := k.enrichValidatorWithOperatorInfo(ctx, &validator); err != nil {
        return types.Validator{}, false
    }
    
    if err := k.calculateStakingInfo(ctx, &validator); err != nil {
        return types.Validator{}, false
    }
    
    return validator, true
}
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between f6f3486 and 50aa3db.

📒 Files selected for processing (1)
  • x/operator/keeper/consensus_keys.go (2 hunks)
🔇 Additional comments (1)
x/operator/keeper/consensus_keys.go (1)

4-11: LGTM: Import statements are appropriate

The new imports are properly aligned with the added functionality and are all being utilized in the code.

x/operator/keeper/consensus_keys.go Outdated Show resolved Hide resolved
x/operator/keeper/consensus_keys.go Outdated Show resolved Hide resolved
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: 1

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 50aa3db and 837d32d.

📒 Files selected for processing (1)
  • x/operator/keeper/grpc_query.go (2 hunks)
🔇 Additional comments (1)
x/operator/keeper/grpc_query.go (1)

284-286: ⚠️ Potential issue

Use consistent chainID variable.

The method uses avstypes.ChainIDWithoutRevision(ctx.ChainID()) instead of the already computed chainIDWithoutRevision. This could lead to inconsistencies if the chain IDs differ.

Apply this diff to fix the inconsistency:

 val, found := k.NewValidatorByConsAddrForChainID(
-    ctx, wrappedKey.ToConsAddr(), avstypes.ChainIDWithoutRevision(ctx.ChainID()),
+    ctx, wrappedKey.ToConsAddr(), chainIDWithoutRevision,
 )

Likely invalid or redundant comment.

x/operator/keeper/grpc_query.go Outdated Show resolved Hide resolved
@trestinlsd
Copy link
Contributor Author

@coderabbitai please review this pr

Copy link
Contributor

coderabbitai bot commented Nov 8, 2024

@trestinlsd: I will review this PR.

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

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

🧹 Outside diff range and nitpick comments (6)
proto/exocore/operator/v1/validator.proto (3)

62-63: Improve field documentation for delegator_tokens.

The current comment "delegator_tokens is the list of asset infos" could be more descriptive. Consider expanding it to explain:

  • The purpose of tracking these tokens
  • The relationship between validators and delegator tokens
  • Any constraints or important notes about this list
-  // delegator_tokens is the list of asset infos
+  // delegator_tokens represents the list of assets delegated to this validator,
+  // including their amounts and USD values. This information is crucial for
+  // tracking delegation distribution and calculating validator voting power.

67-88: Enhance DelegatorInfo documentation and validation.

Consider the following improvements:

  1. Add format specification for asset_id (e.g., "Format: -")
  2. Specify the precision and format for the amount field
  3. Consider adding field validation rules using protobuf field options
 message DelegatorInfo {
-  // asset_id is the asset for which the query is made
+  // asset_id uniquely identifies the asset in the format <chain>-<identifier>
+  // Example: "eth-0x123..." or "cosmos-atom"
   string asset_id = 1 [(gogoproto.customname) = "AssetID"];
   
   // symbol of the asset, like "USDT"
   string symbol = 2;
   
   // name of the asset, like "Tether USD"
   string name = 3;
   
-  // amount is the total amount of the asset which delegation
+  // amount represents the total delegated tokens in the asset's smallest unit
+  // (e.g., Wei for ETH, satoshi for BTC). The value is stored as a string
+  // to handle large numbers precisely.
   string amount = 4
   [
     (cosmos_proto.scalar) = "cosmos.Int",
     (gogoproto.customtype) = "github.com/cosmos/cosmos-sdk/types.Int",
     (gogoproto.nullable) = false
   ];

1-90: Overall implementation looks solid with room for enhancement.

The protobuf definitions align well with the PR objectives and follow Cosmos SDK patterns. A few architectural considerations:

  1. Consider implementing versioning strategy in the proto package (e.g., v1alpha1, v1beta1) to allow for future breaking changes
  2. Document any assumptions about field value ranges or constraints in a separate validation document
  3. Consider adding examples in comments for complex fields to aid integration
🧰 Tools
🪛 buf

4-4: import "amino/amino.proto": file does not exist

(COMPILE)

x/operator/keeper/grpc_query.go (2)

296-296: Return nil validators when none are found.

When no validators are found, it's more idiomatic to return nil instead of an empty slice.

Apply this diff:

-return &types.QueryValidatorsResponse{Validators: vals, Pagination: pageRes}, nil
+return &types.QueryValidatorsResponse{Validators: nil, Pagination: pageRes}, nil

321-323: Remove redundant chainID processing.

The chainIDWithoutRevision variable is already processed, making the additional call to avstypes.ChainIDWithoutRevision redundant.

Apply this diff:

 found, wrappedKey, err := k.GetOperatorConsKeyForChainID(
-    ctx, accAddr, avstypes.ChainIDWithoutRevision(chainIDWithoutRevision),
+    ctx, accAddr, chainIDWithoutRevision,
 )
x/operator/keeper/consensus_keys.go (1)

686-689: Handle error from IterateAssetsForOperator

The error from IterateAssetsForOperator should include more context in the error log to aid debugging.

Consider enhancing the error logging:

 if err := k.assetsKeeper.IterateAssetsForOperator(ctx, false, operatorAddr.String(), assets, opFunc); err != nil {
-    ctx.Logger().Error("IterateAssetsForOperator error", "err", err)
+    ctx.Logger().Error("Failed to iterate assets for operator", 
+        "operator", operatorAddr.String(),
+        "chainID", chainIDWithoutRevision,
+        "err", err)
     return types.Validator{}, false
 }
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 837d32d and 033aaa3.

⛔ Files ignored due to path filters (4)
  • go.sum is excluded by !**/*.sum
  • x/operator/types/query.pb.go is excluded by !**/*.pb.go
  • x/operator/types/query.pb.gw.go is excluded by !**/*.pb.gw.go
  • x/operator/types/validator.pb.go is excluded by !**/*.pb.go
📒 Files selected for processing (5)
  • go.mod (1 hunks)
  • proto/exocore/operator/v1/query.proto (5 hunks)
  • proto/exocore/operator/v1/validator.proto (1 hunks)
  • x/operator/keeper/consensus_keys.go (2 hunks)
  • x/operator/keeper/grpc_query.go (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • go.mod
🧰 Additional context used
🪛 buf
proto/exocore/operator/v1/query.proto

4-4: import "amino/amino.proto": file does not exist

(COMPILE)

proto/exocore/operator/v1/validator.proto

4-4: import "amino/amino.proto": file does not exist

(COMPILE)

🔇 Additional comments (8)
proto/exocore/operator/v1/validator.proto (1)

4-4: Verify the amino import dependency.

The static analysis indicates that the file amino/amino.proto cannot be found. Please ensure that the amino dependency is correctly configured in your build system.

🧰 Tools
🪛 buf

4-4: import "amino/amino.proto": file does not exist

(COMPILE)

x/operator/keeper/grpc_query.go (1)

284-286: ⚠️ Potential issue

Use consistent chainID variable.

The method uses avstypes.ChainIDWithoutRevision(ctx.ChainID()) instead of the already computed chainIDWithoutRevision. This could lead to inconsistencies if the chain IDs differ.

Apply this diff to fix the inconsistency:

 val, found := k.NewValidatorByConsAddrForChainID(
-    ctx, wrappedKey.ToConsAddr(), avstypes.ChainIDWithoutRevision(ctx.ChainID()),
+    ctx, wrappedKey.ToConsAddr(), chainIDWithoutRevision,
 )

Likely invalid or redundant comment.

proto/exocore/operator/v1/query.proto (4)

324-357: LGTM: Well-structured validator message definitions

The new message definitions are well-implemented with:

  • Clear documentation for each message and field
  • Proper use of protobuf options for nullable and omitempty behavior
  • Consistent field naming patterns
  • Support for pagination in list operations

4-4: ⚠️ Potential issue

Fix missing import 'amino/amino.proto'

The import amino/amino.proto is required for the amino.dont_omitempty option used in the message types, but the file appears to be missing. Please ensure the amino proto definitions are properly included in your project dependencies.

🧰 Tools
🪛 buf

4-4: import "amino/amino.proto": file does not exist

(COMPILE)


311-312: Gas consumption and pagination testing needed

The validator endpoints are well-documented about potential gas consumption issues, but we should ensure proper testing is in place:

  • Add gas consumption tests for the Validators query, especially with large datasets
  • Implement pagination tests to verify correct handling of large result sets

310-322: LGTM: Well-implemented validator endpoints

The new validator endpoints are well-implemented with:

  • Proper use of module_query_safe option
  • Clear documentation about gas consumption
  • RESTful endpoint paths matching PR objectives
x/operator/keeper/consensus_keys.go (2)

4-12: LGTM: Import changes are appropriate

The new imports are correctly organized and necessary for the added functionality.


573-695: LGTM: Overall function structure is sound

The new NewValidatorByConsAddrForChainID function is well-structured and handles the necessary validations and calculations correctly. While there are some improvements suggested above, the core logic is solid.

x/operator/keeper/grpc_query.go Outdated Show resolved Hide resolved
x/operator/keeper/consensus_keys.go Outdated Show resolved Hide resolved
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: 2

🧹 Outside diff range and nitpick comments (6)
x/operator/types/validator.go (1)

3-13: Consider organizing imports into logical groups.

While the imports are valid, they could be organized into logical groups for better readability:

  1. Standard library imports
  2. External dependencies
  3. Cosmos SDK imports
 import (
     // Standard library
     "strings"
 
     // External dependencies
     "sigs.k8s.io/yaml"
 
     // Cosmos SDK
     "cosmossdk.io/math"
     "github.com/cosmos/cosmos-sdk/codec"
     codectypes "github.com/cosmos/cosmos-sdk/codec/types"
     cryptotypes "github.com/cosmos/cosmos-sdk/crypto/types"
     sdk "github.com/cosmos/cosmos-sdk/types"
     stakingtypes "github.com/cosmos/cosmos-sdk/x/staking/types"
 )
🧰 Tools
🪛 GitHub Check: Run golangci-lint

[failure] 3-3:
File is not gofumpt-ed (gofumpt)


[failure] 6-6:
File is not gofumpt-ed (gofumpt)

proto/exocore/operator/v1/validator.proto (3)

26-27: Fix typo in field comment.

The comment for operator_earnings_addr contains a typo.

-  // earnoperator_earnings_addrings_addr is the earnings address.
+  // operator_earnings_addr is the earnings address.

40-40: Fix inconsistent spacing in field assignment.

The spacing around the equals sign is inconsistent with other field assignments.

-  cosmos.staking.v1beta1.BondStatus status =7;
+  cosmos.staking.v1beta1.BondStatus status = 7;

1-83: Consider adding pagination support in the proto definitions.

Given that the PR objectives mention support for pagination in the validator endpoints, consider adding pagination-related fields (like page_size, page_token) to support efficient data retrieval, especially when dealing with large sets of validators or delegations.

Would you like assistance in implementing pagination support in these proto definitions?

🧰 Tools
🪛 buf

4-4: import "amino/amino.proto": file does not exist

(COMPILE)

x/operator/keeper/consensus_keys.go (2)

581-582: Standardize error message format.

The error messages have inconsistent prefixes. Some start with "ValidatorByConsAddrForChainID" while others include additional context. Consider standardizing the format for better log parsing.

Apply this format:

-ctx.Logger().Error("ValidatorByConsAddrForChainID the chainIDWithoutRevision is not supported by AVS", "chainIDWithoutRevision", chainIDWithoutRevision)
+ctx.Logger().Error("ValidatorByConsAddrForChainID: chain ID not supported by AVS", "chainIDWithoutRevision", chainIDWithoutRevision)

-ctx.Logger().Error("ValidatorByConsAddrForChainID the operator isn't found by the chainIDWithoutRevision and consensus address", "consAddress", consAddr, "chainIDWithoutRevision", chainIDWithoutRevision)
+ctx.Logger().Error("ValidatorByConsAddrForChainID: operator not found", "consAddress", consAddr, "chainIDWithoutRevision", chainIDWithoutRevision)

-ctx.Logger().Error("ValidatorByConsAddrForChainID the consensus key isn't found by the chainIDWithoutRevision and operator address", "operatorAddr", operatorAddr, "chainIDWithoutRevision", chainIDWithoutRevision, "err", err)
+ctx.Logger().Error("ValidatorByConsAddrForChainID: consensus key not found", "operatorAddr", operatorAddr, "chainIDWithoutRevision", chainIDWithoutRevision, "err", err)

Also applies to: 591-592, 596-597


612-613: Fix incomplete error message.

The error message is missing the context prefix and details.

Apply this fix:

-ctx.Logger().Error(" new validator error", "err", err)
+ctx.Logger().Error("ValidatorByConsAddrForChainID: failed to get operator info", "err", err)
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 033aaa3 and ee5e45a.

⛔ Files ignored due to path filters (1)
  • x/operator/types/validator.pb.go is excluded by !**/*.pb.go
📒 Files selected for processing (4)
  • proto/exocore/operator/v1/validator.proto (1 hunks)
  • x/operator/keeper/consensus_keys.go (2 hunks)
  • x/operator/keeper/grpc_query.go (2 hunks)
  • x/operator/types/validator.go (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • x/operator/keeper/grpc_query.go
🧰 Additional context used
🪛 buf
proto/exocore/operator/v1/validator.proto

4-4: import "amino/amino.proto": file does not exist

(COMPILE)

🪛 GitHub Check: Run golangci-lint
x/operator/types/validator.go

[failure] 3-3:
File is not gofumpt-ed (gofumpt)


[failure] 6-6:
File is not gofumpt-ed (gofumpt)


[warning] 15-15:
unused-parameter: parameter 'description' seems to be unused, consider removing or renaming it as _ (revive)

🔇 Additional comments (5)
x/operator/types/validator.go (2)

33-46: Past review comment is still applicable.

The previous review comment about improving error handling and documentation in the String method remains valid.


48-57: Past review comment is still applicable.

The previous review comment about optimizing String method performance using strings.Builder remains valid.

proto/exocore/operator/v1/validator.proto (2)

59-82: LGTM! Well-structured DelegatorInfo message.

The message definition is well-documented and uses appropriate types for monetary values. The implementation follows best practices for Protocol Buffers and Cosmos SDK conventions.


4-4: Verify the amino import dependency.

The import statement import "amino/amino.proto" is flagged as missing. Please ensure that the amino package is correctly installed and the import path is accurate.

✅ Verification successful

The amino/amino.proto import is consistently used across the codebase

The import of amino/amino.proto is a standard pattern used consistently across multiple proto files in the exocore ecosystem. This is evidenced by its presence in numerous proto files including oracle, operator, delegation, and feedistribution modules. The import is part of the project's protobuf configuration and is likely resolved through the build system's dependency management.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check if amino proto files exist in the repository or in common proto locations
fd -g 'amino.proto' 

Length of output: 19


Script:

#!/bin/bash
# Let's check for any references to amino in proto files and build configurations
rg -g '*.proto' -l 'amino'

# Also check for any build config files that might reference amino
rg -g '*.{yaml,yml,toml,json}' -l 'amino'

# Search for proto-related configuration or documentation
fd -g 'README*' -X grep -l 'amino' {} \;
fd -g 'proto*' -t f -X grep -l 'amino' {} \;

Length of output: 761


Script:

#!/bin/bash
# Let's examine the actual usage of amino imports in these proto files
# to understand the pattern and verify if this is a consistent usage
rg -g '*.proto' '^import.*amino' -A 2

# Also check the content of one of these files to see how amino is used
cat proto/exocore/oracle/v1/tx.proto | head -n 20

Length of output: 3867

🧰 Tools
🪛 buf

4-4: import "amino/amino.proto": file does not exist

(COMPILE)

x/operator/keeper/consensus_keys.go (1)

644-693: LGTM: Staking calculations are well-implemented.

The staking and delegation calculations are thorough with proper error handling and clear variable naming.

x/operator/types/validator.go Outdated Show resolved Hide resolved
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: 2

🧹 Outside diff range and nitpick comments (2)
x/operator/keeper/consensus_keys.go (2)

627-630: Improve error logging for GetAssetsDecimal.

The error from GetAssetsDecimal is silently ignored. Consider logging the error before returning for better debugging.

 decimals, err := k.assetsKeeper.GetAssetsDecimal(ctx, assets)
 if err != nil {
+    ctx.Logger().Error("failed to get asset decimals", "err", err)
     return types.Validator{}, false
 }

619-626: Simplify asset validation logic.

The current implementation has redundant nil checks. The first check returns false without logging, while the second does the same.

 assets, err := k.avsKeeper.GetAVSSupportedAssets(ctx, avsAddrStr)
 if err != nil {
+    ctx.Logger().Error("failed to get AVS supported assets", "err", err)
     return types.Validator{}, false
 }
-if assets == nil {
-    return types.Validator{}, false
-}
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between ee5e45a and 1f91c2b.

📒 Files selected for processing (2)
  • x/operator/keeper/consensus_keys.go (2 hunks)
  • x/operator/types/validator.go (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • x/operator/types/validator.go

x/operator/keeper/consensus_keys.go Outdated Show resolved Hide resolved
proto/exocore/operator/v1/query.proto Outdated Show resolved Hide resolved
proto/exocore/operator/v1/validator.proto Outdated Show resolved Hide resolved
x/operator/keeper/consensus_keys.go Show resolved Hide resolved
Copy link
Contributor

@mikebraver mikebraver left a comment

Choose a reason for hiding this comment

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

LGTM

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 (5)
proto/exocore/operator/v1/validator.proto (2)

26-27: Fix typo in field comment

The comment for operator_earnings_addr contains a typo: "earnoperator_earnings_addrings_addr".

-  // earnoperator_earnings_addrings_addr is the earnings address.
+  // operator_earnings_addr is the earnings address.

21-57: Consider adding validation constraints

The Validator message contains critical fields but lacks explicit validation constraints. Consider adding field validation rules using protobuf validation options.

Key validations to consider:

  1. Address format validation for operator_earnings_addr and operator_approve_addr
  2. Non-empty constraints for operator_meta_info
  3. Range validation for voting_power and delegator_shares
  4. Commission rate bounds

Example validation options:

string operator_earnings_addr = 1 [(validate.rules).string = {
    address: true,
    min_len: 1
}];

Would you like me to provide a complete example with all the validation rules?

x/operator/keeper/grpc_query.go (3)

274-279: Improve error handling and debugging capabilities.

Two suggestions for improvement:

  1. Include the key in error messages for better debugging
  2. Provide more specific error messages for different unmarshal failure scenarios

Apply this diff to enhance error handling:

-pageRes, err := query.Paginate(store, req.Pagination, func(_ []byte, value []byte) error {
+pageRes, err := query.Paginate(store, req.Pagination, func(key []byte, value []byte) error {
     ret := &tmprotocrypto.PublicKey{}
     // don't use MustUnmarshal to not panic for queries
     if err := ret.Unmarshal(value); err != nil {
-        return status.Errorf(codes.Internal, "failed to unmarshal public key: %v", err)
+        return status.Errorf(codes.Internal, "failed to unmarshal public key for key %X: %v", key, err)
     }

321-323: Remove redundant chain ID conversion.

The chain ID is being converted twice unnecessarily.

Apply this diff to remove the redundant conversion:

 found, wrappedKey, err := k.GetOperatorConsKeyForChainID(
-    ctx, accAddr, avstypes.ChainIDWithoutRevision(chainIDWithoutRevision),
+    ctx, accAddr, chainIDWithoutRevision,
 )

325-330: Simplify error handling logic.

The error handling can be simplified by checking conditions separately and providing more specific error messages.

Apply this diff to improve error handling:

-if !found || err != nil || wrappedKey == nil {
-    if err != nil {
-        return nil, err
-    }
-    return nil, status.Errorf(codes.NotFound, "validator %s not found", req.ValidatorAccAddr)
-}
+if err != nil {
+    return nil, status.Errorf(codes.Internal, "failed to get operator consensus key: %v", err)
+}
+if !found {
+    return nil, status.Errorf(codes.NotFound, "no consensus key found for validator %s", req.ValidatorAccAddr)
+}
+if wrappedKey == nil {
+    return nil, status.Errorf(codes.Internal, "invalid consensus key for validator %s", req.ValidatorAccAddr)
+}
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 1f91c2b and 5aa3180.

⛔ Files ignored due to path filters (2)
  • x/operator/types/query.pb.go is excluded by !**/*.pb.go
  • x/operator/types/query.pb.gw.go is excluded by !**/*.pb.gw.go
📒 Files selected for processing (3)
  • proto/exocore/operator/v1/query.proto (5 hunks)
  • proto/exocore/operator/v1/validator.proto (1 hunks)
  • x/operator/keeper/grpc_query.go (2 hunks)
🧰 Additional context used
🪛 buf
proto/exocore/operator/v1/query.proto

4-4: import "amino/amino.proto": file does not exist

(COMPILE)

proto/exocore/operator/v1/validator.proto

4-4: import "amino/amino.proto": file does not exist

(COMPILE)

🔇 Additional comments (2)
proto/exocore/operator/v1/query.proto (2)

324-357: Well-structured validator message types

The implementation follows best practices:

  • Proper use of protobuf field options for deterministic encoding
  • Consistent field naming (using chain instead of chain_id)
  • Standard Cosmos SDK pagination support

4-4: ⚠️ Potential issue

Missing proto dependency: amino/amino.proto

The import amino/amino.proto is required for the amino.dont_omitempty option used in the validator messages. Please ensure this dependency is properly included in your project.

🧰 Tools
🪛 buf

4-4: import "amino/amino.proto": file does not exist

(COMPILE)

proto/exocore/operator/v1/validator.proto Outdated Show resolved Hide resolved
x/operator/keeper/grpc_query.go Show resolved Hide resolved
x/operator/keeper/grpc_query.go Show resolved Hide resolved
proto/exocore/operator/v1/query.proto Show resolved Hide resolved
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: 3

🧹 Outside diff range and nitpick comments (3)
proto/exocore/operator/v1/validator.proto (2)

26-27: Fix typo in field comment.

The comment for operator_earnings_addr contains a typo: "earnoperator_earnings_addrings_addr".

-  // earnoperator_earnings_addrings_addr is the earnings address.
+  // operator_earnings_addr is the earnings address.

59-96: Consider adding validation rules for critical fields.

While the DelegatorInfo message is well-structured, consider adding validation rules for critical fields like asset_id and symbol to ensure data integrity.

Example validation rules:

string asset_id = 1 [
  (gogoproto.customname) = "AssetID",
  (validate.rules).string = {
    min_len: 1,
    max_len: 64,
    pattern: "^[a-zA-Z0-9_-]+$"
  }
];
string symbol = 2 [(validate.rules).string = {
  min_len: 1,
  max_len: 20,
  pattern: "^[A-Z0-9]+$"
}];

This would require adding:

import "validate/validate.proto";
x/operator/keeper/consensus_keys.go (1)

649-684: Refactor asset iteration logic

The asset iteration logic is complex and could be simplified:

  1. Error handling could be more concise
  2. Map validations could be moved to a separate function

Consider extracting the price and decimal validation:

+func validatePriceAndDecimal(prices map[string]oracletype.Price, decimals map[string]uint32, assetID string) error {
+    if price, ok := prices[assetID]; !ok {
+        return errorsmod.Wrap(types.ErrKeyNotExistInMap, "price not found for asset")
+    }
+    if _, ok := decimals[assetID]; !ok {
+        return errorsmod.Wrap(types.ErrKeyNotExistInMap, "decimal not found for asset")
+    }
+    return nil
+}

 opFunc := func(assetID string, state *assetstypes.OperatorAssetInfo) error {
-    price, ok := prices[assetID]
-    if !ok {
-        return errorsmod.Wrap(types.ErrKeyNotExistInMap, "CalculateUSDValueForOperator map: prices, key: assetID")
-    }
-    decimal, ok := decimals[assetID]
-    if !ok {
-        return errorsmod.Wrap(types.ErrKeyNotExistInMap, "CalculateUSDValueForOperator map: decimals, key: assetID")
-    }
+    if err := validatePriceAndDecimal(prices, decimals, assetID); err != nil {
+        return err
+    }
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 5aa3180 and 57f414e.

⛔ Files ignored due to path filters (1)
  • x/operator/types/validator.pb.go is excluded by !**/*.pb.go
📒 Files selected for processing (2)
  • proto/exocore/operator/v1/validator.proto (1 hunks)
  • x/operator/keeper/consensus_keys.go (2 hunks)
🧰 Additional context used
🪛 buf
proto/exocore/operator/v1/validator.proto

4-4: import "amino/amino.proto": file does not exist

(COMPILE)

🔇 Additional comments (3)
proto/exocore/operator/v1/validator.proto (1)

13-57: LGTM: Well-structured Validator message definition.

The Validator message is well-defined with:

  • Clear documentation explaining the validator concept and bond shares
  • Appropriate use of Cosmos SDK types and protobuf options
  • Proper field annotations for serialization and type handling
x/operator/keeper/consensus_keys.go (2)

4-12: LGTM: Required imports added correctly

The new imports are properly organized and necessary for the added functionality.


686-689: LGTM: Proper error handling in IterateAssetsForOperator

The error from IterateAssetsForOperator is now properly handled with appropriate logging.

proto/exocore/operator/v1/validator.proto Show resolved Hide resolved
x/operator/keeper/consensus_keys.go Show resolved Hide resolved
x/operator/keeper/consensus_keys.go Show resolved Hide resolved
@cloud8little
Copy link
Contributor

Test passed, example response:

{
  "validators": [
    {
      "operator_earnings_addr": "exo18cggcpvwspnd5c6ny8wrqxpffj5zmhklprtnph",
      "operator_approve_addr": "",
      "operator_meta_info": "operator1",
      "cons_address": "exovalcons18z3p42xn8pjk338upvzp794h02wh7p4t7jj9jx",
      "consensus_pubkey": {
        "@type": "/cosmos.crypto.ed25519.PubKey",
        "key": "8PaRnlIsW5fbLIJVv/dD+d/d162fw3ywwWcLSA0PmRQ="
      },
      "jailed": false,
      "status": "BOND_STATUS_BONDED",
      "voting_power": "14598200.638350000000000000",
      "delegator_shares": "0",
      "commission": {
        "commission_rates": {
          "rate": "0.500000000000000000",
          "max_rate": "1.000000000000000000",
          "max_change_rate": "0.000000000000000000"
        },
        "update_time": "2024-11-08T07:22:46.356914120Z"
      },
      "delegator_tokens": [
        {
          "asset_id": "0xdac17f958d2ee523a2206206994597c13d831ec7_0x65",
          "symbol": "",
          "name": "Tether USD",
          "amount": "5000",
          "total_usd_value": "14598200.638350000000000000"
        }
      ]
    },
    {
      "operator_earnings_addr": "exo1l6mv868r7ewkr4sk8z22u6yzmt8tk7jyt38kxd",
      "operator_approve_addr": "exo1l6mv868r7ewkr4sk8z22u6yzmt8tk7jyt38kxd",
      "operator_meta_info": "Operator1",
      "cons_address": "exovalcons102r32v5cfml425l5nrmd95y2qf9s6hrr68mfdk",
      "consensus_pubkey": {
        "@type": "/cosmos.crypto.ed25519.PubKey",
        "key": "C7vpWl72OOMdpVuVaMnX+1dhXIGnffwzPxaVyg6YnK8="
      },
      "jailed": true,
      "status": "BOND_STATUS_BONDED",
      "voting_power": "2893363.366520970000000000",
      "delegator_shares": "0",
      "commission": {
        "commission_rates": {
          "rate": "0.500000000000000000",
          "max_rate": "1.000000000000000000",
          "max_change_rate": "1.000000000000000000"
        },
        "update_time": "2024-11-08T07:22:46.356914120Z"
      },
      "delegator_tokens": [
        {
          "asset_id": "0xdac17f958d2ee523a2206206994597c13d831ec7_0x65",
          "symbol": "",
          "name": "Tether USD",
          "amount": "991",
          "total_usd_value": "2893363.366520970000000000"
        }
      ]
    }
  ],
  "pagination": {
    "next_key": null,
    "total": "2"
  }
}

@trestinlsd trestinlsd merged commit 6518f04 into ExocoreNetwork:develop Nov 11, 2024
22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants