fix(x/gov,x/distribution): Balance assertions on genesis import shouldn't be exact #22832
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThe pull request updates the changelogs for the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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
|
fastfadingviolets
force-pushed
the
fix/export-import-gaia
branch
from
ca2ff56 to
5a4d71b
Compare
fastfadingviolets
requested review from
julienrbrt,
sontrinh16,
a team,
alpe and
JulianToledano
as code owners
x/gov/genesis.go
Outdated
| return fmt.Errorf("expected module account was %s but we got %s", balance.String(), totalDeposits.String()) | ||
| // check if the module account can cover the total deposits | ||
| if !balance.IsAllGTE(totalDeposits) { | ||
| panic(fmt.Sprintf("expected module to hold at least %s, but it holds %s", totalDeposits, balance)) |
There was a problem hiding this comment.
can you still return an error here please
There was a problem hiding this comment.
oh, of course, my bad
x/distribution/keeper/genesis.go
Outdated
| if !balances.Equal(moduleHoldingsInt) { | ||
| return fmt.Errorf("distribution module balance does not match the module holdings: %s <-> %s", balances, moduleHoldingsInt) | ||
| if balances.IsAllLT(moduleHoldingsInt) { | ||
| panic(fmt.Sprintf("distribution module balance is less than module holdings: %s < %s", balances, moduleHoldingsInt)) |
julienrbrt
changed the title
![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
🧹 Outside diff range and nitpick comments (2)
x/distribution/CHANGELOG.md (1)
Line range hint
1-1: Use standard Markdown strikethrough syntax.For reverted changes, consider using the standard Markdown strikethrough syntax
~~text~~instead of~text~. This ensures better compatibility across different Markdown viewers.Example:
-~The distribution module keeper now takes a new argument `PoolKeeper` in addition.~ Reverted on #20790 +~~The distribution module keeper now takes a new argument `PoolKeeper` in addition.~~ Reverted on #20790x/distribution/keeper/genesis.go (1)
126-127: LGTM: Consistent balance check implementationThe implementation aligns well with the changes in the gov module. However, the error message could be more descriptive.
Consider enhancing the error message:
- panic(fmt.Sprintf("distribution module balance is less than module holdings: %s < %s", balances, moduleHoldingsInt)) + panic(fmt.Sprintf("distribution module balance insufficient - required: %s, actual: %s", moduleHoldingsInt, balances))
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (4)
x/distribution/CHANGELOG.md(1 hunks)x/distribution/keeper/genesis.go(1 hunks)x/gov/CHANGELOG.md(1 hunks)x/gov/genesis.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
x/gov/genesis.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
x/distribution/keeper/genesis.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
x/distribution/CHANGELOG.md (1)
Pattern **/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"
x/gov/CHANGELOG.md (1)
Pattern **/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"
🔇 Additional comments (3)
x/distribution/CHANGELOG.md (1)
34-34: LGTM! The changelog entry is well-formatted and clear.
The entry follows the changelog format guidelines and accurately describes the change in balance assertions for genesis import.
x/gov/genesis.go (1)
82-84: LGTM: Balance check modification improves robustness
The change from exact balance matching to minimum balance requirement is a good improvement that maintains security while being more flexible.
Let's verify the consistency of this change with other modules:
✅ Verification successful
Balance check modification is consistent with other modules
The verification confirms that the balance check modification in x/gov/genesis.go is consistent with similar checks across the codebase:
- In
x/gov/keeper/invariants.go, the sameIsAllGTEcheck is used for deposits validation - In
x/distributionmodule, analogous checks usingIsAllLTare implemented for module balances
The change maintains consistency with the existing patterns in the codebase while improving the robustness of the balance validation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for similar balance checks in other modules
rg -A 2 "balance.*IsAllGTE|balance.*IsAllLT" --type go
Length of output: 1027
x/gov/CHANGELOG.md (1)
41-41: LGTM: Clear and well-placed changelog entry
The changelog entry accurately describes the change and is correctly placed in the Improvements section.
Certain modules (e.g. ccvprovider) assign truncated fractions as rewards, and transfer the full integer amount to the distribution module. So a long-running chain will almost always fail the assertion if the module holdings are expected to equal the balance
People will deposit tokens into gov for, for instance, ICAs, so we really can't expect it to exactly equal the deposits.
fastfadingviolets
force-pushed
the
fix/export-import-gaia
branch
from
5a4d71b to
b523c25
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
x/distribution/keeper/genesis.go (1)
126-127: LGTM! Balance check logic is improved.The change correctly implements the requirement to ensure sufficient balance rather than exact balance. Consider making the error message more specific:
- return fmt.Errorf("distribution module balance is less than module holdings: %s < %s", balances, moduleHoldingsInt) + return fmt.Errorf("%s module balance is less than module holdings: %s < %s", types.ModuleName, balances, moduleHoldingsInt)
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (4)
x/distribution/CHANGELOG.md(1 hunks)x/distribution/keeper/genesis.go(1 hunks)x/gov/CHANGELOG.md(1 hunks)x/gov/genesis.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- x/gov/genesis.go
- x/distribution/CHANGELOG.md
🧰 Additional context used
📓 Path-based instructions (2)
x/distribution/keeper/genesis.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
x/gov/CHANGELOG.md (1)
Pattern **/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"
🔇 Additional comments (2)
x/gov/CHANGELOG.md (1)
41-41: LGTM! Changelog entry is clear and well-placed.
The entry accurately reflects the changes being made to the governance module's balance assertions at genesis import.
x/distribution/keeper/genesis.go (1)
126-127: Add tests for the balance verification conditions.
Please add tests to verify both the success and failure cases of the balance verification logic.
|
i believe the code is correct, if you import a genesis file and the value is off then the genesis file could be providing in correct data. Or how do you see this being wrong in practice. I dont believe i have heard of a team running into this issue |
it's not currently possible to import a genesis export from the cosmos hub because of these issues :) I believe there are two reasons this is happening
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (3)
tests/integration/distribution/genesis_test.go (2)
3-25: Improve import organization for better readabilityThe imports should be grouped in the following order:
- Standard library
- External dependencies
- Internal dependencies
import ( "encoding/json" "testing" abci "github.com/cometbft/cometbft/api/cometbft/abci/v1" "github.com/stretchr/testify/suite" corestore "cosmossdk.io/core/store" coretesting "cosmossdk.io/core/testing" "cosmossdk.io/depinject" "cosmossdk.io/log" sdkmath "cosmossdk.io/math" "cosmossdk.io/x/distribution/keeper" "cosmossdk.io/x/distribution/types" bankkeeper "cosmossdk.io/x/bank/keeper" stakingkeeper "cosmossdk.io/x/staking/keeper" "github.com/cosmos/cosmos-sdk/codec" "github.com/cosmos/cosmos-sdk/runtime" simtestutil "github.com/cosmos/cosmos-sdk/testutil/sims" sdk "github.com/cosmos/cosmos-sdk/types" _ "github.com/cosmos/cosmos-sdk/x/auth" authkeeper "github.com/cosmos/cosmos-sdk/x/auth/keeper" )🧰 Tools
🪛 golangci-lint (1.62.2)
6-6: File is not
gci-ed with --skip-generated -s standard -s default -s prefix(cosmossdk.io) -s prefix(github.com/cosmos/cosmos-sdk) --custom-order(gci)
11-11: File is not
gci-ed with --skip-generated -s standard -s default -s prefix(cosmossdk.io) -s prefix(github.com/cosmos/cosmos-sdk) --custom-order(gci)
15-15: File is not
gci-ed with --skip-generated -s standard -s default -s prefix(cosmossdk.io) -s prefix(github.com/cosmos/cosmos-sdk) --custom-order(gci)
20-20: File is not
gci-ed with --skip-generated -s standard -s default -s prefix(cosmossdk.io) -s prefix(github.com/cosmos/cosmos-sdk) --custom-order(gci)
175-188: Add documentation for the clearDB helperConsider adding a comment explaining the purpose of this helper function and why it's necessary between tests.
+// clearDB removes all entries from the test database to ensure a clean state between tests. func (s *ImportExportSuite) clearDB(db corestore.KVStoreWithBatch) {tests/integration/gov/genesis_test.go (1)
241-327: LGTM with suggestions for improvementThe new test case effectively validates the insufficient balance scenario. However, consider these improvements:
- The error assertion could be more specific to ensure the exact error condition is being tested
- Consider adding a test case for the boundary condition where the balance exactly equals the required amount
For the error assertion, consider:
-require.ErrorContains(t, err, "expected gov module to hold at least") +expectedErr := fmt.Sprintf("expected gov module to hold at least %s, but only has %s", + params.MinDeposit.String(), + sdk.Coins(params.MinDeposit).QuoInt(sdkmath.NewInt(2)).String()) +require.ErrorContains(t, err, expectedErr)
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (3)
tests/integration/distribution/genesis_test.go(1 hunks)tests/integration/gov/genesis_test.go(3 hunks)x/gov/genesis.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- x/gov/genesis.go
🧰 Additional context used
📓 Path-based instructions (2)
tests/integration/gov/genesis_test.go (3)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern tests/**/*: "Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull request"
Pattern **/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"
tests/integration/distribution/genesis_test.go (3)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern tests/**/*: "Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull request"
Pattern **/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"
🪛 golangci-lint (1.62.2)
tests/integration/distribution/genesis_test.go
6-6: File is not gci-ed with --skip-generated -s standard -s default -s prefix(cosmossdk.io) -s prefix(github.com/cosmos/cosmos-sdk) --custom-order
(gci)
11-11: File is not gci-ed with --skip-generated -s standard -s default -s prefix(cosmossdk.io) -s prefix(github.com/cosmos/cosmos-sdk) --custom-order
(gci)
15-15: File is not gci-ed with --skip-generated -s standard -s default -s prefix(cosmossdk.io) -s prefix(github.com/cosmos/cosmos-sdk) --custom-order
(gci)
20-20: File is not gci-ed with --skip-generated -s standard -s default -s prefix(cosmossdk.io) -s prefix(github.com/cosmos/cosmos-sdk) --custom-order
(gci)
104-104: ineffectual assignment to err
(ineffassign)
153-153: ineffectual assignment to err
(ineffassign)
🔇 Additional comments (4)
tests/integration/distribution/genesis_test.go (3)
27-65: LGTM! Well-structured test suite setup
The test suite setup is comprehensive and follows good practices:
- Proper initialization of all required keepers
- Clear dependency injection setup
- Appropriate test address generation
67-124: LGTM! Comprehensive happy path test
The test effectively validates the new behavior where the distribution module can have more tokens than outstanding rewards.
🧰 Tools
🪛 golangci-lint (1.62.2)
104-104: ineffectual assignment to err
(ineffassign)
126-173: LGTM! Good negative test case
The test properly validates that genesis import fails when the module balance is insufficient to cover outstanding rewards.
🧰 Tools
🪛 golangci-lint (1.62.2)
153-153: ineffectual assignment to err
(ineffassign)
tests/integration/gov/genesis_test.go (1)
106-108: LGTM: Token transfer addition aligns with PR objectives
The addition of token transfer correctly simulates a scenario where the governance module has more tokens than required by deposits, which aligns with the PR's goal of relaxing exact balance requirements.
| app2, err := simtestutil.SetupWithConfiguration( | ||
| depinject.Configs( | ||
| AppConfig, | ||
| depinject.Supply(log.NewNopLogger()), | ||
| ), | ||
| conf2, | ||
| ) | ||
|
|
There was a problem hiding this comment.
Fix ineffectual error assignments in app setup
The error from SetupWithConfiguration is assigned but never checked.
app2, err := simtestutil.SetupWithConfiguration(
depinject.Configs(
AppConfig,
depinject.Supply(log.NewNopLogger()),
),
conf2,
)
+s.Require().NoError(err)Also applies to: 153-160
🧰 Tools
🪛 golangci-lint (1.62.2)
104-104: ineffectual assignment to err
(ineffassign)
| @@ -175,7 +179,7 @@ func TestImportExportQueues(t *testing.T) { | |||
| assert.Assert(t, proposal2.Status == v1.StatusVotingPeriod) | |||
|
|
|||
| macc := s2.GovKeeper.GetGovernanceAccount(ctx2) | |||
| assert.DeepEqual(t, sdk.Coins(params.MinDeposit), s2.BankKeeper.GetAllBalances(ctx2, macc.GetAddress())) | |||
| assert.DeepEqual(t, sdk.Coins(params.MinDeposit).MulInt(sdkmath.NewInt(2)), s2.BankKeeper.GetAllBalances(ctx2, macc.GetAddress())) | |||
There was a problem hiding this comment.
Consider relaxing the exact balance assertion
The current assertion checks for an exact balance (2x minimum deposit), which appears to contradict the PR's objective of relaxing exact balance requirements. Consider modifying this to assert that the balance is greater than or equal to the required amount instead.
-assert.DeepEqual(t, sdk.Coins(params.MinDeposit).MulInt(sdkmath.NewInt(2)), s2.BankKeeper.GetAllBalances(ctx2, macc.GetAddress()))
+assert.Assert(t, s2.BankKeeper.GetAllBalances(ctx2, macc.GetAddress()).IsAllGTE(sdk.Coins(params.MinDeposit)))Committable suggestion skipped: line range outside the PR's diff.
fastfadingviolets
force-pushed
the
fix/export-import-gaia
branch
from
61a28aa to
5534e4f
Compare
oof this is a bug in ica and ccvprovider. we can change it but teams have to stop doing stuff like this as its hacking the software and could introduce issues for other users. this is tricky, what is the reason you are exporting into a genesis file. |
i'm part of the testing team for the hub; when there's a new version of gaia, we fork the cosmos hub from a genesis export and run the new version on the fork to check that the new binary works on mainnet. fwiw i agree there's an issue in ccvprovider, but i do think the ica use case is reasonable as a way of having governance-gated holdings in a foreign token |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
tests/integration/distribution/genesis_test.go (1)
67-125: LGTM! Comprehensive happy path test.The test effectively verifies the new behavior allowing module balance to exceed total rewards. Consider adding test cases for edge cases where the balance is exactly equal to rewards.
// Add test case for exact balance match rewards := sdk.NewDecCoinsFromCoins(sdk.NewCoin("stake", sdkmath.NewInt(33))) err = s.BankKeeper.SendCoinsFromAccountToModule(ctx, s.addrs[0], types.ModuleName, sdk.NewCoins(sdk.NewCoin("stake", sdkmath.NewInt(33))))
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (3)
tests/integration/distribution/genesis_test.go(1 hunks)tests/integration/gov/genesis_test.go(3 hunks)x/gov/genesis.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- x/gov/genesis.go
🧰 Additional context used
📓 Path-based instructions (2)
tests/integration/gov/genesis_test.go (3)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern tests/**/*: "Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull request"
Pattern **/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"
tests/integration/distribution/genesis_test.go (3)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern tests/**/*: "Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull request"
Pattern **/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"
🪛 golangci-lint (1.62.2)
tests/integration/distribution/genesis_test.go
6-6: File is not gci-ed with --skip-generated -s standard -s default -s prefix(cosmossdk.io) -s prefix(github.com/cosmos/cosmos-sdk) --custom-order
(gci)
11-11: File is not gci-ed with --skip-generated -s standard -s default -s prefix(cosmossdk.io) -s prefix(github.com/cosmos/cosmos-sdk) --custom-order
(gci)
15-15: File is not gci-ed with --skip-generated -s standard -s default -s prefix(cosmossdk.io) -s prefix(github.com/cosmos/cosmos-sdk) --custom-order
(gci)
20-20: File is not gci-ed with --skip-generated -s standard -s default -s prefix(cosmossdk.io) -s prefix(github.com/cosmos/cosmos-sdk) --custom-order
(gci)
🔇 Additional comments (5)
tests/integration/distribution/genesis_test.go (3)
1-66: LGTM! Well-structured test setup.
The test suite setup follows best practices with proper initialization, error handling, and test isolation.
🧰 Tools
🪛 golangci-lint (1.62.2)
6-6: File is not gci-ed with --skip-generated -s standard -s default -s prefix(cosmossdk.io) -s prefix(github.com/cosmos/cosmos-sdk) --custom-order
(gci)
11-11: File is not gci-ed with --skip-generated -s standard -s default -s prefix(cosmossdk.io) -s prefix(github.com/cosmos/cosmos-sdk) --custom-order
(gci)
15-15: File is not gci-ed with --skip-generated -s standard -s default -s prefix(cosmossdk.io) -s prefix(github.com/cosmos/cosmos-sdk) --custom-order
(gci)
20-20: File is not gci-ed with --skip-generated -s standard -s default -s prefix(cosmossdk.io) -s prefix(github.com/cosmos/cosmos-sdk) --custom-order
(gci)
127-175: LGTM! Good error case coverage.
The test effectively verifies that the system properly handles and reports insufficient module balance scenarios.
177-190: LGTM! Well-implemented helper function.
The clearDB function properly handles database cleanup with appropriate error handling and resource management.
tests/integration/gov/genesis_test.go (2)
106-109: LGTM! Good test coverage for excess balance scenario.
The added code properly simulates the scenario where the governance module has more tokens than required by deposits.
241-327: LGTM! Comprehensive test for insufficient balance scenario.
The test effectively verifies that genesis import fails when the governance module has insufficient funds to cover deposits.
oh you dont have to do this anymore. We have documentation on how to avoid it: https://docs.cosmos.network/main/build/building-apps/app-testnet, its way faster too. i agree we should fix the issue you opened, but there shouldnt be a need to export genesis anymore imo |
aha, so we're doing this as a workaround because we're not able to use the testnet stuff that's built-in :( i think the issue is that we can't change the chain-id (and this does give us more flexibility in terms of being able to change the genesis file when we start a fork) |
julienrbrt
added
the
backport/v0.52.x
|
Could you fix the conflicts? |
|
Is this ready to merge? |
Description
This PR fixes two assertions that are checked when a chain is restored from a genesis export. In both instances, a module's balance is expected to be exactly equal to its holdings in the chain. So the distribution module is expected to have the exact balance as the sum of all outstanding rewards, and governance is expected to have the exact sum of all proposal deposits. These assertions are often violated in practice, and it makes more sense to ensure that the balances are at least sufficient to cover the module holdings.
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!in the type prefix if API or client breaking changeCHANGELOG.mdReviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
Please see Pull Request Reviewer section in the contributing guide for more information on how to review a pull request.
I have...
Summary by CodeRabbit
MaxVoteOptionsLenandMsgSudoExec.