Implement x/govplus MsgSlashValidator #1016
Conversation
WalkthroughThe overall change introduces and enhances governance functionalities within the dydxprotocol, specifically focusing on the ability to slash misbehaving validators through a new governance module, Changes
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 (
|
| @@ -1,851 +0,0 @@ | |||
| package keeper_test | |||
There was a problem hiding this comment.
Delete. Accidentally copied from x/ratelimit earlier.
| @@ -6,5 +6,5 @@ const ( | |||
| ModuleName = "govplus" | |||
|
|
|||
| // StoreKey defines the primary module store key | |||
| StoreKey = ModuleName | |||
| StoreKey = "dydxgovplus" | |||
There was a problem hiding this comment.
Store keys can't be prefixes of another apparently, so I can't use govplus with gov.
There was a problem hiding this comment.
Looks likes break the convention that StoreKey = ModuleName. Should we just update module name to dydxgovplus (and update directory names accordingly)?
There was a problem hiding this comment.
May be worth adding a comment here to reflect that
There was a problem hiding this comment.
Feel like it's not a particularly worthwhile convention to keep. For example not all cosmos modules even follow the convention: https://github.com/dydxprotocol/cosmos-sdk/blob/dydx-fork-v0.50.3/x/auth/types/keys.go#L12
I'd rather have a nicer module name than try to keep everything the same.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files ignored due to path filters (1)
protocol/x/govplus/types/tx.pb.gois excluded by:!**/*.pb.go
Files selected for processing (19)
- indexer/packages/v4-protos/src/codegen/dydxprotocol/bundle.ts (8 hunks)
- indexer/packages/v4-protos/src/codegen/dydxprotocol/govplus/tx.rpc.msg.ts (1 hunks)
- indexer/packages/v4-protos/src/codegen/dydxprotocol/govplus/tx.ts (1 hunks)
- indexer/packages/v4-protos/src/codegen/dydxprotocol/rpc.tx.ts (1 hunks)
- proto/dydxprotocol/govplus/tx.proto (1 hunks)
- protocol/app/app.go (2 hunks)
- protocol/app/basic_manager/basic_manager.go (2 hunks)
- protocol/app/msgs/all_msgs.go (1 hunks)
- protocol/app/msgs/internal_msgs.go (2 hunks)
- protocol/app/msgs/internal_msgs_test.go (1 hunks)
- protocol/app/simulation_test.go (4 hunks)
- protocol/lib/ante/internal_msg.go (2 hunks)
- protocol/testutil/app/app.go (3 hunks)
- protocol/x/govplus/keeper/keeper.go (1 hunks)
- protocol/x/govplus/keeper/msg_server.go (2 hunks)
- protocol/x/govplus/keeper/msg_server_test.go (2 hunks)
- protocol/x/govplus/types/errors.go (1 hunks)
- protocol/x/govplus/types/expected_keepers.go (1 hunks)
- protocol/x/govplus/types/keys.go (1 hunks)
Files skipped from review due to trivial changes (1)
- protocol/app/simulation_test.go
Additional comments: 24
protocol/x/govplus/types/keys.go (1)
- 9-9: The change from
ModuleNameto"dydxgovplus"forStoreKeyis clear and improves specificity for thegovplusmodule's store key. Ensure any data previously stored under the old key is migrated or accessed correctly with the new key.protocol/x/govplus/types/errors.go (1)
- 8-12: Introduced a new error
ErrValidatorAddressfor handling bech32 address conversion issues. This is a good practice for specific error handling and improves the module's robustness.protocol/x/govplus/types/expected_keepers.go (1)
- 10-12: Introduced
StakingKeeperinterface with aSlashmethod, enabling thegovplusmodule to interact with staking logic for slashing validators. This is a necessary addition for the module's functionality.indexer/packages/v4-protos/src/codegen/dydxprotocol/govplus/tx.rpc.msg.ts (1)
- 6-25: Implemented the
slashValidatormethod in theMsgservice interface andMsgClientImplclass for thegovplusmodule, enabling slashing of validators through governance. The implementation follows standard practices for RPC services in TypeScript.protocol/x/govplus/keeper/keeper.go (2)
- 16-18: Added
stakingKeeperfield to theKeeperstruct to enable interaction with staking logic, particularly for slashing validators. This addition is correctly implemented and necessary for the module's functionality.- 25-33: Updated
NewKeeperfunction to acceptstakingKeeperas a parameter and correctly initialize theKeeperstruct with it. This change is essential for integrating staking logic into thegovplusmodule.indexer/packages/v4-protos/src/codegen/dydxprotocol/rpc.tx.ts (1)
- 13-13: Updated the
createRPCMsgClientfunction to include thegovplusmodule, enabling RPC interactions with the module's services. This change is correctly implemented and aligns with the module's introduction.protocol/x/govplus/keeper/msg_server.go (1)
- 25-59: Implemented the
SlashValidatormethod in themsgServertype, handling the logic for slashing validators. The method includes proper error handling and calls thestakingKeeper.Slashmethod. Note the use ofpanicon slashing error, which could halt the application but might be intended to ensure immediate attention to slashing errors.protocol/x/govplus/keeper/msg_server_test.go (1)
- 29-68: Added
TestSlashValidator_InvalidAuthoritytest function to cover various input cases for theSlashValidatormethod. The test cases are well-structured and enhance the module's test coverage, particularly for error handling scenarios.proto/dydxprotocol/govplus/tx.proto (1)
- 12-55: Introduced the
SlashValidatorRPC method and message typesMsgSlashValidatorandMsgSlashValidatorResponsein thetx.protofile. The definitions are well-structured, with clear documentation and appropriate use of custom types for specific fields. This addition is crucial for the slashing functionality in thegovplusmodule.protocol/lib/ante/internal_msg.go (1)
- 92-93: Added
govplus.MsgSlashValidatorto theIsInternalMsgfunction, correctly categorizing it as an internal message. This change aligns with the governance functionality of thegovplusmodule.protocol/app/basic_manager/basic_manager.go (1)
- 31-31: Added
govplusmoduleto theModuleBasicsmanager, integrating thegovplusmodule into the application's basic module manager. This change is necessary for the module's lifecycle management within the application.protocol/app/msgs/internal_msgs_test.go (1)
- 100-102: Updated test cases in
internal_msgs_test.goto includeMsgSlashValidatorandMsgSlashValidatorResponse, ensuring they are correctly categorized as internal messages. This addition maintains comprehensive test coverage for internal message categorization.indexer/packages/v4-protos/src/codegen/dydxprotocol/govplus/tx.ts (2)
- 5-35: Updated the
MsgSlashValidatorinterface and addedMsgSlashValidatorSDKTypeto align with protobuf naming conventions. Also, implemented encoding and decoding functions, enabling TypeScript-based interactions with thegovplusmodule's slashing functionality.- 73-73: No changes were made to the
MsgSlashValidatorResponseinterface, but it's correctly defined to match the protobuf definition.indexer/packages/v4-protos/src/codegen/dydxprotocol/bundle.ts (2)
- 129-137: Reorganized imports and updated the
dydxprotocolnamespace to include additional entities such asgovplus,indexer,prices, and others. This change improves the structure and expands the protocol's functionality within the TypeScript codebase.- 309-311: Expanded the
ClientFactoryentity within thedydxprotocolnamespace to include a new import, extending its capabilities. This change is consistent with the protocol's functionality expansion.protocol/app/msgs/internal_msgs.go (1)
- 24-24: The import of the
govplusmodule is correctly added to support the new message types for slashing validators.protocol/app/msgs/all_msgs.go (1)
- 193-195: The addition of
MsgSlashValidatorandMsgSlashValidatorResponsetoAllTypeMessagescorrectly registers these new message types for thegovplusmodule. This is necessary for the app'sInterfaceRegistryto recognize and handle these messages appropriately. The syntax and structure of the additions are correct.protocol/testutil/app/app.go (3)
- 58-58: The import of the
govpluspackage aligns with the PR's objective to integrate thegovplusmodule into the application's governance mechanisms.- 202-203: The addition of
govplus.GenesisStateto theGenesisStatesinterface is necessary for supporting the newgovplusmodule's genesis state in the application's test utilities. This change ensures that thegovplusmodule's genesis state can be manipulated and tested like other modules' genesis states.- 255-256: The handling of
govplus.GenesisStatein theUpdateGenesisDocWithAppStateForModulefunction is crucial for enabling tests to modify thegovplusmodule's genesis state within the application's genesis document. This addition is consistent with the PR's goal of integrating thegovplusmodule and allows for comprehensive testing of the module's functionality.protocol/app/app.go (2)
- 394-394: The addition of
govplusmoduletypes.StoreKeyto the KV store keys is correctly implemented, ensuring thegovplusmodule has its dedicated storage space.- 1011-1011: The initialization of
GovPlusKeeperwithapp.StakingKeeperandgovplusmoduletypes.StoreKeyis correctly done, aligning with the PR's objectives to integrate thegovplusmodule into the application. However, ensure that theGovPlusKeeperis correctly utilized in other parts of the application where necessary.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files ignored due to path filters (1)
protocol/x/govplus/types/tx.pb.gois excluded by:!**/*.pb.go
Files selected for processing (20)
- indexer/packages/v4-protos/src/codegen/dydxprotocol/bundle.ts (8 hunks)
- indexer/packages/v4-protos/src/codegen/dydxprotocol/govplus/tx.rpc.msg.ts (1 hunks)
- indexer/packages/v4-protos/src/codegen/dydxprotocol/govplus/tx.ts (1 hunks)
- indexer/packages/v4-protos/src/codegen/dydxprotocol/rpc.tx.ts (1 hunks)
- proto/dydxprotocol/govplus/tx.proto (1 hunks)
- protocol/app/app.go (2 hunks)
- protocol/app/basic_manager/basic_manager.go (2 hunks)
- protocol/app/msgs/all_msgs.go (1 hunks)
- protocol/app/msgs/internal_msgs.go (2 hunks)
- protocol/app/msgs/internal_msgs_test.go (1 hunks)
- protocol/app/simulation_test.go (4 hunks)
- protocol/lib/ante/internal_msg.go (2 hunks)
- protocol/testutil/app/app.go (3 hunks)
- protocol/x/govplus/keeper/keeper.go (1 hunks)
- protocol/x/govplus/keeper/msg_server.go (2 hunks)
- protocol/x/govplus/keeper/msg_server_test.go (2 hunks)
- protocol/x/govplus/types/errors.go (1 hunks)
- protocol/x/govplus/types/expected_keepers.go (1 hunks)
- protocol/x/govplus/types/keys.go (1 hunks)
- protocol/x/govplus/types/keys_test.go (1 hunks)
Files skipped from review as they are similar to previous changes (18)
- indexer/packages/v4-protos/src/codegen/dydxprotocol/bundle.ts
- indexer/packages/v4-protos/src/codegen/dydxprotocol/govplus/tx.rpc.msg.ts
- indexer/packages/v4-protos/src/codegen/dydxprotocol/govplus/tx.ts
- indexer/packages/v4-protos/src/codegen/dydxprotocol/rpc.tx.ts
- proto/dydxprotocol/govplus/tx.proto
- protocol/app/app.go
- protocol/app/basic_manager/basic_manager.go
- protocol/app/msgs/all_msgs.go
- protocol/app/msgs/internal_msgs.go
- protocol/app/msgs/internal_msgs_test.go
- protocol/app/simulation_test.go
- protocol/lib/ante/internal_msg.go
- protocol/testutil/app/app.go
- protocol/x/govplus/keeper/keeper.go
- protocol/x/govplus/keeper/msg_server_test.go
- protocol/x/govplus/types/errors.go
- protocol/x/govplus/types/expected_keepers.go
- protocol/x/govplus/types/keys.go
Additional comments: 2
protocol/x/govplus/types/keys_test.go (1)
- 12-12: The change from
"govplus"to"dydxgovplus"fortypes.StoreKeyin the test assertion correctly aligns with the PR's objective to update theStoreKeyvalue. This modification ensures that the test reflects the newStoreKeyvalue, maintaining consistency across the codebase.protocol/x/govplus/keeper/msg_server.go (1)
- 4-9: The addition of new imports for context handling, error management, SDK types, governance types, and logging is appropriate for the functionality introduced by the
SlashValidatormethod. These imports are necessary for the method's operation, including context management, error handling, and logging.
|
|
||
| // Multiplier for how much of the validator's stake should be slashed. | ||
| // slash_factor * power = amount slashed | ||
| string slash_factor = 5 [ |
There was a problem hiding this comment.
cosmos_protos are just annotations. gogoproto.customtype adds some codegen to help using that type. gogoproto.nullable affects the codegen (pointer or value)
| ctx, | ||
| consAddr, | ||
| int64(msg.InfractionHeight), | ||
| msg.PowerAtInfractionHeight.BigInt().Int64(), |
There was a problem hiding this comment.
It seems pretty bareboned that proposer would have to input the voting power themselves. Can we at least calculating the voting power from the delegation coin amount?
Also, from Slash() implementation, it calculates the amount to check with no additional validation on this number. If proposer mistakenly put a larger than actual amount, would this lead to any delegator being slashed more than its delegation amount? In general, it seems like we need some E2Es test to cover possible edge cases.
There was a problem hiding this comment.
The Slash() function in x/staking was not meant to be exposed for usage by human, and it has a couple of input contracts listed. Can we explicitly check these contracts (if not checked within the function)?
There was a problem hiding this comment.
You cannot because you can only get the delegation coin amount at the time of execution. If someone sees a proposal to slash them and knows they can avoid it by manipulating their delegation amount, it becomes worthless
There was a problem hiding this comment.
Can we at least calculating the voting power from the delegation coin amount?
I meant having user input the total delegated coin amount, as opposed to voting power. The conversion is trivial and can just be done on protocol if needed.
I'm not sure if voting power is ever exposed in any API. Can you check response fields of relevant x/staking queries and determine whether it returns staking power in tokens or voting power?
There was a problem hiding this comment.
Just in genesis state.. I can change this to tokens instead if you still prefer it that way. Let me know
There was a problem hiding this comment.
Can we explicitly check these contracts (if not checked within the function)?
All parts of the contract are checked in the Slash function and will error out. I've added to ValidateBasic as well.
(Except "Infraction was committed equal to or less than an unbonding period in the past, so all unbonding delegations and redelegations from that height are stored". It is up to the community to slash before the tokens are gone.)
There was a problem hiding this comment.
Could you identify a query supported by x/staking that directly returns either a validator's voting power or their total delegation amount, and update the proto field accordingly? This way a user can easily construct the proposal json.
|
|
||
| ctx := sdk.UnwrapSDKContext(goCtx) | ||
|
|
||
| _, err = k.stakingKeeper.Slash( |
There was a problem hiding this comment.
curious - why do we call stakingKeeper.Slash here and not slashingKeeper.Slash? slashing calls staking.Slash but also emits some events
There was a problem hiding this comment.
No particular reason. We didn't have any use for that event.
| @@ -6,5 +6,5 @@ const ( | |||
| ModuleName = "govplus" | |||
|
|
|||
| // StoreKey defines the primary module store key | |||
| StoreKey = ModuleName | |||
| StoreKey = "dydxgovplus" | |||
There was a problem hiding this comment.
May be worth adding a comment here to reflect that
| _, err = k.stakingKeeper.Slash( | ||
| ctx, | ||
| consAddr, | ||
| int64(msg.InfractionHeight), |
There was a problem hiding this comment.
should the proto itself just use int64/sint64?
There was a problem hiding this comment.
We use uint32 elsewhere for height.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files ignored due to path filters (1)
protocol/x/govplus/types/tx.pb.gois excluded by:!**/*.pb.go
Files selected for processing (6)
- indexer/packages/v4-protos/src/codegen/dydxprotocol/govplus/tx.ts (1 hunks)
- proto/dydxprotocol/govplus/tx.proto (1 hunks)
- protocol/x/govplus/keeper/msg_server.go (2 hunks)
- protocol/x/govplus/types/errors.go (1 hunks)
- protocol/x/govplus/types/keys.go (1 hunks)
- protocol/x/govplus/types/tx.go (1 hunks)
Files skipped from review as they are similar to previous changes (4)
- indexer/packages/v4-protos/src/codegen/dydxprotocol/govplus/tx.ts
- proto/dydxprotocol/govplus/tx.proto
- protocol/x/govplus/types/errors.go
- protocol/x/govplus/types/keys.go
Additional comments: 2
protocol/x/govplus/types/tx.go (1)
- 12-36: The
ValidateBasicmethod forMsgSlashValidatorcorrectly validates theAuthorityandValidatorAddressfields, checks for a positivePowerAtInfractionHeight, and ensures theSlashFactoris within a valid range. However, it's important to ensure that all error messages provide clear, actionable feedback and that all conditions are logically sound and secure.protocol/x/govplus/keeper/msg_server.go (1)
- 25-58: The
SlashValidatormethod implementation inmsg_server.gocorrectly handles the slashing of a validator, including authority validation, address parsing, and calling thestakingKeeper.Slashmethod. However, it's crucial to ensure that error handling is robust, logging is appropriate, and all inputs are validated correctly before proceeding with the slashing operation.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 3
Configuration used: CodeRabbit UI
Files ignored due to path filters (1)
protocol/x/govplus/types/tx.pb.gois excluded by:!**/*.pb.go
Files selected for processing (8)
- indexer/packages/v4-protos/src/codegen/dydxprotocol/govplus/tx.ts (1 hunks)
- proto/dydxprotocol/govplus/tx.proto (1 hunks)
- protocol/x/govplus/keeper/msg_server.go (2 hunks)
- protocol/x/govplus/keeper/msg_server_test.go (2 hunks)
- protocol/x/govplus/types/errors.go (1 hunks)
- protocol/x/govplus/types/keys.go (1 hunks)
- protocol/x/govplus/types/tx.go (1 hunks)
- protocol/x/govplus/types/tx_test.go (1 hunks)
Files skipped from review as they are similar to previous changes (6)
- indexer/packages/v4-protos/src/codegen/dydxprotocol/govplus/tx.ts
- proto/dydxprotocol/govplus/tx.proto
- protocol/x/govplus/keeper/msg_server_test.go
- protocol/x/govplus/types/errors.go
- protocol/x/govplus/types/keys.go
- protocol/x/govplus/types/tx.go
Additional comments: 2
protocol/x/govplus/keeper/msg_server.go (1)
- 4-9: The imports include context handling, error management, SDK types, governance types, and logging. Ensure these are all necessary for the functionality being implemented.
protocol/x/govplus/types/tx_test.go (1)
- 17-64: The
TestValidateBasicfunction covers various error scenarios forMsgSlashValidator, including invalid authority, bad address, incorrect power at infraction height, and invalid slash factor. Ensure all other relevant fields and scenarios are covered by tests to guarantee comprehensive validation.
| func (k msgServer) SlashValidator( | ||
| goCtx context.Context, | ||
| msg *types.MsgSlashValidator, | ||
| ) (*types.MsgSlashValidatorResponse, error) { | ||
| if !k.HasAuthority(msg.Authority) { | ||
| return nil, errorsmod.Wrapf( | ||
| govtypes.ErrInvalidSigner, | ||
| "invalid authority %s", | ||
| msg.Authority, | ||
| ) | ||
| } | ||
|
|
||
| consAddr, err := sdk.ConsAddressFromBech32(msg.ValidatorAddress) | ||
| if err != nil { | ||
| return nil, types.ErrValidatorAddress | ||
| } | ||
|
|
||
| ctx := sdk.UnwrapSDKContext(goCtx) | ||
| _, err = k.stakingKeeper.Slash( | ||
| ctx, | ||
| consAddr, | ||
| int64(msg.InfractionHeight), // Casting from uint32 | ||
| msg.PowerAtInfractionHeight.BigInt().Int64(), | ||
| msg.SlashFactor, | ||
| ) | ||
| if err != nil { | ||
| log.ErrorLogWithError( | ||
| ctx, | ||
| "error occured when slashing validator", | ||
| err, | ||
| ) | ||
| return nil, err | ||
| } | ||
| return &types.MsgSlashValidatorResponse{}, nil |
There was a problem hiding this comment.
The SlashValidator method correctly implements the slashing logic, including authority checks, address parsing, and invoking the Slash method on stakingKeeper. However, consider adding more detailed logging for debugging purposes, especially for the slashing operation's success case.
+ log.InfoLog(ctx, "Validator slashed successfully", "validatorAddress", msg.ValidatorAddress)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.
| func (k msgServer) SlashValidator( | |
| goCtx context.Context, | |
| msg *types.MsgSlashValidator, | |
| ) (*types.MsgSlashValidatorResponse, error) { | |
| if !k.HasAuthority(msg.Authority) { | |
| return nil, errorsmod.Wrapf( | |
| govtypes.ErrInvalidSigner, | |
| "invalid authority %s", | |
| msg.Authority, | |
| ) | |
| } | |
| consAddr, err := sdk.ConsAddressFromBech32(msg.ValidatorAddress) | |
| if err != nil { | |
| return nil, types.ErrValidatorAddress | |
| } | |
| ctx := sdk.UnwrapSDKContext(goCtx) | |
| _, err = k.stakingKeeper.Slash( | |
| ctx, | |
| consAddr, | |
| int64(msg.InfractionHeight), // Casting from uint32 | |
| msg.PowerAtInfractionHeight.BigInt().Int64(), | |
| msg.SlashFactor, | |
| ) | |
| if err != nil { | |
| log.ErrorLogWithError( | |
| ctx, | |
| "error occured when slashing validator", | |
| err, | |
| ) | |
| return nil, err | |
| } | |
| return &types.MsgSlashValidatorResponse{}, nil | |
| func (k msgServer) SlashValidator( | |
| goCtx context.Context, | |
| msg *types.MsgSlashValidator, | |
| ) (*types.MsgSlashValidatorResponse, error) { | |
| if !k.HasAuthority(msg.Authority) { | |
| return nil, errorsmod.Wrapf( | |
| govtypes.ErrInvalidSigner, | |
| "invalid authority %s", | |
| msg.Authority, | |
| ) | |
| } | |
| consAddr, err := sdk.ConsAddressFromBech32(msg.ValidatorAddress) | |
| if err != nil { | |
| return nil, types.ErrValidatorAddress | |
| } | |
| ctx := sdk.UnwrapSDKContext(goCtx) | |
| _, err = k.stakingKeeper.Slash( | |
| ctx, | |
| consAddr, | |
| int64(msg.InfractionHeight), // Casting from uint32 | |
| msg.PowerAtInfractionHeight.BigInt().Int64(), | |
| msg.SlashFactor, | |
| ) | |
| if err != nil { | |
| log.ErrorLogWithError( | |
| ctx, | |
| "error occured when slashing validator", | |
| err, | |
| ) | |
| return nil, err | |
| } | |
| log.InfoLog(ctx, "Validator slashed successfully", "validatorAddress", msg.ValidatorAddress) | |
| return &types.MsgSlashValidatorResponse{}, nil |
There was a problem hiding this comment.
This sounds helpful. Can we add some info logs here for debugging purpose?
There was a problem hiding this comment.
@teddyding Absolutely, adding info logs for debugging purposes is a great idea. Here's how you can include an info log right after the slashing operation succeeds, but before the method returns a response:
log.InfoLog(ctx, "Validator slashed successfully", "validatorAddress", msg.ValidatorAddress)You can place this line of code right after the Slash method call within the SlashValidator function, ensuring that it logs a success message whenever a validator is successfully slashed. This will greatly aid in debugging and monitoring the slashing process.
There was a problem hiding this comment.
I used to have info logs here. But it was just redundant cuz the underlying method has a bunch of logging already.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
d6a064b to
7f63e5b
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files ignored due to path filters (1)
protocol/x/govplus/types/tx.pb.gois excluded by:!**/*.pb.go
Files selected for processing (10)
- indexer/packages/v4-protos/src/codegen/dydxprotocol/govplus/tx.ts (1 hunks)
- proto/dydxprotocol/govplus/tx.proto (1 hunks)
- protocol/app/simulation_test.go (2 hunks)
- protocol/x/govplus/keeper/msg_server.go (2 hunks)
- protocol/x/govplus/keeper/msg_server_test.go (2 hunks)
- protocol/x/govplus/types/errors.go (1 hunks)
- protocol/x/govplus/types/expected_keepers.go (1 hunks)
- protocol/x/govplus/types/keys.go (1 hunks)
- protocol/x/govplus/types/tx.go (1 hunks)
- protocol/x/govplus/types/tx_test.go (1 hunks)
Files skipped from review as they are similar to previous changes (9)
- indexer/packages/v4-protos/src/codegen/dydxprotocol/govplus/tx.ts
- proto/dydxprotocol/govplus/tx.proto
- protocol/app/simulation_test.go
- protocol/x/govplus/keeper/msg_server_test.go
- protocol/x/govplus/types/errors.go
- protocol/x/govplus/types/expected_keepers.go
- protocol/x/govplus/types/keys.go
- protocol/x/govplus/types/tx.go
- protocol/x/govplus/types/tx_test.go
Additional comments: 2
protocol/x/govplus/keeper/msg_server.go (2)
- 4-9: The imports section has been updated to include necessary packages for the new functionality. Ensure that all new imports are used within the file to avoid unnecessary dependencies.
- 25-58: The
SlashValidatormethod implementation correctly checks for authority, validates the validator address, and performs the slashing operation. However, there are a few areas to consider:
Authority Validation (Lines 29-35): The method correctly checks if the caller has the authority to perform the slashing. This is crucial for ensuring that only authorized entities can execute this operation.
Validator Address Parsing (Lines 37-40): The method correctly parses the validator address from Bech32 format. Proper error handling is implemented if parsing fails.
Casting Infraction Height (Line 46): The casting of
msg.InfractionHeightfromuint32toint64is necessary due to SDK constraints. It's good practice to add a comment explaining why this casting is necessary for future maintainers.Error Logging (Lines 50-57): The method logs an error if the slashing operation fails, which is good for debugging purposes. However, consider enhancing the error message to include more context about the failure, such as the validator address.
Response (Line 58): The method returns an empty response upon successful execution. Ensure that this aligns with the expected behavior and that no additional information is needed in the response.
Overall, the method is well-implemented with proper error handling and validation. Consider adding more context to the error logging for better traceability.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files ignored due to path filters (1)
protocol/x/govplus/types/tx.pb.gois excluded by:!**/*.pb.go
Files selected for processing (6)
- indexer/packages/v4-protos/src/codegen/dydxprotocol/govplus/tx.ts (1 hunks)
- proto/dydxprotocol/govplus/tx.proto (1 hunks)
- protocol/x/govplus/keeper/msg_server.go (2 hunks)
- protocol/x/govplus/types/errors.go (1 hunks)
- protocol/x/govplus/types/tx.go (1 hunks)
- protocol/x/govplus/types/tx_test.go (1 hunks)
Files skipped from review as they are similar to previous changes (5)
- indexer/packages/v4-protos/src/codegen/dydxprotocol/govplus/tx.ts
- proto/dydxprotocol/govplus/tx.proto
- protocol/x/govplus/types/errors.go
- protocol/x/govplus/types/tx.go
- protocol/x/govplus/types/tx_test.go
Additional comments: 1
protocol/x/govplus/keeper/msg_server.go (1)
- 4-11: The imports have been organized and expanded to include necessary packages for context handling, error management, SDK types, governance types, and logging. This change supports the new functionality introduced in the
SlashValidatormethod.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files ignored due to path filters (1)
protocol/x/govplus/types/tx.pb.gois excluded by:!**/*.pb.go
Files selected for processing (6)
- indexer/packages/v4-protos/src/codegen/dydxprotocol/govplus/tx.ts (1 hunks)
- proto/dydxprotocol/govplus/tx.proto (1 hunks)
- protocol/x/govplus/keeper/msg_server.go (2 hunks)
- protocol/x/govplus/types/errors.go (1 hunks)
- protocol/x/govplus/types/tx.go (1 hunks)
- protocol/x/govplus/types/tx_test.go (1 hunks)
Files skipped from review as they are similar to previous changes (5)
- indexer/packages/v4-protos/src/codegen/dydxprotocol/govplus/tx.ts
- proto/dydxprotocol/govplus/tx.proto
- protocol/x/govplus/types/errors.go
- protocol/x/govplus/types/tx.go
- protocol/x/govplus/types/tx_test.go
Additional comments: 5
protocol/x/govplus/keeper/msg_server.go (5)
- 4-10: The import section has been updated to include necessary packages for context handling, error management, SDK types, governance types, and logging. Ensure all new imports are utilized within the file to avoid unnecessary dependencies.
- 1-13: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [14-20]
The removal of
NewMsgServerImplis not shown in the provided code but mentioned in the summary. If this function is indeed removed, verify that it is not used elsewhere in the codebase to prevent breaking changes.Verification successful
The search results indicate that
NewMsgServerImplis widely used across the codebase, with each module having its own implementation. This suggests that the function's removal from thegovplusmodule is an isolated change. It's important to ensure that thegovplusmodule has an alternative or updated mechanism in place for registering its message server. Given the pattern of usage across modules, the impact of the removal seems to be limited to thegovplusmodule itself, assuming each module implements its own version of this function.* 26-36: The `SlashValidator` method correctly checks if the caller has the necessary authority. This is a crucial step for security, ensuring that only authorized entities can initiate slashing. * 38-41: The conversion of `msg.ValidatorAddress` to a consensus address is properly handled, including error checking. This ensures that the address format is valid before proceeding with the slashing logic. * 52-60: The error handling within the slashing operation is robust, logging the error before returning it. This aids in debugging and monitoring. The method concludes by returning a successful response, which is the expected behavior.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for references to NewMsgServerImpl across the codebase. rg "NewMsgServerImpl"Length of output: 8695
| ctx := sdk.UnwrapSDKContext(goCtx) | ||
| _, err = k.stakingKeeper.Slash( | ||
| ctx, | ||
| consAddr, | ||
| int64(msg.InfractionHeight), // Casting from uint32 | ||
| sdk.TokensToConsensusPower( | ||
| sdkmath.NewIntFromBigInt(msg.TokensAtInfractionHeight.BigInt()), sdk.DefaultPowerReduction), | ||
| msg.SlashFactor, | ||
| ) |
There was a problem hiding this comment.
The slashing operation correctly unwraps the SDK context and performs the slashing with the appropriate parameters. The casting of msg.InfractionHeight from uint32 to int64 is necessary due to SDK constraints. However, adding a comment explaining this casting would improve code readability and maintainability.
+ // Casting from uint32 to int64 is necessary due to SDK method signature constraintsCommittable 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.
| ctx := sdk.UnwrapSDKContext(goCtx) | |
| _, err = k.stakingKeeper.Slash( | |
| ctx, | |
| consAddr, | |
| int64(msg.InfractionHeight), // Casting from uint32 | |
| sdk.TokensToConsensusPower( | |
| sdkmath.NewIntFromBigInt(msg.TokensAtInfractionHeight.BigInt()), sdk.DefaultPowerReduction), | |
| msg.SlashFactor, | |
| ) | |
| ctx := sdk.UnwrapSDKContext(goCtx) | |
| _, err = k.stakingKeeper.Slash( | |
| ctx, | |
| consAddr, | |
| int64(msg.InfractionHeight), // Casting from uint32 to int64 is necessary due to SDK method signature constraints | |
| sdk.TokensToConsensusPower( | |
| sdkmath.NewIntFromBigInt(msg.TokensAtInfractionHeight.BigInt()), sdk.DefaultPowerReduction), | |
| msg.SlashFactor, | |
| ) |
Changelist
Test Plan
Author/Reviewer Checklist
state-breakinglabel.indexer-postgres-breakinglabel.PrepareProposalorProcessProposal, manually add the labelproposal-breaking.feature:[feature-name].backport/[branch-name].refactor,chore,bug.