Remove panic on slash function #201
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughReplaces panic paths in x/oracle/keeper/slash.go's SlashAndResetCounters with explicit error handling and logging: validator retrieval, consensus address extraction, and slashing now return errors instead of panicking; missing validators (ErrNoValidatorFound) are skipped. Adds nil-checks and logs for validator and consensus address errors, and ensures slashing is performed only for bonded, non-jailed validators. Adds TestSlashAndResetCounters_MultipleValidatorsNotFound to verify behavior when some validators are missing, removes one failing rewards genesis test case, and updates CHANGELOG.md. No exported symbols changed. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Pull request overview
This PR addresses a critical bug in the Oracle module's SlashAndResetCounters function by replacing panic calls with error returns, preventing potential chain halts when validators can't be found during the slashing process.
Key Changes:
- Replaced
panic(err)withreturn false, errwhenStakingKeeper.Validator()fails - Replaced
panic(err)withreturn false, errwhenGetConsAddr()fails
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![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
♻️ Duplicate comments (2)
x/oracle/keeper/slash.go (2)
55-58: Inconsistent with past review: returning error may halt iteration.Lines 55-58 return
false, errfor non-ErrNoValidatorFound errors, which will stop processing remaining validators. Past reviews on this PR recommended logging and returningfalse, nilto ensure one validator's error doesn't block others from being processed.🔎 Align with past review feedback
// Check for other errors - if err != nil || validator == nil { - k.Logger(ctx).Error("failed to get validator for slashing", "operator", operator.String(), "error", err) - return false, err - } + if err != nil || validator == nil { + k.Logger(ctx).Error("failed to get validator for slashing", "operator", operator.String(), "error", err) + return false, nil + }
63-67: Inconsistent with past review: consensus address error halts iteration.Line 66 returns
false, errwhen getting the consensus address fails. Past reviews on this PR specifically recommended returningfalse, nilhere to prevent a single validator's consensus address error from stopping the processing of other validators.🔎 Align with past review feedback
consAddr, err := validator.GetConsAddr() if err != nil { k.Logger(ctx).Error("failed to get consensus address for slashing", "operator", operator.String(), "error", err) - return false, err + return false, nil }
🧹 Nitpick comments (1)
x/oracle/keeper/slash_test.go (1)
184-238: Good test coverage for the missing validator scenario.The test correctly addresses the past review comment requesting coverage for when a validator has a VotePenaltyCounter but no longer exists. It verifies that:
- SlashAndResetCounters doesn't panic when validators are missing
- Existing validators are still processed and slashed correctly
Optional: Consider additional error scenario coverage
While this test covers the
ErrNoValidatorFoundcase from issue #199, you might consider adding test cases for other error scenarios mentioned in the code changes:
- Validator retrieval errors (non-ErrNoValidatorFound)
- Consensus address retrieval errors
- Slashing operation errors
These would provide more comprehensive coverage of the error handling paths, though they're not critical given the main issue is addressed.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
x/oracle/keeper/slash.gox/oracle/keeper/slash_test.go
🧰 Additional context used
🧬 Code graph analysis (1)
x/oracle/keeper/slash.go (1)
x/oracle/types/expected_keepers.go (1)
StakingKeeper(17-24)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: tests
- GitHub Check: test-e2e
- GitHub Check: golangci-lint
- GitHub Check: liveness-test
- GitHub Check: Analyze
🔇 Additional comments (3)
x/oracle/keeper/slash.go (3)
4-4: LGTM!The new imports are necessary for the error handling improvements.
Also applies to: 10-10
60-61: Good addition: bonded and non-jailed guard.The check ensures slashing is only performed on bonded, non-jailed validators, which is the correct behavior.
73-77: Verify: should slashing errors halt iteration?Line 76 returns
true, errwhen slashing fails, which stops processing remaining validators. While this might be intentional (slashing errors could indicate serious system issues), it contradicts the overall goal of resilient processing described in issue #199.Consider whether slashing errors should also return
false, nilwith logging to allow other validators to be processed.
There was a problem hiding this comment.
Actionable comments posted: 2
Fix all issues with AI Agents 🤖
In @x/oracle/keeper/slash_test.go:
- Line 196: The comment above the test loop is inaccurate — it says "multiple
non existing validators" but the code only skips a single validator
(ValAddrs[2]); update the comment to accurately reflect one non-existing
validator (e.g., "Set the vote penalty counter for one non-existing validator")
so it matches the logic around ValAddrs[2] in the test.
- Around line 197-213: Validators created via msgServer.CreateValidator are not
being finalized into bonded state because stakingKeeper.EndBlocker(ctx) is not
called; add a call to stakingKeeper.EndBlocker(input.Ctx) (or
stakingKeeper.EndBlocker(ctx)) after the loop that runs CreateValidator (and
VotePenaltyCounter.Set) so the staking state transitions validators to bonded
before the slashing assertions; ensure you use the same ctx (input.Ctx) and keep
the call before the slash verification lines.
🧹 Nitpick comments (1)
x/oracle/keeper/slash_test.go (1)
219-237: Consider using GetBondedTokens() for consistency.Line 236 checks
validator.Tokensdirectly, while the existing test at line 111 usesvalidator.GetBondedTokens(). For bonded validators, these should be the same, but usingGetBondedTokens()would be more consistent with existing test patterns and more explicit about what's being verified.Note: This verification also depends on fixing the missing
EndBlockercall flagged earlier, as unbonded validators won't be slashed.🔎 Suggested change for consistency
// Check that the validator has been slashed oracleParams, err := input.OracleKeeper.Params.Get(ctx) require.NoError(t, err) // Calculate expected bonded tokens after slashing expectedTokens := amount.Sub(oracleParams.SlashFraction.MulInt(amount).TruncateInt()) - require.Equal(t, expectedTokens, validator.Tokens) + require.Equal(t, expectedTokens, validator.GetBondedTokens()) }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
x/oracle/keeper/slash_test.go
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: tests
- GitHub Check: test-e2e
- GitHub Check: Analyze
- GitHub Check: golangci-lint
🔇 Additional comments (3)
x/oracle/keeper/slash_test.go (3)
184-194: LGTM! Test setup is well-structured.The test initialization follows the established patterns in the file and clearly sets up the required dependencies.
215-217: LGTM! Core test assertion is clear.The test correctly verifies that
SlashAndResetCountersdoesn't panic when some validators are not found, which aligns with the PR objective.
197-197: Thefor i := range 5syntax is compatible with the project's Go version. The project requires Go 1.23.8 (specified in go.mod), which fully supports the range-over-integer syntax introduced in Go 1.22.
Description
Remove panic on the slash function.
Related to issue #199
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Tests are passing