Merge pull request #613 from protofire/fix/gas-custom-error-new-requi…

…re-feature Fix: gas custom error new require feature
protofire · Dec 20, 2024 · 7094419 · 7094419
2 parents ded13a1 + e2a5a44
commit 7094419
Show file tree

Hide file tree

Showing 4 changed files with 114 additions and 19 deletions.
diff --git a/docs/rules.md b/docs/rules.md
@@ -57,18 +57,18 @@ 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-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 increments 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-length-in-loops](./rules/gas-consumption/gas-length-in-loops.md)         | Suggest replacing object.length in a loop condition to avoid calculation on each lap |              |            |
-| [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-strict-inequalities](./rules/gas-consumption/gas-strict-inequalities.md) | Suggest Strict Inequalities over non Strict ones                                     |              |            |
-| [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 with strings error and Revert statements | $~~~~~~~~$✔️ |            |
+| [gas-increment-by-one](./rules/gas-consumption/gas-increment-by-one.md)       | Suggest increments 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-length-in-loops](./rules/gas-consumption/gas-length-in-loops.md)         | Suggest replacing object.length in a loop condition to avoid calculation on each lap    |              |            |
+| [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-strict-inequalities](./rules/gas-consumption/gas-strict-inequalities.md) | Suggest Strict Inequalities over non Strict ones                                        |              |            |
+| [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
@@ -12,7 +12,7 @@ title:       "gas-custom-errors | Solhint"
 
 
 ## Description
-Enforces the use of Custom Errors over Require and Revert statements
+Enforces the use of Custom Errors over Require with strings error and Revert statements
 
 ## Options
 This rule accepts a string option of rule severity. Must be one of "error", "warn", "off". Defaults to warn.
@@ -44,6 +44,24 @@ revert CustomErrorFunction();
 revert CustomErrorFunction({ msg: "Insufficient Balance" });
 ```
 
+#### Use of Require with Custom Error with arguments
+
+```solidity
+require(success, CustomErrorFunction({ msg: "Insufficient Balance" });
+```
+
+#### Use of Require with function call and Custom Error
+
+```solidity
+require(isAuthorized(account), CustomErrorFunction();
+```
+
+#### Use of Require with binary comparison and Custom Error
+
+```solidity
+require(a > b, CustomErrorFunction();
+```
+
 ### 👎 Examples of **incorrect** code for this rule
 
 #### Use of require statement

diff --git a/lib/rules/gas-consumption/gas-custom-errors.js b/lib/rules/gas-consumption/gas-custom-errors.js
@@ -9,7 +9,8 @@ const meta = {
   type: 'gas-consumption',
 
   docs: {
-    description: 'Enforces the use of Custom Errors over Require and Revert statements',
+    description:
+      'Enforces the use of Custom Errors over Require with strings error and Revert statements',
     category: 'Gas Consumption Rules',
     options: [
       {
@@ -32,6 +33,18 @@ const meta = {
           description: 'Use of Custom Errors with arguments',
           code: 'revert CustomErrorFunction({ msg: "Insufficient Balance" });',
         },
+        {
+          description: 'Use of Require with Custom Error with arguments',
+          code: 'require(success, CustomErrorFunction({ msg: "Insufficient Balance" });',
+        },
+        {
+          description: 'Use of Require with function call and Custom Error',
+          code: 'require(isAuthorized(account), CustomErrorFunction();',
+        },
+        {
+          description: 'Use of Require with binary comparison and Custom Error',
+          code: 'require(a > b, CustomErrorFunction();',
+        },
       ],
       bad: [
         {
@@ -75,9 +88,9 @@ class GasCustomErrorsChecker extends BaseChecker {
 
   FunctionCall(node) {
     let errorStr = ''
-
     if (this.isVersionGreater(node)) {
-      if (node.expression.name === 'require') {
+      // added second part of conditional to be able to use require with Custom Errors
+      if (node.expression.name === 'require' && node.arguments[1].type !== 'FunctionCall') {
         errorStr = 'require'
       } else if (
         node.expression.name === 'revert' &&

diff --git a/test/rules/gas-consumption/gas-custom-errors.js b/test/rules/gas-consumption/gas-custom-errors.js
@@ -17,6 +17,70 @@ function replaceSolidityVersion(code, newVersion) {
 }
 
 describe('Linter - gas-custom-errors', () => {
+  it('should NOT raise error for require with comparison and custom error()', () => {
+    let code = funcWith(`require(a > b, CustomErrorEmitted());`)
+    code = replaceSolidityVersion(code, '^0.8.4')
+
+    const report = linter.processStr(code, {
+      rules: { 'gas-custom-errors': 'error' },
+    })
+
+    assertNoWarnings(report)
+    assertNoErrors(report)
+  })
+
+  it('should NOT raise error for require with function call and comparison along with custom error', () => {
+    let code = funcWith(`require(isAok(a) > b, CustomErrorEmitted(param1));`)
+    code = replaceSolidityVersion(code, '^0.8.4')
+
+    const report = linter.processStr(code, {
+      rules: { 'gas-custom-errors': 'error' },
+    })
+
+    assertNoWarnings(report)
+    assertNoErrors(report)
+  })
+
+  it('should NOT raise error for require with boolean check and custom error call', () => {
+    let code = funcWith(`require(success, CustomErrorEmitted(param1, param2));`)
+    code = replaceSolidityVersion(code, '^0.8.4')
+
+    const report = linter.processStr(code, {
+      rules: { 'gas-custom-errors': 'error' },
+    })
+
+    assertNoWarnings(report)
+    assertNoErrors(report)
+  })
+
+  it('should NOT raise error for require with function call and custom error call', () => {
+    let code = funcWith(
+      `require(isSuccess(param1) == true && value > 10, CustomErrorEmitted(param1, param2));`
+    )
+    code = replaceSolidityVersion(code, '^0.8.4')
+
+    const report = linter.processStr(code, {
+      rules: { 'gas-custom-errors': 'error' },
+    })
+
+    assertNoWarnings(report)
+    assertNoErrors(report)
+  })
+
+  it('should NOT raise error for require and mapping access with boolean value check and custom error call', () => {
+    let code = funcWith(
+      `require(users[msg.sender].isRegistered, CustomErrorEmitted(param1, param2));`
+    )
+    code = replaceSolidityVersion(code, '^0.8.4')
+
+    const report = linter.processStr(code, {
+      rules: { 'gas-custom-errors': 'error' },
+    })
+
+    assertNoWarnings(report)
+    assertNoErrors(report)
+  })
+
   it('should raise error for revert()', () => {
     let code = funcWith(`revert();`)
     code = replaceSolidityVersion(code, '^0.8.4')
@@ -30,7 +94,7 @@ describe('Linter - gas-custom-errors', () => {
   })
 
   it('should raise error for revert([string])', () => {
-    let code = funcWith(`revert("Insufficent funds");`)
+    let code = funcWith(`revert("Insufficient funds");`)
     code = replaceSolidityVersion(code, '0.8.4')
 
     const report = linter.processStr(code, {
@@ -54,7 +118,7 @@ describe('Linter - gas-custom-errors', () => {
   })
 
   it('should NOT raise error for revert ErrorFunction() with arguments', () => {
-    let code = funcWith(`revert ErrorFunction({ msg: "Insufficent funds msg" });`)
+    let code = funcWith(`revert ErrorFunction({ msg: "Insufficient funds msg" });`)
     code = replaceSolidityVersion(code, '^0.8.5')
 
     const report = linter.processStr(code, {
@@ -134,7 +198,7 @@ describe('Linter - gas-custom-errors', () => {
   })
 
   it('should NOT raise error for lower versions 0.4.4', () => {
-    const code = funcWith(`revert("Insufficent funds");`)
+    const code = funcWith(`revert("Insufficient funds");`)
 
     const report = linter.processStr(code, {
       rules: { 'gas-custom-errors': 'error' },