From 2613e6d683b075f2a8841a076e6f2bce69ec8a4c Mon Sep 17 00:00:00 2001 From: emizzle Date: Thu, 17 Jan 2019 23:01:33 +1100 Subject: [PATCH] feat(@embark/core): Recursively import contracts Add support to recursively import contracts. If we have three contracts 1. A imports B 2. B imports C Then prior to this PR, contract A would import contract B, and a remapping would be added to the contract so the compiler would know how to find contract B. However, contract B imports contracts C, and because the `parseFileForImport` method was not recursive, the remappings were not able to go one level deeper to remap the path to contract C, and thus the compiler would not know how to locate contract C, and would complain with the error `File outside of allowed directories.` With the introduction of this PR, the `parseFileForImport` method is now recursive, and so any contract imported is also checked for it's own imports that can be remapped. Specifically, this use case is applicable when there is a dependency containing contracts that imports one of it's own dependency's contracts, ie: ``` pragma solididty ^0.5.0; import "dependency-1/contract-1.sol"; ``` where the dependencies look like: ``` |- node_modules |--- dependency-1 |----- contract-1.sol <--- contains import "dependency-2/contract-2.sol" |--- dependency-2 |----- contract-2.sol ``` Add unit tests that verify recursive imports work. Add embark depdendency that installs a contract used in the recursive unit tests. --- package.json | 1 + src/lib/core/file.js | 152 +++++++++++++++--------- src/test/contracts/recursive_test_0.sol | 19 +++ src/test/contracts/recursive_test_1.sol | 19 +++ src/test/contracts/recursive_test_2.sol | 19 +++ src/test/file.js | 26 ++++ yarn.lock | 12 ++ 7 files changed, 189 insertions(+), 59 deletions(-) create mode 100644 src/test/contracts/recursive_test_0.sol create mode 100644 src/test/contracts/recursive_test_1.sol create mode 100644 src/test/contracts/recursive_test_2.sol diff --git a/package.json b/package.json index cc8b8323a..fea5c70c7 100644 --- a/package.json +++ b/package.json @@ -97,6 +97,7 @@ "decompress": "4.2.0", "deep-equal": "1.0.1", "ejs": "2.6.1", + "embark-test-contract-0": "0.0.2", "embarkjs": "0.5.0", "eth-ens-namehash": "2.0.8", "ethereumjs-tx": "1.3.7", diff --git a/src/lib/core/file.js b/src/lib/core/file.js index 087d8df3c..227677c8e 100644 --- a/src/lib/core/file.js +++ b/src/lib/core/file.js @@ -6,7 +6,7 @@ const utils = require('../utils/utils'); class File { - constructor (options) { + constructor(options) { this.filename = options.filename.replace(/\\/g, '/'); this.type = options.type; this.path = options.path; @@ -19,74 +19,108 @@ class File { this.providerUrl = null; } + addRemappings(prefix, httpFileObj, level, callback) { + let target = prefix; + if (httpFileObj) { + target = httpFileObj.filePath; + } else if (fs.existsSync(path.join(path.dirname(this.filename), prefix))) { + target = path.join(path.dirname(this.filename), prefix); + } else if (fs.existsSync(path.join("node_modules", prefix))) { + target = path.join("node_modules", prefix); + } + + if (target === prefix) return callback(); + + target = fs.dappPath(target); + + const remapping = { + prefix, + target + }; + + if (httpFileObj) return callback(); + + if (!this.importRemappings.some(existing => existing.prefix === remapping.prefix)) { + this.importRemappings.push(remapping); + } + + fs.readFile(target, (err, importedContract) => { + if (err) return callback(err); + if (!importedContract) return callback(`File not found: ${target}`); + this._parseFileForImport(importedContract.toString(), false, ++level, callback); + }); + } + + _parseFileForImport(content, isHttpContract, level, callback) { + const self = this; + if (self.filename.indexOf('.sol') < 0) { + // Only supported in Solidity + return callback(null, content); + } + const regex = /import ["']([-a-zA-Z0-9@:%_+.~#?&\/=]+)["'];/g; + const filesToDownload = []; + const pathWithoutFile = path.dirname(self.path); + let newContent = content; + let storageConfig = self.storageConfig; + if (storageConfig && storageConfig.upload && storageConfig.upload.getUrl) { + self.providerUrl = storageConfig.upload.getUrl; + } + let m, matches = []; + while ((m = regex.exec(content))) { + matches.push(m[1]); + } + async.each(matches, (match, next) => { + const httpFileObj = utils.getExternalContractUrl(match, self.providerUrl); + const fileObj = { + fileRelativePath: path.join(path.dirname(self.filename), match), + url: `${pathWithoutFile}/${match}` + }; + + self.addRemappings(match, httpFileObj, level, (err) => { + if (err) return next(err); + if (httpFileObj) { + newContent = newContent.replace(match, httpFileObj.filePath); + + fileObj.fileRelativePath = httpFileObj.filePath; + fileObj.url = httpFileObj.url; + } else if (!isHttpContract) { + // Just a normal import + return next(); + } + filesToDownload.push(fileObj); + next(); + }); + }, (err) => { + callback(err, newContent, filesToDownload); + }); + } + parseFileForImport(content, isHttpContract, callback) { const self = this; if (typeof isHttpContract === 'function') { callback = isHttpContract; isHttpContract = false; } - if (self.filename.indexOf('.sol') < 0) { - // Only supported in Solidity - return callback(null, content); - } - const regex = /import ["']([-a-zA-Z0-9@:%_+.~#?&\/=]+)["'];/g; - let matches; - const filesToDownload = []; - const pathWithoutFile = path.dirname(self.path); - let newContent = content; - let storageConfig = self.storageConfig; - if (storageConfig && storageConfig.upload && storageConfig.upload.getUrl) { - self.providerUrl = storageConfig.upload.getUrl; - } - while ((matches = regex.exec(content))) { - const httpFileObj = utils.getExternalContractUrl(matches[1],self.providerUrl); - const fileObj = { - fileRelativePath: path.join(path.dirname(self.filename), matches[1]), - url: `${pathWithoutFile}/${matches[1]}` - }; - var target = matches[1]; - if (httpFileObj) { - target = httpFileObj.filePath; - } else if (fs.existsSync(path.join(path.dirname(self.filename), matches[1]))) { - target = path.join(path.dirname(self.filename), matches[1]); - } else if (fs.existsSync(path.join("node_modules", matches[1]))) { - target = path.join("node_modules", matches[1]); + this._parseFileForImport(content, isHttpContract, 0, (err, newContent, filesToDownload) => { + if (err) return callback(err); + + if (self.downloadedImports) { + // We already parsed this file + return callback(null, newContent); } - - self.importRemappings.push({ - prefix: matches[1], - target: fs.dappPath(target) + async.each(filesToDownload, ((fileObj, eachCb) => { + self.downloadFile(fileObj.fileRelativePath, fileObj.url, (_content) => { + eachCb(); + }); + }), (err) => { + self.downloadedImports = true; + callback(err, newContent); }); - - if (httpFileObj) { - // Replace http import by filePath import in content - newContent = newContent.replace(matches[1], httpFileObj.filePath); - - fileObj.fileRelativePath = httpFileObj.filePath; - fileObj.url = httpFileObj.url; - } else if (!isHttpContract) { - // Just a normal import - continue; - } - filesToDownload.push(fileObj); - } - - if (self.downloadedImports) { - // We already parsed this file - return callback(null, newContent); - } - async.each(filesToDownload, ((fileObj, eachCb) => { - self.downloadFile(fileObj.fileRelativePath, fileObj.url, (_content) => { - eachCb(); - }); - }), (err) => { - self.downloadedImports = true; - callback(err, newContent); }); } - downloadFile (filename, url, callback) { + downloadFile(filename, url, callback) { const self = this; async.waterfall([ function makeTheDir(next) { @@ -128,14 +162,14 @@ class File { } ], (err, content) => { if (err) { - console.error(__('Error while downloading the file'), url, err); + console.error(__('Error while downloading the file'), url, err); return callback(''); } callback(content.toString()); }); } - content (callback) { + content(callback) { let content; if (this.type === File.types.embark_internal) { content = fs.readFileSync(fs.embarkPath(utils.joinPath('dist', this.path))).toString(); diff --git a/src/test/contracts/recursive_test_0.sol b/src/test/contracts/recursive_test_0.sol new file mode 100644 index 000000000..95a2ba4fa --- /dev/null +++ b/src/test/contracts/recursive_test_0.sol @@ -0,0 +1,19 @@ +pragma solidity ^0.5.0; + +import "./recursive_test_1.sol"; + +contract SimpleStorageRecursive0 { + uint public storedData; + + constructor (uint initialValue) public { + storedData = initialValue; + } + + function set(uint x) public { + storedData = x; + } + + function get() public view returns (uint retVal) { + return storedData; + } +} \ No newline at end of file diff --git a/src/test/contracts/recursive_test_1.sol b/src/test/contracts/recursive_test_1.sol new file mode 100644 index 000000000..13bb6be8b --- /dev/null +++ b/src/test/contracts/recursive_test_1.sol @@ -0,0 +1,19 @@ +pragma solidity ^0.5.0; + +import "./recursive_test_2.sol"; + +contract SimpleStorageRecursive1 { + uint public storedData; + + constructor(uint initialValue) public { + storedData = initialValue; + } + + function set(uint x) public { + storedData = x; + } + + function get() public view returns (uint retVal) { + return storedData; + } +} \ No newline at end of file diff --git a/src/test/contracts/recursive_test_2.sol b/src/test/contracts/recursive_test_2.sol new file mode 100644 index 000000000..b5c969ff9 --- /dev/null +++ b/src/test/contracts/recursive_test_2.sol @@ -0,0 +1,19 @@ +pragma solidity ^0.5.0; + +import "embark-test-contract-0/recursive_test_3.sol"; + +contract SimpleStorageRecursive2 { + uint public storedData; + + constructor(uint initialValue) public { + storedData = initialValue; + } + + function set(uint x) public { + storedData = x; + } + + function get() public view returns (uint retVal) { + return storedData; + } +} \ No newline at end of file diff --git a/src/test/file.js b/src/test/file.js index 538fda3eb..8243ee7da 100644 --- a/src/test/file.js +++ b/src/test/file.js @@ -26,6 +26,32 @@ describe('embark.File', function () { }); }); + it('should find and add remappings for all recursive imports', function (done) { + const contract = fs.readFileSync('./dist/test/contracts/recursive_test_0.sol').toString(); + const file = new File({filename: './dist/test/contracts/recursive_test_0.sol', + path: path.join(__dirname, './contracts/recursive_test_0.sol')}); + + file.parseFileForImport(contract, () => { + assert.deepEqual(file.importRemappings[0], { + prefix: "./recursive_test_1.sol", + target: path.join(__dirname, "./contracts/recursive_test_1.sol") + }); + assert.deepEqual(file.importRemappings[1], { + prefix: "./recursive_test_2.sol", + target: path.join(__dirname, "./contracts/recursive_test_2.sol") + }); + assert.deepEqual(file.importRemappings[2], { + prefix: "embark-test-contract-0/recursive_test_3.sol", + target: path.resolve(path.join("node_modules", "./embark-test-contract-0/recursive_test_3.sol")) + }); + assert.deepEqual(file.importRemappings[3], { + prefix: "embark-test-contract-1/recursive_test_4.sol", + target: path.resolve(path.join("node_modules", "./embark-test-contract-1/recursive_test_4.sol")) + }); + done(); + }); + }); + it('should find all the imports but not call download because not a http contract', function (done) { const contract = fs.readFileSync('./dist/test/contracts/contract_with_import.sol').toString(); const file = new File({filename: '.embark/contracts/embark-framework/embark/master/test_app/app/contracts/simple_storage.sol', diff --git a/yarn.lock b/yarn.lock index 2f25366a9..c5efb2266 100644 --- a/yarn.lock +++ b/yarn.lock @@ -4089,6 +4089,18 @@ elliptic@^6.0.0, elliptic@^6.2.3, elliptic@^6.4.0: minimalistic-assert "^1.0.0" minimalistic-crypto-utils "^1.0.0" +embark-test-contract-0@0.0.2: + version "0.0.2" + resolved "https://registry.yarnpkg.com/embark-test-contract-0/-/embark-test-contract-0-0.0.2.tgz#53913fb40e3df4b816a7bef9f00a5f78fa3d56b4" + integrity sha512-bETRyZERYMGwmHuXBlgQGkRmSZoB5sb8kW5M240PsxbO05FE1YyPNcPcDM2+FEJozHFMl9hqxrbt69X7Zzn7xw== + dependencies: + embark-test-contract-1 "^0.0.1" + +embark-test-contract-1@^0.0.1: + version "0.0.1" + resolved "https://registry.yarnpkg.com/embark-test-contract-1/-/embark-test-contract-1-0.0.1.tgz#802b84150e8038fef6681a3f23b6f4c0dc5b3e80" + integrity sha512-yFaXMOXOMfYRNFOKEspdXrZzyovceWedtegHtbfs8RZWRzRYY+v6ZDtGm13TRJt3Qt4VZhKky0l7G/SHZMGHCA== + embarkjs@0.5.0: version "0.5.0" resolved "https://registry.yarnpkg.com/embarkjs/-/embarkjs-0.5.0.tgz#b5b282289896b62f7ef4d4df40566d1777f4b83e"