EIP-1167 Clone Factory (#1162) #1361

jbogacz · 2018-09-28T05:30:46Z

🚀 Description

Introduced CloneFactory implementation consistent with EIP-1167. Based on example implementation and usage from optionality/clone-factory.

Fixes #1162.

[ +] 📘 I've reviewed the OpenZeppelin Contributor Guidelines
[ +] ✅ I've added tests where applicable to test my new functionality.
[ +] 📖 I've made sure that my contracts are well-documented.
[ +] 🎨 I've run the JS/Solidity linters and fixed any issues (npm run lint:fix).

jbogacz · 2018-09-28T06:00:03Z

Hey, let me know if do we need any unit tests which proofs it clone factory is cost efficient.

contracts/utils/CloneFactory.sol

nventuro

Hey there @jbogacz, thanks a lot! This is one of the EIPs that I've been looking at with interest, specially considering our work in https://zeppelinos.org/

I'm not entirely sure what the best approach towards testing should be, but I first have some minor questions we should get out of the way.

contracts/utils/CloneFactory.sol

contracts/mocks/ERC20CloneFactory.sol

nventuro · 2018-09-28T10:02:10Z

test/CloneFactory.test.js

+        clonedTwo = await factory.createNew(_clonedSupplyTwo, { from: owner });
+      });
+
+      it('should have correct state after init', async function () {


It'd be great if we could extract the ERC20 tests into a behavior, and use it here, though I'm not sure we have an issue to track that.

Hey @nventuro I'm afraid that I'm not getting this concept, could you elaborate further on ?

Sure! We have some behavior test files, in which how a contract behaves is described: for example, Ownable.behavior. This makes the tests reusable, and we can call them from the Ownable test itself, or from any other test suite.

It'd be great if we did this for the proxy tests (since we'd be running the same tests, making sure everything works properly), but we have not extracted them into a behavior yet - we can do that in a follow-up PR though, so it's nothing to worry about here :)

Oh I see, so I'll leave it for now.

contracts/utils/CloneFactory.sol

test/CloneFactory.test.js

nventuro

Thanks @jbogacz! We may want to revisit the contract names in the future (I'm not sure master and clone are good terms for what's going on, specially since nothing is ever cloned, and IMO it implies storage would also be copied), but they are fine for now, and we should focus on the tests first.

I think the ones in this PR are a bit too complex (with the initFactory function, etc.), when there's no real need for them to be, and you're using before blocks, making all tests interdependent. We usually exclusively use beforeEach, and setup what we want to test in each it block: take a look at the other test files to get an idea of our style, the one for BreakInvariantBounty comes to mind as an example you may want to get inspired from :)

nventuro · 2018-10-02T10:23:32Z

contracts/mocks/ERC20CloneFactory.sol

-  constructor(address _masterAddress) public {
-    masterAddress = _masterAddress;
+  constructor() public {
+    masterAddress = new ERC20Mock(msg.sender, 0);


Consider adding a comment here, stating that this constructor will only run in the master context, and not for all other created proxies.

nventuro · 2018-10-02T10:27:19Z

contracts/mocks/ERC20CloneFactory.sol

  }

  function createNew(uint256 amount) public returns (address) {
    address cloneAddress = createClone(masterAddress);
-    emit CloneCreated(cloneAddress);
    ERC20Mock(cloneAddress).mint(msg.sender, amount);


It's interesting that we're able to replicate the contents of the constructor with this call, but it's only available because this is a mock, were we need to expose that internal method: I wouldn't want someone to look at this and think this is good code they should copy.

I'm actually not sure how we should handle this, since we can't even use ERC20Mintable because there will be no minters. @frangio WDYT? Maybe test this with something that is constructor-less?

Hey @nventuro I'm not sure if I correctly understood your thoughts about replication the contents of constructor but I believe you meant that it's wrong example for usage. What you would say to create a simple contract only for purpose of test like Peter's did here ?

I've just hit on SimpleToken - WDYT @nventuro @frangio ? Could it be utilized as usage example?

jbogacz · 2018-10-02T16:57:27Z

Hey @nventuro sorry for that, I will double check other tests again and this time try to stick general style. As always I appreciate your constructive feedback :)

* Initialize master contract inside factory mock constructor * Invoke CloneCreated event inside CloneFactory * Add unit test to validate clone instance bytecode

jbogacz · 2018-10-04T06:55:06Z

Hey @nventuro, I've simplified CloneFactory.test.js to use SimpleToken as example. I've left one test case too. I'd like to state with you proper way for this test file. I also stayed with initFactory function but simplified it to be more readable. However I'd like to keep this function as a wrapper that helps to retrieve Truffle contract at clone address and keep this operation capsulated. Let me know your thoughts or ideas. Looking forward to you review! :)

jbogacz · 2018-10-13T05:59:25Z

Hi @nventuro, I believe you are busy but could you put any comments here ? As mentioned before I switched to use SimpleToken as example contract in unit test. What is your opinion and acceptable path to follow with this feature? Looking forward to hear your comments here ;)

nventuro

@jbogacz sorry for taking so long to review this! Last couple weeks were a bit crazy, with devcon and all, plus we're releasing OpenZeppelin 2.1 soon :)

I made a couple suggestions on the test file to make it simpler to read. We may want to review the naming of the contract, since both 'proxy' and 'clone' are used somewhat interchangeably and may convey different meanings. In particular, 'clone' implies storage is copied, IMO.

nventuro · 2018-12-10T18:53:18Z

test/CloneFactory.test.js

+
+    it('should create proxy contract with valid bytecode', async function () {
+      const proxy = await factory.createNew();
+      (await readContractCode(proxy.address)).should.be.equal(expectedBytcode());


We should also test that the event was emitted, using our expectEvent test helper.

nventuro · 2018-12-10T19:00:37Z

test/CloneFactory.test.js

+
+  describe('factory initialized', function () {
+    beforeEach(async function () {
+      factory = await initFactory();


Since #1380 we've stored this kind of stuff in this, it'd be great for consistency if we kept doing that :)

nventuro · 2018-12-10T19:01:13Z

test/CloneFactory.test.js

+contract('CloneFactory', function ([_, owner]) {
+  let factory;
+
+  describe('factory initialized', function () {


I would drop this describe block for now, no need to make the test suite too complicated.

nventuro · 2018-12-10T19:03:01Z

test/CloneFactory.test.js

+    });
+  });
+
+  const initFactory = async function () {


I wouldn't abstract this (along withreadContractCode and expectedBytecode) into standalone functions until we actually need them (e.g. to prevent repeated code): I'd rather the tests be as explicit and simple as possible, and this obscures them somewhat.

nventuro · 2019-01-18T17:44:47Z

Thanks a lot for your time and contribution @jbogacz. We're unfortunately going to close this for now due to the reasons outlined here.

nventuro added feature New contracts, functions, or helpers. contracts Smart contract code. labels Sep 28, 2018

nventuro self-assigned this Sep 28, 2018

nventuro reviewed Sep 28, 2018

View reviewed changes

contracts/utils/CloneFactory.sol Show resolved Hide resolved

nventuro reviewed Sep 28, 2018

View reviewed changes

nventuro reviewed Oct 2, 2018

View reviewed changes

jbogacz added 3 commits October 3, 2018 07:06

EIP-1167 Clone Factory (#1162)

1a5a46a

EIP-1167 Clone Factory (#1162)

cc338e5

* Initialize master contract inside factory mock constructor * Invoke CloneCreated event inside CloneFactory * Add unit test to validate clone instance bytecode

Simplyfie unit test for clone factory

3248317

nventuro reviewed Dec 10, 2018

View reviewed changes

nventuro added this to the v2.2 milestone Dec 10, 2018

nventuro closed this Jan 18, 2019

EIP-1167 Clone Factory (#1162) #1361

EIP-1167 Clone Factory (#1162) #1361

Uh oh!

Conversation

jbogacz commented Sep 28, 2018 • edited by nventuro Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🚀 Description

Uh oh!

jbogacz commented Sep 28, 2018

Uh oh!

Uh oh!

nventuro left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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!

Uh oh!

Uh oh!

Uh oh!

nventuro left a comment

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!

jbogacz Oct 3, 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!

jbogacz commented Oct 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jbogacz commented Oct 4, 2018

Uh oh!

jbogacz commented Oct 13, 2018

Uh oh!

nventuro left a comment

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!

nventuro commented Jan 18, 2019

Uh oh!

Uh oh!

jbogacz commented Sep 28, 2018 •

edited by nventuro

Loading

jbogacz Oct 3, 2018 •

edited

Loading

jbogacz commented Oct 2, 2018 •

edited

Loading