Validate constructors in ancestor contracts

Fixes #1332

packages/cli/src/interface/ValidationLogger.ts

@@ -105,7 +105,7 @@ export default class ValidationLogger {
         __filename,
         'logHasConstructor',
         `validation-has-constructor`,
-        `- Contract ${this.contractName} has an explicit constructor. Change it to an initializer function. See ${INITIALIZERS_LINK}.`,
+        `- Contract ${this.contractName} or an ancestor has a constructor. Change it to an initializer function. See ${INITIALIZERS_LINK}.`,
       );
     }
   }

packages/cli/test/interface/ValidationLogger.test.js

@@ -17,7 +17,7 @@ describe('ValidationLogger', function() {
   describe('errors', function() {
     it('logs constructor', async function() {
       validationLogger().log({ hasConstructor: true });
-      this.logs.errors[0].should.match(/has an explicit constructor/);
+      this.logs.errors[0].should.match(/has a constructor/);
     });
   });
 

packages/lib/contracts/mocks/Invalid.sol

@@ -1,6 +1,7 @@
 pragma solidity ^0.5.0;
 
 import "mock-dependency/contracts/Greeter.sol";
+import "mock-dependency/contracts/DependencyInvalid.sol";
 
 contract WithConstructor {
   uint256 public value;
@@ -14,6 +15,15 @@ contract WithConstructor {
   }
 }
 
+contract WithParentConstructor is WithConstructor {
+}
+
+contract WithAncestorConstructor is WithParentConstructor {
+}
+
+contract WithDependencyParentConstructor is DependencyWithConstructor {
+}
+
 contract WithFailingConstructor {
   constructor() public {
     assert(false);

packages/lib/src/artifacts/BuildArtifacts.ts

@@ -9,22 +9,36 @@ export function getBuildArtifacts(path?: string): BuildArtifacts {
   return new BuildArtifacts(Contracts.listBuildArtifacts(path));
 }
 
-// TS-TODO: can artifacts by typed?
-type Artifact = any;
+export interface Artifact {
+  abi: any[];
+  ast: any;
+  bytecode: string;
+  compiler: any;
+  contractName: string;
+  deployedBytecode: string;
+  deployedSourceMap: string;
+  fileName: string;
+  legacyAST?: any;
+  networks: any;
+  schemaVersion: string;
+  source: string;
+  sourceMap: string;
+  sourcePath: string;
+  updatedAt: string;
+}
 
 interface SourcePathMapping {
   [sourcePath: string]: Artifact[];
 }
 
-// TS-TODO: Review which members of this class could be private.
 export class BuildArtifacts {
   private sourcesToArtifacts: SourcePathMapping;
 
   public constructor(artifactsPaths: string[]) {
     this.sourcesToArtifacts = {};
 
     artifactsPaths.forEach(path => {
-      const artifact: any = fs.readJsonSync(path);
+      const artifact: Artifact = fs.readJsonSync(path);
       const sourcePath: string = this.getSourcePathFromArtifact(artifact);
       this.registerArtifactForSourcePath(sourcePath, artifact);
     });
@@ -38,6 +52,10 @@ export class BuildArtifacts {
     return flatten(values(this.sourcesToArtifacts));
   }
 
+  public getArtifactByName(name: string): Artifact | undefined {
+    return this.listArtifacts().find(a => a.contractName === name);
+  }
+
   public getArtifactsFromSourcePath(sourcePath: string): Artifact[] {
     return this.sourcesToArtifacts[sourcePath] || [];
   }
@@ -46,7 +64,7 @@ export class BuildArtifacts {
     return artifact.ast.absolutePath;
   }
 
-  public registerArtifactForSourcePath(sourcePath: string, artifact: Artifact): void {
+  private registerArtifactForSourcePath(sourcePath: string, artifact: Artifact): void {
     if (!this.sourcesToArtifacts[sourcePath]) this.sourcesToArtifacts[sourcePath] = [];
     this.sourcesToArtifacts[sourcePath].push(artifact);
   }

packages/lib/src/artifacts/Contract.ts

@@ -2,11 +2,12 @@ import { isAddress } from 'web3-utils';
 
 import ZWeb3 from './ZWeb3';
 import Contracts from './Contracts';
-import ContractAST from '../utils/ContractAST';
+import ContractAST, { ContractDefinitionFilter } from '../utils/ContractAST';
 import { StorageLayoutInfo } from '../validations/Storage';
 import { TransactionReceipt } from 'web3-core';
 import { Contract as Web3Contract } from 'web3-eth-contract';
 import { getArgTypeLabel } from '../utils/ABIs';
+import { Artifact } from './BuildArtifacts';
 
 /*
  * Contract is an interface that extends Web3's Contract interface, adding some properties and methods like:
@@ -27,29 +28,12 @@ export default interface Contract extends Web3Contract {
     transactionReceipt: TransactionReceipt;
   };
   schema: {
-    // openzeppelin schema specific.
     directory: string;
     linkedBytecode: string;
     linkedDeployedBytecode: string;
-    warnings: any;
     storageInfo: StorageLayoutInfo;
-
-    // Solidity schema.
-    schemaVersion: string;
-    contractName: string;
-    abi: any[];
-    bytecode: string;
-    deployedBytecode: string;
-    sourceMap: string;
-    deployedSourceMap: string;
-    source: string;
-    sourcePath: string;
-    ast: any;
-    legacyAST?: any;
-    compiler: any;
-    networks: any;
-    updatedAt: string;
-  };
+    warnings: any;
+  } & Artifact;
 }
 
 export enum ContractMethodMutability {
@@ -175,9 +159,7 @@ export function contractMethodsFromAst(
 }
 
 function getAstMethods(instance: Contract): any[] {
-  return new ContractAST(instance, null, {
-    nodesFilter: ['ContractDefinition'],
-  }).getMethods();
+  return new ContractAST(instance, null, ContractDefinitionFilter).getMethods();
 }
 
 function abiStateMutabilitiesFor(constant: ContractMethodMutability) {

packages/lib/src/utils/ContractAST.ts

@@ -46,6 +46,10 @@ interface ContractASTProps {
   nodesFilter?: string[];
 }
 
+export const ContractDefinitionFilter = {
+  nodesFilter: ['ContractDefinition'],
+};
+
 class NodeNotFoundError extends Error {
   public constructor(id, type) {
     super(`No AST nodes of type ${type} with id ${id} found.`);

packages/lib/src/validations/Constructors.ts

@@ -1,4 +1,19 @@
-// TS-TODO: Web3 typings?
-export function hasConstructor(contract: any): boolean {
-  return !!contract.schema.abi.find(fn => fn.type === 'constructor');
+import Contract from '../artifacts/Contract';
+import { BuildArtifacts } from '..';
+import ContractAST, { ContractDefinitionFilter } from '../utils/ContractAST';
+import { Artifact } from '../artifacts/BuildArtifacts';
+
+export function hasConstructor(contract: Contract, buildArtifacts: BuildArtifacts): boolean {
+  if (hasConstructorInABI(contract.schema)) return true;
+
+  const baseContracts = new ContractAST(contract, buildArtifacts, ContractDefinitionFilter)
+    .getLinearizedBaseContracts()
+    .map(node => buildArtifacts.getArtifactByName(node.name));
+
+  const baseContractsWithConstructors = baseContracts.filter(hasConstructorInABI);
+  return baseContractsWithConstructors.length > 0;
+}
+
+function hasConstructorInABI(contract: Artifact): boolean {
+  return !!contract.abi.find(fn => fn.type === 'constructor');
 }

packages/lib/src/validations/index.ts

@@ -15,7 +15,9 @@ import { compareStorageLayouts, Operation } from './Layout';
 import { hasInitialValuesInDeclarations } from './InitialValues';
 import { importsVanillaContracts } from './VanillaContracts';
 import Contract from '../artifacts/Contract.js';
-import ContractAST, { StorageInfo } from '../utils/ContractAST';
+import ContractAST, { StorageInfo, ContractDefinitionFilter } from '../utils/ContractAST';
+import { BuildArtifacts } from '..';
+import { getBuildArtifacts } from '../artifacts/BuildArtifacts';
 
 export interface ValidationInfo {
   hasConstructor: boolean;
@@ -28,13 +30,17 @@ export interface ValidationInfo {
   importsVanillaContracts?: string[];
 }
 
-export function validate(contract: Contract, existingContractInfo: any = {}, buildArtifacts?: any): any {
+export function validate(
+  contract: Contract,
+  existingContractInfo: any = {},
+  buildArtifacts: BuildArtifacts = getBuildArtifacts(),
+): any {
   checkArtifactsForImportedSources(contract, buildArtifacts);
   const storageValidation = validateStorage(contract, existingContractInfo, buildArtifacts);
   const uninitializedBaseContracts = [];
 
   return {
-    hasConstructor: hasConstructor(contract),
+    hasConstructor: hasConstructor(contract, buildArtifacts),
     hasSelfDestruct: hasSelfDestruct(contract),
     hasDelegateCall: hasDelegateCall(contract),
     hasInitialValuesInDeclarations: hasInitialValuesInDeclarations(contract),
@@ -110,8 +116,6 @@ function tryGetUninitializedBaseContracts(contract: Contract): string[] {
   }
 }
 
-function checkArtifactsForImportedSources(contract: Contract, buildArtifacts: any): void | never {
-  new ContractAST(contract, buildArtifacts, {
-    nodesFilter: ['ContractDefinition'],
-  }).getBaseContractsRecursively();
+function checkArtifactsForImportedSources(contract: Contract, buildArtifacts: BuildArtifacts): void | never {
+  new ContractAST(contract, buildArtifacts, ContractDefinitionFilter).getBaseContractsRecursively();
 }

packages/lib/test/mocks/mock-dependency/contracts/DependencyInvalid.sol

@@ -0,0 +1,8 @@
+pragma solidity ^0.5.0;
+
+contract DependencyWithConstructor {
+  uint256 public dependencyValue;
+  constructor() public {
+    dependencyValue = 42;
+  }
+}

packages/lib/test/src/validations/Validations.test.js

@@ -6,49 +6,61 @@ import Contracts from '../../../src/artifacts/Contracts';
 import { setVanillaContractsPackageName } from '../../../src/validations/VanillaContracts';
 
 describe('Validations', function() {
-  it('should warn when adding a contract with a constructor', async function() {
+  it('warns when adding a contract with a constructor', async function() {
     validate('WithConstructor').hasConstructor.should.be.true;
   });
 
-  it('should warn when adding a contract with a selfdestruct call', async function() {
+  it('warns when adding a contract with a parent with a constructor', async function() {
+    validate('WithParentConstructor').hasConstructor.should.be.true;
+  });
+
+  it('warns when adding a contract with an ancestor with a constructor', async function() {
+    validate('WithAncestorConstructor').hasConstructor.should.be.true;
+  });
+
+  it('warns when adding a contract with a parent from a dependency with a constructor', async function() {
+    validate('WithDependencyParentConstructor').hasConstructor.should.be.true;
+  });
+
+  it('warns when adding a contract with a selfdestruct call', async function() {
     validate('WithSelfDestruct').hasSelfDestruct.should.be.true;
   });
 
-  it('should warn when adding a contract whose parent has a selfdestruct call', async function() {
+  it('warns when adding a contract whose parent has a selfdestruct call', async function() {
     validate('WithParentWithSelfDestruct').hasSelfDestruct.should.be.true;
   });
 
-  it('should warn when adding a contract with a delegatecall call', async function() {
+  it('warns when adding a contract with a delegatecall call', async function() {
     validate('WithDelegateCall').hasDelegateCall.should.be.true;
   });
 
-  it('should warn when adding a contract whose parent has a delegatecall call', async function() {
+  it('warns when adding a contract whose parent has a delegatecall call', async function() {
     validate('WithParentWithDelegateCall').hasDelegateCall.should.be.true;
   });
 
   it('should not warn when adding a contract without initial values in fields declarations', async function() {
     validate('WithoutInitialValuesInFieldsDeclarations').hasInitialValuesInDeclarations.should.be.false;
   });
 
-  it('should warn when adding a contract with initial values in fields declarations', async function() {
+  it('warns when adding a contract with initial values in fields declarations', async function() {
     validate('WithInitialValuesInFieldsDeclarations').hasInitialValuesInDeclarations.should.be.true;
   });
 
-  it('should warn when adding a contract whose parent has initial values in fields declarations', async function() {
+  it('warns when adding a contract whose parent has initial values in fields declarations', async function() {
     validate('WithParentWithInitialValuesInFieldsDeclarations').hasInitialValuesInDeclarations.should.be.true;
   });
 
-  it('should warn on contract that extends vanilla openzeppelin contracts', async function() {
+  it('warns on contract that extends vanilla openzeppelin contracts', async function() {
     setVanillaContractsPackageName('mock-dependency/');
-    validate('WithVanillaBaseContract').importsVanillaContracts.should.deep.eq(['Greeter.sol']);
+    validate('WithVanillaBaseContract').importsVanillaContracts.should.include('Greeter.sol');
   });
 
   after(function() {
     setVanillaContractsPackageName('@openzeppelin/contracts/');
   });
 
   describe.skip('uninitialized base contracts', function() {
-    it('should warn when adding a contract with uninitialized base contracts', async function() {
+    it('warns when adding a contract with uninitialized base contracts', async function() {
       validate('WithBaseUninitialized').uninitializedBaseContracts.should.deep.eq([
         'WithInitialize',
         'AnotherWithInitialize',
@@ -63,7 +75,7 @@ describe('Validations', function() {
       validate('WithSimpleBaseUninitialized').uninitializedBaseContracts.should.be.empty;
     });
 
-    it('should warn when adding a contract without initializer with multiple base contracts that have initialize', async function() {
+    it('warns when adding a contract without initializer with multiple base contracts that have initialize', async function() {
       validate('ShouldHaveInitialize').uninitializedBaseContracts.should.be.deep.eq([
         'WithInitialize',
         'AnotherWithInitialize',
@@ -74,7 +86,7 @@ describe('Validations', function() {
       validate('DoesNotNeedAnInitialize').uninitializedBaseContracts.should.be.empty;
     });
 
-    it('should warn when adding a contract that inherits another contracts that does not initialize base contracts', async function() {
+    it('warns when adding a contract that inherits another contracts that does not initialize base contracts', async function() {
       validate('ExtendsFromShouldHaveInitialize').uninitializedBaseContracts.should.be.deep.eq([
         'WithInitialize',
         'AnotherWithInitialize',