fix: improve reward genesis validation and reward pool validation

[Thaleszh]

Description

• Use DecCoins.Validate() on RewardPool.ValidateGenesis to catch malformed denom formats, duplicate denoms, bad ordering
• Enforce denom consistency in GenesisState.Validate with Params.TokenDenom

Type of change

• Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

Added regression tests

PR Checklist:

Make sure each step was done:

• Updated changelog with PR's intent
• Lint with make lint-fix

[coderabbitai]

Walkthrough

The PR adds denom consistency validation to the rewards module's genesis handling. It updates RewardPool.ValidateGenesis() to validate CommunityPool using DecCoins.Validate(), and adds denom consistency checks in GenesisState.Validate() to ensure CommunityPool and ReleaseSchedule.TotalAmount denoms match Params.TokenDenom. Test coverage is expanded with a table-driven test suite for RewardPool validation and three new negative test cases validating various denom mismatch scenarios.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately describes the main changes: improving reward genesis validation and reward pool validation through denom consistency checks and DecCoins validation.
Description check	✅ Passed	The description is directly related to the changeset, clearly explaining the two main improvements and confirming that tests were added and changelog updated.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/reward-genesis-validation

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (2)

x/rewards/types/genesis_test.go (1)

84-91: Isolate this case to only the field under test.

For the “mismatched total amount denom” case, mutate only TotalAmount.Denom and keep ReleasedAmount aligned with TokenDenom. This makes the test prove the intended validation path unambiguously.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@x/rewards/types/genesis_test.go` around lines 84 - 91, Test mutates too many
fields; narrow the "mismatched total amount denom" case to only change
TotalAmount.Denom. In the modifyFn for the ReleaseSchedule test case update only
TotalAmount to use "notkii" while keeping ReleasedAmount (and any TokenDenom
references) using the expected token denom (e.g., sdk.NewCoin(TokenDenom,
math.NewInt(0))) and leave EndTime and Active as-is so the validation failure is
attributable solely to the mismatched TotalAmount denom.

x/rewards/types/reward_pool_test.go (1)

75-83: Strengthen negative-case assertions with error content checks.

Right now, any error satisfies failing cases. Consider adding per-case errContains and using require.ErrorContains so regressions fail for the right reason.

Proposed test hardening

 testCases := []struct {
 	name      string
 	pool      types.RewardPool
 	expectErr bool
+	errContains string
 }{
 	{
 		name:      "negative amount",
 		pool:      ...,
 		expectErr: true,
+		errContains: "negative",
 	},
 	{
 		name:      "duplicate denoms",
 		pool:      ...,
 		expectErr: true,
+		errContains: "duplicate",
 	},
 }
 ...
 if tc.expectErr {
 	require.Error(t, err)
+	if tc.errContains != "" {
+		require.ErrorContains(t, err, tc.errContains)
+	}
 } else {
 	require.NoError(t, err)
 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@x/rewards/types/reward_pool_test.go` around lines 75 - 83, The test's
negative cases accept any error; augment the test table entries in testCases
with an errContains string for expected error substrings and, inside the t.Run
loop where ValidateGenesis() is called, replace require.Error(...) with
require.ErrorContains(t, err, tc.errContains) when tc.expectErr is true (keep
require.NoError for the non-error branch). Ensure testCases entries that expect
failures populate errContains with the specific message fragment to assert the
correct failure reason for ValidateGenesis.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@x/rewards/types/genesis.go`:
- Around line 39-51: GenesisState denom consistency checks in Validate() can be
bypassed because AppModule.InitGenesis calls keeper.InitGenesis without
validating; update the module initialization to enforce validation by calling
GenesisState.Validate() from AppModule.InitGenesis before invoking
keeper.InitGenesis (or alternatively move the denom checks into
keeper.InitGenesis if you prefer validation inside the keeper). Locate
AppModule.InitGenesis and add a validation step that invokes
GenesisState.Validate() (handling/returning the error) for the incoming gs,
referencing GenesisState.Validate(), AppModule.InitGenesis, and
keeper.InitGenesis to ensure invalid genesis is rejected before state is
applied.

---

Nitpick comments:
In `@x/rewards/types/genesis_test.go`:
- Around line 84-91: Test mutates too many fields; narrow the "mismatched total
amount denom" case to only change TotalAmount.Denom. In the modifyFn for the
ReleaseSchedule test case update only TotalAmount to use "notkii" while keeping
ReleasedAmount (and any TokenDenom references) using the expected token denom
(e.g., sdk.NewCoin(TokenDenom, math.NewInt(0))) and leave EndTime and Active
as-is so the validation failure is attributable solely to the mismatched
TotalAmount denom.

In `@x/rewards/types/reward_pool_test.go`:
- Around line 75-83: The test's negative cases accept any error; augment the
test table entries in testCases with an errContains string for expected error
substrings and, inside the t.Run loop where ValidateGenesis() is called, replace
require.Error(...) with require.ErrorContains(t, err, tc.errContains) when
tc.expectErr is true (keep require.NoError for the non-error branch). Ensure
testCases entries that expect failures populate errContains with the specific
message fragment to assert the correct failure reason for ValidateGenesis.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 00089e97-ec0a-40e0-8424-bac668ed29da

📥 Commits

Reviewing files that changed from the base of the PR and between 16d7ad8 and 533fcd0.

📒 Files selected for processing (5)

• CHANGELOG.md
• x/rewards/types/genesis.go
• x/rewards/types/genesis_test.go
• x/rewards/types/reward_pool.go
• x/rewards/types/reward_pool_test.go

[codecov]

Codecov Report

❌ Patch coverage is 84.61538% with 2 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
x/rewards/types/genesis.go	81.81%	1 Missing and 1 partial ⚠️

📢 Thoughts on this report? Let us know!