Skip to content

Commit

Permalink
Address PR feedback
Browse files Browse the repository at this point in the history 
- Added messages to asserts in tests
- Split up Contract#setAddress into Contract#setAddress and Contract#setGeneratedAddress
- Don't use console.log to write to STDOUT in merge_chainspec_accounts.js
  • Loading branch information
@hobofan
hobofan committed Jan 8, 2019
1 parent a24a718 commit bd9635e
Show file tree
Hide file tree
Showing 4 changed files with 122 additions and 44 deletions.
40 changes: 28 additions & 12 deletions tools/deployment_tool/contract.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ class Contract {
* @param {string} contractName Name of the contract to create. If this contract is
* linked into another contracted, the provided contractName has to
* match the linking placeholder.
* @param {string} contracBytecode Bytecode of the contract.
* @param {string} contractBytecode Bytecode of the contract.
* @param {Array<*>} [constructorABI] ABI of the contract constructor.
* @param {Array<*>} [constructorArgs] Arguments for the contract constructor.
*/
Expand All @@ -37,6 +37,7 @@ class Contract {
this.linkedBytecode = this.linkedBytecode.bind(this);
this.reference = this.reference.bind(this);
this.setAddress = this.setAddress.bind(this);
this.setGeneratedAddress = this.setGeneratedAddress.bind(this);
}

/**
Expand Down Expand Up @@ -80,26 +81,41 @@ class Contract {
}

/**
* Returns the contract's address as detemined by the provided AddressGenerator.
* Set the address of the contract to a fixed address.
*
* @param {Object|string} addressGeneratorOrAddress The AddressGenerator used for
* generating the addresses. Can alternatively be a fixed address.
* See {@link Contract#setGeneratedAddress} for setting an address for the contract
* via an AddressGenerator instead.
*
* @param {string} address The fixed address to use for this contract.
* @returns {string} The contract's address.
*/
setAddress(addressGeneratorOrAddress) {
setAddress(address) {
this._ensureStateOneOf([STATE_NEW, STATE_LINKED]);
// Return early if the address has previously been set.
// This allows for setting a fixed address for specific contracts.
if (this.address) {
return this.address;
if (typeof address !== 'string') {
throw new Error(`Invalid address provided for ${this.contractName}`);
}

if (typeof addressGeneratorOrAddress === 'string') {
this.address = addressGeneratorOrAddress;
this.address = address;
return this.address;
}

/**
* Set the address by generating an address with provided AddressGenerator,
* and return that address.
*
* See {@link Contract#setAddress} for setting a fixed address for the contract instead.
*
* @param {Object} addressGenerator The AddressGenerator used for generating the addresses.
* @returns {string} The contract's address.
*/
setGeneratedAddress(addressGenerator) {
this._ensureStateOneOf([STATE_NEW, STATE_LINKED]);
// Return early if the address has previously been set, so we don't overwrite it.
// This allows for setting a fixed address for specific contracts via `setAddress`.
if (this.address) {
return this.address;
}

const addressGenerator = addressGeneratorOrAddress;
if (!addressGenerator) {
throw new Error(`addressGenerator not provided when generating address for ${this.contractName}`);
}
Expand Down
4 changes: 2 additions & 2 deletions tools/deployment_tool/contract_registry.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ class ContractRegistry {
// prepare contracts by ordering and generating addresses
const { addressGenerator } = mergedOptions;
this._orderContracts();
this.contracts.forEach(contract => contract.setAddress(addressGenerator));
this.contracts.forEach(contract => contract.setGeneratedAddress(addressGenerator));
this.contracts.forEach(contract => contract.instantiate());

const genesisAccounts = {
Expand Down Expand Up @@ -95,7 +95,7 @@ class ContractRegistry {
const addressGenerator = new IncrementingNonceAddressGenerator(fromAddress, startingNonce);

this._orderContracts();
this.contracts.forEach(contract => contract.setAddress(addressGenerator));
this.contracts.forEach(contract => contract.setGeneratedAddress(addressGenerator));
this.contracts.forEach(contract => contract.instantiate());

const transactionObjects = [];
Expand Down
118 changes: 90 additions & 28 deletions tools/deployment_tool/test/deployment_tools.test.js
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
const chai = require('chai');
const assert = chai.assert;

const { assert } = chai;

const {
Contract,
Expand All @@ -23,8 +24,8 @@ describe('Contract', () => {
registry.addContract(EIP20Token);

const output = registry.toParityGenesisAccounts();
assert(Object.keys(output).includes('0x0000000000444440000000000000000000000100'));
assert.equal(Object.keys(output).length, 1);
assert.strictEqual(Object.keys(output).length, 1, 'Output should only contain a single address-contstructor pair');
assert.strictEqual(Object.keys(output)[0], '0x0000000000444440000000000000000000000100', 'Address in output is different to the explicitly set address');
});

it('can not set address after instantiation', () => {
Expand All @@ -34,7 +35,12 @@ describe('Contract', () => {
EIP20Token.setAddress('0x0000000000444440000000000000000000000100');
EIP20Token.instantiate();

assert.throws(() => EIP20Token.setAddress('0x0000000000444440000000000000000000000100'));
assert.throws(
() => EIP20Token.setAddress('0x0000000000444440000000000000000000000100'),
null,
null,
"Didn't throw an Error when trying to set an address after instantiation",
);
});
});

Expand All @@ -46,7 +52,11 @@ describe('Contract', () => {
{ rootDir: `${__dirname}/../../../` },
);

assert.equal(EIP20Token.contractName, 'EIP20Token');
assert.strictEqual(
EIP20Token.contractName,
'EIP20Token',
'Name of loaded contract is incorrect',
);
});
});
});
Expand All @@ -63,9 +73,11 @@ describe('ContractRegistry', () => {
registry.addContracts([MerklePatriciaProof, MessageBus]);

const output = registry.toParityGenesisAccounts();
assert.equal(Object.keys(output).length, 2);
assert.strictEqual(Object.keys(output).length, 2, 'Output should contain both link source and link target');
// check that link placeholders have been removed
Object.values(output).forEach(bytecode => assert.isNotOk(bytecode.constructor.includes('_')));
Object
.values(output)
.forEach(bytecode => assert.isNotOk(bytecode.constructor.includes('_'), 'Contract still contains linking placholder'));
});

it('orders contracts correctly (correct order provided)', () => {
Expand All @@ -78,8 +90,16 @@ describe('ContractRegistry', () => {
registry.addContracts([MerklePatriciaProof, MessageBus]);

const output = registry.toParityGenesisAccounts();
assert.equal(Object.keys(output)[0], MerklePatriciaProof.getAddress());
assert.equal(Object.keys(output)[1], MessageBus.getAddress());
assert.strictEqual(
Object.keys(output)[0],
MerklePatriciaProof.getAddress(),
'MerklePatriciaProof is not at first output position',
);
assert.strictEqual(
Object.keys(output)[1],
MessageBus.getAddress(),
'MessageBus is not at second output position',
);
});

it('orders contracts correctly (wrong order provided)', () => {
Expand All @@ -92,8 +112,16 @@ describe('ContractRegistry', () => {
registry.addContracts([MessageBus, MerklePatriciaProof]);

const output = registry.toParityGenesisAccounts();
assert.equal(Object.keys(output)[0], MerklePatriciaProof.getAddress());
assert.equal(Object.keys(output)[1], MessageBus.getAddress());
assert.strictEqual(
Object.keys(output)[0],
MerklePatriciaProof.getAddress(),
'MerklePatriciaProof is not at first output position',
);
assert.strictEqual(
Object.keys(output)[1],
MessageBus.getAddress(),
'MessageBus is not at second output position',
);
});

it('orders contracts correctly (address reference in constructor)', () => {
Expand All @@ -116,9 +144,20 @@ describe('ContractRegistry', () => {
registry.addContracts([MosaicCore, EIP20Token]);

const output = registry.toParityGenesisAccounts();
assert.equal(Object.keys(output)[0], EIP20Token.getAddress());
assert.equal(Object.keys(output)[1], MosaicCore.getAddress());
assert(Object.values(output)[1].constructor.includes(EIP20Token.getAddress().slice(2)));
assert.strictEqual(
Object.keys(output)[0],
EIP20Token.getAddress(),
'EIP20Token is not at first output position',
);
assert.strictEqual(
Object.keys(output)[1],
MosaicCore.getAddress(),
'MosaicCore is not at second output position',
);
assert(
Object.values(output)[1].constructor.includes(EIP20Token.getAddress().slice(2)),
'EIP20Token address is not part of MosaicCore constructor bytecode',
);
});
});

Expand All @@ -134,16 +173,19 @@ describe('ContractRegistry', () => {
registry.addContract(EIP20Token);

const output = registry.toLiveTransactionObjects('0xc02345a911471fd46c47c4d3c2e5c85f5ae93d13', 0);
assert.deepEqual(output[0], {
transactionObject: {
from: '0xc02345a911471fd46c47c4d3c2e5c85f5ae93d13',
data: EIP20Token.constructorData,
nonce: 0,
assert.deepEqual(
output[0],
{
transactionObject: {
from: '0xc02345a911471fd46c47c4d3c2e5c85f5ae93d13',
data: EIP20Token.constructorData,
nonce: 0,
},

address: '0x5eceb671884153e2e312f8c5ae8e38fdc473c18d',
contractName: 'EIP20Token',
},

address: '0x5eceb671884153e2e312f8c5ae8e38fdc473c18d',
contractName: 'EIP20Token',
});
);
});
});
});
Expand All @@ -153,7 +195,11 @@ describe('IncrementingNonceAddressGenerator', () => {
const generator = new IncrementingNonceAddressGenerator('0xc02345a911471fd46c47c4d3c2e5c85f5ae93d13', 0);
const address = generator.generateAddress();

assert.equal(address, '0x5eceb671884153e2e312f8c5ae8e38fdc473c18d');
assert.strictEqual(
address,
'0x5eceb671884153e2e312f8c5ae8e38fdc473c18d',
'Generated address is not the address expected for account 0xc02345a911471fd46c47c4d3c2e5c85f5ae93d13 at nonce 0',
);
});

it('generates multiple addresses correctly', () => {
Expand All @@ -163,9 +209,25 @@ describe('IncrementingNonceAddressGenerator', () => {
const address3 = generator.generateAddress();
const address4 = generator.generateAddress();

assert.equal(address1, '0x5eceb671884153e2e312f8c5ae8e38fdc473c18d');
assert.equal(address2, '0x20e8a23a99c26334aed05051d6e5c6cdf50d63f6');
assert.equal(address3, '0xf0cd575450fc03b90eead03d65e79741a19665e4');
assert.equal(address4, '0x10ef71366ad76d6bddddc66677c38e137aa564db');
assert.strictEqual(
address1,
'0x5eceb671884153e2e312f8c5ae8e38fdc473c18d',
'Generated address is not the address expected for account at nonce 0',
);
assert.strictEqual(
address2,
'0x20e8a23a99c26334aed05051d6e5c6cdf50d63f6',
'Generated address is not the address expected for account at nonce 1',
);
assert.strictEqual(
address3,
'0xf0cd575450fc03b90eead03d65e79741a19665e4',
'Generated address is not the address expected for account at nonce 2',
);
assert.strictEqual(
address4,
'0x10ef71366ad76d6bddddc66677c38e137aa564db',
'Generated address is not the address expected for account at nonce 3',
);
});
});
4 changes: 2 additions & 2 deletions tools/merge_chainspec_accounts.js
View file
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
#! /usr/bin/env node
// Called like this:
// node merge_chainspec_accounts.js chainspec.json accounts.json
const fs = require("fs");
const fs = require('fs');

const chainspecPath = process.argv[2];
const accountsPath = process.argv[3];
Expand All @@ -17,4 +17,4 @@ Object.entries(accounts).forEach(([address, obj]) => {
});

// Pretty print as JSON
console.log(JSON.stringify(chainspec, null, 4));
process.stdout.write(JSON.stringify(chainspec, null, 4));

0 comments on commit bd9635e

Please sign in to comment.