From 97cafff947c9d2ce6a3d4b5cc2ab22cbc337d987 Mon Sep 17 00:00:00 2001 From: Igor Yalovoy Date: Thu, 6 Feb 2020 17:11:16 +0800 Subject: [PATCH 1/4] Push only required contract during create command --- packages/cli/src/commands/push.ts | 13 ++++++++---- packages/cli/src/models/TestHelper.ts | 2 +- .../src/models/network/NetworkController.ts | 21 ++++++++++++------- packages/cli/src/scripts/interfaces.ts | 1 + packages/cli/src/scripts/push.ts | 3 ++- packages/cli/src/telemetry/index.ts | 2 +- 6 files changed, 28 insertions(+), 14 deletions(-) diff --git a/packages/cli/src/commands/push.ts b/packages/cli/src/commands/push.ts index ade2b2268..2286f9744 100644 --- a/packages/cli/src/commands/push.ts +++ b/packages/cli/src/commands/push.ts @@ -3,6 +3,7 @@ import { ZWeb3 } from '@openzeppelin/upgrades'; import add from './add'; import push from '../scripts/push'; +import { PushParams } from '../scripts/interfaces'; import Session from '../models/network/Session'; import { compile } from '../models/compiler/Compiler'; import Dependency from '../models/dependency/Dependency'; @@ -42,6 +43,7 @@ async function commandActions(options: any): Promise { async function action(options: any): Promise { const { + contractAlias, force, deployDependencies, reset: reupload, @@ -66,7 +68,7 @@ async function action(options: any): Promise { }); const promptDeployDependencies = await promptForDeployDependencies(deployDependencies, network, interactive); - const pushArguments = { + const pushArguments: PushParams = { deployProxyAdmin, deployProxyFactory, force, @@ -76,7 +78,10 @@ async function action(options: any): Promise { ...promptDeployDependencies, }; - if (!options.skipTelemetry) await Telemetry.report('push', pushArguments, interactive); + if (contractAlias) pushArguments.contractAliases = [contractAlias]; + + if (!options.skipTelemetry) + await Telemetry.report('push', (pushArguments as unknown) as Record, interactive); await push(pushArguments); if (!options.dontExitProcess && process.env.NODE_ENV !== 'test') process.exit(0); } @@ -89,9 +94,9 @@ async function runActionIfRequested(externalOptions: any): Promise { return action(options); } -async function runActionIfNeeded(contractName: string, network: string, options: any): Promise { +async function runActionIfNeeded(contractAlias: string, network: string, options: any): Promise { if (!options.interactive) return; - await action({ ...options, dontExitProcess: true, skipTelemetry: true }); + await action({ ...options, dontExitProcess: true, skipTelemetry: true, contractAlias }); } async function promptForDeployDependencies( diff --git a/packages/cli/src/models/TestHelper.ts b/packages/cli/src/models/TestHelper.ts index 06c894d4c..2a24ce272 100644 --- a/packages/cli/src/models/TestHelper.ts +++ b/packages/cli/src/models/TestHelper.ts @@ -14,7 +14,7 @@ export default async function( ): Promise { const controller = new NetworkController('test', txParams, networkFile); await controller.deployDependencies(); - await controller.push(false, true); + await controller.push(undefined, { reupload: false, force: true }); return controller.project; } diff --git a/packages/cli/src/models/network/NetworkController.ts b/packages/cli/src/models/network/NetworkController.ts index 007283d27..aee986e2b 100644 --- a/packages/cli/src/models/network/NetworkController.ts +++ b/packages/cli/src/models/network/NetworkController.ts @@ -122,13 +122,13 @@ export default class NetworkController { } // DeployerController - public async push(reupload = false, force = false): Promise { + public async push(aliases: string[], { reupload = false, force = false } = {}): Promise { const changedLibraries = this._solidityLibsForPush(!reupload); - const contracts = this._contractsListForPush(!reupload, changedLibraries); + const contractObjects = this._contractsListForPush(aliases, !reupload, changedLibraries); const buildArtifacts = getBuildArtifacts(); // ValidateContracts also extends each contract class with validation errors and storage info - if (!this.validateContracts(contracts, buildArtifacts) && !force) { + if (!this.validateContracts(contractObjects, buildArtifacts) && !force) { throw Error( 'One or more contracts have validation errors. Please review the items listed above and fix them, or run this command again with the --force option.', ); @@ -140,11 +140,11 @@ export default class NetworkController { this.checkNotFrozen(); await this.uploadSolidityLibs(changedLibraries); - await Promise.all([this.uploadContracts(contracts), this.unsetContracts()]); + await Promise.all([this.uploadContracts(contractObjects), this.unsetContracts()]); await this._unsetSolidityLibs(); - if (isEmpty(contracts) && isEmpty(changedLibraries)) { + if (isEmpty(contractObjects) && isEmpty(changedLibraries)) { Loggy.noSpin(__filename, 'push', `after-push`, `All contracts are up to date`); } else { Loggy.noSpin(__filename, 'push', `after-push`, `All contracts have been deployed`); @@ -179,10 +179,17 @@ export default class NetworkController { } // Contract model - private _contractsListForPush(onlyChanged = false, changedLibraries: Contract[] = []): [string, Contract][] { + private _contractsListForPush( + aliases: string[], + onlyChanged = false, + changedLibraries: Contract[] = [], + ): [string, Contract][] { const newVersion = this._newVersionRequired(); - return toPairs(this.projectFile.contracts) + aliases = aliases || Object.keys(this.projectFile.contracts); + + return aliases + .map(alias => [alias, this.projectFile.contracts[alias]]) .map(([contractAlias, contractName]): [string, Contract] => [contractAlias, Contracts.getFromLocal(contractName)]) .filter( ([contractAlias, contract]) => diff --git a/packages/cli/src/scripts/interfaces.ts b/packages/cli/src/scripts/interfaces.ts index 5dc3bb7b3..761504ce5 100644 --- a/packages/cli/src/scripts/interfaces.ts +++ b/packages/cli/src/scripts/interfaces.ts @@ -108,6 +108,7 @@ export interface UnpackParams { } export interface PushParams extends Network { + contractAliases?: string[]; force?: boolean; reupload?: boolean; deployDependencies?: boolean; diff --git a/packages/cli/src/scripts/push.ts b/packages/cli/src/scripts/push.ts index 4aa747837..eb3336b2e 100644 --- a/packages/cli/src/scripts/push.ts +++ b/packages/cli/src/scripts/push.ts @@ -2,6 +2,7 @@ import NetworkController from '../models/network/NetworkController'; import { PushParams } from './interfaces'; export default async function push({ + contractAliases, network, deployDependencies, deployProxyAdmin, @@ -17,7 +18,7 @@ export default async function push({ if (deployDependencies) await controller.deployDependencies(); if (deployProxyAdmin) await controller.deployProxyAdmin(); if (deployProxyFactory) await controller.deployProxyFactory(); - await controller.push(reupload, force); + await controller.push(contractAliases, { reupload, force }); const { appAddress } = controller; } finally { controller.writeNetworkPackageIfNeeded(); diff --git a/packages/cli/src/telemetry/index.ts b/packages/cli/src/telemetry/index.ts index b5ed761f6..a5febe6e2 100644 --- a/packages/cli/src/telemetry/index.ts +++ b/packages/cli/src/telemetry/index.ts @@ -33,7 +33,7 @@ export interface UserEnvironment { export default { DISABLE_TELEMETRY: !!process.env.OPENZEPPELIN_DISABLE_TELEMETRY, - async report(commandName: string, params: { [key: string]: unknown }, interactive: boolean): Promise { + async report(commandName: string, params: Record, interactive: boolean): Promise { const telemetryOptions = await checkOptIn(interactive); if (telemetryOptions === undefined || !telemetryOptions.optIn) return; From 3a883233dd5b3c0670f5f6752e97944e85da0bb0 Mon Sep 17 00:00:00 2001 From: Igor Yalovoy Date: Fri, 7 Feb 2020 17:27:35 +0800 Subject: [PATCH 2/4] Refactor NetworkController#push --- packages/cli/src/models/network/NetworkController.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/cli/src/models/network/NetworkController.ts b/packages/cli/src/models/network/NetworkController.ts index aee986e2b..7f655619f 100644 --- a/packages/cli/src/models/network/NetworkController.ts +++ b/packages/cli/src/models/network/NetworkController.ts @@ -122,7 +122,7 @@ export default class NetworkController { } // DeployerController - public async push(aliases: string[], { reupload = false, force = false } = {}): Promise { + public async push(aliases: string[] | undefined, { reupload = false, force = false } = {}): Promise { const changedLibraries = this._solidityLibsForPush(!reupload); const contractObjects = this._contractsListForPush(aliases, !reupload, changedLibraries); const buildArtifacts = getBuildArtifacts(); From ec099d1d773078ce342d01dd55edc83e84841d15 Mon Sep 17 00:00:00 2001 From: Igor Yalovoy Date: Fri, 7 Feb 2020 17:58:36 +0800 Subject: [PATCH 3/4] Add push tests --- .../src/models/network/NetworkController.ts | 3 +- packages/cli/test/scripts/push.test.js | 102 +++++++++++++++++- 2 files changed, 99 insertions(+), 6 deletions(-) diff --git a/packages/cli/src/models/network/NetworkController.ts b/packages/cli/src/models/network/NetworkController.ts index 7f655619f..042252df1 100644 --- a/packages/cli/src/models/network/NetworkController.ts +++ b/packages/cli/src/models/network/NetworkController.ts @@ -180,14 +180,13 @@ export default class NetworkController { // Contract model private _contractsListForPush( - aliases: string[], + aliases: string[] | undefined, onlyChanged = false, changedLibraries: Contract[] = [], ): [string, Contract][] { const newVersion = this._newVersionRequired(); aliases = aliases || Object.keys(this.projectFile.contracts); - return aliases .map(alias => [alias, this.projectFile.contracts[alias]]) .map(([contractAlias, contractName]): [string, Contract] => [contractAlias, Contracts.getFromLocal(contractName)]) diff --git a/packages/cli/test/scripts/push.test.js b/packages/cli/test/scripts/push.test.js index 6ca88eb69..33e0352bd 100644 --- a/packages/cli/test/scripts/push.test.js +++ b/packages/cli/test/scripts/push.test.js @@ -1,7 +1,7 @@ 'use strict'; require('../setup'); -const zosLib = require('@openzeppelin/upgrades'); // eslint-disable-line @typescript-eslint/no-var-requires +const upgrades = require('@openzeppelin/upgrades'); // eslint-disable-line @typescript-eslint/no-var-requires import { ZWeb3, Contracts, App, Package, ProxyAdmin, ProxyFactory } from '@openzeppelin/upgrades'; import { accounts } from '@openzeppelin/test-environment'; @@ -169,7 +169,7 @@ describe('push script', function() { }); it('should refuse to redeploy a contract if validation throws', async function() { - sinon.stub(zosLib, 'validate').throws(new Error('Stubbed error during contract validation')); + sinon.stub(upgrades, 'validate').throws(new Error('Stubbed error during contract validation')); await push({ networkFile: this.networkFile, network, @@ -179,7 +179,7 @@ describe('push script', function() { }); it('should redeploy contract skipping errors', async function() { - sinon.stub(zosLib, 'validate').throws(new Error('Stubbed error during contract validation')); + sinon.stub(upgrades, 'validate').throws(new Error('Stubbed error during contract validation')); await push({ force: true, networkFile: this.networkFile, @@ -195,6 +195,98 @@ describe('push script', function() { }); }; + const shouldDeployOnlySpecifiedContracts = function() { + describe('when contracts specified explicitly', function() { + beforeEach('loading previous addresses', function() { + this.previousAddress = this.networkFile.contract('Impl').address; + this.withLibraryPreviousAddress = this.networkFile.contract('WithLibraryImpl').address; + }); + + it('should not deploy contracts if unmodified', async function() { + await push({ contractAliases: ['Impl'], networkFile: this.networkFile, network, txParams }); + this.networkFile.contract('Impl').address.should.eq(this.previousAddress); + }); + + it('should deploy unmodified contract if forced', async function() { + await push({ + contractAliases: ['Impl'], + networkFile: this.networkFile, + network, + txParams, + reupload: true, + }); + this.networkFile.contract('Impl').address.should.not.eq(this.previousAddress); + }); + + it('should deploy contracts if modified', async function() { + modifyBytecode.call(this, 'Impl'); + await push({ contractAliases: ['Impl'], networkFile: this.networkFile, network, txParams }); + this.networkFile.contract('Impl').address.should.not.eq(this.previousAddress); + }); + + it('should not deploy contracts if library is modified', async function() { + modifyLibraryBytecode.call(this, 'UintLib'); + await push({ contractAliases: ['Impl'], networkFile: this.networkFile, network, txParams }); + this.networkFile.contract('WithLibraryImpl').address.should.eq(this.withLibraryPreviousAddress); + }); + + context('validations', function() { + beforeEach('modifying contracts', function() { + modifyBytecode.call(this, 'Impl'); + modifyStorageInfo.call(this, 'Impl'); + }); + + it('should refuse to deploy a contract if storage is incompatible', async function() { + await push({ + contractAliases: ['Impl'], + networkFile: this.networkFile, + network, + txParams, + }).should.be.rejectedWith(/have validation errors/); + this.networkFile.contract('Impl').address.should.eq(this.previousAddress); + }); + + it('should deploy contract ignoring warnings', async function() { + await push({ + contractAliases: ['Impl'], + force: true, + networkFile: this.networkFile, + network, + txParams, + }); + this.networkFile.contract('Impl').address.should.not.eq(this.previousAddress); + }); + + it('should refuse to deploy a contract if validation throws', async function() { + sinon.stub(upgrades, 'validate').throws(new Error('Stubbed error during contract validation')); + await push({ + contractAliases: ['Impl'], + networkFile: this.networkFile, + network, + txParams, + }).should.be.rejectedWith(/have validation errors/); + this.networkFile.contract('Impl').address.should.eq(this.previousAddress); + }); + + it('should deploy contract skipping errors', async function() { + sinon.stub(upgrades, 'validate').throws(new Error('Stubbed error during contract validation')); + await push({ + contractAliases: ['Impl'], + force: true, + networkFile: this.networkFile, + network, + txParams, + }); + this.networkFile.contract('Impl').address.should.not.eq(this.previousAddress); + }); + + afterEach(function() { + sinon.restore(); + }); + }); + }); + }; + const shouldValidateContracts = function() { describe('validations', function() { beforeEach('capturing log output', function() { @@ -521,7 +613,7 @@ describe('push script', function() { this.networkFile = new NetworkFile(projectFile, network); }); - describe('on push', function() { + describe.only('on push', function() { beforeEach('pushing', async function() { await push({ network, txParams, networkFile: this.networkFile }); const newProjectFile = new ProjectFile('test/mocks/packages/package-with-contracts-v2.zos.json'); @@ -531,6 +623,7 @@ describe('push script', function() { shouldDeployApp(); shouldDeployProvider(); shouldDeployContracts(); + shouldDeployOnlySpecifiedContracts(); shouldRegisterContractsInDirectory(); shouldRedeployContracts(); shouldValidateContracts(); @@ -687,6 +780,7 @@ describe('push script', function() { }); shouldDeployContracts(); + shouldDeployOnlySpecifiedContracts(); shouldValidateContracts(); shouldRedeployContracts(); shouldDeleteContracts({ unregisterFromDirectory: false }); From f2284fc620d538171ac687bcc5a1b4fb40c2f121 Mon Sep 17 00:00:00 2001 From: Igor Yalovoy Date: Fri, 7 Feb 2020 18:30:36 +0800 Subject: [PATCH 4/4] Add push tests --- packages/cli/test/scripts/push.test.js | 100 +++++++++++++++++++------ 1 file changed, 78 insertions(+), 22 deletions(-) diff --git a/packages/cli/test/scripts/push.test.js b/packages/cli/test/scripts/push.test.js index 33e0352bd..90be9e832 100644 --- a/packages/cli/test/scripts/push.test.js +++ b/packages/cli/test/scripts/push.test.js @@ -1,6 +1,8 @@ 'use strict'; require('../setup'); +import { expect } from 'chai'; + const upgrades = require('@openzeppelin/upgrades'); // eslint-disable-line @typescript-eslint/no-var-requires import { ZWeb3, Contracts, App, Package, ProxyAdmin, ProxyFactory } from '@openzeppelin/upgrades'; import { accounts } from '@openzeppelin/test-environment'; @@ -202,32 +204,86 @@ describe('push script', function() { this.withLibraryPreviousAddress = this.networkFile.contract('WithLibraryImpl').address; }); - it('should not deploy contracts if unmodified', async function() { - await push({ contractAliases: ['Impl'], networkFile: this.networkFile, network, txParams }); - this.networkFile.contract('Impl').address.should.eq(this.previousAddress); - }); + describe('when a NetworkFile is empty', function() { + beforeEach('purging NetworkFile', function() { + this.networkFile.data.contracts = {}; + this.networkFile.data.proxies = {}; + }); - it('should deploy unmodified contract if forced', async function() { - await push({ - contractAliases: ['Impl'], - networkFile: this.networkFile, - network, - txParams, - reupload: true, + it('should record contracts in network file', async function() { + await push({ contractAliases: ['Impl'], networkFile: this.networkFile, network, txParams }); + + const contract = this.networkFile.contract('Impl'); + contract.address.should.be.nonzeroAddress; + contract.localBytecodeHash.should.not.be.empty; + contract.storage.should.not.be.empty; + contract.types.should.not.be.empty; + const deployed = await ImplV1.at(contract.address); + (await deployed.methods.say().call()).should.eq('V1'); }); - this.networkFile.contract('Impl').address.should.not.eq(this.previousAddress); - }); - it('should deploy contracts if modified', async function() { - modifyBytecode.call(this, 'Impl'); - await push({ contractAliases: ['Impl'], networkFile: this.networkFile, network, txParams }); - this.networkFile.contract('Impl').address.should.not.eq(this.previousAddress); + it('should not record not specified contracts in network file', async function() { + await push({ contractAliases: ['Impl'], networkFile: this.networkFile, network, txParams }); + + const contract = this.networkFile.contract('WithLibraryImpl'); + expect(contract).to.be.undefined; + }); + + it('should deploy contract instance', async function() { + await push({ contractAliases: ['Impl'], networkFile: this.networkFile, network, txParams }); + + const address = this.networkFile.contract('Impl').address; + const deployed = await ImplV1.at(address); + (await deployed.methods.say().call()).should.eq('V1'); + }); + + it('should deploy required libraries', async function() { + await push({ contractAliases: ['WithLibraryImpl'], networkFile: this.networkFile, network, txParams }); + + const address = this.networkFile.solidityLib('UintLib').address; + const code = await ZWeb3.eth.getCode(address); + const uintLib = Contracts.getFromLocal('UintLib'); + code.length.should.eq(uintLib.schema.deployedBytecode.length).and.be.greaterThan(40); + }); + + it('should deploy and link contracts that require libraries', async function() { + await push({ contractAliases: ['WithLibraryImpl'], networkFile: this.networkFile, network, txParams }); + + const address = this.networkFile.contract('WithLibraryImpl').address; + const deployed = await WithLibraryImplV1.at(address); + const result = await deployed.methods.double(10).call(); + result.should.eq('20'); + }); }); - it('should not deploy contracts if library is modified', async function() { - modifyLibraryBytecode.call(this, 'UintLib'); - await push({ contractAliases: ['Impl'], networkFile: this.networkFile, network, txParams }); - this.networkFile.contract('WithLibraryImpl').address.should.eq(this.withLibraryPreviousAddress); + describe('on a redeploy', function() { + it('should not deploy contracts if unmodified', async function() { + await push({ contractAliases: ['Impl'], networkFile: this.networkFile, network, txParams }); + this.networkFile.contract('Impl').address.should.eq(this.previousAddress); + }); + + it('should deploy unmodified contract if forced', async function() { + await push({ + contractAliases: ['Impl'], + networkFile: this.networkFile, + network, + txParams, + reupload: true, + }); + this.networkFile.contract('Impl').address.should.not.eq(this.previousAddress); + }); + + it('should deploy contracts if modified', async function() { + modifyBytecode.call(this, 'Impl'); + await push({ contractAliases: ['Impl'], networkFile: this.networkFile, network, txParams }); + this.networkFile.contract('Impl').address.should.not.eq(this.previousAddress); + }); + + it('should not deploy contracts if library is modified', async function() { + modifyLibraryBytecode.call(this, 'UintLib'); + await push({ contractAliases: ['Impl'], networkFile: this.networkFile, network, txParams }); + this.networkFile.contract('WithLibraryImpl').address.should.eq(this.withLibraryPreviousAddress); + }); }); context('validations', function() { @@ -613,7 +669,7 @@ describe('push script', function() { this.networkFile = new NetworkFile(projectFile, network); }); - describe.only('on push', function() { + describe('on push', function() { beforeEach('pushing', async function() { await push({ network, txParams, networkFile: this.networkFile }); const newProjectFile = new ProjectFile('test/mocks/packages/package-with-contracts-v2.zos.json');