test(integration): port x/slashing tests to server v2 #22754
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThis pull request involves the deletion of several files related to the slashing module in a Cosmos SDK application, including 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
|
akhilkumarpilli
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: 4
🧹 Outside diff range and nitpick comments (9)
tests/integration/v2/slashing/keeper_test.go (3)
45-46: Simplify Height CalculationThe expression
+ 1 + 1in the height calculation can be simplified to+ 2for clarity.- newHeight := int64(f.app.LastBlockHeight()) + 1 + 1 + newHeight := int64(f.app.LastBlockHeight()) + 2
Line range hint
62-112: Refactor Repeated Header Update CodeThe code for updating
newHeightand setting the header info is repeated multiple times. Consider refactoring this into a helper function to improve maintainability and reduce duplication.Example:
func incrementHeaderHeight(ctx context.Context) context.Context { newHeight := integration.HeaderInfoFromContext(ctx).Height + 1 return integration.SetHeaderInfo(ctx, coreheader.Info{Height: newHeight}) }Then use:
- newHeight = integration.HeaderInfoFromContext(f.ctx).Height + 1 - f.ctx = integration.SetHeaderInfo(f.ctx, coreheader.Info{Height: newHeight}) + f.ctx = incrementHeaderHeight(f.ctx)
353-355: Maintain Consistent Context UsageInconsistent context update in
HandleValidatorSignature. In line 353, the context is set directly within the function call, whereas earlier it's set before the call. For consistency and readability, consider updating the context before the function call.- err = f.slashingKeeper.HandleValidatorSignature(integration.SetHeaderInfo(f.ctx, coreheader.Info{Height: height}), val.Address(), newPower, comet.BlockIDFlagAbsent) + f.ctx = integration.SetHeaderInfo(f.ctx, coreheader.Info{Height: height}) + err = f.slashingKeeper.HandleValidatorSignature(f.ctx, val.Address(), newPower, comet.BlockIDFlagAbsent)tests/integration/v2/slashing/slash_redelegation_test.go (1)
42-44: Avoid Repetition in Account FundingThe calls to
fundAccountfortestAcc1andtestAcc2are repeated. Consider using a loop or helper function to fund multiple accounts to improve code clarity.accounts := []sdk.AccAddress{testAcc1, testAcc2} for _, acc := range accounts { fundAccount(t, ctx, f.bankKeeper, f.accountKeeper, acc, testCoin) }testutil/sims/address_helpers.go (3)
Line range hint
24-29: Update Function Documentation for Context ChangeThe context parameter in the functions has changed from
sdk.Contexttocontext.Context. Update the function comments to reflect this change.-// AddTestAddrsFromPubKeys adds the addresses into the SimApp providing only the public keys. +// AddTestAddrsFromPubKeys adds the addresses into the SimApp providing only the public keys using context.Context.
Line range hint
51-51: Remove Unused VariableaddrCodecThe variable
addrCodecis declared but not used in the code. Consider removing it to clean up the code.-var addrCodec = codecaddress.NewBech32Codec("cosmos")
Line range hint
156-156: Handle Ineffectual Assignment toerrAt line 156, the variable
erris assigned but not used. If the error is not needed, you can omit the assignment.- createValidatorMsg, err := stakingtypes.NewMsgCreateValidator( + createValidatorMsg, _ := stakingtypes.NewMsgCreateValidator(Or handle the error appropriately.
tests/integration/v2/slashing/slashing_test.go (2)
51-51: Remove Unused VariableaddrCodecThe variable
addrCodecis declared but not used. Consider removing it to clean up the code.-var addrCodec = codecaddress.NewBech32Codec("cosmos")🧰 Tools
🪛 golangci-lint (1.62.2)
51-51: var
addrCodecis unused(unused)
156-156: Handle Ineffectual Assignment toerrAt line 156, the variable
erris assigned but not used. If the error is not needed, you can replace it with_or handle the error.- createValidatorMsg, err := stakingtypes.NewMsgCreateValidator( + createValidatorMsg, _ := stakingtypes.NewMsgCreateValidator(Or handle the error:
if err != nil { // handle error }🧰 Tools
🪛 golangci-lint (1.62.2)
156-156: ineffectual assignment to err
(ineffassign)
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (7)
tests/integration/slashing/app_config.go(0 hunks)tests/integration/slashing/slashing_test.go(0 hunks)tests/integration/v2/slashing/abci_test.go(2 hunks)tests/integration/v2/slashing/keeper_test.go(19 hunks)tests/integration/v2/slashing/slash_redelegation_test.go(15 hunks)tests/integration/v2/slashing/slashing_test.go(1 hunks)testutil/sims/address_helpers.go(4 hunks)
💤 Files with no reviewable changes (2)
- tests/integration/slashing/app_config.go
- tests/integration/slashing/slashing_test.go
🧰 Additional context used
📓 Path-based instructions (5)
tests/integration/v2/slashing/slashing_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/v2/slashing/abci_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"
testutil/sims/address_helpers.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
tests/integration/v2/slashing/keeper_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/v2/slashing/slash_redelegation_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/v2/slashing/slashing_test.go
51-51: var addrCodec is unused
(unused)
156-156: ineffectual assignment to err
(ineffassign)
26-26: ST1019: package "cosmossdk.io/x/slashing/types" is being imported more than once
(stylecheck)
27-27: ST1019(related information): other import of "cosmossdk.io/x/slashing/types"
(stylecheck)
tests/integration/v2/slashing/slash_redelegation_test.go
221-221: ineffectual assignment to err
(ineffassign)
🔇 Additional comments (4)
tests/integration/v2/slashing/keeper_test.go (2)
1-1: Verify Package Name Change
The package name has changed from keeper_test to slashing. Ensure that this change aligns with the project's package structure and does not cause issues with test discovery or access to unexported identifiers.
88-93: Ensure Proper Error Handling in Message Execution
When running f.app.RunMsg, ensure that errors are handled appropriately. In tests, it's important to check for specific errors or confirm success.
Consider adding an assertion:
_, err = f.app.RunMsg(
t,
f.ctx,
func(ctx context.Context) (transaction.Msg, error) {
res, err := f.slashingMsgServer.Unjail(ctx, &msgUnjail)
return res, err
},
integration.WithAutomaticCommit(),
)
+assert.NilError(t, err)tests/integration/v2/slashing/abci_test.go (1)
27-27: Initialize Context with Correct Height
Ensure that the initial context height is set appropriately before starting the test to avoid any issues with block heights.
+ctx = integration.SetHeaderInfo(ctx, coreheader.Info{Height: 1})testutil/sims/address_helpers.go (1)
26-27: 🛠️ Refactor suggestion
Consolidate Duplicate Imports
Packages cosmossdk.io/x/slashing/types are imported more than once. Remove the duplicate import to follow Go import conventions.
-import (
//...
- "cosmossdk.io/x/slashing/types"
- //...
- slashingtypes "cosmossdk.io/x/slashing/types"
+import (
//...
+ slashingtypes "cosmossdk.io/x/slashing/types"
//...
)Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
tests/integration/v2/slashing/slash_redelegation_test.go (1)
161-164: Consider extracting magic numbers into named constants.Values like slashing percentages and test amounts could be more maintainable as named constants at the package level.
+const ( + defaultSlashFraction = "0.45" + defaultUndelegationPercentage = "0.30" + defaultTestAmount = 1_000_000 +) func TestOverSlashing(t *testing.T) { f := initFixture(t) ctx := f.ctx - slashFraction := "0.45" - undelegationPercentageStr := "0.30" + slashFraction := defaultSlashFraction + undelegationPercentageStr := defaultUndelegationPercentageAlso applies to: 189-191
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (2)
tests/integration/v2/slashing/slash_redelegation_test.go(15 hunks)tests/integration/v2/slashing/slashing_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/integration/v2/slashing/slashing_test.go
🧰 Additional context used
📓 Path-based instructions (1)
tests/integration/v2/slashing/slash_redelegation_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"
🔇 Additional comments (4)
tests/integration/v2/slashing/slash_redelegation_test.go (4)
Line range hint 1-21: LGTM! Package and imports are well organized.
The package name change to slashing better reflects the module being tested, and the imports are appropriately organized.
145-154: LGTM! Well-structured helper function.
The fundAccount helper function follows testing best practices with proper error handling and account initialization.
Line range hint 23-431: LGTM! Comprehensive test coverage.
The test suite thoroughly covers various slashing scenarios including:
- Basic slashing with redelegation
- Over-slashing protection
- Edge case with validator having zero tokens
72-73:
Add error checks for app.Commit calls.
Multiple instances of unchecked errors from f.app.Commit(state). While this was flagged in previous reviews, the issue persists across the file.
Apply this pattern to all commit calls:
_, state = f.app.Deliver(t, ctx, nil)
-_, err = f.app.Commit(state)
+_, err = f.app.Commit(state)
+require.NoError(t, err)Also applies to: 89-90, 117-118, 133-134, 238-239, 253-254, 280-281, 290-291, 303-304, 369-370, 381-382, 398-399, 412-413
Description
Closes: #22716
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
Refactor
Bug Fixes