refactor: simplify group dependency graph and align 0.52 and main #22978
Conversation
julienrbrt
added
the
backport/v0.52.x
📝 Walkthrough📝 WalkthroughWalkthroughThis pull request introduces several modifications across multiple modules in the Cosmos SDK, focusing on refactoring test utilities, updating dependency management, and streamlining interfaces. The changes primarily affect the Changes
Possibly related PRs
Suggested Reviewers
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 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
|
github-actions
bot
added
C:x/distribution
julienrbrt
changed the title
julienrbrt
requested review from
a team,
lucaslopezf,
facundomedica,
kocubinski,
akhilkumarpilli and
sontrinh16
as code owners
github-prbot
assigned akhilkumarpilli, facundomedica, kocubinski, lucaslopezf and sontrinh16
![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 (8)
x/group/keeper/msg_server_test.go (2)
2058-2058: Ensure consistent state for minted or transferred coins.
Double-check that repeated calls to SendCoinsFromModuleToAccount won't create unexpected balances in subsequent tests.
2722-2722: Validate repeated fund operations if test logic is reused.
Repeatedly sending coins could inflate test accounts more than intended. Check if each test scenario starts from a known baseline and resets state properly.x/group/testutil/expected_keepers.go (1)
38-38: New SendCoinsFromModuleToModule method supports flexible module transfers.
This expansion of the BankKeeper interface is generally good for covering more scenarios in tests. Ensure all module-to-module transfers are properly tested for error handling.x/mint/depinject.go (1)
91-95: Consider simplifying the nested conditional logic.The current implementation uses nested if-else statements which could be simplified for better readability.
Consider this alternative structure:
-if mintFn == nil { - if inflationCalculationFn != nil && stakingKeeper != nil { - mintFn = keeper.DefaultMintFn(inflationCalculationFn, stakingKeeper, mintKeeper) - } else { - mintFn = keeper.DefaultMintFn(types.DefaultInflationCalculationFn, stakingKeeper, mintKeeper) - } +if mintFn == nil { + calculationFn := types.DefaultInflationCalculationFn + if inflationCalculationFn != nil { + calculationFn = inflationCalculationFn + } + mintFn = keeper.DefaultMintFn(calculationFn, stakingKeeper, mintKeeper) }x/group/keeper/keeper_test.go (1)
49-49: LGTM! Consider extracting test data setup.The test suite setup is well-structured with proper mock initialization and error handling. Consider extracting the test data setup (addresses, weights) into test constants or helper functions for better maintainability.
Also applies to: 60-74
x/group/keeper/abci_test.go (3)
Line range hint
16-313: Consider more descriptive test case names.While the test coverage is comprehensive, some test case names could be more descriptive. For example, "proposal pruned after executor result success" could be "should_prune_proposal_when_execution_succeeds".
🧰 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)
Line range hint
502-540: Add documentation for helper functions.Consider adding documentation comments for the helper functions to explain their purpose, parameters, and return values.
Example:
// submitProposalHelper submits a proposal with the given messages and proposers. // It returns the proposal ID and any error encountered. func submitProposalHelper(...)
11-12: Fix import ordering.The imports should be ordered according to the project's gci configuration:
- standard library
- default packages
- prefix(cosmossdk.io)
- prefix(github.com/cosmos/cosmos-sdk)
🧰 Tools
🪛 golangci-lint (1.62.2)
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)
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
x/group/go.sumis excluded by!**/*.sum
📒 Files selected for processing (13)
x/bank/testutil/helpers.go(2 hunks)x/distribution/go.mod(0 hunks)x/group/go.mod(1 hunks)x/group/keeper/abci_test.go(27 hunks)x/group/keeper/keeper_test.go(5 hunks)x/group/keeper/msg_server_test.go(3 hunks)x/group/testutil/app_config.go(0 hunks)x/group/testutil/expected_keepers.go(2 hunks)x/group/testutil/expected_keepers_mocks.go(2 hunks)x/mint/CHANGELOG.md(1 hunks)x/mint/depinject.go(1 hunks)x/mint/go.mod(0 hunks)x/params/go.mod(0 hunks)
💤 Files with no reviewable changes (4)
- x/distribution/go.mod
- x/mint/go.mod
- x/params/go.mod
- x/group/testutil/app_config.go
🧰 Additional context used
📓 Path-based instructions (8)
x/bank/testutil/helpers.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
x/mint/depinject.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
x/mint/CHANGELOG.md (1)
Pattern **/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"
x/group/testutil/expected_keepers.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
x/group/keeper/keeper_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"
x/group/testutil/expected_keepers_mocks.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
x/group/keeper/msg_server_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"
x/group/keeper/abci_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"
🪛 golangci-lint (1.62.2)
x/group/keeper/abci_test.go
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)
🔇 Additional comments (14)
x/group/keeper/msg_server_test.go (2)
20-20: No issues identified with the new import.
This import for "github.com/cosmos/cosmos-sdk/testutil" seems consistent and does not introduce any observable problem.
2057-2057: Confirm mock usage logic for multiple calls.
Using .AnyTimes() in the mock setup is acceptable for repeated calls; ensure it's not masking unexpected repeated invocations that might indicate a latent bug.
x/group/testutil/expected_keepers.go (2)
8-8: Slight import alias change is fine.
The import "cosmossdk.io/core/address" is properly referenced as is; no concerns seen.
42-45: Introduction of StakingKeeper interface.
This separate interface for staking–specific calls is a clean separation of concerns. It's recommended to provide thorough test coverage to verify edge cases (e.g., a nonexistent bond denom).
x/bank/testutil/helpers.go (3)
11-17: New minimalBankKeeper interface clarifies required capabilities for tests.
This is a valuable refactor that ensures tests rely on a narrower set of methods, reducing coupling.
25-25: FundAccount now depends on minimalBankKeeper.
Switching to minimalBankKeeper keeps the function signature aligned with the new, slimmer testing interface. The logic remains unchanged.
39-39: FundModuleAccount now also uses minimalBankKeeper.
Similar to FundAccount, this maintains consistency. Confirm that existing test cases cover scenarios for module funding thoroughly.
x/mint/CHANGELOG.md (1)
42-42: Document updated signatures for DefaultMintFn.
This clarifies that DefaultMintFn now needs StakingKeeper and MintKeeper. The changelog effectively communicates the breaking change and required migration steps.
x/mint/depinject.go (1)
80-89: LGTM: Robust error handling for function parameters.
The error handling is comprehensive, properly validating the mutual exclusivity of mintFn and inflationCalculationFn while ensuring at least one required parameter is provided.
x/group/go.mod (1)
37-37: LGTM: Appropriate dependency management.
The addition of cosmossdk.io/x/staking as an indirect dependency is correctly marked and follows Go module conventions.
x/group/testutil/expected_keepers_mocks.go (2)
223-235: LGTM: Well-structured mock implementation.
The SendCoinsFromModuleToModule mock implementation follows the established patterns and includes proper helper method usage.
281-318: LGTM: Comprehensive StakingKeeper mock implementation.
The MockStakingKeeper implementation is well-structured and follows the gomock conventions consistently.
x/group/keeper/keeper_test.go (2)
78-79: LGTM! Mock expectations are properly configured.
The bank keeper mock setup is correct with appropriate expectations for coin operations.
Also applies to: 136-141
Line range hint 314-538: LGTM! Comprehensive test coverage for tally scenarios.
The test cases thoroughly cover various voting scenarios and edge cases, with proper assertions and error handling.
julienrbrt
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
x/group/keeper/abci_test.go (2)
17-19: Consider using more precise mock expectationsUsing
AnyTimes()for mock expectations could potentially mask issues by being too permissive. Consider setting specific expectations for the number of timesSendshould be called in each test case.-s.bankKeeper.EXPECT().Send(gomock.Any(), gomock.Any()).Return(&banktypes.MsgSendResponse{}, nil).AnyTimes() +s.bankKeeper.EXPECT().Send(gomock.Any(), gomock.Any()).Return(&banktypes.MsgSendResponse{}, nil).Times(1)
Line range hint
503-541: Add function documentationConsider adding documentation to explain the purpose and parameters of these helper functions. This would improve maintainability and make it easier for other developers to understand their usage.
+// submitProposalHelper creates and submits a proposal with the given messages and proposers +// Returns the proposal ID and any error encountered func submitProposalHelper(s *TestSuite, ctx context.Context, msgs []sdk.Msg, proposers []string, groupPolicyAddr sdk.AccAddress) (uint64, error) { +// submitProposalAndVoteHelper creates a proposal and submits a vote with the specified option +// Returns the proposal ID and any error encountered func submitProposalAndVoteHelper(tests/integration/v2/services.go (2)
Line range hint
221-232: BranchService struct: evaluate potential concurrency scenario
Since the integrationContext is mutated directly (gasMeter assignment, restoration), concurrency could lead to conflicting states if multiple tests run concurrently. Ensure in your test harness that each test uses an isolated context, or add synchronization if concurrency is desired.
Line range hint
305-331: RouterService: consider thread-safety on handler registrations
The current approach relies on a map of message type URLs to handlers without locking. For integration tests, this is usually acceptable. If you ever parallelize the message dispatch or add dynamic registrations at runtime, locking or concurrency guarantees might be needed.
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
tests/integration/v2/gov/common_test.go(0 hunks)tests/integration/v2/gov/keeper/fixture_test.go(0 hunks)tests/integration/v2/services.go(1 hunks)x/accounts/defaults/lockup/lockup.go(0 hunks)x/accounts/defaults/lockup/utils_test.go(0 hunks)x/group/keeper/abci_test.go(27 hunks)
💤 Files with no reviewable changes (4)
- tests/integration/v2/gov/keeper/fixture_test.go
- tests/integration/v2/gov/common_test.go
- x/accounts/defaults/lockup/utils_test.go
- x/accounts/defaults/lockup/lockup.go
🧰 Additional context used
📓 Path-based instructions (2)
x/group/keeper/abci_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"
tests/integration/v2/services.go (2)
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"
🔇 Additional comments (8)
x/group/keeper/abci_test.go (3)
7-13: LGTM: Import changes align with test suite refactoring
The addition of gomock and updated imports properly support the transition to the new test suite structure.
315-317: Same mock expectation issue as in TestEndBlockerPruning
The mock expectations should be more specific to each test case.
Line range hint 315-500: LGTM: Comprehensive test coverage
The test cases thoroughly cover various scenarios including:
- Tally updates after voting period
- Voting within the period
- Different voting outcomes
- Withdrawn proposal handling
tests/integration/v2/services.go (5)
Line range hint 173-183: Review of eventService struct and EventManager creation
The introduction of a dedicated eventService struct returning an EventManager is clear and consistent with the rest of the code.
Line range hint 185-211: Suggestion: Ensure concurrency safety if multiple goroutines emit events
The eventManager stores events in a slice within integrationContext. If there's any chance these tests run concurrently, consider synchronizing access (via a mutex or concurrency-safe structure). Otherwise, the single-threaded usage in an integration test context is likely sufficient.
Line range hint 234-266: ExecuteWithGasLimit: confirm gas logic validity
Reassigning iCtx.gasMeter to a new meter and then partially refilling the originalGasMeter is clever, but be mindful that if originalGasMeter doesn’t have enough remaining gas to consume this additional usage, it could silently cause issues. Consider either checking for an overflow or returning an error if originalGasMeter is insufficient.
Line range hint 333-340: HeaderService: usage is straightforward
The HeaderService simply returns header information from the integration context. This is fine for an integration test environment and meets the Uber Go style recommendations of short, straightforward methods.
Line range hint 345-352: GasService: usage is appropriate
The GasService retrieves the gas meter and config from the context with minimal overhead. This is aligned with the code’s pattern and consistent with the style guidelines.
Description
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
New Features
minimalBankKeeperinterface for streamlined coin operations.BankKeeperinterface for transferring coins between modules.StakingKeeperinterface for staking operations in tests.MintFnin thex/mintmodule.Bug Fixes
Documentation
x/mintmodule.Refactor
Chores
go.modfiles across multiple modules.