feat: introduce function support for deploy lifecycle hooks #1101

0x-r4bbit · 2018-11-19T07:08:35Z

Prior to this commits deployment lifecycle hooks had been defined as Array due
to historical reasons (contracts.js) used to be a json file back in the days.

deployIf, onDeploy and afterDeploy can now be defined as (async)
function and have access to several dependencies such as contract instances and web3.
However, in order to have needed dependencies registered in the dependencies object,
all lifecycle hook dependencies need to be listed in the deps property
as shown below.

Also note that this is NOT a breaking change. Existing deployment lifecycle
hooks written in Array style still work.

All three lifecycle hooks can now be defined as (async) functions and get an dependency
object with a shape like this:

interface DeploymentLifecycleHookDependencies {
  contracts: Map<string, ContractInstance>;
  web3: Web3Instance
}

deployIf lifecycle hook has to return a promise (or be defined using async/await and return
a value) like this:

contracts: {
  MyToken: {...},
  SimpleStorage: {
    deps: ['MyToken'], // this is needed to make `MyToken` available within `dependencies`
    deployIf: async (dependencies) => {
      return dependencies.contracts.MyToken_address;
    }
  },
}

Vanilla promises (instead of async/await) can be used as well:

contracts: {
  MyToken: {...},
  SimpleStorage: {
    deps: ['MyToken'],
    deployIf: (dependencies) => {
      return new Promise(resolve => resolve(dependencies.contracts.MyToken_address);
    }
  },
}

onDeploy as well, returns either a promise or is used using async/await:

contracts: {
  SimpleStorage: {
    onDeploy: async (dependencies) => {
      const simpleStorage = dependencies.contracts.SimpleStorage;
      const value = await simpleStorage.methods.get().call();
      console.log(value);
    }
  },
}

afterDeploy has automatically access to all configured and deployed contracts of the dapp:

contracts: {
  SimpleStorage: {...},
  MyToken: {...},
  afterDeploy: (dependencies) => {
    console.log('Done!');
  }
}

Closes #1029

0x-r4bbit · 2018-11-19T07:09:04Z

Ah, this needs a rebase.

0x-r4bbit · 2018-11-19T07:13:24Z

src/lib/modules/specialconfigs/index.js

+      if (typeof self.config.contractsConfig.afterDeploy === 'function') {
+        this.getAfterDeployLifecycleHookDependencies().then(dependencies => {
+          (async () => {
+            await self.config.contractsConfig.afterDeploy(dependencies);


Notice that we always call lifecycle hook within async/await context.

This also means that lifecycle hooks have to either (optionally) return a Promise, or being written in async/await themselves, to notify Embark that a hook is done

@alaibe I looked into supporting both, promise support as well as callback support, unfortunately, I couldn't make both APIs work, without the user worrying about how their lifecycle hooks are being called.

I also believe, users shouldn't be needed to do both at the same time (calling callback and returning promise).

So I decided to have them use either Promise APIs or async/await. This seems natural to me as well (especially because this is a new API and I don't think there's a good reason to keep callback based APIs around)

0x-r4bbit · 2018-11-19T07:20:13Z

src/lib/modules/specialconfigs/index.js

+      if (typeof cmd === 'function') {
+        this.getOnDeployLifecycleHookDependencies(contract).then(dependencies => {
+          const callbackFn = (value) => {
+          };


This is a left over.

0x-r4bbit · 2018-11-19T07:21:30Z

src/lib/modules/specialconfigs/index.js

-          return cb(new Error("error running afterDeploy"));
-        }
+      if (typeof self.config.contractsConfig.afterDeploy === 'function') {
+        this.getAfterDeployLifecycleHookDependencies().then(dependencies => {


Might be worth putting the whole function in async/await as well, so that getAfterDeployLIfecyclehookDependencies() can be called with await, meaning also that afterDeploy() can be called with await without wrapping it in an IIFE.

0x-r4bbit · 2018-11-19T07:21:52Z

src/lib/modules/specialconfigs/index.js

+    });
+  }
+
+  getOnDeployLifecycleHookDependencies(contractConfig) {


Could be rewritten using async/await

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

0x-r4bbit · 2018-11-19T07:22:03Z

src/lib/modules/specialconfigs/index.js

    });
  }

+  getAfterDeployLifecycleHookDependencies() {


Could be rewritten using async/await

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

alaibe · 2018-11-19T09:06:12Z

src/lib/modules/contracts_manager/index.js


+          self.contractDependencies[className] = self.contractDependencies[className] || [];
+
+          if (contract.deps && contract.deps.length) {


Array.isArray(contract.deps) same technique as before, seems more elegant?

alaibe · 2018-11-19T09:10:53Z

src/lib/modules/specialconfigs/index.js

+          return cb(new Error(`Error when registering onDeploy hook for ${contract.name}: ${err.message}`));
+        });
+      } else {
+


cosmetic: no extra line here?

alaibe · 2018-11-19T09:11:23Z

src/lib/modules/specialconfigs/index.js

+      if (typeof cmd === 'function') {
+        this.getOnDeployLifecycleHookDependencies(contract).then(dependencies => {
+          const callbackFn = (value) => {
+          };


cosmetic: can be one line

This one is actually a left over that has been removed in the meantime :)

alaibe · 2018-11-19T09:13:17Z

src/lib/modules/specialconfigs/index.js

    });
  }

+  getAfterDeployLifecycleHookDependencies() {


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

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 :)

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?

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.

0x-r4bbit · 2018-11-19T10:46:52Z

src/lib/modules/specialconfigs/index.js

+      dependencies.contracts[contractInstance.className] = contractInstance.instance;
+    });
+    return dependencies;
+  }


@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.

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

0x-r4bbit · 2018-11-19T16:42:08Z

Documentation for this feature can be found here: embark-framework/embark-site#106

jrainville · 2018-11-19T16:50:51Z

src/lib/core/config.js

    }

-    if (onDeploy && onDeploy.length) {
+    if (Array.isArray(onDeploy)) {


Does it have issues with empty arrays?

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.

jrainville · 2018-11-19T16:52:00Z

src/lib/modules/contracts_manager/index.js

+          if (Array.isArray(contract.deps)) {
+            contract.deps.forEach(name => {
+              self.contractDependencies[className].push(name);
+            });


Can just do a concat self.contractDependencies[className] = self.contractDependencies[className].concat(contract.deps);

Good point!

jrainville · 2018-11-19T16:58:21Z

src/lib/modules/specialconfigs/index.js

+
+  getOnDeployLifecycleHookDependencies(contractConfig) {
+    let dependencyNames = contractConfig.deps || [];
+    dependencyNames.push(contractConfig.className);


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.

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

jrainville · 2018-11-19T16:59:05Z

src/lib/modules/specialconfigs/index.js

+      async.map(dependencyNames, (contractName, next) => {
+        this.events.request('contracts:contract', contractName, (contractRecipe) => {
+          if (!contractRecipe) {
+            next(new Error('ReferredContractDoesNotExist'));


Could you put the name of the referred contract too.

jrainville · 2018-11-19T17:00:20Z

src/lib/modules/specialconfigs/index.js

-        } else if (!result) {
-          self.logger.info(params.contract.className + ' deployIf directive returned false; contract will not deploy');
-          params.shouldDeploy = false;
+          reject(err);


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

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

jrainville · 2018-11-19T17:12:11Z

src/lib/modules/specialconfigs/index.js

    });
  }

+  getAfterDeployLifecycleHookDependencies() {


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?

Prior to this commits deployment lifecycle hooks had been defined as Array<string> due to historical reasons (contracts.js) used to be a json file back in the days. `deployIf`, `onDeploy` and `afterDeploy` can now be defined as (async) function and have access to several dependencies such as contract instances and web3. However, in order to have needed dependencies registered in the dependencies object, all lifecycle hook dependencies need to be listed in the `deps` property as shown below. Also note that this is NOT a breaking change. Existing deployment lifecycle hooks written in Array<string> style still work. All three lifecycle hooks can now be defined as (async) functions and get an dependency object with a shape like this: ``` interface DeploymentLifecycleHookDependencies { contracts: Map<string, ContractInstance>; web3: Web3Instance } ``` `deployIf` lifecycle hook has to return a promise (or be defined using async/await and return a value) like this: ``` contracts: { MyToken: {...}, SimpleStorage: { deps: ['MyToken'], // this is needed to make `MyToken` available within `dependencies` deployIf: async (dependencies) => { return dependencies.contracts.MyToken_address; } }, } ``` Vanilla promises (instead of async/await) can be used as well: ``` contracts: { MyToken: {...}, SimpleStorage: { deps: ['MyToken'], deployIf: (dependencies) => { return new Promise(resolve => resolve(dependencies.contracts.MyToken_address); } }, } ``` `onDeploy` as well, returns either a promise or is used using async/await: ``` contracts: { SimpleStorage: { onDeploy: async (dependencies) => { const simpleStorage = dependencies.contracts.SimpleStorage; const value = await simpleStorage.methods.get().call(); console.log(value); } }, } ``` `afterDeploy` has automatically access to all configured and deployed contracts of the dapp: ``` contracts: { SimpleStorage: {...}, MyToken: {...}, afterDeploy: (dependencies) => { console.log('Done!'); } } ``` Closes #1029

0x-r4bbit commented Nov 19, 2018

View reviewed changes

alaibe reviewed Nov 19, 2018

View reviewed changes

0x-r4bbit force-pushed the feat/deploy-hook-functions branch 2 times, most recently from 204b001 to c9e445e Compare November 19, 2018 10:40

0x-r4bbit commented Nov 19, 2018

View reviewed changes

0x-r4bbit force-pushed the feat/deploy-hook-functions branch 2 times, most recently from 7abf485 to 7452df5 Compare November 19, 2018 11:37

jrainville reviewed Nov 19, 2018

View reviewed changes

0x-r4bbit force-pushed the feat/deploy-hook-functions branch from 7452df5 to 77cc23b Compare November 20, 2018 08:48

jrainville approved these changes Nov 20, 2018

View reviewed changes

0x-r4bbit merged commit 8b68bec into master Nov 20, 2018

0x-r4bbit deleted the feat/deploy-hook-functions branch November 20, 2018 15:41


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

		if (contract.deps && contract.deps.length) {

feat: introduce function support for deploy lifecycle hooks #1101

feat: introduce function support for deploy lifecycle hooks #1101

Uh oh!

Conversation

0x-r4bbit commented Nov 19, 2018

Uh oh!

0x-r4bbit commented Nov 19, 2018

Uh oh!

0x-r4bbit Nov 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

0x-r4bbit Nov 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

0x-r4bbit commented Nov 19, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

0x-r4bbit Nov 19, 2018 •

edited

Loading

0x-r4bbit Nov 19, 2018 •

edited

Loading