feat(x/distribution): panic on set community pool #23507
Conversation
DeVikingMark
requested review from
alpe,
JulianToledano and
a team
as code owners
📝 WalkthroughWalkthroughThe changes focus on the Changes
Sequence DiagramsequenceDiagram
participant Genesis as Genesis Initialization
participant FeePool as FeePool Validator
participant CommunityPool as Community Pool
Genesis->>FeePool: Validate Genesis
FeePool->>CommunityPool: Check Pool Values
alt CommunityPool Not Zero
FeePool->>FeePool: Panic Immediately
else Valid Pool
FeePool-->>Genesis: Validation Successful
end
The sequence diagram illustrates the new validation flow, highlighting the immediate panic mechanism when the Tip 🌐 Web search-backed reviews and chat
✨ 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
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
x/distribution/types/fee_pool.go (2)
23-24: Document the panic behavior.Consider adding a doc comment to
ValidateGenesisto explicitly document that it panics on negative CommunityPool values.// ValidateGenesis validates the fee pool for a genesis state +// Panics if CommunityPool contains negative values. func (f FeePool) ValidateGenesis() error {
23-24: Consider consistent error handling approach.The method handles similar conditions differently:
- DecimalPool: Returns error for negative values
- CommunityPool: Panics for negative values
This asymmetric approach might be confusing. Consider either:
- Making both checks use panic for consistency, or
- Document the rationale for the different approaches
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
x/distribution/types/fee_pool.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
x/distribution/types/fee_pool.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Summary
🔇 Additional comments (1)
x/distribution/types/fee_pool.go (1)
23-24: Approved: Breaking change from error to panic.The change aligns with the PR objectives to enforce the invariant that CommunityPool cannot be negative. This is a breaking change for v0.53.
Let's verify the impact of this breaking change:
✅ Verification successful
Breaking change confirmed: Panic on negative community pool during genesis
The change from error to panic affects chain initialization, as ValidateGenesis is called during genesis state validation. Node operators should ensure their genesis state has no negative community pool values before upgrading to v0.53.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for direct callers of ValidateGenesis that might need updates ast-grep --pattern 'ValidateGenesis()'Length of output: 40
Script:
#!/bin/bash # Find the complete type definition and method signature ast-grep --pattern 'type FeePool struct' # Search for any imports of this package rg "\".*distribution/types\"" -A 1 -B 1 # Search for ValidateGenesis calls with context rg "ValidateGenesis" -A 2 -B 2Length of output: 53247
x/distribution/types/fee_pool.go
Outdated
| @@ -20,8 +20,8 @@ func (f FeePool) ValidateGenesis() error { | |||
| return fmt.Errorf("negative DecimalPool in distribution fee pool, is %v", f.DecimalPool) | |||
| } | |||
|
|
|||
| if f.CommunityPool.IsAnyNegative() { // TODO(@julienrbrt) in v0.53, panic if the community pool is set | |||
| return fmt.Errorf("negative CommunityPool in distribution fee pool, is %v", f.CommunityPool) | |||
| if f.CommunityPool.IsAnyNegative() { | |||
There was a problem hiding this comment.
The comment means to panic if it is set at all, as it should be specified in protocolpool.
There was a problem hiding this comment.
The comment means to panic if it is set at all, as it should be specified in protocolpool.
@julienrbrt Made an update and it seems that code works as expected—if CommunityPool has any non-zero value, it triggers a panic because those values should now be set in the protocolpool module.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
x/distribution/types/fee_pool.go (1)
23-24: LGTM! Note: This is a breaking change.The change from error to panic is appropriate for genesis validation and aligns with the invariant that CommunityPool must be zero. The error message clearly explains that this value should be specified in protocolpool instead.
Consider adding a comment to explicitly document this as a breaking change:
// ValidateGenesis validates the fee pool for a genesis state +// Breaking change in v0.53: Now panics if CommunityPool is not zero instead of returning an error func (f FeePool) ValidateGenesis() error {
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
x/distribution/types/fee_pool.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
x/distribution/types/fee_pool.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Summary
julienrbrt
changed the title
|
Could you update the relevant tests? https://github.com/cosmos/cosmos-sdk/actions/runs/13118313027/job/36636497444?pr=23507 |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
x/distribution/types/fee_pool_test.go (2)
15-48: Add more test cases for comprehensive coverage.While the current test cases cover the basic scenarios, consider adding the following cases to ensure thorough testing:
- Negative community pool values
- Multiple coins in community pool
- Boundary conditions (zero vs near-zero values)
- Nil DecCoins scenarios
Here's a suggested addition to the test cases:
testCases := []struct { name string feePool types.FeePool shouldPanic bool }{ + { + name: "negative community pool", + feePool: types.FeePool{ + DecimalPool: sdk.DecCoins{}, + CommunityPool: sdk.DecCoins{ + sdk.DecCoin{Denom: "stake", Amount: sdk.NewDec(-1)}, + }, + }, + shouldPanic: true, + }, + { + name: "multiple coins in community pool", + feePool: types.FeePool{ + DecimalPool: sdk.DecCoins{}, + CommunityPool: sdk.DecCoins{ + sdk.DecCoin{Denom: "stake", Amount: sdk.NewDec(1)}, + sdk.DecCoin{Denom: "atom", Amount: sdk.NewDec(2)}, + }, + }, + shouldPanic: true, + }, + { + name: "nil decimal pool", + feePool: types.FeePool{ + CommunityPool: sdk.DecCoins{}, + }, + shouldPanic: false, + },
50-65: Enhance test assertions with more descriptive messages.The test execution logic is solid, but the assertions could be more descriptive to aid in debugging test failures.
Consider this enhancement:
for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { if tc.shouldPanic { require.Panics(t, func() { tc.feePool.ValidateGenesis() - }, "expected ValidateGenesis to panic with non-zero community pool") + }, "ValidateGenesis should panic for test case: %s", tc.name) } else { err := tc.feePool.ValidateGenesis() if tc.feePool.DecimalPool.IsAnyNegative() { - require.Error(t, err) + require.Error(t, err, "ValidateGenesis should return error for negative DecimalPool in test case: %s", tc.name) } else { - require.NoError(t, err) + require.NoError(t, err, "ValidateGenesis should not return error for valid FeePool in test case: %s", tc.name) } } }) }
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
x/distribution/types/fee_pool_test.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
x/distribution/types/fee_pool_test.go (2)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern **/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Summary
🔇 Additional comments (1)
x/distribution/types/fee_pool_test.go (1)
14-66: Test coverage aligns with PR objectives but could be strengthened.The tests cover the core change of panicking on non-zero CommunityPool values, which aligns with the PR objectives. However, to ensure robust testing of this breaking change for version 0.53, consider:
- Adding more edge cases as suggested above
- Including documentation about the breaking change in test comments
- Adding test cases that verify the panic message content
To ensure comprehensive test coverage, run:
I've updated tests, does it seem fine? |
Previously, the ValidateGenesis function would return an error when encountering
a negative CommunityPool value. This change makes it panic instead, as specified
in the TODO comment. This is a breaking change that will be introduced in v0.53.
This change helps maintain the invariant that the community pool should never
be negative, making it easier to catch and debug issues earlier in the
development process.
Summary by CodeRabbit