Skip to content

feat: introduce function support for deploy lifecycle hooks #1101

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Nov 20, 2018
Merged

feat: introduce function support for deploy lifecycle hooks #1101

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
4 changes: 2 additions & 2 deletions src/lib/core/config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -330,14 +330,14 @@ Config.prototype.loadContractsConfigFile = function() {
});
}

if (onDeploy && onDeploy.length) {
if (Array.isArray(onDeploy)) {
Copy link
Collaborator

@jrainville jrainville Nov 19, 2018

Choose a reason for hiding this comment

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

Does it have issues with empty arrays?

Copy link
Contributor Author

@0x-r4bbit 0x-r4bbit Nov 19, 2018

Choose a reason for hiding this comment

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

That was actually the reason why I didn't use isArray in the first place, however, if it happens to be an empty array, it's fine as the following code path will basically be a noop.

newContractsConfig.contracts[contractName].onDeploy = onDeploy.map(replaceZeroAddressShorthand);
}
});

const afterDeploy = newContractsConfig.contracts.afterDeploy;

if (afterDeploy && afterDeploy.length) {
if (Array.isArray(afterDeploy)) {
newContractsConfig.contracts.afterDeploy = afterDeploy.map(replaceZeroAddressShorthand);
}

Expand Down
12 changes: 7 additions & 5 deletions src/lib/modules/contracts_manager/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -407,10 +407,15 @@ class ContractsManager {
for (className in self.contracts) {
contract = self.contracts[className];

self.contractDependencies[className] = self.contractDependencies[className] || [];

if (Array.isArray(contract.deps)) {
self.contractDependencies[className] = self.contractDependencies[className].concat(contract.deps);
}

// look in code for dependencies
let libMatches = (contract.code.match(/:(.*?)(?=_)/g) || []);
for (let match of libMatches) {
self.contractDependencies[className] = self.contractDependencies[className] || [];
self.contractDependencies[className].push(match.substr(1));
}

Expand All @@ -427,14 +432,12 @@ class ContractsManager {
for (let j = 0; j < ref.length; j++) {
let arg = ref[j];
if (arg[0] === "$" && !arg.startsWith('$accounts')) {
self.contractDependencies[className] = self.contractDependencies[className] || [];
self.contractDependencies[className].push(arg.substr(1));
self.checkDependency(className, arg.substr(1));
}
if (Array.isArray(arg)) {
for (let sub_arg of arg) {
if (sub_arg[0] === "$" && !sub_arg.startsWith('$accounts')) {
self.contractDependencies[className] = self.contractDependencies[className] || [];
self.contractDependencies[className].push(sub_arg.substr(1));
self.checkDependency(className, sub_arg.substr(1));
}
Expand All @@ -443,7 +446,7 @@ class ContractsManager {
}

// look in onDeploy for dependencies
if (contract.onDeploy !== [] && contract.onDeploy !== undefined) {
if (Array.isArray(contract.onDeploy)) {
let regex = /\$\w+/g;
contract.onDeploy.map((cmd) => {
if (cmd.indexOf('$accounts') > -1) {
Expand All @@ -454,7 +457,6 @@ class ContractsManager {
// Contract self-referencing. In onDeploy, it should be available
return;
}
self.contractDependencies[className] = self.contractDependencies[className] || [];
self.contractDependencies[className].push(match.substr(1));
});
});
Expand Down
171 changes: 128 additions & 43 deletions src/lib/modules/specialconfigs/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,23 +82,33 @@ class SpecialConfigs {
registerAfterDeployAction() {
const self = this;

this.embark.registerActionForEvent("contracts:deploy:afterAll", (cb) => {
let afterDeployCmds = self.config.contractsConfig.afterDeploy || [];
async.mapLimit(afterDeployCmds, 1, (cmd, nextMapCb) => {
async.waterfall([
function replaceWithAddresses(next) {
self.replaceWithAddresses(cmd, next);
},
self.replaceWithENSAddress.bind(self)
], nextMapCb);
}, (err, onDeployCode) => {
if (err) {
self.logger.trace(err);
return cb(new Error("error running afterDeploy"));
this.embark.registerActionForEvent("contracts:deploy:afterAll", async (cb) => {
if (typeof self.config.contractsConfig.afterDeploy === 'function') {
try {
const dependencies = await this.getAfterDeployLifecycleHookDependencies();
await self.config.contractsConfig.afterDeploy(dependencies);
cb();
} catch (err) {
return cb(new Error(`Error registering afterDeploy lifecycle hook: ${err.message}`));
}
} else {
let afterDeployCmds = self.config.contractsConfig.afterDeploy || [];
async.mapLimit(afterDeployCmds, 1, (cmd, nextMapCb) => {
async.waterfall([
function replaceWithAddresses(next) {
self.replaceWithAddresses(cmd, next);
},
self.replaceWithENSAddress.bind(self)
], nextMapCb);
}, (err, onDeployCode) => {
if (err) {
self.logger.trace(err);
return cb(new Error("error running afterDeploy"));
}

self.runOnDeployCode(onDeployCode, cb);
});
self.runOnDeployCode(onDeployCode, cb);
});
}
});
}

Expand All @@ -122,7 +132,7 @@ class SpecialConfigs {
registerOnDeployAction() {
const self = this;

this.embark.registerActionForEvent("deploy:contract:deployed", (params, cb) => {
this.embark.registerActionForEvent("deploy:contract:deployed", async (params, cb) => {
let contract = params.contract;

if (!contract.onDeploy || contract.deploy === false) {
Expand All @@ -133,54 +143,129 @@ class SpecialConfigs {
self.logger.info(__('executing onDeploy commands'));
}

let onDeployCmds = contract.onDeploy;
async.mapLimit(onDeployCmds, 1, (cmd, nextMapCb) => {
async.waterfall([
function replaceWithAddresses(next) {
self.replaceWithAddresses(cmd, next);
},
self.replaceWithENSAddress.bind(self)
], (err, code) => {
if (typeof contract.onDeploy === 'function') {
try {
const dependencies = await this.getOnDeployLifecycleHookDependencies(contract);
await contract.onDeploy(dependencies);
cb();
} catch (err) {
return cb(new Error(`Error when registering onDeploy hook for ${contract.name}: ${err.message}`));
}
} else {
let onDeployCmds = contract.onDeploy;
async.mapLimit(onDeployCmds, 1, (cmd, nextMapCb) => {
async.waterfall([
function replaceWithAddresses(next) {
self.replaceWithAddresses(cmd, next);
},
self.replaceWithENSAddress.bind(self)
], (err, code) => {
if (err) {
self.logger.error(err.message || err);
return nextMapCb(); // Don't return error as we just skip the failing command
}
nextMapCb(null, code);
});
}, (err, onDeployCode) => {
if (err) {
self.logger.error(err.message || err);
return nextMapCb(); // Don't return error as we just skip the failing command
return cb(new Error("error running onDeploy for " + contract.className.cyan));
}
nextMapCb(null, code);
});
}, (err, onDeployCode) => {
if (err) {
return cb(new Error("error running onDeploy for " + contract.className.cyan));
}

self.runOnDeployCode(onDeployCode, cb, contract.silent);
});
self.runOnDeployCode(onDeployCode, cb, contract.silent);
});
}
});
}

registerDeployIfAction() {
const self = this;

self.embark.registerActionForEvent("deploy:contract:shouldDeploy", (params, cb) => {
self.embark.registerActionForEvent("deploy:contract:shouldDeploy", async (params, cb) => {
let cmd = params.contract.deployIf;
const contract = params.contract;
if (!cmd) {
return cb(params);
}

self.events.request('runcode:eval', cmd, (err, result) => {
if (typeof cmd === 'function') {
try {
const dependencies = await this.getOnDeployLifecycleHookDependencies(contract);
params.shouldDeploy = await contract.deployIf(dependencies);
cb(params);
} catch (err) {
return cb(new Error(`Error when registering deployIf hook for ${contract.name}: ${err.message}`));
}
} else {

self.events.request('runcode:eval', cmd, (err, result) => {
if (err) {
self.logger.error(params.contract.className + ' deployIf directive has an error; contract will not deploy');
self.logger.error(err.message || err);
params.shouldDeploy = false;
} else if (!result) {
self.logger.info(params.contract.className + ' deployIf directive returned false; contract will not deploy');
params.shouldDeploy = false;
}

cb(params);
});
}
});
}

getOnDeployLifecycleHookDependencies(contractConfig) {
Copy link
Contributor Author

@0x-r4bbit 0x-r4bbit Nov 19, 2018

Choose a reason for hiding this comment

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

Could be rewritten using async/await

Copy link
Contributor Author

@0x-r4bbit 0x-r4bbit Nov 19, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Actually, I've realised those aren't easily converted to async/await because our entire event bus is callback-based.

let dependencyNames = contractConfig.deps || [];
dependencyNames.push(contractConfig.className);
Copy link
Collaborator

@jrainville jrainville Nov 19, 2018

Choose a reason for hiding this comment

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

I think the same contract can be added multiple times to the list like this. I don't think it's a super big deal, since it's only references, but we never know.

Copy link
Contributor Author

@0x-r4bbit 0x-r4bbit Nov 19, 2018

Choose a reason for hiding this comment

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

Sure, we can dedupe dependencyNames before we enter the async.map()

Copy link
Contributor Author

@0x-r4bbit 0x-r4bbit Nov 20, 2018

Choose a reason for hiding this comment

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

deduped

dependencyNames = [...new Set(dependencyNames)];

return new Promise((resolve, reject) => {
async.map(dependencyNames, (contractName, next) => {
this.events.request('contracts:contract', contractName, (contractRecipe) => {
if (!contractRecipe) {
next(new Error(`ReferredContractDoesNotExist: ${contractName}`));
}
this.events.request('blockchain:contract:create', {
abi: contractRecipe.abiDefinition,
address: contractRecipe.deployedAddress
}, contractInstance => {
next(null, { className: contractRecipe.className, instance: contractInstance });
});
});
}, (err, contractInstances) => {
if (err) {
self.logger.error(params.contract.className + ' deployIf directive has an error; contract will not deploy');
self.logger.error(err.message || err);
params.shouldDeploy = false;
} else if (!result) {
self.logger.info(params.contract.className + ' deployIf directive returned false; contract will not deploy');
params.shouldDeploy = false;
reject(err);
Copy link
Collaborator

@jrainville jrainville Nov 19, 2018

Choose a reason for hiding this comment

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

I think you don't have to return here, but I feel like it's safer to anyway.

Copy link
Contributor Author

@0x-r4bbit 0x-r4bbit Nov 19, 2018

Choose a reason for hiding this comment

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

Correct, no return statement needed for resolve/reject as this will return the promise immediately.

}
this.events.request('blockchain:get', web3 => resolve(this.assembleLifecycleHookDependencies(contractInstances, web3)));
});
});
}

cb(params);
getAfterDeployLifecycleHookDependencies() {
Copy link
Contributor Author

@0x-r4bbit 0x-r4bbit Nov 19, 2018

Choose a reason for hiding this comment

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

Could be rewritten using async/await

Copy link
Contributor Author

@0x-r4bbit 0x-r4bbit Nov 19, 2018

Choose a reason for hiding this comment

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

Same here. Turns out we can't really convert this to async/await as there's not promise based API used within this function.

Copy link
Contributor

@alaibe alaibe Nov 19, 2018

Choose a reason for hiding this comment

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

getOnDeployLifecycleHookDependencies and getAfterDeployLifecycleHookDependencies seems to share some logic, do you think it could be possible to extract the sharing logic into function?

Copy link
Contributor Author

@0x-r4bbit 0x-r4bbit Nov 19, 2018

Choose a reason for hiding this comment

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

Yes I was considering that too. Especially the part that puts together the dependencies ( think the rest is already pretty broken down into pieces and now really reusable).

But then I thought, it might be a little overkill to introduce a new function for:

          this.events.request('blockchain:get', web3 => {

            let dependencies = { contracts: {}, web3 };

            contractInstances.forEach(contractInstance => {
              dependencies.contracts[contractInstance.className] = contractInstance.instance;
            });
            resolve(dependencies);
          });

Anyways, will update it :)

Copy link
Collaborator

@jrainville jrainville Nov 19, 2018

Choose a reason for hiding this comment

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

To reduce the shared code and also not having to re-create the contract object every time there is a new onDeploy, I think we should have one of the contract modules have a list of the web3 contracts an retrieve them here with an event.

Every time a contract is deployed, it would create that contract object. That would be quite useful for a couple of our modules. I'm thinking about this one, the ENS one, maybe even tests.

Maybe one of our modules already does that, but I don't recall. Also, maybe there is a good reason we didn't do that before (maybe a memory issue)? What do you think?

Copy link
Contributor Author

@0x-r4bbit 0x-r4bbit Nov 20, 2018

Choose a reason for hiding this comment

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

This is a very good point you're raising here @jrainville

From the top of my head I'd say ContractsManager should be responsible for that. re: Memory issues: AFAIK node has a max memory limit of ~1.5 GB by default but can be easily extended if needed.

I guess that's something we'll have to experiment with.

I believe this would be a good candidate for a separate PR/effort though.

return new Promise((resolve, reject) => {
this.events.request('contracts:list', (err, contracts) => {
async.map(contracts, (contract, next) => {
this.events.request('blockchain:contract:create', {
abi: contract.abiDefinition,
address: contract.deployedAddress
}, contractInstance => {
next(null, { className: contract.className, instance: contractInstance });
});
}, (err, contractInstances) => {
if (err) {
reject(err);
}
this.events.request('blockchain:get', web3 => resolve(this.assembleLifecycleHookDependencies(contractInstances, web3)));
});
});
});
}

assembleLifecycleHookDependencies(contractInstances, web3) {
return contractInstances.reduce((dependencies, contractInstance) => {
dependencies.contracts[contractInstance.className] = contractInstance.instance;
return dependencies;
}, { contracts: {}, web3 });
}
Copy link
Contributor Author

@0x-r4bbit 0x-r4bbit Nov 19, 2018

Choose a reason for hiding this comment

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

@alaibe this is the handler that's shared in both get*Dependencies() functions.

Notice that that's the only part that could potentially be shared.

Copy link
Contributor

@alaibe alaibe Nov 19, 2018

Choose a reason for hiding this comment

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

Fair enough, can I suggest using reduce? That will be more declarative

}

module.exports = SpecialConfigs;