feat: Add consensus address to validator query response #20434
Conversation
WalkthroughThe recent updates to the Changes
Sequence DiagramssequenceDiagram
participant User as User
participant Node as Node
participant StakingModule as x/staking
User->>Node: Request validator query
Node->>StakingModule: Query validators
StakingModule->>StakingModule: Include consensus address
StakingModule->>Node: Return validators with consensus address
Node->>User: Response with validators
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (1)
CHANGELOG.md (1)
65-65: Consider providing more detail in the changelog entry.While the current entry is clear, adding a brief description of how the consensus address is added (e.g., through updates to the
Validatorstruct and gRPC queries) could provide more context and clarity for the reader.
Review Details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files ignored due to path filters (1)
x/staking/types/staking.pb.gois excluded by!**/*.pb.go
Files selected for processing (6)
- CHANGELOG.md (1 hunks)
- api/cosmos/staking/v1beta1/staking.pulsar.go (17 hunks)
- x/staking/keeper/grpc_query.go (4 hunks)
- x/staking/proto/cosmos/staking/v1beta1/staking.proto (1 hunks)
- x/staking/types/validator.go (1 hunks)
- x/staking/types/validator_test.go (1 hunks)
Files not summarized due to errors (1)
- api/cosmos/staking/v1beta1/staking.pulsar.go: Error: Message exceeds token limit
Additional Context Used
Path-based Instructions (5)
x/staking/types/validator_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"x/staking/types/validator.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/staking/keeper/grpc_query.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.CHANGELOG.md (1)
Pattern
**/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"api/cosmos/staking/v1beta1/staking.pulsar.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Additional comments not posted (20)
x/staking/types/validator_test.go (1)
322-325: The test functionTestValidatorSetConsensusAddresscorrectly verifies the setting of theConsensusAddress. Good coverage for the new feature.x/staking/types/validator.go (1)
49-49: The addition of theConsensusAddressfield in theValidatorstruct is correctly initialized in theNewValidatorfunction. This aligns with the changes in the protobuf definition and the PR objectives.x/staking/proto/cosmos/staking/v1beta1/staking.proto (1)
149-150: The addition of theconsensus_addressfield in theValidatormessage is correctly defined with appropriate protobuf options. This change is consistent with the updates in the Go types and aligns with the PR objectives.x/staking/keeper/grpc_query.go (3)
657-668: The implementation ofsetConsensusAddresscorrectly handles potential nil validators and successfully sets the consensus address if the public key type assertion passes.
52-52: The integration ofsetConsensusAddressin theValidatorsmethod is correctly placed to ensure each validator's consensus address is set during the query.
89-89: The use ofsetConsensusAddressin theValidatormethod is correctly applied to set the consensus address for the queried validator.api/cosmos/staking/v1beta1/staking.pulsar.go (14)
2963-2963: Addition offd_Validator_consensus_addressfield descriptor is consistent with the protobuf changes.
2982-2982: Proper initialization offd_Validator_consensus_addressusingmd_Validator.Fields().ByName("consensus_address").
3128-3133: EnsureConsensusAddressis included in the message only if it is non-empty. This is a good practice to avoid unnecessary data transmission.
3175-3176: Correct implementation of the case forConsensusAddressin the switch statement. It checks for non-empty values correctly.
3219-3220: Proper handling of theConsensusAddressfield in the reset case, setting it to an empty string.
3279-3281: Correct retrieval of theConsensusAddressvalue for the protobuf message.
3330-3331: Correct assignment of theConsensusAddressfrom the interface value. This ensures type safety and correctness.
3394-3395: Correctly marking theConsensusAddressfield as immutable in this context, which aligns with the protobuf specifications for non-mutable fields.
3440-3441: Returning a default empty string forConsensusAddresswhen requested, which is a safe fallback.
3562-3565: Proper calculation of the serialized size forConsensusAddress, including handling of non-empty strings.
3595-3601: Correct serialization logic forConsensusAddress, ensuring it is included only if non-empty and properly calculating its position in the byte array.
4205-4236: Proper unmarshalling logic forConsensusAddress, including error handling for incorrect wire types and unexpected EOF.
15020-15021: Addition ofConsensusAddressto theValidatorstruct is consistent with the protobuf changes and PR objectives.
15135-15140: Implementation ofGetConsensusAddressmethod is correct, ensuring it returns an empty string if theValidatoris nil, which is a good null safety practice.
github-actions
bot
added
C:x/slashing
C:x/distribution
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range and nitpick comments (3)
x/staking/CHANGELOG.md (2)
Line range hint
103-103: Use "an" instead of "a" before words that start with a vowel sound.- In a undelegation or redelegation if the shares being left delegated correspond to less than 1 token (in base denom) the entire delegation gets removed. + In an undelegation or redelegation if the shares being left delegated correspond to less than 1 token (in base denom) the entire delegation gets removed.
Line range hint
55-99: The indentation for list items should be consistent. Adjust to 2 spaces for better readability and adherence to Markdown best practices.- * [#20238](https://github.com/cosmos/cosmos-sdk/pull/20238) `NewKeeper` now accepts a `core/comet.Service` as its last argument. + * [#20238](https://github.com/cosmos/cosmos-sdk/pull/20238) `NewKeeper` now accepts a `core/comet.Service` as its last argument. - * [#19788](https://github.com/cosmos/cosmos-sdk/pull/19788) Remove `ABCIValidatorUpdate` and `ABCIValidatorUpdateZero`, use `ModuleValidatorUpdate` and `ModuleValidatorUpdateIsZero` instead. + * [#19788](https://github.com/cosmos/cosmos-sdk/pull/19788) Remove `ABCIValidatorUpdate` and `ABCIValidatorUpdateZero`, use `ModuleValidatorUpdate` and `ModuleValidatorUpdateIsZero` instead. - * [#19754](https://github.com/cosmos/cosmos-sdk/pull/19754) Update to use `[]appmodule.ValidatorUpdate` as return for `ApplyAndReturnValidatorSetUpdates`. + * [#19754](https://github.com/cosmos/cosmos-sdk/pull/19754) Update to use `[]appmodule.ValidatorUpdate` as return for `ApplyAndReturnValidatorSetUpdates`. ...x/staking/keeper/msg_server.go (1)
112-112: Ensure that error handling is consistent across all methods.It's good practice to ensure that error messages are consistent and informative across all methods. This helps in debugging and maintaining the code more effectively.
Review Details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files ignored due to path filters (1)
x/staking/types/staking.pb.gois excluded by!**/*.pb.go
Files selected for processing (20)
- api/cosmos/staking/v1beta1/staking.pulsar.go (16 hunks)
- tests/integration/distribution/keeper/msg_server_test.go (1 hunks)
- tests/integration/gov/keeper/common_test.go (2 hunks)
- tests/integration/staking/keeper/genesis_test.go (2 hunks)
- tests/integration/staking/keeper/msg_server_test.go (1 hunks)
- tests/integration/staking/keeper/validator_test.go (2 hunks)
- tests/sims/distribution/operations_test.go (1 hunks)
- tests/sims/slashing/operations_test.go (2 hunks)
- x/distribution/keeper/allocation_test.go (1 hunks)
- x/distribution/testutil/staking_helper.go (1 hunks)
- x/slashing/keeper/hooks_test.go (2 hunks)
- x/slashing/keeper/msg_server_test.go (5 hunks)
- x/staking/CHANGELOG.md (1 hunks)
- x/staking/keeper/grpc_query.go (4 hunks)
- x/staking/keeper/msg_server.go (2 hunks)
- x/staking/proto/cosmos/staking/v1beta1/staking.proto (1 hunks)
- x/staking/simulation/genesis.go (2 hunks)
- x/staking/testutil/validator.go (2 hunks)
- x/staking/types/validator.go (1 hunks)
- x/staking/types/validator_test.go (2 hunks)
Files skipped from review as they are similar to previous changes (4)
- x/staking/keeper/grpc_query.go
- x/staking/proto/cosmos/staking/v1beta1/staking.proto
- x/staking/types/validator.go
- x/staking/types/validator_test.go
Additional Context Used
LanguageTool (2)
x/staking/CHANGELOG.md (2)
Near line 96: Consider a shorter alternative to avoid wordiness.
Context: ...pi/cosmos/staking/v1beta1".Infraction_` in order to remove dependency between modules on st...
Near line 103: Use “an” instead of ‘a’ if the following word starts with a vowel sound, e.g. ‘an article’, ‘an hour’.
Context: ...ub.com//pull/18841) In a undelegation or redelegation if the sha...
Markdownlint (25)
x/staking/CHANGELOG.md (25)
55: Expected: 2; Actual: 4
Unordered list indentation
57: Expected: 2; Actual: 4
Unordered list indentation
58: Expected: 2; Actual: 4
Unordered list indentation
60: Expected: 2; Actual: 4
Unordered list indentation
61: Expected: 2; Actual: 4
Unordered list indentation
63: Expected: 2; Actual: 4
Unordered list indentation
64: Expected: 2; Actual: 4
Unordered list indentation
66: Expected: 2; Actual: 4
Unordered list indentation
68: Expected: 2; Actual: 4
Unordered list indentation
70: Expected: 2; Actual: 4
Unordered list indentation
71: Expected: 2; Actual: 4
Unordered list indentation
74: Expected: 2; Actual: 4
Unordered list indentation
75: Expected: 2; Actual: 4
Unordered list indentation
77: Expected: 2; Actual: 4
Unordered list indentation
79: Expected: 2; Actual: 4
Unordered list indentation
81: Expected: 2; Actual: 4
Unordered list indentation
83: Expected: 2; Actual: 4
Unordered list indentation
85: Expected: 2; Actual: 4
Unordered list indentation
88: Expected: 2; Actual: 4
Unordered list indentation
90: Expected: 2; Actual: 4
Unordered list indentation
92: Expected: 2; Actual: 4
Unordered list indentation
95: Expected: 2; Actual: 4
Unordered list indentation
99: Expected: 2; Actual: 4
Unordered list indentation
47: Expected: 0 or 2; Actual: 1
Trailing spaces
98: Expected: 0 or 2; Actual: 1
Trailing spaces
Path-based Instructions (16)
x/staking/testutil/validator.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/slashing/keeper/hooks_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"tests/integration/gov/keeper/common_test.go (3)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
tests/**/*: "Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull request"
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"x/staking/simulation/genesis.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/distribution/testutil/staking_helper.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.tests/integration/staking/keeper/genesis_test.go (3)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
tests/**/*: "Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull request"
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"x/staking/CHANGELOG.md (1)
Pattern
**/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"tests/sims/slashing/operations_test.go (3)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
tests/**/*: "Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull request"
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"tests/sims/distribution/operations_test.go (3)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
tests/**/*: "Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull request"
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"x/slashing/keeper/msg_server_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"tests/integration/staking/keeper/msg_server_test.go (3)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
tests/**/*: "Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull request"
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"x/distribution/keeper/allocation_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"x/staking/keeper/msg_server.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.tests/integration/distribution/keeper/msg_server_test.go (3)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
tests/**/*: "Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull request"
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"tests/integration/staking/keeper/validator_test.go (3)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
tests/**/*: "Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull request"
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"api/cosmos/staking/v1beta1/staking.pulsar.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Additional comments not posted (40)
x/staking/testutil/validator.go (1)
Line range hint
10-21: LGTM! Consider adding a comment explaining the use ofaddresscodec.NewBech32Codecfor future maintainability.x/slashing/keeper/hooks_test.go (1)
Line range hint
6-31: LGTM! Consider adding a comment explaining the use ofaddresscodec.NewBech32Codecfor future maintainability.tests/integration/gov/keeper/common_test.go (1)
Line range hint
15-49: LGTM! Consider adding a comment explaining the use ofaddresscodec.NewBech32Codecfor future maintainability.x/staking/simulation/genesis.go (1)
Line range hint
12-93: LGTM! Consider adding a comment explaining the use ofaddresscodec.NewBech32Codecfor future maintainability.x/distribution/testutil/staking_helper.go (1)
11-17: LGTM! Consider adding a comment explaining the use ofaddresscodec.NewBech32Codecfor future maintainability.tests/integration/staking/keeper/genesis_test.go (1)
Line range hint
16-200: LGTM! Consider adding a comment explaining the use ofaddresscodec.NewBech32Codecfor future maintainability.x/staking/CHANGELOG.md (1)
31-31: The changelog entry succinctly captures the essence of the PR. Good job on maintaining clarity and brevity.tests/sims/slashing/operations_test.go (2)
211-211: Ensure that theNewValidatorfunction is correctly setting up the validator, especially with respect to the consensus address and other new fields introduced in the PR.Verification successful
The
NewValidatorfunction inx/staking/types/validator.gocorrectly initializes all fields, including the consensus address and other relevant fields. Here are the key points from the implementation:
- The consensus address is derived from the public key and converted to a string using the
consensusAddressCodec.- All other fields such as
OperatorAddress,ConsensusPubkey,Jailed,Status,Tokens,DelegatorShares,Description,UnbondingHeight,UnbondingTime,Commission,MinSelfDelegation, andUnbondingOnHoldRefCountare properly initialized.No issues were found with the setup of the
NewValidatorfunction.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the correct setup of validators in the test environment. # Test: Search for the `NewValidator` function usage and check for correct field initializations. Expect: Proper initialization of all fields. ast-grep --lang go --pattern $'func NewValidator($_, $_, $_, $_) { $$$ }'Length of output: 76
Script:
#!/bin/bash # Description: Locate the `NewValidator` function definition and inspect its implementation. # Step 1: Locate the `NewValidator` function definition. rg 'func NewValidator' --glob '*.go' # Step 2: Once located, print the function implementation to verify field initializations. # Note: The file path and line numbers will be dynamically determined from the previous command's output.Length of output: 1063
Script:
#!/bin/bash # Description: Extract and inspect the `NewValidator` function implementation from `x/staking/types/validator.go`. # Step 1: Extract the `NewValidator` function implementation. ast-grep --lang go --pattern $'func NewValidator(operator string, pubKey cryptotypes.PubKey, description Description, consensusAddressCodec address.Codec) (Validator, error) { $$$ }' x/staking/types/validator.goLength of output: 2102
34-34: Ensure that all necessary imports are used and there are no unused imports to maintain clean code.tests/sims/distribution/operations_test.go (1)
264-264: Ensure that theNewValidatorfunction is correctly setting up the validator, especially with respect to the consensus address and other new fields introduced in the PR.Verification successful
The
NewValidatorfunction inx/staking/types/validator.gocorrectly initializes all necessary fields, including the consensus address and other relevant fields. The function implementation ensures that the validator is set up properly with the provided parameters.
OperatorAddressConsensusAddressConsensusPubkeyJailedStatusTokensDelegatorSharesDescriptionUnbondingHeightUnbondingTimeCommissionMinSelfDelegationUnbondingOnHoldRefCountThis confirms that the validator setup in the test file
tests/sims/distribution/operations_test.gois correct.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the correct setup of validators in the test environment. # Test: Search for the `NewValidator` function usage and check for correct field initializations. Expect: Proper initialization of all fields. ast-grep --lang go --pattern $'func NewValidator($_, $_, $_, $_) { $$$ }'Length of output: 76
Script:
#!/bin/bash # Description: Locate the definition of the `NewValidator` function and inspect its implementation. # Search for the `NewValidator` function definition in the codebase. rg 'func NewValidator' --glob '*.go'Length of output: 1063
Script:
#!/bin/bash # Description: Extract the implementation of the `NewValidator` function from `x/staking/types/validator.go`. # Extract the implementation of the `NewValidator` function. ast-grep --lang go --pattern $'func NewValidator(operator string, pubKey cryptotypes.PubKey, description Description, consensusAddressCodec address.Codec) (Validator, error) { $$$ }' x/staking/types/validator.goLength of output: 2102
x/slashing/keeper/msg_server_test.go (1)
176-176: Ensure that the test cases cover all new functionalities introduced in the PR, especially those related to the handling of the consensus address.Also applies to: 217-217, 252-252, 287-287, 322-322
tests/integration/staking/keeper/msg_server_test.go (2)
47-47: Ensure theNewValidatorfunction is updated to handle the newConsensusAddressfield as per the changes inx/staking/types/validator.go.Verification successful
The
NewValidatorfunction inx/staking/types/validator.gohas been updated to handle the newConsensusAddressfield. The function signature and implementation confirm that theConsensusAddressis being processed correctly.
x/staking/types/validator.go:41: The function signature includesconsensusAddressCodec address.Codec.x/staking/types/validator.go:54: TheConsensusAddressfield is set in the returnedValidatorstruct.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the `NewValidator` function is updated to handle the new `ConsensusAddress` field. # Test: Search for the function usage. Expect: Only occurrences of the new signature. ast-grep --lang go --pattern $'func NewValidator($_, $_, $_, $_) $_ { $$$ }' src/x/staking/types/validator.goLength of output: 190
Script:
#!/bin/bash # Locate the correct path for the `validator.go` file fd validator.go # Verify that the `NewValidator` function is updated to handle the new `ConsensusAddress` field. ast-grep --lang go --pattern $'func NewValidator($_, $_, $_, $_) $_ { $$$ }' $(fd validator.go)Length of output: 2522
47-47: Ensure theRotateConsPubKeyfunction is updated to handle the newConsensusAddressfield as per the changes inx/staking/types/validator.go.x/distribution/keeper/allocation_test.go (3)
297-297: Ensure theNewValidatorfunction is updated to handle the newConsensusAddressfield as per the changes inx/staking/types/validator.go.Verification successful
The
NewValidatorfunction inx/staking/types/validator.gohas been updated to handle the newConsensusAddressfield.
- The function signature includes
consensusAddressCodec address.Codec.- The function body processes the
ConsensusAddressand assigns it to theValidatorstruct.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the `NewValidator` function is updated to handle the new `ConsensusAddress` field. # Test: Search for the function usage. Expect: Only occurrences of the new signature. ast-grep --lang go --pattern $'func NewValidator($_, $_, $_, $_) $_ { $$$ }' src/x/staking/types/validator.goLength of output: 190
Script:
#!/bin/bash # Locate the correct path for validator.go fd validator.go # Verify that the `NewValidator` function is updated to handle the new `ConsensusAddress` field. ast-grep --lang go --pattern $'func NewValidator($_, $_, $_, $_) $_ { $$$ }' $(fd validator.go)Length of output: 2522
297-297: Ensure theNewValidatorfunction is updated to handle the newConsensusAddressfield as per the changes inx/staking/types/validator.go.Verification successful
The
NewValidatorfunction inx/staking/types/validator.gohas been updated to handle the newConsensusAddressfield. The function signature and implementation confirm that theConsensusAddressis being processed correctly.
x/staking/types/validator.go:54:ConsensusAddressis included in theValidatorstruct.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the `NewValidator` function is updated to handle the new `ConsensusAddress` field. # Test: Search for the function usage. Expect: Only occurrences of the new signature. ast-grep --lang go --pattern $'func NewValidator($_, $_, $_, $_) $_ { $$$ }' src/x/staking/types/validator.goLength of output: 190
Script:
#!/bin/bash # Locate the correct path for the `validator.go` file fd validator.go # Verify that the `NewValidator` function is updated to handle the new `ConsensusAddress` field ast-grep --lang go --pattern $'func NewValidator($_, $_, $_, $_) $_ { $$$ }' $(fd validator.go)Length of output: 2522
297-297: Ensure theNewValidatorfunction is updated to handle the newConsensusAddressfield as per the changes inx/staking/types/validator.go.Verification successful
The
NewValidatorfunction inx/staking/types/validator.gohas been updated to handle the newConsensusAddressfield. The function signature and implementation reflect the changes, ensuring the new field is properly managed.
x/staking/types/validator.go:41:func NewValidator(operator string, pubKey cryptotypes.PubKey, description Description, consensusAddressCodec address.Codec) (Validator, error) {x/staking/types/validator.go:54:ConsensusAddress: consAddressStr,Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the `NewValidator` function is updated to handle the new `ConsensusAddress` field. # Test: Search for the function usage. Expect: Only occurrences of the new signature. ast-grep --lang go --pattern $'func NewValidator($_, $_, $_, $_) $_ { $$$ }' src/x/staking/types/validator.goLength of output: 190
Script:
#!/bin/bash # Locate the file `validator.go` and verify the `NewValidator` function is updated to handle the new `ConsensusAddress` field. # Step 1: Locate the `validator.go` file. validator_file=$(fd validator.go) # Step 2: Verify the `NewValidator` function in the located file. if [ -n "$validator_file" ]; then ast-grep --lang go --pattern $'func NewValidator($_, $_, $_, $_) $_ { $$$ }' "$validator_file" else echo "validator.go file not found." fiLength of output: 1219
Script:
#!/bin/bash # Locate the file `validator.go` and verify the `NewValidator` function is updated to handle the new `ConsensusAddress` field. # Step 1: Locate the `validator.go` files. validator_files=$(fd validator.go) # Step 2: Verify the `NewValidator` function in each located file. for file in $validator_files; do echo "Checking file: $file" ast-grep --lang go --pattern $'func NewValidator($_, $_, $_, $_) $_ { $$$ }' "$file" doneLength of output: 4150
x/staking/keeper/msg_server.go (1)
112-112: Ensure theNewValidatorfunction correctly initializes theConsensusAddress.Verification successful
The
NewValidatorfunction inx/staking/types/validator.gocorrectly initializes theConsensusAddressfield. The consensus address is derived from the public key and converted to a string using the provided codec before being assigned to theConsensusAddressfield.
x/staking/types/validator.go:func NewValidator(operator string, pubKey cryptotypes.PubKey, description Description, consensusAddressCodec address.Codec) (Validator, error) { pkAny, err := codectypes.NewAnyWithValue(pubKey) if err != nil { return Validator{}, err } consAddress := sdk.ConsAddress(pubKey.Address()) consAddressStr, err := consensusAddressCodec.BytesToString(consAddress) if err != nil { return Validator{}, err } return Validator{ OperatorAddress: operator, ConsensusAddress: consAddressStr, ConsensusPubkey: pkAny, Jailed: false, Status: Unbonded, Tokens: math.ZeroInt(), DelegatorShares: math.LegacyZeroDec(), Description: description, UnbondingHeight: int64(0),Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the `ConsensusAddress` is correctly initialized in the `NewValidator` function. # Test: Search for the function usage. Expect: Initialization of `ConsensusAddress`. ast-grep --lang go --pattern $'func NewValidator($_, $_, $_, $_) Validator { $$$ ConsensusAddress: $_, $$$ }'Length of output: 116
Script:
#!/bin/bash # Description: Locate the `NewValidator` function definition and verify the initialization of `ConsensusAddress`. # Step 1: Locate the `NewValidator` function definition. rg 'func NewValidator' --glob '*.go' # Step 2: Verify the initialization of `ConsensusAddress` within the located function. # This will be done by extracting the function definition and checking its content. ast-grep --lang go --pattern 'func NewValidator($_, $_, $_, $_) Validator { $$$ }'Length of output: 1148
Script:
#!/bin/bash # Description: Extract and inspect the `NewValidator` function from `x/staking/types/validator.go`. # Step 1: Extract the `NewValidator` function definition. rg -A 20 'func NewValidator' x/staking/types/validator.goLength of output: 843
tests/integration/distribution/keeper/msg_server_test.go (7)
219-219: LGTM! The test cases for setting withdraw addresses are comprehensive.
219-219: Excellent coverage of scenarios for withdrawing validator commissions.
219-219: Comprehensive testing of community pool funding scenarios.
219-219: Thorough testing for updating distribution parameters.
219-219: Effective testing of community pool spending scenarios.
219-219: Comprehensive testing for depositing into the validator rewards pool.
219-219: Ensure theNewValidatorfunction correctly initializes theConsensusAddress.Verification successful
The
NewValidatorfunction correctly initializes theConsensusAddressas part of its implementation.
- The
ConsensusAddressis derived from the public key and converted to a string using the provided codec.- This initialization is confirmed in the following lines of the function:
consAddress := sdk.ConsAddress(pubKey.Address()) consAddressStr, err := consensusAddressCodec.BytesToString(consAddress)Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the `NewValidator` function initializes the `ConsensusAddress`. # Test: Search for the function usage. Expect: Initialization of `ConsensusAddress`. rg --type go $'NewValidator'Length of output: 21852
Script:
#!/bin/bash # Extract the implementation of the `NewValidator` function from `x/staking/types/validator.go` ast-grep --lang go --pattern $'func NewValidator(operator string, pubKey cryptotypes.PubKey, description Description, consensusAddressCodec address.Codec) (Validator, error) { $$$ }' x/staking/types/validator.goLength of output: 2102
tests/integration/staking/keeper/validator_test.go (1)
Line range hint
15-24: The implementation ofnewMonikerValidatorcorrectly utilizes the updatedNewValidatorfunction to include the consensus address. Good use of assertions for error handling.api/cosmos/staking/v1beta1/staking.pulsar.go (15)
2963-2963: Addedfd_Validator_consensus_addressfield descriptor forConsensusAddress.
2982-2982: Correctly initialized the field descriptor forConsensusAddress.
3128-3133: Properly implemented conditional logic to handleConsensusAddressif it is not empty.
3175-3176: Correctly added a case to check ifConsensusAddressis not empty.
3219-3220: Properly handled the clearing ofConsensusAddressin the default case.
3279-3281: Correctly implemented the getter forConsensusAddress.
3330-3331: Properly implemented the setter forConsensusAddress.
3394-3395: Correctly markedConsensusAddressas not mutable.
3440-3441: Correctly implemented the default getter forConsensusAddress.
3562-3565: Correctly calculated the size ofConsensusAddressfor serialization.
3595-3601: Properly implemented serialization logic forConsensusAddress.
4205-4236: Correctly implemented unmarshalling logic forConsensusAddress, including error handling for wire type and length.
15020-15021: AddedConsensusAddressfield to theValidatorstruct.
15135-15140: Implemented a getter method forConsensusAddress. Ensures that the method returns an empty string if theValidatorinstance is nil, which is a good null safety practice.
Line range hint
16197-16260: The protobuf field definition forConsensusAddressis correctly defined with the appropriate field number and type.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Review Details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (4)
- x/distribution/testutil/staking_helper.go (1 hunks)
- x/staking/keeper/msg_server.go (2 hunks)
- x/staking/simulation/genesis.go (2 hunks)
- x/staking/testutil/validator.go (2 hunks)
Files skipped from review as they are similar to previous changes (4)
- x/distribution/testutil/staking_helper.go
- x/staking/keeper/msg_server.go
- x/staking/simulation/genesis.go
- x/staking/testutil/validator.go
There was a problem hiding this comment.
Actionable comments posted: 0
Review Details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (1)
- x/staking/keeper/grpc_query.go (4 hunks)
Files skipped from review as they are similar to previous changes (1)
- x/staking/keeper/grpc_query.go
There was a problem hiding this comment.
When I understand this correct, you added a new field to the Validator type and lazy set it in the GRPC query handler for the response. I was wondering if a data migration would be more efficient so that you can remove the code in the handler? For new entries the value should be set in the constructor already.
I have noticed that there are also a lot of changes to tests for the constructor. This is not a problem in general but sometimes test fixtures can be very helpful to setup complex objects.
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range and nitpick comments (2)
x/staking/proto/cosmos/staking/v1beta1/query.proto (1)
142-146: Add thesince sdk versioncomment for theValidatorInfomessage.Consider adding a comment indicating the SDK version since which
ValidatorInfois available, as previously suggested byfacundomedica. This helps developers understand version compatibility at a glance.x/staking/keeper/grpc_query.go (1)
16-16: Ensure consistent import ordering.Reorder the imports to group standard library imports, third-party imports, and internal module imports separately for better readability and conformity with the Go coding standards.
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files ignored due to path filters (1)
x/staking/types/query.pb.gois excluded by!**/*.pb.go
Files selected for processing (2)
- x/staking/keeper/grpc_query.go (2 hunks)
- x/staking/proto/cosmos/staking/v1beta1/query.proto (1 hunks)
Additional context used
Path-based instructions (1)
x/staking/keeper/grpc_query.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Co-authored-by: Julien Robert <[email protected]>
Co-authored-by: Julien Robert <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (1)
- x/staking/proto/cosmos/staking/v1beta1/query.proto (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- x/staking/proto/cosmos/staking/v1beta1/query.proto
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files ignored due to path filters (1)
x/staking/types/query.pb.gois excluded by!**/*.pb.go
Files selected for processing (3)
- x/staking/keeper/grpc_query.go (2 hunks)
- x/staking/keeper/grpc_query_test.go (1 hunks)
- x/staking/proto/cosmos/staking/v1beta1/query.proto (1 hunks)
Files skipped from review as they are similar to previous changes (2)
- x/staking/keeper/grpc_query.go
- x/staking/proto/cosmos/staking/v1beta1/query.proto
Additional context used
Path-based instructions (1)
x/staking/keeper/grpc_query_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"
Additional comments not posted (1)
x/staking/keeper/grpc_query_test.go (1)
Line range hint
9-63: The test cases inTestGRPCQueryValidatorare well-structured and effectively cover both negative and positive scenarios. Good use of therequireassertions ensures that the tests are halted at the first failure, which is appropriate for unit testing.
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (3)
x/staking/CHANGELOG.md (3)
31-31: The changelog entry succinctly captures the essence of the PR. It might be beneficial to include a brief description or impact of this addition to provide more context to users.
Line range hint
104-104: Use "an" instead of "a" before a word beginning with a vowel sound.- In a undelegation or redelegation if the shares being left delegated correspond to less than 1 token (in base denom) the entire delegation gets removed. + In an undelegation or redelegation if the shares being left delegated correspond to less than 1 token (in base denom) the entire delegation gets removed.
Line range hint
55-55: Correct the indentation of the unordered list items to maintain consistency and adhere to Markdown best practices.- * remove from `Keeper`: `GetParams`, `SetParams` + * remove from `Keeper`: `GetParams`, `SetParams` - * remove from `types`: `GetRedelegationTimeKey` + * remove from `types`: `GetRedelegationTimeKey` - * remove from `Keeper`: `RedelegationQueueIterator` + * remove from `Keeper`: `RedelegationQueueIterator` - * remove from `types`: `GetLastValidatorPowerKey` + * remove from `types`: `GetLastValidatorPowerKey` - * remove from `Keeper`: `LastValidatorsIterator`, `IterateLastValidators` + * remove from `Keeper`: `LastValidatorsIterator`, `IterateLastValidators` - * remove from `types`: `GetUBDKeyFromValIndexKey`, `GetUBDsByValIndexKey`, `GetUBDByValIndexKey` + * remove from `types`: `GetUBDKeyFromValIndexKey`, `GetUBDsByValIndexKey`, `GetUBDByValIndexKey` - * remove from `Keeper`: `UBDQueueIterator` + * remove from `Keeper`: `UBDQueueIterator` - * remove from `types`: `GetUnbondingDelegationTimeKey` + * remove from `types`: `GetUnbondingDelegationTimeKey` - * remove `Keeper`: `GetHistoricalInfo`, `SetHistoricalInfo` + * remove `Keeper`: `GetHistoricalInfo`, `SetHistoricalInfo` - * remove `Keeper`: `SetLastTotalPower`, `GetLastTotalPower` + * remove `Keeper`: `SetLastTotalPower`, `GetLastTotalPower`Also applies to: 57-57, 58-58, 60-60, 61-61, 63-63, 64-64, 66-66, 68-68, 70-70, 71-71, 74-74, 75-75, 77-77, 79-79, 81-81, 83-83, 85-85, 88-88, 90-90, 92-92, 95-95, 99-99
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (1)
- x/staking/CHANGELOG.md (1 hunks)
Additional context used
Path-based instructions (1)
x/staking/CHANGELOG.md (1)
Pattern
**/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"
LanguageTool
x/staking/CHANGELOG.md
[style] ~96-~96: Consider a shorter alternative to avoid wordiness. (IN_ORDER_TO_PREMIUM)
Context: ...pi/cosmos/staking/v1beta1".Infraction_` in order to remove dependency between modules on st...
[misspelling] ~104-~104: Use “an” instead of ‘a’ if the following word starts with a vowel sound, e.g. ‘an article’, ‘an hour’. (EN_A_VS_AN)
Context: ...ub.com//pull/18841) In a undelegation or redelegation if the sha...
Markdownlint
x/staking/CHANGELOG.md
55-55: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation
57-57: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation
58-58: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation
60-60: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation
61-61: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation
63-63: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation
64-64: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation
66-66: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation
68-68: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation
70-70: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation
71-71: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation
74-74: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation
75-75: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation
77-77: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation
79-79: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation
81-81: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation
83-83: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation
85-85: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation
88-88: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation
90-90: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation
92-92: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation
95-95: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation
99-99: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation
47-47: Expected: 0 or 2; Actual: 1 (MD009, no-trailing-spaces)
Trailing spaces
98-98: Expected: 0 or 2; Actual: 1 (MD009, no-trailing-spaces)
Trailing spaces
100-100: Expected: 0 or 2; Actual: 1 (MD009, no-trailing-spaces)
Trailing spaces
Description
Closes: 19159
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!in the type prefix if API or client breaking changeCHANGELOG.mdReviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
I have...
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Documentation
Chores