From 1f2ec2ca072a5970b5ddc8d8b4b27bbe41053f56 Mon Sep 17 00:00:00 2001 From: Santiago Palladino Date: Fri, 13 Dec 2019 18:46:21 -0300 Subject: [PATCH 1/2] 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. --- .../cli/src/interface/ValidationLogger.ts | 17 ++++++++++++++++- .../test/interface/ValidationLogger.test.js | 5 +++++ packages/lib/contracts/mocks/Invalid.sol | 4 ++++ packages/lib/src/utils/ContractAST.ts | 10 +++++++--- .../lib/src/validations/VanillaContracts.ts | 19 +++++++++++++++++++ packages/lib/src/validations/index.ts | 10 +++++++++- .../test/src/validations/Validations.test.js | 10 ++++++++++ 7 files changed, 70 insertions(+), 5 deletions(-) create mode 100644 packages/lib/src/validations/VanillaContracts.ts diff --git a/packages/cli/src/interface/ValidationLogger.ts b/packages/cli/src/interface/ValidationLogger.ts index 83609f641..a623d7dc7 100644 --- a/packages/cli/src/interface/ValidationLogger.ts +++ b/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 { diff --git a/packages/cli/test/interface/ValidationLogger.test.js b/packages/cli/test/interface/ValidationLogger.test.js index 64cda2581..b78fe7718 100644 --- a/packages/cli/test/interface/ValidationLogger.test.js +++ b/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() { diff --git a/packages/lib/contracts/mocks/Invalid.sol b/packages/lib/contracts/mocks/Invalid.sol index bf5499ecb..9b4a96aad 100644 --- a/packages/lib/contracts/mocks/Invalid.sol +++ b/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 { } \ No newline at end of file diff --git a/packages/lib/src/utils/ContractAST.ts b/packages/lib/src/utils/ContractAST.ts index 1a85553aa..d801a0f16 100644 --- a/packages/lib/src/utils/ContractAST.ts +++ b/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; + private imports: Set; private nodes: NodeMapping; private types: TypeInfoMapping; private nodesFilter: string[]; @@ -96,6 +96,10 @@ export default class ContractAST { ); } + public getImports(): Set { + 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 => { diff --git a/packages/lib/src/validations/VanillaContracts.ts b/packages/lib/src/validations/VanillaContracts.ts new file mode 100644 index 000000000..9f22e7da2 --- /dev/null +++ b/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[] | null { + const ast = new ContractAST(contract, buildArtifacts, { nodesFilter: ['ContractDefinition'] }); + 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 : null; +} + +// Used for testing purposes; +export function setVanillaContractsPackageName(value: string) { + VANILLA_CONTRACTS = value; +} diff --git a/packages/lib/src/validations/index.ts b/packages/lib/src/validations/index.ts index f782821fc..a3af67cfc 100644 --- a/packages/lib/src/validations/index.ts +++ b/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) ); } diff --git a/packages/lib/test/src/validations/Validations.test.js b/packages/lib/test/src/validations/Validations.test.js index e8a4fcf89..3e3bd1bb6 100644 --- a/packages/lib/test/src/validations/Validations.test.js +++ b/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([ From f5e802cb58ac190e1bc8b505432b977625228b6b Mon Sep 17 00:00:00 2001 From: Santiago Palladino Date: Mon, 16 Dec 2019 18:04:46 -0300 Subject: [PATCH 2/2] Implement suggestions by @frangio --- packages/lib/src/validations/VanillaContracts.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/packages/lib/src/validations/VanillaContracts.ts b/packages/lib/src/validations/VanillaContracts.ts index 9f22e7da2..9334038a0 100644 --- a/packages/lib/src/validations/VanillaContracts.ts +++ b/packages/lib/src/validations/VanillaContracts.ts @@ -3,14 +3,14 @@ import ContractAST from '../utils/ContractAST'; let VANILLA_CONTRACTS = '@openzeppelin/contracts/'; -export function importsVanillaContracts(contract: Contract, buildArtifacts?: any): string[] | null { - const ast = new ContractAST(contract, buildArtifacts, { nodesFilter: ['ContractDefinition'] }); +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 : null; + return illegalImports.length > 0 ? illegalImports : undefined; } // Used for testing purposes;