fix(keeper/tests): dedupe FundCommunityPool expectation and remove redundant balance tracking #25314
Conversation
📝 WalkthroughWalkthroughTest scaffolding for the gov keeper was simplified by removing the FundCommunityPool gomock expectation and eliminating distribution keeper usage in deposit-related tests. The setup function no longer asserts FundCommunityPool calls, and deposit tests stop tracking mock balances via the distribution keeper. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
x/gov/keeper/deposit_test.go (1)
445-447: Bug: wrong ratio used for cancellation charge expectationThe test computes cancellationCharges with MinInitialDepositRatio instead of ProposalCancelRatio, which will silently zero out expected charges unless MinInitialDepositRatio is set. Fix to align with the exercised parameter.
- burnAmount := sdkmath.LegacyNewDecFromInt(deposit.Amount).Mul(sdkmath.LegacyMustNewDecFromStr(params.MinInitialDepositRatio)).TruncateInt() + burnAmount := sdkmath.LegacyNewDecFromInt(deposit.Amount).Mul(sdkmath.LegacyMustNewDecFromStr(params.ProposalCancelRatio)).TruncateInt()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Knowledge Base: Disabled due to Reviews > Disable Knowledge Base setting
📒 Files selected for processing (2)
x/gov/keeper/common_test.go(0 hunks)x/gov/keeper/deposit_test.go(2 hunks)
💤 Files with no reviewable changes (1)
- x/gov/keeper/common_test.go
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Summary
🔇 Additional comments (2)
x/gov/keeper/deposit_test.go (2)
209-209: LGTM: removed dist keeper binding in TestDepositAmountSame rationale as above; the test doesn’t observe community-pool effects, so dropping dist keeper plumbing is fine.
376-388: Switch to classic for-loop or confirm Go ≥1.22
for i := range 3requires Go 1.22+ (and changes closure-capture); if your CI isn’t on 1.22+, revert to:-for i := range 3 { +for i := 0; i < 3; i++ { + i := i // capture for the closure on older Go versionsEnsure your go.mod’s
godirective is ≥ 1.22 (and update CI) or use the above loop.
| @@ -38,8 +38,7 @@ func TestDeposits(t *testing.T) { | |||
|
|
|||
| for _, tc := range testcases { | |||
| t.Run(tc.name, func(t *testing.T) { | |||
| govKeeper, authKeeper, bankKeeper, stakingKeeper, distKeeper, _, ctx := setupGovKeeper(t) | |||
| trackMockBalances(bankKeeper, distKeeper) | |||
| govKeeper, authKeeper, bankKeeper, stakingKeeper, _, _, ctx := setupGovKeeper(t) | |||
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
LGTM: removed dist keeper binding in TestDeposits
Ignoring the 5th/6th returns from setupGovKeeper aligns with deduping FundCommunityPool expectations and removing redundant balance tracking.
Run to ensure no leftover redundant tracking calls remain:
🏁 Script executed:
#!/bin/bash
# Find redundant balance tracking helpers still present in gov tests
rg -nP 'trackMockBalances\s*\(' x/gov -g '**/*_test.go'Length of output: 292
Remove leftover redundant balance-tracking helper calls
- x/gov/keeper/common_test.go: delete the
trackMockBalances(bankKeeper, distributionKeeper)invocation at line 90 and thetrackMockBalanceshelper definition at line 122.
🤖 Prompt for AI Agents
In x/gov/keeper/deposit_test.go around line 41, and specifically in
x/gov/keeper/common_test.go at lines 90 and 122, there are leftover redundant
balance-tracking helper usages: remove the call to trackMockBalances(bankKeeper,
distributionKeeper) at line 90 and delete the entire trackMockBalances helper
definition at line 122; after removal, run tests and remove any now-unused
imports related to that helper (or references) to keep the file clean.
Description
Removed duplicate distributionKeeper.EXPECT().FundCommunityPool(...).Return(nil).AnyTimes() in setupGovKeeper, keeping only the balance-aware DoAndReturn from trackMockBalances.
Removed redundant trackMockBalances calls in tests that already use setupGovKeeper, and adjusted unused return bindings.
Rationale: duplicated gomock expectations for the same method increase brittleness and ambiguity—order-dependent matching can mask mistakes and confuse future maintainers. Retaining a single expectation with explicit balance mutations matches test intentions and reduces risk. Cleaning redundant calls avoids unnecessary state duplication and keeps tests clearer.
Summary by CodeRabbit