feat: module for multi-coin-rewards #244
Conversation
…/compliance # Conflicts: # x/global/ante_test.go
# Conflicts: # testutil/integration/integration.go # x/global/ante_test.go # x/stakers/keeper/keeper_suite_protocol_bridge_test.go
WalkthroughThis pull request integrates a comprehensive multi-coin rewards system throughout the application. It adds new modules, protobuf definitions, CLI commands, keeper logic, genesis handling, and gRPC query endpoints to support multi-coin rewards management. Additionally, dependency versions and configuration files have been updated, and various renaming and type adjustments have been made to align with the new functionality. Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant CLI as CLI Command
participant MS as MsgServer
participant K as Keeper
participant BM as Bank Module
U->>CLI: Execute toggle multi-coin rewards command (true/false)
CLI->>MS: Send MsgToggleMultiCoinRewards
MS->>K: Validate request and check current rewards state
alt Rewards need enabling
K->>K: Retrieve and sum pending rewards
K->>BM: Transfer pending rewards to user account
K->>K: Mark rewards as enabled
else Rewards need disabling
K->>K: Remove user's reward enabling entry
end
MS->>CLI: Return response
CLI->>U: Display success message
sequenceDiagram
participant Gov as Governance/Authority
participant CLI as CLI/TX Command
participant MS as MsgServer
participant K as Keeper
participant EVT as Event System
Gov->>CLI: Submit MsgUpdateParams with new parameters payload
CLI->>MS: Send MsgUpdateParams for processing
MS->>K: Validate authority and unmarshal new parameters
K->>K: Update stored parameters via SetParams
K->>EVT: Emit EventUpdateParams (old & new parameters)
MS->>CLI: Return successful response
Suggested reviewers
Poem
✨ Finishing Touches
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: 15
🧹 Nitpick comments (36)
x/multi_coin_rewards/keeper/keeper.go (1)
71-74: Provide a more descriptive panic message or return an error.
Panicking halts the chain if schema initialization fails. This is typical for catastrophic module init failures, but returning (or logging) a more descriptive error message can help operators diagnose issues more quickly.x/multi_coin_rewards/keeper/logic_distribution.go (4)
11-11: Use sdk.AccAddress instead of string for address.
Using an sdk.AccAddress for the "address" parameter would prevent invalid addresses from being passed in, improving type safety.-func (k Keeper) addPendingRewards(ctx sdk.Context, address string, rewards sdk.Coins) { +func (k Keeper) addPendingRewards(ctx sdk.Context, address sdk.AccAddress, rewards sdk.Coins) {
28-34: Avoid silent continuation.
The loop continues if the entry isn’t found, which can mask data inconsistencies or indexing bugs. Consider logging a warning.if !found { + k.Logger().Error("Queue entry not found", "index", index) return true }
56-59: Check the distribution policy’s validity earlier.
Validation of the distribution policy is done here, but if errors are frequent, consider a preflight check or caching.
84-89: Warn about nonexistent pools instead of failing distribution.
Returning an error cancels distribution entirely. Decide if partial distribution to valid pools is acceptable or maintain current approach where any invalid pool halts the process.x/multi_coin_rewards/keeper/getters_compliance.go (2)
55-69: Check the returned "found" flag.
Currently, "GetMultiCoinPendingRewardsEntry"’s "found" flag isn’t used after retrieving an entry by index. Handling the case where the entry is missing could avoid referencing entries that no longer exist.
72-82: Safeguard against missing entries.
A "RemoveMultiCoinPendingRewardsEntry" call with a non-existent entry might silently no-op. If the queue logic is complex, consider logging or checking for existence first.app/upgrades/v2_0/upgrade.go (1)
37-37: Added 'accountKeeper' parameter.Expanding the function signature to include 'accountKeeper' will facilitate module account operations during the upgrade. Ensure that any upstream calls to this function are updated accordingly.
x/multi_coin_rewards/module.go (1)
150-167: Block-based distribution scheduling.Triggering non-claimed rewards distribution every 50 blocks is straightforward. Check for potential performance implications if the distribution logic grows in complexity. Consider using a parameter instead of a hard-coded 50 for more flexibility if needed.
x/multi_coin_rewards/keeper/msg_server.go (1)
7-9: Embedding Keeper in 'msgServer'.By embedding the keeper, the 'msgServer' has direct access to all keeper methods. Just ensure the keeper is not performing complex business logic that might be better placed in dedicated functions for clarity.
x/multi_coin_rewards/types/errors.go (1)
7-8: Fix incorrect comment.The comment "staking errors" is incorrect as these errors are related to multi-coin rewards.
-// staking errors +// multi-coin rewards errorsx/multi_coin_rewards/keeper/keeper_suite_test.go (1)
22-31: Consider using test assertions instead of panics.While using panics in test helpers is acceptable, using test assertions would provide better error messages and test diagnostics.
Apply this diff to improve error handling:
-func payoutRewards(s *i.KeeperTestSuite, staker string, coins sdk.Coins) { +func payoutRewards(s *i.KeeperTestSuite, staker string, coins sdk.Coins) error { err := s.App().BankKeeper.MintCoins(s.Ctx(), mintTypes.ModuleName, coins) if err != nil { - panic(err) + return fmt.Errorf("failed to mint coins: %w", err) } err = s.App().StakersKeeper.PayoutRewards(s.Ctx(), staker, coins, mintTypes.ModuleName) if err != nil { - panic(err) + return fmt.Errorf("failed to payout rewards: %w", err) } + return nil }Then in the test:
err := payoutRewards(s, staker, coins) Expect(err).To(BeNil())x/multi_coin_rewards/keeper/msg_server_distribution_policy.go (1)
11-24: Consider emitting an event for policy changes.While the function correctly validates and updates the policy, it should emit an event to track policy changes for better observability.
Apply this diff to add event emission:
func (k msgServer) SetMultiCoinRewardDistributionPolicy(goCtx context.Context, policy *types.MsgSetMultiCoinRewardsDistributionPolicy) (*types.MsgSetMultiCoinRewardsDistributionPolicyResponse, error) { ctx := sdk.UnwrapSDKContext(goCtx) params := k.GetParams(ctx) if params.MultiCoinDistributionPolicyAdminAddress != policy.Creator { return nil, types.ErrMultiCoinDistributionPolicyInvalidAdminAddress } if err := k.MultiCoinDistributionPolicy.Set(ctx, *policy.Policy); err != nil { return nil, errors.Wrap(err, types.ErrMultiCoinDistributionPolicyInvalid.Error()) } + ctx.EventManager().EmitEvent( + sdk.NewEvent( + types.EventTypeSetMultiCoinDistributionPolicy, + sdk.NewAttribute(types.AttributeKeyCreator, policy.Creator), + sdk.NewAttribute(types.AttributeKeyPolicy, policy.Policy.String()), + ), + ) + return &types.MsgSetMultiCoinRewardsDistributionPolicyResponse{}, nil }x/multi_coin_rewards/client/cli/query.go (1)
8-9: Remove commented imports.Clean up the code by removing the commented imports that are not being used.
- // "strings" - - // "github.com/cosmos/cosmos-sdk/client/flags" - // sdk "github.com/cosmos/cosmos-sdk/types"Also applies to: 13-15
x/multi_coin_rewards/types/params.go (1)
5-6: Add proper documentation for the default value.The comment for
DefaultMultiCoinDistributionPendingTimeshould explain what this value represents (14 days in seconds) and its significance.-// DefaultMultiCoinDistributionPendingTime ... +// DefaultMultiCoinDistributionPendingTime represents the default pending time of 14 days in seconds +// before a multi-coin distribution policy takes effectx/multi_coin_rewards/client/cli/tx_toggle_multi_coin_rewards.go (1)
25-27: Consider usingstrconv.ParseBoolfor more flexible input validation.The current validation only accepts "true" or "false". Using
strconv.ParseBoolwould allow more user-friendly inputs like "1", "t", "TRUE", etc.-if args[0] != "true" && args[0] != "false" { - return fmt.Errorf("value must be 'true' or 'false'") +enabled, err := strconv.ParseBool(args[0]) +if err != nil { + return fmt.Errorf("value must be a valid boolean: %v", err) }x/multi_coin_rewards/types/keys.go (2)
24-32: Document magic numbers in key prefixes.The byte arrays use magic numbers (8, 9) without explanation. Consider adding comments explaining their significance or using named constants.
+const ( + // Key type identifiers + KEY_TYPE_PENDING_REWARDS = 8 + KEY_TYPE_POLICY = 9 +) -MultiCoinPendingRewardsEntryKeyPrefix = []byte{8, 0} +MultiCoinPendingRewardsEntryKeyPrefix = []byte{KEY_TYPE_PENDING_REWARDS, 0}
34-37: Consider using iota for QUEUE_IDENTIFIER.The type could be better implemented as an enum using iota for better type safety and maintainability.
-type QUEUE_IDENTIFIER []byte +type QueueIdentifier int const ( + QueueIdentifierMultiCoinRewards QueueIdentifier = iota + 1 ) -var QUEUE_IDENTIFIER_MULTI_COIN_REWARDS QUEUE_IDENTIFIER = []byte{30, 5} +var QUEUE_IDENTIFIER_MULTI_COIN_REWARDS = QueueIdentifierMultiCoinRewardsx/multi_coin_rewards/client/cli/tx_set_multi_coin_refund_policy.go (1)
17-19: Enhance command documentation.The command's usage and description could be more descriptive. Consider adding:
- Example usage
- Policy format description
- Required permissions
- Use: "set-multi-coin-distribution-policy [path-to-json-file]", - Short: "Broadcast message to update the distribution policy", + Use: "set-multi-coin-distribution-policy [path-to-json-file]", + Short: "Update the multi-coin rewards distribution policy", + Long: `Update the distribution policy for multi-coin rewards. + +Example: + $ kyved tx multi-coin-rewards set-multi-coin-distribution-policy policy.json + +Policy JSON format: + { + "distribution_map": { + "ukex": "70", + "ukyve": "30" + } + } + +Required permissions: Must be executed by the policy admin address.`,x/multi_coin_rewards/types/genesis.go (1)
20-33: Optimize map usage and improve error messages.The validation logic could be optimized and error messages could be more descriptive.
+ // Pre-allocate map with expected size + multiCoinPendingRewardsMap := make(map[string]struct{}, len(gs.MultiCoinPendingRewardsEntries)) - multiCoinPendingRewardsMap := make(map[string]struct{}) for _, elem := range gs.MultiCoinPendingRewardsEntries { index := string(MultiCoinPendingRewardsKeyEntry(elem.Index)) if _, ok := multiCoinPendingRewardsMap[index]; ok { - return fmt.Errorf("duplicated index for multi coin pending rewards entry %v", elem) + return fmt.Errorf("duplicate index %d found in multi coin pending rewards entries", elem.Index) } if elem.Index > gs.QueueStatePendingRewards.HighIndex { - return fmt.Errorf(" multi coin pending rewards entry index too high: %v", elem) + return fmt.Errorf("index %d exceeds maximum allowed value %d", elem.Index, gs.QueueStatePendingRewards.HighIndex) } if elem.Index < gs.QueueStatePendingRewards.LowIndex { - return fmt.Errorf(" multi coin pending rewards entry index too low: %v", elem) + return fmt.Errorf("index %d is below minimum allowed value %d", elem.Index, gs.QueueStatePendingRewards.LowIndex) }x/multi_coin_rewards/keeper/logic_queue.go (1)
31-43: Improve queue processing loop condition and variable naming.The loop condition and variable naming could be clearer. The variable
commissionChangePerformedseems to be misnamed for the context.- for commissionChangePerformed := true; commissionChangePerformed; { - commissionChangePerformed = false + for shouldContinueProcessing := true; shouldContinueProcessing; { + shouldContinueProcessing = false entryRemoved := processEntry(queueState.LowIndex + 1) if entryRemoved { if queueState.LowIndex < queueState.HighIndex { queueState.LowIndex += 1 - commissionChangePerformed = true + shouldContinueProcessing = true } } }x/multi_coin_rewards/keeper/msg_server_toggle.go (2)
35-38: Add validation before sending coins.Verify that totalRewards is not empty before attempting to send coins to avoid unnecessary transactions.
+ if totalRewards.IsZero() { + // Skip bank transfer if no rewards to send + if err := k.MultiCoinRewardsEnabled.Set(ctx, accountAddress); err != nil { + return nil, err + } + return &types.MsgToggleMultiCoinRewardsResponse{}, nil + } err := k.bankKeeper.SendCoinsFromModuleToAccount(ctx, types.ModuleName, accountAddress, totalRewards) if err != nil { return nil, err }
55-56: Consider adding event emission.Add event emission to track when multi-coin rewards are toggled.
+ ctx.EventManager().EmitEvent( + sdk.NewEvent( + types.EventTypeToggleMultiCoinRewards, + sdk.NewAttribute(types.AttributeKeyCreator, toggle.Creator), + sdk.NewAttribute(types.AttributeKeyEnabled, strconv.FormatBool(toggle.Enabled)), + ), + ) return &types.MsgToggleMultiCoinRewardsResponse{}, nilx/multi_coin_rewards/keeper/msg_server_update_params_test.go (3)
65-77: Consider adding more test cases for invalid authority.The current test only checks one scenario with a dummy address. Consider adding tests for:
- Empty authority
- Malformed address
- Authority with insufficient permissions
168-196: Enhance invalid payload test coverage.The test could be expanded to include more edge cases:
- Empty payload
- Malformed JSON
- Invalid types for both parameters
- Missing required fields
46-51: Extract common setup code into helper functions.The BeforeEach block contains repeated code that could be extracted into helper functions for better maintainability.
+func (s *KeeperTestSuite) setupTestValidator() sdk.AccAddress { + delegations, _ := s.App().StakingKeeper.GetAllDelegations(s.Ctx()) + return sdk.MustAccAddressFromBech32(delegations[0].DelegatorAddress) +} BeforeEach(func() { s = i.NewCleanChain() - delegations, _ := s.App().StakingKeeper.GetAllDelegations(s.Ctx()) - voter = sdk.MustAccAddressFromBech32(delegations[0].DelegatorAddress) + voter = s.setupTestValidator() })x/multi_coin_rewards/keeper/logic_distribution_test.go (2)
73-75: Replace magic number with a named constant.The loop uses a hardcoded value of 100 iterations. This should be extracted into a named constant for better maintainability and clarity.
+const defaultCommitIterations = 100 -for i := 0; i < 100; i++ { +for i := 0; i < defaultCommitIterations; i++ { s.CommitAfterSeconds(1) }
214-216: Add explanatory comments for weight values.The weight values in the test case lack explanation. Consider adding comments to clarify the expected distribution ratios.
{ PoolId: 0, - Weight: math.LegacyMustNewDecFromStr("1"), // -> 16acoin + Weight: math.LegacyMustNewDecFromStr("1"), // 1/3 of total weight -> 16 acoin (33.33%) }, { PoolId: 1, - Weight: math.LegacyMustNewDecFromStr("2"), // -> 33acoin + Weight: math.LegacyMustNewDecFromStr("2"), // 2/3 of total weight -> 33 acoin (66.67%) },app/app_config.go (1)
188-190: Group related module accounts together.Consider grouping the multi-coin rewards module accounts with other related modules for better organization.
{Account: stakerstypes.ModuleName}, +{Account: multicoinrewardstypes.ModuleName}, +{Account: multicoinrewardstypes.MultiCoinRewardsRedistributionAccountName}, {Account: teamtypes.ModuleName}, {Account: funderstypes.ModuleName}, -{Account: multicoinrewardstypes.ModuleName}, -{Account: multicoinrewardstypes.MultiCoinRewardsRedistributionAccountName},proto/kyve/multi_coin_rewards/v1beta1/params.proto (1)
7-15: Params Message Definition Quality:
The new "Params" message clearly defines the fields "multi_coin_distribution_policy_admin_address" and "multi_coin_distribution_pending_time". However, the comment for "multi_coin_distribution_pending_time" is rather terse. Enhancing it with more detail on its purpose would improve clarity.proto/kyve/multi_coin_rewards/v1beta1/events.proto (2)
10-19: EventUpdateParams Message Clarity:
The "EventUpdateParams" message is well-formed, with clear definitions for the old and new parameters as well as the payload. If further context is needed for the payload’s content, consider expanding its comment.
21-22: TODO Reminder:
There is a TODO comment to add more events. Be sure to follow up on this to cover additional event scenarios in the module.proto/kyve/multi_coin_rewards/v1beta1/types.proto (1)
11-58: Multi-Coin Rewards Types Definition:
The message types (QueueState, MultiCoinPendingRewardsEntry, MultiCoinDistributionPolicy, MultiCoinDistributionDenomEntry, and MultiCoinDistributionPoolWeightEntry) are well-structured and make effective use of gogoproto options. Verify that field types (for example, using int64 for creation_date) satisfy the required precision and that the custom type settings align with downstream usage.proto/kyve/multi_coin_rewards/v1beta1/tx.proto (2)
39-47: MsgToggleMultiCoinRewards Naming Consistency:
The comment preceding this message refers to "MsgEnableMultiCoinReward", which differs from the actual message name "MsgToggleMultiCoinRewards." For clarity, update the comment to accurately reflect that the message toggles the multi-coin rewards status.
52-60: MsgSetMultiCoinRewardsDistributionPolicy Clarity:
The comment for this message appears to be a copy-paste from another message (mentioning enabling rewards) rather than describing its role in setting the distribution policy. Please update it to accurately describe its purpose.config.yml (1)
72-75: Approved: Multi-Coin Rewards Configuration Added.
The new "multi_coin_rewards" section under genesis.app_state is properly integrated with parameters for pending time and the policy admin address. Consider adding inline comments or documentation in the config to explain the purpose of each new parameter for future maintainability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (10)
go.sumis excluded by!**/*.sumgo.work.sumis excluded by!**/*.sumx/multi_coin_rewards/types/events.pb.gois excluded by!**/*.pb.gox/multi_coin_rewards/types/genesis.pb.gois excluded by!**/*.pb.gox/multi_coin_rewards/types/params.pb.gois excluded by!**/*.pb.gox/multi_coin_rewards/types/query.pb.gois excluded by!**/*.pb.gox/multi_coin_rewards/types/query.pb.gw.gois excluded by!**/*.pb.gw.gox/multi_coin_rewards/types/tx.pb.gois excluded by!**/*.pb.gox/multi_coin_rewards/types/types.pb.gois excluded by!**/*.pb.gox/stakers/types/genesis.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (57)
api/kyve/multi_coin_rewards/module/module.pulsar.go(1 hunks)app/app.go(5 hunks)app/app_config.go(6 hunks)app/upgrades/v2_0/store.go(1 hunks)app/upgrades/v2_0/upgrade.go(3 hunks)config.yml(1 hunks)go.mod(4 hunks)proto/kyve/multi_coin_rewards/module/module.proto(1 hunks)proto/kyve/multi_coin_rewards/v1beta1/events.proto(1 hunks)proto/kyve/multi_coin_rewards/v1beta1/genesis.proto(1 hunks)proto/kyve/multi_coin_rewards/v1beta1/params.proto(1 hunks)proto/kyve/multi_coin_rewards/v1beta1/query.proto(1 hunks)proto/kyve/multi_coin_rewards/v1beta1/tx.proto(1 hunks)proto/kyve/multi_coin_rewards/v1beta1/types.proto(1 hunks)proto/kyve/stakers/v1beta1/genesis.proto(1 hunks)testutil/integration/integration.go(2 hunks)util/expected_keepers.go(1 hunks)x/global/ante_test.go(1 hunks)x/multi_coin_rewards/client/cli/query.go(1 hunks)x/multi_coin_rewards/client/cli/query_params.go(1 hunks)x/multi_coin_rewards/client/cli/tx.go(1 hunks)x/multi_coin_rewards/client/cli/tx_set_multi_coin_refund_policy.go(1 hunks)x/multi_coin_rewards/client/cli/tx_toggle_multi_coin_rewards.go(1 hunks)x/multi_coin_rewards/genesis.go(1 hunks)x/multi_coin_rewards/keeper/exported.go(1 hunks)x/multi_coin_rewards/keeper/getters_compliance.go(1 hunks)x/multi_coin_rewards/keeper/getters_params.go(1 hunks)x/multi_coin_rewards/keeper/getters_queue.go(1 hunks)x/multi_coin_rewards/keeper/grpc_query.go(1 hunks)x/multi_coin_rewards/keeper/keeper.go(1 hunks)x/multi_coin_rewards/keeper/keeper_suite_test.go(1 hunks)x/multi_coin_rewards/keeper/logic_distribution.go(1 hunks)x/multi_coin_rewards/keeper/logic_distribution_test.go(1 hunks)x/multi_coin_rewards/keeper/logic_queue.go(1 hunks)x/multi_coin_rewards/keeper/msg_server.go(1 hunks)x/multi_coin_rewards/keeper/msg_server_distribution_policy.go(1 hunks)x/multi_coin_rewards/keeper/msg_server_distribution_policy_test.go(1 hunks)x/multi_coin_rewards/keeper/msg_server_toggle.go(1 hunks)x/multi_coin_rewards/keeper/msg_server_toggle_test.go(1 hunks)x/multi_coin_rewards/keeper/msg_server_update_params.go(1 hunks)x/multi_coin_rewards/keeper/msg_server_update_params_test.go(1 hunks)x/multi_coin_rewards/module.go(1 hunks)x/multi_coin_rewards/types/codec.go(1 hunks)x/multi_coin_rewards/types/errors.go(1 hunks)x/multi_coin_rewards/types/expected_keepers.go(1 hunks)x/multi_coin_rewards/types/genesis.go(1 hunks)x/multi_coin_rewards/types/keys.go(1 hunks)x/multi_coin_rewards/types/message_set_multi_coin_refund_policy.go(1 hunks)x/multi_coin_rewards/types/message_toggle_multi_coin_rewards.go(1 hunks)x/multi_coin_rewards/types/msgs.go(1 hunks)x/multi_coin_rewards/types/params.go(1 hunks)x/multi_coin_rewards/types/types.go(1 hunks)x/query/keeper/keeper.go(3 hunks)x/query/module.go(1 hunks)x/stakers/genesis.go(2 hunks)x/stakers/module.go(1 hunks)x/stakers/types/genesis.go(1 hunks)
✅ Files skipped from review due to trivial changes (3)
- proto/kyve/stakers/v1beta1/genesis.proto
- x/stakers/genesis.go
- api/kyve/multi_coin_rewards/module/module.pulsar.go
🧰 Additional context used
🪛 Buf (1.47.2)
proto/kyve/multi_coin_rewards/module/module.proto
4-4: import "cosmos/app/v1alpha1/module.proto": file does not exist
(COMPILE)
proto/kyve/multi_coin_rewards/v1beta1/events.proto
5-5: import "gogoproto/gogo.proto": file does not exist
(COMPILE)
proto/kyve/multi_coin_rewards/v1beta1/types.proto
5-5: import "amino/amino.proto": file does not exist
(COMPILE)
proto/kyve/multi_coin_rewards/v1beta1/params.proto
3-3: Files with package "kyve.multi_coin_rewards.v1beta1" must be within a directory "kyve/multi_coin_rewards/v1beta1" relative to root but were in directory "proto/kyve/multi_coin_rewards/v1beta1".
(PACKAGE_DIRECTORY_MATCH)
proto/kyve/multi_coin_rewards/v1beta1/tx.proto
5-5: import "cosmos/msg/v1/msg.proto": file does not exist
(COMPILE)
proto/kyve/multi_coin_rewards/v1beta1/query.proto
5-5: import "amino/amino.proto": file does not exist
(COMPILE)
proto/kyve/multi_coin_rewards/v1beta1/genesis.proto
5-5: import "gogoproto/gogo.proto": file does not exist
(COMPILE)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Summary
🔇 Additional comments (68)
x/multi_coin_rewards/keeper/keeper.go (2)
26-26: Consider validating the "authority" value.
While it’s common to store the authority as a string, adding a simple validation or sanity check (e.g., verifying whether it’s a valid address) could help detect configuration mistakes early.
81-83: Logger instantiation looks good.
Returning the logger with a "module" string is consistent with Cosmos SDK patterns. No changes needed.x/multi_coin_rewards/keeper/getters_compliance.go (2)
20-38: Handle duplicate entries with the same index.
Overwriting existing entries might cause data corruption if "pendingRewards.Index" is reused. Consider rejecting duplicates or logging a warning.
101-111: Ensure iteration is safe for large sets.
If a large number of addresses have multi-coin rewards enabled, this loop could become expensive. Lazy pagination or a streaming approach might be more scalable.
[performance]app/upgrades/v2_0/upgrade.go (3)
7-10: Imports organization looks consistent!The newly added imports for multi-coin rewards and the Cosmos SDK auth modules appear properly referenced. Confirm that you need both "authkeeper" and "authTypes"; if either is unused, it could be removed to reduce redundancy.
53-53: Ensure account creation logic is invoked at upgrade.This call to 'EnsureMultiCoinDistributionAccount' is a solid approach to guarantee the module account is established. Verify there's no possibility of repeated calls inadvertently overwriting an existing valid setup.
61-76: Robust module account creation with fallback for existing accounts.This function ensures the correct module account is present. Some considerations:
• If the existing account isn't a module account, the code forcibly transforms it. Ensure that it doesn't inadvertently overwrite attributes unrelated to multi-coin rewards.
• Remember to handle any possible type assertions on the existing module account if needed in the future.x/multi_coin_rewards/module.go (4)
1-30: Imports look coherent and minimal.All imported packages are used meaningfully for module definitions, CLI commands, keeper references, and the cosmos-sdk structures. No glaring duplicates found.
46-49: Clear definition of AppModuleBasic.The minimal struct storing the codec is consistent with Cosmos SDK module patterns. Good job keeping it concise.
123-127: Registering gRPC services.This approach ties the Msg and Query server to the Configurator effectively. Ensure all message types are registered in 'types/msgs.go' so they can be dispatched properly.
207-229: Dependency injection setup is well-structured.The 'ProvideModule' function neatly aggregates and configures the keeper, populating the app module outputs. This is aligned with recommended depinject patterns. Great approach to setting a default authority.
x/multi_coin_rewards/keeper/msg_server.go (1)
13-15: Proper server constructor.'NewMsgServerImpl' cleanly installs the keeper into the 'msgServer'. Great for testability and clarity. The rest of the file is minimal and follows standard Cosmos SDK conventions.
x/multi_coin_rewards/types/expected_keepers.go (1)
9-14: LGTM! Well-designed interface with clear method signatures.The
PoolKeeperinterface is well-structured with:
- Clear method names that describe their purpose
- Appropriate error handling for critical operations
- Consistent use of
sdk.Contextacross methodsapp/upgrades/v2_0/store.go (1)
6-6: LGTM! Proper integration of multi-coin rewards module in store upgrades.The changes correctly add the multi-coin rewards module to the store upgrades for v2.0.
Also applies to: 12-12
x/multi_coin_rewards/client/cli/tx.go (1)
14-27: LGTM! Clean implementation following CLI best practices.The command is well-structured with:
- Standard Cobra command setup
- Appropriate validation
- Clear subcommand organization
x/multi_coin_rewards/client/cli/query_params.go (1)
13-35: LGTM! Well-structured CLI command implementation.The command follows the standard Cosmos SDK CLI patterns, with proper error handling and query flags.
x/multi_coin_rewards/keeper/keeper_suite_test.go (1)
17-20: LGTM! Standard test suite setup.The test suite is properly configured with failure handling and module name.
x/multi_coin_rewards/types/msgs.go (2)
14-17: LGTM! Safe to ignore error as address is validated in ValidateBasic.The function correctly returns the authority address as the expected signer.
20-35: LGTM! Thorough message validation.The function follows the standard pattern for message validation and properly handles all error cases.
x/multi_coin_rewards/client/cli/query.go (1)
17-32: LGTM!The implementation follows standard Cosmos SDK CLI patterns and correctly sets up the query command for the multi-coin rewards module.
x/multi_coin_rewards/keeper/msg_server_update_params.go (1)
28-32: LGTM!The event emission is correctly implemented with all necessary fields.
x/multi_coin_rewards/keeper/getters_queue.go (2)
9-21: LGTM!The
GetQueueStateimplementation correctly handles nil state and usesMustUnmarshalfor state deserialization, which is appropriate for handling invalid state by panicking.
23-29: LGTM!The
SetQueueStateimplementation correctly serializes and stores the state usingMustMarshal.x/multi_coin_rewards/types/params.go (2)
19-25: Verify if empty admin address is intended in DefaultParams.Please confirm if having an empty
MultiCoinDistributionPolicyAdminAddressinDefaultParamsis the intended behavior. If not, consider setting a default admin address.
27-37: LGTM!The parameter validation correctly checks for valid Bech32 addresses when an admin address is provided.
x/multi_coin_rewards/types/codec.go (3)
10-14: LGTM! Message registration follows Cosmos SDK conventions.The message type registration follows the standard Cosmos SDK pattern, using the correct module namespace prefix "kyve/multi_coin_rewards/".
16-20: LGTM! Interface registration is properly implemented.The implementation correctly registers all message types as implementations of the
sdk.Msginterface.
22-28: LGTM! Codec initialization is complete.The initialization properly registers all necessary codecs: legacy amino, crypto, and SDK.
x/multi_coin_rewards/client/cli/tx_toggle_multi_coin_rewards.go (2)
29-32: LGTM! Message construction is correct.The message is properly constructed with the creator's address and the parsed boolean value.
34-36: LGTM! Basic validation is properly implemented.The implementation correctly validates the message before broadcasting.
x/multi_coin_rewards/types/message_toggle_multi_coin_rewards.go (1)
37-43: LGTM! Basic validation is properly implemented.The implementation correctly validates the creator's address.
x/multi_coin_rewards/types/keys.go (1)
39-45: LGTM! Key generation utilities are properly implemented.The implementation correctly uses the util package for consistent key generation.
x/multi_coin_rewards/client/cli/tx_set_multi_coin_refund_policy.go (1)
26-29: 🛠️ Refactor suggestionAdd file path validation.
The command reads a file without validating the path. Consider adding checks for:
- File size limits to prevent memory exhaustion
- File extension validation (.json)
- Path traversal protection
- file, err := os.ReadFile(args[0]) + // Validate file path + if !strings.HasSuffix(args[0], ".json") { + return fmt.Errorf("file must have .json extension") + } + + // Get file info for size check + fileInfo, err := os.Stat(args[0]) + if err != nil { + return fmt.Errorf("failed to get file info: %w", err) + } + + // Limit file size (e.g., 1MB) + if fileInfo.Size() > 1024*1024 { + return fmt.Errorf("file size exceeds 1MB limit") + } + + file, err := os.ReadFile(args[0])x/multi_coin_rewards/types/message_set_multi_coin_refund_policy.go (1)
20-27:⚠️ Potential issueHandle GetSigners error gracefully.
The
GetSignersmethod panics on invalid address. Consider returning an error instead.-func (msg *MsgSetMultiCoinRewardsDistributionPolicy) GetSigners() []sdk.AccAddress { +func (msg *MsgSetMultiCoinRewardsDistributionPolicy) GetSigners() ([]sdk.AccAddress, error) { creator, err := sdk.AccAddressFromBech32(msg.Creator) if err != nil { - panic(err) + return nil, errors.Wrapf(errorsTypes.ErrInvalidAddress, "invalid creator address: %s", err) } - return []sdk.AccAddress{creator} + return []sdk.AccAddress{creator}, nil }Likely invalid or redundant comment.
x/multi_coin_rewards/keeper/exported.go (1)
26-27: Verify coin operations for potential panics.The
Suboperation might panic ifenabledRewardscontains denominations not present incoins. Add validation to ensure safe coin operations.+ // Verify that native denom exists in coins + if !coins.HasDenom(globalTypes.Denom) { + return sdk.Coins{} + } enabledRewards := sdk.NewCoins(sdk.NewCoin(globalTypes.Denom, coins.AmountOf(globalTypes.Denom))) + // Verify subtraction is valid + if !coins.IsAllGTE(enabledRewards) { + return sdk.Coins{} + } disabledRewards := coins.Sub(enabledRewards...)x/multi_coin_rewards/types/types.go (1)
9-51: LGTM! Well-structured implementation with thorough validation.The implementation includes comprehensive validation checks:
- Positive weights validation
- Duplicate pool IDs detection
- Weight normalization
- Duplicate denoms prevention
x/query/keeper/keeper.go (1)
31-31: LGTM! Clean type update for distribution keeper.The change from
distrkeeper.Keepertoutil.DistributionKeeperis consistent across the keeper struct and constructor.Also applies to: 51-51
x/stakers/types/genesis.go (1)
76-80: LGTM! Consistent field renaming in validation logic.The update from
QueueStateStateFractiontoQueueStateStakeFractionis applied consistently in the validation checks.util/expected_keepers.go (1)
24-24: LGTM!The addition of
GetAllBalancesmethod to theBankKeeperinterface aligns with the Cosmos SDK's implementation and is necessary for the multi-coin rewards module.x/multi_coin_rewards/keeper/msg_server_distribution_policy_test.go (4)
29-50: LGTM! Test setup is comprehensive.The test setup properly initializes a clean chain, creates a pool, and sets up a validator with the required permissions.
52-54: LGTM! Test cleanup is well-handled.The
PerformValidityChecks()after each test ensures the chain state remains valid.
56-94: LGTM! Valid test case is well-structured.The test verifies:
- Policy update with multiple entries
- Multiple denominations (acoin, bcoin, ccoin)
- Different weights for pools
- Proper parsing of the distribution map
96-226: LGTM! Invalid test cases cover essential error scenarios.The test suite thoroughly validates error handling for:
- Duplicate denomination entries
- Duplicate pool entries for the same denomination
- Negative weights
- Zero weights
Each test properly verifies both the error message and the policy state.
x/stakers/module.go (1)
189-189: LGTM!The update to use
util.DistributionKeeperaligns with the module's integration of the multi-coin rewards system.x/query/module.go (1)
182-182: LGTM!The update to use
util.DistributionKeeperis consistent with the changes in other modules and supports the multi-coin rewards integration.testutil/integration/integration.go (1)
233-233: LGTM! Good addition of ValAddress field.The addition of the ValAddress field to TestValidatorAddress struct improves the test utility by providing easy access to validator addresses.
x/global/ante_test.go (1)
294-295: Document whyBaseCostchanges with implementation.The TODO comment raises a valid question about why
BaseCostchanges with implementation changes. Consider documenting the factors that influence this value to help future maintainers understand the relationship between implementation changes and gas costs.x/multi_coin_rewards/keeper/msg_server_toggle_test.go (1)
1-311: Well-structured and comprehensive test coverage!The test suite thoroughly covers various scenarios for multi-coin rewards, including:
- Native and foreign rewards handling
- Mixed rewards scenarios
- Toggle functionality
- Claim periods and pending rewards
- Edge cases like empty claims
The tests follow the AAA pattern and include clear assertions.
app/app.go (2)
84-85: LGTM: Clean module import.The multi-coin rewards module is properly imported with side-effects handling.
124-124: LGTM: Consistent keeper implementation.The changes maintain consistency in the keeper implementation:
DistributionKeeperis now a pointer type, aligning with other keeper typesMultiCoinRewardsKeeperis properly integrated into the app structureAlso applies to: 156-156
proto/kyve/multi_coin_rewards/module/module.proto (2)
7-12: LGTM: Clean module definition.The
Modulemessage is well-defined with:
- Clear go import path option
- Properly documented authority field with default behavior
4-4: 💡 Verification agentVerify the import path resolution.
The static analysis tool indicates that the import file
cosmos/app/v1alpha1/module.protodoes not exist. Please verify that the import path is correct and the file is available in your proto files directory.Run this script to check the import path:
❌ Verification inconclusive
🏁 Script executed:
#!/bin/bash # Description: Check if the proto import file exists fd -t f "module.proto" proto/cosmos/app/v1alpha1/Length of output: 163
Import path resolution issue detected
It appears that the repository does not contain the directory (and consequently the file) referenced bycosmos/app/v1alpha1/module.proto. The error from the initial command indicates that the search path isn’t valid, which confirms that the expected directory “proto/cosmos/app/v1alpha1/” does not exist. This matches the static analysis tool’s warning.To further verify, please run the following script to search the entire repository for the file with the expected path:
If this script produces no output, it confirms that the file does not exist at that location. In that case, please update the import in
•proto/kyve/multi_coin_rewards/module/module.proto
to point to the correct location if the file was relocated, or remove the import if it is no longer needed.
🏁 Script executed:
#!/bin/bash # Search for the module.proto file anywhere in the repo and filter for the expected path pattern find . -type f | grep -E "proto/cosmos/app/v1alpha1/module.proto"Length of output: 187
Import Path Resolution Error Detected
Based on the searches performed:
- A search for "proto/cosmos/app/v1alpha1/module.proto" did not return any matches.
- A repository-wide search for files named "module.proto" within paths containing "cosmos" also did not locate the expected file.
This confirms that the import statement in
•proto/kyve/multi_coin_rewards/module/module.proto
pointing tocosmos/app/v1alpha1/module.protodoes not resolve to an existing file.Please update the import to use the correct file path or remove it if the file is no longer available.
🧰 Tools
🪛 Buf (1.47.2)
4-4: import "cosmos/app/v1alpha1/module.proto": file does not exist
(COMPILE)
proto/kyve/multi_coin_rewards/v1beta1/params.proto (1)
1-6:⚠️ Potential issueDirectory Structure Compliance:
The package is declared as “kyve.multi_coin_rewards.v1beta1”, yet the file is located under "proto/kyve/multi_coin_rewards/v1beta1". According to Buf guidelines, it should reside in "kyve/multi_coin_rewards/v1beta1" relative to the repository root. Consider relocating the file to ensure proper package management.🧰 Tools
🪛 Buf (1.47.2)
3-3: Files with package "kyve.multi_coin_rewards.v1beta1" must be within a directory "kyve/multi_coin_rewards/v1beta1" relative to root but were in directory "proto/kyve/multi_coin_rewards/v1beta1".
(PACKAGE_DIRECTORY_MATCH)
proto/kyve/multi_coin_rewards/v1beta1/events.proto (1)
5-5:⚠️ Potential issueVerify Import of gogoproto:
The import "gogoproto/gogo.proto" is reported as not found. Please verify that the dependency is available, or update the import path accordingly.🧰 Tools
🪛 Buf (1.47.2)
5-5: import "gogoproto/gogo.proto": file does not exist
(COMPILE)
proto/kyve/multi_coin_rewards/v1beta1/genesis.proto (2)
11-26: GenesisState Message Structure:
The "GenesisState" message efficiently aggregates the module’s parameters, pending rewards entries, queue state, enabled coins, and the distribution policy. Double-check that the non-nullable constraints match the module’s initialization and validation requirements.
5-5:⚠️ Potential issueVerify gogoproto Import:
Similar to other files, the import "gogoproto/gogo.proto" isn’t found. Confirm that the correct dependency path is referenced or update it if needed.🧰 Tools
🪛 Buf (1.47.2)
5-5: import "gogoproto/gogo.proto": file does not exist
(COMPILE)
proto/kyve/multi_coin_rewards/v1beta1/types.proto (1)
5-7:⚠️ Potential issueDependency Import Verification:
The import "amino/amino.proto" is flagged as not existing. Please verify the correct path for this dependency or consider removing it if it’s unnecessary. Additionally, confirm that "gogoproto/gogo.proto" is accessible.🧰 Tools
🪛 Buf (1.47.2)
5-5: import "amino/amino.proto": file does not exist
(COMPILE)
proto/kyve/multi_coin_rewards/v1beta1/tx.proto (3)
11-24: Msg Service Definition:
The "Msg" service is clearly defined with RPC methods for updating parameters, toggling rewards, and setting the distribution policy. Ensure that the corresponding message handlers implement the desired functionality correctly.
26-34: MsgUpdateParams Message:
This message concisely defines an authority (with its signer option) and a payload for updating parameters. Verify that the payload’s format meets the requirements for module parameter updates.
5-7:⚠️ Potential issueVerify Import Paths:
The import "cosmos/msg/v1/msg.proto" is reported as not existing. Verify that the correct import paths for Cosmos SDK message definitions are used, and similarly check the "cosmos_proto/cosmos.proto" import.🧰 Tools
🪛 Buf (1.47.2)
5-5: import "cosmos/msg/v1/msg.proto": file does not exist
(COMPILE)
proto/kyve/multi_coin_rewards/v1beta1/query.proto (4)
14-30: Approved: gRPC Query Service Definition.
The Query service and its RPC methods (Params, MultiCoinDistributionPolicyQuery, MultiCoinStatus) are correctly defined with the appropriate HTTP options. The overall structure aligns with Cosmos SDK standards.
5-5:⚠️ Potential issueCritical: Verify Import Reference.
The import of "amino/amino.proto" is flagged by static analysis as missing. Please verify that the file exists under the expected path or update the import to the correct location to avoid compilation errors.🧰 Tools
🪛 Buf (1.47.2)
5-5: import "amino/amino.proto": file does not exist
(COMPILE)
41-44: 🛠️ Refactor suggestionDocstring Mismatch: Request Message Comment.
The comment on line 41 labels the message as "QueryMultiCoinDistributionPolicyRequest", yet the actual message defined on line 51 is "QueryMultiCoinStatusRequest". Please update the comment or rename the message so that documentation accurately reflects the message’s intent.
56-59: 🛠️ Refactor suggestionDocstring Mismatch: Response Message Comment.
The comment on line 56 refers to a "QueryMultiCoinDistributionPolicyResponse" while the defined message is "QueryMultiCoinStatusResponse". For clarity and consistency, update the comment to reference the correct message name.go.mod (4)
22-22: Approved: Dependency Update for cosmos-db.
The update to "github.com/cosmos/cosmos-db v1.1.1" appears to be in line with our requirements. Please verify compatibility with the multi-coin rewards module.
45-45: Approved: Updated spf13/cast Version.
The version bump for "github.com/spf13/cast" to v1.7.1 is noted. Ensure that components relying on this library are tested for any breaking changes.
110-110: Approved: Ledger-Cosmos-Go Version Bump.
The dependency "github.com/cosmos/ledger-cosmos-go" has been updated to v0.14.0. This update should support the enhancements brought by the multi-coin rewards functionality; please run integration tests to confirm full compatibility.
373-377: Verify: Custom Cosmos SDK Replacement Directive.
The replace directive for "github.com/cosmos/cosmos-sdk" now points to "github.com/KYVENetwork/cosmos-sdk v0.50.11-kyve-rc0". This customization is critical for integrating the new multi-coin rewards module. Please verify that all interfaces from the forked SDK are compatible with the rest of the code and consider updating the inline comment with a reference to any related issue or documentation.
troykessler
left a comment
troykessler
left a comment
There was a problem hiding this comment.
Please add the module specs and include a description of the token flow (on which module accounts the rewards are moved on which steps) and a description of the policy map so users know how to submit proposals when updating it
# Conflicts: # app/upgrades/v2_0/upgrade.go
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
x/multi_coin_rewards/genesis.go (2)
18-23:⚠️ Potential issueReplace panic with proper error handling in InitGenesis.
Instead of panicking on error, return an error to allow for graceful chain initialization.
- for _, entry := range genState.MultiCoinEnabled { - err := k.MultiCoinRewardsEnabled.Set(ctx, sdk.MustAccAddressFromBech32(entry)) - if err != nil { - panic(err) - } - } + for _, entry := range genState.MultiCoinEnabled { + addr, err := sdk.AccAddressFromBech32(entry) + if err != nil { + return fmt.Errorf("invalid address %s in genesis state: %w", entry, err) + } + if err := k.MultiCoinRewardsEnabled.Set(ctx, addr); err != nil { + return fmt.Errorf("failed to set enabled address %s: %w", entry, err) + } + }
37-41:⚠️ Potential issueReplace panic with proper error handling in ExportGenesis.
Instead of panicking on policy retrieval error, handle it gracefully.
policy, err := k.MultiCoinDistributionPolicy.Get(ctx) if err != nil { - panic(err) + // Log error and continue with nil policy + genesis.MultiCoinDistributionPolicy = nil + } else { + genesis.MultiCoinDistributionPolicy = &policy } - genesis.MultiCoinDistributionPolicy = &policyx/multi_coin_rewards/keeper/grpc_query.go (1)
43-43:⚠️ Potential issueHandle error from GetMultiCoinPendingRewardsEntriesByIndex2.
The error from
GetMultiCoinPendingRewardsEntriesByIndex2is ignored, which could mask potential issues.- entries, _ := k.GetMultiCoinPendingRewardsEntriesByIndex2(sdk.UnwrapSDKContext(ctx), request.Address) + entries, err := k.GetMultiCoinPendingRewardsEntriesByIndex2(sdk.UnwrapSDKContext(ctx), request.Address) + if err != nil { + return nil, status.Errorf(codes.Internal, "failed to get pending rewards: %v", err) + }
🧹 Nitpick comments (11)
x/multi_coin_rewards/keeper/keeper_suite_test.go (1)
22-31: **Use Ginkgo’s assertion instead of panicking. **Panicking on errors prevents graceful handling during tests. Consider using Ginkgo's expectations for consistency:
func payoutRewards(s *i.KeeperTestSuite, staker string, coins sdk.Coins) { - err := s.App().BankKeeper.MintCoins(s.Ctx(), mintTypes.ModuleName, coins) - if err != nil { - panic(err) - } + Expect( + s.App().BankKeeper.MintCoins(s.Ctx(), mintTypes.ModuleName, coins), + ).ToNot(HaveOccurred()) - err = s.App().StakersKeeper.PayoutRewards(s.Ctx(), staker, coins, mintTypes.ModuleName) - if err != nil { - panic(err) - } + Expect( + s.App().StakersKeeper.PayoutRewards(s.Ctx(), staker, coins, mintTypes.ModuleName), + ).ToNot(HaveOccurred()) }x/multi_coin_rewards/types/keys.go (1)
28-29: Add missing documentation for MultiCoinDistributionPolicyKey.The comment for
MultiCoinDistributionPolicyKeyis incomplete. Add a description of what this key prefix is used for.- // MultiCoinDistributionPolicyKey + // MultiCoinDistributionPolicyKey is the key prefix for storing the multi-coin distribution policy.x/multi_coin_rewards/keeper/grpc_query.go (1)
32-55: Improve context handling in MultiCoinStatus.The context unwrapping is inconsistent. Use unwrapped context consistently throughout the method.
func (k Keeper) MultiCoinStatus(ctx context.Context, request *types.QueryMultiCoinStatusRequest) (*types.QueryMultiCoinStatusResponse, error) { + sdkCtx := sdk.UnwrapSDKContext(ctx) + account, err := sdk.AccAddressFromBech32(request.Address) if err != nil { return nil, err } - has, err := k.MultiCoinRewardsEnabled.Has(ctx, account) + has, err := k.MultiCoinRewardsEnabled.Has(sdkCtx, account) if err != nil { return nil, err } - entries, _ := k.GetMultiCoinPendingRewardsEntriesByIndex2(sdk.UnwrapSDKContext(ctx), request.Address) + entries, err := k.GetMultiCoinPendingRewardsEntriesByIndex2(sdkCtx, request.Address) + if err != nil { + return nil, status.Errorf(codes.Internal, "failed to get pending rewards: %v", err) + }x/multi_coin_rewards/spec/03_messages.md (1)
9-10: Add missing comma for better readability.A comma should be added after "rewards" to improve readability.
Apply this diff:
-User can enable or disable the retrieval of multi-coin-rewards. If they -enable multi-coin rewards all current pending rewards will be claimed. +User can enable or disable the retrieval of multi-coin-rewards. If they +enable multi-coin rewards, all current pending rewards will be claimed.🧰 Tools
🪛 LanguageTool
[uncategorized] ~10-~10: A comma might be missing here.
Context: ...oin-rewards. If they enable multi-coin rewards all current pending rewards will be cla...(AI_EN_LECTOR_MISSING_PUNCTUATION_COMMA)
x/multi_coin_rewards/spec/07_exported.md (1)
11-16: Fix indentation in interface documentation.The interface is well-defined, but there are inconsistencies in the indentation (hard tabs vs spaces).
Apply this diff to use consistent indentation:
type MultiCoinRewardsKeeper interface { // HandleMultiCoinRewards checks if the user has opted in to receive multi-coin rewards - // and returns the amount which can get paid out. - HandleMultiCoinRewards(goCtx context.Context, withdrawAddress sdk.AccAddress, coins sdk.Coins) sdk.Coins + // and returns the amount which can get paid out. + HandleMultiCoinRewards(goCtx context.Context, withdrawAddress sdk.AccAddress, coins sdk.Coins) sdk.Coins }🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
14-14: Hard tabs
Column: 1(MD010, no-hard-tabs)
15-15: Hard tabs
Column: 1(MD010, no-hard-tabs)
x/multi_coin_rewards/spec/06_events.md (1)
18-25: Add descriptive documentation for event fields.The event fields lack proper documentation. Please add clear descriptions for each field:
message EventToggleMultiCoinRewards { - // address ... + // address represents the account that modified their multi-coin settings string address = 1; - // enabled ... + // enabled indicates whether multi-coin rewards are now enabled (true) or disabled (false) for this address bool enabled = 2; - // pending_rewards_claimed ... + // pending_rewards_claimed represents the amount of rewards that were claimed during this operation string pending_rewards_claimed = 3; }proto/kyve/multi_coin_rewards/v1beta1/events.proto (1)
21-31: Add descriptive documentation for event fields.For consistency with
EventUpdateParams, please add proper documentation forEventToggleMultiCoinRewardsand its fields:-// EventToggleMultiCoinRewards ... +// EventToggleMultiCoinRewards is an event emitted when a user modifies their multi-coin reward settings. +// emitted_by: MsgToggleMultiCoinRewards message EventToggleMultiCoinRewards { - // address ... + // address represents the account that modified their multi-coin settings string address = 1; - // enabled ... + // enabled indicates whether multi-coin rewards are now enabled (true) or disabled (false) for this address bool enabled = 2; - // pending_rewards_claimed ... + // pending_rewards_claimed represents the amount of rewards that were claimed during this operation string pending_rewards_claimed = 3; }x/multi_coin_rewards/spec/02_state.md (2)
12-14: Fix grammatical error in the user opt-in description.There's a subject-verb agreement issue in the sentence.
-The users who have opted in for multi-coin rewards are stored as a key in -the IAVL tree. There is no value associated with it. If the key exists, -the users has opted in. +The users who have opted in for multi-coin rewards are stored as keys in +the IAVL tree. There is no value associated with them. If a key exists, +the user has opted in.🧰 Tools
🪛 LanguageTool
[grammar] ~14-~14: It looks like you are using the wrong form of the noun or the verb. Did you mean “user has” or “users have”?
Context: ...ociated with it. If the key exists, the users has opted in. - MultiCoinRewardsEnabled: `...(NOUN_PLURAL_HAS)
27-45: Add examples to clarify the distribution policy structure.Consider adding examples to demonstrate how the weights are used and their impact on distribution:
Example: ```protobuf // For instance, if USDC rewards need to be distributed: { entries: [ { denom: "usdc", pool_weights: [ { pool_id: 1, weight: "0.7" }, // 70% to pool 1 { pool_id: 2, weight: "0.3" } // 30% to pool 2 ] } ] }</blockquote></details> <details> <summary>x/multi_coin_rewards/spec/01_concepts.md (1)</summary><blockquote> `7-14`: **Polish the introduction paragraph.** Several grammatical improvements can be made: ```diff -For policy reasons not everybody can or is allowed to receive, hold and -control other tokens. Therefore, users need to opt-in for multi-coin rewards. +For policy reasons, not everybody can or is allowed to receive, hold, and +control other tokens. Therefore, users need to opt in for multi-coin rewards. Because users could miss the opt-in, there is a grace-period in which multi-coin rewards can still be retrieved after a claim. If a user does not enable multi-coin rewards and does not enable it within the grace-period after a claim, the multi-coin rewards get re-distributed to the existing pools according to -a distribution policy which can be modified by an admin-address which is set +a distribution policy that can be modified by an admin address that is set by the governance.🧰 Tools
🪛 LanguageTool
[uncategorized] ~7-~7: A comma might be missing here.
Context: ...-- order: 1 --> # Concepts For policy reasons not everybody can or is allowed to rece...(AI_EN_LECTOR_MISSING_PUNCTUATION_COMMA)
[grammar] ~8-~8: The word “opt-in” is a noun. The verb is spelled with a space.
Context: ... other tokens. Therefore, users need to opt-in for multi-coin rewards. Because users c...(NOUN_VERB_CONFUSION)
x/multi_coin_rewards/spec/04_begin_block.md (1)
7-9: Add implementation details for the redistribution process.Consider adding more details about:
- How the redistribution is triggered (e.g., block height calculation)
- What happens if redistribution fails
- Performance considerations for large numbers of coins
Example addition:
The redistribution is triggered when `block_height % 50 == 0`. The process: 1. Retrieves all coins from the module account 2. For each coin: - Checks if it's covered by the policy - If covered, distributes according to weights - If not covered, leaves it in the account 3. Handles any failed distributions by logging errors🧰 Tools
🪛 LanguageTool
[uncategorized] ~7-~7: Possible missing comma found.
Context: ...-- order: 4 --> # BeginBlock Every 50 blocks all coins in the `multi_coin_rewards_di...(AI_HYDRA_LEO_MISSING_COMMA)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
x/multi_coin_rewards/types/events.pb.gois excluded by!**/*.pb.gox/multi_coin_rewards/types/params.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (24)
app/app.go(5 hunks)app/upgrades/v2_0/upgrade.go(4 hunks)proto/kyve/multi_coin_rewards/v1beta1/events.proto(1 hunks)proto/kyve/multi_coin_rewards/v1beta1/params.proto(1 hunks)x/multi_coin_rewards/client/cli/query.go(1 hunks)x/multi_coin_rewards/genesis.go(1 hunks)x/multi_coin_rewards/keeper/getters_queue.go(1 hunks)x/multi_coin_rewards/keeper/grpc_query.go(1 hunks)x/multi_coin_rewards/keeper/keeper.go(1 hunks)x/multi_coin_rewards/keeper/keeper_suite_test.go(1 hunks)x/multi_coin_rewards/keeper/logic_distribution.go(1 hunks)x/multi_coin_rewards/keeper/logic_distribution_test.go(1 hunks)x/multi_coin_rewards/keeper/msg_server_toggle.go(1 hunks)x/multi_coin_rewards/keeper/msg_server_update_params.go(1 hunks)x/multi_coin_rewards/spec/01_concepts.md(1 hunks)x/multi_coin_rewards/spec/02_state.md(1 hunks)x/multi_coin_rewards/spec/03_messages.md(1 hunks)x/multi_coin_rewards/spec/04_begin_block.md(1 hunks)x/multi_coin_rewards/spec/05_params.md(1 hunks)x/multi_coin_rewards/spec/06_events.md(1 hunks)x/multi_coin_rewards/spec/07_exported.md(1 hunks)x/multi_coin_rewards/types/keys.go(1 hunks)x/multi_coin_rewards/types/message_set_multi_coin_refund_policy.go(1 hunks)x/multi_coin_rewards/types/message_toggle_multi_coin_rewards.go(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- x/multi_coin_rewards/spec/05_params.md
🚧 Files skipped from review as they are similar to previous changes (6)
- x/multi_coin_rewards/client/cli/query.go
- x/multi_coin_rewards/keeper/msg_server_update_params.go
- x/multi_coin_rewards/keeper/logic_distribution_test.go
- x/multi_coin_rewards/keeper/getters_queue.go
- x/multi_coin_rewards/keeper/keeper.go
- x/multi_coin_rewards/keeper/msg_server_toggle.go
🧰 Additional context used
🧠 Learnings (1)
x/multi_coin_rewards/keeper/logic_distribution.go (1)
Learnt from: troykessler
PR: KYVENetwork/chain#244
File: x/multi_coin_rewards/keeper/logic_distribution.go:45-47
Timestamp: 2025-02-11T09:41:07.954Z
Learning: In the KYVE codebase, prefer returning errors over panicking to allow proper error handling at the BeginBlock level, especially for operations that could fail like module-to-module transfers.
🪛 LanguageTool
x/multi_coin_rewards/spec/03_messages.md
[uncategorized] ~10-~10: A comma might be missing here.
Context: ...oin-rewards. If they enable multi-coin rewards all current pending rewards will be cla...
(AI_EN_LECTOR_MISSING_PUNCTUATION_COMMA)
x/multi_coin_rewards/spec/02_state.md
[grammar] ~14-~14: It looks like you are using the wrong form of the noun or the verb. Did you mean “user has” or “users have”?
Context: ...ociated with it. If the key exists, the users has opted in. - MultiCoinRewardsEnabled: `...
(NOUN_PLURAL_HAS)
x/multi_coin_rewards/spec/01_concepts.md
[uncategorized] ~7-~7: A comma might be missing here.
Context: ...-- order: 1 --> # Concepts For policy reasons not everybody can or is allowed to rece...
(AI_EN_LECTOR_MISSING_PUNCTUATION_COMMA)
[grammar] ~8-~8: The word “opt-in” is a noun. The verb is spelled with a space.
Context: ... other tokens. Therefore, users need to opt-in for multi-coin rewards. Because users c...
(NOUN_VERB_CONFUSION)
[uncategorized] ~19-~19: A comma might be missing here.
Context: ...ing on a certain weight. To allow quick updates an admin address (other than the govern...
(AI_EN_LECTOR_MISSING_PUNCTUATION_COMMA)
[style] ~21-~21: Unless you want to emphasize “not”, use “cannot” which is more common.
Context: ...hich can update the policy. The address can not drain any rewards, but only modify to w...
(CAN_NOT_PREMIUM)
[duplication] ~28-~28: Possible typo: you repeated a word.
Context: ...All tokens are directly paid out to the user 2. User has not opted in: Only the native token...
(ENGLISH_WORD_REPEAT_RULE)
[uncategorized] ~31-~31: Use a comma before “and” if it connects two independent clauses (unless they are closely connected and short).
Context: ...ule account. A queue entry is created and a user has a certain amount of time to ...
(COMMA_COMPOUND_SENTENCE_2)
[duplication] ~33-~33: Possible typo: you repeated a word.
Context: ... pending rewards are transferred to the user 2. User does not enable rewards within time: Th...
(ENGLISH_WORD_REPEAT_RULE)
[uncategorized] ~37-~37: Possible missing comma found.
Context: ...distributionmodule account. Every 50 blocks all coins inmulti_coin_rewards_distri...
(AI_HYDRA_LEO_MISSING_COMMA)
[uncategorized] ~39-~39: A comma might be missing here.
Context: ...olicy. If tokens are not covered by the policy they remain inside the module account. ...
(AI_EN_LECTOR_MISSING_PUNCTUATION_COMMA)
x/multi_coin_rewards/spec/04_begin_block.md
[uncategorized] ~7-~7: Possible missing comma found.
Context: ...-- order: 4 --> # BeginBlock Every 50 blocks all coins in the `multi_coin_rewards_di...
(AI_HYDRA_LEO_MISSING_COMMA)
🪛 markdownlint-cli2 (0.17.2)
x/multi_coin_rewards/spec/07_exported.md
14-14: Hard tabs
Column: 1
(MD010, no-hard-tabs)
15-15: Hard tabs
Column: 1
(MD010, no-hard-tabs)
🪛 Buf (1.47.2)
proto/kyve/multi_coin_rewards/v1beta1/events.proto
5-5: import "gogoproto/gogo.proto": file does not exist
(COMPILE)
proto/kyve/multi_coin_rewards/v1beta1/params.proto
3-3: Files with package "kyve.multi_coin_rewards.v1beta1" must be within a directory "kyve/multi_coin_rewards/v1beta1" relative to root but were in directory "proto/kyve/multi_coin_rewards/v1beta1".
(PACKAGE_DIRECTORY_MATCH)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Summary
🔇 Additional comments (20)
proto/kyve/multi_coin_rewards/v1beta1/params.proto (1)
14-15: Complete the documentation formulti_coin_distribution_pending_time.The comment for this field is incomplete, making it unclear what this parameter represents. Please specify:
- The purpose of this pending time
- The time unit (seconds, blocks, etc.)
Example completion:
- // multi_coin_distribution_pending_time ... + // multi_coin_distribution_pending_time specifies the duration (in seconds) that must elapse + // before changes to coin distribution weights take effect. uint64 multi_coin_distribution_pending_time = 2;x/multi_coin_rewards/keeper/logic_distribution.go (4)
11-22: **Looks good! **The
addPendingRewardsfunction concisely handles pending rewards. Storing creation timestamps ensures predictable reward distribution timing.
25-48: **Overall logic appears correct. **The
ProcessPendingRewardsQueuefunction employs a queue-based approach that aligns well with the module’s design. It checks for eligible entries based onCreationDate, then collects rewards for redistribution. This design is consistent and clear.
45-47: **Return an error rather than panic. **This mirrors a past review comment on the same lines. In the KYVE codebase, prefer returning errors to allow higher-level handlers (e.g.,
BeginBlock) to manage them:-if err := k.bankKeeper.SendCoinsFromModuleToModule(ctx, types.ModuleName, types.MultiCoinRewardsRedistributionAccountName, collectedRewards); err != nil { - panic(err) +if err := k.bankKeeper.SendCoinsFromModuleToModule(ctx, types.ModuleName, types.MultiCoinRewardsRedistributionAccountName, collectedRewards); err != nil { + return // or wrap the error and handle it gracefully }
50-128: **LGTM! **The
DistributeNonClaimedRewardsflow carefully checks for invalid pools and redistributes remaining coins as per the distribution map. The approach of returning errors instead of panicking is well-aligned with Cosmos SDK best practices.x/multi_coin_rewards/keeper/keeper_suite_test.go (1)
17-20: **No issues with the test suite initialization. **
TestMultiCoinRewardsKeepercorrectly sets up Ginkgo’s test runner. The module name is referenced properly in the suite description.x/multi_coin_rewards/types/message_toggle_multi_coin_rewards.go (3)
15-18: **Serialization logic confirmed. **The
GetSignBytesmethod properly marshals and sorts JSON, supporting deterministic signatures.
20-27: **Return an error or empty slice instead of panicking. **Panicking on invalid addresses may disrupt the application flow. This was flagged in a previous review as well. Consider returning an empty slice or appropriate error:
func (msg *MsgToggleMultiCoinRewards) GetSigners() []sdk.AccAddress { creator, err := sdk.AccAddressFromBech32(msg.Creator) if err != nil { - panic(err) + // return an empty slice or handle the error gracefully } return []sdk.AccAddress{creator} }
37-43: **Validations look good. **
ValidateBasiccorrectly ensures the creator’s address is valid, returning a wrapped error on failure.app/upgrades/v2_0/upgrade.go (4)
7-10: LGTM!The new imports are correctly added for the multi-coin rewards module and auth types.
39-40: LGTM!The
CreateUpgradeHandlerfunction signature is correctly updated to include theaccountKeeperparameter, which is required for managing module accounts.
56-56: LGTM!The
EnsureMultiCoinDistributionAccountfunction is correctly called during the upgrade process to ensure the module account exists.
67-82: LGTM!The
EnsureMultiCoinDistributionAccountfunction implementation is well-structured and handles both cases:
- Creating a new module account if it doesn't exist
- Converting an existing account to a module account if it exists
The implementation follows best practices for module account management.
app/app.go (4)
84-85: LGTM!The multi-coin rewards module is correctly imported with side-effects and its keeper.
124-124: LGTM!The
Appstruct is correctly updated with:
- Changed
DistributionKeeperto pointer type- Added new
MultiCoinRewardsKeeperfieldAlso applies to: 156-156
302-302: LGTM!The
MultiCoinRewardsKeeperis correctly initialized in theNewfunction.
412-412: LGTM!The
AccountKeeperis correctly passed to the upgrade handler.x/multi_coin_rewards/spec/03_messages.md (1)
14-16: LGTM!The admin policy is clearly documented, specifying that only the governance address or a trusted entity can set the distribution policy.
x/multi_coin_rewards/spec/07_exported.md (1)
7-9: LGTM!The module exports are clearly documented with a concise introduction.
proto/kyve/multi_coin_rewards/v1beta1/events.proto (1)
5-5: Verify the gogoproto import path.The static analysis indicates that the gogoproto import file is not found. Please verify the import path and ensure the dependency is properly set up in the project.
❌ Verification inconclusive
🏁 Script executed:
#!/bin/bash # Check if gogoproto is properly set up fd -t f "gogo.proto"Length of output: 20
I'll proceed to re-run the file search using a different command to verify if the
gogoproto/gogo.protofile exists anywhere in the repository.
🏁 Script executed:
#!/bin/bash # Using find to search for any file named "gogo.proto" in the repository find . -type f -name "gogo.proto"Length of output: 33
Gogoproto Import Path Issue
- The search commands did not return any file matching “gogo.proto” in the repository.
- The import
"gogoproto/gogo.proto"inproto/kyve/multi_coin_rewards/v1beta1/events.protoappears invalid based on the current repository contents.- Please verify whether the dependency is supposed to be vendored into the repository or if the import path should be updated (for example, to a full path like
"github.com/gogo/protobuf/gogoproto/gogo.proto") based on your dependency management.🧰 Tools
🪛 Buf (1.47.2)
5-5: import "gogoproto/gogo.proto": file does not exist
(COMPILE)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
x/multi_coin_rewards/spec/01_concepts.md (4)
7-15: Improve grammar and readability.A few grammatical improvements would enhance readability:
- Line 8: Replace "opt-in" (noun) with "opt in" (verb) as it's being used as a verb
- Line 7: Add a comma after "For policy reasons"
Apply this diff:
-For policy reasons not everybody can or is allowed to receive, hold and -control other tokens. Therefore, users need to opt-in for multi-coin rewards. +For policy reasons, not everybody can or is allowed to receive, hold and +control other tokens. Therefore, users need to opt in for multi-coin rewards.🧰 Tools
🪛 LanguageTool
[uncategorized] ~7-~7: Possible missing comma found.
Context: ...-- order: 1 --> # Concepts For policy reasons not everybody can or is allowed to rece...(AI_HYDRA_LEO_MISSING_COMMA)
[grammar] ~8-~8: The word “opt-in” is a noun. The verb is spelled with a space.
Context: ... other tokens. Therefore, users need to opt-in for multi-coin rewards. Because users c...(NOUN_VERB_CONFUSION)
16-24: Improve grammar and emphasize security boundary.The section effectively explains the distribution policy. Consider:
- Replace "can not" with "cannot" for better grammar
- Consider emphasizing the security boundary of the admin role
Apply this diff:
-The address can not drain any rewards, but only modify to which +The address cannot drain any rewards, but only modify to whichAdditionally, consider adding a note about implementing proper access controls to ensure the admin address cannot exceed its intended permissions.
🧰 Tools
🪛 LanguageTool
[style] ~21-~21: Unless you want to emphasize “not”, use “cannot” which is more common.
Context: ...hich can update the policy. The address can not drain any rewards, but only modify to w...(CAN_NOT_PREMIUM)
25-35: Improve readability of the token flow section.Add missing punctuation to improve readability:
- Line 31: Add a comma before "and" in the compound sentence
Apply this diff:
- created and a user has a certain amount of time to enable multi-coin-rewards. + created, and a user has a certain amount of time to enable multi-coin-rewards.🧰 Tools
🪛 LanguageTool
[duplication] ~28-~28: Possible typo: you repeated a word.
Context: ...All tokens are directly paid out to the user 2. User has not opted in: Only the native token...(ENGLISH_WORD_REPEAT_RULE)
[uncategorized] ~31-~31: Use a comma before “and” if it connects two independent clauses (unless they are closely connected and short).
Context: ...ule account. A queue entry is created and a user has a certain amount of time to ...(COMMA_COMPOUND_SENTENCE_2)
[duplication] ~32-~32: Possible typo: you repeated a word.
Context: ... pending rewards are transferred to the user 2. User does not enable rewards within time: Th...(ENGLISH_WORD_REPEAT_RULE)
36-38: Add rationale for block interval and improve grammar.Consider:
- Add explanation for why 50 blocks was chosen as the interval
- Add missing comma in the conditional statement
Apply this diff:
-If tokens are not covered by the policy they remain inside the module account. +If tokens are not covered by the policy, they remain inside the module account.Please verify and document:
- Why was 50 blocks chosen as the interval?
- What's the expected time duration between redistributions based on the chain's block time?
- Are there any performance implications of this interval?
🧰 Tools
🪛 LanguageTool
[uncategorized] ~36-~36: Possible missing comma found.
Context: ...distributionmodule account. Every 50 blocks all coins inmulti_coin_rewards_distri...(AI_HYDRA_LEO_MISSING_COMMA)
[uncategorized] ~38-~38: A comma might be missing here.
Context: ...olicy. If tokens are not covered by the policy they remain inside the module account. ...(AI_EN_LECTOR_MISSING_PUNCTUATION_COMMA)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
x/multi_coin_rewards/spec/01_concepts.md(1 hunks)
🧰 Additional context used
🪛 LanguageTool
x/multi_coin_rewards/spec/01_concepts.md
[uncategorized] ~7-~7: Possible missing comma found.
Context: ...-- order: 1 --> # Concepts For policy reasons not everybody can or is allowed to rece...
(AI_HYDRA_LEO_MISSING_COMMA)
[grammar] ~8-~8: The word “opt-in” is a noun. The verb is spelled with a space.
Context: ... other tokens. Therefore, users need to opt-in for multi-coin rewards. Because users c...
(NOUN_VERB_CONFUSION)
[style] ~21-~21: Unless you want to emphasize “not”, use “cannot” which is more common.
Context: ...hich can update the policy. The address can not drain any rewards, but only modify to w...
(CAN_NOT_PREMIUM)
[duplication] ~28-~28: Possible typo: you repeated a word.
Context: ...All tokens are directly paid out to the user 2. User has not opted in: Only the native token...
(ENGLISH_WORD_REPEAT_RULE)
[uncategorized] ~31-~31: Use a comma before “and” if it connects two independent clauses (unless they are closely connected and short).
Context: ...ule account. A queue entry is created and a user has a certain amount of time to ...
(COMMA_COMPOUND_SENTENCE_2)
[duplication] ~32-~32: Possible typo: you repeated a word.
Context: ... pending rewards are transferred to the user 2. User does not enable rewards within time: Th...
(ENGLISH_WORD_REPEAT_RULE)
[uncategorized] ~36-~36: Possible missing comma found.
Context: ...distributionmodule account. Every 50 blocks all coins inmulti_coin_rewards_distri...
(AI_HYDRA_LEO_MISSING_COMMA)
[uncategorized] ~38-~38: A comma might be missing here.
Context: ...olicy. If tokens are not covered by the policy they remain inside the module account. ...
(AI_EN_LECTOR_MISSING_PUNCTUATION_COMMA)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: test / test
- GitHub Check: Summary
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
x/multi_coin_rewards/module.go (2)
150-169: Consider adding additional logging inBeginBlock.When
DistributeNonClaimedRewardsfails, the only feedback is an error log. It might be helpful to also log the current block height and any relevant context to facilitate diagnosis if repeated errors occur.
159-165: Evaluate potential impact of large pending queues.The distribution logic triggers every 50 blocks. For high-throughput networks with many pending entries, you may want to ensure that distributing non-claimed rewards doesn’t become a performance bottleneck. Consider batching or a more granular approach if the queue can grow large.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
go.sumis excluded by!**/*.sumgo.work.sumis excluded by!**/*.sum
📒 Files selected for processing (8)
go.mod(4 hunks)x/multi_coin_rewards/keeper/exported.go(1 hunks)x/multi_coin_rewards/keeper/logic_distribution.go(1 hunks)x/multi_coin_rewards/keeper/msg_server_distribution_policy_test.go(1 hunks)x/multi_coin_rewards/module.go(1 hunks)x/multi_coin_rewards/types/genesis.go(1 hunks)x/multi_coin_rewards/types/message_set_multi_coin_refund_policy.go(1 hunks)x/multi_coin_rewards/types/types.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- x/multi_coin_rewards/keeper/msg_server_distribution_policy_test.go
- x/multi_coin_rewards/types/genesis.go
- x/multi_coin_rewards/keeper/logic_distribution.go
- x/multi_coin_rewards/types/message_set_multi_coin_refund_policy.go
- go.mod
🧰 Additional context used
🧠 Learnings (1)
x/multi_coin_rewards/keeper/exported.go (1)
Learnt from: troykessler
PR: KYVENetwork/chain#244
File: x/multi_coin_rewards/keeper/exported.go:19-23
Timestamp: 2025-02-11T09:41:10.529Z
Learning: When handling errors in reward distribution, prefer returning all rewards as a fallback to prevent potential loss of funds, rather than restricting to native rewards only.
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: lint / golangci
- GitHub Check: test / test
- GitHub Check: Summary
🔇 Additional comments (7)
x/multi_coin_rewards/types/types.go (4)
9-15: LGTM! Well-structured type declarations.The types are well-defined with appropriate field types and visibility for their intended use.
17-19: Improve function name and documentation.As mentioned in a previous review, consider renaming this function to better reflect its responsibilities. Additionally, the documentation could be more detailed.
Apply this diff to improve the documentation:
-// ParseAndNormalizeMultiCoinDistributionMap turns the list structured policy map into a go map and -// normalizes the pool weights. Due to go maps indeterminism one can not store go-maps directly in state. +// NormalizeMultiCoinDistributionPolicy validates and normalizes the distribution policy by: +// 1. Converting the list-structured policy into a map for efficient lookups +// 2. Validating pool weights (positive values, no duplicates) +// 3. Normalizing weights for each denomination +// Note: The input policy uses a list structure since Go maps are non-deterministic +// and cannot be stored directly in blockchain state. +func NormalizeMultiCoinDistributionPolicy(policy MultiCoinDistributionPolicy) (MultiCoinDistributionMap, error) {
22-43: LGTM! Thorough validation and normalization logic.The code effectively:
- Validates positive weights
- Prevents duplicate pool IDs
- Correctly normalizes weights using the total
45-50: LGTM! Proper duplicate check and map population.The code correctly prevents duplicate denominations and provides clear error messages.
x/multi_coin_rewards/module.go (1)
210-214: Verify customized authority usage.The code defaults to the governance authority unless a custom authority is provided. Ensure that external interactions (e.g., CLI commands or other modules) consistently handle cases where a custom authority is set, to avoid unintentional errors.
x/multi_coin_rewards/keeper/exported.go (2)
20-23: Return all coins as fallback to avoid potential loss of rewards.When an error occurs in
k.MultiCoinRewardsEnabled.Has, you currently discard the distribution by returningnil, err. Per your earlier learnings, consider logging the error and returning all coins as a fallback to prevent potential loss of funds.
31-43: Confirm thatglobalTypes.Denomalways matches the intended native token.You subtract only
globalTypes.Denomfrom the total coins to produceenabledRewards. In scenarios where the native denom changes or multiple native denominations exist, this logic might not behave as expected. Verify that this is aligned with your chain’s design.
Summary by CodeRabbit
• New Features
– Integrated a comprehensive multi-coin rewards system that allows users to opt-in for rewards, manage distribution policies, and query reward parameters through the CLI.
– Introduced functionality for toggling multi-coin rewards and setting distribution policies, with detailed event logging.
– Enhanced genesis state support for initializing and exporting rewards configuration, including new parameters for distribution policies.
– Added new commands for querying and managing multi-coin rewards through the CLI, including commands for toggling rewards and setting distribution policies.
– Implemented a structured approach for handling pending rewards and redistributing unclaimed rewards based on defined policies.
– Added a new module for managing multi-coin rewards, including support for protobuf messages and improved error handling.
• Chores
– Updated dependencies and refined configuration settings, improving overall system stability and distribution processing.
• Tests
– Added extensive tests to validate reward distribution logic, toggling functionality, and parameter updates.