Warn when importing vanilla contracts package (#1335)

* Warn when importing vanilla contracts package

Users should only install contracts from oz/contracts-eth-package, not oz/contracts. Warn about this when an import for the vanilla contracts package is found.

* Implement suggestions by @frangio

packages/cli/src/interface/ValidationLogger.ts

@@ -11,11 +11,12 @@ import {
 } from '@openzeppelin/upgrades';
 import { ContractInterface } from '../models/files/NetworkFile';
 
-const DOCS_HOME = 'https://docs.openzeppelin.com/sdk/2.5';
+const DOCS_HOME = 'https://docs.openzeppelin.com/sdk/2.6';
 const DANGEROUS_OPERATIONS_LINK = `${DOCS_HOME}/writing-contracts#potentially-unsafe-operations`;
 const AVOID_INITIAL_VALUES_LINK = `${DOCS_HOME}/writing-contracts#avoid-initial-values-in-field-declarations`;
 const INITIALIZERS_LINK = `${DOCS_HOME}/writing-contracts#initializers`;
 const STORAGE_CHECKS_LINK = `${DOCS_HOME}/writing-contracts#modifying-your-contracts`;
+const VANILLA_CONTRACTS_LINK = `${DOCS_HOME}/linking#linking-the-contracts-ethereum-package`;
 
 export default class ValidationLogger {
   public contract: Contract;
@@ -39,6 +40,7 @@ export default class ValidationLogger {
       uninitializedBaseContracts,
       storageDiff,
       storageUncheckedVars,
+      importsVanillaContracts,
     } = validations;
 
     this.logHasConstructor(hasConstructor);
@@ -48,6 +50,19 @@ export default class ValidationLogger {
     this.logUncheckedVars(storageUncheckedVars);
     this.logUninitializedBaseContracts(uninitializedBaseContracts);
     this.logStorageLayoutDiffs(storageDiff, getStorageLayout(this.contract, buildArtifacts));
+    this.logImportsVanillaContracts(importsVanillaContracts);
+  }
+
+  public logImportsVanillaContracts(vanillaContracts: string[] | null): void {
+    if (!vanillaContracts) return;
+    Loggy.noSpin.warn(
+      __filename,
+      'logImportsVanillaContracts',
+      `validation-imports-vanilla-contracts`,
+      `- Contract ${this.contractName} imports ${vanillaContracts.join(
+        ', ',
+      )} from @openzeppelin/contracts. Use @openzeppelin/contracts-ethereum-package instead. See ${VANILLA_CONTRACTS_LINK}.`,
+    );
   }
 
   public logHasSelfDestruct(hasSelfDestruct: boolean): void {

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

@@ -50,6 +50,11 @@ describe('ValidationLogger', function() {
       validationLogger().log({ hasInitialValuesInDeclarations: true });
       this.logs.warns[0].should.match(/sets an initial value/);
     });
+
+    it('logs when importing openzeppelin-contracts', async function() {
+      validationLogger().log({ importsVanillaContracts: ['Foo.sol', 'Bar.sol'] });
+      this.logs.warns[0].should.match(/@openzeppelin\/contracts/);
+    });
   });
 
   describe('storage', function() {

packages/lib/contracts/mocks/Invalid.sol

@@ -1,5 +1,7 @@
 pragma solidity ^0.5.0;
 
+import "mock-dependency/contracts/Greeter.sol";
+
 contract WithConstructor {
   uint256 public value;
 
@@ -58,3 +60,5 @@ contract WithParentWithDelegateCall is WithDelegateCall {
     return "WithParentWithDelegateCall";
   }
 }
+
+contract WithVanillaBaseContract is Greeter { }

packages/lib/src/utils/ContractAST.ts

@@ -63,7 +63,7 @@ class MultipleNodesFoundError extends Error {
 export default class ContractAST {
   private artifacts: BuildArtifacts;
   private contract: Contract;
-  private imports: Set<any>;
+  private imports: Set<string>;
   private nodes: NodeMapping;
   private types: TypeInfoMapping;
   private nodesFilter: string[];
@@ -96,6 +96,10 @@ export default class ContractAST {
     );
   }
 
+  public getImports(): Set<string> {
+    return this.imports;
+  }
+
   public getMethods(attributes?: string[]): any {
     const baseContracts = this.getLinearizedBaseContracts();
     return flatten(baseContracts.map(contract => contract.nodes))
@@ -140,7 +144,7 @@ export default class ContractAST {
       } catch (err) {
         if (err instanceof NodeNotFoundError) {
           throw new Error(
-            `Cannot find source data for contract ${name} (base contract of ${this.contract.schema.contractName}). This often happens because either:\n- An incremental compilation step went wrong. Clear your build folder and recompile.\n- There is more than one contract named ${name} in your project (including dependencies). Make sure all contracts have a unique name, and that you are not importing dependencies with duplicated contract names (for example, openzeppelin-eth and openzeppelin-solidity).`,
+            `Cannot find source data for contract ${name} (base contract of ${this.contract.schema.contractName}). This often happens because either:\n- An incremental compilation step went wrong. Clear your build folder and recompile.\n- There is more than one contract named ${name} in your project (including dependencies). Make sure all contracts have a unique name, and that you are not importing dependencies with duplicated contract names (for example, @openzeppelin/contracts-ethereum-package and @openzeppelin/contracts).`,
           );
         } else {
           throw err;
@@ -174,7 +178,7 @@ export default class ContractAST {
     ast.nodes
       .filter(node => node.nodeType === 'ImportDirective')
       .map(node => node.absolutePath)
-      .forEach(importPath => {
+      .forEach((importPath: string) => {
         if (this.imports.has(importPath)) return;
         this.imports.add(importPath);
         this.artifacts.getArtifactsFromSourcePath(importPath).forEach(importedArtifact => {

packages/lib/src/validations/VanillaContracts.ts

@@ -0,0 +1,19 @@
+import Contract from '../artifacts/Contract';
+import ContractAST from '../utils/ContractAST';
+
+let VANILLA_CONTRACTS = '@openzeppelin/contracts/';
+
+export function importsVanillaContracts(contract: Contract, buildArtifacts?: any): string[] | undefined {
+  const ast = new ContractAST(contract, buildArtifacts, { nodesFilter: [] });
+  const illegalImports = [...ast.getImports()]
+    .filter(i => i.startsWith(VANILLA_CONTRACTS))
+    .map(i => i.slice(VANILLA_CONTRACTS.length))
+    .map(i => i.replace(/^contracts\//, ''));
+
+  return illegalImports.length > 0 ? illegalImports : undefined;
+}
+
+// Used for testing purposes;
+export function setVanillaContractsPackageName(value: string) {
+  VANILLA_CONTRACTS = value;
+}

packages/lib/src/validations/index.ts

@@ -13,6 +13,7 @@ import { getUninitializedBaseContracts } from './Initializers';
 import { getStorageLayout, getStructsOrEnums } from './Storage';
 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';
 
@@ -24,6 +25,7 @@ export interface ValidationInfo {
   uninitializedBaseContracts: any[];
   storageUncheckedVars?: StorageInfo[];
   storageDiff?: Operation[];
+  importsVanillaContracts?: string[];
 }
 
 export function validate(contract: Contract, existingContractInfo: any = {}, buildArtifacts?: any): any {
@@ -36,6 +38,7 @@ export function validate(contract: Contract, existingContractInfo: any = {}, bui
     hasSelfDestruct: hasSelfDestruct(contract),
     hasDelegateCall: hasDelegateCall(contract),
     hasInitialValuesInDeclarations: hasInitialValuesInDeclarations(contract),
+    importsVanillaContracts: importsVanillaContracts(contract, buildArtifacts),
     uninitializedBaseContracts,
     ...storageValidation,
   };
@@ -54,6 +57,10 @@ export function newValidationErrors(validations: any, existingValidations: any =
     ),
     storageUncheckedVars: difference(validations.storageUncheckedVars, existingValidations.storageUncheckedVars),
     storageDiff: validations.storageDiff,
+    importsVanillaContracts: difference(
+      validations.importsVanillaContracts,
+      existingValidations.importsVanillaContracts,
+    ),
   };
 }
 
@@ -64,7 +71,8 @@ export function validationPasses(validations: any): boolean {
     !validations.hasSelfDestruct &&
     !validations.hasDelegateCall &&
     !validations.hasInitialValuesInDeclarations &&
-    isEmpty(validations.uninitializedBaseContracts)
+    isEmpty(validations.uninitializedBaseContracts) &&
+    isEmpty(validations.importsVanillaContracts)
   );
 }
 

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

@@ -3,6 +3,7 @@ require('../../setup');
 
 import { validate as validateContract } from '../../../src/validations';
 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() {
@@ -37,6 +38,15 @@ describe('Validations', function() {
     validate('WithParentWithInitialValuesInFieldsDeclarations').hasInitialValuesInDeclarations.should.be.true;
   });
 
+  it('should warn on contract that extends vanilla openzeppelin contracts', async function() {
+    setVanillaContractsPackageName('mock-dependency/');
+    validate('WithVanillaBaseContract').importsVanillaContracts.should.deep.eq(['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() {
       validate('WithBaseUninitialized').uninitializedBaseContracts.should.deep.eq([