-
Notifications
You must be signed in to change notification settings - Fork 4k
fix(x/slashing): fix logical error in TestSlashingMsgs test #25254
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
base: main
Are you sure you want to change the base?
Conversation
📝 WalkthroughWalkthroughUpdated x/slashing/app_test.go: TestSlashingMsgs now queries validator signing info using the consensus address derived from the validator’s public key instead of the account address. No other logic or public APIs changed. Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~2 minutes Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this 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
🧹 Nitpick comments (4)
x/slashing/app_test.go (4)
110-110
: Fix misleading comment: it’s “not jailed,” not “unknown validator.”The validator exists; the expected error is
ErrValidatorNotJailed
, so the comment should reflect that.Apply this diff:
- // unjail should fail with unknown validator + // unjail should fail because the validator is not jailed
106-106
: Optional: Use a consistent context constructor.You’ve used both
baseApp.NewContext(true)
andapp.NewContext(true)
in this test. They’re equivalent here; consider standardizing on one for readability.- ctxCheck = app.NewContext(true) + ctxCheck = baseApp.NewContext(true)
78-78
: Redundantrequire.NoError(t, err)
check.This re-checks the same
err
from Line 71 without any new assignment. Safe to remove.- require.NoError(t, err)
33-35
: Drop the unusedvalAddr
declaration in x/slashing/app_test.go
We ranrg -nP '\bvalAddr\b' -C2 --type=go x/slashing/app_test.go
and confirmed the only occurrence is the assignment on line 34; removing it leaves no references and silences linter warnings.
Apply this diff:
var ( priv1 = secp256k1.GenPrivKey() addr1 = sdk.AccAddress(priv1.PubKey().Address()) - valKey = ed25519.GenPrivKey() - valAddr = sdk.AccAddress(valKey.PubKey().Address()) + valKey = ed25519.GenPrivKey() )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Knowledge Base: Disabled due to Reviews > Disable Knowledge Base setting
📒 Files selected for processing (1)
x/slashing/app_test.go
(1 hunks)
⏰ 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). (1)
- GitHub Check: Summary
🔇 Additional comments (1)
x/slashing/app_test.go (1)
107-108
: Correctly targeting the validator’s consensus address for signing info.Switching to
sdk.ConsAddress(valKey.PubKey().Address())
ensures the query matches the validator created withvalKey
inMsgCreateValidator
. This aligns the slashing lookup with the actual consensus key under test. LGTM.
lint fixeed |
Fix logical inconsistency in TestSlashingMsgs where validator signing info
was being queried for the wrong address.
The test creates a validator using addr1 and valKey, but then attempts
to get signing info for valAddr (which is derived from valKey but not
the same as the created validator's address). This creates a mismatch
between the validator being tested and the address being queried.
Changed GetValidatorSigningInfo call from sdk.ConsAddress(valAddr) to
sdk.ConsAddress(valKey.PubKey().Address()) to ensure the signing info
is queried for the correct validator that was actually created in the test.
Summary by CodeRabbit