Merge pull request #553 from protofire/gc-custom-errors

fix: custom-errors
protofire · Mar 5, 2024 · 02d94a8 · 02d94a8
2 parents a266ecb + e58addf
commit 02d94a8
Show file tree

Hide file tree

Showing 9 changed files with 104 additions and 32 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -3,6 +3,7 @@
 
 ### Updated
 - Rule: named-return-values rule was renamed to gas-named-return-values and now it is part of Gas Consumption ruleset
+- Rule: custom-errors rule was renamed to gas-custom-errors and now it is part of Gas Consumption ruleset
 
 
 ## [4.1.2] - 2024-02-06

diff --git a/conf/rulesets/solhint-all.js b/conf/rulesets/solhint-all.js
@@ -6,7 +6,6 @@
 module.exports = Object.freeze({
   rules: {
     'code-complexity': ['warn', 7],
-    'custom-errors': 'warn',
     'explicit-types': ['warn', 'explicit'],
     'function-max-lines': ['warn', 50],
     'max-line-length': ['error', 120],
@@ -26,6 +25,7 @@ module.exports = Object.freeze({
     ],
     'constructor-syntax': 'warn',
     'gas-calldata-parameters': 'warn',
+    'gas-custom-errors': 'warn',
     'gas-increment-by-one': 'warn',
     'gas-indexed-events': 'warn',
     'gas-multitoken1155': 'warn',

diff --git a/conf/rulesets/solhint-recommended.js b/conf/rulesets/solhint-recommended.js
@@ -5,7 +5,6 @@
 
 module.exports = Object.freeze({
   rules: {
-    'custom-errors': 'warn',
     'explicit-types': ['warn', 'explicit'],
     'max-states-count': ['warn', 15],
     'no-console': 'error',
@@ -21,6 +20,7 @@ module.exports = Object.freeze({
         maxLength: 32,
       },
     ],
+    'gas-custom-errors': 'warn',
     quotes: ['error', 'double'],
     'const-name-snakecase': 'warn',
     'contract-name-camelcase': 'warn',

diff --git a/docs/rules.md b/docs/rules.md
@@ -9,7 +9,6 @@ title:       "Rule Index of Solhint"
 | Rule Id                                                                  | Error                                                                                                                                      | Recommended  | Deprecated |
 | ------------------------------------------------------------------------ | ------------------------------------------------------------------------------------------------------------------------------------------ | ------------ | ---------- |
 | [code-complexity](./rules/best-practises/code-complexity.md)             | Function has cyclomatic complexity "current" but allowed no more than maxcompl.                                                            |              |            |
-| [custom-errors](./rules/best-practises/custom-errors.md)                 | Enforces the use of Custom Errors over Require and Revert statements                                                                       | $~~~~~~~~$✔️ |            |
 | [explicit-types](./rules/best-practises/explicit-types.md)               | Forbid or enforce explicit types (like uint256) that have an alias (like uint).                                                            | $~~~~~~~~$✔️ |            |
 | [function-max-lines](./rules/best-practises/function-max-lines.md)       | Function body contains "count" lines but allowed no more than maxlines.                                                                    |              |            |
 | [max-line-length](./rules/best-practises/max-line-length.md)             | Line length must be no more than maxlen.                                                                                                   |              |            |
@@ -27,15 +26,16 @@ title:       "Rule Index of Solhint"
 
 ## Gas Consumption Rules
 
-| Rule Id                                                                       | Error                                                             | Recommended | Deprecated |
-| ----------------------------------------------------------------------------- | ----------------------------------------------------------------- | ----------- | ---------- |
-| [gas-calldata-parameters](./rules/gas-consumption/gas-calldata-parameters.md) | Suggest calldata keyword on function arguments when read only     |             |            |
-| [gas-increment-by-one](./rules/gas-consumption/gas-increment-by-one.md)       | Suggest incrementation by one like this ++i instead of other type |             |            |
-| [gas-indexed-events](./rules/gas-consumption/gas-indexed-events.md)           | Suggest indexed arguments on events for uint, bool and address    |             |            |
-| [gas-multitoken1155](./rules/gas-consumption/gas-multitoken1155.md)           | ERC1155 is a cheaper non-fungible token than ERC721               |             |            |
-| [gas-named-return-values](./rules/gas-consumption/gas-named-return-values.md) | Enforce the return values of a function to be named               |             |            |
-| [gas-small-strings](./rules/gas-consumption/gas-small-strings.md)             | Keep strings smaller than 32 bytes                                |             |            |
-| [gas-struct-packing](./rules/gas-consumption/gas-struct-packing.md)           | Suggest to re-arrange struct packing order when it is inefficient |             |            |
+| Rule Id                                                                       | Error                                                                | Recommended  | Deprecated |
+| ----------------------------------------------------------------------------- | -------------------------------------------------------------------- | ------------ | ---------- |
+| [gas-calldata-parameters](./rules/gas-consumption/gas-calldata-parameters.md) | Suggest calldata keyword on function arguments when read only        |              |            |
+| [gas-custom-errors](./rules/gas-consumption/gas-custom-errors.md)             | Enforces the use of Custom Errors over Require and Revert statements | $~~~~~~~~$✔️ |            |
+| [gas-increment-by-one](./rules/gas-consumption/gas-increment-by-one.md)       | Suggest incrementation by one like this ++i instead of other type    |              |            |
+| [gas-indexed-events](./rules/gas-consumption/gas-indexed-events.md)           | Suggest indexed arguments on events for uint, bool and address       |              |            |
+| [gas-multitoken1155](./rules/gas-consumption/gas-multitoken1155.md)           | ERC1155 is a cheaper non-fungible token than ERC721                  |              |            |
+| [gas-named-return-values](./rules/gas-consumption/gas-named-return-values.md) | Enforce the return values of a function to be named                  |              |            |
+| [gas-small-strings](./rules/gas-consumption/gas-small-strings.md)             | Keep strings smaller than 32 bytes                                   |              |            |
+| [gas-struct-packing](./rules/gas-consumption/gas-struct-packing.md)           | Suggest to re-arrange struct packing order when it is inefficient    |              |            |
         
 
 ## Miscellaneous

diff --git a/docs/rules/gas-consumption/gas-custom-errors.md b/docs/rules/gas-consumption/gas-custom-errors.md
@@ -0,0 +1,71 @@
+---
+warning:     "This is a dynamically generated file. Do not edit manually."
+layout:      "default"
+title:       "gas-custom-errors | Solhint"
+---
+
+# gas-custom-errors
+![Recommended Badge](https://img.shields.io/badge/-Recommended-brightgreen)
+![Category Badge](https://img.shields.io/badge/-Gas%20Consumption%20Rules-informational)
+![Default Severity Badge warn](https://img.shields.io/badge/Default%20Severity-warn-yellow)
+> The {"extends": "solhint:recommended"} property in a configuration file enables this rule.
+
+
+## Description
+Enforces the use of Custom Errors over Require and Revert statements
+
+## Options
+This rule accepts a string option of rule severity. Must be one of "error", "warn", "off". Default to warn.
+
+### Example Config
+```json
+{
+  "rules": {
+    "gas-custom-errors": "warn"
+  }
+}
+```
+
+
+## Examples
+### 👍 Examples of **correct** code for this rule
+
+#### Use of Custom Errors
+
+```solidity
+revert CustomErrorFunction();
+```
+
+#### Use of Custom Errors with arguments
+
+```solidity
+revert CustomErrorFunction({ msg: "Insufficient Balance" });
+```
+
+### 👎 Examples of **incorrect** code for this rule
+
+#### Use of require statement
+
+```solidity
+require(userBalance >= availableAmount, "Insufficient Balance");
+```
+
+#### Use of plain revert statement
+
+```solidity
+revert();
+```
+
+#### Use of revert statement with message
+
+```solidity
+revert("Insufficient Balance");
+```
+
+## Version
+This rule is introduced in the latest version.
+
+## Resources
+- [Rule source](https://github.com/protofire/solhint/tree/master/lib/rules/gas-consumption/gas-custom-errors.js)
+- [Document source](https://github.com/protofire/solhint/tree/master/docs/rules/gas-consumption/gas-custom-errors.md)
+- [Test cases](https://github.com/protofire/solhint/tree/master/test/rules/gas-consumption/gas-custom-errors.js)
diff --git a/lib/rules/best-practises/index.js b/lib/rules/best-practises/index.js
@@ -10,7 +10,6 @@ const NoConsoleLogChecker = require('./no-console')
 const NoGlobalImportsChecker = require('./no-global-import')
 const NoUnusedImportsChecker = require('./no-unused-import')
 const ExplicitTypesChecker = require('./explicit-types')
-const CustomErrorsChecker = require('./custom-errors')
 const OneContractPerFileChecker = require('./one-contract-per-file')
 
 module.exports = function checkers(reporter, config, inputSrc, tokens) {
@@ -27,7 +26,6 @@ module.exports = function checkers(reporter, config, inputSrc, tokens) {
     new NoGlobalImportsChecker(reporter),
     new NoUnusedImportsChecker(reporter, tokens),
     new ExplicitTypesChecker(reporter, config),
-    new CustomErrorsChecker(reporter),
     new OneContractPerFileChecker(reporter),
   ]
 }
diff --git a/lib/rules/best-practises/custom-errors.js → ...ules/gas-consumption/gas-custom-errors.js b/lib/rules/best-practises/custom-errors.js → ...ules/gas-consumption/gas-custom-errors.js
@@ -3,13 +3,13 @@ const { severityDescription } = require('../../doc/utils')
 
 const DEFAULT_SEVERITY = 'warn'
 
-const ruleId = 'custom-errors'
+const ruleId = 'gas-custom-errors'
 const meta = {
-  type: 'best-practises',
+  type: 'gas-consumption',
 
   docs: {
     description: 'Enforces the use of Custom Errors over Require and Revert statements',
-    category: 'Best Practise Rules',
+    category: 'Gas Consumption Rules',
     options: [
       {
         description: severityDescription,
@@ -51,7 +51,7 @@ const meta = {
   schema: null,
 }
 
-class CustomErrorsChecker extends BaseChecker {
+class GasCustomErrorsChecker extends BaseChecker {
   constructor(reporter) {
     super(reporter, ruleId, meta)
   }
@@ -68,9 +68,9 @@ class CustomErrorsChecker extends BaseChecker {
     }
 
     if (errorStr !== '') {
-      this.error(node, `Use Custom Errors instead of ${errorStr} statements`)
+      this.error(node, `GC: Use Custom Errors instead of ${errorStr} statements`)
     }
   }
 }
 
-module.exports = CustomErrorsChecker
+module.exports = GasCustomErrorsChecker
diff --git a/lib/rules/gas-consumption/index.js b/lib/rules/gas-consumption/index.js
@@ -5,6 +5,7 @@ const GasCalldataParameters = require('./gas-calldata-parameters')
 const GasIncrementByOne = require('./gas-increment-by-one')
 const GasStructPacking = require('./gas-struct-packing')
 const GasNamedReturnValuesChecker = require('./gas-named-return-values')
+const GasCustomErrorsChecker = require('./gas-custom-errors')
 
 module.exports = function checkers(reporter, config) {
   return [
@@ -15,5 +16,6 @@ module.exports = function checkers(reporter, config) {
     new GasIncrementByOne(reporter, config),
     new GasStructPacking(reporter, config),
     new GasNamedReturnValuesChecker(reporter),
+    new GasCustomErrorsChecker(reporter),
   ]
 }
diff --git a/test/rules/best-practises/custom-errors.js → ...ules/gas-consumption/gas-custom-errors.js b/test/rules/best-practises/custom-errors.js → ...ules/gas-consumption/gas-custom-errors.js
@@ -9,31 +9,31 @@ const {
 const linter = require('../../../lib/index')
 const { funcWith } = require('../../common/contract-builder')
 
-describe('Linter - custom-errors', () => {
+describe('Linter - gas-custom-errors', () => {
   it('should raise error for revert()', () => {
     const code = funcWith(`revert();`)
     const report = linter.processStr(code, {
-      rules: { 'custom-errors': 'error' },
+      rules: { 'gas-custom-errors': 'error' },
     })
 
     assertErrorCount(report, 1)
-    assertErrorMessage(report, 'Use Custom Errors instead of revert statement')
+    assertErrorMessage(report, 'GC: Use Custom Errors instead of revert statement')
   })
 
   it('should raise error for revert([string])', () => {
     const code = funcWith(`revert("Insufficent funds");`)
     const report = linter.processStr(code, {
-      rules: { 'custom-errors': 'error' },
+      rules: { 'gas-custom-errors': 'error' },
     })
 
     assertErrorCount(report, 1)
-    assertErrorMessage(report, 'Use Custom Errors instead of revert statement')
+    assertErrorMessage(report, 'GC: Use Custom Errors instead of revert statement')
   })
 
   it('should NOT raise error for revert ErrorFunction()', () => {
     const code = funcWith(`revert ErrorFunction();`)
     const report = linter.processStr(code, {
-      rules: { 'custom-errors': 'error' },
+      rules: { 'gas-custom-errors': 'error' },
     })
 
     assertNoWarnings(report)
@@ -43,7 +43,7 @@ describe('Linter - custom-errors', () => {
   it('should NOT raise error for revert ErrorFunction() with arguments', () => {
     const code = funcWith(`revert ErrorFunction({ msg: "Insufficent funds msg" });`)
     const report = linter.processStr(code, {
-      rules: { 'custom-errors': 'error' },
+      rules: { 'gas-custom-errors': 'error' },
     })
 
     assertNoWarnings(report)
@@ -55,19 +55,19 @@ describe('Linter - custom-errors', () => {
         role.bearer[account] = true;role.bearer[account] = true;
     `)
     const report = linter.processStr(code, {
-      rules: { 'custom-errors': 'error' },
+      rules: { 'gas-custom-errors': 'error' },
     })
 
     assertErrorCount(report, 1)
-    assertErrorMessage(report, 'Use Custom Errors instead of require statement')
+    assertErrorMessage(report, 'GC: Use Custom Errors instead of require statement')
   })
 
   it('should NOT raise error for regular function call', () => {
     const code = funcWith(`callOtherFunction();
         role.bearer[account] = true;role.bearer[account] = true;
     `)
     const report = linter.processStr(code, {
-      rules: { 'custom-errors': 'error' },
+      rules: { 'gas-custom-errors': 'error' },
     })
     assertNoWarnings(report)
     assertNoErrors(report)
@@ -84,8 +84,8 @@ describe('Linter - custom-errors', () => {
     })
 
     assertWarnsCount(report, 2)
-    assert.equal(report.reports[0].message, 'Use Custom Errors instead of require statements')
-    assert.equal(report.reports[1].message, 'Use Custom Errors instead of revert statements')
+    assert.equal(report.reports[0].message, 'GC: Use Custom Errors instead of require statements')
+    assert.equal(report.reports[1].message, 'GC: Use Custom Errors instead of revert statements')
   })
 
   it('should NOT raise error for default config', () => {