Integrated topological sorting into the library dependency resolution

OpenZeppelin · Nov 8, 2019 · d49cbf9 · d49cbf9
1 parent 1088251
commit d49cbf9
Show file tree

Hide file tree

Showing 3 changed files with 109 additions and 31 deletions.
diff --git a/packages/cli/package.json b/packages/cli/package.json
@@ -119,6 +119,7 @@
     "solc-wrapper": "^0.5.8",
     "solidity-parser-antlr": "^0.4.2",
     "spinnies": "^0.3.0",
+    "topological-sort": "^0.3.0",
     "truffle-config": "1.1.16",
     "underscore": "^1.9.1",
     "uuid": "^3.3.3",

diff --git a/packages/cli/src/models/network/NetworkController.ts b/packages/cli/src/models/network/NetworkController.ts
@@ -11,6 +11,7 @@ import partition from 'lodash.partition';
 import map from 'lodash.map';
 import concat from 'lodash.concat';
 import toPairs from 'lodash.topairs';
+import TopologicalSort from 'topological-sort';
 
 import {
   Contracts,
@@ -318,27 +319,34 @@ export default class NetworkController {
 
   // Contract model || SolidityLib model
   private _getAllSolidityLibNames(contractNames: string[], allDependencies: string[] = []): string[] {
-    let newLibNames = this._getSolidityLibNames(contractNames);
+    const graph = new TopologicalSort<string, string>(new Map<string, string>());
 
-    let newLibs = difference(newLibNames, allDependencies);
-    let repeatLibs = intersection(newLibNames, allDependencies);
-    let oldLibs = difference(allDependencies, newLibNames);
-
-    // reorder so that dependencies are pulled up
-    let reorderedOldLibs = repeatLibs.concat(oldLibs);
+    contractNames.forEach(contractName => {
+      const deps = this._getContractDependencies(contractName);
+      deps.forEach(dependencyContractName => {
+        this._populateDependencyGraph(dependencyContractName, graph);
+      });
+    });
 
-    // create a new list of all libs
-    let allLibs = newLibs.concat(reorderedOldLibs);
+    const sorted = graph.sort();
+    return [...sorted.keys()].reverse();
+  }
 
-    return uniq([...(newLibs.length ? this._getAllSolidityLibNames(newLibs, allLibs) : []), ...reorderedOldLibs]);
+  private _populateDependencyGraph(contractName: string, graph: TopologicalSort<string, string>) {
+    // @ts-ignore
+    if (!graph.nodes.has(contractName)) {
+      graph.addNode(contractName, contractName);
+      const deps = this._getContractDependencies(contractName);
+      deps.forEach(dependencyContractName => {
+        this._populateDependencyGraph(dependencyContractName, graph);
+        graph.addEdge(contractName, dependencyContractName);
+      });
+    }
   }
 
-  private _getSolidityLibNames(contractNames: string[]): string[] {
-    const libNames = contractNames.map(contractName => {
-      const contract = Contracts.getFromLocal(contractName);
-      return getSolidityLibNames(contract.schema.bytecode);
-    });
-    return uniq(flatten(libNames));
+  private _getContractDependencies(contractName: string): string[] {
+    const contract = Contracts.getFromLocal(contractName);
+    return getSolidityLibNames(contract.schema.bytecode);
   }
 
   // Contract model

diff --git a/packages/cli/test/models/network/NetworkController.test.js b/packages/cli/test/models/network/NetworkController.test.js
@@ -28,30 +28,99 @@ contract('NetworkController', function() {
     });
 
     it('should order dependencies correctly', () => {
-      const de = sinon.match.array.deepEquals;
+      /**
+       *
+       *  A => D, E, F
+       *  B => C, E, F, G
+       *  C => D, G
+       *  D =>
+       *  E => D
+       *  F => G
+       *  G => E
+       *
+       *  Expected ordering:
+       *
+       *  [D, E, G, F, A, C, B]
+       *
+       */
 
-      const contractNames = ['A', 'B', 'C'];
+      controllerMock
+        .expects('_getContractDependencies')
+        .withArgs('0')
+        .returns(['A', 'B', 'C']);
 
       controllerMock
-        .expects('_getSolidityLibNames')
-        .withArgs(de(contractNames))
-        .returns(['E', 'F', 'D']);
+        .expects('_getContractDependencies')
+        .withArgs('A')
+        .returns(['D', 'E', 'F']);
+
       controllerMock
-        .expects('_getSolidityLibNames')
-        .withArgs(de(['E', 'F', 'D']))
-        .returns(['D', 'G', 'H']);
+        .expects('_getContractDependencies')
+        .withArgs('B')
+        .returns(['C', 'E', 'F', 'G']);
+
       controllerMock
-        .expects('_getSolidityLibNames')
-        .withArgs(de(['G', 'H']))
-        .returns(['D', 'I']);
+        .expects('_getContractDependencies')
+        .withArgs('C')
+        .returns(['D', 'G']);
+
       controllerMock
-        .expects('_getSolidityLibNames')
-        .withArgs(de(['I']))
+        .expects('_getContractDependencies')
+        .withArgs('D')
         .returns([]);
 
-      const result = controller._getAllSolidityLibNames(['A', 'B', 'C']);
+      controllerMock
+        .expects('_getContractDependencies')
+        .withArgs('E')
+        .returns(['D']);
+
+      controllerMock
+        .expects('_getContractDependencies')
+        .withArgs('F')
+        .returns(['G']);
+
+      controllerMock
+        .expects('_getContractDependencies')
+        .withArgs('G')
+        .returns(['E']);
+
+      const result = controller._getAllSolidityLibNames(['0']);
+
+      assert.deepEqual(result, ['D', 'E', 'G', 'F', 'A', 'C', 'B']);
+    });
+
+    it('should throw on cycles', () => {
+      /**
+       *
+       *  A => B
+       *  B => C
+       *  C => A
+       *
+       */
+
+      controllerMock
+        .expects('_getContractDependencies')
+        .withArgs('0')
+        .returns(['A']);
+
+      controllerMock
+        .expects('_getContractDependencies')
+        .withArgs('A')
+        .returns(['B']);
+
+      controllerMock
+        .expects('_getContractDependencies')
+        .withArgs('B')
+        .returns(['C']);
+
+      controllerMock
+        .expects('_getContractDependencies')
+        .withArgs('C')
+        .returns(['A']);
 
-      assert.deepEqual(result, ['I', 'D', 'G', 'H', 'E', 'F']);
+      expect(() => {
+        controller._getAllSolidityLibNames(['0']);
+      }).to.throw(/circular/);
     });
   });