From 6c08f86fadf16fdc1534759a4253e657bcbc0b0c Mon Sep 17 00:00:00 2001 From: Luiz Hemerly Date: Wed, 22 Mar 2023 23:21:54 -0300 Subject: [PATCH 1/7] Add custom linting rules Fixes #3919 Add solhint-plugin-ozcontracts to devDependencies Add ozonctracts to .solhint.json plugins Add oz-contracts-custom to .solhint.json rules #### PR Checklist - [ x] Tests - [ ] Documentation - [ ] Changeset entry (run `npx changeset add`) --- .solhint.json | 8 ++++++-- package.json | 3 ++- 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/.solhint.json b/.solhint.json index 772928849e0..680ddc1c1d6 100644 --- a/.solhint.json +++ b/.solhint.json @@ -1,4 +1,7 @@ { + "plugins": [ + "ozcontracts" + ], "rules": { "no-unused-vars": "error", "const-name-snakecase": "error", @@ -9,6 +12,7 @@ "modifier-name-mixedcase": "error", "private-vars-leading-underscore": "error", "var-name-mixedcase": "error", - "imports-on-top": "error" + "imports-on-top": "error", + "ozcontracts/oz-contracts-custom": "error" } -} +} \ No newline at end of file diff --git a/package.json b/package.json index 56798f78859..b190ccde944 100644 --- a/package.json +++ b/package.json @@ -86,10 +86,11 @@ "rimraf": "^3.0.2", "semver": "^7.3.5", "solhint": "^3.3.6", + "solhint-plugin-ozcontracts": "1.0.2", "solidity-ast": "^0.4.25", "solidity-coverage": "^0.8.0", "solidity-docgen": "^0.6.0-beta.29", "web3": "^1.3.0", "yargs": "^17.0.0" } -} +} \ No newline at end of file From 7e1fe6d3338758a3be2e2a6b175c23c640545912 Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Wed, 5 Jul 2023 22:45:40 -0300 Subject: [PATCH 2/7] vendor plugin code --- .solhint.json | 6 +- package-lock.json | 18 +++++ package.json | 4 +- scripts/solhint-custom/index.js | 103 ++++++++++++++++++++++++++++ scripts/solhint-custom/package.json | 4 ++ 5 files changed, 130 insertions(+), 5 deletions(-) create mode 100644 scripts/solhint-custom/index.js create mode 100644 scripts/solhint-custom/package.json diff --git a/.solhint.json b/.solhint.json index 680ddc1c1d6..dc1a97a1eba 100644 --- a/.solhint.json +++ b/.solhint.json @@ -1,6 +1,6 @@ { "plugins": [ - "ozcontracts" + "openzeppelin" ], "rules": { "no-unused-vars": "error", @@ -13,6 +13,6 @@ "private-vars-leading-underscore": "error", "var-name-mixedcase": "error", "imports-on-top": "error", - "ozcontracts/oz-contracts-custom": "error" + "openzeppelin/custom": "error" } -} \ No newline at end of file +} diff --git a/package-lock.json b/package-lock.json index f44a1b939a0..738efe7a5eb 100644 --- a/package-lock.json +++ b/package-lock.json @@ -45,6 +45,7 @@ "rimraf": "^3.0.2", "semver": "^7.3.5", "solhint": "^3.3.6", + "solhint-plugin-openzeppelin": "file:scripts/solhint-custom", "solidity-ast": "^0.4.25", "solidity-coverage": "^0.8.0", "solidity-docgen": "^0.6.0-beta.29", @@ -12312,6 +12313,10 @@ "prettier": "^2.8.3" } }, + "node_modules/solhint-plugin-openzeppelin": { + "resolved": "scripts/solhint-custom", + "link": true + }, "node_modules/solhint/node_modules/@solidity-parser/parser": { "version": "0.16.0", "resolved": "https://registry.npmjs.org/@solidity-parser/parser/-/parser-0.16.0.tgz", @@ -15272,6 +15277,16 @@ "funding": { "url": "https://github.com/sponsors/sindresorhus" } + }, + "scripts/lints": { + "version": "1.0.2", + "extraneous": true, + "license": "MIT" + }, + "scripts/solhint-custom": { + "version": "1.0.2", + "dev": true, + "license": "MIT" } }, "dependencies": { @@ -24878,6 +24893,9 @@ } } }, + "solhint-plugin-openzeppelin": { + "version": "file:scripts/solhint-custom" + }, "solidity-ast": { "version": "0.4.46", "resolved": "https://registry.npmjs.org/solidity-ast/-/solidity-ast-0.4.46.tgz", diff --git a/package.json b/package.json index b190ccde944..649108219c6 100644 --- a/package.json +++ b/package.json @@ -86,11 +86,11 @@ "rimraf": "^3.0.2", "semver": "^7.3.5", "solhint": "^3.3.6", - "solhint-plugin-ozcontracts": "1.0.2", + "solhint-plugin-openzeppelin": "file:scripts/solhint-custom", "solidity-ast": "^0.4.25", "solidity-coverage": "^0.8.0", "solidity-docgen": "^0.6.0-beta.29", "web3": "^1.3.0", "yargs": "^17.0.0" } -} \ No newline at end of file +} diff --git a/scripts/solhint-custom/index.js b/scripts/solhint-custom/index.js new file mode 100644 index 00000000000..50db0b206c7 --- /dev/null +++ b/scripts/solhint-custom/index.js @@ -0,0 +1,103 @@ +class OpenZeppelinCustom { + constructor(reporter, config) { + this.ruleId = 'custom'; + + this.reporter = reporter; + this.config = config; + } + + VariableDeclaration(node) { + if (!node.isStateVar) { + return; + } + + if (node.visibility != 'private') { + this.reporter.error( + node, + this.ruleId, + 'State variables must be private.', + fixer => { + return fixer.replaceTextRange( + [node.typeName.range[1] + 1, node.identifier.range[0] - 1], + ' private ' + ); + } + ); + if (node.name[0] != '_') { + this.reporter.error( + node, + this.ruleId, + 'Private and internal variables must be prefixed with underscore.', + fixer => { + return fixer.replaceTextRange( + node.identifier.range, + '_' + node.identifier.name + ); + } + ); + } + } + + if ( + (node.visibility == 'private' || node.visibility == 'internal') && + node.name[0] != '_' + ) { + this.reporter.error( + node, + this.ruleId, + 'Private and internal variables must be prefixed with underscore.', + fixer => { + return fixer.replaceTextRange( + node.identifier.range, + '_' + node.identifier.name + ); + } + ); + } + } + + FunctionDefinition(node) { + if (node.visibility == 'external' && node.isVirtual) { + this.reporter.error( + node, + this.ruleId, + "Functions can't be external and virtual." + ); + } + + if ( + (node.visibility == 'internal' || node.visibility == 'private') && + node.name[0] != '_' + ) { + this.reporter.error( + node, + this.ruleId, + 'Private and internal functions must be prefixed with underscore.', + fixer => { + return fixer.replaceTextRange( + [node.range[0] + 9, node.range[0] + 8 + node.name.length], + '_' + node.name + ); + } + ); + } + } + + ContractDefinition(node) { + if (node.kind == 'interface' && node.name[0] != 'I') { + this.reporter.error( + node, + this.ruleId, + 'Interfaces names should have a capital I prefix.', + fixer => { + return fixer.replaceTextRange( + [node.range[0] + 10, node.range[0] + 10 + node.name.length], + 'I' + node.name + ' ' + ); + } + ); + } + } +} + +module.exports = [OpenZeppelinCustom]; diff --git a/scripts/solhint-custom/package.json b/scripts/solhint-custom/package.json new file mode 100644 index 00000000000..d91e327a4cc --- /dev/null +++ b/scripts/solhint-custom/package.json @@ -0,0 +1,4 @@ +{ + "name": "solhint-plugin-openzeppelin", + "private": true +} From b3e6d7986678ce93e904308096c2eb85d9538ee8 Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Thu, 6 Jul 2023 00:08:48 -0300 Subject: [PATCH 3/7] refactor and improve rules --- .solhint.json | 18 ---- scripts/solhint-custom/index.js | 155 +++++++++++++++----------------- solhint.config.js | 19 ++++ 3 files changed, 89 insertions(+), 103 deletions(-) delete mode 100644 .solhint.json create mode 100644 solhint.config.js diff --git a/.solhint.json b/.solhint.json deleted file mode 100644 index dc1a97a1eba..00000000000 --- a/.solhint.json +++ /dev/null @@ -1,18 +0,0 @@ -{ - "plugins": [ - "openzeppelin" - ], - "rules": { - "no-unused-vars": "error", - "const-name-snakecase": "error", - "contract-name-camelcase": "error", - "event-name-camelcase": "error", - "func-name-mixedcase": "error", - "func-param-name-mixedcase": "error", - "modifier-name-mixedcase": "error", - "private-vars-leading-underscore": "error", - "var-name-mixedcase": "error", - "imports-on-top": "error", - "openzeppelin/custom": "error" - } -} diff --git a/scripts/solhint-custom/index.js b/scripts/solhint-custom/index.js index 50db0b206c7..c98d99ae2a5 100644 --- a/scripts/solhint-custom/index.js +++ b/scripts/solhint-custom/index.js @@ -1,103 +1,88 @@ -class OpenZeppelinCustom { - constructor(reporter, config) { - this.ruleId = 'custom'; +const path = require('path'); +const minimatch = require('minimatch'); +// Files matching these patterns will be ignored unless a rule has `static global = true` +const ignore = [ + 'contracts/mocks/**/*', + 'test/**/*', +]; + +class Base { + constructor(reporter, config, source, fileName) { this.reporter = reporter; - this.config = config; + this.ignored = this.constructor.global || ignore.some(p => minimatch(path.normalize(fileName), p)); + this.ruleId = this.constructor.ruleId; + if (this.ruleId === undefined) { + throw Error('missing ruleId static property'); + } } - VariableDeclaration(node) { - if (!node.isStateVar) { - return; + error(node, message) { + if (!this.ignored) { + this.reporter.error(node, this.ruleId, message); } + } +} - if (node.visibility != 'private') { - this.reporter.error( - node, - this.ruleId, - 'State variables must be private.', - fixer => { - return fixer.replaceTextRange( - [node.typeName.range[1] + 1, node.identifier.range[0] - 1], - ' private ' - ); - } - ); - if (node.name[0] != '_') { - this.reporter.error( - node, - this.ruleId, - 'Private and internal variables must be prefixed with underscore.', - fixer => { - return fixer.replaceTextRange( - node.identifier.range, - '_' + node.identifier.name - ); - } - ); +module.exports = [ + class extends Base { + static ruleId = 'interface-names'; + + ContractDefinition(node, ...args) { + if (node.kind === 'interface' && !/^I[A-Z]/.test(node.name)) { + this.error(node, 'Interface names should have a capital I prefix'); } } + }, - if ( - (node.visibility == 'private' || node.visibility == 'internal') && - node.name[0] != '_' - ) { - this.reporter.error( - node, - this.ruleId, - 'Private and internal variables must be prefixed with underscore.', - fixer => { - return fixer.replaceTextRange( - node.identifier.range, - '_' + node.identifier.name - ); - } - ); - } - } + class extends Base { + static ruleId = 'private-variables'; - FunctionDefinition(node) { - if (node.visibility == 'external' && node.isVirtual) { - this.reporter.error( - node, - this.ruleId, - "Functions can't be external and virtual." - ); + VariableDeclaration(node) { + const constantOrImmutable = node.isDeclaredConst || node.isImmutable; + if (node.isStateVar && !constantOrImmutable && node.visibility !== 'private') { + this.error(node, 'State variables must be private'); + } } + }, + + class extends Base { + static ruleId = 'underscore-prefix'; - if ( - (node.visibility == 'internal' || node.visibility == 'private') && - node.name[0] != '_' - ) { - this.reporter.error( - node, - this.ruleId, - 'Private and internal functions must be prefixed with underscore.', - fixer => { - return fixer.replaceTextRange( - [node.range[0] + 9, node.range[0] + 8 + node.name.length], - '_' + node.name - ); + VariableDeclaration(node) { + const constantOrImmutable = node.isDeclaredConst || node.isImmutable; + if (node.isStateVar && !constantOrImmutable && node.visibility === 'private') { + if (!/^_/.test(node.name)) { + this.error(node, 'Private variables must be prefixed with underscore'); } - ); + } } - } - ContractDefinition(node) { - if (node.kind == 'interface' && node.name[0] != 'I') { - this.reporter.error( - node, - this.ruleId, - 'Interfaces names should have a capital I prefix.', - fixer => { - return fixer.replaceTextRange( - [node.range[0] + 10, node.range[0] + 10 + node.name.length], - 'I' + node.name + ' ' - ); + FunctionDefinition(node) { + if ( + node.visibility === 'private' || + (node.visibility === 'internal' && node.parent.kind !== 'library') + ) { + if (!/^_/.test(node.name)) { + this.error(node, 'Private and internal functions must be prefixed with underscore'); } - ); + } + if (node.visibility === 'internal' && node.parent.kind === 'library') { + if (/^_/.test(node.name)) { + this.error(node, 'Library internal functions should not be prefixed with underscore'); + } + } } - } -} + }, -module.exports = [OpenZeppelinCustom]; + // TODO: re-enable and fix + // class extends Base { + // static ruleId = 'no-external-virtual'; + // + // FunctionDefinition(node) { + // if (node.visibility == 'external' && node.isVirtual) { + // this.error(node, 'Functions should not be external and virtual'); + // } + // } + // }, +]; diff --git a/solhint.config.js b/solhint.config.js new file mode 100644 index 00000000000..35a1aeaf1e2 --- /dev/null +++ b/solhint.config.js @@ -0,0 +1,19 @@ +const customRules = require('./scripts/solhint-custom').map(r => r.ruleId); + +const rules = [ + 'no-unused-vars', + 'const-name-snakecase', + 'contract-name-camelcase', + 'event-name-camelcase', + 'func-name-mixedcase', + 'func-param-name-mixedcase', + 'modifier-name-mixedcase', + 'var-name-mixedcase', + 'imports-on-top', + ...customRules.map(r => `openzeppelin/${r}`), +]; + +module.exports = { + plugins: ['openzeppelin'], + rules: Object.fromEntries(rules.map(r => [r, 'error'])), +}; From 9c7dc1970654ed8a40e86fc898073d3fea615bc5 Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Thu, 6 Jul 2023 00:17:09 -0300 Subject: [PATCH 4/7] lint --- scripts/solhint-custom/index.js | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-) diff --git a/scripts/solhint-custom/index.js b/scripts/solhint-custom/index.js index c98d99ae2a5..7c326c10d69 100644 --- a/scripts/solhint-custom/index.js +++ b/scripts/solhint-custom/index.js @@ -2,10 +2,7 @@ const path = require('path'); const minimatch = require('minimatch'); // Files matching these patterns will be ignored unless a rule has `static global = true` -const ignore = [ - 'contracts/mocks/**/*', - 'test/**/*', -]; +const ignore = ['contracts/mocks/**/*', 'test/**/*']; class Base { constructor(reporter, config, source, fileName) { @@ -59,10 +56,7 @@ module.exports = [ } FunctionDefinition(node) { - if ( - node.visibility === 'private' || - (node.visibility === 'internal' && node.parent.kind !== 'library') - ) { + if (node.visibility === 'private' || (node.visibility === 'internal' && node.parent.kind !== 'library')) { if (!/^_/.test(node.name)) { this.error(node, 'Private and internal functions must be prefixed with underscore'); } From 8338d2ae022bd68be11f645abd075fafd22c814c Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Thu, 6 Jul 2023 00:19:46 -0300 Subject: [PATCH 5/7] lint --- scripts/solhint-custom/index.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scripts/solhint-custom/index.js b/scripts/solhint-custom/index.js index 7c326c10d69..13197e79a43 100644 --- a/scripts/solhint-custom/index.js +++ b/scripts/solhint-custom/index.js @@ -25,7 +25,7 @@ module.exports = [ class extends Base { static ruleId = 'interface-names'; - ContractDefinition(node, ...args) { + ContractDefinition(node) { if (node.kind === 'interface' && !/^I[A-Z]/.test(node.name)) { this.error(node, 'Interface names should have a capital I prefix'); } From 2e54ac284e13033f24f66f8560e701939258798d Mon Sep 17 00:00:00 2001 From: Francisco Date: Thu, 6 Jul 2023 10:43:45 -0300 Subject: [PATCH 6/7] Update solhint.config.js Co-authored-by: Hadrien Croubois --- solhint.config.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/solhint.config.js b/solhint.config.js index 05f04ab3b76..123ff91fa2d 100644 --- a/solhint.config.js +++ b/solhint.config.js @@ -1,4 +1,4 @@ -const customRules = require('./scripts/solhint-custom').map(r => r.ruleId); +const customRules = require('./scripts/solhint-custom'); const rules = [ 'no-unused-vars', @@ -11,7 +11,7 @@ const rules = [ 'var-name-mixedcase', 'imports-on-top', 'no-global-import', - ...customRules.map(r => `openzeppelin/${r}`), + ...customRules.map(r => `openzeppelin/${r.ruleId}`), ]; module.exports = { From 276dbae7c93b3f1c438f856f6db46b3b3099fced Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Thu, 6 Jul 2023 16:18:07 -0300 Subject: [PATCH 7/7] adjust leading underscore rule --- scripts/solhint-custom/index.js | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/scripts/solhint-custom/index.js b/scripts/solhint-custom/index.js index 13197e79a43..a4cba1a93cf 100644 --- a/scripts/solhint-custom/index.js +++ b/scripts/solhint-custom/index.js @@ -44,26 +44,28 @@ module.exports = [ }, class extends Base { - static ruleId = 'underscore-prefix'; + static ruleId = 'leading-underscore'; VariableDeclaration(node) { - const constantOrImmutable = node.isDeclaredConst || node.isImmutable; - if (node.isStateVar && !constantOrImmutable && node.visibility === 'private') { - if (!/^_/.test(node.name)) { - this.error(node, 'Private variables must be prefixed with underscore'); + if (node.isDeclaredConst) { + if (/^_/.test(node.name)) { + // TODO: re-enable and fix + // this.error(node, 'Constant variables should not have leading underscore'); } + } else if (node.visibility === 'private' && !/^_/.test(node.name)) { + this.error(node, 'Non-constant private variables must have leading underscore'); } } FunctionDefinition(node) { if (node.visibility === 'private' || (node.visibility === 'internal' && node.parent.kind !== 'library')) { if (!/^_/.test(node.name)) { - this.error(node, 'Private and internal functions must be prefixed with underscore'); + this.error(node, 'Private and internal functions must have leading underscore'); } } if (node.visibility === 'internal' && node.parent.kind === 'library') { if (/^_/.test(node.name)) { - this.error(node, 'Library internal functions should not be prefixed with underscore'); + this.error(node, 'Library internal functions should not have leading underscore'); } } }