Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

test(integration): port x/slashing tests to server v2 #22754

Merged
merged 6 commits into from
Dec 5, 2024

Conversation

akhilkumarpilli
Copy link
Contributor

@akhilkumarpilli akhilkumarpilli commented Dec 4, 2024

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...

  • included the correct type prefix in the PR title, you can find examples of the prefixes below:
  • confirmed ! in the type prefix if API or client breaking change
  • targeted the correct branch (see PR Targeting)
  • provided a link to the relevant issue or specification
  • reviewed "Files changed" and left comments if necessary
  • included the necessary unit and integration tests
  • added a changelog entry to CHANGELOG.md
  • updated the relevant documentation or specification, including comments for documenting Go code
  • confirmed all CI checks have passed

Reviewers 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...

  • confirmed the correct type prefix in the PR title
  • confirmed all author checklist items have been addressed
  • reviewed state machine logic, API design and naming, documentation is accurate, tests and test coverage

Summary by CodeRabbit

  • New Features

    • Introduced a new test file for slashing integration tests, enhancing validation of slashing functionalities.
  • Refactor

    • Refactored test setups to streamline context management and improve clarity in integration tests.
    • Updated function signatures to utilize a new context type for better compatibility.
  • Bug Fixes

    • Ensured that the slashing logic and validator management are correctly validated in the new testing framework.

Copy link
Contributor

coderabbitai bot commented Dec 4, 2024

📝 Walkthrough
📝 Walkthrough

Walkthrough

This pull request involves the deletion of several files related to the slashing module in a Cosmos SDK application, including app_config.go and slashing_test.go. The changes also include significant refactoring in the integration tests located in abci_test.go, keeper_test.go, and slash_redelegation_test.go, focusing on the restructuring of test setups and the management of context and dependencies. Additionally, there are modifications to function signatures in address_helpers.go, changing the context type used in various functions.

Changes

File Path Change Summary
tests/integration/slashing/app_config.go Deleted: Responsible for application configuration and module integration within the Cosmos SDK.
tests/integration/slashing/slashing_test.go Deleted: Contained integration tests for the slashing module, including validator management.
tests/integration/v2/slashing/abci_test.go Refactored: Updated setup process, replaced variable declarations with fixture initialization.
tests/integration/v2/slashing/keeper_test.go Modified: Changed package name and removed various imports; updated context and block height management.
tests/integration/v2/slashing/slash_redelegation_test.go Modified: Renamed package, updated test setup to use a fixture pattern, and adjusted context management.
tests/integration/v2/slashing/slashing_test.go New: Introduced integration tests for the slashing module with a fixture-based setup.
testutil/sims/address_helpers.go Modified: Changed context parameter type from sdk.Context to context.Context in several functions.

Possibly related PRs

Suggested labels

C:x/slashing, C:server/v2, backport/v0.52.x

Suggested reviewers

  • tac0turtle
  • sontrinh16
  • julienrbrt

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?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@akhilkumarpilli akhilkumarpilli changed the title WIP test(integration): port x/slashing tests to server v2 test(integration): port x/slashing tests to server v2 Dec 5, 2024
@akhilkumarpilli akhilkumarpilli marked this pull request as ready for review December 5, 2024 11:56
@akhilkumarpilli akhilkumarpilli requested a review from a team as a code owner December 5, 2024 11:56
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 4

🧹 Outside diff range and nitpick comments (9)
tests/integration/v2/slashing/keeper_test.go (3)

45-46: Simplify Height Calculation

The expression + 1 + 1 in the height calculation can be simplified to + 2 for clarity.

-	newHeight := int64(f.app.LastBlockHeight()) + 1 + 1
+	newHeight := int64(f.app.LastBlockHeight()) + 2

Line range hint 62-112: Refactor Repeated Header Update Code

The code for updating newHeight and 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 Usage

Inconsistent 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 Funding

The calls to fundAccount for testAcc1 and testAcc2 are 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 Change

The context parameter in the functions has changed from sdk.Context to context.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 Variable addrCodec

The variable addrCodec is 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 to err

At line 156, the variable err is 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 Variable addrCodec

The variable addrCodec is 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 addrCodec is unused

(unused)


156-156: Handle Ineffectual Assignment to err

At line 156, the variable err is 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

📥 Commits

Reviewing files that changed from the base of the PR and between 7fa2356 and c4fd8c0.

📒 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.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 := defaultUndelegationPercentage

Also applies to: 189-191

📜 Review details

Configuration used: .coderabbit.yml
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between c4fd8c0 and c4f83f2.

📒 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: ⚠️ Potential issue

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

Copy link
Member

@julienrbrt julienrbrt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👏🏾 👏🏾 👏🏾

@julienrbrt julienrbrt added this pull request to the merge queue Dec 5, 2024
Merged via the queue into main with commit 00c7756 Dec 5, 2024
76 of 77 checks passed
@julienrbrt julienrbrt deleted the akhil/slashing-integration-v2 branch December 5, 2024 13:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

x/slashing: server/v2 integration tests
4 participants