Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 9 minutes and 24 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughOracle slashing now sends slashed tokens to the distribution module’s community pool instead of burning them. The oracle Keeper gained a DistributionKeeper field and its constructor/signature was updated. A new keeper method Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Additional contextFocus review on token arithmetic and truncation, clamping to validator balances, correct use of RemoveValidatorTokens vs. burning, accurate creation/minting of bonded-denom coins and module sender, distribution keeper wiring, preservation of total supply invariants in tests, and consistency of updated interfaces and keeper wiring. 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@x/oracle/keeper/slash.go`:
- Around line 107-153: The current slashToPool helper bypasses
staking.Keeper.Slash and therefore loses infraction-height filtering, hook
invocation, and correct effective-slash semantics; update slashToPool to call
k.StakingKeeper.Slash(ctx, consAddr, infractionHeight,
slashAmountConsensusPower, slashFactor) (using the same infraction height used
by the module when detecting the infraction) to apply all staking semantics,
then intercept/replace the final burn step by transferring the burned amount to
the community pool via k.distrKeeper.FundCommunityPool instead of leaving it
burned; ensure you no longer call RemoveValidatorTokens directly for slashing
(so BeforeValidatorSlashed and UBD/redelegation filtering run) and keep logging
the redirected amount.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c9dc7b5f-3d7e-4f62-bf79-e45911ed270d
📒 Files selected for processing (7)
CHANGELOG.mdapp/keepers/keepers.gox/oracle/keeper/keeper.gox/oracle/keeper/slash.gox/oracle/keeper/slash_test.gox/oracle/keeper/test_utils.gox/oracle/types/expected_keepers.go
| func (k Keeper) slashToPool(ctx sdk.Context, consAddr sdk.ConsAddress, consensusPower int64, slashFactor math.LegacyDec) (math.Int, error) { | ||
| if slashFactor.IsZero() { | ||
| return math.ZeroInt(), nil | ||
| } | ||
|
|
||
| // Calculate slash amount: same math as the staking module | ||
| amount := k.StakingKeeper.TokensFromConsensusPower(ctx, consensusPower) | ||
| slashAmount := math.LegacyNewDecFromInt(amount).Mul(slashFactor).TruncateInt() | ||
|
|
||
| // Get the concrete validator (needed for RemoveValidatorTokens) | ||
| validator, err := k.StakingKeeper.GetValidatorByConsAddr(ctx, consAddr) | ||
| if err != nil { | ||
| return math.ZeroInt(), err | ||
| } | ||
|
|
||
| // Clamp slash amount to the validator's available tokens | ||
| tokensToBurn := math.MinInt(slashAmount, validator.Tokens) | ||
| if tokensToBurn.IsZero() { | ||
| return math.ZeroInt(), nil | ||
| } | ||
|
|
||
| // Remove tokens from the validator's bookkeeping without burning | ||
| _, err = k.StakingKeeper.RemoveValidatorTokens(ctx, validator, tokensToBurn) | ||
| if err != nil { | ||
| return math.ZeroInt(), err | ||
| } | ||
|
|
||
| // Create the coin (value but with akii) | ||
| bondDenom, err := k.StakingKeeper.BondDenom(ctx) | ||
| if err != nil { | ||
| return math.ZeroInt(), err | ||
| } | ||
| coins := sdk.NewCoins(sdk.NewCoin(bondDenom, tokensToBurn)) | ||
| // Send the slashed tokens from the bonded pool to the community pool | ||
| bondedPoolAddr := authtypes.NewModuleAddress(stakingtypes.BondedPoolName) | ||
| if err := k.distrKeeper.FundCommunityPool(ctx, coins, bondedPoolAddr); err != nil { | ||
| return math.ZeroInt(), err | ||
| } | ||
|
|
||
| k.Logger(ctx).Info( | ||
| "oracle slash redirected to community pool", | ||
| "validator", consAddr.String(), | ||
| "slash_factor", slashFactor.String(), | ||
| "amount", tokensToBurn.String(), | ||
| ) | ||
|
|
||
| return tokensToBurn, nil |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, find the oracle keeper slash file
find . -name "slash.go" -path "*/oracle/keeper/*" 2>/dev/nullRepository: KiiChain/kiichain
Length of output: 87
🏁 Script executed:
# Check the file size before reading
wc -l x/oracle/keeper/slash.go 2>/dev/null || find . -name "slash.go" -path "*/oracle*" -exec wc -l {} \;Repository: KiiChain/kiichain
Length of output: 89
🏁 Script executed:
# Search for where slashToPool or similar functions are used
rg "slashToPool|Slash" --type go -l | head -20Repository: KiiChain/kiichain
Length of output: 573
🏁 Script executed:
# Look for BeforeValidatorSlashed usage
rg "BeforeValidatorSlashed" --type go -A 2 -B 2Repository: KiiChain/kiichain
Length of output: 43
🏁 Script executed:
# Search for RemoveValidatorTokens usage to understand the pattern
rg "RemoveValidatorTokens" --type go -lRepository: KiiChain/kiichain
Length of output: 120
🏁 Script executed:
# Read the complete slash.go file
cat -n x/oracle/keeper/slash.goRepository: KiiChain/kiichain
Length of output: 6558
🏁 Script executed:
# Search for where slash functions are called to understand the flow
rg "slash\(" --type go x/oracle -A 3 -B 3Repository: KiiChain/kiichain
Length of output: 43
🏁 Script executed:
# Check the staking keeper interface to understand what Slash does
rg "interface.*StakingKeeper|Slash.*func" --type go -A 5 | head -100Repository: KiiChain/kiichain
Length of output: 2081
🏁 Script executed:
# Look at the expected_keepers.go to see what methods are expected
cat x/oracle/types/expected_keepers.goRepository: KiiChain/kiichain
Length of output: 3866
🏁 Script executed:
# Search for any usage of the actual Slash method from staking keeper
rg "\.Slash\(" --type go -B 3 -A 3Repository: KiiChain/kiichain
Length of output: 43
🏁 Script executed:
# Look for BeforeValidatorSlashed hook registration
rg "BeforeValidatorSlashed|Hooks" --type go x/oracle -A 2 -B 2Repository: KiiChain/kiichain
Length of output: 429
🏁 Script executed:
# Check if there are any tests for slashToPool to understand the expected behavior
cat -n x/oracle/keeper/slash_test.go 2>/dev/null | head -100Repository: KiiChain/kiichain
Length of output: 4254
🏁 Script executed:
# Look for staking keeper imports to understand what methods are available
rg "staking" --type go x/oracle/keeper -i -lRepository: KiiChain/kiichain
Length of output: 298
🏁 Script executed:
# Search for any references to unbonding or redelegation handling
rg "unbonding|Unbonding|redelegation|Redelegation" --type go x/oracleRepository: KiiChain/kiichain
Length of output: 43
🏁 Script executed:
# Look at the full slash_test.go to understand what's being tested
cat -n x/oracle/keeper/slash_test.goRepository: KiiChain/kiichain
Length of output: 25621
🏁 Script executed:
# Search for any slash event emission or distribution hooks in oracle keeper
rg "slash|event|Event" --type go x/oracle/keeper/slash.go -iRepository: KiiChain/kiichain
Length of output: 1716
🏁 Script executed:
# Check if there's any direct usage of staking.Keeper.Slash elsewhere in the codebase
rg "staking\..*Slash|keeper\.Slash" --type go -iRepository: KiiChain/kiichain
Length of output: 1337
🏁 Script executed:
# Look at the Cosmos SDK documentation by checking what staking.Keeper.Slash method signature should be
# Check if there are any imports or references that show what Slash should do
rg "import.*staking" --type go x/oracleRepository: KiiChain/kiichain
Length of output: 43
🏁 Script executed:
# Check if there's any infraction height tracking or unbonding handling
rg "unbonding|InfractionHeight|Fraction" --type go x/oracle/keeperRepository: KiiChain/kiichain
Length of output: 1572
🏁 Script executed:
# Look for distribution slash event emission elsewhere
rg "EventTypeSlash|slash.*event|Event.*Slash" --type go -iRepository: KiiChain/kiichain
Length of output: 320
🏁 Script executed:
# Check the app keepers configuration to see how distribution hooks are set up
cat -n app/keepers/keepers.go | head -300Repository: KiiChain/kiichain
Length of output: 13984
🏁 Script executed:
# Check if there are any other slash implementations or references in the staking keeper interface
rg "Slash\(" --type go -B 2 -A 5 | head -80Repository: KiiChain/kiichain
Length of output: 825
🏁 Script executed:
# Look for any hook registration related to oracle or validation
rg "SetHooks|Hooks\(\)|BeforeValidator" --type go app/keepers/keepers.go -iRepository: KiiChain/kiichain
Length of output: 255
🏁 Script executed:
# Search for any distribution slash event handling
rg "ValidatorSlashEvent|SlashEvent" --type goRepository: KiiChain/kiichain
Length of output: 126
🏁 Script executed:
# Search for any actual usage of StakingKeeper.Slash method in the codebase
rg "StakingKeeper\.Slash|\.Slash\(" --type go -B 3 -A 5Repository: KiiChain/kiichain
Length of output: 791
🏁 Script executed:
# Check the Cosmos SDK version to understand what Slash method does
rg "cosmos-sdk" go.mod | head -5Repository: KiiChain/kiichain
Length of output: 239
🏁 Script executed:
# Look at the test helper to see the context of distribution hooks
cat -n x/oracle/keeper/test_utils.go | head -100Repository: KiiChain/kiichain
Length of output: 4397
🏁 Script executed:
# Look for the Cosmos SDK Slash method signature in go.mod dependencies
grep -A 5 "cosmos-sdk" go.modRepository: KiiChain/kiichain
Length of output: 876
🏁 Script executed:
# Check if there's any BeforeValidatorSlashed hook in the distribution keeper
rg "BeforeValidatorSlashed" --type go -B 3 -A 3Repository: KiiChain/kiichain
Length of output: 43
🏁 Script executed:
# Look for any unbonding delegation concepts in oracle
rg "unbond|Unbond" --type go x/oracleRepository: KiiChain/kiichain
Length of output: 219
🏁 Script executed:
# Examine the test that explicitly mentions previous behavior used StakingKeeper.Slash
sed -n '242,250p' x/oracle/keeper/slash_test.goRepository: KiiChain/kiichain
Length of output: 549
🌐 Web query:
cosmos-sdk staking keeper Slash method infraction height unbonding
💡 Result:
The Cosmos SDK Staking Keeper's Slash method is used by the Slashing module to penalize validators for infractions like downtime or double-signing. It is called from the slashing keeper's Slash or SlashWithInfractionReason methods. Signature (from pkg.go.dev docs and source references): func (k Keeper) Slash(ctx context.Context, consAddr sdk.ConsAddress, fraction sdkmath.LegacyDec, power int64, ...) error More precisely, the staking Slash method takes parameters including: - validator consensus address - slash fraction (e.g., SlashFractionDowntime or SlashFractionDoubleSign) - validator power at time of infraction - infraction height (int64 block height where infraction occurred) The infraction height is critical for handling unbonding delegations. When slashing: 1. Slash bonded tokens from the validator's pool (BondedPool or NotBondedPool). 2. Iterate over all unbonding delegations (UBDs) from the validator. For each UBD entry: - If the unbonding completion time is after the infraction height + unbonding period (i.e., UBD started after infraction), slash proportionally from InitialBalance. - Mature/completed UBDs (IsMature && !OnHold) are skipped. 3. Similarly slash ongoing redelegations (pseudo-unbonding) that started after the infraction. 4. Burn slashed tokens, reducing total supply. This ensures only stake "at risk" at infraction time is slashed, using historical state via infraction height to filter UBDs/redelegs. Examples: - Liveness slash: distributionHeight = ctx.BlockHeight - sdk.ValidatorUpdateDelay - 1 - Evidence-based (double-sign): exact infraction height from evidence. Source: Official Cosmos SDK GitHub (x/staking/keeper/slash.go), docs.cosmos.network (staking/slashing modules), pkg.go.dev/cosmossdk.io/x/staking, interchain-security ADR on equivocation slashing logic.
Citations:
- 1: https://github.com/cosmos/cosmos-sdk/blob/master/x/staking/keeper/slash.go
- 2: https://pkg.go.dev/cosmossdk.io/x/slashing/keeper
- 3: https://github.com/cosmos/cosmos-sdk/blob/master/x/slashing/keeper/infractions.go
- 4: https://pkg.go.dev/cosmossdk.io/x/slashing
- 5: https://cosmos.github.io/interchain-security/adrs/adr-013-equivocation-slashing
- 6: https://docs.cosmos.network/main/build/modules/staking
- 7: https://docs.cosmos.network/main/build/modules/slashing
Use the full staking.Keeper.Slash semantics, only swapping the burn destination to preserve supply.
The helper at lines 107–153 sidesteps the SDK's Slash path in favor of a simplified token removal and community pool redirection. While supply preservation is valuable, this loses critical semantic guarantees:
- No infraction-height slashing: The SDK's Slash method uses an infraction height to filter which unbonding delegations and redelegations to penalize. Without it, no UBDs/redelegations are slashed at all, allowing validators to move stake into unbonding queues before the slashing window closes and escape the penalty entirely.
- No hook invocation: The
BeforeValidatorSlashedhook is never fired, so x/distribution never records the slash event. - Incomplete slash semantics: The effective slash fraction is not calculated, and mature/completed unbonding entries are not properly skipped.
Instead of reimplementing this logic, call StakingKeeper.Slash with the correct infraction height and only replace the final token-burn step with a transfer to the community pool. This preserves both total supply and the full slash semantics that prevent validators from escaping slashing via unbonding or redelegation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@x/oracle/keeper/slash.go` around lines 107 - 153, The current slashToPool
helper bypasses staking.Keeper.Slash and therefore loses infraction-height
filtering, hook invocation, and correct effective-slash semantics; update
slashToPool to call k.StakingKeeper.Slash(ctx, consAddr, infractionHeight,
slashAmountConsensusPower, slashFactor) (using the same infraction height used
by the module when detecting the infraction) to apply all staking semantics,
then intercept/replace the final burn step by transferring the burned amount to
the community pool via k.distrKeeper.FundCommunityPool instead of leaving it
burned; ensure you no longer call RemoveValidatorTokens directly for slashing
(so BeforeValidatorSlashed and UBD/redelegation filtering run) and keep logging
the redirected amount.
There was a problem hiding this comment.
Check point 1 of this, it might be wroth
There was a problem hiding this comment.
🧩 Analysis chain
Let me check the original implementation to understand what was previously slashed (bonded tokens only vs. UBDs/redelegations) and how significant point 1 is.
🏁 Script executed:
#!/bin/bash
# Check git log for the original slash.go before this PR
git log --oneline x/oracle/keeper/slash.go | head -10
# Check git diff to see the original implementation
git show HEAD~3:x/oracle/keeper/slash.go 2>/dev/null || git show HEAD~2:x/oracle/keeper/slash.go 2>/dev/null || git show HEAD~1:x/oracle/keeper/slash.go 2>/dev/null
Length of output: 5860
🏁 Script executed:
#!/bin/bash
# The PR branch has these commits - let's go back to the base commit before this PR
# Looking at the commit order, the first PR commit is 2bf4a8d
# Let's check the commit just before that
git show 2bf4a8d^:x/oracle/keeper/slash.go 2>/dev/null | head -80
echo "---SEPARATOR---"
# Also check original expected_keepers.go before this PR
git show 2bf4a8d^:x/oracle/types/expected_keepers.go 2>/dev/null
Length of output: 5872
@Thaleszh, yes — point 1 is worth addressing, though the severity depends on your slash fraction. Here's the concrete breakdown:
What the original code did:
distributionHeight := height - sdk.ValidatorUpdateDelay - 1
k.StakingKeeper.Slash(ctx, consAddr, distributionHeight, consensusPower, slashFraction)The SDK's Slash() not only reduces bonded tokens but also iterates all unbonding delegations and redelegations that began after distributionHeight and slashes them proportionally.
What the new code does:
RemoveValidatorTokens only reduces the validator's currently-bonded stake. Any unbonding queue entries escape the penalty entirely.
The exploit path:
- A validator notices their oracle participation is low within the window.
- They initiate unbonding of a significant portion of their delegation.
- When the slash window closes,
slashToPoolonly sees their reduced bonded stake — the unbonding tokens are not touched.
Practical severity:
- If
SlashFractionis small (e.g., 0.1%), the economic incentive to game this is low. - If the slash window is long (days), the exploit is observable but requires deliberate action.
- The original
distributionHeightwindow wasValidatorUpdateDelay + 1blocks (~7–8 blocks on most Cosmos chains), so only very recent UBDs were caught anyway — limiting the practical scope.
Suggested fix approach:
The cleanest fix without fully reimplementing Slash() is to also slash UBDs in slashToPool. But since you want to redirect to community pool instead of burn, you need to iterate UBDs manually. A simpler middle ground is to add UBD slashing alongside the bonded slash, redirecting all burned coins to the pool:
// After RemoveValidatorTokens, also slash UBDs started after distributionHeight
distributionHeight := sdk.Context.BlockHeight() - sdk.ValidatorUpdateDelay - 1
// Iterate unbonding delegations from this validator
unbondingDels, err := k.StakingKeeper.GetUnbondingDelegationsFromValidator(ctx, valAddr)
for _, ubd := range unbondingDels {
for i, entry := range ubd.Entries {
if entry.IsMature(ctx.BlockTime()) {
continue // skip already matured entries
}
if entry.CreationHeight < distributionHeight {
continue // only slash entries started after infraction height
}
slashAmountUBD := slashFactor.MulInt(entry.InitialBalance).TruncateInt()
slashAmountUBD = math.MinInt(slashAmountUBD, entry.Balance)
ubd.Entries[i].Balance = entry.Balance.Sub(slashAmountUBD)
totalSlashed = totalSlashed.Add(slashAmountUBD)
// ... accumulate coins and send to community pool
}
}Bottom line: If your oracle slash fraction is low and gaming it requires deliberate and observable action by validators, you could defer this. But if you want full parity with original semantics (which protected against this escape hatch), it's worth adding UBD slashing with the funds redirected to the community pool instead of being burned. Would you like me to open a follow-up issue to track this?
There was a problem hiding this comment.
🧹 Nitpick comments (1)
x/tokenfactory/keeper/createdenom_test.go (1)
61-97: Consider addingsuite.SetupTest()for test isolation.The test logic is correct and the assertions properly verify that:
- Community pool increases by exactly the fee amount
- Total supply remains unchanged (no burn)
However, unlike the table-driven
TestCreateDenom(which callssuite.SetupTest()in its loop at line 163), this test doesn't reset state. If tests run in a specific order, residual state fromTestMsgCreateDenom(which creates denoms and modifies balances) could affect this test's assertions or cause unexpected behavior.Suggested improvement for test isolation
func (suite *KeeperTestSuite) TestCreateDenomCommunityPoolFunding() { + suite.SetupTest() + tokenFactoryKeeper := suite.App.TokenFactoryKeeper bankKeeper := suite.App.BankKeeper distrKeeper := suite.App.DistrKeeper🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@x/tokenfactory/keeper/createdenom_test.go` around lines 61 - 97, This test lacks isolation; call suite.SetupTest() at the start of TestCreateDenomCommunityPoolFunding (before reading tokenFactoryKeeper, bankKeeper, distrKeeper) to reset chain state so prior tests like TestMsgCreateDenom won't leak state; mirror how TestCreateDenom invokes suite.SetupTest() in its loop to ensure the fee, community pool and supply assertions run against a fresh context.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@x/tokenfactory/keeper/createdenom_test.go`:
- Around line 61-97: This test lacks isolation; call suite.SetupTest() at the
start of TestCreateDenomCommunityPoolFunding (before reading tokenFactoryKeeper,
bankKeeper, distrKeeper) to reset chain state so prior tests like
TestMsgCreateDenom won't leak state; mirror how TestCreateDenom invokes
suite.SetupTest() in its loop to ensure the fee, community pool and supply
assertions run against a fresh context.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5c6b6a7f-4fed-4fa7-a11a-8f8eec37e215
📒 Files selected for processing (1)
x/tokenfactory/keeper/createdenom_test.go
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
app/keepers/slash_hooks_test.go (1)
82-106: Please add a non-happy-path slash case here.A freshly created validator with a 5% factor round-trips cleanly, so this test will not catch drift between the hook’s reconstructed mint amount and the actual
slashedvalue. Add a case where the effective ratio is not exactly representable or current tokens differ from the slash basis, and assert the community-pool delta matchesslashed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/keepers/slash_hooks_test.go` around lines 82 - 106, Add a non-happy-path slash case that creates a rounding/mismatch between the hook's reconstructed mint and the actual slashed amount: after createBondedValidator (use createBondedValidator, valAddr, consAddr, stakedAmount) record supplyBefore and feePool before the slash, then mutate the validator's token basis so it is not an exact consensus-power multiple (for example by moving some tokens from the validator's account via BankKeeper.SendCoins or adjusting balances), choose a slashFactor that produces non-representable division (e.g., a percentile that causes rounding), call app.StakingKeeper.Slash(ctx, consAddr, 0, consensusPower, slashFactor) and then assert that (1) supply is unchanged (compare supplyBefore to GetSupply) and (2) the community-pool delta (new FeePool.CommunityPool - feePool before) equals math.LegacyNewDecFromInt(slashed) rather than relying on exact equality to a precomputed value so the test catches rounding/drift between Slash and the hook.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/keepers/slash_hooks.go`:
- Around line 47-75: The new BeforeValidatorSlashed hook in SlashHooks
(registered via NewSlashHooks) unconditionally mints and forwards slashed tokens
for every staking slash; restrict the change so it only affects oracle-triggered
slashes: either move the mint+FundCommunityPool logic out of
BeforeValidatorSlashed and into the oracle-specific flow in
x/oracle/keeper/slash.go, or add a guard in BeforeValidatorSlashed (checking an
explicit flag/marker passed from oracle slashing or an enum on the slash call)
so downtime/double-sign slashes are unchanged; update NewSlashHooks registration
usage in app/keepers/keepers.go accordingly to ensure only oracle slashes
trigger this behavior.
- Around line 53-55: The mint amount is being recomputed from validator.Tokens
and fraction which causes rounding/drift; instead use the exact tokensToBurn
computed during slashing (or pass it into this hook) rather than reconstructing
it. Update the caller (slashToPool / whatever invokes this hook) to provide the
exact tokensToBurn (sdk.Int) and change this code path to use that value for
mintAmount (replace the
math.LegacyNewDecFromInt(validator.Tokens).Mul(fraction).TruncateInt() logic),
ensuring the hook signature and any callers use the precise tokens burned to
avoid under-minting and drift.
---
Nitpick comments:
In `@app/keepers/slash_hooks_test.go`:
- Around line 82-106: Add a non-happy-path slash case that creates a
rounding/mismatch between the hook's reconstructed mint and the actual slashed
amount: after createBondedValidator (use createBondedValidator, valAddr,
consAddr, stakedAmount) record supplyBefore and feePool before the slash, then
mutate the validator's token basis so it is not an exact consensus-power
multiple (for example by moving some tokens from the validator's account via
BankKeeper.SendCoins or adjusting balances), choose a slashFactor that produces
non-representable division (e.g., a percentile that causes rounding), call
app.StakingKeeper.Slash(ctx, consAddr, 0, consensusPower, slashFactor) and then
assert that (1) supply is unchanged (compare supplyBefore to GetSupply) and (2)
the community-pool delta (new FeePool.CommunityPool - feePool before) equals
math.LegacyNewDecFromInt(slashed) rather than relying on exact equality to a
precomputed value so the test catches rounding/drift between Slash and the hook.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ad4b3e9d-fbaa-4dd6-81bd-ec5be2ece87e
📒 Files selected for processing (4)
CHANGELOG.mdapp/keepers/keepers.goapp/keepers/slash_hooks.goapp/keepers/slash_hooks_test.go
✅ Files skipped from review due to trivial changes (1)
- CHANGELOG.md
🚧 Files skipped from review as they are similar to previous changes (1)
- app/keepers/keepers.go
| // The effectiveFraction passed by slashToPool is tokensToBurn / validator.Tokens, | ||
| // so multiplying back recovers tokensToBurn | ||
| mintAmount := math.LegacyNewDecFromInt(validator.Tokens).Mul(fraction).TruncateInt() |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In cosmos-sdk v0.53.6, what exact value does x/staking/keeper/slash.gopass as thefractionargument toStakingHooks.BeforeValidatorSlashed? Please cite the implementation and clarify whether it is the raw slash factor or an effective post-clamp fraction.
💡 Result:
In cosmos-sdk v0.53.6, the x/staking/keeper/slash.go file passes the raw slash factor (the unclamped fraction parameter received by the Slash function) as the fraction argument to StakingHooks.BeforeValidatorSlashed. This occurs before any clamping of the slash amount to ensure the minimum balance is maintained. The hook receives the intended slash fraction prior to applying clamps in the staking keeper's Slash logic.
Citations:
- 1: https://github.com/cosmos/cosmos-sdk/blob/master/x/staking/keeper/slash.go
- 2: perf(x/staking/keeper): make Slash return early iff zero tokens to burn cosmos/cosmos-sdk#18049
- 3: https://pkg.go.dev/cosmossdk.io/x/slashing/keeper
- 4: https://pkg.go.dev/github.com/cosmos/cosmos-sdk/x/staking/types
🏁 Script executed:
# Find the BeforeValidatorSlashed hook implementation in the repo
find . -type f -name "*.go" | xargs grep -l "BeforeValidatorSlashed" | head -20Repository: KiiChain/kiichain
Length of output: 1885
🏁 Script executed:
# Check the slash_hooks.go file to understand the full context
cat -n app/keepers/slash_hooks.go | head -70Repository: KiiChain/kiichain
Length of output: 1885
🏁 Script executed:
# Search for where slash amounts or tokensToBurn are computed in the staking context
rg -A 5 -B 5 "tokensToBurn|slash.*amount" app/keepers/slash_hooks.goRepository: KiiChain/kiichain
Length of output: 1885
Reconstruct the exact slash amount instead of deriving it from the current validator tokens.
The fraction parameter is the raw slash factor (before clamping). Multiplying it against validator.Tokens and converting through decimals introduces rounding loss that can under-mint, and delayed evidence execution where validator tokens differ from the infraction-power basis will further drift from the actual amount slashed. Pass or compute the exact tokensToBurn value from the slash execution path instead.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/keepers/slash_hooks.go` around lines 53 - 55, The mint amount is being
recomputed from validator.Tokens and fraction which causes rounding/drift;
instead use the exact tokensToBurn computed during slashing (or pass it into
this hook) rather than reconstructing it. Update the caller (slashToPool /
whatever invokes this hook) to provide the exact tokensToBurn (sdk.Int) and
change this code path to use that value for mintAmount (replace the
math.LegacyNewDecFromInt(validator.Tokens).Mul(fraction).TruncateInt() logic),
ensuring the hook signature and any callers use the precise tokens burned to
avoid under-minting and drift.
Thaleszh
changed the title
1 participant
Description
Instead of burning on slash, we will send it to the community pool
Applied this directly on oracle, used a hook for everything else.
Added tests for all, including tokenfactory
Type of change
How Has This Been Tested?
PR Checklist:
Make sure each step was done:
make lint-fix