Repository: protofire/solhint
Pull request #528 (2023-12-20 13:35:09)

autofix payable-fallback added


Patch:
diff --git a/lib/common/ast-types.js b/lib/common/ast-types.js
index 0bdae5a..d2fbe4d 100644
--- a/lib/common/ast-types.js
+++ b/lib/common/ast-types.js
@@ -1,5 +1,5 @@
 function isFallbackFunction(node) {
-  return isFunctionDefinition(node) && node.isFallback
+  return isFunctionDefinition(node) && (node.isFallback || node.isReceiveEther)
 }
 
 function isReceiveFunction(node) {
diff --git a/lib/rules/best-practises/payable-fallback.js b/lib/rules/best-practises/payable-fallback.js
index 79921a9..723925a 100644
--- a/lib/rules/best-practises/payable-fallback.js
+++ b/lib/rules/best-practises/payable-fallback.js
@@ -27,7 +27,7 @@ const meta = {
   isDefault: false,
   recommended: true,
   defaultSetup: 'warn',
-
+  fixable: true,
   schema: null,
 }
 
@@ -39,10 +39,27 @@ class PayableFallbackChecker extends BaseChecker {
   FunctionDefinition(node) {
     if (isFallbackFunction(node)) {
       if (node.stateMutability !== 'payable') {
-        this.warn(node, 'When fallback is not payable you will not be able to receive ether')
+        this.warn(
+          node,
+          'Fallback should be external and payable to accept native currency',
+          this.fixStatement(node)
+        )
       }
     }
   }
+
+  fixStatement(node) {
+    const range = node.range
+    const stringToPut = ' payable '
+
+    if (node.isReceiveEther) {
+      range[0] += 9
+    } else {
+      range[0] += 10
+    }
+
+    return (fixer) => fixer.insertTextBeforeRange(range, stringToPut)
+  }
 }
 
 module.exports = PayableFallbackChecker


Test patch:
diff --git a/e2e/08-autofix/payable-fallback/.solhint.json b/e2e/08-autofix/payable-fallback/.solhint.json
new file mode 100644
index 0000000..aa02d3b
--- /dev/null
+++ b/e2e/08-autofix/payable-fallback/.solhint.json
@@ -0,0 +1,5 @@
+{
+  "rules": {
+    "payable-fallback": "error"
+  }
+}
diff --git a/e2e/08-autofix/payable-fallback/Foo1.sol b/e2e/08-autofix/payable-fallback/Foo1.sol
new file mode 100644
index 0000000..eb1cfda
--- /dev/null
+++ b/e2e/08-autofix/payable-fallback/Foo1.sol
@@ -0,0 +1,50 @@
+// SPDX-License-Identifier: Apache-2.0
+pragma solidity ^0.8.0;
+
+contract Generic {
+    
+    constructor()  {}
+
+    function anyFunction() {}
+
+    //// fallback with receive
+    receive() public {}
+    
+    receive() external onlyOwner {}
+
+    receive() external virtual {
+        uint256 anUintToFillSpace;
+    }
+
+    //// fallback with no name
+    function() public {}
+
+    function() {
+        uint256 anUintToFillSpace;
+    }
+
+    function() external onlyOwner {}
+
+    function() virtual {
+        uint256 anUintToFillSpace;
+    }
+
+
+    //// fallback explicit
+    fallback() {}
+
+    fallback() {
+        uint256 anUintToFillSpace;
+    }
+
+    fallback() external onlyOwner{
+        uint256 anUintToFillSpace;
+    }
+
+    fallback() virtual {}
+
+
+    fallback() external payable {}
+    function() external payable {}    
+    receive() public virtual payable {}
+}
diff --git a/e2e/08-autofix/payable-fallback/Foo1AfterFix.sol b/e2e/08-autofix/payable-fallback/Foo1AfterFix.sol
new file mode 100644
index 0000000..f32e5c5
--- /dev/null
+++ b/e2e/08-autofix/payable-fallback/Foo1AfterFix.sol
@@ -0,0 +1,50 @@
+// SPDX-License-Identifier: Apache-2.0
+pragma solidity ^0.8.0;
+
+contract Generic {
+    
+    constructor()  {}
+
+    function anyFunction() {}
+
+    //// fallback with receive
+    receive() payable public {}
+    
+    receive() payable external onlyOwner {}
+
+    receive() payable external virtual {
+        uint256 anUintToFillSpace;
+    }
+
+    //// fallback with no name
+    function() payable public {}
+
+    function() payable {
+        uint256 anUintToFillSpace;
+    }
+
+    function() payable external onlyOwner {}
+
+    function() payable virtual {
+        uint256 anUintToFillSpace;
+    }
+
+
+    //// fallback explicit
+    fallback() payable {}
+
+    fallback() payable {
+        uint256 anUintToFillSpace;
+    }
+
+    fallback() payable external onlyOwner{
+        uint256 anUintToFillSpace;
+    }
+
+    fallback() payable virtual {}
+
+
+    fallback() external payable {}
+    function() external payable {}    
+    receive() public virtual payable {}
+}
diff --git a/e2e/08-autofix/payable-fallback/Foo1BeforeFix.sol b/e2e/08-autofix/payable-fallback/Foo1BeforeFix.sol
new file mode 100644
index 0000000..eb1cfda
--- /dev/null
+++ b/e2e/08-autofix/payable-fallback/Foo1BeforeFix.sol
@@ -0,0 +1,50 @@
+// SPDX-License-Identifier: Apache-2.0
+pragma solidity ^0.8.0;
+
+contract Generic {
+    
+    constructor()  {}
+
+    function anyFunction() {}
+
+    //// fallback with receive
+    receive() public {}
+    
+    receive() external onlyOwner {}
+
+    receive() external virtual {
+        uint256 anUintToFillSpace;
+    }
+
+    //// fallback with no name
+    function() public {}
+
+    function() {
+        uint256 anUintToFillSpace;
+    }
+
+    function() external onlyOwner {}
+
+    function() virtual {
+        uint256 anUintToFillSpace;
+    }
+
+
+    //// fallback explicit
+    fallback() {}
+
+    fallback() {
+        uint256 anUintToFillSpace;
+    }
+
+    fallback() external onlyOwner{
+        uint256 anUintToFillSpace;
+    }
+
+    fallback() virtual {}
+
+
+    fallback() external payable {}
+    function() external payable {}    
+    receive() public virtual payable {}
+}
diff --git a/e2e/autofix-test.js b/e2e/autofix-test.js
index 5579f83..b79d961 100644
--- a/e2e/autofix-test.js
+++ b/e2e/autofix-test.js
@@ -25,7 +25,6 @@ function retrieveParams(subpath) {
 function compareTextFiles(file1Path, file2Path) {
   const file1Content = fs.readFileSync(file1Path, 'utf-8')
   const file2Content = fs.readFileSync(file2Path, 'utf-8')
-
   return file1Content === file2Content
 }
 
@@ -51,6 +50,8 @@ function useFixture(dir) {
 
 describe('e2e', function () {
   let result = false
+  let code
+  let stdout
 
   describe('autofix tests', () => {
     if (E2E) {
@@ -160,28 +161,32 @@ describe('e2e', function () {
           }
         })
 
-        it('should compare Foo1 file with template BEFORE FIX file and they should match 2', () => {
+        it('should compare Foo1 file with template BEFORE FIX file and they should match (2)', () => {
           result = compareTextFiles(currentFile, beforeFixFile)
           expect(result).to.be.true
         })
 
-        it('should compare Foo1 file with template AFTER FIX file and they should match 2', () => {
-          const { code, stdout } = shell.exec(
+        it('should execute and compare Foo1 with template AFTER FIX and they should match (2)', () => {
+          ({ code, stdout } = shell.exec(
             `${params.command} ${params.param1} -c ${currentConfig} ${currentFile} --fix --disc --noPrompt`
-          )
+          ))
+
+          result = compareTextFiles(currentFile, afterFixFile)
+          expect(result).to.be.true
+        })
 
+        it('should execute and exit with code 1 (2)', () => {
           expect(code).to.equal(1)
+        })
 
+        it('should get the right report (2)', () => {
           const reportLines = stdout.split('\n')
           const finalLine = '5 problems (5 errors, 0 warnings)'
           expect(reportLines[reportLines.length - 3]).to.contain(finalLine)
-
-          result = compareTextFiles(currentFile, afterFixFile)
-          expect(result).to.be.true
         })
       })
 
-      it('should check FOO1 does not change after test 2', () => {
+      it('should check FOO1 does not change after test (2)', () => {
         result = compareTextFiles(currentFile, beforeFixFile)
         expect(result).to.be.true
       })
@@ -202,28 +207,32 @@ describe('e2e', function () {
           }
         })
 
-        it('should compare Foo1 file with template BEFORE FIX file and they should match 3', () => {
+        it('should compare Foo1 file with template BEFORE FIX file and they should match (3)', () => {
           result = compareTextFiles(currentFile, beforeFixFile)
           expect(result).to.be.true
         })
 
-        it('should compare Foo1 file with template AFTER FIX file and they should match 3', () => {
-          const { code, stdout } = shell.exec(
+        it('should execute and compare Foo1 with template AFTER FIX and they should match (3)', () => {
+          ({ code, stdout } = shell.exec(
             `${params.command} ${params.param1} -c ${currentConfig} ${currentFile} --fix --disc --noPrompt`
-          )
+          ))
 
+          result = compareTextFiles(currentFile, afterFixFile)
+          expect(result).to.be.true
+        })
+
+        it('should execute and exit with code 1 (3)', () => {
           expect(code).to.equal(1)
+        })
 
+        it('should get the right report (3)', () => {
           const reportLines = stdout.split('\n')
           const finalLine = '9 problems (9 errors, 0 warnings)'
           expect(reportLines[reportLines.length - 3]).to.contain(finalLine)
-
-          result = compareTextFiles(currentFile, afterFixFile)
-          expect(result).to.be.true
         })
       })
 
-      it('should check FOO1 does not change after test 3', () => {
+      it('should check FOO1 does not change after test (3)', () => {
         result = compareTextFiles(currentFile, beforeFixFile)
         expect(result).to.be.true
       })
@@ -244,27 +253,78 @@ describe('e2e', function () {
           }
         })
 
-        it('should compare Foo1 file with template BEFORE FIX file and they should match 4', () => {
+        it('should compare Foo1 file with template BEFORE FIX file and they should match (4)', () => {
           result = compareTextFiles(currentFile, beforeFixFile)
           expect(result).to.be.true
         })
 
-        it('should compare Foo1 file with template AFTER FIX file and they should match 4', () => {
-          const { code, stdout } = shell.exec(
+        it('should execute and compare Foo1 with template AFTER FIX and they should match (4)', () => {
+          ({ code, stdout } = shell.exec(
             `${params.command} ${params.param1} -c ${currentConfig} ${currentFile} --fix --disc --noPrompt`
-          )
+          ))
 
+          result = compareTextFiles(currentFile, afterFixFile)
+          expect(result).to.be.true
+        })
+
+        it('should execute and exit with code 1 (4)', () => {
           expect(code).to.equal(1)
+        })
 
+        it('should get the right report (4)', () => {
           const reportLines = stdout.split('\n')
           const finalLine = '19 problems (19 errors, 0 warnings)'
           expect(reportLines[reportLines.length - 3]).to.contain(finalLine)
+        })
+      })
+
+      it('should check FOO1 does not change after test (4)', () => {
+        result = compareTextFiles(currentFile, beforeFixFile)
+        expect(result).to.be.true
+      })
+    })
+
+    describe('autofix rule: payable-fallback', () => {
+      before(function () {
+        params = retrieveParams('payable-fallback/')
+        currentConfig = `${params.path}${params.subpath}.solhint.json`
+        currentFile = `${params.path}${params.subpath}Foo1.sol`
+        beforeFixFile = `${params.path}${params.subpath}Foo1BeforeFix.sol`
+        afterFixFile = `${params.path}${params.subpath}Foo1AfterFix.sol`
+      })
+      describe('--fix with noPrompt', () => {
+        after(function () {
+          if (!E2E) {
+            copyFile(beforeFixFile, currentFile)
+          }
+        })
+
+        it('should compare Foo1 file with template BEFORE FIX file and they should match (5)', () => {
+          result = compareTextFiles(currentFile, beforeFixFile)
+          expect(result).to.be.true
+        })
+
+        it('should execute and compare Foo1 with template AFTER FIX and they should match (5)', () => {
+          ({ code, stdout } = shell.exec(
+            `${params.command} ${params.param1} -c ${currentConfig} ${currentFile} --fix --disc --noPrompt`
+          ))
 
           result = compareTextFiles(currentFile, afterFixFile)
           expect(result).to.be.true
         })
+
+        it('should execute and exit with code 1 (5)', () => {
+          expect(code).to.equal(1)
+        })
+
+        it('should get the right report (5)', () => {
+          const reportLines = stdout.split('\n')
+          const finalLine = '11 problems (11 errors, 0 warnings)'
+          expect(reportLines[reportLines.length - 3]).to.contain(finalLine)
+        })
       })
-      it('should check FOO1 does not change after test 4', () => {
+
+      it('should check FOO1 does not change after test (5)', () => {
         result = compareTextFiles(currentFile, beforeFixFile)
         expect(result).to.be.true
       })
@@ -272,5 +332,4 @@ describe('e2e', function () {
   })
 })
 
-// FALTA LA COMPARACION DEL FIX CON EL TEMPLATE FIX
 // FALTA LA PRUEBA DEL STORE TO FILE
diff --git a/test/rules/best-practises/payable-fallback.js b/test/rules/best-practises/payable-fallback.js
index 734c551..281b776 100644
--- a/test/rules/best-practises/payable-fallback.js
+++ b/test/rules/best-practises/payable-fallback.js
@@ -33,4 +33,15 @@ describe('Linter - payable-fallback', () => {
 
     assertNoWarnings(report)
   })
+
+  it('should raise for other fallback types when are not payable', () => {
+    const code = contractWith('fallback() external {} receive() onlyOwner {}')
+
+    const report = linter.processStr(code, {
+      rules: { 'payable-fallback': 'warn' },
+    })
+
+    assertWarnsCount(report, 2)
+    assertErrorMessage(report, 'payable')
+  })
 })