feat(@embark/plugins): introduce API to register test contract factories #1131

0x-r4bbit · 2018-11-26T12:59:31Z

This commit introduces a new API registerTestContractFactory() which can be
used to register a factory function for the creation of web3 contract instances
in the testing environment.

Example:

// some.plugin.js

module.exports = function (embark) {
  embark.registerTestContractFactory((contractRecipe, web3) => {
    // do something with web3 and contractRecipe and return contract instance here
  });
};

Closes #1066

0x-r4bbit · 2018-11-26T13:06:55Z

Another thing to be aware of:

I realized that the plugin doesn't get executed when written in fat arrow syntax. E.g.:

module.exports =  embark => {
  embark.registerTestContractFactory((contractRecipe, web3) => {
    // do something with web3 and contractRecipe and return contract instance here
  });
};

0x-r4bbit · 2018-11-26T13:04:03Z

src/lib/core/plugin.js

@@ -120,6 +121,11 @@ Plugin.prototype.registerContractsGeneration = function(cb) {
  this.addPluginType('contractGeneration');
 };

+Plugin.prototype.registerTestContractFactory = function(cb) {
+  this.testContractFactory = cb;


So there's something to keep in mind and probably needs further discussion:

The current plugin API doesn't work very well for use cases like registering a contract factory for testing scenario. The reason for that is that there could be multiple plugins and use this API. Then the question is: which one is going to win? There can be only one factory function at a time.

The way it is done right now is that we always only take the last one that is registered (which could confuse people).

Another thing that wasn't clear to me, if this factory should affect both, embark test and embark console.

I could imagine we probably don't want to blindly use the same factory for both scenarios, which is why in a follow up commit, there'd be a registerConsoleContractFactory() API. The same problems exist there as well though.

Over all this API addition feels like it needs more thought. Probably better done in an iteration where we rethink our entire Plugin API where needed.

Yeah, the plugin API is really made so that the last one gets applied. It works well in the case of internal vs plugin, as the plugin will always win, which makes sense.

However, like you pointed, if there are multiple plugins that do the same thing, then it gets confusing.

We never had that issue before because the number of plugins is still small and no one tried to use two plugins that do the same thing at the same time (eg: two plugins to compile solidity files).

One thing to check is if the order in which we put the plugins in embark.json affects the order of application of the plugin, if so, it can be a workaround if we document it.

0x-r4bbit · 2018-11-26T13:04:41Z

src/lib/modules/tests/test.js

@@ -268,7 +269,9 @@ class Test {
              self.contracts[contract.className] = {};
            }

-            const newContract = Test.getWeb3Contract(contract, web3);
+            const testContractFactoryPlugin = self.plugins.getPluginsFor('testContractFactory').slice(-1)[0];


As mentioned above, we're always taking the last one that has been registered as there can be only one. Hacky feelings coming up here when reading this code. =/

If there's more than one, could we chain them together, e.g. the return value of the first becomes input to the second, and so on (or reverse order)?

I actually believe that could break many scenarios.

This factory function allows you to do whatever you want. So if we pipe the return value in to other functions, they can never know what exactly they'll be getting.

I'm also not sure whether there would be a reasonable scenario to have multiple plugins that would alter this behaviour. I'm pretty close to saying that this should be a singleton-last-one-wins setting..

In that case, perhaps in a later refactoring we should make it an error to supply more than one.

jrainville

I would make this plugin API generic (console and test and whetever)
The user himself can control the context in which he wants to execute the plugin using the context api.

This API was crated a while back, but it never was super used I think, but this here is agreat use case for it. Here's the docs for it: https://embark.status.im/docs/installing_plugins.html

Basicall,y it lets the user put an array of when the plguin should be executed. Something like

context: ['test']

Would enable it only in the tests

jrainville · 2018-11-26T15:39:45Z

src/lib/core/plugin.js

@@ -120,6 +121,11 @@ Plugin.prototype.registerContractsGeneration = function(cb) {
  this.addPluginType('contractGeneration');
 };

+Plugin.prototype.registerTestContractFactory = function(cb) {
+  this.testContractFactory = cb;


Yeah, the plugin API is really made so that the last one gets applied. It works well in the case of internal vs plugin, as the plugin will always win, which makes sense.

However, like you pointed, if there are multiple plugins that do the same thing, then it gets confusing.

We never had that issue before because the number of plugins is still small and no one tried to use two plugins that do the same thing at the same time (eg: two plugins to compile solidity files).

One thing to check is if the order in which we put the plugins in embark.json affects the order of application of the plugin, if so, it can be a workaround if we document it.

0x-r4bbit · 2018-11-27T09:46:00Z

@jrainville I've updated the PR and now use the same function for both scenarios, console and testing.

This still comes with a couple of characteristics we should document and make ppl aware of (I still think two different APIs would be better here). Those characteristics are:

This factory function is used for contract instance creation within tests.
A contractRecipe and a web3 instance is accessible within the factory.
It's also used for EmbarkJS.Blockchain.Contract within embark console.
In that scenario, this function has to be a constructor function and therefore
cannot be registered using fat arrow functions. Also, we don't control what
gets passed to that function in that scenario.

So not only is the usage of that API different, depending on where it's used (console and testing), but it also behaves entirely different.

I don't have much better ideas now apart from better introducing two APIs for this, but overall this feels rather sloppy to me. Hope we can iron this out with an API redesign in the future..

jrainville · 2018-11-27T14:19:28Z

I just checked what the console actually uses and it's not using EmbarkJS.BLockchain.Contract, but a template that get's evald.
Here's the line where we eval: https://github.com/embark-framework/embark/blob/master/src/lib/modules/deployment/contract_deployer.js#L198
Here is the template: https://github.com/embark-framework/embark/blob/master/src/lib/modules/code_generator/code_templates/vanilla-contract.js.ejs

So knowing that, we can either:

Reuse the other plugin API that was mentionned at first (registerContractsGeneration) to be used for the console and keep the one you created here for tests only.
Use the one you created here for the console too, by eval'ing the contract object in the global VM, though I have no idea if it would work.

There is probably a new solution, but I agree that things aren't super clean with either solutions.

0x-r4bbit · 2018-11-28T11:57:12Z

I just checked what the console actually uses and it's not using EmbarkJS.BLockchain.Contract, but a template that get's evald.

@jrainville we might be talking about two different consoles here, but I've tested it with EmbarkJS.Blockchain.Contract() within embark console and it did work as expected.

0x-r4bbit · 2018-11-28T12:01:29Z

So just to clarify, here's what I understood when we talked "making it work inside embark console as well":

User starts embark console
User runs EmbarkJS.Blockchain.Contract(), which returns the instance as expected

I have to admit that I was wondering how that use case is feasible, considering that the function expects parameters that one has to pass manually in that case. But maybe that's exactly where we're talking about two different things...

jrainville · 2018-11-28T13:58:52Z

@PascalPrecht For the console case, what we want is the global contracts available in the console to be generated using the new generator.
eg: in the console, do > SimpleStorage.methods.get()

0x-r4bbit · 2018-11-28T14:24:44Z

Okay, I clearly misunderstood the desired outcome of this then. Will update the PR.

0x-r4bbit · 2018-11-29T10:58:26Z

Quick update here @jrainville: after some further digging I found out that templates created using registerContractsGeneration() is already being picked up by embark console.

So functionality-wise, we already have that. The problem however is that, whatever is generated in global scope is undefined within the console. I then found out that the code that Embark generates by default is actually not the one you've linked, but this one: https://github.com/embark-framework/embark/blob/master/src/lib/modules/code_generator/code_templates/embarkjs-contract.js.ejs

The crucial difference here seems to be that this template attaches the contract instances to __mainContext, which I assume is the VM context in which the code gets executed.

__mainContext.<%- className %> = ...

If you write a plugin that looks like this:

  embark.registerContractsGeneration(function (options) {
    const contractsGenerations = []
    options.contracts.forEach(contract => {
      contractsGenerations.push(`${contract.className} = function () { return 'Hello world'; }`);
    })
    return contractsGenerations.join('\n');
  });

The code does end up in the generated ABI, but it's not attached to __mainContext.

0x-r4bbit · 2018-11-29T10:59:55Z

Actually, turns out that even putting it on __mainContext doesn't do the trick 🤷‍♂️

I changed

contractsGenerations.push(`${contract.className} = function () { return 'Hello world'; }`);

To

contractsGenerations.push(`__mainContext.${contract.className} = function () { return 'Hello world'; }`);

But executing that generated functions still causes an undefined error... need to dig further here..

0x-r4bbit · 2018-11-29T11:03:59Z

Ah, so it turns out there's a little bit more happening by default:

__mainContext.__loadManagerInstance.execWhenReady(function() {
  __mainContext.SimpleStorage = new EmbarkJS.Blockchain.Contract({abi: [{"constant":true,"inputs":[],"name":"storedData","outputs":[{"name":"","type":"uint256"}],"payable":false,"stateMutability":"view","type":"function","signature":"0x2a1afcd9"},{"constant":false,"inputs":[{"name":"x","type":"uint256"}],"name":"set","outputs":[],"payable":false,"stateMutability":"nonpayable","type":"function","signature":"0x60fe47b1"},{"constant":true,"inputs":[],"name":"get","outputs":[{"name":"retVal","type":"uint256"}],"payable":false,"stateMutability":"view","type":"function","signature":"0x6d4ce63c"},{"inputs":[{"name":"initialValue","type":"uint256"}],"payable":false,"stateMutability":"nonpayable","type":"constructor"}], address: '0xCedE0186225ff7e5D455725947Db919E24AA269c', code: '608060405234801561001057600080fd5b506040516020806101358339810180604052602081101561003057600080fd5b505160005560f2806100436000396000f3fe60806040526004361060525763ffffffff7c01000000000000000000000000000000000000000000000000000000006000350416632a1afcd98114605757806360fe47b114607b5780636d4ce63c1460a3575b600080fd5b348015606257600080fd5b50606960b5565b60408051918252519081900360200190f35b348015608657600080fd5b5060a160048036036020811015609b57600080fd5b503560bb565b005b34801560ae57600080fd5b50606960c0565b60005481565b600055565b6000549056fea165627a7a7230582053e5843f1eaaf67dbd6af43560db1c6fe5801a3209ffb56fdb8562655850bca00029', gasEstimates: {"creation":{"codeDepositCost":"48400","executionCost":"20172","totalCost":"68572"},"external":{"get()":"428","set(uint256)":"20197","storedData()":"384"}}});

});

I assume that __mainContext.__loadManagerInstance.execWhenReady(function() { ensures that the following code gets executed at a certain event. We obviously don't have that in the plugin APIs..

0x-r4bbit · 2018-11-29T11:41:05Z

Scratch that too. Generating the same code within a plugin still doesn't do it. Digging further..

0x-r4bbit · 2018-11-29T17:35:45Z

Quick update here for transparency:

Turns out code generated via registerContractsGeneration() is completely ignored by Embark at the moment. Everything that's available inside dapps is generated from templates.
We'll introduce a new API that'll be honored by Embark for code that'll be executed within the VM for embark console.
We can probably use the same for code generation within the dapp too.

I'm on it and hopefully have this done tomorrow.

0x-r4bbit

Alright this is updated now and I think needs another iteration for feedback and review.

Here's what currently going on here:

we introduce two APIs: registerTestContractFactory() and registerCustomContractGenerator()
Two APIs because Embark is doing two different things for different scenarios. Contract instances using test factory are created through execution of the factory. Contract instances created through generator are eval'd and made available inside embark console
None of these APIs actually affect how contract instances are created inside the dapp. This is handled with yet another routine in our pipeline, where JS code is generated and written to disk.
It's also important to keep in mind that, when using deployment hooks with the string API, contract instances created for those will be the ones generated from the custom generator as well, however, when function APIs are used, those will be different objects controlled by Embark.

Overall I have to say that I'm rather unhappy with how these APIs behave at the moment. It probably needs a bit more thought to figure out how this can be streamlined and made more predictable. At the time being, I'd almost vote for not adding this feature here to avoid introducing a lot of confusion.

Happy to discuss this with the team.

0x-r4bbit · 2018-11-30T11:35:28Z

src/lib/modules/deployment/contract_deployer.js

-      self.events.request('runcode:eval', contractCode, () => {}, true);
-      return callback();
+    self.events.request('code-generator:contract:custom', contract, (customContractCode) => {
+      if (!customContractCode) {


Notice that we first check if there's a custom contract code generator registered that returns dedicated code. We fallback to the vanilla contract creation in case non had been registered.

Also notice that this gets executed for all contracts, including the ones that Embark comes with (e.g. ENSRegistry etc).

We might want to check for built-in vs owned contracts and only do this for the ones owned by the developer.

Thoughts?

jrainville · 2018-11-30T13:44:04Z

Welp, I think my review didn't publish for some obscure reason. Here is is again:

I think the API should take care of calling a fallback. Example: we calling the custom contracts API, the API should get the vanilla one if there is no custom.

In an ideal world, we use the plugin API too so that the vanilla contract comes from us through the plugin API, so that way the fallback is automatically handled, but we'll need to refactor the code generator first.

0x-r4bbit · 2018-12-03T11:24:16Z

I think the API should take care of calling a fallback. Example: we calling the custom contracts API, the API should get the vanilla one if there is no custom.

@jrainville are you referring to code-generator:contract:custom here? As in, you want code-generator:contract:custom to return the vanilla contract in case no custom generator was registered?

In an ideal world, we use the plugin API too so that the vanilla contract comes from us through the plugin API, so that way the fallback is automatically handled, but we'll need to refactor the code generator first.

I read this part a couple of times now but it doesn't really make sense to me, sorry. Would you mind elaborating on what you mean by "in an ideal world we use the plugin API too" ?

jrainville · 2018-12-03T13:57:59Z

are you referring to code-generator:contract:custom

Yes

Would you mind elaborating on what you mean by "in an ideal world we use the plugin API too" ?

Sorry, it is indeed unclear. What I mean is that we should have used a plugin ourselves when we did the vanilla contract generation.
That way, Embark registers its generation, but if a plugin does too, the plugin overrides it, so we don't have to handle fallback ourselves.

0x-r4bbit · 2018-12-04T12:28:34Z

src/lib/modules/deployment/contract_deployer.js

-          // just need to figure out the gasLimit coupling issue
-          self.events.request('code-generator:contract:vanilla', contract, contract._gasLimit || false, (contractCode) => {
+
+          self.events.request('code-generator:contract:custom', contract, (contractCode) => {


Made the change as requested. Notice that we now always ask for custom generated code and then fallback to vanilla generated code behind the scenes. I personally don't like it so much because it's less clear what's going on, e.g. I'll have to dig in the code generator to find out that :custom actually returns code from the vanilla generator.

Anyways, something we might want to reconsider in the future.

This commit introduces two new plugin APIs `registerTestContractFactory()` and `registerCustomContractGenerator()`, which can be used to register a factory function for the creation of web3 contract instances within tests, and custom code generation for `embark console` respectively. Example: ``` // some.plugin.js module.exports = function (embark) { embark.registerTestContractFactory(function (contractRecipe, web3) { // do something with web3 and contractRecipe and return contract instance here }); }; ``` **Notice that**: - This factory function is used for contract instance creation within tests. A `contractRecipe` and a `web3` instance is accessible within the factory. Example: ``` // some.plugin.js module.exports = function (embark) { embark.registerCustomContractGenerator(function (contractRecipe) { // returns code string that will be eval'ed }); }; ``` **Notice that**: - Once registered, this generator will be used for **all** contract instances that will be created for `embark console`, including built-in once like ENSRegistry. - While this does affect contract creation in client-side code, it doesn't actually affect the instances created for deployment hooks **if** deployment hooks are written as functions. Closes #1066 Always use custom generator and fallback to vanilla

0x-r4bbit commented Nov 26, 2018

View reviewed changes

jrainville reviewed Nov 26, 2018

View reviewed changes

0x-r4bbit force-pushed the feat/contract-factory-api branch from 84a8c3d to b9b34db Compare November 27, 2018 09:42

0x-r4bbit force-pushed the feat/contract-factory-api branch from b9b34db to 635c5cf Compare November 27, 2018 10:30

0x-r4bbit force-pushed the feat/contract-factory-api branch 2 times, most recently from 782c599 to 4aa2451 Compare November 30, 2018 11:37

0x-r4bbit commented Nov 30, 2018

View reviewed changes

0x-r4bbit force-pushed the feat/contract-factory-api branch 2 times, most recently from d1ca610 to 05218c3 Compare December 4, 2018 12:26

0x-r4bbit commented Dec 4, 2018

View reviewed changes

jrainville approved these changes Dec 4, 2018

View reviewed changes

0x-r4bbit added 2 commits December 5, 2018 12:12

chore(@embark/code-generator): remove unused code

2581bbb

0x-r4bbit force-pushed the feat/contract-factory-api branch from 05218c3 to b81361d Compare December 5, 2018 11:14

0x-r4bbit merged commit 90aac83 into master Dec 5, 2018

0x-r4bbit deleted the feat/contract-factory-api branch December 5, 2018 11:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(@embark/plugins): introduce API to register test contract factories #1131

feat(@embark/plugins): introduce API to register test contract factories #1131

0x-r4bbit commented Nov 26, 2018

0x-r4bbit commented Nov 26, 2018

0x-r4bbit Nov 26, 2018 •

edited

Loading

jrainville Nov 26, 2018

0x-r4bbit Nov 26, 2018

michaelsbradleyjr Nov 26, 2018

0x-r4bbit Nov 26, 2018

michaelsbradleyjr Nov 26, 2018

jrainville left a comment

jrainville Nov 26, 2018

0x-r4bbit commented Nov 27, 2018

jrainville commented Nov 27, 2018

0x-r4bbit commented Nov 28, 2018

0x-r4bbit commented Nov 28, 2018

jrainville commented Nov 28, 2018

0x-r4bbit commented Nov 28, 2018

0x-r4bbit commented Nov 29, 2018

0x-r4bbit commented Nov 29, 2018

0x-r4bbit commented Nov 29, 2018

0x-r4bbit commented Nov 29, 2018

0x-r4bbit commented Nov 29, 2018

0x-r4bbit left a comment

0x-r4bbit Nov 30, 2018

0x-r4bbit Nov 30, 2018

jrainville commented Nov 30, 2018

0x-r4bbit commented Dec 3, 2018 •

edited

Loading

jrainville commented Dec 3, 2018

0x-r4bbit Dec 4, 2018

feat(@embark/plugins): introduce API to register test contract factories #1131

feat(@embark/plugins): introduce API to register test contract factories #1131

Conversation

0x-r4bbit commented Nov 26, 2018

0x-r4bbit commented Nov 26, 2018

0x-r4bbit Nov 26, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jrainville left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0x-r4bbit commented Nov 27, 2018

jrainville commented Nov 27, 2018

0x-r4bbit commented Nov 28, 2018

0x-r4bbit commented Nov 28, 2018

jrainville commented Nov 28, 2018

0x-r4bbit commented Nov 28, 2018

0x-r4bbit commented Nov 29, 2018

0x-r4bbit commented Nov 29, 2018

0x-r4bbit commented Nov 29, 2018

0x-r4bbit commented Nov 29, 2018

0x-r4bbit commented Nov 29, 2018

0x-r4bbit left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jrainville commented Nov 30, 2018

0x-r4bbit commented Dec 3, 2018 • edited Loading

jrainville commented Dec 3, 2018

Choose a reason for hiding this comment

0x-r4bbit Nov 26, 2018 •

edited

Loading

0x-r4bbit commented Dec 3, 2018 •

edited

Loading