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

refactor(staking): move delegation and validator interfaces to ./types #18198

Merged
merged 11 commits into from
Nov 30, 2023

Conversation

tac0turtle
Copy link
Member

@tac0turtle tac0turtle commented Oct 21, 2023

Description

Closes: #17331
Closes: #17330

This pr proposes moving the staking interfaces to the cosmos-sdk as a short term solution to decouple staking from evidence, distribution and others, this allows teams like berachain and cosmos hub to import these types without needing to fork the cosmos sdk


Author Checklist

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

I have...

  • included the correct type prefix in the PR title
  • added ! to the type prefix if API or client breaking change
  • targeted the correct branch (see PR Targeting)
  • provided a link to the relevant issue or specification
  • followed the guidelines for building modules
  • included the necessary unit and integration tests
  • added a changelog entry to CHANGELOG.md
  • included comments for documenting Go code
  • updated the relevant documentation or specification
  • reviewed "Files changed" and left comments if necessary
  • run make lint and make test
  • confirmed all CI checks have passed

Reviewers Checklist

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

I have...

  • confirmed the correct type prefix in the PR title
  • confirmed ! in the type prefix if API or client breaking change
  • confirmed all author checklist items have been addressed
  • reviewed state machine logic
  • reviewed API design and naming
  • reviewed documentation is accurate
  • reviewed tests and test coverage
  • manually tested (if applicable)

Summary by CodeRabbit

  • New Features

    • Search functionality introduced, allowing users to quickly find content within the app.
    • A new search bar is now available at the top of the main page for easy access.
  • Style Updates

    • Implemented new styles for the search bar to enhance visual appeal and user experience.
  • Documentation

    • Updated error messages for a more consistent and user-friendly language.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 21, 2023

Walkthrough

Walkthrough

The changes across various files indicate a significant refactoring effort to decouple certain modules from x/staking, specifically x/evidence and x/slashing. This is achieved by replacing stakingtypes with sdk types and updating function signatures and logic to accommodate these new types. The updates also include changes to error messages, status constants, and the introduction of new interfaces and constants to streamline the codebase and reduce dependencies.

Changes

File Pattern Change Summary
math/int.go, simapp/export.go, tests/integration/... Updated error messages and types for validators and statuses.
types/staking.go, x/distribution/..., x/evidence/..., x/gov/..., x/slashing/..., x/staking/... Replaced stakingtypes with sdk types, updated function signatures, and introduced new interfaces/constants.
x/staking/CHANGELOG.md Documented breaking changes related to API updates in the changelog.

Assessment against linked issues

Objective Addressed Explanation
Decouple x/evidence from x/staking (#17331) The changes remove direct dependencies on x/staking by replacing stakingtypes with sdk types.
Decouple x/slashing from x/staking (#17330) Similar to x/evidence, the updates show a shift from stakingtypes to sdk types, aligning with the decoupling objective.

Tips

Chat with CodeRabbit Bot (@coderabbitai)

  • If you reply to a review comment from CodeRabbit, the bot will automatically respond.
  • To engage with CodeRabbit bot directly around the specific lines of code in the PR, mention @coderabbitai in your review comment
  • Note: Review comments are made on code diffs or files, not on the PR overview.
  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Commands (invoked as PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger a review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai help to get help.

Note: For conversation with the bot, please use the review comments on code diffs or files.

CodeRabbit Configration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • The JSON schema for the configuration file is available here.
  • 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/coderabbit-overrides.v2.json

@tac0turtle tac0turtle changed the title move staking interfaces from staking to types to decouple them refactor(staking): move delegation and validator interfaces to ./types Oct 21, 2023
@tac0turtle tac0turtle added the S:blocked Status: Blocked label Oct 22, 2023
@itsdevbear
Copy link
Contributor

fire

@tac0turtle tac0turtle removed the S:blocked Status: Blocked label Nov 29, 2023
@tac0turtle tac0turtle marked this pull request as ready for review November 29, 2023 16:54
@tac0turtle tac0turtle requested a review from a team as a code owner November 29, 2023 16:54
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.

Review Status

Actionable comments generated: 6

Configuration used: CodeRabbit UI

Commits Files that changed from the base of the PR and between a37f416 and 28cca68.
Files selected for processing (34)
  • math/int.go (1 hunks)
  • simapp/export.go (2 hunks)
  • tests/integration/slashing/keeper/keeper_test.go (2 hunks)
  • tests/integration/staking/keeper/slash_test.go (3 hunks)
  • tests/integration/staking/keeper/validator_test.go (2 hunks)
  • types/staking.go (2 hunks)
  • x/distribution/keeper/allocation.go (2 hunks)
  • x/distribution/keeper/delegation.go (4 hunks)
  • x/distribution/keeper/grpc_query.go (3 hunks)
  • x/distribution/keeper/invariants.go (3 hunks)
  • x/distribution/keeper/validator.go (2 hunks)
  • x/distribution/testutil/expected_keepers_mocks.go (5 hunks)
  • x/distribution/types/expected_keepers.go (1 hunks)
  • x/evidence/testutil/expected_keepers_mocks.go (14 hunks)
  • x/evidence/types/expected_keepers.go (2 hunks)
  • x/gov/keeper/tally.go (3 hunks)
  • x/gov/keeper/tally_test.go (2 hunks)
  • x/gov/testutil/expected_keepers_mocks.go (33 hunks)
  • x/gov/types/expected_keepers.go (2 hunks)
  • x/slashing/abci_test.go (2 hunks)
  • x/slashing/keeper/genesis.go (2 hunks)
  • x/slashing/testutil/expected_keepers_mocks.go (4 hunks)
  • x/slashing/types/expected_keepers.go (2 hunks)
  • x/staking/keeper/alias_functions.go (4 hunks)
  • x/staking/keeper/delegation.go (1 hunks)
  • x/staking/keeper/genesis.go (1 hunks)
  • x/staking/keeper/grpc_query.go (1 hunks)
  • x/staking/keeper/invariants.go (1 hunks)
  • x/staking/keeper/msg_server.go (1 hunks)
  • x/staking/keeper/slash.go (1 hunks)
  • x/staking/testutil/expected_keepers_mocks.go (6 hunks)
  • x/staking/types/delegation.go (1 hunks)
  • x/staking/types/expected_keepers.go (3 hunks)
  • x/staking/types/validator.go (4 hunks)
Files skipped from review due to trivial changes (3)
  • math/int.go
  • x/staking/types/delegation.go
  • x/staking/types/expected_keepers.go
Additional comments: 105
simapp/export.go (2)
  • 78-81: The change from stakingtypes.ValidatorI to sdk.ValidatorI is consistent with the summary and the rest of the code in the hunk appears to be compatible with this change. The use of panic for error handling is appropriate in this context.

  • 124-127: The use of sdk.ValidatorI in the IterateValidators function is consistent with the changes described in the summary. The error handling and logic within the hunk are appropriate and follow the Cosmos SDK's conventions.

tests/integration/slashing/keeper/keeper_test.go (2)
  • 279-283: The changes in TestHandleNewValidator correctly reflect the use of sdk.Bonded instead of stakingtypes.Bonded. This aligns with the refactor to use constants from the sdk package.

  • 337-341: The changes in TestHandleAlreadyJailed correctly reflect the use of sdk.Unbonding instead of stakingtypes.Unbonding. This aligns with the refactor to use constants from the sdk package.

tests/integration/staking/keeper/slash_test.go (4)
  • 384-387: The code correctly reflects the changes mentioned in the summary, where validator.GetStatus() now returns sdk.Unbonding instead of types.Unbonding.

  • 384-387: The use of sdk.Unbonding in the assertions is consistent with the refactor mentioned in the summary, indicating that the validator status is now being accessed through the sdk package.

  • 384-387: The logic for checking the validator's status after a slash operation appears to be unchanged and consistent with the expected behavior of the staking module.

  • 384-387: The test logic in slash_test.go correctly uses the updated sdk.Unbonding status, reflecting the changes made in the refactor.

tests/integration/staking/keeper/validator_test.go (3)
  • 110-112: The change from types.BondStatus to sdk.BondStatus in expectedValStatus is correctly reflected in the code.

  • 124-124: The update of the format specifier from %s to %x in the fmt.Sprintf call for the status variable is correctly reflected in the code.

  • 107-115: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [107-126]

No additional issues found in the provided hunks. The changes made are consistent and do not introduce any new logic, security, or performance concerns.

types/staking.go (4)
  • 3-9: The import changes are consistent with the summary provided.

  • 34-42: The changes to TokensToConsensusPower and TokensFromConsensusPower functions to use math.Int are consistent with the summary.

  • 61-91: The addition of DelegationI and ValidatorI interfaces is consistent with the summary and the pull request description.

  • 61-91: Verify that the new interfaces are properly integrated and do not break existing functionality throughout the codebase.

x/distribution/keeper/allocation.go (2)
  • 86-86: The change in the parameter type of AllocateTokensToValidator from stakingtypes.ValidatorI to sdk.ValidatorI is consistent with the refactor and should not cause issues as long as all calls to this function are updated accordingly.

  • 8-13: The removal of the import "cosmossdk.io/x/staking/types" is consistent with the refactor and is not present in the provided file, which aligns with the summary.

x/distribution/keeper/delegation.go (11)
  • 8-13: The removal of stakingtypes and the update to use sdk package types in the function signatures are correctly reflected in the code.

  • 49-52: The logic and control flow within calculateDelegationRewardsBetween function appear to be correctly adjusted to accommodate the new sdk.ValidatorI type.

  • 88-91: The logic and control flow within CalculateDelegationRewards function appear to be correctly adjusted to accommodate the new sdk.ValidatorI and sdk.DelegationI types.

  • 198-201: The logic and control flow within withdrawDelegationRewards function appear to be correctly adjusted to accommodate the new sdk.ValidatorI and sdk.DelegationI types.

  • 198-201: The error handling within withdrawDelegationRewards function is consistent with the previous implementation and correctly uses errors.Is for checking specific error types.

  • 49-52: No performance issues are evident in the calculateDelegationRewardsBetween function due to the type changes.

  • 88-91: No performance issues are evident in the CalculateDelegationRewards function due to the type changes.

  • 198-201: No performance issues are evident in the withdrawDelegationRewards function due to the type changes.

  • 49-52: No new potential data races are introduced in the calculateDelegationRewardsBetween function due to the type changes.

  • 88-91: No new potential data races are introduced in the CalculateDelegationRewards function due to the type changes.

  • 198-201: No new potential data races are introduced in the withdrawDelegationRewards function due to the type changes.

x/distribution/keeper/grpc_query.go (1)
  • 267-272: The change from stakingtypes.DelegationI to sdk.DelegationI in the IterateDelegations function call is consistent with the summary. Ensure that all implementations of DelegationI are compatible with the new sdk.DelegationI interface.
x/distribution/keeper/invariants.go (3)
  • 5-10: The removal of the stakingtypes import is consistent with the summary and the changes made in the codebase.

  • 92-95: The change from stakingtypes.ValidatorI to sdk.ValidatorI in the IterateValidators function call aligns with the summary and the refactor's goal of decoupling modules.

  • 134-136: The change from stakingtypes.ValidatorI to sdk.ValidatorI in the IterateValidators function call within the ReferenceCountInvariant function is consistent with the refactor's objectives.

x/distribution/keeper/validator.go (2)
  • 17-20: The change from stakingtypes.ValidatorI to sdk.ValidatorI in the initializeValidator function signature is consistent with the summary.

  • 46-49: The change from stakingtypes.ValidatorI to sdk.ValidatorI in the IncrementValidatorPeriod function signature is consistent with the summary.

x/distribution/testutil/expected_keepers_mocks.go (5)
  • 347-354: The changes to the Delegation method correctly reflect the updates mentioned in the summary, where types.DelegationI has been replaced with types0.DelegationI.

  • 407-411: The changes to the IterateDelegations method correctly reflect the updates mentioned in the summary, where types.DelegationI has been replaced with types0.DelegationI.

  • 421-425: The changes to the IterateValidators method correctly reflect the updates mentioned in the summary, where types.ValidatorI has been replaced with types0.ValidatorI.

  • 435-442: The changes to the Validator method correctly reflect the updates mentioned in the summary, where types.ValidatorI has been replaced with types0.ValidatorI.

  • 464-471: The changes to the ValidatorByConsAddr method correctly reflect the updates mentioned in the summary, where types.ConsAddress has been replaced with types0.ConsAddress.

x/distribution/types/expected_keepers.go (3)
  • 63-63: The summary states that the return type of GetAllSDKDelegations has been changed to return an array of stakingtypes.Delegation instead of sdk.Delegation, but the hunk shows that the function still returns stakingtypes.Delegation. Please verify if the summary or the code needs to be updated.

  • 64-64: The summary states that the return type of GetAllValidators has been changed to return an array of stakingtypes.Validator instead of sdk.Validator, but the hunk shows that the function still returns stakingtypes.Validator. Please verify if the summary or the code needs to be updated.

  • 53-58: The changes to the StakingKeeper interface to use sdk types instead of stakingtypes for the Validator, ValidatorByConsAddr, and Delegation functions are consistent with the summary and reflect the intended refactor.

x/evidence/testutil/expected_keepers_mocks.go (14)
  • 12-18: The import paths have been correctly updated to reflect the changes in the Cosmos SDK structure.

  • 57-63: The function signature of ValidatorByConsAddr has been correctly updated to use the new types.

  • 95-101: The function signature of GetPubkey has been correctly updated to use the new types.

  • 110-114: The function signature of HasValidatorSigningInfo has been correctly updated to use the new types.

  • 124-128: The function signature of IsTombstoned has been correctly updated to use the new types.

  • 138-142: The function signature of Jail has been correctly updated to use the new types.

  • 152-156: The function signature of JailUntil has been correctly updated to use the new types.

  • 166-170: The function signature of Slash has been correctly updated to use the new types.

  • 195-199: The function signature of SlashWithInfractionReason has been correctly updated to use the new types.

  • 209-213: The function signature of Tombstone has been correctly updated to use the new types.

  • 246-249: The function signature of SetAccount in MockAccountKeeper has been correctly updated to use the new types.

  • 281-286: The function signature of GetAllBalances in MockBankKeeper has been correctly updated to use the new types.

  • 295-299: The function signature of MintCoins in MockBankKeeper has been correctly updated to use the new types.

  • 309-313: The function signature of SendCoinsFromModuleToAccount in MockBankKeeper has been correctly updated to use the new types.

x/evidence/types/expected_keepers.go (2)
  • 7-12: The removal of the import "cosmossdk.io/x/staking/types" aligns with the summary and the refactor's goal to decouple modules.

  • 20-20: The change in the return type of ValidatorByConsAddr() from stakingtypes.ValidatorI to sdk.ValidatorI is consistent with the summary and the refactor's objectives.

x/gov/keeper/tally.go (3)
  • 6-11: The removal of the import "cosmossdk.io/x/staking/types" is consistent with the summary.

  • 28-28: The function signature for IterateBondedValidatorsByPower correctly uses sdk.ValidatorI as per the summary.

  • 65-65: The function signature for IterateDelegations correctly uses sdk.DelegationI as per the summary.

x/gov/keeper/tally_test.go (2)
  • 43-49: The change in the type of the second parameter in the function passed to IterateDelegations from stakingtypes.DelegationI to sdk.DelegationI is correctly reflected in the code.

  • 382-388: The change in the type of the second parameter in the function passed to IterateBondedValidatorsByPower from stakingtypes.ValidatorI to sdk.ValidatorI is correctly reflected in the code.

x/gov/testutil/expected_keepers_mocks.go (3)
  • 12-18: The import paths have been correctly updated to reflect the new location of the types in the github.com/cosmos/cosmos-sdk/types package.

  • 69-78: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [58-1041]

The function signatures in the mocks have been correctly updated to use types from the types0 package, ensuring consistency with the new import paths.

  • 55-64: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [12-1041]

The summary mentions the replacement of types1 with types0, but the hunks do not show any previous usage of types1. Please verify if types1 was indeed used before and whether all necessary replacements have been made.

x/gov/types/expected_keepers.go (3)
  • 5-10: The removal of the stakingtypes import aligns with the summary and the changes in the function signatures.

  • 14-18: The change in the validator parameter type from stakingtypes.ValidatorI to sdk.ValidatorI in the IterateBondedValidatorsByPower function is consistent with the summary.

  • 20-24: The change in the delegation parameter type from stakingtypes.DelegationI to sdk.DelegationI in the IterateDelegations function is consistent with the summary.

x/slashing/abci_test.go (1)
  • 119-122: The change from stakingtypes.Unbonding to sdk.Unbonding is consistent with the summary and reflects the refactor within the Cosmos SDK to improve modularity. Ensure that all references to the Unbonding status in the codebase are updated to use sdk.Unbonding.
x/slashing/keeper/genesis.go (3)
  • 4-9: The import statement for stakingtypes "cosmossdk.io/x/staking/types" has been correctly removed as it is no longer needed.

  • 14-18: The IterateValidators function signature has been updated to use sdk.ValidatorI instead of stakingtypes.ValidatorI, which aligns with the refactor to move interfaces to the types package.

  • 12-18: Verify that all references to the old stakingtypes.ValidatorI and stakingtypes.DelegationI interfaces have been updated throughout the entire codebase to use the new sdk.ValidatorI and sdk.DelegationI interfaces.

x/slashing/testutil/expected_keepers_mocks.go (4)
  • 198-204: The change in the return type of the Delegation function to types0.DelegationI is consistent with the summary.

  • 244-247: The change in the parameter type of the callback function in IterateValidators to types0.ValidatorI is consistent with the summary.

  • 331-336: The change in the return type of the Validator function to types0.ValidatorI is consistent with the summary.

  • 360-365: The summary states that the parameter type of the ValidatorByConsAddr function has been changed, but the hunk shows a change in the return type to types0.ValidatorI. Please verify this discrepancy.

- func (m *MockStakingKeeper) ValidatorByConsAddr(arg0 context.Context, arg1 types0.ConsAddress) (types0.ValidatorI, error) {
+ func (m *MockStakingKeeper) ValidatorByConsAddr(arg0 context.Context, arg1 types0.ValidatorI) (types0.ValidatorI, error) {
x/slashing/types/expected_keepers.go (2)
  • 34-41: The changes to the StakingKeeper interface to use sdk types instead of stakingtypes types are consistent with the summary and the broader refactor goals.

  • 49-49: The return type of GetAllValidators should be []sdk.ValidatorI to align with the refactor's goal of using sdk types instead of stakingtypes types. The current return type []stakingtypes.Validator is inconsistent with the summary and the refactor's objectives.

- GetAllValidators(ctx context.Context) ([]stakingtypes.Validator, error)
+ GetAllValidators(ctx context.Context) ([]sdk.ValidatorI, error)
x/staking/keeper/alias_functions.go (4)
  • 16-19: The changes to the function signature of IterateValidators match the summary and the refactor's intent.

  • 43-46: The changes to the function signature of IterateBondedValidatorsByPower match the summary and the refactor's intent.

  • 74-80: The changes to the return type of Validator and ValidatorByConsAddr match the summary and the refactor's intent.

  • 96-100: The changes to the function signature of IterateDelegations match the summary and the refactor's intent.

x/staking/keeper/delegation.go (1)
  • 1080-1080: The summary states that BeginRedelegation has been modified to use types.BondStatus to convert the status obtained from srcValidator.GetStatus() before passing it to the Delegate function. However, the code does not show any conversion using types.BondStatus. Please verify if the code reflects the intended changes or if the summary needs to be updated.
x/staking/keeper/genesis.go (1)
  • 74-81: The update in the switch statement to use sdk.Bonded, sdk.Unbonding, and sdk.Unbonded is consistent with the summary and the broader refactor to use the sdk package for statuses. This change should be verified across the entire codebase to ensure that all references to these constants are updated accordingly.
x/staking/keeper/grpc_query.go (1)
  • 46-49: The change from val.GetStatus().String() to string(val.GetStatus()) in the comparison logic should be verified to ensure that the Status field of the Validator type is correctly compared as a string. This is a critical change as it affects how validators are filtered based on their status.
x/staking/keeper/invariants.go (5)
  • 61-68: The changes to the IterateValidators function and the use of sdk package statuses (sdk.Bonded, sdk.Unbonding, sdk.Unbonded) are consistent with the summary and the pull request description. This should ensure that the ModuleAccountInvariants function works with the updated interfaces and types.

  • 58-58: The use of panic for error handling when the bond denomination cannot be fetched is appropriate here, as it indicates a critical failure that should halt the program execution.

  • 68-68: The use of panic in case of an invalid validator status is also appropriate, as it indicates a critical and unexpected state that should not occur under normal operation.

  • 62-67: The update of the validator status checks to use the sdk package statuses (sdk.Bonded, sdk.Unbonding, sdk.Unbonded) aligns with the refactor described in the summary and pull request.

  • 63-66: The logic for updating the bonded and notBonded variables based on the validator's status and tokens appears to be correct and unchanged, which is expected as the summary does not mention any logic changes in this area.

x/staking/keeper/msg_server.go (1)
  • 645-650: The changes to the RotateConsPubKey function are consistent with the summary and the pull request description. The use of sdk.Bonded aligns with the refactor to access constants through the sdk package, and the error message now includes the status value using %x format specifier, which is suitable for logging purposes.
x/staking/keeper/slash.go (3)
  • 184-192: The code correctly uses sdk.Bonded, sdk.Unbonding, and sdk.Unbonded statuses from the sdk package as per the summary. This change aligns with the refactor to use the sdk package for status constants.

  • 189-189: The summary does not mention the use of sdk.Unbonded, but the code has been updated to include this status as well. This might be an oversight in the summary.

  • 182-192: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [1-191]

The rest of the file is not annotated with changes, so it is assumed to be consistent with the refactor to use the sdk package for status constants and other related changes.

x/staking/testutil/expected_keepers_mocks.go (6)
  • 310-315: The change in the return type of the Delegation method from types.DelegationI to types0.DelegationI is consistent with the summary.

  • 340-343: The change in the parameter type of the callback function in IterateBondedValidatorsByPower from types.ValidatorI to types0.ValidatorI aligns with the summary.

  • 354-357: The change in the parameter type of the callback function in IterateValidators from types.ValidatorI to types0.ValidatorI is in accordance with the summary.

  • 471-476: The change in the return type of the Validator method from types.ValidatorI to types0.ValidatorI matches the summary.

  • 486-491: The change in the return type of the ValidatorByConsAddr method from types.ValidatorI to types0.ValidatorI is consistent with the summary.

  • 538-541: The change in the parameter type of the callback function in IterateDelegations from types.DelegationI to types0.DelegationI aligns with the summary.

x/staking/types/validator.go (4)
  • 41-41: The implementation of the sdk.ValidatorI interface by Validator is correctly updated to reflect the new interface location.

  • 80-84: The ToSDKValidators method correctly converts a slice of Validator to a slice of sdk.ValidatorI, aligning with the new interface location.

  • 174-187: The methods IsBonded, IsUnbonded, and IsUnbonding are correctly using the sdk package constants for status comparison.

  • 461-461: The GetStatus method's return type is correctly updated to sdk.BondStatus, reflecting the new type location.

x/staking/keeper/genesis.go Show resolved Hide resolved
x/distribution/keeper/grpc_query.go Show resolved Hide resolved
x/distribution/keeper/grpc_query.go Show resolved Hide resolved
Comment on lines 13 to 19
// Validator Set

// IterateValidators iterates through the validator set and perform the provided function
func (k Keeper) IterateValidators(ctx context.Context, fn func(index int64, validator types.ValidatorI) (stop bool)) error {
func (k Keeper) IterateValidators(ctx context.Context, fn func(index int64, validator sdk.ValidatorI) (stop bool)) error {
store := k.storeService.OpenKVStore(ctx)
iterator, err := store.Iterator(types.ValidatorsKey, storetypes.PrefixEndBytes(types.ValidatorsKey))
if err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

Note: This review was outside the patches, and no patch overlapping with it was found. Original lines [26-26]

The comment "XXX is this safe will the validator unexposed fields be able to get written to?" should be addressed or removed if it's no longer relevant. It's important to ensure that the code is clean and free of outdated or unnecessary comments.


Note: This review was outside the patches, and no patch overlapping with it was found. Original lines [53-53]

Similarly, the comment "XXX is this safe will the validator unexposed fields be able to get written to?" in IterateBondedValidatorsByPower should be addressed or removed if it's no longer relevant.

Comment on lines 25 to 31
currValidators := make(map[string]v1.ValidatorGovInfo)

// fetch all the bonded validators, insert them into currValidators
err = keeper.sk.IterateBondedValidatorsByPower(ctx, func(index int64, validator stakingtypes.ValidatorI) (stop bool) {
err = keeper.sk.IterateBondedValidatorsByPower(ctx, func(index int64, validator sdk.ValidatorI) (stop bool) {
valBz, err := keeper.sk.ValidatorAddressCodec().StringToBytes(validator.GetOperator())
if err != nil {
return false
Copy link
Contributor

Choose a reason for hiding this comment

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

Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [6-31]

Consider refactoring the Tally function to improve maintainability and reduce complexity as indicated by the existing TODO comment.

Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
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.

Review Status

Actionable comments generated: 1

Configuration used: CodeRabbit UI

Commits Files that changed from the base of the PR and between 28cca68 and 15053e8.
Files selected for processing (1)
  • x/staking/keeper/genesis.go (1 hunks)

Comment on lines 82 to 83
panic(fmt.Sprintf("invalid validator status: %v", validator.GetStatus()))
panic("invalid validator status")
Copy link
Contributor

Choose a reason for hiding this comment

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

There is a duplicate panic call in the default case of the switch statement. The second panic call should be removed as it is redundant and the first panic call already includes the invalid status value.

82  panic(fmt.Sprintf("invalid validator status: %v", validator.GetStatus()))
-  panic("invalid validator status")

Committable suggestion

IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Suggested change
panic(fmt.Sprintf("invalid validator status: %v", validator.GetStatus()))
panic("invalid validator status")
panic(fmt.Sprintf("invalid validator status: %v", validator.GetStatus()))

types/staking.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.

Review Status

Actionable comments generated: 0

Configuration used: CodeRabbit UI

Commits Files that changed from the base of the PR and between 15053e8 and edf3adc.
Files selected for processing (1)
  • types/staking.go (2 hunks)
Additional comments: 5
types/staking.go (5)
  • 3-9: The import changes are consistent with the summary provided. The sdkmath package has been replaced with cosmossdk.io/math, and new packages cmtprotocrypto and cryptotypes have been added.

  • 34-42: The function signatures for TokensToConsensusPower and TokensFromConsensusPower have been correctly updated to use math.Int as per the summary.

  • 47-91: The new constants and interfaces BondStatus, DelegationI, and ValidatorI have been added as described in the summary.

  • 47-91: The new interfaces and constants are well-defined and follow the Cosmos SDK design patterns.

  • 61-91: The methods within the new interfaces DelegationI and ValidatorI are correctly typed and align with the expected functionality within the Cosmos SDK.

Copy link
Member

@julienrbrt julienrbrt left a comment

Choose a reason for hiding this comment

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

lgtm! Can we get a changelog (./ or ./x/staking 🤷🏾‍♂️) given that there is an import change ?

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.

Review Status

Actionable comments generated: 0

Configuration used: CodeRabbit UI

Commits Files that changed from the base of the PR and between 4a67e5d and 0f72142.
Files selected for processing (1)
  • x/staking/keeper/genesis.go (1 hunks)
Additional comments: 1
x/staking/keeper/genesis.go (1)
  • 74-83: The changes to the InitGenesis function correctly reflect the updates mentioned in the summary, including the use of sdk statuses and the improved panic message in the default case of the switch statement.

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.

Review Status

Actionable comments generated: 0

Configuration used: CodeRabbit UI

Commits Files that changed from the base of the PR and between 0f72142 and a3bf1da.
Files selected for processing (1)
  • types/staking.go (2 hunks)
Additional comments: 4
types/staking.go (4)
  • 3-9: The import changes are consistent with the summary provided.

  • 35-42: The function signatures for TokensToConsensusPower and TokensFromConsensusPower have been correctly updated to use math.Int.

  • 47-91: The addition of the BondStatus constant type and the DelegationI and ValidatorI interfaces align with the summary and the pull request's goal of decoupling modules.

  • 47-91: The newly added interfaces and constants are well-defined and follow Go conventions.

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.

Review Status

Actionable comments generated: 0

Configuration used: CodeRabbit UI

Commits Files that changed from the base of the PR and between a3bf1da and 2408dd4.
Files selected for processing (2)
  • tests/integration/staking/keeper/validator_test.go (3 hunks)
  • types/staking.go (2 hunks)
Additional comments: 8
tests/integration/staking/keeper/validator_test.go (3)
  • 107-108: The summary states that types.BondStatus has been replaced with sdk.BondStatus, but the code still uses types.Validator. Please verify if types.Validator should also be replaced with sdk.Validator to maintain consistency with the changes described.

  • 124-124: The change from %s to %x in fmt.Sprintf for the status variable is consistent with the summary and is a valid change if the intention is to print the status as a hexadecimal number.

  • 158-158: The expected status of the validator has been changed from types.Unbonded to sdk.Unbonding. This change is not mentioned in the summary but seems to be in line with the other changes where types.BondStatus is replaced with sdk.BondStatus. Please ensure that this change is intentional and correctly reflects the desired test behavior.

types/staking.go (5)
  • 3-9: The import changes are consistent with the summary provided.

  • 35-36: The update to the TokensToConsensusPower function signature is consistent with the summary.

  • 40-41: The update to the TokensFromConsensusPower function signature is consistent with the summary.

  • 47-103: The addition of new constants and interfaces aligns with the summary.

  • 47-103: Please verify that the new constants and interfaces do not conflict with any existing ones in the codebase.

@tac0turtle tac0turtle added this pull request to the merge queue Nov 30, 2023
Merged via the queue into main with commit a7ececc Nov 30, 2023
62 of 63 checks passed
@tac0turtle tac0turtle deleted the marko/decouple_staking branch November 30, 2023 21:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature]: Decouple x/evidence from x/staking [Feature]: Decouple x/slashing from x/staking
5 participants