Core fix: do not emit TokenAndETHShift for zero amounts, no zero transfer #2135
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Caution Review failedThe pull request is closed. WalkthroughAdded guards in KlerosCore to skip PNK/ETH transfers, staking, bookkeeping, and TokenAndETHShift emission when corresponding amounts are zero; updated tests and integration assertions to stop expecting zero-value TokenAndETHShift events and to assert precise event args. Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant Core as KlerosCore
participant SM as SortitionModule
participant PNK as PNK Token
U->>Core: execute(disputeID, ...)
activate Core
rect rgb(235,245,255)
note right of Core: Penalties flow (guarded)
Core->>Core: _executePenalties()
alt availablePenalty > 0
Core->>SM: update pnkPenaltiesInRound
Core-->>U: emit TokenAndETHShift (penalty)
else
note over Core: Skip updates and event
end
end
rect rgb(240,255,240)
note right of Core: Rewards flow (guarded)
Core->>Core: _executeRewards()
alt feeReward > 0
Core-->>U: transfer fee reward
end
alt pnkReward > 0
Core->>PNK: stake PNK reward
end
opt (feeReward > 0) or (pnkReward > 0)
Core-->>U: emit TokenAndETHShift (reward)
end
end
deactivate Core
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
✨ Finishing Touches🧪 Generate unit tests
Comment |
![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
🧹 Nitpick comments (4)
contracts/src/arbitration/KlerosCore.sol (2)
914-941: Zero-penalty: short-circuit to skip external calls and event; keeps semantics, saves gasGood guard on emitting only when
availablePenalty != 0. You can also early-return whenpenalty == 0to avoid an external call tounlockStake()and bypasssetStakePenaltyentirely for fully coherent jurors.Apply:
// Fully coherent jurors won't be penalized. uint256 penalty = (round.pnkAtStakePerJuror * (ONE_BASIS_POINT - coherence)) / ONE_BASIS_POINT; - // Unlock the PNKs affected by the penalty - address account = round.drawnJurors[_params.repartition]; - sortitionModule.unlockStake(account, penalty); + // Nothing to penalize? Skip external calls and event emission. + if (penalty == 0) { + return _params.pnkPenaltiesInRound; + } + + // Unlock the PNKs affected by the penalty + address account = round.drawnJurors[_params.repartition]; + sortitionModule.unlockStake(account, penalty);
989-1011: Reward path gas nits: skip unlock when zero and avoid += 0 SSTOREsThe new zero-amount guards look good. Two tiny gas wins:
- Only call
unlockStakewhenpnkLocked != 0.- Avoid
+= 0writes tosumPnkRewardPaid/sumFeeRewardPaid.Apply:
- // Release the rest of the PNKs of the juror for this round. - sortitionModule.unlockStake(account, pnkLocked); + // Release the rest of the PNKs of the juror for this round. + if (pnkLocked != 0) { + sortitionModule.unlockStake(account, pnkLocked); + } // Compute the rewards uint256 pnkReward = _applyCoherence(_params.pnkPenaltiesInRound / _params.coherentCount, pnkCoherence); - round.sumPnkRewardPaid += pnkReward; + if (pnkReward != 0) { + round.sumPnkRewardPaid += pnkReward; + } uint256 feeReward = _applyCoherence(round.totalFeesForJurors / _params.coherentCount, feeCoherence); - round.sumFeeRewardPaid += feeReward; + if (feeReward != 0) { + round.sumFeeRewardPaid += feeReward; + }contracts/test/integration/index.ts (2)
134-151: Assert event payload to prevent silent regressionsNice swap to asserting
DisputeRequest. Consider asserting critical args withwithArgs(...)(useanyValuefor non-deterministic ones) so changes in ordering/IDs don’t slip by.If you share
HomeGateway.DisputeRequestsignature, I can propose an exactwithArgs(...)snippet.
189-197: Good: positive assertion on non-zero TokenAndETHShift; also assert absence of zero-value shiftsYou can additionally verify that no
TokenAndETHShiftwith both amounts equal to zero was emitted.Apply:
- await expect(core.execute(0, 0, 1000)) + const execTx = await core.execute(0, 0, 1000); + await expect(execTx) .to.emit(core, "TokenAndETHShift") .withArgs(deployer, 0, 0, 10000, 10000, 0, arbitrationCost / 3n, ethers.ZeroAddress); + + // Ensure no zero-amount TokenAndETHShift slipped through. + const rc = await execTx.wait(); + const parsed = rc.logs + .map((l) => { + try { return core.interface.parseLog(l); } catch { return null; } + }) + .filter((x) => x && x.name === "TokenAndETHShift"); + expect(parsed.every((ev: any) => ev.args._amountPnk !== 0n || ev.args._amountFee !== 0n)).to.equal(true);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
contracts/src/arbitration/KlerosCore.sol(2 hunks)contracts/test/foundry/KlerosCore_Execution.t.sol(0 hunks)contracts/test/integration/index.ts(2 hunks)
💤 Files with no reviewable changes (1)
- contracts/test/foundry/KlerosCore_Execution.t.sol
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: jaybuidl
PR: kleros/kleros-v2#0
File: :0-0
Timestamp: 2025-09-03T22:48:32.972Z
Learning: In the Kleros v2 codebase, the team prioritizes gas optimization over strict CEI pattern compliance when dealing with trusted contracts. For penalty execution logic, they prefer batching storage writes (`round.pnkPenalties`) rather than updating incrementally after each penalty calculation to save gas costs, as the risk is extremely low between trusted contracts.
Learnt from: jaybuidl
PR: kleros/kleros-v2#2126
File: contracts/src/arbitration/KlerosCore.sol:472-489
Timestamp: 2025-09-04T23:36:16.415Z
Learning: In this repo, KlerosCore emits AcceptedFeeToken and NewCurrencyRate events that are declared in contracts/src/arbitration/interfaces/IArbitratorV2.sol; implementations don’t need to redeclare these events.
📚 Learning: 2025-09-03T22:48:32.972Z
Learnt from: jaybuidl
PR: kleros/kleros-v2#0
File: :0-0
Timestamp: 2025-09-03T22:48:32.972Z
Learning: In the Kleros v2 codebase, the team prioritizes gas optimization over strict CEI pattern compliance when dealing with trusted contracts. For penalty execution logic, they prefer batching storage writes (`round.pnkPenalties`) rather than updating incrementally after each penalty calculation to save gas costs, as the risk is extremely low between trusted contracts.
Applied to files:
contracts/src/arbitration/KlerosCore.sol
📚 Learning: 2025-09-04T23:36:16.415Z
Learnt from: jaybuidl
PR: kleros/kleros-v2#2126
File: contracts/src/arbitration/KlerosCore.sol:472-489
Timestamp: 2025-09-04T23:36:16.415Z
Learning: In this repo, KlerosCore emits AcceptedFeeToken and NewCurrencyRate events that are declared in contracts/src/arbitration/interfaces/IArbitratorV2.sol; implementations don’t need to redeclare these events.
Applied to files:
contracts/src/arbitration/KlerosCore.sol
⏰ 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: SonarCloud
unknownunknown1
previously approved these changes
Sep 10, 2025
jaybuidl
dismissed stale reviews from unknownunknown1 and coderabbitai[bot]
The base branch was changed.
jaybuidl
force-pushed
the
fix/no-zero-amount-shift
branch
from
801e42e to
e7578d2
Compare
jaybuidl
force-pushed
the
fix/no-zero-amount-shift
branch
from
878bf28 to
bd983ca
Compare
|
✅ Deploy Preview for kleros-v2-testnet ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
❌ Deploy Preview for kleros-v2-university failed. Why did it fail? →
|
✅ Deploy Preview for kleros-v2-neo ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
❌ Deploy Preview for kleros-v2-testnet-devtools failed. Why did it fail? →
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
PR-Codex overview
This PR focuses on enhancing the
KlerosCorecontract by preventing the emission of events and transfers when amounts are zero. It improves the handling of penalties and rewards in the system, ensuring that unnecessary transactions are avoided.Detailed summary
CHANGELOG.mdto reflect changes in event emissions and transfers.KlerosCore.TokenAndETHShiftwhen amounts are zero inKlerosCore_Execution.t.sol.KlerosCore.solto only emitTokenAndETHShiftwhen penalties or rewards are non-zero.Summary by CodeRabbit