diff --git a/.gitignore b/.gitignore index 77dd2e5e..063bb153 100644 --- a/.gitignore +++ b/.gitignore @@ -9,4 +9,4 @@ convertLib.sol antlr4.jar /docs/.sass-cache/ _temp/ -.solhint.json + diff --git a/conf/rulesets/solhint-all.js b/conf/rulesets/solhint-all.js index ef938dce..f0fb2e49 100644 --- a/conf/rulesets/solhint-all.js +++ b/conf/rulesets/solhint-all.js @@ -6,6 +6,7 @@ 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], @@ -28,6 +29,7 @@ module.exports = Object.freeze({ 'const-name-snakecase': 'warn', 'contract-name-camelcase': 'warn', 'event-name-camelcase': 'warn', + 'foundry-test-functions': ['off', ['setUp']], 'func-name-mixedcase': 'warn', 'func-named-parameters': ['warn', 4], 'func-param-name-mixedcase': 'warn', diff --git a/conf/rulesets/solhint-recommended.js b/conf/rulesets/solhint-recommended.js index a8a0e0d1..df6b2c41 100644 --- a/conf/rulesets/solhint-recommended.js +++ b/conf/rulesets/solhint-recommended.js @@ -5,6 +5,7 @@ module.exports = Object.freeze({ rules: { + 'custom-errors': 'warn', 'explicit-types': ['warn', 'explicit'], 'max-states-count': ['warn', 15], 'no-console': 'error', diff --git a/docs/rules.md b/docs/rules.md index eed7b6d0..2b5c944e 100644 --- a/docs/rules.md +++ b/docs/rules.md @@ -9,6 +9,7 @@ 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. | | | @@ -38,6 +39,7 @@ title: "Rule Index of Solhint" | [const-name-snakecase](./rules/naming/const-name-snakecase.md) | Constant name must be in capitalized SNAKE_CASE. (Does not check IMMUTABLES, use immutable-vars-naming) | $~~~~~~~~$✔️ | | | [contract-name-camelcase](./rules/naming/contract-name-camelcase.md) | Contract name must be in CamelCase. | $~~~~~~~~$✔️ | | | [event-name-camelcase](./rules/naming/event-name-camelcase.md) | Event name must be in CamelCase. | $~~~~~~~~$✔️ | | +| [foundry-test-functions](./rules/naming/foundry-test-functions.md) | Enforce naming convention on functions for Foundry test cases | | | | [func-name-mixedcase](./rules/naming/func-name-mixedcase.md) | Function name must be in mixedCase. | $~~~~~~~~$✔️ | | | [func-named-parameters](./rules/naming/func-named-parameters.md) | Enforce named parameters for function calls with 4 or more arguments. This rule may have some false positives | | | | [func-param-name-mixedcase](./rules/naming/func-param-name-mixedcase.md) | Function param name must be in mixedCase. | | | diff --git a/docs/rules/best-practises/custom-errors.md b/docs/rules/best-practises/custom-errors.md new file mode 100644 index 00000000..afe5b3c7 --- /dev/null +++ b/docs/rules/best-practises/custom-errors.md @@ -0,0 +1,71 @@ +--- +warning: "This is a dynamically generated file. Do not edit manually." +layout: "default" +title: "custom-errors | Solhint" +--- + +# custom-errors +![Recommended Badge](https://img.shields.io/badge/-Recommended-brightgreen) +![Category Badge](https://img.shields.io/badge/-Best%20Practise%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": { + "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/best-practises/custom-errors.js) +- [Document source](https://github.com/protofire/solhint/tree/master/docs/rules/best-practises/custom-errors.md) +- [Test cases](https://github.com/protofire/solhint/tree/master/test/rules/best-practises/custom-errors.js) diff --git a/docs/rules/best-practises/explicit-types.md b/docs/rules/best-practises/explicit-types.md index 96cb06a5..8448bc6e 100644 --- a/docs/rules/best-practises/explicit-types.md +++ b/docs/rules/best-practises/explicit-types.md @@ -63,7 +63,7 @@ uint256 public variableName ``` ## Version -This rule is introduced in the latest version. +This rule was introduced in [Solhint 3.5.1](https://github.com/protofire/solhint/tree/v3.5.1) ## Resources - [Rule source](https://github.com/protofire/solhint/tree/master/lib/rules/best-practises/explicit-types.js) diff --git a/docs/rules/best-practises/no-unused-import.md b/docs/rules/best-practises/no-unused-import.md index 8d13e897..8d567c91 100644 --- a/docs/rules/best-practises/no-unused-import.md +++ b/docs/rules/best-practises/no-unused-import.md @@ -51,7 +51,7 @@ This rule accepts a string option of rule severity. Must be one of "error", "war ``` ## Version -This rule is introduced in the latest version. +This rule was introduced in [Solhint 3.5.1](https://github.com/protofire/solhint/tree/v3.5.1) ## Resources - [Rule source](https://github.com/protofire/solhint/tree/master/lib/rules/best-practises/no-unused-import.js) diff --git a/docs/rules/naming/foundry-test-functions.md b/docs/rules/naming/foundry-test-functions.md new file mode 100644 index 00000000..534ad6e5 --- /dev/null +++ b/docs/rules/naming/foundry-test-functions.md @@ -0,0 +1,73 @@ +--- +warning: "This is a dynamically generated file. Do not edit manually." +layout: "default" +title: "foundry-test-functions | Solhint" +--- + +# foundry-test-functions +![Category Badge](https://img.shields.io/badge/-Style%20Guide%20Rules-informational) +![Default Severity Badge off](https://img.shields.io/badge/Default%20Severity-off-undefined) + +## Description +Enforce naming convention on functions for Foundry test cases + +## Options +This rule accepts an array of options: + +| Index | Description | Default Value | +| ----- | ----------------------------------------------------- | ------------- | +| 0 | Rule severity. Must be one of "error", "warn", "off". | off | + + +### Example Config +```json +{ + "rules": { + "foundry-test-functions": ["off",["setUp"]] + } +} +``` + +### Notes +- This rule can be configured to skip certain function names in the SKIP array. In Example Config. ```setUp``` function will be skipped +- Supported Regex: ```test(Fork)?(Fuzz)?(Fail)?_(Revert(If_|When_){1})?\w{1,}``` +- This rule should be executed in a separate folder with a separate .solhint.json => ```solhint --config .solhint.json testFolder/**/*.sol``` +- This rule applies only to `external` and `public` functions +- This rule skips the `setUp()` function + +## Examples +### 👍 Examples of **correct** code for this rule + +#### Foundry test case with correct Function declaration + +```solidity +function test_NumberIs42() public {} +``` + +#### Foundry test case with correct Function declaration + +```solidity +function testFail_Subtract43() public {} +``` + +#### Foundry test case with correct Function declaration + +```solidity +function testFuzz_FuzzyTest() public {} +``` + +### 👎 Examples of **incorrect** code for this rule + +#### Foundry test case with incorrect Function declaration + +```solidity +function numberIs42() public {} +``` + +## Version +This rule is introduced in the latest version. + +## Resources +- [Rule source](https://github.com/protofire/solhint/tree/master/lib/rules/naming/foundry-test-functions.js) +- [Document source](https://github.com/protofire/solhint/tree/master/docs/rules/naming/foundry-test-functions.md) +- [Test cases](https://github.com/protofire/solhint/tree/master/test/rules/naming/foundry-test-functions.js) diff --git a/docs/rules/naming/func-named-parameters.md b/docs/rules/naming/func-named-parameters.md index 7d219c6a..44fe7da6 100644 --- a/docs/rules/naming/func-named-parameters.md +++ b/docs/rules/naming/func-named-parameters.md @@ -60,7 +60,7 @@ functionName(_senderAddress, 1e18, _tokenAddress, _receiverAddress ) ``` ## Version -This rule is introduced in the latest version. +This rule was introduced in [Solhint 3.5.1](https://github.com/protofire/solhint/tree/v3.5.1) ## Resources - [Rule source](https://github.com/protofire/solhint/tree/master/lib/rules/naming/func-named-parameters.js) diff --git a/docs/rules/naming/immutable-vars-naming.md b/docs/rules/naming/immutable-vars-naming.md index 8705d7ec..a12c2291 100644 --- a/docs/rules/naming/immutable-vars-naming.md +++ b/docs/rules/naming/immutable-vars-naming.md @@ -37,7 +37,7 @@ This rule accepts an array of options: This rule does not have examples. ## Version -This rule is introduced in the latest version. +This rule was introduced in [Solhint 3.5.1](https://github.com/protofire/solhint/tree/v3.5.1) ## Resources - [Rule source](https://github.com/protofire/solhint/tree/master/lib/rules/naming/immutable-vars-naming.js) diff --git a/e2e/07-foundry-test/.solhint.json b/e2e/07-foundry-test/.solhint.json new file mode 100644 index 00000000..92277f3d --- /dev/null +++ b/e2e/07-foundry-test/.solhint.json @@ -0,0 +1,7 @@ +{ + "extends": "solhint:recommended", + "rules": { + "compiler-version": "off", + "func-visibility": "off" + } +} diff --git a/e2e/07-foundry-test/contracts/Foo.sol b/e2e/07-foundry-test/contracts/Foo.sol new file mode 100644 index 00000000..d5a6ce1b --- /dev/null +++ b/e2e/07-foundry-test/contracts/Foo.sol @@ -0,0 +1,10 @@ +// SPDX-License-Identifier: MIT +pragma solidity >=0.6.0; + +contract Foo { + uint256 public constant TEST = 1; + + constructor() { + + } +} diff --git a/e2e/07-foundry-test/test/.solhint.json b/e2e/07-foundry-test/test/.solhint.json new file mode 100644 index 00000000..da87cc47 --- /dev/null +++ b/e2e/07-foundry-test/test/.solhint.json @@ -0,0 +1,5 @@ +{ + "rules": { + "foundry-test-functions": ["error", ["setUp", "finish"]] + } +} diff --git a/e2e/07-foundry-test/test/FooTest.sol b/e2e/07-foundry-test/test/FooTest.sol new file mode 100644 index 00000000..be603e53 --- /dev/null +++ b/e2e/07-foundry-test/test/FooTest.sol @@ -0,0 +1,20 @@ +// SPDX-License-Identifier: MIT +pragma solidity >=0.6.0; + +import "./Test.sol"; + +contract FooTest is Test { + uint256 testNumber; + + function setUp() public { + testNumber = 42; + } + + function testFail_Subtract43() public { + testNumber -= 43; + } + + function wrongFunctionDefinitionName() public { + testNumber -= 43; + } +} \ No newline at end of file diff --git a/e2e/07-foundry-test/test/Test.sol b/e2e/07-foundry-test/test/Test.sol new file mode 100644 index 00000000..d7d16242 --- /dev/null +++ b/e2e/07-foundry-test/test/Test.sol @@ -0,0 +1,4 @@ +// SPDX-License-Identifier: MIT +pragma solidity >=0.6.0; + +contract Test {} diff --git a/e2e/test.js b/e2e/test.js index bf3910f3..c218dae1 100644 --- a/e2e/test.js +++ b/e2e/test.js @@ -147,4 +147,26 @@ describe('e2e', function () { expect(stdout.trim()).to.not.contain(errorFound) }) }) + + describe('Linter - foundry-test-functions with shell', () => { + // Foo contract has 1 warning + // FooTest contract has 1 error + useFixture('07-foundry-test') + + it(`should raise error for empty blocks only`, () => { + const { code, stdout } = shell.exec('solhint contracts/Foo.sol') + + expect(code).to.equal(0) + expect(stdout.trim()).to.contain('Code contains empty blocks') + }) + + it(`should raise error for wrongFunctionDefinitionName() only`, () => { + const { code, stdout } = shell.exec('solhint -c test/.solhint.json test/FooTest.sol') + + expect(code).to.equal(1) + expect(stdout.trim()).to.contain( + 'Function wrongFunctionDefinitionName() must match Foundry test naming convention' + ) + }) + }) }) diff --git a/lib/common/identifier-naming.js b/lib/common/identifier-naming.js index 1444d398..efc7e3f5 100644 --- a/lib/common/identifier-naming.js +++ b/lib/common/identifier-naming.js @@ -30,4 +30,9 @@ module.exports = { hasLeadingUnderscore(text) { return text && text[0] === '_' }, + + isNotFoundryTestCase(text) { + const regex = /^test(Fork)?(Fuzz)?(Fail)?_(Revert(If_|When_){1})?\w{1,}$/ + return !match(text, regex) + }, } diff --git a/lib/config.js b/lib/config.js index 44b4c420..130b92f0 100644 --- a/lib/config.js +++ b/lib/config.js @@ -35,4 +35,10 @@ module.exports = { getString(ruleName, defaultValue) { return this.getStringByPath(`rules["${ruleName}"][1]`, defaultValue) }, + + getArray(ruleName, defaultValue) { + const path = `rules["${ruleName}"][1]` + const configVal = _.get(this, path) + return _.isArray(configVal) ? configVal : defaultValue + }, } diff --git a/lib/rules/best-practises/custom-errors.js b/lib/rules/best-practises/custom-errors.js new file mode 100644 index 00000000..ce4304af --- /dev/null +++ b/lib/rules/best-practises/custom-errors.js @@ -0,0 +1,76 @@ +const BaseChecker = require('../base-checker') +const { severityDescription } = require('../../doc/utils') + +const DEFAULT_SEVERITY = 'warn' + +const ruleId = 'custom-errors' +const meta = { + type: 'best-practises', + + docs: { + description: 'Enforces the use of Custom Errors over Require and Revert statements', + category: 'Best Practise Rules', + options: [ + { + description: severityDescription, + default: DEFAULT_SEVERITY, + }, + ], + examples: { + good: [ + { + description: 'Use of Custom Errors', + code: 'revert CustomErrorFunction();', + }, + { + description: 'Use of Custom Errors with arguments', + code: 'revert CustomErrorFunction({ msg: "Insufficient Balance" });', + }, + ], + bad: [ + { + description: 'Use of require statement', + code: 'require(userBalance >= availableAmount, "Insufficient Balance");', + }, + { + description: 'Use of plain revert statement', + code: 'revert();', + }, + { + description: 'Use of revert statement with message', + code: 'revert("Insufficient Balance");', + }, + ], + }, + }, + + isDefault: false, + recommended: true, + defaultSetup: DEFAULT_SEVERITY, + + schema: null, +} + +class CustomErrorsChecker extends BaseChecker { + constructor(reporter) { + super(reporter, ruleId, meta) + } + + FunctionCall(node) { + let errorStr = '' + if (node.expression.name === 'require') { + errorStr = 'require' + } else if ( + node.expression.name === 'revert' && + (node.arguments.length === 0 || node.arguments[0].type === 'StringLiteral') + ) { + errorStr = 'revert' + } + + if (errorStr !== '') { + this.error(node, `Use Custom Errors instead of ${errorStr} statements`) + } + } +} + +module.exports = CustomErrorsChecker diff --git a/lib/rules/best-practises/index.js b/lib/rules/best-practises/index.js index 6b2ac780..52133e83 100644 --- a/lib/rules/best-practises/index.js +++ b/lib/rules/best-practises/index.js @@ -10,6 +10,7 @@ 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') module.exports = function checkers(reporter, config, inputSrc) { return [ @@ -25,5 +26,6 @@ module.exports = function checkers(reporter, config, inputSrc) { new NoGlobalImportsChecker(reporter), new NoUnusedImportsChecker(reporter), new ExplicitTypesChecker(reporter, config), + new CustomErrorsChecker(reporter), ] } diff --git a/lib/rules/naming/foundry-test-functions.js b/lib/rules/naming/foundry-test-functions.js new file mode 100644 index 00000000..1cc367a6 --- /dev/null +++ b/lib/rules/naming/foundry-test-functions.js @@ -0,0 +1,93 @@ +const BaseChecker = require('../base-checker') +const naming = require('../../common/identifier-naming') +const { severityDescription } = require('../../doc/utils') + +const DEFAULT_SEVERITY = 'off' +const DEFAULT_SKIP_FUNCTIONS = ['setUp'] + +const ruleId = 'foundry-test-functions' +const meta = { + type: 'naming', + + docs: { + description: `Enforce naming convention on functions for Foundry test cases`, + category: 'Style Guide Rules', + options: [ + { + description: severityDescription, + default: DEFAULT_SEVERITY, + }, + ], + examples: { + good: [ + { + description: 'Foundry test case with correct Function declaration', + code: 'function test_NumberIs42() public {}', + }, + { + description: 'Foundry test case with correct Function declaration', + code: 'function testFail_Subtract43() public {}', + }, + { + description: 'Foundry test case with correct Function declaration', + code: 'function testFuzz_FuzzyTest() public {}', + }, + ], + bad: [ + { + description: 'Foundry test case with incorrect Function declaration', + code: 'function numberIs42() public {}', + }, + ], + }, + notes: [ + { + note: 'This rule can be configured to skip certain function names in the SKIP array. In Example Config. ```setUp``` function will be skipped', + }, + { note: 'Supported Regex: ```test(Fork)?(Fuzz)?(Fail)?_(Revert(If_|When_){1})?\\w{1,}```' }, + { + note: 'This rule should be executed in a separate folder with a separate .solhint.json => ```solhint --config .solhint.json testFolder/**/*.sol```', + }, + { + note: 'This rule applies only to `external` and `public` functions', + }, + { + note: 'This rule skips the `setUp()` function', + }, + ], + }, + + isDefault: false, + recommended: false, + defaultSetup: [DEFAULT_SEVERITY, DEFAULT_SKIP_FUNCTIONS], + + schema: { type: 'array' }, +} + +class FoundryTestFunctionsChecker extends BaseChecker { + constructor(reporter, config) { + super(reporter, ruleId, meta) + this.skippedFunctions = config + ? config.getArray(ruleId, DEFAULT_SKIP_FUNCTIONS) + : DEFAULT_SKIP_FUNCTIONS + } + + FunctionDefinition(node) { + // function name should not be in skipped functions array + // should be external or public + if ( + !this.searchInArray(this.skippedFunctions, node.name) && + (node.visibility === 'public' || node.visibility === 'external') + ) { + if (naming.isNotFoundryTestCase(node.name)) { + this.error(node, `Function ${node.name}() must match Foundry test naming convention`) + } + } + } + + searchInArray(array, searchString) { + return array.indexOf(searchString) !== -1 + } +} + +module.exports = FoundryTestFunctionsChecker diff --git a/lib/rules/naming/index.js b/lib/rules/naming/index.js index 1dffa3f8..43ead75b 100644 --- a/lib/rules/naming/index.js +++ b/lib/rules/naming/index.js @@ -10,6 +10,7 @@ const VarNameMixedcaseChecker = require('./var-name-mixedcase') const NamedParametersMappingChecker = require('./named-parameters-mapping') const ImmutableVarsNamingChecker = require('./immutable-vars-naming') const FunctionNamedParametersChecker = require('./func-named-parameters') +const FoundryTestFunctionsChecker = require('./foundry-test-functions') module.exports = function checkers(reporter, config) { return [ @@ -25,5 +26,6 @@ module.exports = function checkers(reporter, config) { new NamedParametersMappingChecker(reporter), new ImmutableVarsNamingChecker(reporter, config), new FunctionNamedParametersChecker(reporter, config), + new FoundryTestFunctionsChecker(reporter, config), ] } diff --git a/lib/rules/security/check-send-result.js b/lib/rules/security/check-send-result.js index a20ee2ed..485ca88f 100644 --- a/lib/rules/security/check-send-result.js +++ b/lib/rules/security/check-send-result.js @@ -14,13 +14,13 @@ const meta = { good: [ { description: 'result of "send" call checked with if statement', - code: require('../../../test/fixtures/security/send-result-checked'), + code: 'if(x.send(55)) {}', }, ], bad: [ { description: 'result of "send" call ignored', - code: require('../../../test/fixtures/security/send-result-ignored'), + code: 'x.send(55);', }, ], }, @@ -44,17 +44,24 @@ class CheckSendResultChecker extends BaseChecker { validateSend(node) { if (node.memberName === 'send') { - const hasVarDeclaration = traversing.statementNotContains(node, 'VariableDeclaration') - const hasIfStatement = traversing.statementNotContains(node, 'IfStatement') - const hasRequire = traversing.someParent(node, this.isRequire) - const hasAssert = traversing.someParent(node, this.isAssert) + if (this.isNotErc777Token(node)) { + const hasVarDeclaration = traversing.statementNotContains(node, 'VariableDeclaration') + const hasIfStatement = traversing.statementNotContains(node, 'IfStatement') + const hasRequire = traversing.someParent(node, this.isRequire) + const hasAssert = traversing.someParent(node, this.isAssert) - if (!hasIfStatement && !hasVarDeclaration && !hasRequire && !hasAssert) { - this.error(node, 'Check result of "send" call') + if (!hasIfStatement && !hasVarDeclaration && !hasRequire && !hasAssert) { + this.error(node, 'Check result of "send" call') + } } } } + isNotErc777Token(node) { + const isErc777 = node.parent.type === 'FunctionCall' && node.parent.arguments.length >= 3 + return !isErc777 + } + isRequire(node) { return node.type === 'FunctionCall' && node.expression.name === 'require' } diff --git a/scripts/generate-rule-docs.js b/scripts/generate-rule-docs.js index 1e7a02e1..c170bb74 100755 --- a/scripts/generate-rule-docs.js +++ b/scripts/generate-rule-docs.js @@ -1,75 +1,77 @@ #!env node const { loadRules } = require('../lib/load-rules') -const fs = require('fs'); -const { exec, mkdir } = require("shelljs"); -const semver = require('semver'); -const path = require('path'); -const table = require('markdown-table'); -const { ruleSeverityEnum } = require('../lib/doc/utils'); +const fs = require('fs') +const { exec, mkdir } = require('shelljs') +const semver = require('semver') +const path = require('path') +const table = require('markdown-table') +const { ruleSeverityEnum } = require('../lib/doc/utils') /** * Borrowed from https://github.com/eslint/eslint/blob/master/Makefile.js */ class GitHelper { - /** - * Gets the tag name where a given file was introduced first. - * @param {string} filePath The file path to check. - * @returns {string} The tag name. - */ - static getFirstVersionOfFile(filePath) { - const firstCommit = GitHelper.getFirstCommitOfFile(filePath); - let tags = GitHelper.execSilent(`git tag --contains ${firstCommit}`); - - tags = GitHelper.splitCommandResultToLines(tags); - return tags.reduce((list, version) => { - const validatedVersion = semver.valid(version.trim()); - - if (validatedVersion) { - list.push(validatedVersion); - } - return list; - }, []).sort(semver.compare)[0]; - } + /** + * Gets the tag name where a given file was introduced first. + * @param {string} filePath The file path to check. + * @returns {string} The tag name. + */ + static getFirstVersionOfFile(filePath) { + const firstCommit = GitHelper.getFirstCommitOfFile(filePath) + let tags = GitHelper.execSilent(`git tag --contains ${firstCommit}`) + + tags = GitHelper.splitCommandResultToLines(tags) + return tags + .reduce((list, version) => { + const validatedVersion = semver.valid(version.trim()) + + if (validatedVersion) { + list.push(validatedVersion) + } + return list + }, []) + .sort(semver.compare)[0] + } - /** - * Gets the first commit sha of the given file. - * @param {string} filePath The file path which should be checked. - * @returns {string} The commit sha. - */ - static getFirstCommitOfFile(filePath) { - let commits = GitHelper.execSilent(`git rev-list HEAD -- ${filePath}`); + /** + * Gets the first commit sha of the given file. + * @param {string} filePath The file path which should be checked. + * @returns {string} The commit sha. + */ + static getFirstCommitOfFile(filePath) { + let commits = GitHelper.execSilent(`git rev-list HEAD -- ${filePath}`) - commits = GitHelper.splitCommandResultToLines(commits); - return commits[commits.length - 1].trim(); - } + commits = GitHelper.splitCommandResultToLines(commits) + return commits[commits.length - 1].trim() + } - /** - * Splits a command result to separate lines. - * @param {string} result The command result string. - * @returns {Array} The separated lines. - */ - static splitCommandResultToLines(result) { - return result.trim().split("\n"); - } + /** + * Splits a command result to separate lines. + * @param {string} result The command result string. + * @returns {Array} The separated lines. + */ + static splitCommandResultToLines(result) { + return result.trim().split('\n') + } - /** - * Executes a command and returns the output instead of printing it to stdout. - * @param {string} cmd The command string to execute. - * @returns {string} The result of the executed command. - */ - static execSilent(cmd) { - return exec(cmd, {silent: true}).stdout; - } + /** + * Executes a command and returns the output instead of printing it to stdout. + * @param {string} cmd The command string to execute. + * @returns {string} The result of the executed command. + */ + static execSilent(cmd) { + return exec(cmd, { silent: true }).stdout + } } function generateRuleDoc(rule) { - const isDefault = !rule.meta.deprecated && rule.meta.isDefault; - const isRecommended = !rule.meta.deprecated && rule.meta.recommended; - const isDeprecated = rule.meta.deprecated; - const version = GitHelper.getFirstVersionOfFile(rule.file); - const defaultSeverity = getDefaultSeverity(rule); + const isDefault = !rule.meta.deprecated && rule.meta.isDefault + const isRecommended = !rule.meta.deprecated && rule.meta.recommended + const isDeprecated = rule.meta.deprecated + const version = GitHelper.getFirstVersionOfFile(rule.file) + const defaultSeverity = getDefaultSeverity(rule) - return `--- + return `--- warning: "This is a dynamically generated file. Do not edit manually." layout: "default" title: "${rule.ruleId} | Solhint" @@ -77,14 +79,20 @@ title: "${rule.ruleId} | Solhint" # ${rule.ruleId} ${[ - recommendedBadge(isRecommended), - deprecatedBadge(isDeprecated), - categoryBadge(rule.meta.docs.category), - defaultSeverityBadge(defaultSeverity), - isDefault ? '> The {"extends": "solhint:default"} property in a configuration file enables this rule.\n' : '', - isRecommended ? '> The {"extends": "solhint:recommended"} property in a configuration file enables this rule.\n' : '', - isDeprecated ? '> This rule is **deprecated**\n' : '' -].filter(s => s !== '').join("\n")} + recommendedBadge(isRecommended), + deprecatedBadge(isDeprecated), + categoryBadge(rule.meta.docs.category), + defaultSeverityBadge(defaultSeverity), + isDefault + ? '> The {"extends": "solhint:default"} property in a configuration file enables this rule.\n' + : '', + isRecommended + ? '> The {"extends": "solhint:recommended"} property in a configuration file enables this rule.\n' + : '', + isDeprecated ? '> This rule is **deprecated**\n' : '', +] + .filter((s) => s !== '') + .join('\n')} ## Description ${rule.meta.docs.description} @@ -94,7 +102,7 @@ ${loadOptions(rule)} ### Example Config ${loadExampleConfig(rule)} - +${loadNotes(rule)} ## Examples ${loadExamples(rule)} @@ -109,44 +117,48 @@ ${linkToVersion(version)} } function categoryBadge(category) { - return `![Category Badge](https://img.shields.io/badge/-${encodeURIComponent(category)}-informational)`; + return `![Category Badge](https://img.shields.io/badge/-${encodeURIComponent( + category + )}-informational)` } function recommendedBadge(isRecommended) { - return isRecommended ? `![Recommended Badge](https://img.shields.io/badge/-Recommended-brightgreen)` : ''; + return isRecommended + ? `![Recommended Badge](https://img.shields.io/badge/-Recommended-brightgreen)` + : '' } function deprecatedBadge(isDeprecated) { - return isDeprecated ? `![Deprecated Badge](https://img.shields.io/badge/-Deprecated-yellow)` : ''; + return isDeprecated ? `![Deprecated Badge](https://img.shields.io/badge/-Deprecated-yellow)` : '' } function defaultSeverityBadge(severity) { - const colors = { - 'warn': 'yellow', - 'error': 'red' - }; - return `![Default Severity Badge ${severity}](https://img.shields.io/badge/Default%20Severity-${severity}-${colors[severity]})`; + const colors = { + warn: 'yellow', + error: 'red', + } + return `![Default Severity Badge ${severity}](https://img.shields.io/badge/Default%20Severity-${severity}-${colors[severity]})` } function loadOptions(rule) { - if (Array.isArray(rule.meta.defaultSetup)) { - const optionsTable = [['Index', 'Description', 'Default Value']]; - rule.meta.docs.options.forEach((option, index) => { - optionsTable.push([index, option.description, option.default]); - }); - return `This rule accepts an array of options: + if (Array.isArray(rule.meta.defaultSetup)) { + const optionsTable = [['Index', 'Description', 'Default Value']] + rule.meta.docs.options.forEach((option, index) => { + optionsTable.push([index, option.description, option.default]) + }) + return `This rule accepts an array of options: ${table(optionsTable)} -`; - } else if (typeof rule.meta.defaultSetup === 'string') { - return `This rule accepts a string option of rule severity. Must be one of ${ruleSeverityEnum}. Default to ${rule.meta.defaultSetup}.` - } else { - throw new Error(`Unhandled type of rule.meta.defaultSetup from rule ${rule.ruleId}`); - } +` + } else if (typeof rule.meta.defaultSetup === 'string') { + return `This rule accepts a string option of rule severity. Must be one of ${ruleSeverityEnum}. Default to ${rule.meta.defaultSetup}.` + } else { + throw new Error(`Unhandled type of rule.meta.defaultSetup from rule ${rule.ruleId}`) + } } function loadExampleConfig(rule) { - return `\`\`\`json + return `\`\`\`json { "rules": { "${rule.ruleId}": ${JSON.stringify(rule.meta.defaultSetup)} @@ -156,77 +168,109 @@ function loadExampleConfig(rule) { ` } -function linkToVersion(version) { - if (version) { - return `This rule was introduced in [Solhint ${version}](https://github.com/protofire/solhint/tree/v${version})`; - } else { - return `This rule is introduced in the latest version.`; +function loadNotes(rule) { + let textToReturn = '' + let noteValue = '' + if (rule.meta.docs.notes) { + textToReturn = `### Notes\n` + for (let i = 0; i < rule.meta.docs.notes.length; i++) { + noteValue = rule.meta.docs.notes[i].note + textToReturn = textToReturn + `- ${noteValue}\n` } + } + + return textToReturn +} + +function linkToVersion(version) { + if (version) { + return `This rule was introduced in [Solhint ${version}](https://github.com/protofire/solhint/tree/v${version})` + } else { + return `This rule is introduced in the latest version.` + } } function linkToSource(rule) { - const link = rule.file.replace(path.resolve(path.join(__dirname, '..')), ''); - return `https://github.com/protofire/solhint/tree/master${link}`; + const link = rule.file.replace(path.resolve(path.join(__dirname, '..')), '') + return `https://github.com/protofire/solhint/tree/master${link}` } function linkToDocumentSource(rule) { - const link = rule.file.replace(path.resolve(path.join(__dirname, '..')), '').replace("lib/rules", "docs/rules").replace(/\.js$/, ".md"); - return `https://github.com/protofire/solhint/tree/master${link}`; + const link = rule.file + .replace(path.resolve(path.join(__dirname, '..')), '') + .replace('lib/rules', 'docs/rules') + .replace(/\.js$/, '.md') + return `https://github.com/protofire/solhint/tree/master${link}` } function linkToTestCase(rule) { - const link = rule.file.replace(path.resolve(path.join(__dirname, '..', 'lib', 'rules')), ''); - return `https://github.com/protofire/solhint/tree/master/test/rules${link}`; + const link = rule.file.replace(path.resolve(path.join(__dirname, '..', 'lib', 'rules')), '') + return `https://github.com/protofire/solhint/tree/master/test/rules${link}` } function loadExamples(rule) { - if (!rule.meta.docs.examples) { - return "This rule does not have examples."; - } + if (!rule.meta.docs.examples) { + return 'This rule does not have examples.' + } - const examples = [loadCorrectExample(rule), loadIncorrectExample(rule)].filter(s => s !== '').join('\n\n'); - return examples === '' ? 'This rule does not have examples.' : examples; + const examples = [loadCorrectExample(rule), loadIncorrectExample(rule)] + .filter((s) => s !== '') + .join('\n\n') + return examples === '' ? 'This rule does not have examples.' : examples } function loadIncorrectExample(rule) { - if (rule.meta.docs.examples.bad && rule.meta.docs.examples.bad.length) { - return `### 👎 Examples of **incorrect** code for this rule - -${rule.meta.docs.examples.bad.map(ex => `#### ${ex.description}\n\n\`\`\`solidity\n${ex.code}\n\`\`\``).join("\n\n")}`; - } else { - return ''; - } + if (rule.meta.docs.examples.bad && rule.meta.docs.examples.bad.length) { + return `### 👎 Examples of **incorrect** code for this rule + +${rule.meta.docs.examples.bad + .map((ex) => `#### ${ex.description}\n\n\`\`\`solidity\n${ex.code}\n\`\`\``) + .join('\n\n')}` + } else { + return '' + } } function loadCorrectExample(rule) { - if (rule.meta.docs.examples.good && rule.meta.docs.examples.good.length) { - return `### 👍 Examples of **correct** code for this rule - -${rule.meta.docs.examples.good.map(ex => `#### ${ex.description}\n\n\`\`\`solidity\n${ex.code}\n\`\`\``).join("\n\n")}`; - } else { - return ''; - } + if (rule.meta.docs.examples.good && rule.meta.docs.examples.good.length) { + return `### 👍 Examples of **correct** code for this rule + +${rule.meta.docs.examples.good + .map((ex) => `#### ${ex.description}\n\n\`\`\`solidity\n${ex.code}\n\`\`\``) + .join('\n\n')}` + } else { + return '' + } } function getDefaultSeverity(rule) { - if (Array.isArray(rule.meta.defaultSetup)) { - return rule.meta.defaultSetup[0]; - } else { - return rule.meta.defaultSetup; - } + if (Array.isArray(rule.meta.defaultSetup)) { + return rule.meta.defaultSetup[0] + } else { + return rule.meta.defaultSetup + } } function generateRuleIndex(rulesIndexed) { - const contents = Object.keys(rulesIndexed).map(category => { - const rows = [["Rule Id", "Error", "Recommended", "Deprecated"]]; - rulesIndexed[category].map(rule => [`[${rule.ruleId}](./rules/${rule.meta.type}/${rule.ruleId}.md)`, rule.meta.docs.description, (rule.meta.recommended && !rule.meta.deprecated) ? '$~~~~~~~~$✔️' : '', rule.meta.deprecated ? '$~~~~~~~$✔️' : '']).forEach(row => rows.push(row)); - return `## ${category} + const contents = Object.keys(rulesIndexed) + .map((category) => { + const rows = [['Rule Id', 'Error', 'Recommended', 'Deprecated']] + rulesIndexed[category] + .map((rule) => [ + `[${rule.ruleId}](./rules/${rule.meta.type}/${rule.ruleId}.md)`, + rule.meta.docs.description, + rule.meta.recommended && !rule.meta.deprecated ? '$~~~~~~~~$✔️' : '', + rule.meta.deprecated ? '$~~~~~~~$✔️' : '', + ]) + .forEach((row) => rows.push(row)) + return `## ${category} ${table(rows)} - `; - }).join("\n\n"); + ` + }) + .join('\n\n') - return `--- + return `--- warning: "This is a dynamically generated file. Do not edit manually." layout: "default" title: "Rule Index of Solhint" @@ -238,61 +282,63 @@ ${contents} - [ConsenSys Guide for Smart Contracts](https://consensys.github.io/smart-contract-best-practices/recommendations/) - [Solidity Style Guide](http://solidity.readthedocs.io/en/develop/style-guide.html) -`; +` } function main() { - const program = require('commander'); - program.option('--rule-id ', 'rule id'); - program.option('--index-only', 'only generate rule index'); - program.parse(process.argv); - - const rules = loadRules(); - if (!program.indexOnly) { - let ruleIds = []; - if (program.ruleId) { - ruleIds = program.ruleId.split(","); - } - - rules.filter(rule => { - if (ruleIds.length) { - return ruleIds.includes(rule.ruleId); - } else { - return true; - } - }).forEach(rule => { - const ruleDoc = generateRuleDoc(rule); - const dir = path.resolve(path.join(__dirname, '..', 'docs', 'rules', rule.meta.type)); - const fileName = `${rule.ruleId}.md`; - const filePath = path.resolve(path.join(dir, fileName)); - mkdir('-p', dir); - fs.writeFile(filePath, ruleDoc, function (err) { - if (err) { - console.error(err); - } else { - console.log(`Wrote ${filePath}`) - } - }); - }); + const program = require('commander') + program.option('--rule-id ', 'rule id') + program.option('--index-only', 'only generate rule index') + program.parse(process.argv) + + const rules = loadRules() + if (!program.indexOnly) { + let ruleIds = [] + if (program.ruleId) { + ruleIds = program.ruleId.split(',') } - const rulesIndexed = {}; - rules.forEach(rule => { - if (!rulesIndexed[rule.meta.docs.category]) { - rulesIndexed[rule.meta.docs.category] = [rule]; + rules + .filter((rule) => { + if (ruleIds.length) { + return ruleIds.includes(rule.ruleId) } else { - rulesIndexed[rule.meta.docs.category].push(rule); + return true } - }); - const ruleIndexDoc = generateRuleIndex(rulesIndexed); - const ruleIndexDocPath = path.resolve(path.join(__dirname, '..', 'docs', 'rules.md')); - fs.writeFile(ruleIndexDocPath, ruleIndexDoc, function (err) { - if (err) { - console.error(err); - } else { - console.log(`Wrote ${ruleIndexDocPath}`) - } - }); + }) + .forEach((rule) => { + const ruleDoc = generateRuleDoc(rule) + const dir = path.resolve(path.join(__dirname, '..', 'docs', 'rules', rule.meta.type)) + const fileName = `${rule.ruleId}.md` + const filePath = path.resolve(path.join(dir, fileName)) + mkdir('-p', dir) + fs.writeFile(filePath, ruleDoc, function (err) { + if (err) { + console.error(err) + } else { + console.log(`Wrote ${filePath}`) + } + }) + }) + } + + const rulesIndexed = {} + rules.forEach((rule) => { + if (!rulesIndexed[rule.meta.docs.category]) { + rulesIndexed[rule.meta.docs.category] = [rule] + } else { + rulesIndexed[rule.meta.docs.category].push(rule) + } + }) + const ruleIndexDoc = generateRuleIndex(rulesIndexed) + const ruleIndexDocPath = path.resolve(path.join(__dirname, '..', 'docs', 'rules.md')) + fs.writeFile(ruleIndexDocPath, ruleIndexDoc, function (err) { + if (err) { + console.error(err) + } else { + console.log(`Wrote ${ruleIndexDocPath}`) + } + }) } -main(); +main() diff --git a/test/fixtures/security/send-result-checked.js b/test/fixtures/security/send-result-checked.js deleted file mode 100644 index 6b4f3848..00000000 --- a/test/fixtures/security/send-result-checked.js +++ /dev/null @@ -1 +0,0 @@ -module.exports = 'if(x.send(55)) {}' diff --git a/test/fixtures/security/send-result-ignored.js b/test/fixtures/security/send-result-ignored.js deleted file mode 100644 index 040959e6..00000000 --- a/test/fixtures/security/send-result-ignored.js +++ /dev/null @@ -1 +0,0 @@ -module.exports = 'x.send(55);' diff --git a/test/rules/best-practises/custom-errors.js b/test/rules/best-practises/custom-errors.js new file mode 100644 index 00000000..1904a3ad --- /dev/null +++ b/test/rules/best-practises/custom-errors.js @@ -0,0 +1,103 @@ +const assert = require('assert') +const { + assertNoWarnings, + assertNoErrors, + assertErrorMessage, + assertErrorCount, + assertWarnsCount, +} = require('../../common/asserts') +const linter = require('../../../lib/index') +const { funcWith } = require('../../common/contract-builder') + +describe('Linter - custom-errors', () => { + it('should raise error for revert()', () => { + const code = funcWith(`revert();`) + const report = linter.processStr(code, { + rules: { 'custom-errors': 'error' }, + }) + + assertErrorCount(report, 1) + assertErrorMessage(report, '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' }, + }) + + assertErrorCount(report, 1) + assertErrorMessage(report, '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' }, + }) + + assertNoWarnings(report) + assertNoErrors(report) + }) + + 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' }, + }) + + assertNoWarnings(report) + assertNoErrors(report) + }) + + it('should raise error for require', () => { + const code = funcWith(`require(!has(role, account), "Roles: account already has role"); + role.bearer[account] = true;role.bearer[account] = true; + `) + const report = linter.processStr(code, { + rules: { 'custom-errors': 'error' }, + }) + + assertErrorCount(report, 1) + assertErrorMessage(report, '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' }, + }) + assertNoWarnings(report) + assertNoErrors(report) + }) + + it('should raise error for require, revert message and not for revert CustomError() for [recommended] config', () => { + const code = funcWith(`require(!has(role, account), "Roles: account already has role"); + revert("RevertMessage"); + revert CustomError(); + `) + const report = linter.processStr(code, { + extends: 'solhint:recommended', + rules: { 'compiler-version': 'off' }, + }) + + 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') + }) + + it('should NOT raise error for default config', () => { + const code = funcWith(`require(!has(role, account), "Roles: account already has role"); + revert("RevertMessage"); + revert CustomError(); + `) + const report = linter.processStr(code, { + extends: 'solhint:default', + }) + + assertWarnsCount(report, 0) + assertErrorCount(report, 0) + }) +}) diff --git a/test/rules/naming/foundry-test-functions.js b/test/rules/naming/foundry-test-functions.js new file mode 100644 index 00000000..9a0feeb9 --- /dev/null +++ b/test/rules/naming/foundry-test-functions.js @@ -0,0 +1,224 @@ +const assert = require('assert') +const linter = require('../../../lib/index') +const contractWith = require('../../common/contract-builder').contractWith +const { assertErrorCount, assertNoErrors, assertErrorMessage } = require('../../common/asserts') + +const ALLOWED_FUNCTION_NAMES = [ + 'test_', + 'testFork_', + 'testFuzz_', + 'testFail_', + 'test_Revert_', + 'test_If_', + 'test_When_', + 'testFail_Revert_', + 'testFail_If_', + 'testFail_When_', + 'testFork_Revert_', + 'testFork_If_', + 'testFork_When_', + 'testFuzz_Revert_', + 'testFuzz_If_', + 'testFuzz_When_', +] + +const DISALLOWED_FUNCTION_NAMES = ['Test_', 'Test', 'test', 'testo_', '', 'any', 'setUp', 'other'] + +const composeFunctionName = (prefix, name, visibility) => + 'function ' + prefix + name + ' ' + visibility + ' { testNumber = 42; }' + +describe('Linter - foundry-test-functions', () => { + for (const prefix of DISALLOWED_FUNCTION_NAMES) { + it(`should raise error for DISALLOWED_FUNCTION_NAMES [${prefix}] when PUBLIC`, () => { + const functionDefinition = composeFunctionName(prefix, 'FunctionName()', 'public') + const code = contractWith(functionDefinition) + + const report = linter.processStr(code, { + rules: { 'foundry-test-functions': ['error', ['setUp', 'finish']] }, + }) + + assertErrorCount(report, 1) + assertErrorMessage( + report, + `Function ${prefix + 'FunctionName()'} must match Foundry test naming convention` + ) + }) + } + + for (const prefix of DISALLOWED_FUNCTION_NAMES) { + it(`should NOT raise error for DISALLOWED_FUNCTION_NAMES [${prefix}] when INTERNAL`, () => { + const functionDefinition = composeFunctionName(prefix, 'FunctionName()', 'internal') + const code = contractWith(functionDefinition) + + const report = linter.processStr(code, { + rules: { 'foundry-test-functions': ['error', ['setUp', 'finish']] }, + }) + + assertNoErrors(report) + }) + } + + for (const prefix of ALLOWED_FUNCTION_NAMES) { + it(`should NOT raise error for ALLOWED_FUNCTION_NAMES [${prefix}] when PUBLIC`, () => { + const functionDefinition = composeFunctionName(prefix, 'FunctionName()', 'public') + const code = contractWith(functionDefinition) + + const report = linter.processStr(code, { + rules: { 'foundry-test-functions': ['error', ['setUp', 'finish']] }, + }) + + assertNoErrors(report) + }) + } + + for (const prefix of ALLOWED_FUNCTION_NAMES) { + it(`should NOT raise error for ALLOWED_FUNCTION_NAMES [${prefix}] when EXTERNAL`, () => { + const functionDefinition = composeFunctionName(prefix, 'FunctionName()', 'external') + const code = contractWith(functionDefinition) + + const report = linter.processStr(code, { + rules: { 'foundry-test-functions': ['error', ['setUp', 'finish']] }, + }) + + assertNoErrors(report) + }) + } + + for (const prefix of ALLOWED_FUNCTION_NAMES) { + it(`should NOT raise error for ALLOWED_FUNCTION_NAMES [${prefix}] when INTERNAL`, () => { + const functionDefinition = composeFunctionName(prefix, 'FunctionName()', 'external') + const code = contractWith(functionDefinition) + + const report = linter.processStr(code, { + rules: { 'foundry-test-functions': ['error', ['setUp', 'finish']] }, + }) + + assertNoErrors(report) + }) + } + + it(`should NOT raise error for setUp function, since is confired as SKIPPED`, () => { + const code = contractWith( + 'function setUp() public { testNumber = 42; } function finish() public { testNumber = 42; }' + ) + + const report = linter.processStr(code, { + rules: { 'foundry-test-functions': ['error', ['setUp', 'finish']] }, + }) + + assertNoErrors(report) + }) + + it(`should NOT raise error for setUp and finish functions but RAISE for the other two functions`, () => { + const code = contractWith(` + function setUp() public { testNumber = 42; } + function finish() public { testNumber = 43; } + function invalidFunction1() external { testNumber = 44; } + function invalidFunction2() external { testNumber = 45; }`) + + const report = linter.processStr(code, { + rules: { 'foundry-test-functions': ['error', ['setUp', 'finish']] }, + }) + + assertErrorCount(report, 2) + assert.equal( + report.reports[0].message, + `Function invalidFunction1() must match Foundry test naming convention` + ) + assert.equal( + report.reports[1].message, + 'Function invalidFunction2() must match Foundry test naming convention' + ) + }) + + it('should NOT raise error when recommended rules are configured', () => { + const code = contractWith(` + function setUp() public { testNumber = 42; } + function finish() public { testNumber = 43; } + function invalidFunction1() external { testNumber = 44; } + function invalidFunction2() external { testNumber = 45; }`) + + const report = linter.processStr(code, { + extends: 'solhint:recommended', + rules: { 'compiler-version': 'off' }, + }) + + assertNoErrors(report) + }) + + it('should raise 2 errors when all rules are configured and setUp is skipped', () => { + const code = contractWith(` + function setUp() public { testNumber = 42; } + function invalidFunction1() external { testNumber = 44; } + function invalidFunction2() external { testNumber = 45; }`) + + const report = linter.processStr(code, { + extends: 'solhint:recommended', + rules: { + 'compiler-version': 'off', + 'foundry-test-functions': ['error', ['setUp', 'finish']], + }, + }) + + assertErrorCount(report, 2) + assert.equal( + report.reports[0].message, + `Function invalidFunction1() must match Foundry test naming convention` + ) + assert.equal( + report.reports[1].message, + 'Function invalidFunction2() must match Foundry test naming convention' + ) + }) + + it(`should NOT raise error only for setUp when rule is just on 'error'`, () => { + const code = contractWith(` + function setUp() public { testNumber = 42; } + function finish() public { testNumber = 43; } + function invalidFunction1() external { testNumber = 44; } + function invalidFunction2() external { testNumber = 45; }`) + + const report = linter.processStr(code, { + rules: { 'foundry-test-functions': 'error' }, + }) + + assertErrorCount(report, 3) + assert.equal( + report.reports[0].message, + 'Function finish() must match Foundry test naming convention' + ) + assert.equal( + report.reports[1].message, + `Function invalidFunction1() must match Foundry test naming convention` + ) + assert.equal( + report.reports[2].message, + 'Function invalidFunction2() must match Foundry test naming convention' + ) + }) + + it(`should raise error for all functions when rule SKIP array is empty`, () => { + const code = contractWith(` + function setUp() public { testNumber = 42; } + function finish() public { testNumber = 43; } + function invalidFunction() external { testNumber = 44; }`) + + const report = linter.processStr(code, { + rules: { 'foundry-test-functions': ['error', []] }, + }) + + assertErrorCount(report, 3) + assert.equal( + report.reports[0].message, + 'Function setUp() must match Foundry test naming convention' + ) + assert.equal( + report.reports[1].message, + `Function finish() must match Foundry test naming convention` + ) + assert.equal( + report.reports[2].message, + 'Function invalidFunction() must match Foundry test naming convention' + ) + }) +}) diff --git a/test/rules/security/check-send-result.js b/test/rules/security/check-send-result.js index c2d75d8b..69f0875d 100644 --- a/test/rules/security/check-send-result.js +++ b/test/rules/security/check-send-result.js @@ -4,7 +4,7 @@ const funcWith = require('../../common/contract-builder').funcWith describe('Linter - check-send-result', () => { it('should return "send" call verification error', () => { - const code = funcWith(require('../../fixtures/security/send-result-ignored')) + const code = funcWith('x.send(55);') const report = linter.processStr(code, { rules: { 'check-send-result': 'error' }, @@ -15,7 +15,7 @@ describe('Linter - check-send-result', () => { }) it('should not return "send" call verification error', () => { - const code = funcWith(require('../../fixtures/security/send-result-checked')) + const code = funcWith('if(x.send(55)) {}') const report = linter.processStr(code, { rules: { 'check-send-result': 'error' }, @@ -73,4 +73,14 @@ describe('Linter - check-send-result', () => { assert.equal(report.errorCount, 1) }) + + it('should not emit error when the send() is used for an ERC777', () => { + const code = funcWith('erc777.send(recipient, amount, "");') + + const report = linter.processStr(code, { + rules: { 'check-send-result': 'error' }, + }) + + assert.equal(report.errorCount, 0) + }) })