From ac0618c34fb37784b09a8bf82ed7cf3cbc7156fc Mon Sep 17 00:00:00 2001 From: cgewecke Date: Mon, 11 Jan 2021 00:27:03 -0800 Subject: [PATCH] Add ABI diff logic (#598) --- README.md | 3 +- lib/abi.js | 99 ++++++++++++++ lib/api.js | 10 +- lib/validator.js | 4 +- package.json | 2 + plugins/resources/nomiclabs.utils.js | 32 ++++- plugins/resources/truffle.utils.js | 33 +++++ .../solidity/contracts/diff/addition.sol | 38 ++++++ .../solidity/contracts/diff/events.sol | 23 ++++ .../solidity/contracts/diff/no-change.sol | 34 +++++ .../solidity/contracts/diff/param-change.sol | 33 +++++ .../solidity/contracts/diff/removal.sol | 29 ++++ .../solidity/contracts/diff/reorder.sol | 33 +++++ .../solidity/contracts/diff/return-sig.sol | 25 ++++ .../contracts/diff/state-mod-change.sol | 13 ++ test/units/diff.js | 128 ++++++++++++++++++ test/units/validator.js | 4 +- test/util/integration.js | 1 - test/util/util.js | 35 ++++- 19 files changed, 566 insertions(+), 13 deletions(-) create mode 100644 lib/abi.js create mode 100644 test/sources/solidity/contracts/diff/addition.sol create mode 100644 test/sources/solidity/contracts/diff/events.sol create mode 100644 test/sources/solidity/contracts/diff/no-change.sol create mode 100644 test/sources/solidity/contracts/diff/param-change.sol create mode 100644 test/sources/solidity/contracts/diff/removal.sol create mode 100644 test/sources/solidity/contracts/diff/reorder.sol create mode 100644 test/sources/solidity/contracts/diff/return-sig.sol create mode 100644 test/sources/solidity/contracts/diff/state-mod-change.sol create mode 100644 test/units/diff.js diff --git a/README.md b/README.md index 16d7303..f42ac84 100644 --- a/README.md +++ b/README.md @@ -101,8 +101,9 @@ module.exports = { | measureStatementCoverage | *boolean* | `true` | Computes statement (in addition to line) coverage. [More...][34] | | measureFunctionCoverage | *boolean* | `true` | Computes function coverage. [More...][34] | | measureModifierCoverage | *boolean* | `true` | Computes each modifier invocation as a code branch. [More...][34] | -| matrixOutputPath | *String* | `./testMatrix.json` | Relative path to write test matrix JSON object to. [More...][39] | | modifierWhitelist | *String[]* | `[]` | List of modifier names (ex: "onlyOwner") to exclude from branch measurement. (Useful for modifiers which prepare something instead of acting as a gate.)) | +| matrixOutputPath | *String* | `./testMatrix.json` | Relative path to write test matrix JSON object to. [More...][39] | +| abiOutputPath | *String* | `./humanReadableAbis.json` | Relative path to write diff-able ABI data to | | istanbulFolder | *String* | `./coverage` | Folder location for Istanbul coverage reports. | | istanbulReporter | *Array* | `['html', 'lcov', 'text', 'json']` | [Istanbul coverage reporters][2] | | mocha | *Object* | `{ }` | [Mocha options][3] to merge into existing mocha config. `grep` and `invert` are useful for skipping certain tests under coverage using tags in the test descriptions.| diff --git a/lib/abi.js b/lib/abi.js new file mode 100644 index 0000000..2b8455e --- /dev/null +++ b/lib/abi.js @@ -0,0 +1,99 @@ +const ethersABI = require("@ethersproject/abi"); +const difflib = require('difflib'); + +class AbiUtils { + + diff(orig={}, cur={}){ + let plus = 0; + let minus = 0; + + const unifiedDiff = difflib.unifiedDiff( + orig.humanReadableAbiList, + cur.humanReadableAbiList, + { + fromfile: orig.contractName, + tofile: cur.contractName, + fromfiledate: `sha: ${orig.sha}`, + tofiledate: `sha: ${cur.sha}`, + lineterm: '' + } + ); + + // Count changes (unified diff always has a plus & minus in header); + if (unifiedDiff.length){ + plus = -1; + minus = -1; + } + + unifiedDiff.forEach(line => { + if (line[0] === `+`) plus++; + if (line[0] === `-`) minus++; + }) + + return { + plus, + minus, + unifiedDiff + } + } + + toHumanReadableFunctions(contract){ + const human = []; + const ethersOutput = new ethersABI.Interface(contract.abi).functions; + const signatures = Object.keys(ethersOutput); + + for (const sig of signatures){ + const method = ethersOutput[sig]; + let returns = ''; + + method.outputs.forEach(output => { + (returns.length) + ? returns += `, ${output.type}` + : returns += output.type; + }); + + let readable = `${method.type} ${sig} ${method.stateMutability}`; + + if (returns.length){ + readable += ` returns (${returns})` + } + + human.push(readable); + } + + return human; + } + + toHumanReadableEvents(contract){ + const human = []; + const ethersOutput = new ethersABI.Interface(contract.abi).events; + const signatures = Object.keys(ethersOutput); + + for (const sig of signatures){ + const method = ethersOutput[sig]; + const readable = `${ethersOutput[sig].type} ${sig}`; + human.push(readable); + } + + return human; + } + + generateHumanReadableAbiList(_artifacts, sha){ + const list = []; + if (_artifacts.length){ + for (const item of _artifacts){ + const fns = this.toHumanReadableFunctions(item); + const evts = this.toHumanReadableEvents(item); + const all = fns.concat(evts); + list.push({ + contractName: item.contractName, + sha: sha, + humanReadableAbiList: all + }) + } + } + return list; + } +} + +module.exports = AbiUtils; \ No newline at end of file diff --git a/lib/api.js b/lib/api.js index d0f89c8..ef31254 100644 --- a/lib/api.js +++ b/lib/api.js @@ -12,13 +12,15 @@ const Instrumenter = require('./instrumenter'); const Coverage = require('./coverage'); const DataCollector = require('./collector'); const AppUI = require('./ui').AppUI; +const AbiUtils = require('./abi'); /** * Coverage Runner */ class API { constructor(config={}) { - this.validator = new ConfigValidator() + this.validator = new ConfigValidator(); + this.abiUtils = new AbiUtils(); this.config = config || {}; this.testMatrix = {}; @@ -31,6 +33,7 @@ class API { this.testsErrored = false; this.cwd = config.cwd || process.cwd(); + this.abiOutputPath = config.abiOutputPath || "humanReadableAbis.json"; this.matrixOutputPath = config.matrixOutputPath || "testMatrix.json"; this.matrixReporterPath = config.matrixReporterPath || "solidity-coverage/plugins/resources/matrix.js" @@ -360,6 +363,11 @@ class API { fs.writeFileSync(matrixPath, JSON.stringify(mapping, null, ' ')); } + saveHumanReadableAbis(data){ + const abiPath = path.join(this.cwd, this.abiOutputPath); + fs.writeFileSync(abiPath, JSON.stringify(data, null, ' ')); + } + // ===== // Paths // ===== diff --git a/lib/validator.js b/lib/validator.js index 1cf4f9e..1e1fdf0 100644 --- a/lib/validator.js +++ b/lib/validator.js @@ -14,7 +14,9 @@ const configSchema = { client: {type: "object"}, cwd: {type: "string"}, host: {type: "string"}, - + abiOutputPath: {type: "string"}, + matrixOutputPath: {type: "string"}, + matrixReporterPath: {type: "string"}, port: {type: "number"}, providerOptions: {type: "object"}, silent: {type: "boolean"}, diff --git a/package.json b/package.json index df695f7..274beaf 100644 --- a/package.json +++ b/package.json @@ -25,10 +25,12 @@ "license": "ISC", "dependencies": { "@solidity-parser/parser": "^0.14.0", + "@ethersproject/abi": "^5.0.9", "@truffle/provider": "^0.2.24", "chalk": "^2.4.2", "death": "^1.1.0", "detect-port": "^1.3.0", + "difflib": "^0.2.4", "fs-extra": "^8.1.0", "ghost-testrpc": "^0.0.2", "global-modules": "^2.0.0", diff --git a/plugins/resources/nomiclabs.utils.js b/plugins/resources/nomiclabs.utils.js index 4f2ed1c..59a671c 100644 --- a/plugins/resources/nomiclabs.utils.js +++ b/plugins/resources/nomiclabs.utils.js @@ -181,6 +181,35 @@ function collectTestMatrixData(args, env, api){ } } +/** + * Returns all Hardhat artifacts. + * @param {HRE} env + * @return {Artifact[]} + */ +async function getAllArtifacts(env){ + const all = []; + const qualifiedNames = await env.artifacts.getArtifactPaths(); + for (const name of qualifiedNames){ + all.push(await env.artifacts.readArtifact(name)); + } + return all; +} + +/** + * Compiles project + * Collects all artifacts from Hardhat project, + * Converts them to a format that can be consumed by api.abiUtils.diff + * Saves them to `api.abiOutputPath` + * @param {HRE} env + * @param {SolidityCoverageAPI} api + */ +async function generateHumanReadableAbiList(env, api){ + await env.run(TASK_COMPILE); + const _artifacts = getAllArtifacts(env); + const list = api.abiUtils.generateHumanReadableAbiList(_artifacts) + api.saveHumanReadableAbis(list); +} + /** * Sets the default `from` account field in the network that will be used. * This needs to be done after accounts are fetched from the launched client. @@ -233,6 +262,7 @@ module.exports = { setupHardhatNetwork, getTestFilePaths, setNetworkFrom, - collectTestMatrixData + collectTestMatrixData, + getAllArtifacts } diff --git a/plugins/resources/truffle.utils.js b/plugins/resources/truffle.utils.js index 5aaa21c..13997e4 100644 --- a/plugins/resources/truffle.utils.js +++ b/plugins/resources/truffle.utils.js @@ -35,6 +35,39 @@ async function getTestFilePaths(config){ return target.filter(f => f.match(testregex) != null); } +/** + * Returns all Truffle artifacts. + * @param {TruffleConfig} config + * @return {Artifact[]} + */ +function getAllArtifacts(config){ + const all = []; + const artifactsGlob = path.join(config.artifactsDir, '/**/*.json'); + const files = globby.sync([artifactsGlob]) + for (const file of files){ + const candidate = require(file); + if (candidate.contractName && candidate.abi){ + all.push(candidate); + } + } + return all; +} + +/** + * Compiles project + * Collects all artifacts from Truffle project, + * Converts them to a format that can be consumed by api.abiUtils.diff + * Saves them to `api.abiOutputPath` + * @param {TruffleConfig} config + * @param {TruffleAPI} truffle + * @param {SolidityCoverageAPI} api + */ +async function generateHumanReadableAbiList(config, truffle, api){ + await truffle.compile(config); + const _artifacts = getAllArtifacts(config); + const list = api.abiUtils.generateHumanReadableAbiList(_artifacts) + api.saveHumanReadableAbis(list); +} /** * Configures the network. Runs before the server is launched. diff --git a/test/sources/solidity/contracts/diff/addition.sol b/test/sources/solidity/contracts/diff/addition.sol new file mode 100644 index 0000000..2820dd8 --- /dev/null +++ b/test/sources/solidity/contracts/diff/addition.sol @@ -0,0 +1,38 @@ +pragma solidity ^0.7.0; + +contract Old { + uint public y; + + function a() public { + bool x = true; + } + + function b() external { + bool x = true; + } + + function c() external { + bool x = true; + } +} + +contract New { + uint public y; + + function a() public { + bool x = true; + } + + function b() external { + bool x = true; + } + + function c() external { + bool x = true; + } + + function d() external { + bool x = true; + } +} + diff --git a/test/sources/solidity/contracts/diff/events.sol b/test/sources/solidity/contracts/diff/events.sol new file mode 100644 index 0000000..131828e --- /dev/null +++ b/test/sources/solidity/contracts/diff/events.sol @@ -0,0 +1,23 @@ +pragma solidity ^0.7.0; + +contract Old { + uint y; + + event Evt(uint x, bytes8 y); + + function a() public { + bool x = true; + } +} + +contract New { + uint y; + + function a() public { + bool x = true; + } + + event aEvt(bytes8); + event _Evt(bytes8 x, bytes8 y); +} + diff --git a/test/sources/solidity/contracts/diff/no-change.sol b/test/sources/solidity/contracts/diff/no-change.sol new file mode 100644 index 0000000..7cc9ce2 --- /dev/null +++ b/test/sources/solidity/contracts/diff/no-change.sol @@ -0,0 +1,34 @@ +pragma solidity ^0.7.0; + +contract Old { + uint y; + + function a() public { + bool x = true; + } + + function b() external { + bool x = true; + } + + function c() external { + bool x = true; + } +} + +contract New { + uint y; + + function a() public { + bool x = true; + } + + function b() external { + bool x = true; + } + + function c() external { + bool x = true; + } +} + diff --git a/test/sources/solidity/contracts/diff/param-change.sol b/test/sources/solidity/contracts/diff/param-change.sol new file mode 100644 index 0000000..87a048e --- /dev/null +++ b/test/sources/solidity/contracts/diff/param-change.sol @@ -0,0 +1,33 @@ +pragma solidity ^0.7.0; + +contract Old { + uint y; + + function a() public { + bool x = true; + } + + function b() external { + bool x = true; + } + + function c() external { + bool x = true; + } +} + +contract New { + uint y; + + function a() public { + bool x = true; + } + + function b(bytes8 z) external { + bool x = true; + } + + function c(uint q, uint r) external { + bool x = true; + } +} diff --git a/test/sources/solidity/contracts/diff/removal.sol b/test/sources/solidity/contracts/diff/removal.sol new file mode 100644 index 0000000..53a29e4 --- /dev/null +++ b/test/sources/solidity/contracts/diff/removal.sol @@ -0,0 +1,29 @@ +pragma solidity ^0.7.0; + +contract Old { + uint y; + + function a() public { + bool x = true; + } + + function b() external { + bool x = true; + } + + function c() external { + bool x = true; + } +} + +contract New { + uint y; + + function a() public { + bool x = true; + } + + function b() external { + bool x = true; + } +} diff --git a/test/sources/solidity/contracts/diff/reorder.sol b/test/sources/solidity/contracts/diff/reorder.sol new file mode 100644 index 0000000..ff9e8c9 --- /dev/null +++ b/test/sources/solidity/contracts/diff/reorder.sol @@ -0,0 +1,33 @@ +pragma solidity ^0.7.0; + +contract Old { + uint y; + + function a() public { + bool x = true; + } + + function b() external { + bool x = true; + } + + function c() external { + bool x = true; + } +} + +contract New { + uint y; + + function c() external { + bool x = true; + } + + function a() public { + bool x = true; + } + + function b() external { + bool x = true; + } +} diff --git a/test/sources/solidity/contracts/diff/return-sig.sol b/test/sources/solidity/contracts/diff/return-sig.sol new file mode 100644 index 0000000..b49cce6 --- /dev/null +++ b/test/sources/solidity/contracts/diff/return-sig.sol @@ -0,0 +1,25 @@ +pragma solidity ^0.7.0; + +contract Old { + function a() public view returns (uint) { + return 1; + } +} + +contract New { + function a() public view returns (bool) { + return true; + } + + function e() public view returns (uint8[2] memory) { + return [5,7]; + } + + function f() public view returns (uint8[2] memory, uint) { + return ([5,7], 7); + } + + function g() public view returns (uint8[3] memory) { + return [5,7,8]; + } +} diff --git a/test/sources/solidity/contracts/diff/state-mod-change.sol b/test/sources/solidity/contracts/diff/state-mod-change.sol new file mode 100644 index 0000000..36a451c --- /dev/null +++ b/test/sources/solidity/contracts/diff/state-mod-change.sol @@ -0,0 +1,13 @@ +pragma solidity ^0.7.0; + +contract Old { + function a() public { + bool x = true; + } +} + +contract New { + function a() public view returns (bool) { + return true; + } +} diff --git a/test/units/diff.js b/test/units/diff.js new file mode 100644 index 0000000..3b184ac --- /dev/null +++ b/test/units/diff.js @@ -0,0 +1,128 @@ +const assert = require('assert'); +const util = require('./../util/util.js'); +const Api = require('./../../lib/api') + +describe('abi diffs', function(){ + const api = new Api(); + + function setUp(source){ + const abis = util.getDiffABIs(source); + const orig = api.abiUtils.generateHumanReadableAbiList([abis.original], abis.original.sha); + const cur = api.abiUtils.generateHumanReadableAbiList([abis.current], abis.current.sha); + return api.abiUtils.diff(orig[0], cur[0]); + } + + function validate(result, expectPlus, expectMinus, expectDiff){ + assert.equal(result.plus, expectPlus); + assert.equal(result.minus, expectMinus); + assert.deepEqual(result.unifiedDiff, expectDiff); + } + + it('when methods are added', function() { + const expectPlus = 1; + const expectMinus = 0; + const expectDiff = [ + "--- Test\tsha: d8b26d8", + "+++ Test\tsha: e77e29d", + "@@ -1,4 +1,5 @@", + " function a() nonpayable", + " function b() nonpayable", + " function c() nonpayable", + "+function d() nonpayable", + " function y() view returns (uint256)" + ]; + + validate(setUp('diff/addition'), expectPlus, expectMinus, expectDiff); + }); + + it('when there are events', function() { + const expectPlus = 2; + const expectMinus = 1; + const expectDiff = [ + "--- Test\tsha: d8b26d8", + "+++ Test\tsha: e77e29d", + "@@ -1,2 +1,3 @@", + " function a() nonpayable", + "-event Evt(uint256,bytes8)", + "+event _Evt(bytes8,bytes8)", + "+event aEvt(bytes8)" + ]; + + validate(setUp('diff/events'), expectPlus, expectMinus, expectDiff); + }); + + it('when parameters change', function() { + const expectPlus = 2; + const expectMinus = 2; + const expectDiff = [ + "--- Test\tsha: d8b26d8", + "+++ Test\tsha: e77e29d", + "@@ -1,3 +1,3 @@", + " function a() nonpayable", + "-function b() nonpayable", + "-function c() nonpayable", + "+function b(bytes8) nonpayable", + "+function c(uint256,uint256) nonpayable" + ]; + + validate(setUp('diff/param-change'), expectPlus, expectMinus, expectDiff); + }); + + it('when there is no change', function() { + const expectPlus = 0; + const expectMinus = 0; + const expectDiff = []; + validate(setUp('diff/no-change'), expectPlus, expectMinus, expectDiff); + }); + + it('when methods are removed', function() { + const expectPlus = 0; + const expectMinus = 1; + const expectDiff = [ + '--- Test\tsha: d8b26d8', + '+++ Test\tsha: e77e29d', + '@@ -1,3 +1,2 @@', + ' function a() nonpayable', + ' function b() nonpayable', + '-function c() nonpayable' + ]; + + validate(setUp('diff/removal'), expectPlus, expectMinus, expectDiff); + }); + + it('when methods are reordered', function() { + const expectPlus = 0; + const expectMinus = 0; + const expectDiff = []; + validate(setUp('diff/reorder'), expectPlus, expectMinus, expectDiff); + }); + + it('when return signatures change', function() { + const expectPlus = 4; + const expectMinus = 1; + const expectDiff = [ + '--- Test\tsha: d8b26d8', + '+++ Test\tsha: e77e29d', + '@@ -1 +1,4 @@', + '-function a() view returns (uint256)', + '+function a() view returns (bool)', + '+function e() view returns (uint8[2])', + '+function f() view returns (uint8[2], uint256)', + '+function g() view returns (uint8[3])' + ]; + validate(setUp('diff/return-sig'), expectPlus, expectMinus, expectDiff); + }); + + it('when state modifiablility changes', function() { + const expectPlus = 1; + const expectMinus = 1; + const expectDiff = [ + '--- Test\tsha: d8b26d8', + '+++ Test\tsha: e77e29d', + '@@ -1 +1 @@', + '-function a() nonpayable', + '+function a() view returns (bool)' + ]; + validate(setUp('diff/state-mod-change'), expectPlus, expectMinus, expectDiff); + }); +}); diff --git a/test/units/validator.js b/test/units/validator.js index 1cb74c0..2479aed 100644 --- a/test/units/validator.js +++ b/test/units/validator.js @@ -22,7 +22,9 @@ describe('config validation', () => { const options = [ "cwd", "host", - "istanbulFolder" + "istanbulFolder", + "abiOutputPath", + "matrixOutputPath", ] options.forEach(name => { diff --git a/test/util/integration.js b/test/util/integration.js index 6a9b70a..d6fdcc4 100644 --- a/test/util/integration.js +++ b/test/util/integration.js @@ -50,7 +50,6 @@ function decacheConfigs(){ function clean() { shell.config.silent = true; - shell.rm('-Rf', temp); shell.rm('-Rf', 'coverage'); shell.rm('coverage.json'); diff --git a/test/util/util.js b/test/util/util.js index aa306a9..2e0ba91 100644 --- a/test/util/util.js +++ b/test/util/util.js @@ -68,6 +68,26 @@ function codeToCompilerInput(code) { }); } +// =========== +// Diff tests +// =========== +function getDiffABIs(sourceName, testFile="test.sol", original="Old", current="New"){ + const contract = getCode(`${sourceName}.sol`) + const solcOutput = compile(contract) + return { + original: { + contractName: "Test", + sha: "d8b26d8", + abi: solcOutput.contracts[testFile][original].abi, + }, + current: { + contractName: "Test", + sha: "e77e29d", + abi: solcOutput.contracts[testFile][current].abi, + } + } +} + // ============================ // Instrumentation Correctness // ============================ @@ -118,11 +138,12 @@ function initializeProvider(ganache){ } module.exports = { - getCode: getCode, - pathPrefix: pathPrefix, - filePath: filePath, - report: report, - instrumentAndCompile: instrumentAndCompile, - bootstrapCoverage: bootstrapCoverage, - initializeProvider: initializeProvider, + getCode, + pathPrefix, + filePath, + report, + instrumentAndCompile, + bootstrapCoverage, + initializeProvider, + getDiffABIs }