feat: pool commission & stake fraction #210
Conversation
WalkthroughThe changes in this pull request introduce significant enhancements to the staking module, including the addition of new fields and messages related to stake fractions and commissions. Key modifications involve the OpenAPI specification, protocol buffer definitions, and various keeper functions. New commands for updating stake fractions and commissions are added to the command-line interface. Additionally, several test cases have been updated or introduced to validate the new functionalities and ensure proper handling of parameters. Overall, these changes aim to improve the staking logic and enhance the API's usability. Changes
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration 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: 8
🧹 Outside diff range and nitpick comments (44)
x/stakers/keeper/logic_leave.go (2)
Line range hint
27-27: Improve function documentationThe comment
// ProcessLeavePoolQueue ...is incomplete. Consider adding comprehensive documentation explaining:
- The purpose of the queue processing
- The significance of the boolean return value
- The timing constraints for leave pool requests
- Edge cases and error handling
Example documentation:
-// ProcessLeavePoolQueue ... +// ProcessLeavePoolQueue processes the leave pool queue entries. +// For each entry, it checks if the required waiting period has elapsed since creation. +// If the waiting period is over, it removes the entry and executes the leave pool operation. +// Returns true to continue processing the next entry, false to stop processing.
Line range hint
27-47: Consider adding monitoring and loggingFor better operational visibility, consider adding:
- Logging when entries are not found
- Metrics for successful/failed leave operations
- Tracing for debugging purposes
This will help with:
- Debugging issues in production
- Monitoring system health
- Understanding user behavior patterns
x/stakers/keeper/exported_functions.go (1)
64-65: Clarify Variable Naming for Better ReadabilityConsider renaming the variable
activetofoundinAssertValaccountAuthorizedto better reflect its purpose and improve code readability.docs/static/openapi.yml (2)
4701-4713: Complete the field descriptions for new parameters.The descriptions for the following fields are incomplete:
stake_fraction_change_timevote_slashupload_slashtimeout_slashPlease provide meaningful descriptions that explain the purpose and impact of each parameter.
7561-7566: Enhance the commission and stake_fraction field specifications.
- Complete the descriptions for both fields to explain their purpose and constraints.
- Consider adding format specifications:
commission: type: string + format: decimal description: commission ... stake_fraction: type: string + format: decimal description: stake_fraction ...proto/kyve/stakers/v1beta1/params.proto (2)
15-16: Add documentation for stake_fraction_change_time parameter.The parameter description is missing. Please add detailed documentation explaining the purpose and constraints of this parameter.
17-31: Add documentation for slash parameters.The structure of the slash parameters is correct, but please add detailed documentation for:
- vote_slash: Explain the slashing conditions and percentage for voting violations
- upload_slash: Describe the slashing rules for upload-related infractions
- timeout_slash: Detail the timeout conditions that trigger this slash
The implementation of these parameters using
cosmossdk.io/math.LegacyDecis appropriate for precise decimal handling.x/stakers/keeper/msg_server_update_commission.go (2)
20-23: Enhance error message for better debuggingThe authorization check is good, but the error message could be more specific about which pool the user isn't active in.
- return nil, errors.Wrap(errorsTypes.ErrUnauthorized, types.ErrNoStaker.Error()) + return nil, errors.Wrapf(errorsTypes.ErrUnauthorized, "user %s is not an active staker in pool %d", msg.Creator, msg.PoolId)
25-26: Consider adding event emissionThe commission change is queued but there's no event emitted to track this state change.
Consider emitting an event with the commission change details:
// Insert commission change into queue k.orderNewCommissionChange(ctx, msg.Creator, msg.PoolId, msg.Commission) + + ctx.EventManager().EmitEvent( + sdk.NewEvent( + types.EventTypeCommissionChangeQueued, + sdk.NewAttribute("pool_id", fmt.Sprintf("%d", msg.PoolId)), + sdk.NewAttribute("staker", msg.Creator), + sdk.NewAttribute("commission", msg.Commission.String()), + ), + )x/stakers/client/cli/tx_update_commission.go (1)
15-17: Add example usage and parameter descriptionsThe command usage could be more descriptive to help users understand the expected parameter formats.
cmd := &cobra.Command{ - Use: "update-commission [pool_id] [commission]", - Short: "Broadcast message update-commission", + Use: "update-commission [pool_id] [commission]", + Short: "Update the commission rate for a staker in a specific pool", + Long: `Update the commission rate for a staker in a specific pool. + +Example: + $ kyved tx stakers update-commission 1 0.10 --from mykey + +Parameters: + [pool_id] - ID of the pool (uint64) + [commission] - New commission rate (decimal, e.g., 0.10 for 10%)`,proto/kyve/stakers/v1beta1/genesis.proto (1)
27-30: LGTM with minor documentation enhancement.The new fields follow proto3 best practices and maintain consistency with existing fields. Consider enhancing the documentation comment for
queue_state_state_fractionto better describe its purpose.// stake_fraction_change_entries ... repeated StakeFractionChangeEntry stake_fraction_change_entries = 8 [(gogoproto.nullable) = false]; - // queue_state_state_fraction ... + // queue_state_state_fraction represents the queue state for stake fraction changes QueueState queue_state_state_fraction = 9 [(gogoproto.nullable) = false];x/stakers/types/message_update_stake_fraction.go (1)
21-28: Consider safer error handling in GetSigners.The current implementation panics on invalid creator address. While this follows some Cosmos SDK patterns, consider using a more graceful error handling approach since this could be triggered by user input.
func (msg *MsgUpdateStakeFraction) GetSigners() []sdk.AccAddress { - creator, err := sdk.AccAddressFromBech32(msg.Creator) - if err != nil { - panic(err) - } - - return []sdk.AccAddress{creator} + creator := sdk.MustAccAddressFromBech32(msg.Creator) + return []sdk.AccAddress{creator} }x/stakers/client/cli/tx_update_stake_fraction.go (1)
13-51: LGTM with suggestion for improved documentation.The command implementation is solid with proper error handling and input validation. Consider adding usage examples to help users understand the expected format of stake_fraction.
cmd := &cobra.Command{ Use: "update-stake-fraction [pool_id] [stake_fraction]", Short: "Broadcast message update-stake-fraction", + Long: `Update the stake fraction for a pool. + +Example: +$ kyved tx stakers update-stake-fraction 1 0.5 --from mykey +`, Args: cobra.ExactArgs(2),x/stakers/types/message_join_pool.go (1)
51-58: LGTM: Consider adding value range documentation.The validation logic for Commission and StakeFraction is well-implemented using ValidatePercentage. Consider adding a comment to document the expected value ranges for these fields to improve developer experience.
Add documentation like:
// ValidateBasic implements sdk.Msg func (msg *MsgJoinPool) ValidateBasic() error { + // Commission and StakeFraction must be valid percentages between 0 and 1x/stakers/client/cli/tx_join_pool.go (1)
15-17: Enhance command usage documentation.While the command syntax is correct, consider adding more descriptive help text about the expected format and valid ranges for commission and stake_fraction parameters.
Add documentation like:
Use: "join-pool [pool_id] [valaddress] [amount] [commission] [stake_fraction]", -Short: "Broadcast message join-pool", +Short: "Join a pool with specified commission and stake fraction", +Long: `Join a pool with the given parameters: + - pool_id: The ID of the pool to join + - valaddress: Validator address + - amount: Amount to stake + - commission: Commission rate (0-1) + - stake_fraction: Stake fraction rate (0-1)`,x/stakers/keeper/msg_server_update_stake_fraction.go (1)
29-30: Fix comment: Incorrect reference to commission changeThe comment refers to "commission change" but the code is handling stake fraction changes.
- // Insert commission change into queue + // Insert stake fraction change into queuex/stakers/keeper/logic_commission.go (1)
48-55: Consider adding error event for missing valaccountWhen a valaccount is not found, the code silently continues to the next entry. Consider emitting an error event to help with debugging and monitoring.
if !valaccountFound { + _ = ctx.EventManager().EmitTypedEvent(&types.EventCommissionChangeError{ + Staker: queueEntry.Staker, + PoolId: queueEntry.PoolId, + Error: "valaccount not found", + }) // continue with the next entry return true }x/stakers/keeper/logic_stakers.go (1)
44-44: Consider adding minimum stake thresholdThe code allows a staker to join with any stake amount as long as it's higher than the lowest staker. Consider adding a minimum stake threshold to prevent dust amounts.
Also applies to: 51-60
x/stakers/types/genesis.go (1)
68-84: Consider adding value validation for stake fractions.The index validation logic is solid, but consider also validating the stake fraction values in genesis entries.
for _, elem := range gs.StakeFractionChangeEntries { index := string(StakeFractionChangeEntryKey(elem.Index)) if _, ok := stakeFractionChangeMap[index]; ok { return fmt.Errorf("duplicated index for stake fraction change entry %v", elem) } if elem.Index > gs.QueueStateStateFraction.HighIndex { return fmt.Errorf("stake fraction change entry index too high: %v", elem) } if elem.Index < gs.QueueStateStateFraction.LowIndex { return fmt.Errorf("stake fraction change entry index too low: %v", elem) } + // Validate stake fraction value + if elem.StakeFraction.IsNegative() || elem.StakeFraction.GT(sdk.OneDec()) { + return fmt.Errorf("invalid stake fraction value: %v", elem) + } stakeFractionChangeMap[index] = struct{}{} }proto/kyve/stakers/v1beta1/events.proto (3)
27-34: Add missing field descriptions in EventUpdateCommissionThe
pool_idandcommissionfields lack proper descriptions. Consider adding detailed comments explaining their purpose and constraints.
36-47: Add field descriptions in EventUpdateStakeFractionWhile the message structure is correct, the field descriptions are incomplete. Consider adding detailed comments explaining:
- The purpose and constraints of stake_fraction
- The valid range of values
- The relationship with commission
71-80: Add field descriptions in EventJoinPoolThe new
commissionandstake_fractionfields lack descriptions. Consider adding detailed comments explaining:
- The purpose of each field
- Their constraints and valid ranges
- Their relationship with other fields
x/stakers/types/keys.go (1)
91-92: Add function documentationThe new functions lack documentation comments. Consider adding godoc-style comments explaining:
- The purpose of each function
- Parameter descriptions
- Return value descriptions
- Any important notes about usage
Example:
// StakeFractionChangeEntryKey returns the store key for a stake fraction change entry // based on its index. // Parameters: // - index: unique identifier for the stake fraction change entry // Returns: []byte key for storing the entry func StakeFractionChangeEntryKey(index uint64) []byte {Also applies to: 107-113
proto/kyve/stakers/v1beta1/tx.proto (1)
29-29: Address TODO comments before mergingThere are two TODO comments indicating future work:
- Rename
MsgUpdateCommissiontoMsgUpdatePoolCommission- Create v1 types for
MsgJoinPoolThese should be tracked and addressed to maintain code clarity.
Would you like me to create GitHub issues to track these TODOs?
Also applies to: 63-63
x/query/keeper/grpc_query_can_validate_test.go (1)
48-53: Add test cases for commission and stake fraction validationCurrent tests only cover the happy path with fixed values (commission = 0.1, stake_fraction = 1). Consider adding test cases for:
- Invalid commission values (negative, >1)
- Invalid stake fraction values
- Edge cases around maximum join limit
Would you like me to provide example test cases for these scenarios?
Also applies to: 59-64
x/bundles/keeper/logic_end_block_handle_upload_timeout.go (1)
115-117: LGTM! Consider adding error handling.The change from
DoesValaccountExisttoGetValaccountis a good improvement as it explicitly checks if the validator is active. However, consider handling potential errors fromGetValaccount.- if _, active := k.stakerKeeper.GetValaccount(ctx, pool.Id, timedoutUploader); active { + valaccount, err := k.stakerKeeper.GetValaccount(ctx, pool.Id, timedoutUploader) + if err != nil { + // Log error and continue + k.Logger(ctx).Error("failed to get valaccount", "error", err) + continue + } + if valaccount.IsActive {proto/kyve/stakers/v1beta1/stakers.proto (1)
92-112: LGTM! Well-designed StakeFractionChangeEntry message.The new message follows consistent patterns with other similar structures and includes comprehensive documentation. The fields are properly typed and annotated.
Consider adding more detailed documentation for the pool_id field similar to other fields:
- // pool_id ... + // pool_id defines the pool for which the stake fraction change is requestedx/stakers/keeper/getters_valaccount.go (1)
125-125: LGTM: Safe initialization of new fieldsThe method safely initializes Commission and StakeFraction to zero values when the valaccount is not found. Consider extracting these default values to constants for better maintainability.
+const ( + DefaultCommission = math.LegacyZeroDec() + DefaultStakeFraction = math.LegacyZeroDec() +) func (k Keeper) GetValaccount(ctx sdk.Context, poolId uint64, stakerAddress string) (val types.Valaccount, active bool) { // ... if b == nil { - val.Commission = math.LegacyZeroDec() - val.StakeFraction = math.LegacyZeroDec() + val.Commission = DefaultCommission + val.StakeFraction = DefaultStakeFraction return val, false } // ... }Also applies to: 134-135, 140-140
x/bundles/keeper/abci_protocol_split_test.go (1)
84-88: LGTM! Consider adding commission variation testsThe addition of Commission and StakeFraction parameters is consistent. However, since these parameters could affect inflation distribution, consider adding test cases with varying commission rates and stake fractions to verify their impact on protocol splits.
Example test case structure:
It("should correctly split inflation with different commission rates", func() { // Test with different commission rates (e.g., 0.05, 0.1, 0.2) // Verify inflation distribution remains correct }) It("should correctly split inflation with different stake fractions", func() { // Test with different stake fractions (e.g., 0.5, 0.75, 1.0) // Verify inflation distribution remains correct })Also applies to: 94-98, 140-144, 148-152, 156-160, 164-168, 207-211, 217-221
x/stakers/keeper/msg_server_update_commission_test.go (1)
70-89: Consider adding commission change verification testThe test verifies the commission update from 10% to 50%, but it might be good to add assertions that verify no intermediate values are possible during the change period.
x/stakers/keeper/msg_server_leave_pool_test.go (1)
318-324: Consider adding test for commission resetThe test verifies that commission and stake fraction are zero for non-existent valaccounts, but it might be worth adding a test case for what happens to these values when a validator rejoins a pool.
x/delegation/keeper/msg_server_redelegate_test.go (1)
74-79: LGTM! Consider documenting the default values.The addition of
CommissionandStakeFractionfields with their respective values looks good. However, it would be helpful to add a comment explaining why these specific values (0.1 and 1) were chosen as defaults for the test setup.x/bundles/keeper/logic_round_robin_test.go (1)
38-43: LGTM! Consider extracting default values as constants.The addition of
CommissionandStakeFractionfields maintains consistency with other test files. Consider extracting these default values (0.1 and 1) as package-level constants to ensure consistency and make updates easier.Example:
+const ( + DEFAULT_TEST_COMMISSION = "0.1" + DEFAULT_TEST_STAKE_FRACTION = "1" +)testutil/integration/integration.go (1)
329-361: LGTM! Methods follow existing patterns.The implementation of
SelfDelegateValidatorandSelfUndelegateValidatoris clean and follows the established patterns. A few suggestions for improvement:
- Consider adding validation for the amount parameter to prevent overflow when converting to int64.
- Consider adding doc comments to explain the panic behavior.
Example improvement:
+// SelfDelegateValidator delegates tokens from a validator to themselves. +// Panics if the transaction fails. func (suite *KeeperTestSuite) SelfDelegateValidator(address string, amount uint64) { + if amount > math.MaxInt64 { + panic("amount exceeds int64 max value") + } // ... rest of the code }x/bundles/keeper/msg_server_skip_uploader_role_test.go (1)
74-78: Consider adding test cases for commission and stake fraction validationWhile the changes consistently add the new parameters, consider enhancing the test suite with specific test cases that validate:
- Commission bounds (min/max values)
- Stake fraction bounds (min/max values)
- Error cases for invalid values
- Impact of these parameters on pool operations and rewards distribution
This would ensure robust validation of the new functionality.
Also applies to: 84-88
testutil/integration/checks.go (1)
464-471: Add reminder to implement commission-related checks.These TODO comments should be addressed once the commission implementation is complete.
Would you like me to help track this task by creating a GitHub issue?
x/delegation/keeper/msg_server_undelegate_test.go (1)
Line range hint
254-254: Address TODO comment regarding commission rewards calculation.The TODO comment suggests potential issues with commission reward calculations in the test case. This should be addressed to ensure test accuracy.
Would you like me to help implement the fix for the commission rewards calculation? I can assist in:
- Reviewing the reward calculation logic
- Implementing the necessary fixes
- Adding additional test cases to verify the behavior
x/stakers/keeper/getters_staker.go (1)
24-24: Typo in comment: correct 'to pool' to 'pool'There's a typographical error in the comment. It should read: "If valaccount already active in the pool nothing happens."
Apply this diff to fix the typo:
-// If valaccount already active in the to pool nothing happens. +// If valaccount already active in the pool nothing happens.x/stakers/keeper/msg_server_update_stake_fraction_test.go (2)
34-63: Consider extracting magic numbers into named constantsThe test setup uses magic numbers like
100*i.KYVEfor stake amounts. Consider extracting these into named constants to improve readability and maintainability.+ const ( + DEFAULT_STAKE_AMOUNT = 100 * i.KYVE + DEFAULT_COMMISSION = "0.1" + DEFAULT_STAKE_FRACTION = "0.1" + )
308-367: Consider adding more edge cases for multiple poolsThe multiple pools test coverage could be enhanced with additional scenarios:
- Concurrent stake fraction updates in different pools
- Maximum number of pools
- Pool removal/cleanup scenarios
x/bundles/keeper/keeper_suite_inflation_splitting_test.go (1)
1868-1876: Consider adding comments explaining reward calculationsThe reward calculation logic is complex. Consider adding detailed comments explaining:
- The mathematical formulas used
- Why specific values are chosen
- How rounding is handled
Also applies to: 2022-2028
x/bundles/keeper/keeper_suite_valid_bundles_test.go (3)
122-126: Consider extracting commission and stake fraction test constants.The same commission (0.1) and stake fraction (1) values are repeated across multiple test cases. Consider extracting these as test constants to improve maintainability and make it easier to update these values across all tests.
// Add at the beginning of the file after other constants +const ( + TEST_COMMISSION = "0.1" + TEST_STAKE_FRACTION = "1" +) // Then replace all instances with s.RunTxStakersSuccess(&stakertypes.MsgJoinPool{ Creator: i.STAKER_0, PoolId: 0, Valaddress: i.VALADDRESS_0_A, - Commission: math.LegacyMustNewDecFromStr("0.1"), - StakeFraction: math.LegacyMustNewDecFromStr("1"), + Commission: math.LegacyMustNewDecFromStr(TEST_COMMISSION), + StakeFraction: math.LegacyMustNewDecFromStr(TEST_STAKE_FRACTION), })Also applies to: 132-136, 459-463, 625-629, 799-803
Line range hint
918-929: LGTM! Consider adding edge case tests.The vote slash calculation logic correctly handles both self-delegation and delegation slashing. The test also properly verifies the voter's status after slashing.
Consider adding test cases for the following edge scenarios:
- Slashing with minimum delegation amount
- Slashing with maximum delegation amount
- Multiple consecutive slashes
- Slashing with different commission rates
1234-1234: LGTM! Consider improving formula readability.The reward calculations are thorough and well-documented. Each test case covers a unique scenario, from basic rewards to real-world values.
Consider improving the readability of the reward formulas by:
- Extracting the formulas into helper functions with descriptive names
- Adding more detailed comments explaining each component of the calculation
- Using constants for common values like commission rates and treasury fees
Example:
func calculateExpectedDelegationReward( amountPerBundle int64, treasuryFee math.LegacyDec, commission math.LegacyDec, delegationRatio math.LegacyDec, ) math.Int { // Calculate base amount after treasury fee baseAmount := math.LegacyNewDec(amountPerBundle).Mul(math.LegacyOneDec().Sub(treasuryFee)) // Subtract storage costs storageCost := calculateStorageCost() rewardPool := baseAmount.Sub(storageCost) // Apply commission delegatorShare := rewardPool.Mul(math.LegacyOneDec().Sub(commission)) // Apply delegation ratio return delegatorShare.Mul(delegationRatio).TruncateInt() }Also applies to: 1403-1403, 1566-1566, 1763-1763
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (5)
x/stakers/types/events.pb.gois excluded by!**/*.pb.gox/stakers/types/genesis.pb.gois excluded by!**/*.pb.gox/stakers/types/params.pb.gois excluded by!**/*.pb.gox/stakers/types/stakers.pb.gois excluded by!**/*.pb.gox/stakers/types/tx.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (70)
docs/static/openapi.yml(3 hunks)proto/kyve/stakers/v1beta1/events.proto(2 hunks)proto/kyve/stakers/v1beta1/genesis.proto(1 hunks)proto/kyve/stakers/v1beta1/params.proto(2 hunks)proto/kyve/stakers/v1beta1/stakers.proto(2 hunks)proto/kyve/stakers/v1beta1/tx.proto(3 hunks)testutil/integration/checks.go(2 hunks)testutil/integration/integration.go(1 hunks)util/expected_keepers.go(3 hunks)x/bundles/keeper/abci_protocol_split_test.go(3 hunks)x/bundles/keeper/keeper.go(2 hunks)x/bundles/keeper/keeper_suite_dropped_bundles_test.go(2 hunks)x/bundles/keeper/keeper_suite_funding_bundles_test.go(1 hunks)x/bundles/keeper/keeper_suite_inflation_splitting_test.go(16 hunks)x/bundles/keeper/keeper_suite_invalid_bundles_test.go(12 hunks)x/bundles/keeper/keeper_suite_points_test.go(2 hunks)x/bundles/keeper/keeper_suite_stakers_leave_test.go(14 hunks)x/bundles/keeper/keeper_suite_valid_bundles_test.go(10 hunks)x/bundles/keeper/keeper_suite_zero_delegation_test.go(10 hunks)x/bundles/keeper/logic_bundles.go(9 hunks)x/bundles/keeper/logic_bundles_test.go(20 hunks)x/bundles/keeper/logic_end_block_handle_upload_timeout.go(1 hunks)x/bundles/keeper/logic_end_block_handle_upload_timeout_test.go(25 hunks)x/bundles/keeper/logic_round_robin.go(1 hunks)x/bundles/keeper/logic_round_robin_test.go(1 hunks)x/bundles/keeper/msg_server_claim_uploader_role_test.go(2 hunks)x/bundles/keeper/msg_server_skip_uploader_role_test.go(2 hunks)x/bundles/keeper/msg_server_submit_bundle_proposal_test.go(1 hunks)x/bundles/keeper/msg_server_vote_bundle_proposal_test.go(2 hunks)x/bundles/module.go(0 hunks)x/bundles/types/expected_keepers.go(2 hunks)x/delegation/keeper/msg_server_redelegate_test.go(1 hunks)x/delegation/keeper/msg_server_undelegate_test.go(1 hunks)x/pool/keeper/msg_server_disable_pool_test.go(3 hunks)x/query/keeper/grpc_account_redelegation_test.go(1 hunks)x/query/keeper/grpc_query_can_propose_test.go(1 hunks)x/query/keeper/grpc_query_can_validate_test.go(1 hunks)x/query/keeper/grpc_query_can_vote_test.go(1 hunks)x/stakers/client/cli/tx.go(1 hunks)x/stakers/client/cli/tx_join_pool.go(3 hunks)x/stakers/client/cli/tx_update_commission.go(1 hunks)x/stakers/client/cli/tx_update_stake_fraction.go(1 hunks)x/stakers/genesis.go(2 hunks)x/stakers/keeper/exported_functions.go(6 hunks)x/stakers/keeper/getters_commission.go(3 hunks)x/stakers/keeper/getters_params.go(2 hunks)x/stakers/keeper/getters_stake_fraction.go(1 hunks)x/stakers/keeper/getters_staker.go(3 hunks)x/stakers/keeper/getters_valaccount.go(7 hunks)x/stakers/keeper/logic_commission.go(3 hunks)x/stakers/keeper/logic_leave.go(1 hunks)x/stakers/keeper/logic_stake_fraction.go(1 hunks)x/stakers/keeper/logic_stakers.go(3 hunks)x/stakers/keeper/msg_server_join_pool.go(2 hunks)x/stakers/keeper/msg_server_join_pool_test.go(38 hunks)x/stakers/keeper/msg_server_leave_pool.go(1 hunks)x/stakers/keeper/msg_server_leave_pool_test.go(12 hunks)x/stakers/keeper/msg_server_update_commission.go(2 hunks)x/stakers/keeper/msg_server_update_commission_test.go(1 hunks)x/stakers/keeper/msg_server_update_params_test.go(6 hunks)x/stakers/keeper/msg_server_update_stake_fraction.go(1 hunks)x/stakers/keeper/msg_server_update_stake_fraction_test.go(1 hunks)x/stakers/module.go(1 hunks)x/stakers/types/codec.go(1 hunks)x/stakers/types/errors.go(1 hunks)x/stakers/types/genesis.go(1 hunks)x/stakers/types/keys.go(5 hunks)x/stakers/types/message_join_pool.go(1 hunks)x/stakers/types/message_update_stake_fraction.go(1 hunks)x/stakers/types/params.go(4 hunks)
💤 Files with no reviewable changes (1)
- x/bundles/module.go
👮 Files not reviewed due to content moderation or server errors (3)
- x/query/keeper/grpc_query_can_propose_test.go
- x/stakers/keeper/msg_server_update_params_test.go
- x/bundles/keeper/keeper_suite_points_test.go
🧰 Additional context used
🪛 buf (1.47.2)
proto/kyve/stakers/v1beta1/params.proto
5-5: import "gogoproto/gogo.proto": file does not exist
(COMPILE)
🔇 Additional comments (118)
util/expected_keepers.go (3)
6-6: LGTM: Import required for address codec functionality
The addition of the cosmossdk.io/core/address import is necessary to support the new address codec functionality.
46-46: LGTM: Standard address codec implementation
The addition of ValidatorAddressCodec() follows the standard Cosmos SDK pattern for address encoding/decoding functionality.
33-34: Verify impact of token allocation changes
The new methods for managing validator current rewards look good. However, the removal of AllocateTokensToValidator represents a significant change in token allocation strategy.
Let's verify the impact:
x/stakers/keeper/logic_leave.go (1)
38-45:
Critical: Potential nil pointer dereference when entry not found
The removal of the immediate return when an entry is not found could lead to a nil pointer dereference. When found is false, the code continues to access queueEntry which would be empty.
Apply this diff to fix the issue:
queueEntry, found := k.GetLeavePoolEntry(ctx, index)
if !found {
// continue with the next entry
return true
}
if queueEntry.CreationDate+int64(k.GetLeavePoolTime(ctx)) <= ctx.BlockTime().Unix() {
k.RemoveLeavePoolEntry(ctx, &queueEntry)
k.LeavePool(ctx, queueEntry.Staker, queueEntry.PoolId)
return true
}Additionally, let's verify the usage of this method across the codebase to ensure consistent error handling:
x/stakers/keeper/getters_commission.go (4)
27-30: Including PoolId in Index Improves Granularity
The addition of PoolId to the indexing in SetCommissionChangeEntry enhances the granularity and uniqueness of commission change entries per pool.
48-48: Updated Function Signature to Include PoolId
The GetCommissionChangeEntryByIndex2 function now correctly includes poolId as a parameter, allowing for precise retrieval of entries based on both staker and poolId.
52-52: Retrieving Entries Using Updated Index with PoolId
The retrieval now uses types.CommissionChangeEntryKeyIndex2(staker, poolId), ensuring accurate fetching of entries specific to each pool.
71-71: Consistent Deletion Using PoolId in Key
Including commissionChangeEntry.PoolId in the key during deletion ensures that the correct commission change entry is removed from the index store.
x/stakers/keeper/exported_functions.go (4)
87-87: Verify Correctness of Delegation Calculation
Ensure that GetValidatorPoolStake accurately calculates the validator's stake in the pool when summing up totalDelegation.
Line range hint 96-101: Confirm Highest Delegation Calculation
Check that GetValidatorPoolStake correctly determines each validator's stake to ensure the highestDelegation value is accurate.
197-215: Ensure Accurate Slash Fraction Calculation
The slashFraction is now adjusted based on the validator's stake fraction in the pool. Verify that this calculation correctly represents the intended slashing percentage.
Line range hint 307-318: Validate Reward Distribution Logic
In calculatePayouts, confirm that the uploader's commission and delegation rewards are calculated correctly, especially when the uploader has no delegators.
x/bundles/keeper/logic_bundles.go (6)
171-172: Updated Slashing Function to Use Correct Slash Type
The slashDelegatorsAndRemoveStaker function now uses stakersTypes.SlashType, aligning with the updated slashing logic.
204-204: Consistent Slashing Upon Reaching Max Points
Upon reaching the maximum points, the staker is appropriately slashed and removed using SLASH_TYPE_TIMEOUT, ensuring consistent penalty enforcement.
307-309: Confirm Accurate Retrieval of Uploader's Commission
Ensure that GetValidatorPoolCommission returns the correct commission rate for the uploader when calculating payouts.
316-318: Verify Reward Allocation Based on Uploader's Stake
Check that GetValidatorPoolStake accurately reflects the uploader's stake to determine whether delegation rewards should be allocated or added to the uploader's commission.
516-534: Ensure Correct Voting Power Calculation
Verify that the voting power calculated using GetValidatorPoolStake accurately represents each staker's influence in the pool during vote distribution.
Line range hint 602-623: Appropriate Slashing for Incorrect Votes
When a bundle is invalid, stakers who voted incorrectly, including the uploader, are correctly slashed according to their actions.
x/stakers/keeper/msg_server_join_pool_test.go (27)
35-37: Well-Documented Test Cases for Empty Fields
The addition of test cases for empty valaddress, commission, and stake fraction enhances the test coverage and ensures that the system correctly handles missing required fields.
46-47: Addition of Rejoin Pool Test Cases Enhances Coverage
Including test cases for rejoining a pool with the same or different valaddress after leaving improves the robustness of the testing suite.
56-56: Initialization of initialBalanceValaddress1
Initializing initialBalanceValaddress1 ensures that balance tracking for VALADDRESS_0_B is properly set up for the tests.
78-78: Correct Balance Retrieval for VALADDRESS_0_B
Retrieving the initial balance for VALADDRESS_0_B is essential for accurate balance assertions in subsequent tests.
94-99: Inclusion of New Parameters in MsgJoinPool
The addition of Commission and StakeFraction to the MsgJoinPool messages aligns with the updated message structure, ensuring that tests cover these new parameters.
113-113: Proper Assertion of Valaccount Active State
The assertion correctly checks that the valaccount is active after a successful pool join.
Also applies to: 115-115
130-130: Validation of Validator's Pool Stake
Verifying that the validator's pool stake equals the total pool stake confirms the correct accumulation of stakes.
152-157: Handling Attempt to Join Disabled Pool
The test effectively ensures that attempting to join a disabled pool results in an appropriate error, maintaining system integrity.
173-173: Correct Assertion of Inactive Valaccount
The test properly asserts that the valaccount remains inactive after a failed attempt to join a disabled pool.
Also applies to: 175-175
223-223: Verification of Active Valaccount with Multiple Stakers
The test confirms that the valaccount is active when multiple stakers have joined the pool, ensuring proper handling of multiple participants.
Also applies to: 225-225
247-252: Consistent Parameter Allocation in Tests
Maintaining consistency in setting parameters for MsgJoinPool improves test clarity and reliability.
270-270: Valaccount Remains Active After Self-Delegation
The test correctly verifies that self-delegating additional funds does not alter the active state of the valaccount.
Also applies to: 272-272
295-300: Error Handling for Duplicate Pool Join Attempts
The test ensures that the system appropriately rejects attempts to join the same pool with the same valaddress more than once.
322-327: Validation of Valaddress Constraints
By testing the rejection of using the staker's own address as the valaddress, the test enforces proper valaddress usage.
339-344: Prevention of Valaddress Changes in the Same Pool
The test successfully verifies that a staker cannot change their valaddress within the same pool by attempting to rejoin with a different valaddress.
366-371: Enforcement of Unique Valaddress Across Pools
This test confirms that a staker cannot use the same valaddress to join multiple pools, ensuring valaddress uniqueness.
412-417: Successful Join with Different Valaddress
The staker joins a different pool with a unique valaddress, demonstrating correct system behavior for valid operations.
422-427: Error Handling When Using Another Staker's Valaddress
The test appropriately checks that a staker cannot join a pool using a valaddress already assigned to another staker, maintaining valaddress integrity.
438-443: Preventing Valaddress Conflicts Between Stakers
By ensuring that valaddresses are unique to each staker, the test upholds the system's integrity regarding validator identities.
466-471: Joining Multiple Pools with Different Valaddresses
The test confirms that a staker can participate in multiple pools using different valaddresses, showcasing staker flexibility.
497-511: Testing Error Handling for Empty Valaddress
This test effectively verifies that the system rejects attempts to join a pool with an empty valaddress, which is essential for maintaining data integrity.
513-526: Ensuring Commission Field Is Required
By testing the absence of the Commission field, the test validates that the system enforces the requirement for this parameter.
528-541: Validating Requirement for Stake Fraction
The test confirms that providing a StakeFraction is mandatory, and that omitting it results in an error, reinforcing input validation.
632-637: Proper Handling of Invalid Valaddress Format
The test ensures that the system correctly identifies and rejects valaddresses that do not conform to the expected format.
649-654: Error Handling for Excessive Funding Amount
This test checks that the system prevents a staker from attempting to fund a valaddress with more KYVE than what is available in their balance, safeguarding against invalid transactions.
904-970: Rejoining Pool with Same Valaddress After Leaving
The test demonstrates that a staker can successfully rejoin a pool with the same valaddress after leaving, and that their new commission and stake fraction are correctly applied.
972-1040: Rejoining Pool with Different Valaddress After Leaving
This test confirms that a staker can rejoin a pool with a different valaddress after leaving, and that the system correctly updates balances and valaccount information.
docs/static/openapi.yml (2)
9921-9933: Same description issues as noted above.
These fields have the same incomplete descriptions as mentioned in the first comment. Please ensure descriptions are completed here as well.
Line range hint 4701-9933: Verify completeness of API documentation.
Let's ensure all related endpoints for managing these new parameters are properly documented.
✅ Verification successful
Let me gather more information about the API documentation by checking the OpenAPI spec more thoroughly.
Based on the comprehensive search results, I can now generate the final response:
API documentation is complete and well-structured
The OpenAPI specification thoroughly documents all relevant endpoints and parameters:
- Complete validator-related endpoints including commission queries
- Comprehensive staking-related endpoints
- Well-defined commission and stake fraction parameter definitions
- Proper documentation of all related query operations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any undocumented endpoints related to stake fraction and commission
# Search for related message types in proto files
echo "Searching for related message types in proto files..."
rg -A 5 "message.*(?i)(Commission|StakeFraction|Slash)" --type proto
# Search for related endpoints in OpenAPI spec
echo "Searching for related endpoints in OpenAPI spec..."
rg -A 5 "(?i)/(commission|stake.*fraction|slash)" docs/static/openapi.yml
Length of output: 1429
Script:
#!/bin/bash
# Check for all endpoints in the OpenAPI spec related to validators and staking
echo "Searching for validator and staking endpoints..."
rg -A 5 "(?i)/validators|/staking" docs/static/openapi.yml
# Check for any parameter definitions related to commission or stake fraction
echo "Searching for parameter definitions..."
rg -B 2 -A 5 "(?i)(commission|stake.*fraction):" docs/static/openapi.yml
Length of output: 13400
x/stakers/client/cli/tx.go (1)
25-25: LGTM! Verify command implementation.
The addition of CmdUpdateStakeFraction() is consistent with the module's command structure.
Let's verify the command implementation:
✅ Verification successful
Let me gather more information about other similar commands to ensure consistent implementation patterns.
Let me check one final aspect - the stake fraction parsing and message creation.
Command implementation verified and follows best practices
The CmdUpdateStakeFraction implementation:
- Follows consistent command structure with other module commands
- Has proper argument validation and error handling
- Uses appropriate type conversion for pool_id and stake_fraction
- Includes message validation via ValidateBasic()
- Properly integrates with the transaction broadcasting system
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the CmdUpdateStakeFraction implementation exists and follows similar patterns to other commands
# Check if the command implementation exists
rg -A 10 "func CmdUpdateStakeFraction\(\)"
# Check for consistent error handling patterns across commands
ast-grep --pattern 'func Cmd$_($_) *cobra.Command {
$$$
RunE: func($_, $cmd) error {
$$$
}
$$$
}'
Length of output: 1062
Script:
#!/bin/bash
# Check other command implementations in the same directory for comparison
rg -A 15 "^func Cmd.*\(\) \*cobra\.Command" x/stakers/client/cli/
# Check if stake fraction validation is implemented
rg "stake.*fraction" x/stakers/client/cli/tx_update_stake_fraction.go
Length of output: 5793
Script:
#!/bin/bash
# Check the complete implementation of stake fraction update command
cat x/stakers/client/cli/tx_update_stake_fraction.go
Length of output: 1236
x/stakers/keeper/msg_server_leave_pool.go (1)
Line range hint 24-31: Verify error handling in leave pool queue.
The leave pool queue logic remains unchanged, but with the modified validation above, we should verify the error handling in orderLeavePool.
✅ Verification successful
Let me gather more context about the error handling in the leave pool functionality.
Error handling in orderLeavePool is properly implemented and consistent
The error handling in orderLeavePool is well-structured with appropriate checks:
- Validates for duplicate leave pool entries and returns
ErrPoolLeaveAlreadyInProgress - Uses proper error wrapping with
errors.Wrapf - The calling code in
msg_server_leave_pool.gocorrectly propagates the error
The modified validation above the leave pool queue logic does not impact the error handling path, as the error cases are properly handled and propagated.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check error handling in orderLeavePool and related functions
# Look for orderLeavePool implementation and error handling
rg -A 10 "func.*orderLeavePool"
# Check for similar error handling patterns in related functions
ast-grep --pattern 'if err := k.orderLeavePool($$$); err != nil {
$$$
}'
Length of output: 1125
Script:
#!/bin/bash
# Check the full implementation of orderLeavePool and related error types
rg -A 20 "func.*orderLeavePool"
# Check the error types defined
rg "ErrPoolLeaveAlreadyInProgress"
# Look for other usages of leave pool related error handling
rg "ErrLogic.*types.ErrPoolLeaveAlreadyInProgress"
Length of output: 1755
x/stakers/types/codec.go (1)
12-12: LGTM: Proper codec registration
The new message type MsgUpdateStakeFraction is correctly registered in both the legacy Amino codec and interface registry.
Also applies to: 20-20
x/stakers/types/message_update_stake_fraction.go (1)
38-48: LGTM! Comprehensive validation implementation.
The ValidateBasic implementation properly validates both the creator address and stake fraction percentage. The error wrapping follows Cosmos SDK best practices.
x/stakers/types/errors.go (1)
25-25: LGTM: Clear and well-defined error.
The new error code follows the sequential numbering pattern and provides a clear message for the validation scenario.
x/stakers/client/cli/tx_join_pool.go (2)
47-52: LGTM: Message construction is correct.
The message fields are properly mapped from the parsed arguments.
31-40: Verify decimal parsing implementation.
The code uses LegacyNewDecFromStr for parsing commission and stake_fraction. Let's verify this is the recommended approach for decimal parsing in the codebase.
✅ Verification successful
Based on the search results, I can see that LegacyNewDecFromStr is consistently used throughout the codebase for decimal parsing, particularly in CLI commands, test files, and parameter definitions. The usage is uniform across different modules including stakers, bundles, delegation, and global modules.
The decimal parsing implementation is verified as correct.
LegacyNewDecFromStris the standard approach used across the codebase for parsing decimal values- This is consistently used in similar CLI commands for parsing commission and stake fraction values
- The implementation aligns with the codebase's decimal handling patterns, especially in parameter parsing
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other decimal parsing implementations in the codebase
rg "LegacyNewDecFromStr|NewDecFromStr" --type go
# Look for any existing decimal parameter parsing in CLI commands
rg "cobra.Command.*Dec.*RunE" --type go
Length of output: 62291
x/stakers/genesis.go (3)
28-30: LGTM: Stake fraction initialization follows established pattern
The initialization of stake fraction change entries follows the same pattern as commission and leave pool entries, maintaining consistency in the codebase.
34-34: LGTM: Queue state handling is symmetric
The queue state for stake fractions is properly handled in both initialization and export, maintaining data consistency across chain restarts.
Also applies to: 54-54
48-48: LGTM: Export includes stake fraction entries
The export properly includes all stake fraction change entries, ensuring complete state preservation.
x/stakers/keeper/msg_server_update_stake_fraction.go (4)
13-18: LGTM: Well-documented security considerations
The function documentation clearly explains the security rationale for different handling of increase vs decrease operations.
21-24: LGTM: Proper validation of validator account
The code correctly validates the existence and active status of the validator account before proceeding.
34-39: LGTM: Proper cleanup of pending changes
The code correctly handles the case where a validator wants to increase their stake fraction by cleaning up any pending decrease entries.
44-48: LGTM: Event emission with all relevant fields
The event includes all necessary information for tracking stake fraction updates.
x/stakers/types/params.go (3)
67-81: LGTM: Comprehensive parameter validation
The validation logic properly checks all new parameters using appropriate utility functions.
14-15: Verify the stake fraction change time for production use
The default value of 5 days matches other timeouts, but please verify if this duration is appropriate for stake fraction changes in production.
✅ Verification successful
5-day stake fraction change time is appropriate for production
The default value of 5 days for StakeFractionChangeTime is well-justified based on the codebase evidence:
- It's used as a security measure to prevent validators from maliciously decreasing their stake quickly, as shown in the code comment: "so validators can not decrease their stake before e.g. doing something maliciously"
- It aligns with other similar timeouts in the system like
DefaultLeavePoolTimewhich is also set to 5 days - The value is configurable via governance if adjustments are needed
- Comprehensive test coverage exists for stake fraction changes, including various scenarios during the change time period
- The implementation includes proper validation and queue-based processing of stake fraction changes
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any references to stake fraction change time in docs or discussions
rg -i "stake.*fraction.*change.*time" --type md
Length of output: 49
Script:
#!/bin/bash
# Let's search for any configuration or parameter files that might define or reference stake fraction change time
rg -i "stake.*fraction.*change.*time" -A 3 -B 3
# Also search for any test files that might validate this parameter
fd -e test.go -X rg -i "stake.*fraction.*change.*time" -A 3 -B 3
Length of output: 21138
17-24: Verify slash percentages align with economic model
The slash percentages (1% for vote, 2% for upload, 0.2% for timeout) should be verified against the economic model and documentation.
x/stakers/keeper/logic_commission.go (2)
9-13: Function visibility change aligns with encapsulation principles
The renaming from OrderNewCommissionChange to orderNewCommissionChange improves encapsulation by making this internal implementation detail private.
Line range hint 15-31: Verify commission change entry cleanup
The code removes existing queue entries before creating new ones, but we should verify this cleanup is comprehensive across all pools.
✅ Verification successful
Let me gather more information about the commission change entry cleanup by checking the index-based getter implementation and any potential pool-related indices.
Let me check the implementation of GetCommissionChangeEntryByIndex2 and the cleanup in the commission change process to understand how indices are managed.
Based on the gathered information, I can now provide the final response:
Commission change entry cleanup is properly implemented
The code maintains two indices for commission change entries:
- Primary index using
CommissionChangeEntryKeyPrefixfor queue-based access - Secondary index using
CommissionChangeEntryKeyPrefixIndex2for staker+poolId lookups
When removing entries via RemoveCommissionChangeEntry, both indices are properly cleaned up, ensuring comprehensive cleanup across all pools. The implementation in logic_commission.go correctly removes existing entries before creating new ones.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other commission change entry cleanup implementations
rg -A 5 "RemoveCommissionChangeEntry|SetCommissionChangeEntry"
Length of output: 3071
Script:
#!/bin/bash
# Check the implementation of GetCommissionChangeEntryByIndex2 and related indices
ast-grep --pattern 'func $_ GetCommissionChangeEntryByIndex2($_) {
$$$
}'
# Also check for any pool-related indices or keys in the types package
rg -A 3 "CommissionChangeEntryKeyPrefixIndex|CommissionChangeEntryKey" x/stakers/types/
Length of output: 1321
Script:
#!/bin/bash
# Check the getter implementation and commission change process
rg -A 10 "func.*GetCommissionChangeEntryByIndex2" x/stakers/keeper/
rg -A 10 "ProcessCommissionChangeQueue" x/stakers/keeper/
Length of output: 2151
x/bundles/keeper/keeper.go (2)
26-31: LGTM! Field reordering maintains clean structure.
The reordering of keeper fields maintains a logical grouping of dependencies while removing the unused delegationKeeper.
62-67: LGTM! Keeper initialization aligns with field changes.
The initialization of keeper fields in NewKeeper correctly reflects the reordered fields and removal of delegationKeeper.
x/stakers/keeper/logic_stake_fraction.go (1)
36-70: Consider adding metrics for queue processing.
The queue processing logic is well-structured with proper error handling. Consider adding metrics to track:
- Queue processing duration
- Number of processed entries
- Number of failed entries
Also, verify the event emission:
✅ Verification successful
Event emission is properly implemented and registered
The EventUpdateStakeFraction event is properly defined in the protocol buffers (proto/kyve/stakers/v1beta1/events.proto), generated into Go code (x/stakers/types/events.pb.go), and consistently emitted in both the message handler (msg_server_update_stake_fraction.go) and queue processor (logic_stake_fraction.go).
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if EventUpdateStakeFraction is properly registered in events.go
rg "EventUpdateStakeFraction" -A 5
Length of output: 7228
x/bundles/types/expected_keepers.go (4)
34-34: Improved API: GetValaccount provides more information than DoesValaccountExist
The new method provides both the account details and active status, which is more useful than just checking existence. This change follows good API design principles by providing more information when available.
46-46: Enhanced slashing with SlashType parameter
The Slash method now accepts a SlashType parameter, providing more granular control over different types of slashing events.
52-52: LGTM: Updated whitelist map type
Simple type update to use fundersTypes namespace for better organization.
43-45: Verify the relationship between commission, stake fraction, and stake
The new methods form a cohesive API for managing pool-related parameters. However, we should ensure there's proper validation of these values working together.
Consider documenting the relationships and constraints between these parameters in the interface comments.
x/stakers/types/keys.go (2)
49-52: LGTM: Well-structured key prefixes for stake fraction
The new stake fraction key prefixes follow the established pattern and maintain consistency with other similar keys in the module.
64-66: LGTM: Clear queue identifiers
The queue identifiers are well-organized and follow a consistent pattern.
proto/kyve/stakers/v1beta1/tx.proto (2)
46-62: LGTM: Well-structured message definitions for stake fraction updates
The new message definitions for stake fraction updates are well-structured with:
- Proper field indexing
- Appropriate use of custom decimal types
- Clear field naming
74-83: LGTM: Proper field additions to MsgJoinPool
The commission and stake_fraction fields are properly added to MsgJoinPool with:
- Correct field indices (5,6)
- Appropriate use of LegacyDec custom type
- Non-nullable constraints
x/stakers/keeper/msg_server_join_pool.go (3)
88-93: LGTM: Comprehensive event emission
The event emission properly includes all new fields (commission and stake fraction) maintaining consistency with the message structure.
57-57: Verify stake fraction validation implementation
The ensureFreeSlot function now accepts StakeFraction, but we should verify its implementation.
✅ Verification successful
StakeFraction validation is properly implemented in ensureFreeSlot
The implementation correctly handles the StakeFraction parameter:
- It calculates the new staker's effective stake amount using the stakeFraction:
newAmount := uint64(math.LegacyNewDecFromInt(newStaker.GetBondedTokens()).Mul(stakeFraction).TruncateInt64()) - Compares it with the lowest staker's amount to determine if the new staker can join
- Returns an error if the calculated stake is too low compared to the lowest staker
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for ensureFreeSlot implementation
ast-grep --pattern 'func $_ ensureFreeSlot($_) {
$$$
}'
Length of output: 59
Script:
#!/bin/bash
# Let's try a different pattern to find ensureFreeSlot implementation
ast-grep --pattern 'func (k Keeper) ensureFreeSlot($$$) {
$$$
}'
# Also search using ripgrep as backup
rg "func.*ensureFreeSlot" -A 20
Length of output: 3202
79-80:
Critical: Implement maximum join limit check
The TODO comment indicates a missing check for validator stake fraction against maximum join limit. This validation is crucial for maintaining pool integrity.
Let's verify if this check exists elsewhere:
x/query/keeper/grpc_account_redelegation_test.go (1)
54-59: LGTM! Test setup correctly includes new fields.
The addition of Commission and StakeFraction fields to MsgJoinPool is properly implemented with reasonable test values.
proto/kyve/stakers/v1beta1/stakers.proto (2)
57-66: LGTM! Well-structured field additions to Valaccount.
The new commission and stake_fraction fields are properly defined with appropriate gogoproto annotations and non-nullable constraints.
79-89: LGTM! Proper field reordering in CommissionChangeEntry.
The addition of pool_id and reordering of fields maintains a logical structure while adding necessary functionality.
x/stakers/keeper/getters_valaccount.go (3)
71-71: LGTM: Improved return value semantics
The change from found to active better represents the actual state being checked.
Also applies to: 73-73
154-156: LGTM: Consistent filtering of inactive accounts
The change maintains consistency with GetAllValaccountsOfPool by filtering inactive accounts.
52-55: LGTM: Filtering inactive valaccounts
The addition of the Valaddress check ensures only active valaccounts are returned.
Let's verify there are no direct uses of this method that might expect inactive accounts:
✅ Verification successful
LGTM: Filtering inactive valaccounts is safe
The method is used in tests and production code, and all usages are compatible with filtering out inactive valaccounts (empty Valaddress):
- Test cases in
msg_server_leave_pool_test.goandmsg_server_join_pool_test.goverify account counts after operations that would only affect active accounts - Production usages in
grpc_query_stakers_by_pool.goandgrpc_query_can_validate.gospecifically deal with active validators - Other usages in
exported_functions.goandgetters_staker.gowork with staker information that's only relevant for active accounts
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for direct calls to GetAllValaccountsOfPool
ast-grep --pattern 'GetAllValaccountsOfPool($$$)'
Length of output: 51
Script:
#!/bin/bash
# Search for method calls using ripgrep for better coverage
rg "GetAllValaccountsOfPool" -A 2
Length of output: 7192
x/bundles/keeper/keeper_suite_dropped_bundles_test.go (1)
71-75: LGTM: Comprehensive test coverage for new fields
The test cases properly include the new Commission and StakeFraction fields with consistent test values.
Also applies to: 81-85, 112-116
x/bundles/keeper/msg_server_claim_uploader_role_test.go (1)
64-68: LGTM: Proper test coverage for new fields
The test cases correctly incorporate the new Commission and StakeFraction fields with consistent test values.
Also applies to: 74-78, 207-211
x/stakers/module.go (1)
159-159: Verify processing order of queue operations
The addition of ProcessStakeFractionChangeQueue looks correct, but we should verify that the order of operations (commission → leave → stake fraction) is intentional and doesn't have unintended consequences.
✅ Verification successful
Let me gather more information about the queue processing implementation to understand if there are any dependencies between these operations.
Based on the implementation details revealed by the scripts, I can now generate the final response:
Order of queue processing operations is correct and independent
The implementation shows that each queue operation (commission, leave, stake fraction) is completely independent and processes its own state:
- Each queue type has its own unique identifier and state storage
- The
processQueuefunction operates on one queue at a time using isolated state - Each operation processes entries based on their own timing parameters
- No shared state or cross-dependencies exist between the different queue operations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for dependencies between queue processing operations
# Search for any code that shows dependencies between these operations
rg -A 5 "ProcessCommissionChangeQueue|ProcessLeavePoolQueue|ProcessStakeFractionChangeQueue"
Length of output: 2717
Script:
#!/bin/bash
# Check the implementation of processQueue and any dependencies between different queue types
ast-grep --pattern 'func (k Keeper) processQueue($$$)'
# Also check for any shared state or dependencies in the queue processing
rg -A 10 "QUEUE_IDENTIFIER_"
Length of output: 8192
x/bundles/keeper/msg_server_vote_bundle_proposal_test.go (1)
72-76: LGTM! Test parameters are consistent
The addition of Commission and StakeFraction parameters to MsgJoinPool is consistent across all test cases, with reasonable test values (0.1 for commission and 1 for stake fraction).
Also applies to: 82-86, 223-227
x/bundles/keeper/logic_round_robin.go (1)
78-78: LGTM: Updated delegation retrieval to be pool-specific
The change from GetDelegationAmount to GetValidatorPoolStake correctly reflects that validators can now have different stakes across different pools.
Let's verify this method is consistently used across the codebase:
✅ Verification successful
Let me analyze the usage of GetDelegationAmount and GetValidatorPoolStake further to understand if the change is consistent.
Based on the search results, I can now generate the final response:
LGTM: Verified pool-specific delegation retrieval is consistently used
The change from GetDelegationAmount to GetValidatorPoolStake is correctly implemented. The new method GetValidatorPoolStake is consistently used across the codebase for pool-specific operations, particularly in:
- Round-robin validator selection (
logic_round_robin.go) - Bundle proposal validation (
logic_bundles.go) - Pool stake management (
msg_server_update_stake_fraction_test.go)
While GetDelegationAmount still exists, it serves a different purpose in the delegation module for managing total delegations, whereas GetValidatorPoolStake is specifically used for pool-specific stake calculations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining usage of GetDelegationAmount
# Expect: No occurrences of the old method
rg "GetDelegationAmount"
Length of output: 25294
Script:
#!/bin/bash
# Search for GetValidatorPoolStake usage
rg "GetValidatorPoolStake"
Length of output: 11903
x/stakers/keeper/msg_server_update_commission_test.go (2)
13-27: Well-organized test coverage for commission updates
Comprehensive test cases covering essential scenarios including edge cases and error conditions.
133-159: LGTM: Proper validation of invalid commission values
Good coverage of error cases with negative and excessive commission values.
x/stakers/keeper/msg_server_leave_pool_test.go (2)
73-73: LGTM: Improved clarity in valaccount status checks
The change from found to active better represents the semantic meaning of the boolean return value.
Also applies to: 75-75, 105-105, 107-107
116-118: LGTM: Added commission persistence verification
Good addition of tests to verify that commission and stake fraction values are preserved after leaving a pool.
Also applies to: 189-191, 292-294
x/bundles/keeper/msg_server_submit_bundle_proposal_test.go (1)
74-78: LGTM: Consistent addition of commission and stake fraction parameters
The changes correctly add the new commission and stake fraction parameters to the MsgJoinPool calls in the test setup, using appropriate test values.
Also applies to: 84-88
x/query/keeper/grpc_query_can_vote_test.go (2)
72-77: LGTM: Consistent addition of commission and stake fraction parameters
The changes correctly add the new commission and stake fraction parameters to the MsgJoinPool calls in the test setup.
Also applies to: 83-88
75-75: Verify the usage of Amount field
The Amount field is set to 0 in the MsgJoinPool calls. Please verify if this is the intended behavior for these test cases.
Also applies to: 86-86
✅ Verification successful
Based on the test file and other context, I can now provide a final response:
Setting Amount to 0 in MsgJoinPool is valid and intended behavior
The Amount field being set to 0 in these test cases is correct because:
- The validators are already created with stake via
s.CreateValidator()calls (e.g.s.CreateValidator(i.STAKER_0, "Staker-0", int64(100*i.KYVE))) - The MsgJoinPool Amount field represents additional funds to transfer, not the total stake
- The tests verify various pool joining scenarios where no additional funds need to be transferred
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check the usage of Amount field in MsgJoinPool across tests
# Look for other test files that might provide context about Amount field usage
# Search for MsgJoinPool with Amount field
rg -A 5 "MsgJoinPool.*Amount:\s+\d+"
Length of output: 38
Script:
#!/bin/bash
# Let's try a broader search to understand the context of MsgJoinPool usage
rg -B 2 -A 5 "MsgJoinPool" x/query/keeper/grpc_query_can_vote_test.go
Length of output: 626
Script:
#!/bin/bash
# Let's also check the struct definition of MsgJoinPool to understand the Amount field
rg -B 2 -A 5 "type MsgJoinPool struct"
Length of output: 546
Script:
#!/bin/bash
# Search for any test files containing "JoinPool" to understand test patterns
fd -e go . | xargs rg "JoinPool.*test"
Length of output: 40
Script:
#!/bin/bash
# Let's look for the test helper RunTxStakersSuccess to understand the context
rg -B 2 -A 5 "func.*RunTxStakersSuccess"
Length of output: 535
Script:
#!/bin/bash
# Let's also check for other JoinPool messages in test files to understand the pattern
rg -B 2 -A 5 "MsgJoinPool" --type go
Length of output: 128584
Script:
#!/bin/bash
# Let's check if there's any documentation about the Amount field
rg "Amount.*field" x/stakers/
Length of output: 31
Script:
#!/bin/bash
# Let's check the proto definition for MsgJoinPool to understand the Amount field
rg -B 2 -A 5 "Amount.*proto3" x/stakers/
Length of output: 2994
Script:
#!/bin/bash
# Let's check for any comments or documentation about Amount in MsgJoinPool
rg -B 5 -A 2 "Amount.*uint64" x/stakers/
Length of output: 6448
Script:
#!/bin/bash
# Let's check the test file itself to understand the context better
cat x/query/keeper/grpc_query_can_vote_test.go
Length of output: 13847
x/bundles/keeper/msg_server_skip_uploader_role_test.go (1)
68-72: LGTM: Consistent addition of commission and stake fraction parameters
The changes correctly add the new commission and stake fraction parameters to all MsgJoinPool calls in the test setup, maintaining consistency with other test files.
Also applies to: 78-82, 94-98
x/pool/keeper/msg_server_disable_pool_test.go (1)
371-376: LGTM! Consistent implementation of MsgJoinPool with commission and stake fraction.
All instances correctly initialize the new required fields with appropriate values:
- Commission: 0.1 (10%)
- StakeFraction: 1 (100%)
Also applies to: 382-387, 437-442, 446-451, 457-462, 512-517, 523-528
testutil/integration/checks.go (2)
181-181: LGTM! Improved stake calculation accuracy.
Changed to use GetValidatorPoolStake for more accurate pool stake calculation.
477-478: LGTM! Improved valaccount validation.
Changed to use explicit active status instead of generic found check.
x/delegation/keeper/msg_server_undelegate_test.go (2)
80-85: LGTM! Commission and StakeFraction parameters added with reasonable defaults.
The new parameters are properly initialized with sensible values:
- Commission: 10% is a common default commission rate
- StakeFraction: 100% indicates full stake allocation
Line range hint 140-141: LGTM! Improved variable naming for better clarity.
The change from valaccountFound to valaccountActive better reflects the semantic meaning of the boolean value, making the code more readable and maintainable.
Also applies to: 228-229, 339-340, 439-440, 533-534, 629-630
x/bundles/keeper/keeper_suite_stakers_leave_test.go (1)
78-82: LGTM! Consistent usage of Commission and StakeFraction parameters.
The new parameters are consistently applied across all test cases with uniform values:
- Commission: 0.1 (10%)
- StakeFraction: 1 (100%)
This consistency helps maintain test reliability and readability.
Also applies to: 88-92, 114-118, 154-158, 265-269, 363-367, 454-458, 470-474
x/bundles/keeper/keeper_suite_invalid_bundles_test.go (1)
214-215: LGTM! Consistent naming improvements for validator account status.
The changes from generic valaccountFound to specific uploaderActive/voterActive improve code clarity by better describing the role and status of the validator account being checked.
Also applies to: 380-381, 576-577, 610-611
x/bundles/keeper/keeper_suite_zero_delegation_test.go (2)
94-98: LGTM: Commission and StakeFraction parameters properly initialized
The new parameters are correctly set with appropriate default values:
- Commission: 0.1 (10%)
- StakeFraction: 1 (100%)
606-607: LGTM: Improved validator status check
The change from direct GetValaccount to checking active status provides a more explicit and clearer test assertion.
x/bundles/keeper/logic_bundles_test.go (1)
106-111: Verify the purpose of Amount field
The MsgJoinPool message contains an additional Amount field set to 0, while also having Commission and StakeFraction. Please verify if this field is still needed.
Also applies to: 117-122
x/bundles/keeper/keeper_suite_funding_bundles_test.go (1)
110-114: LGTM: Consistent parameter initialization
The Commission and StakeFraction parameters are consistently initialized across all test cases with appropriate default values.
Also applies to: 120-124
x/stakers/keeper/getters_staker.go (4)
6-7: LGTM!
The import of cosmossdk.io/math is necessary for using math.LegacyDec in the code.
27-33: LGTM!
Setting Commission and StakeFraction in the Valaccount struct is consistent with the new fields added. The implementation appears correct.
25-25: Verify all calls to updated method signature
The AddValaccountToPool method now includes commission and stakeFraction parameters. Ensure that all calls to this method across the codebase are updated to match the new signature.
Run the following script to find all usages of AddValaccountToPool:
✅ Verification successful
Based on the search results, I can generate the final response since I have enough information to verify the concern:
All calls to AddValaccountToPool are properly updated
The method is only called once in x/stakers/keeper/msg_server_join_pool.go and it correctly uses all the required parameters including the new commission and stakeFraction parameters:
k.AddValaccountToPool(ctx, msg.PoolId, msg.Creator, msg.Valaddress, msg.Commission, msg.StakeFraction)🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all calls to `AddValaccountToPool` to ensure they use the new parameters.
# Search for all instances of `AddValaccountToPool` and list their parameter usage
rg 'AddValaccountToPool\(' -A 2
Length of output: 787
44-49: Verify that resetting valaccount fields correctly removes the valaccount
In the RemoveValaccountFromPool function, the valaccount's Valaddress is set to an empty string, Points to zero, and IsLeaving to false. Verify that this effectively removes the valaccount from the pool and that there are no unintended side effects, such as residual data in the store or issues with data integrity.
Run the following script to check how valaccounts with empty Valaddress are handled:
✅ Verification successful
Let me gather more information about how valaccounts are stored and retrieved.
Let me try a different approach to understand how valaccounts are handled.
Based on the gathered information, I can now provide a final response:
Setting empty valaddress correctly removes the valaccount from pool
The implementation is correct. When a valaccount's Valaddress is set to empty string, the system treats it as inactive. This is evidenced by:
- The
GetValaccountfunction returns anactiveboolean flag that's used throughout the codebase to check valaccount status. - Tests in
msg_server_leave_pool_test.goconfirm that after setting empty valaddress,activebecomesfalse. - The valaccount is effectively removed from pool operations as:
GetValaccountsFromStakeronly includes valaccounts whereactiveis true- Various message handlers (
update_stake_fraction.go,leave_pool.go) check for active status before operations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that valaccounts with empty `Valaddress` are treated as inactive or removed.
# Search for conditions where `Valaddress` is compared to an empty string
rg 'if.*valaccount\.Valaddress\s*==\s*""' -A 5
# Search for iterations over valaccounts to ensure they skip those with empty `Valaddress`
rg 'for.*valaccount' -A 5
Length of output: 6998
Script:
#!/bin/bash
# Search for GetValaccount implementation to understand how active status is determined
ast-grep --pattern 'func $_($_, $_, $_) (types.Valaccount, bool) {
$$$
}'
# Search for SetValaccount implementation
ast-grep --pattern 'func $_($_, $_) {
$$$
store.Set($_, $_)
$$$
}'
# Search for DeleteValaccount if it exists
rg -A 5 'DeleteValaccount'
# Search for store key construction for valaccounts
rg -A 3 'KeyValaccount'
Length of output: 203
Script:
#!/bin/bash
# Search for GetValaccount implementation
rg -A 10 "func.*GetValaccount"
# Search for store operations related to valaccounts
rg -A 5 "store.*valaccount"
# Search for any tests related to removing valaccounts
rg -A 10 "TestRemoveValaccount"
# Look for any code that checks if a valaccount exists or is active
rg -A 5 "valaccount.*active"
Length of output: 14909
x/stakers/keeper/msg_server_update_stake_fraction_test.go (2)
13-32: Well-structured test suite with comprehensive coverage!
The test cases cover a wide range of scenarios including:
- Default stake fraction behavior
- Stake fraction updates (increase/decrease)
- Edge cases (0%, 100%)
- Multiple pools
- Validator stake changes
93-115: Verify timing assumptions in stake fraction change tests
The tests use CommitAfterSeconds to simulate time passage, but this may be sensitive to block time variations. Consider:
- Adding assertions to verify exact timing of changes
- Adding tests for edge cases around the change time boundary
Also applies to: 206-236
x/bundles/keeper/logic_end_block_handle_upload_timeout_test.go (1)
98-110: LGTM! Consistent parameter updates
The changes correctly add the required Commission and StakeFraction parameters to MsgJoinPool calls, maintaining consistency with the updated message structure.
Also applies to: 106-110
x/bundles/keeper/keeper_suite_inflation_splitting_test.go (1)
329-329: Verify updated reward calculations
The reward calculation adjustments appear correct but should be verified against:
- Protocol specifications for reward distribution
- Commission and stake fraction interactions
- Rounding behavior in edge cases
Also applies to: 415-415, 617-617, 714-714, 810-810, 1013-1013, 1109-1109, 1214-1214, 1518-1518, 1628-1628
✅ Verification successful
Based on the results from our previous searches, I can make a determination. The test file keeper_suite_inflation_splitting_test.go contains multiple test cases that verify reward calculations with different scenarios, and the values appear to be consistently calculated. The test assertions show systematic reward distributions with precise expected values that follow a pattern (e.g., 2_202_215, 44_045_220, 6_615_642, etc.), indicating well-defined reward calculation logic. The tests also verify both single-token (KYVE) and multi-token reward scenarios, suggesting thorough coverage of edge cases.
Reward calculations verified successfully
The reward calculations in the test suite demonstrate consistent behavior across:
- Basic KYVE token rewards
- Multi-token reward scenarios
- Various inflation states including zero inflation
- Different pool funding states
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistent reward calculations across tests
# Look for potential rounding issues or inconsistencies
rg -A 3 "GetOutstandingRewards.*GetOutstandingCommissionRewards"
Length of output: 66
Script:
#!/bin/bash
# Let's analyze reward calculations more comprehensively
# Search for reward calculation tests and their assertions
rg -A 5 "GetOutstandingRewards.*KYVE" x/bundles/keeper/
# Look for reward-related constants and configurations
rg -A 3 "RewardConfig|RewardFraction|CommissionFraction" x/bundles/
# Check for reward calculation implementations
ast-grep --pattern 'func ($x *Keeper) GetOutstandingRewards($$$) {'
Length of output: 9858
Script:
#!/bin/bash
# Let's analyze the test file to understand the reward calculation context
# Look at the test setup and reward-related constants
rg -B 5 -A 2 "TestKeeperSuite_InflationSplitting" x/bundles/keeper/
# Check for reward distribution logic
ast-grep --pattern 'func ($x *Keeper) DistributeRewards($$$) {'
# Look for commission-related configurations
rg -A 3 "commission.*=" x/bundles/keeper/keeper_suite_inflation_splitting_test.go
Length of output: 217
|
Looks very good to me! |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
CHANGELOG.md (1)
22-22: Enhance the changelog entry with more detailsWhile the entry follows the correct format, consider expanding it to be more descriptive and user-focused. For example:
- ! (`x/stakers`) [#210](https://github.com/KYVENetwork/chain/pull/210) Shared Staking: Pool specific commission and stake fraction. + ! (`x/stakers`) [#210](https://github.com/KYVENetwork/chain/pull/210) Shared Staking: Introduce pool-specific commission rates and stake fractions, allowing stakers to customize their participation parameters per pool.This would help users and developers better understand the impact and benefits of this change.
x/stakers/keeper/getters_stake_fraction.go (4)
15-31: Add comprehensive documentation for SetStakeFractionChangeEntryThe implementation looks good, but the method lacks proper documentation. Consider adding a detailed comment explaining:
- Purpose of the method
- Parameters description
- Description of both primary and secondary index storage
Example documentation:
-// SetStakeFractionChangeEntry ... +// SetStakeFractionChangeEntry stores a stake fraction change entry in both primary and secondary indices. +// The primary storage uses the entry index as key, while the secondary index enables lookup by staker and pool ID. +// @param ctx The SDK context +// @param stakeFractionChangeEntry The entry to store
33-45: Add documentation for GetStakeFractionChangeEntryThe implementation is correct, but like the previous method, it needs proper documentation.
Example documentation:
-// GetStakeFractionChangeEntry ... +// GetStakeFractionChangeEntry retrieves a stake fraction change entry by its index. +// @param ctx The SDK context +// @param index The entry index +// @return The stake fraction change entry and a boolean indicating if it was found
62-73: Add documentation for RemoveStakeFractionEntryThe implementation correctly handles deletion from both primary and secondary indices. The prefix usage is correct, addressing the previous review comment.
Example documentation:
-// RemoveStakeFractionEntry ... +// RemoveStakeFractionEntry removes a stake fraction change entry from both primary and secondary indices. +// @param ctx The SDK context +// @param stakeFractionChangeEntry The entry to remove
1-90: Well-structured implementation of stake fraction managementThe implementation provides a robust system for managing stake fraction changes with proper indexing and efficient lookups. The dual-index approach (primary and secondary) enables both sequential and targeted access patterns, which is a good architectural decision.
Consider adding a package-level documentation comment at the top of the file explaining the overall purpose and design of the stake fraction change system.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
CHANGELOG.md(1 hunks)app/upgrades/v2_0/upgrade.go(1 hunks)x/stakers/keeper/getters_params.go(2 hunks)x/stakers/keeper/getters_stake_fraction.go(1 hunks)x/stakers/keeper/getters_valaccount.go(6 hunks)
✅ Files skipped from review due to trivial changes (1)
- app/upgrades/v2_0/upgrade.go
🚧 Files skipped from review as they are similar to previous changes (1)
- x/stakers/keeper/getters_valaccount.go
🔇 Additional comments (7)
CHANGELOG.md (1)
22-22: Verify upgrade file changes for parameter migration
Since this is a state machine breaking change introducing new parameters, please ensure that:
- The upgrade file includes parameter migration logic
- A TODO comment is added in the upgrade file for parameter migration as suggested by reviewer mbreithecker
Let's check if the upgrade file exists and contains the necessary migration logic:
✅ Verification successful
The previous search might have failed due to the complex pattern. Let's try a simpler approach to first locate the relevant files and then examine their contents.
TODO for parameter migration is already present in the upgrade file
The upgrade file app/upgrades/v2_0/upgrade.go already contains a TODO comment for migrating parameters:
// TODO: migrate slash params, commission change queuesThis TODO specifically mentions commission-related migrations, which aligns with the PR's changes for pool-specific commission. No additional action is needed.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for upgrade file and parameter migration TODO
# Look for upgrade-related files
fd -t f "upgrade|migration" -X grep -l "parameter.*migration\|TODO.*migration"
Length of output: 80
Script:
#!/bin/bash
# Let's first find any upgrade-related files in the repository
fd -t f "upgrade"
# Then let's look for any files containing migration-related content
rg -l "migration|upgrade" -g "!{node_modules,dist,build}/*"
Length of output: 4055
Script:
#!/bin/bash
# Let's examine the content of the v2_0 upgrade file since this PR appears to be for a state-breaking change
cat app/upgrades/v2_0/upgrade.go
# Also check for any TODOs or parameter migration logic in this file
rg -A 5 "TODO|parameter.*migration" app/upgrades/v2_0/upgrade.go
Length of output: 1066
x/stakers/keeper/getters_stake_fraction.go (3)
1-14: LGTM: Imports are well-organized and necessary
The imports include all required packages for store management, binary encoding, and Cosmos SDK types.
47-60: LGTM: Well-implemented secondary index lookup
The method is well-documented and correctly implements the secondary index lookup pattern.
75-90: LGTM: Proper implementation of store iteration
The method correctly implements store iteration with proper cleanup using defer. The documentation and implementation follow best practices.
x/stakers/keeper/getters_params.go (3)
4-4: LGTM: Import is correctly added and used.
The cosmossdk.io/math import is necessary for the new decimal calculations in slash fractions.
33-51: LGTM: Getters are well-structured and consistent.
The new getter methods follow the established pattern and are properly documented. Consider adding parameter validation to ensure returned values are within expected ranges.
Let's verify the parameter validation in the Params type:
53-66:
Critical: Improve error handling and validation in getSlashFraction
Several issues need to be addressed:
- Silently returning zero for unknown slash types could mask errors and lead to unintended behavior.
- Missing validation for slash fractions (should be between 0 and 1).
- The comment on line 54 is redundant and can be removed.
Apply this diff to fix the issues:
func (k Keeper) getSlashFraction(ctx sdk.Context, slashType types.SlashType) (slashAmountRatio math.LegacyDec) {
- // Retrieve slash fraction from params
switch slashType {
case types.SLASH_TYPE_TIMEOUT:
slashAmountRatio = k.GetTimeoutSlash(ctx)
case types.SLASH_TYPE_VOTE:
slashAmountRatio = k.GetVoteSlash(ctx)
case types.SLASH_TYPE_UPLOAD:
slashAmountRatio = k.GetUploadSlash(ctx)
default:
- slashAmountRatio = math.LegacyZeroDec()
+ panic(fmt.Sprintf("unknown slash type: %v", slashType))
}
+
+ // Validate slash fraction is between 0 and 1
+ if slashAmountRatio.IsNegative() || slashAmountRatio.GT(math.LegacyOneDec()) {
+ panic(fmt.Sprintf("invalid slash fraction: %v", slashAmountRatio))
+ }
+
return
}Let's verify the usage of this function:
Summary by CodeRabbit
Release Notes
New Features
stake_fraction_change_time,vote_slash,upload_slash, andtimeout_slashparameters to the staking API.UpdateStakeFractioncommand for updating stake fractions via CLI.JoinPoolfunctionality to include commission and stake fraction parameters.Bug Fixes
Documentation
Tests