From d9eb5feb8da2b23d19f7d5d260d54a6268c99f9c Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Thu, 17 Jun 2021 22:58:39 +0200 Subject: [PATCH 1/7] add a lint:inheritance command to check the inheritance tree --- package-lock.json | 132 ++++++++++++++++++++++++++++++++- package.json | 5 +- scripts/inheritanceOrdering.js | 43 +++++++++++ 3 files changed, 177 insertions(+), 3 deletions(-) create mode 100644 scripts/inheritanceOrdering.js diff --git a/package-lock.json b/package-lock.json index 96182c370e0..c6c81288e24 100644 --- a/package-lock.json +++ b/package-lock.json @@ -27,6 +27,7 @@ "eth-sig-util": "^3.0.0", "ethereumjs-util": "^7.0.7", "ethereumjs-wallet": "^1.0.1", + "graphlib": "^2.1.8", "hardhat": "^2.0.6", "hardhat-gas-reporter": "^1.0.4", "keccak256": "^1.0.2", @@ -38,6 +39,7 @@ "prettier-plugin-solidity": "^1.0.0-beta.13", "rimraf": "^3.0.2", "solhint": "^3.3.6", + "solidity-ast": "^0.4.25", "solidity-coverage": "^0.7.11", "solidity-docgen": "^0.5.3", "web3": "^1.3.0", @@ -7713,7 +7715,104 @@ "bundleDependencies": [ "source-map-support", "yargs", - "ethereumjs-util" + "ethereumjs-util", + "@types/bn.js", + "@types/node", + "@types/pbkdf2", + "@types/secp256k1", + "ansi-regex", + "ansi-styles", + "base-x", + "blakejs", + "bn.js", + "brorand", + "browserify-aes", + "bs58", + "bs58check", + "buffer-from", + "buffer-xor", + "camelcase", + "cipher-base", + "cliui", + "color-convert", + "color-name", + "create-hash", + "create-hmac", + "cross-spawn", + "decamelize", + "elliptic", + "emoji-regex", + "end-of-stream", + "ethereum-cryptography", + "ethjs-util", + "evp_bytestokey", + "execa", + "find-up", + "get-caller-file", + "get-stream", + "hash-base", + "hash.js", + "hmac-drbg", + "inherits", + "invert-kv", + "is-fullwidth-code-point", + "is-hex-prefixed", + "is-stream", + "isexe", + "keccak", + "lcid", + "locate-path", + "map-age-cleaner", + "md5.js", + "mem", + "mimic-fn", + "minimalistic-assert", + "minimalistic-crypto-utils", + "nice-try", + "node-addon-api", + "node-gyp-build", + "npm-run-path", + "once", + "os-locale", + "p-defer", + "p-finally", + "p-is-promise", + "p-limit", + "p-locate", + "p-try", + "path-exists", + "path-key", + "pbkdf2", + "pump", + "randombytes", + "readable-stream", + "require-directory", + "require-main-filename", + "ripemd160", + "rlp", + "safe-buffer", + "scrypt-js", + "secp256k1", + "semver", + "set-blocking", + "setimmediate", + "sha.js", + "shebang-command", + "shebang-regex", + "signal-exit", + "source-map", + "string_decoder", + "string-width", + "strip-ansi", + "strip-eof", + "strip-hex-prefix", + "util-deprecate", + "which", + "which-module", + "wrap-ansi", + "wrappy", + "y18n", + "yargs-parser" ], "dev": true, "dependencies": { @@ -9040,6 +9139,15 @@ "integrity": "sha512-nTnJ528pbqxYanhpDYsi4Rd8MAeaBA67+RZ10CM1m3bTAVFEDcd5AuA4a6W5YkGZ1iNXHzZz8T6TBKLeBuNriQ==", "dev": true }, + "node_modules/graphlib": { + "version": "2.1.8", + "resolved": "https://registry.npmjs.org/graphlib/-/graphlib-2.1.8.tgz", + "integrity": "sha512-jcLLfkpoVGmH7/InMC/1hIvOPSUh38oJtGhvrOFGzioE1DZ+0YW16RgmOJhHiuWTvGiJQ9Z1Ik43JvkRPRvE+A==", + "dev": true, + "dependencies": { + "lodash": "^4.17.15" + } + }, "node_modules/growl": { "version": "1.10.5", "resolved": "https://registry.npmjs.org/growl/-/growl-1.10.5.tgz", @@ -15965,6 +16073,12 @@ "node": ">=4" } }, + "node_modules/solidity-ast": { + "version": "0.4.25", + "resolved": "https://registry.npmjs.org/solidity-ast/-/solidity-ast-0.4.25.tgz", + "integrity": "sha512-8IpweS/vgHEpKvY4l0sfr3EsHk+JFIzRWqq/0JefRjzP/Wyi2xZYfx8aHlJ9kcEn2M2RCQK9PexuZ+scaa83OQ==", + "dev": true + }, "node_modules/solidity-comments-extractor": { "version": "0.0.7", "resolved": "https://registry.npmjs.org/solidity-comments-extractor/-/solidity-comments-extractor-0.0.7.tgz", @@ -20678,7 +20792,6 @@ "integrity": "sha512-5vwpq6kbvwkQwKqAoOU3L72GZ3Ta8RRrewKj9OJRolx28KLJJ8Dg9Rf7obRwt5jQA9bkYd8gqzMTrI7H3xLfaw==", "dev": true, "requires": { - "@oclif/config": "^1.15.1", "@oclif/errors": "^1.3.3", "@oclif/parser": "^3.8.3", "@oclif/plugin-help": "^3", @@ -27034,6 +27147,15 @@ "integrity": "sha512-nTnJ528pbqxYanhpDYsi4Rd8MAeaBA67+RZ10CM1m3bTAVFEDcd5AuA4a6W5YkGZ1iNXHzZz8T6TBKLeBuNriQ==", "dev": true }, + "graphlib": { + "version": "2.1.8", + "resolved": "https://registry.npmjs.org/graphlib/-/graphlib-2.1.8.tgz", + "integrity": "sha512-jcLLfkpoVGmH7/InMC/1hIvOPSUh38oJtGhvrOFGzioE1DZ+0YW16RgmOJhHiuWTvGiJQ9Z1Ik43JvkRPRvE+A==", + "dev": true, + "requires": { + "lodash": "^4.17.15" + } + }, "growl": { "version": "1.10.5", "resolved": "https://registry.npmjs.org/growl/-/growl-1.10.5.tgz", @@ -32535,6 +32657,12 @@ } } }, + "solidity-ast": { + "version": "0.4.25", + "resolved": "https://registry.npmjs.org/solidity-ast/-/solidity-ast-0.4.25.tgz", + "integrity": "sha512-8IpweS/vgHEpKvY4l0sfr3EsHk+JFIzRWqq/0JefRjzP/Wyi2xZYfx8aHlJ9kcEn2M2RCQK9PexuZ+scaa83OQ==", + "dev": true + }, "solidity-comments-extractor": { "version": "0.0.7", "resolved": "https://registry.npmjs.org/solidity-comments-extractor/-/solidity-comments-extractor-0.0.7.tgz", diff --git a/package.json b/package.json index 40fa5696306..a6607f96a19 100644 --- a/package.json +++ b/package.json @@ -16,12 +16,13 @@ "docs": "oz-docs", "docs:watch": "npm run docs watch contracts 'docs/*.hbs' docs/helpers.js", "prepare-docs": "scripts/prepare-docs.sh", - "lint": "npm run lint:js && npm run lint:sol", + "lint": "npm run lint:js && npm run lint:sol && npm run lint:inheritance", "lint:fix": "npm run lint:js:fix && npm run lint:sol:fix", "lint:js": "eslint --ignore-path .gitignore .", "lint:js:fix": "eslint --ignore-path .gitignore . --fix", "lint:sol": "solhint 'contracts/**/*.sol' && prettier -c 'contracts/**/*.sol'", "lint:sol:fix": "prettier --write \"contracts/**/*.sol\"", + "lint:inheritance": "node scripts/inheritanceOrdering artifacts/build-info/*", "prepublish": "rimraf build contracts/build artifacts cache", "prepare": "env COMPILE_MODE=production npm run compile", "prepack": "scripts/prepack.sh", @@ -64,6 +65,7 @@ "eth-sig-util": "^3.0.0", "ethereumjs-util": "^7.0.7", "ethereumjs-wallet": "^1.0.1", + "graphlib": "^2.1.8", "hardhat": "^2.0.6", "hardhat-gas-reporter": "^1.0.4", "keccak256": "^1.0.2", @@ -75,6 +77,7 @@ "prettier-plugin-solidity": "^1.0.0-beta.13", "rimraf": "^3.0.2", "solhint": "^3.3.6", + "solidity-ast": "^0.4.25", "solidity-coverage": "^0.7.11", "solidity-docgen": "^0.5.3", "web3": "^1.3.0", diff --git a/scripts/inheritanceOrdering.js b/scripts/inheritanceOrdering.js new file mode 100644 index 00000000000..6034b23f1e0 --- /dev/null +++ b/scripts/inheritanceOrdering.js @@ -0,0 +1,43 @@ +const path = require('path'); +const { Graph } = require('graphlib'); +const { findAll } = require('solidity-ast/utils'); +const { _: artifacts } = require('yargs').argv; + +for (const artifact of artifacts) { + const { output: solcOutput } = require(path.resolve(__dirname, '..', artifact)); + + const fromId = {}; + const inheritIds = {}; + for (const source in solcOutput.contracts) { + for (const contractDef of findAll('ContractDefinition', solcOutput.sources[source].ast)) { + fromId[contractDef.id] = contractDef.name; + inheritIds[contractDef.name] = contractDef.linearizedBaseContracts; + } + } + + const chains = Object.values(inheritIds).filter(ids => ids.length > 1).map(ids => ids.map(id => fromId[id])); + + const graph = new Graph({ directed: true }); + chains.flatMap(chain => chain.flatMap((name, i, parents) => parents.slice(i + 1).map(parent => { + graph.setNode(name); + graph.setNode(parent); + graph.setEdge(parent, name); + return graph.successors(name).includes(parent) ? [name, parent] : null; + }))) + .filter(Boolean) + .filter((obj, i, array) => array.findIndex(obj2 => obj.join() === obj2.join()) === i) + .forEach(([a, b]) => { + console.log(`Conflict between ${a} and ${b} detected in the following dependecy chains:`); + chains + .filter(chain => chain.includes(a) && chain.includes(b)) + .forEach(chain => { + const comp = chain.indexOf(a) < chain.indexOf(b) ? '>' : '<'; + console.log(`- ${a} ${comp} ${b}: ${chain.reverse().join(', ')}`); + }); + process.exitCode = 1; + }); +} + +if (!process.exitCode) { + console.log('Contract ordering is valid.'); +} From 196890529e6b2732050b4e7a68e07b606004e944 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Thu, 17 Jun 2021 23:00:35 +0200 Subject: [PATCH 2/7] fix inheritance ordering in ERC1155ReceiverMock --- contracts/mocks/ERC1155ReceiverMock.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contracts/mocks/ERC1155ReceiverMock.sol b/contracts/mocks/ERC1155ReceiverMock.sol index 1d9def93dea..6443a56c7ae 100644 --- a/contracts/mocks/ERC1155ReceiverMock.sol +++ b/contracts/mocks/ERC1155ReceiverMock.sol @@ -5,7 +5,7 @@ pragma solidity ^0.8.0; import "../token/ERC1155/IERC1155Receiver.sol"; import "../utils/introspection/ERC165.sol"; -contract ERC1155ReceiverMock is IERC1155Receiver, ERC165 { +contract ERC1155ReceiverMock is ERC165, IERC1155Receiver { bytes4 private _recRetval; bool private _recReverts; bytes4 private _batRetval; From 2f772555c8a2d5793ed5e9b8ebdbd3e954b05f1e Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Thu, 17 Jun 2021 23:12:07 +0200 Subject: [PATCH 3/7] fix typo --- scripts/inheritanceOrdering.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scripts/inheritanceOrdering.js b/scripts/inheritanceOrdering.js index 6034b23f1e0..27c1545a284 100644 --- a/scripts/inheritanceOrdering.js +++ b/scripts/inheritanceOrdering.js @@ -27,7 +27,7 @@ for (const artifact of artifacts) { .filter(Boolean) .filter((obj, i, array) => array.findIndex(obj2 => obj.join() === obj2.join()) === i) .forEach(([a, b]) => { - console.log(`Conflict between ${a} and ${b} detected in the following dependecy chains:`); + console.log(`Conflict between ${a} and ${b} detected in the following dependency chains:`); chains .filter(chain => chain.includes(a) && chain.includes(b)) .forEach(chain => { From e44ab7c337c265da796054ec8006ccd7c39af46b Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Thu, 17 Jun 2021 23:27:14 +0200 Subject: [PATCH 4/7] cleanup inheritanceOrdering.js --- scripts/inheritanceOrdering.js | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/scripts/inheritanceOrdering.js b/scripts/inheritanceOrdering.js index 27c1545a284..3ac6dc9d439 100644 --- a/scripts/inheritanceOrdering.js +++ b/scripts/inheritanceOrdering.js @@ -6,19 +6,18 @@ const { _: artifacts } = require('yargs').argv; for (const artifact of artifacts) { const { output: solcOutput } = require(path.resolve(__dirname, '..', artifact)); - const fromId = {}; - const inheritIds = {}; + const names = {}; + const linearized = []; for (const source in solcOutput.contracts) { for (const contractDef of findAll('ContractDefinition', solcOutput.sources[source].ast)) { - fromId[contractDef.id] = contractDef.name; - inheritIds[contractDef.name] = contractDef.linearizedBaseContracts; + names[contractDef.id] = contractDef.name; + linearized.push(contractDef.linearizedBaseContracts); } } - const chains = Object.values(inheritIds).filter(ids => ids.length > 1).map(ids => ids.map(id => fromId[id])); - + const linearizedNames = linearized.map(ids => ids.map(id => names[id])); const graph = new Graph({ directed: true }); - chains.flatMap(chain => chain.flatMap((name, i, parents) => parents.slice(i + 1).map(parent => { + linearizedNames.flatMap(chain => chain.flatMap((name, i, parents) => parents.slice(i + 1).map(parent => { graph.setNode(name); graph.setNode(parent); graph.setEdge(parent, name); @@ -28,7 +27,7 @@ for (const artifact of artifacts) { .filter((obj, i, array) => array.findIndex(obj2 => obj.join() === obj2.join()) === i) .forEach(([a, b]) => { console.log(`Conflict between ${a} and ${b} detected in the following dependency chains:`); - chains + linearizedNames .filter(chain => chain.includes(a) && chain.includes(b)) .forEach(chain => { const comp = chain.indexOf(a) < chain.indexOf(b) ? '>' : '<'; From 89a5c8af7d859c1ca6f51280f3de171faca57fa2 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Fri, 18 Jun 2021 00:35:25 +0200 Subject: [PATCH 5/7] improve inheritance check --- scripts/inheritanceOrdering.js | 38 +++++++++++++++------------------- 1 file changed, 17 insertions(+), 21 deletions(-) diff --git a/scripts/inheritanceOrdering.js b/scripts/inheritanceOrdering.js index 3ac6dc9d439..75d11ae431d 100644 --- a/scripts/inheritanceOrdering.js +++ b/scripts/inheritanceOrdering.js @@ -1,40 +1,36 @@ const path = require('path'); -const { Graph } = require('graphlib'); +const graphlib = require('graphlib'); const { findAll } = require('solidity-ast/utils'); const { _: artifacts } = require('yargs').argv; for (const artifact of artifacts) { const { output: solcOutput } = require(path.resolve(__dirname, '..', artifact)); + const graph = new graphlib.Graph({ directed: true }); const names = {}; const linearized = []; + for (const source in solcOutput.contracts) { for (const contractDef of findAll('ContractDefinition', solcOutput.sources[source].ast)) { names[contractDef.id] = contractDef.name; linearized.push(contractDef.linearizedBaseContracts); + + contractDef.linearizedBaseContracts.forEach((c1, i, contracts) => contracts.slice(i + 1).forEach(c2 => { + graph.setEdge(c1, c2); + })); } } - const linearizedNames = linearized.map(ids => ids.map(id => names[id])); - const graph = new Graph({ directed: true }); - linearizedNames.flatMap(chain => chain.flatMap((name, i, parents) => parents.slice(i + 1).map(parent => { - graph.setNode(name); - graph.setNode(parent); - graph.setEdge(parent, name); - return graph.successors(name).includes(parent) ? [name, parent] : null; - }))) - .filter(Boolean) - .filter((obj, i, array) => array.findIndex(obj2 => obj.join() === obj2.join()) === i) - .forEach(([a, b]) => { - console.log(`Conflict between ${a} and ${b} detected in the following dependency chains:`); - linearizedNames - .filter(chain => chain.includes(a) && chain.includes(b)) - .forEach(chain => { - const comp = chain.indexOf(a) < chain.indexOf(b) ? '>' : '<'; - console.log(`- ${a} ${comp} ${b}: ${chain.reverse().join(', ')}`); - }); - process.exitCode = 1; - }); + graphlib.alg.findCycles(graph).forEach(([ c1, c2 ]) => { + console.log(`Conflict between ${names[c1]} and ${names[c2]} detected in the following dependency chains:`); + linearized + .filter(chain => chain.includes(parseInt(c1)) && chain.includes(parseInt(c2))) + .forEach(chain => { + const comp = chain.indexOf(c1) < chain.indexOf(c2) ? '>' : '<'; + console.log(`- ${names[c1]} ${comp} ${names[c2]}: ${chain.reverse().map(id => names[id]).join(', ')}`); + }); + process.exitCode = 1; + }); } if (!process.exitCode) { From 270ac5cee8d25b827ebf15d540a0643f1646ca0b Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Fri, 18 Jun 2021 00:53:17 +0200 Subject: [PATCH 6/7] move inheritance check to test:inheritance --- .github/workflows/test.yml | 1 + package.json | 4 ++-- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 7cdf212e012..38e5c54cd22 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -29,6 +29,7 @@ jobs: env: FORCE_COLOR: 1 ENABLE_GAS_REPORT: true + - run: npm run test:inheritance - name: Print gas report run: cat gas-report.txt diff --git a/package.json b/package.json index a6607f96a19..384e14327a4 100644 --- a/package.json +++ b/package.json @@ -16,19 +16,19 @@ "docs": "oz-docs", "docs:watch": "npm run docs watch contracts 'docs/*.hbs' docs/helpers.js", "prepare-docs": "scripts/prepare-docs.sh", - "lint": "npm run lint:js && npm run lint:sol && npm run lint:inheritance", + "lint": "npm run lint:js && npm run lint:sol", "lint:fix": "npm run lint:js:fix && npm run lint:sol:fix", "lint:js": "eslint --ignore-path .gitignore .", "lint:js:fix": "eslint --ignore-path .gitignore . --fix", "lint:sol": "solhint 'contracts/**/*.sol' && prettier -c 'contracts/**/*.sol'", "lint:sol:fix": "prettier --write \"contracts/**/*.sol\"", - "lint:inheritance": "node scripts/inheritanceOrdering artifacts/build-info/*", "prepublish": "rimraf build contracts/build artifacts cache", "prepare": "env COMPILE_MODE=production npm run compile", "prepack": "scripts/prepack.sh", "release": "scripts/release/release.sh", "version": "scripts/release/version.sh", "test": "hardhat test", + "test:inheritance": "node scripts/inheritanceOrdering artifacts/build-info/*", "gas-report": "env ENABLE_GAS_REPORT=true npm run test" }, "repository": { From 6611ddfae1be6b1cebad94cb593f859c9d555979 Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Fri, 18 Jun 2021 12:02:14 -0300 Subject: [PATCH 7/7] reword valid -> consistent --- scripts/inheritanceOrdering.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scripts/inheritanceOrdering.js b/scripts/inheritanceOrdering.js index 75d11ae431d..dd965989b16 100644 --- a/scripts/inheritanceOrdering.js +++ b/scripts/inheritanceOrdering.js @@ -34,5 +34,5 @@ for (const artifact of artifacts) { } if (!process.exitCode) { - console.log('Contract ordering is valid.'); + console.log('Contract ordering is consistent.'); }