Reinstate the TestAggregateIntentWithPRClaims #1320
Conversation
WalkthroughThe recent changes involve refactoring the test functions in Changes
Assessment against linked issues
Poem
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1320 +/- ##
==========================================
+ Coverage 63.60% 63.68% +0.08%
==========================================
Files 171 171
Lines 11652 11652
==========================================
+ Hits 7411 7421 +10
+ Misses 3497 3488 -9
+ Partials 744 743 -1
Flags with carried forward coverage won't be shown. Click here to find out more. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| }, | ||
| &types.Validator{ | ||
| ValoperAddress: "cosmosvaloper156gqf9837u7d4c4678yt3rl4ls9c5vuursrrzf", | ||
| /* |
Check notice
Code scanning / devskim
A "TODO" or similar was left in source code, possibly indicating incomplete functionality Note test
tuantran1702
requested review from
joe-bowman,
faddat and
minhngoc274
as code owners
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (2)
- x/interchainstaking/keeper/intent_test.go (3 hunks)
- x/interchainstaking/keeper/zones_test.go (3 hunks)
Additional comments: 5
x/interchainstaking/keeper/zones_test.go (2)
- 341-391: The function
TestZone_GetBondedValidatorAddressesAsSliceis entirely commented out. While there's a previous bot comment regarding a TODO in the source code, it's essential to address or remove commented-out code sections to maintain codebase cleanliness and readability. If this test is no longer relevant, consider removing it entirely or updating it to reflect the current codebase state.- 434-445: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [392-465]
The addition of
TestZone_GetAggregateIntentOrDefaultis a positive step towards enhancing test coverage for intent aggregation. To further improve the robustness of this test, consider adding more scenarios and edge cases, such as handling empty validator lists or validators with zero voting power.x/interchainstaking/keeper/intent_test.go (3)
- 138-138: The use of a hardcoded value
7313913for setting up test balances might limit the flexibility and readability of the test. Consider defining this as a constant at the top of your test file or within the test function itself, with a descriptive name that explains its purpose or origin. This approach enhances maintainability and readability.- 360-484: This test case,
TestAggregateIntentWithPRClaims, introduces scenarios that combine intents, balances, and claims to test the aggregation logic. However, there are a few areas that could be improved for clarity and maintainability:
- The test cases use hardcoded values for balances and claims. Similar to the previous comment, consider using named constants for these values to improve readability and maintainability.
- The comments within the test cases, such as "four delegators each at 25%", could be expanded to explain the rationale behind the test scenario more clearly. This would help future maintainers understand the purpose of each test case more quickly.
- The use of
Sort()in theexpectedfunction closures is a good practice for ensuring consistent ordering, but it's worth noting in a comment that the order is significant for the comparison to succeed, as this might not be immediately obvious to someone new to the codebase.Overall, these tests are well-structured and provide good coverage of the functionality being tested. Enhancing the clarity and maintainability of the test cases as suggested would make them even more valuable.
- 486-640: The functions
TestDefaultIntent,TestDefaultIntentWithJailed,TestDefaultIntentWithTombstoned,TestDefaultIntentWithCommission100, andTestDefaultIntentWithOneUnbondedOneUnbondingprovide comprehensive coverage for various scenarios involving default intent calculation. A few observations and suggestions:
- The repeated setup code for creating a
zoneand setting validators could be refactored into a helper function to reduce duplication and improve readability. This would make each test function more focused on the specific scenario it's testing.- The assertions for the expected weights in the validators' intents are clear and straightforward. However, adding brief comments explaining why certain weights are expected in each scenario could help future maintainers understand the test logic more quickly.
- The handling of different validator statuses (e.g., jailed, tombstoned, unbonded, unbonding) is well-tested, which is crucial for ensuring the robustness of the default intent calculation logic.
Refactoring to reduce code duplication and adding explanatory comments would further improve the quality of these tests.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- x/interchainstaking/keeper/intent_test.go (3 hunks)
Files skipped from review as they are similar to previous changes (1)
- x/interchainstaking/keeper/intent_test.go
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- x/interchainstaking/keeper/intent_test.go (3 hunks)
Files not summarized due to errors (1)
- x/interchainstaking/keeper/intent_test.go: Error: Message exceeds token limit
Additional comments: 2
x/interchainstaking/keeper/intent_test.go (2)
- 360-531: The test cases in
TestAggregateIntentWithPRClaimsare well-structured and cover a variety of scenarios, including single and multiple intents and claims, with different supply percentages. However, there's a potential improvement in the clarity of test case names and the documentation of the logic behind expected outcomes. Specifically, for complex scenarios like "two multi-val intents, two claims, 50% supply," it would be beneficial to add comments explaining how the expected weights are calculated to aid in understanding the test's intent and ensuring its correctness.Consider adding detailed comments within each test case, especially for the
expectedfunction, to explain the rationale behind the expected outcomes. This will improve maintainability and ease future modifications or debugging.
- 587-709: The
TestDefaultIntentand related tests (TestDefaultIntentWithJailed,TestDefaultIntentWithTombstoned,TestDefaultIntentWithCommission100,TestDefaultIntentWithOneUnbondedOneUnbonding) effectively cover various scenarios affecting the default intent aggregation. It's important to ensure that these tests accurately reflect the intended behavior of the system, especially in edge cases like handling jailed or tombstoned validators, and validators with different bonding statuses.These tests are comprehensive and cover a wide range of scenarios. Ensure that the logic for handling different validator statuses remains consistent with the overall design and requirements of the interchainstaking module.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (1)
- x/interchainstaking/keeper/intent_test.go (3 hunks)
Files not summarized due to errors (1)
- x/interchainstaking/keeper/intent_test.go: Error: Message exceeds token limit
Additional comments (5)
x/interchainstaking/keeper/intent_test.go (5)
- 587-616: In
TestDefaultIntent, the test verifies that the default aggregate intents are calculated correctly, expecting each validator to have a weight of 0.2 (20%). The logic and assertions appear correct and align with the expected behavior of evenly distributing weights among validators when no specific intents are set. This test effectively ensures the fallback mechanism to default intents works as intended.- 618-647: In
TestDefaultIntentWithJailed, the test correctly handles the scenario where a validator is jailed, ensuring they are excluded from the default aggregate intents calculation. This is crucial for maintaining the integrity of the staking process, as jailed validators should not receive delegations. The test's logic and assertions are appropriate and validate the expected behavior accurately.- 649-678:
TestDefaultIntentWithTombstonedeffectively tests the scenario where a validator is tombstoned, ensuring they are excluded from the default aggregate intents calculation. This aligns with the expected behavior, as tombstoned validators, similar to jailed ones, should not receive delegations. The test's logic and assertions are correct, ensuring the system's integrity in handling validator statuses.- 680-709: In
TestDefaultIntentWithCommission100, the test scenario where a validator has a 100% commission rate is correctly handled, ensuring they are excluded from the default aggregate intents calculation. This scenario is important for ensuring that validators with unrealistic commission rates do not unfairly influence the default intent calculation. The test's logic and assertions are appropriate and validate the expected behavior accurately.- 711-741:
TestDefaultIntentWithOneUnbondedOneUnbondingaccurately tests the scenario where validators are in unbonded and unbonding states, ensuring they are excluded from the default aggregate intents calculation. This is crucial for the staking process, as validators not in an active bonded state should not receive delegations. The test's logic and assertions correctly validate the expected behavior, ensuring the system's integrity in handling various validator states.
| user1.String(): {UserAddress: user1.String(), ChainId: zone.ChainId, Module: cmtypes.ClaimTypeLiquidToken, Amount: sdkmath.NewInt(1000), SourceChainId: "osmosis-1"}, | ||
| user1.String(): {UserAddress: user1.String(), ChainId: zone.ChainId, Module: cmtypes.ClaimTypeLiquidToken, Amount: sdkmath.NewInt(1000), SourceChainId: "osmosis-1"}, | ||
| } |
There was a problem hiding this comment.
Rabit flagged that only one claim is set here because they both use the same map index.
There was a problem hiding this comment.
Actionable comments posted: 0
Review Status
Configuration used: CodeRabbit UI
Files selected for processing (1)
- x/interchainstaking/keeper/intent_test.go (3 hunks)
Files not summarized due to errors (1)
- x/interchainstaking/keeper/intent_test.go: Error: Message exceeds token limit
Additional Context Used
Additional comments not posted (2)
x/interchainstaking/keeper/intent_test.go (2)
360-588: The test cases inTestAggregateIntentWithPRClaimsare comprehensive, covering various scenarios with single and multiple intents and claims. However, ensure that the test cases accurately reflect the intended scenarios, especially considering the previous comment about a duplicate map key in the claims setup. This could potentially lead to misleading test outcomes if not addressed.
590-744: The functionsTestDefaultIntent,TestDefaultIntentWithJailed,TestDefaultIntentWithTombstoned,TestDefaultIntentWithCommission100, andTestDefaultIntentWithOneUnbondedOneUnbondingprovide a good range of scenarios for testing default intent calculations under various conditions. It's crucial to ensure that these tests accurately simulate the conditions they're meant to test, such as the handling of jailed, tombstoned, unbonded, and unbonding validators, as well as those with a 100% commission rate. This will help ensure the robustness of the default intent calculation logic.
|
This pull request has been deployed to Vercel.
|
* fix undefined window error (#1396) * chore(deps): bump github.com/cosmos/ibc-go/v5 from 5.3.2 to 5.4.0 (#1398) Bumps [github.com/cosmos/ibc-go/v5](https://github.com/cosmos/ibc-go) from 5.3.2 to 5.4.0. - [Release notes](https://github.com/cosmos/ibc-go/releases) - [Changelog](https://github.com/cosmos/ibc-go/blob/v5.4.0/CHANGELOG.md) - [Commits](cosmos/ibc-go@v5.3.2...v5.4.0) --- updated-dependencies: - dependency-name: github.com/cosmos/ibc-go/v5 dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * chore(deps): bump golang.org/x/tools from 0.19.0 to 0.20.0 (#1392) Bumps [golang.org/x/tools](https://github.com/golang/tools) from 0.19.0 to 0.20.0. - [Release notes](https://github.com/golang/tools/releases) - [Commits](golang/tools@v0.19.0...v0.20.0) --- updated-dependencies: - dependency-name: golang.org/x/tools dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * chore(deps): bump google.golang.org/grpc from 1.62.1 to 1.63.0 (#1383) Bumps [google.golang.org/grpc](https://github.com/grpc/grpc-go) from 1.62.1 to 1.63.0. - [Release notes](https://github.com/grpc/grpc-go/releases) - [Commits](grpc/grpc-go@v1.62.1...v1.63.0) --- updated-dependencies: - dependency-name: google.golang.org/grpc dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Joe Bowman <[email protected]> * use correct supply when determining proportions (#1389) * use correct supply when determining proportions * fix HandleMsgTransfer tests --------- Co-authored-by: Tuan Tran <[email protected]> * Reinstate the TestAggregateIntentWithPRClaims (#1320) * fix(intent-test): fix the claim to match the supply * refactor: enable some tests and convert them to keeper tests * fix int64 to sdkmath.Int * rename alias to sdkmath to avoid confusion * fix claim to match supply * remove balance * add unclaimedRatio * add multi claim, less than 100% supply * add tests, write comment for reasoning expected output * fix tests failing due to #1345 * fix test --------- Co-authored-by: Jacob Gadikian <[email protected]> Co-authored-by: Joe Bowman <[email protected]> * fix: Withdrawal records should not exist with 0 burnAmount (#1284) * add guard to ```AddWithdrawalRecord``` and ```SetWithdrawalRecord``` * fix tests missing BurnAmount field * return error instead of panic for ```SetwithdrawalRecord``` and ```AddWithdrawalRecord``` * fix linting * return error * refactor to check for error * add missing BurnAmount field in TestGetRatio * refactor test * add old function for testing upgrade handler * add upgrade handler and test * add Upgrade handler to testnet upgrades * Update x/interchainstaking/keeper/withdrawal_record.go comment Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * add test for set withdrawal records * refactor: remove old code from keeper * linting * linting * resolve conflict, fix upgrade name * linting --------- Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Co-authored-by: Jacob Gadikian <[email protected]> Co-authored-by: Joe Bowman <[email protected]> --------- Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: Joseph Chalabi <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Joe Bowman <[email protected]> Co-authored-by: Tuan Tran <[email protected]> Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
1. Summary
Fixes #1296
2.Type of change
3. Implementation details
4. How to test/use
5. Checklist
6. Limitations (optional)
7. Future Work (optional)
Summary by CodeRabbit