feat: replace requires with custom errors #2084
Conversation
✅ 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. |
|
Caution Review failedThe pull request is closed. WalkthroughStandardizes revert handling across many contracts by replacing string-based require checks with custom error reverts and adding numerous new error declarations. Tests updated to assert error selectors. No public function signatures changed. Changes
Sequence Diagram(s)(omitted — changes are limited to error-type substitutions and do not introduce new control-flow interactions) Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
✨ 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 (
|
✅ Deploy Preview for kleros-v2-testnet-devtools ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
![coderabbitai[bot]](https://app.smartcontext.ai/default/https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (6)
contracts/src/arbitration/PolicyRegistry.sol (1)
89-94: Consider centralizing governor guard to avoid duplication across contracts.Given multiple contracts now declare GovernorOnly and an identical onlyByGovernor guard, consider a small base “Governable” mixin that provides:
- governor storage
- onlyByGovernor modifier
- GovernorOnly error
This will reduce duplication and deployment size, and keep access control consistent across the codebase.
If you’d like, I can draft a lightweight Governable.sol base and a minimal migration plan.
contracts/src/arbitration/DisputeTemplateRegistry.sol (1)
88-89: Reduce repetition by sharing governor guard + error across contracts.Same note as in PolicyRegistry: a shared Governable base (governor storage, onlyByGovernor, GovernorOnly) would keep these in one place.
contracts/src/arbitration/view/KlerosCoreSnapshotProxy.sol (1)
77-78: Optional: centralize errors and modifiers.Same suggestion as other files—extract a tiny Governable to avoid repeating GovernorOnly and onlyByGovernor.
contracts/src/arbitration/evidence/EvidenceModule.sol (1)
75-76: Optional: centralize the governor error and modifier.Consider using a shared Governable base to avoid repeating the same pattern.
contracts/test/arbitration/staking-neo.ts (2)
266-269: Use the canonical Hardhat matcher chain (.to.be.revertedWithCustomError).For consistency with the rest of the test suite and Hardhat docs, prefer
.to.be.revertedWithCustomError(...)over.to.revertedWithCustomError(...).Apply:
- await expect(sortition.executeDelayedStakes(10)).to.revertedWithCustomError( + await expect(sortition.executeDelayedStakes(10)).to.be.revertedWithCustomError( sortition, "NoDelayedStakeToExecute" );
333-336: Use the canonical Hardhat matcher chain (.to.be.revertedWithCustomError).Same here; align with
.to.be.revertedWithCustomError(...).Apply:
- await expect(sortition.executeDelayedStakes(10)).to.revertedWithCustomError( + await expect(sortition.executeDelayedStakes(10)).to.be.revertedWithCustomError( sortition, "NoDelayedStakeToExecute" );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
contracts/src/arbitration/DisputeTemplateRegistry.sol(2 hunks)contracts/src/arbitration/KlerosGovernor.sol(8 hunks)contracts/src/arbitration/PolicyRegistry.sol(2 hunks)contracts/src/arbitration/SortitionModuleBase.sol(6 hunks)contracts/src/arbitration/arbitrables/ArbitrableExample.sol(3 hunks)contracts/src/arbitration/arbitrables/DisputeResolver.sol(4 hunks)contracts/src/arbitration/devtools/DisputeResolverRuler.sol(1 hunks)contracts/src/arbitration/dispute-kits/DisputeKitClassicBase.sol(8 hunks)contracts/src/arbitration/evidence/EvidenceModule.sol(2 hunks)contracts/src/arbitration/university/SortitionModuleUniversity.sol(3 hunks)contracts/src/arbitration/view/KlerosCoreSnapshotProxy.sol(2 hunks)contracts/test/arbitration/index.ts(1 hunks)contracts/test/arbitration/staking-neo.ts(2 hunks)contracts/test/foundry/KlerosCore.t.sol(16 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). (15)
- GitHub Check: Redirect rules - kleros-v2-neo
- GitHub Check: Redirect rules - kleros-v2-university
- GitHub Check: Redirect rules - kleros-v2-testnet
- GitHub Check: Redirect rules - kleros-v2-testnet
- GitHub Check: Header rules - kleros-v2-neo
- GitHub Check: Header rules - kleros-v2-testnet
- GitHub Check: Header rules - kleros-v2-university
- GitHub Check: Header rules - kleros-v2-testnet
- GitHub Check: Pages changed - kleros-v2-neo
- GitHub Check: Pages changed - kleros-v2-testnet
- GitHub Check: Pages changed - kleros-v2-university
- GitHub Check: Pages changed - kleros-v2-testnet
- GitHub Check: contracts-testing
- GitHub Check: SonarCloud
- GitHub Check: Analyze (javascript)
🔇 Additional comments (38)
contracts/src/arbitration/PolicyRegistry.sol (1)
35-37: Switch to custom error improves gas and consistency.Using revert GovernorOnly() over string-based require aligns with the PR’s objective and reduces bytecode size.
contracts/src/arbitration/DisputeTemplateRegistry.sol (1)
28-30: Custom error-based governor guard is correct.The modifier now reverts with GovernorOnly(), which is the intended migration and is gas-friendlier than strings.
contracts/src/arbitration/view/KlerosCoreSnapshotProxy.sol (1)
29-31: Good: Governor check migrated to typed error.The modifier now uses revert GovernorOnly(), matching the new standard and improving gas efficiency.
contracts/src/arbitration/evidence/EvidenceModule.sol (1)
25-27: Typed error migration looks good.onlyByGovernor now reverts GovernorOnly(), aligning with the migration and saving gas.
contracts/src/arbitration/arbitrables/DisputeResolver.sol (1)
133-134: Custom error for ruling options check is correct.ShouldBeAtLeastTwoRulingOptions() precisely captures the invariant and is preferable to string-based require.
contracts/src/arbitration/devtools/DisputeResolverRuler.sol (1)
39-39: LGTM! Custom error properly implements the validation.The replacement of the require statement with a custom error
ShouldBeAtLeastTwoRulingOptions()is correct and maintains the same validation logic while improving gas efficiency.contracts/src/arbitration/arbitrables/ArbitrableExample.sol (3)
40-40: LGTM! Access control properly enforced with custom error.The governor-only modifier correctly reverts with the
GovernorOnly()custom error, maintaining the same access control semantics.
145-147: LGTM! Rule validation properly implemented with custom errors.The sequential validation checks with custom errors provide clear and specific error messages for each failure condition.
159-164: LGTM! Well-organized error declarations.The custom errors are properly declared and follow a logical organization.
contracts/src/arbitration/SortitionModuleBase.sol (7)
125-126: LGTM! Access control modifiers properly updated.Both
onlyByGovernorandonlyByCoremodifiers correctly use custom errors for access control validation.
174-186: LGTM! Phase transition validations properly implemented.The phase transition logic correctly uses custom errors for all validation conditions, providing clear error messages for each failure case.
200-201: LGTM! Tree creation validations properly implemented.The tree creation validations correctly check for existing trees and validate the K parameter with appropriate custom errors.
209-210: LGTM! Delayed stake execution validations properly implemented.The validations correctly check the phase and ensure there are delayed stakes to execute.
419-419: LGTM! Withdrawal eligibility check properly implemented.The eligibility check correctly validates that there are funds to withdraw.
443-443: LGTM! Drawing phase validation properly implemented.The draw function correctly validates that the system is in the drawing phase.
696-707: LGTM! Comprehensive error declarations.The custom errors are well-organized and cover all the validation scenarios in the contract.
contracts/src/arbitration/KlerosGovernor.sol (9)
73-73: LGTM! Period modifiers properly updated with custom errors.Both submission and approval period modifiers correctly validate timing constraints with descriptive custom errors.
Also applies to: 79-79
84-84: LGTM! Governor-only modifier properly enforces access control.The modifier correctly validates that the caller is the contract itself (governor pattern).
211-212: LGTM! Input validation properly implemented.The array length validations ensure consistency between target, value, and data size arrays.
218-218: LGTM! Deposit and duplicate submission checks properly implemented.The validations correctly ensure sufficient deposit and prevent duplicate list submissions.
Also applies to: 236-236
259-263: LGTM! Withdrawal validations properly implemented.All withdrawal conditions are properly validated with specific custom errors.
276-276: LGTM! Dispute status validation properly implemented.The check prevents multiple dispute creations for the same session.
313-315: LGTM! Rule function validations properly implemented.The validations correctly ensure only the arbitrator can rule on active disputes with valid rulings.
341-342: LGTM! Transaction execution validations properly implemented.The validations ensure only approved submissions can be executed within the timeout period, with re-entrancy protection.
Also applies to: 350-350
415-432: LGTM! Comprehensive error declarations.All custom errors are well-organized and provide clear, specific error messages for each validation scenario.
contracts/src/arbitration/dispute-kits/DisputeKitClassicBase.sol (8)
128-128: LGTM! Access control modifiers properly updated.Both governor and core access control modifiers correctly use custom errors.
Also applies to: 133-133
138-138: LGTM! Dispute jump validation properly implemented.The modifier correctly prevents operations on disputes that have jumped to parent DKs.
174-174: LGTM! Call success validation properly implemented.The governor proposal execution correctly validates call success.
272-274: LGTM! Commit validation properly implemented.All commit period validations are correctly implemented with specific custom errors for each failure condition.
Also applies to: 279-279
313-315: LGTM! Vote casting validations comprehensive and correct.The vote casting validations properly check period, vote ownership, hidden vote hash matching, and prevent double voting with specific custom errors.
Also applies to: 318-318, 328-331
362-363: LGTM! Appeal funding validations properly implemented.All appeal funding validations including choice bounds, dispute activity, appeal period timing, and payment status are correctly implemented with specific custom errors.
Also applies to: 366-366, 373-378, 385-385
442-444: LGTM! Withdrawal validations properly implemented.The withdrawal validations correctly check dispute resolution status, core pause state, and dispute activity.
717-734: LGTM! Comprehensive error declarations.The custom errors are well-organized and cover all validation scenarios in the dispute kit.
contracts/test/arbitration/index.ts (1)
76-78: LGTM: migrated to custom error assertion (KlerosCoreOnly).The expectation
.to.be.revertedWithCustomError(disputeKit, "KlerosCoreOnly")correctly reflects the modifier change and ensures the call path is restricted to KlerosCore.contracts/src/arbitration/university/SortitionModuleUniversity.sol (3)
69-72: LGTM: access control moved to custom errors.Replacing requires with
GovernorOnly()andKlerosCoreOnly()in modifiers cleanly aligns with the PR objective and reduces gas on revert.Also applies to: 75-77
283-287: LGTM: specific custom error for leftover withdrawal gating.The change to
if (amount == 0) revert NotEligibleForWithdrawal();is semantically equivalent to the prior require and improves gas usage and selector-based testability.
368-375: LGTM: error declarations added.Custom errors are clearly named and scoped to the contract. No conflicts observed.
contracts/test/foundry/KlerosCore.t.sol (1)
1140-1141: LGTM: migrated revert checks to error selectors throughout the suite.
- The updates consistently use
.selectoracross KlerosCoreBase, SortitionModuleBase, DisputeKitClassicBase, and KlerosCoreSnapshotProxy.- Coverage includes staking phases, delayed stake execution, snapshot proxy guards, commit/reveal voting, appeal windows, DK jumps, and leftover withdrawals.
- Assertions align with the new revert paths and preserve original intent.
No issues spotted with contract scoping or selector usage; imports provide the necessary symbols.
Also applies to: 1153-1154, 1273-1273, 1280-1280, 1568-1569, 1586-1587, 1592-1593, 1630-1631, 1633-1635, 1702-1703, 1720-1721, 1726-1727, 1729-1731, 1738-1739, 1974-1975, 2028-2029, 2035-2036, 2042-2044, 2213-2214, 2597-2599, 2867-2868, 2874-2875, 2973-2974
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
contracts/src/gateway/ForeignGateway.sol (2)
106-108: Consider using theonlyByGovernormodifier for consistency.The
changeGovernorfunction duplicates the governor check instead of using the existingonlyByGovernormodifier. This creates unnecessary code duplication.Apply this diff to use the modifier:
-function changeGovernor(address _governor) external { - if (governor != msg.sender) revert GovernorOnly(); +function changeGovernor(address _governor) external onlyByGovernor { governor = _governor; }
123-125: Use theonlyByGovernormodifier for consistency.Similar to
changeGovernor, this function duplicates the governor check logic.Apply this diff:
-function changeHomeGateway(address _homeGateway) external { - if (governor != msg.sender) revert GovernorOnly(); +function changeHomeGateway(address _homeGateway) external onlyByGovernor { homeGateway = _homeGateway; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
contracts/src/gateway/ForeignGateway.sol(8 hunks)contracts/src/gateway/HomeGateway.sol(7 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). (15)
- GitHub Check: Redirect rules - kleros-v2-neo
- GitHub Check: Header rules - kleros-v2-neo
- GitHub Check: Pages changed - kleros-v2-neo
- GitHub Check: Redirect rules - kleros-v2-testnet
- GitHub Check: Redirect rules - kleros-v2-testnet
- GitHub Check: Header rules - kleros-v2-testnet
- GitHub Check: Header rules - kleros-v2-testnet
- GitHub Check: Pages changed - kleros-v2-testnet
- GitHub Check: Pages changed - kleros-v2-testnet
- GitHub Check: Redirect rules - kleros-v2-university
- GitHub Check: Header rules - kleros-v2-university
- GitHub Check: Pages changed - kleros-v2-university
- GitHub Check: SonarCloud
- GitHub Check: Analyze (javascript)
- GitHub Check: contracts-testing
🔇 Additional comments (15)
contracts/src/gateway/ForeignGateway.sol (7)
52-56: Good implementation of custom errors in the modifier.The replacement of
requirestatements with custom error reverts improves gas efficiency and provides clearer error reporting. The implementation correctly maintains the original validation logic.
60-62: Consistent error handling pattern applied.The modifier correctly uses the new
GovernorOnly()custom error, maintaining consistency with the PR's objective.
145-145: Good implementation of custom error for fee validation.The custom error
ArbitrationFeesNotEnough()provides clear feedback when arbitration fees are insufficient.
208-209: Clear and descriptive custom errors for dispute validation.The custom errors
DisputeDoesNotExist()andCannotRuleTwice()provide precise error information for dispute state validation.
221-222: Well-structured error handling for withdrawal validation.The sequential checks with custom errors provide clear feedback about why a withdrawal might fail.
249-251: Section header formatting is clean and consistent.The internal section header follows standard Solidity documentation conventions.
271-281: Well-organized custom error declarations.All custom errors are properly declared at the end of the contract following Solidity best practices. The error names are descriptive and follow a consistent naming convention.
contracts/src/gateway/HomeGateway.sol (8)
47-49: Consistent implementation of custom error in modifier.The
onlyByGovernormodifier correctly uses the newGovernorOnly()custom error, aligning with the PR's objective.
132-133: Clear custom errors for fee payment validation.The custom errors
FeesPaidInNativeCurrencyOnly()andForeignChainIDNotSupported()provide precise feedback about invalid payment methods and unsupported chains.
147-147: Good validation for preventing duplicate dispute relay.The custom error
DisputeAlreadyRelayed()clearly indicates when a dispute has already been processed.
170-171: Consistent error handling for ERC20 payment path.The validation logic correctly mirrors the native currency path with appropriate custom errors.
185-185: Consistent duplicate check across both payment methods.Good that the same validation is applied for both native and ERC20 payment paths.
187-188: Comprehensive error handling for token operations.The custom errors
TransferFailed()andAllowanceIncreaseFailed()provide clear feedback for ERC20 operation failures. The use ofSafeERC20library functions with custom error handling is appropriate.
212-212: Clear access control with custom error.The
ArbitratorOnly()custom error provides precise feedback for unauthorized rule attempts.
238-249: Well-organized and comprehensive error declarations.All custom errors are properly declared at the end of the contract. The error names are descriptive and follow consistent naming conventions, covering all validation scenarios in the contract.
PR-Codex overview
This PR focuses on replacing
require()statements withrevert()and custom error messages across various contracts for consistency and reduced bytecode size.Detailed summary
require()withifstatements followed byrevert()in several contracts.GovernorOnly,ArbitratorOnly,NotEligibleForWithdrawal, etc.Summary by CodeRabbit