Skip to content
This repository has been archived by the owner on Jan 24, 2022. It is now read-only.

Push only required contract during create command #1426

Merged
merged 4 commits into from
Feb 7, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 9 additions & 4 deletions packages/cli/src/commands/push.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -42,6 +43,7 @@ async function commandActions(options: any): Promise<void> {

async function action(options: any): Promise<void> {
const {
contractAlias,
force,
deployDependencies,
reset: reupload,
Expand All @@ -66,7 +68,7 @@ async function action(options: any): Promise<void> {
});
const promptDeployDependencies = await promptForDeployDependencies(deployDependencies, network, interactive);

const pushArguments = {
const pushArguments: PushParams = {
deployProxyAdmin,
deployProxyFactory,
force,
Expand All @@ -76,7 +78,10 @@ async function action(options: any): Promise<void> {
...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<string, unknown>, interactive);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm curious: why was this cast needed when changing from { [key: string]: unknown } to Record<string, unknown> in the report method definition? I thought that both were semantically the same.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The cast is needed not because of the change of report method but because of change pushParams to type PushParams which doesn't have required index signature.

await push(pushArguments);
if (!options.dontExitProcess && process.env.NODE_ENV !== 'test') process.exit(0);
}
Expand All @@ -89,9 +94,9 @@ async function runActionIfRequested(externalOptions: any): Promise<void> {
return action(options);
}

async function runActionIfNeeded(contractName: string, network: string, options: any): Promise<void> {
async function runActionIfNeeded(contractAlias: string, network: string, options: any): Promise<void> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This contractName was not even used, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct.

if (!options.interactive) return;
await action({ ...options, dontExitProcess: true, skipTelemetry: true });
await action({ ...options, dontExitProcess: true, skipTelemetry: true, contractAlias });
}

async function promptForDeployDependencies(
Expand Down
2 changes: 1 addition & 1 deletion packages/cli/src/models/TestHelper.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ export default async function(
): Promise<ProxyAdminProject | AppProject> {
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;
}
20 changes: 13 additions & 7 deletions packages/cli/src/models/network/NetworkController.ts
Original file line number Diff line number Diff line change
Expand Up @@ -122,13 +122,13 @@ export default class NetworkController {
}

// DeployerController
public async push(reupload = false, force = false): Promise<void | never> {
public async push(aliases: string[] | undefined, { reupload = false, force = false } = {}): Promise<void | never> {
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.',
);
Expand All @@ -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`);
Expand Down Expand Up @@ -179,10 +179,16 @@ export default class NetworkController {
}

// Contract model
private _contractsListForPush(onlyChanged = false, changedLibraries: Contract[] = []): [string, Contract][] {
private _contractsListForPush(
aliases: string[] | undefined,
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]) =>
Expand Down
1 change: 1 addition & 0 deletions packages/cli/src/scripts/interfaces.ts
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,7 @@ export interface UnpackParams {
}

export interface PushParams extends Network {
contractAliases?: string[];
force?: boolean;
reupload?: boolean;
deployDependencies?: boolean;
Expand Down
3 changes: 2 additions & 1 deletion packages/cli/src/scripts/push.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import NetworkController from '../models/network/NetworkController';
import { PushParams } from './interfaces';

export default async function push({
contractAliases,
network,
deployDependencies,
deployProxyAdmin,
Expand All @@ -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();
Expand Down
2 changes: 1 addition & 1 deletion packages/cli/src/telemetry/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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<void> {
async report(commandName: string, params: Record<string, unknown>, interactive: boolean): Promise<void> {
const telemetryOptions = await checkOptIn(interactive);
if (telemetryOptions === undefined || !telemetryOptions.optIn) return;

Expand Down
156 changes: 153 additions & 3 deletions packages/cli/test/scripts/push.test.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,9 @@
'use strict';
require('../setup');

const zosLib = require('@openzeppelin/upgrades'); // eslint-disable-line @typescript-eslint/no-var-requires
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';

Expand Down Expand Up @@ -169,7 +171,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,
Expand All @@ -179,7 +181,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,
Expand All @@ -195,6 +197,152 @@ 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;
});

describe('when a NetworkFile is empty', function() {
beforeEach('purging NetworkFile', function() {
this.networkFile.data.contracts = {};
this.networkFile.data.proxies = {};
});

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');
});

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');
});
});

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() {
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() {
Expand Down Expand Up @@ -531,6 +679,7 @@ describe('push script', function() {
shouldDeployApp();
shouldDeployProvider();
shouldDeployContracts();
shouldDeployOnlySpecifiedContracts();
shouldRegisterContractsInDirectory();
shouldRedeployContracts();
shouldValidateContracts();
Expand Down Expand Up @@ -687,6 +836,7 @@ describe('push script', function() {
});

shouldDeployContracts();
shouldDeployOnlySpecifiedContracts();
shouldValidateContracts();
shouldRedeployContracts();
shouldDeleteContracts({ unregisterFromDirectory: false });
Expand Down