test(integration): port x/slashing tests to server v2 #22754
Conversation
📝 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.
| evilPower := f.stakingKeeper.TokensToConsensusPower(ctx, evilVal.Tokens) | ||
|
|
There was a problem hiding this comment.
Check Error Returns When Getting Validator
At line 93, the error from f.stakingKeeper.GetValidator is not checked. Ensure that you handle potential errors.
evilVal, err := f.stakingKeeper.GetValidator(ctx, evilValAddr)
+require.NoError(t, err)Committable suggestion skipped: line range outside the PR's diff.
| require.NoError(t, err) | ||
| ctx = integration.SetHeaderInfo(ctx, header.Info{Height: int64(f.app.LastBlockHeight()) + 1}) | ||
| _, state := f.app.Deliver(t, ctx, nil) | ||
| _, err = f.app.Commit(state) |
There was a problem hiding this comment.
Handle Error from f.app.Commit
At line 221, the error returned by f.app.Commit(state) is not handled. It's important to check for errors to ensure the commit was successful.
_, err = f.app.Commit(state)
+ require.NoError(t, err)Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 golangci-lint (1.62.2)
221-221: ineffectual assignment to err
(ineffassign)
|
|
||
| err = slashing.BeginBlocker(ctx, slashingKeeper, cometInfoService) | ||
| err = slashing.BeginBlocker(ctx, f.slashingKeeper, cometInfoService) |
There was a problem hiding this comment.
Pass Correct Comet Info to BeginBlocker
The cometInfoService is currently uninitialized. Make sure it provides the necessary Comet info to BeginBlocker.
Consider initializing cometInfoService with the required data or mocking it appropriately.
| "cosmossdk.io/x/slashing/types" | ||
| slashingtypes "cosmossdk.io/x/slashing/types" |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove Duplicate Import of slashing/types
The package cosmossdk.io/x/slashing/types is imported twice. Remove the duplicate import to adhere to Go's import style guidelines.
import (
//...
- "cosmossdk.io/x/slashing/types"
//...
slashingtypes "cosmossdk.io/x/slashing/types"
//...
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "cosmossdk.io/x/slashing/types" | |
| slashingtypes "cosmossdk.io/x/slashing/types" | |
| slashingtypes "cosmossdk.io/x/slashing/types" |
🧰 Tools
🪛 golangci-lint (1.62.2)
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)
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