Add Create2 library #1744

AugustoL · 2019-05-09T18:00:15Z

This PR adds a simple CREATE2Factory based on different implementations I saw around.
The goal is to have the minimum and necessary contract factory to deploy contracts using CREATE2 evm opcode. This I why I didnt added any extra methods to deploy smart contracts with a fixed bytecode, nevertheless this contract can be extended into metatxs or ownable create2 factories.

AugustoL · 2019-05-09T18:07:12Z

@spalladino I followed the same nomeclature used in https://github.com/zeppelinos/zos/blob/master/packages/lib/contracts/upgradeability/ProxyFactory.sol

I dont know if you plan to allow CREATE2 to be used in the proxy factory, but I guess thats something that should be discussed in zeppelinos repo.

ping to @nventuro @cwhinfrey who discussed the implementation in #1644

I Think this PR should be focused on a simple implementation of a CREATE2 factory. And once accepted we can extend this to two more factories, one that support Ownable contracts and another that deploys contracts with a fixed bytecode, where the fixed bytecode version can be used for metatxs contracts.

nventuro

Hello @AugustoL, thank you for working on this!

I only made a couple comments on the contract code so far. What do you think about turning it into a library with internal functions? Otherwise, with the current API we'd be forcing any contract that wants to use the factory to either deploy one itself, or extend it and provide public methods that it doesn't need.

contracts/utils/CREATE2Factory.sol

Giulio2002 · 2019-05-11T12:30:35Z

After having looked at this PR, I would like to suggest to split CREATE2Factory into two contracts. a BasicFactory Contract without public functions and then a standard factory Contract which contains public functions such as deploy and computeAddress. so that if someone wants to create a Factory that takes different arguments in deploy and computeAddress we can still make it inherit from BasicFactory without completely rewriting the contract. an example of this kind of scenario has been provided by @marekkirejczyk in #1651.

contracts/utils/CREATE2Factory.sol

cwhinfrey · 2019-05-12T18:21:32Z

@AugustoL this is awesome! Great to see this coming along. I think one thing to think about is whether people will want to use the regular constructor or the initialize pattern like @spalladino is using in the ZOS Create2 Factory and whether or not we should be opinionated about it here.

Using the regular constructor will mean appending any constructor parameters to the end of the init code before passing it in as the code parameter in deploy(). It will also mean that constructor parameters are locked with the reserved address because changing them will change the address. That's probably desirable for some use cases and may not be for others.

Using the initialize pattern is a bit more flexible in terms of whether the constructor parameters affect the address the contract will be deployed to. However, providing direct support for it may be too opinionated for this depending on how it's done.

After thinking through all of this, I really like @nventuro's idea of having a Create2 library rather than a factory (or having a library and a factory that uses it). It would give people the most flexibility in how they calculate the salt, initialize the contracts, ect. while still encapsulating the Create2 assembly code and address calculation.

nventuro · 2019-05-12T23:34:58Z

@cwhinfrey a flexible library that simply makes the opcode easier to use and then building on top of that with common patterns sounds like the way to go :)

AugustoL · 2019-05-14T14:51:25Z

@nventuro I changed the contract for a library. Next:

We have 2 computeAddress function, one that can receive the deployer address and one that takes in count address(this) as deployer. Should we leave it like that? I think is good to have both of them, other contracts using this libs might want to compute address with a deployer address.
I added an example of Create2 for Ownable contracts, but if you wnat we can remove it, I think is good to have an example of how to use the library.
For initilizable contracts we can add a deployAndInitilize funciton in the library or an example of how to use it with Initilizable contracts, I think having a funciton in the library would be better. WDYT? @cwhinfrey @spalladino

nventuro · 2019-05-14T23:26:24Z

We have 2 computeAddress function, one that can receive the deployer address and one that takes in count address(this) as deployer. Should we leave it like that? I think is good to have both of them, other contracts using this libs might want to compute address with a deployer address.

I like this, since a user may either use this to be a factory itself, or to compute addresses of contracts deployed by other factories. I worry that the names being the same may make this confusing, but am unsure as to what a better name would be (computeOwnDeployedAddress? ew).

The Ownable example is interesting, thanks! We may want to include such a thing in a guide.

Note that the library functions are public: we should make them internal so that contracts can use it by having the code inlined (as opposed to needing to deploy a separate library contract).

AugustoL · 2019-05-15T15:32:15Z

@nventuro what about having only the computeAddress function with the deployer arg? Most of the cases it will be used with address(this) but not all the time, it is more flexible and we wont have "duplicated" code.

Also, what about having an anotrher example of a create2 factory for meta txs? that can be useful.

nventuro · 2019-05-16T17:03:18Z

what about having only the computeAddress function with the deployer arg?

I was also thinking that having just the more flexible one sounds like the way to go, but now I wonder, in which scenario would you need to compute the address where another factory would deploy a contract? @spalladino thoughts?

Regarding the extra example, meta transactions does seem like a useful case. However, I'm not particularly sold on the idea of standalone example contracts, I think they're not very useful by themselves without an accompanying guide. We'll be updating the current guides and docsite in the following days: I'd wait until then to see how we make make this better fit that new format.

stale · 2019-06-07T16:21:22Z

Hi all!
This Pull Request has not had any recent activity, is it still relevant? If so, what is blocking it? Is there anything we can do to help move it forward?
Thanks!

stale · 2019-06-22T17:04:21Z

Hi folks!
This Pull Request is being closed as there was no response to the previous prompt. However, please leave a comment whenever you're ready to resume, so it can be reopened.
Thanks again!

nventuro · 2019-06-24T21:14:17Z

We'd really like to have support for create2 in some way in OpenZeppelin. If anyone has specific thoughts regarding capabilities, API, etc., please share here!

marekkirejczyk · 2019-06-26T12:22:06Z

A couple of comments after working with @JustynaBroniszewska on CREATE2 based functionality in Universal Login:

Create2.deploy could return address, so that in example Create2OwnableFactory we don't need to calculate it
Emitting events seems unnecessary in most cases, but always cost gas, I would propose to move it to user's Factory code.
It seems computeAddress don't need to be implemented in the smart contract, could be js function, that would look sth like this:

const computeAddress = (deployer, salt, code) => {
  const codeHash = keccak256(['bytes'], [code]);
  const hashedData = keccak256(['bytes1', 'address', 'bytes32', 'bytes32'], ['0xff', deployer, salt, codeHash]);
  return `0x${hashedData.slice(26)}`;
};

Looking forward to your opinions.

ngmachado · 2019-06-29T21:12:55Z

A couple of comments after working with @JustynaBroniszewska on CREATE2 based functionality in Universal Login:

Create2.deploy could return address, so that in example Create2OwnableFactory we don't need to calculate it

Emitting events seems unnecessary in most cases, but always cost gas, I would propose to move it to user's Factory code.

It seems computeAddress don't need to be implemented in the smart contract, could be js function, that would look sth like this:
const computeAddress = (deployer, salt, code) => {
  const codeHash = keccak256(['bytes'], [code]);
  const hashedData = keccak256(['bytes1', 'address', 'bytes32', 'bytes32'], ['0xff', deployer, salt, codeHash]);
  return `0x${hashedData.slice(26)}`;
};
Looking forward to your opinions.

Hi,

The two firsts point seems ok, but the third, I think the lib should maintain a method to calculate the address by itself. (is kinda a syntax sugar)

We can build some smart contract logic by calculating the address without the need of external source (feed by the user).

Cheers

ngmachado · 2019-06-29T21:14:27Z

A couple of comments after working with @JustynaBroniszewska on CREATE2 based functionality in Universal Login:

Create2.deploy could return address, so that in example Create2OwnableFactory we don't need to calculate it

Emitting events seems unnecessary in most cases, but always cost gas, I would propose to move it to user's Factory code.

It seems computeAddress don't need to be implemented in the smart contract, could be js function, that would look sth like this:
const computeAddress = (deployer, salt, code) => {
  const codeHash = keccak256(['bytes'], [code]);
  const hashedData = keccak256(['bytes1', 'address', 'bytes32', 'bytes32'], ['0xff', deployer, salt, codeHash]);
  return `0x${hashedData.slice(26)}`;
};
Looking forward to your opinions.
Hi,

The two firsts point seems ok, but the third, I think the lib should maintain a method to calculate the address by itself. (is kinda a syntax sugar)

We can build some smart contract logic by calculating the address without the need of external source (feed by the user).

Cheers

I found this PR because i was looking for that specific method 💯

marekkirejczyk · 2019-06-30T07:29:15Z

A couple of comments after working with @JustynaBroniszewska on CREATE2 based functionality in Universal Login:

Create2.deploy could return address, so that in example Create2OwnableFactory we don't need to calculate it

Emitting events seems unnecessary in most cases, but always cost gas, I would propose to move it to user's Factory code.

It seems computeAddress don't need to be implemented in the smart contract, could be js function, that would look sth like this:
const computeAddress = (deployer, salt, code) => {
  const codeHash = keccak256(['bytes'], [code]);
  const hashedData = keccak256(['bytes1', 'address', 'bytes32', 'bytes32'], ['0xff', deployer, salt, codeHash]);
  return `0x${hashedData.slice(26)}`;
};
Looking forward to your opinions.
Hi,
The two firsts point seems ok, but the third, I think the lib should maintain a method to calculate the address by itself. (is kinda a syntax sugar)
We can build some smart contract logic by calculating the address without the need of external source (feed by the user).
Cheers
I found this PR because i was looking for that specific method 💯

Sounds fair.
I propose adding a js util function, as it would come in handy for some cases.

ngmachado · 2019-06-30T19:50:04Z

Hi everybody,

It looks like the is a code duplication in two methods, why not encapsulate as :

 function computeAddress(bytes32 salt, bytes memory codeInit) internal returns(address) {
        return computeAddress(address(this), salt, codeInit);
 }

The deploy function should return an address instead of emitting an event as:

function deploy(bytes32 salt, bytes memory codeInit) internal returns(address) {
        return _deploy(salt, codeInit);
}

Cheers

Skyge · 2019-07-05T10:13:38Z

contracts/utils/Create2.sol

+     * @param code The bytecode of of the contract to be deployed
+     */
+    function deploy(bytes32 salt, bytes memory code) internal {
+        address addr = _deploy(salt, code);


Maybe you can use deployAddress to replace addr, it is shorter, but it will be more difficult to understand.

Skyge · 2019-07-05T10:23:12Z

@ngmachado

It looks like the is a code duplication in two methods, why not encapsulate as :

Not really, just like the function name, one is to deploy, and the other is to calculate. you can use computeAddress() to find an expected contract address with a suitable salt, and then you can use deploy() to get that expected address.

Skyge · 2019-07-05T11:07:44Z

A couple of comments after working with @JustynaBroniszewska on CREATE2 based functionality in Universal Login:

Create2.deploy could return address, so that in example Create2OwnableFactory we don't need to calculate it

Emitting events seems unnecessary in most cases, but always cost gas, I would propose to move it to user's Factory code.

It seems computeAddress don't need to be implemented in the smart contract, could be js function, that would look sth like this:
const computeAddress = (deployer, salt, code) => {
  const codeHash = keccak256(['bytes'], [code]);
  const hashedData = keccak256(['bytes1', 'address', 'bytes32', 'bytes32'], ['0xff', deployer, salt, codeHash]);
  return `0x${hashedData.slice(26)}`;
};
Looking forward to your opinions.
Hi,

The two firsts point seems ok, but the third, I think the lib should maintain a method to calculate the address by itself. (is kinda a syntax sugar)

We can build some smart contract logic by calculating the address without the need of external source (feed by the user).

Cheers

At first, I think we can keep both, more than one approach, more than one choice. But when I decide to compute a special address, I find it is more convenient by js function.

Skyge · 2019-07-05T11:18:12Z

contracts/examples/Create2OwnableFactory.sol

+import "../ownership/Ownable.sol";
+
+contract Create2OwnableFactory {
+  function deploy(bytes32 salt, bytes memory code) public {


If I use uint256 rather than bytes32, is it OK?

@Skyge I believe it will be fairly common to calculate the salt with a keccak256 hash. Is there a case where the salt would be calculated as an integer?

ngmachado · 2019-07-05T11:26:32Z

@ngmachado

It looks like the is a code duplication in two methods, why not encapsulate as :

Not really, just like the function name, one is to deploy, and the other is to calculate. you can use computeAddress() to find an expected contract address with a suitable salt, and then you can use deploy() to get that expected address.

I was thinking in maintaining the two methods, just make one call the other if you dont pass the first param (deployer address). The code inside the two function are same.

stale · 2019-08-21T19:20:58Z

Hi all!
This Pull Request has not had any recent activity, is it still relevant? If so, what is blocking it? Is there anything we can do to help move it forward?
Thanks!

nventuro · 2019-09-05T02:25:19Z

It looks like the latest version of solidity-coverage will be able to support this, but we can't yet upgrade due to sc-forks/solidity-coverage#378. Once that's fixed, we should upgrade and merge this.

nventuro · 2019-09-06T18:25:53Z

@frangio I've upgraded solidity-coverage to a version that properly supports inline assembly, and updated the documentation to the new format, this should be ready to be merged now.

frangio

Thanks everyone involved! This is a great addition to the library.

contracts/utils/Create2.sol

contracts/mocks/Create2Impl.sol

frangio · 2019-09-11T23:51:21Z

contracts/utils/Create2.sol

+ * See the https://eips.ethereum.org/EIPS/eip-1014#motivation[EIP] for more
+ * information.
+ */
+library Create2 {


I'm not a fan of this name. Do we have other options?

Usage will be Create2.deploy(). I also dislike the name, but I'm not sure how we can more succinctly convey the underlying idea.

Yep, I think we wont came up with a better name.

… of created contract

stale · 2019-09-27T15:45:41Z

Hi all!
This Pull Request has not had any recent activity, is it still relevant? If so, what is blocking it? Is there anything we can do to help move it forward?
Thanks!

frangio

Thank you @AugustoL!

I'm merging temporarily into a feature branch. I want us to think a bit more about alternative names. I also want to add a revert reason for when the deploy fails, but this PR has been open for too long.

* Add Create2 library (#1744) * feat(contracts): Add Create2 library to use create2 evm opcode * Upgrade sol-coverage * Add changelog entry * Update comments and code style * Remove create2 helper * Fix linter error * Fix truffle dependency * Fix linter error * refactor(Create2): Remove _deploy internal function * test(Create2): test Create2 with inline assembly code * fix(Create2): Check address returned form create2 instead of codesize of created contract * refactor(Create2):Add revert reason when Create2 deploy fails (#2062) * fix merge with master * fix test Co-authored-by: Augusto Lemble <me@augustol.com>

* Add Create2 library (OpenZeppelin#1744) * feat(contracts): Add Create2 library to use create2 evm opcode * Upgrade sol-coverage * Add changelog entry * Update comments and code style * Remove create2 helper * Fix linter error * Fix truffle dependency * Fix linter error * refactor(Create2): Remove _deploy internal function * test(Create2): test Create2 with inline assembly code * fix(Create2): Check address returned form create2 instead of codesize of created contract * refactor(Create2):Add revert reason when Create2 deploy fails (OpenZeppelin#2062) * fix merge with master * fix test Co-authored-by: Augusto Lemble <me@augustol.com>

nventuro reviewed May 10, 2019

View reviewed changes

nventuro added contracts Smart contract code. feature New contracts, functions, or helpers. labels May 10, 2019

cwhinfrey reviewed May 12, 2019

View reviewed changes

contracts/utils/CREATE2Factory.sol Outdated Show resolved Hide resolved

AugustoL changed the title ~~[WIP] Add CREATE2factory~~ Add CREATE2factory May 23, 2019

stale bot added the stale label Jun 7, 2019

stale bot closed this Jun 22, 2019

nventuro reopened this Jun 24, 2019

stale bot removed the stale label Jun 24, 2019

Skyge reviewed Jul 5, 2019

View reviewed changes

stale bot added the stale label Aug 21, 2019

Merge branch 'master' into feat/add-create2factory

d86832d

stale bot removed the stale label Aug 26, 2019

cgewecke mentioned this pull request Aug 27, 2019

Cover Assembly sc-forks/solidity-coverage#176

Open

nventuro added 4 commits September 6, 2019 14:53

Upgrade sol-coverage

c2c2afe

Add changelog entry

9da7146

Update comments and code style

b62393f

Remove create2 helper

8bdf59b

nventuro requested a review from frangio September 6, 2019 18:23

Fix linter error

22c81cb

nventuro added 3 commits September 6, 2019 16:07

Merge branch 'master' into feat/add-create2factory

fd6b259

Fix truffle dependency

be73b23

Fix linter error

7e485e2

frangio reviewed Sep 11, 2019

View reviewed changes

AugustoL added 3 commits September 12, 2019 10:25

refactor(Create2): Remove _deploy internal function

3921ba1

test(Create2): test Create2 with inline assembly code

c8391fd

fix(Create2): Check address returned form create2 instead of codesize…

0df686c

… of created contract

stale bot added the stale label Sep 27, 2019

nventuro requested a review from frangio September 30, 2019 18:29

stale bot removed the stale label Sep 30, 2019

frangio changed the base branch from master to feature-create2 October 4, 2019 15:41

frangio approved these changes Oct 4, 2019

View reviewed changes

frangio merged commit 970f687 into OpenZeppelin:feature-create2 Oct 4, 2019

frangio mentioned this pull request Dec 2, 2019

Create2 feature pending tasks #2013

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Create2 library #1744

Add Create2 library #1744

AugustoL commented May 9, 2019

AugustoL commented May 9, 2019

nventuro left a comment

Giulio2002 commented May 11, 2019

cwhinfrey commented May 12, 2019

nventuro commented May 12, 2019

AugustoL commented May 14, 2019

nventuro commented May 14, 2019

AugustoL commented May 15, 2019

nventuro commented May 16, 2019 •

edited

Loading

stale bot commented Jun 7, 2019

stale bot commented Jun 22, 2019

nventuro commented Jun 24, 2019

marekkirejczyk commented Jun 26, 2019 •

edited

Loading

ngmachado commented Jun 29, 2019

ngmachado commented Jun 29, 2019

marekkirejczyk commented Jun 30, 2019

ngmachado commented Jun 30, 2019

Skyge Jul 5, 2019

Skyge commented Jul 5, 2019

Skyge commented Jul 5, 2019 •

edited

Loading

Skyge Jul 5, 2019

cwhinfrey Jul 5, 2019

ngmachado commented Jul 5, 2019

stale bot commented Aug 21, 2019

nventuro commented Sep 5, 2019

nventuro commented Sep 6, 2019

frangio left a comment

frangio Sep 11, 2019

nventuro Sep 12, 2019 •

edited

Loading

AugustoL Sep 12, 2019

stale bot commented Sep 27, 2019

frangio left a comment

Add Create2 library #1744

Add Create2 library #1744

Conversation

AugustoL commented May 9, 2019

AugustoL commented May 9, 2019

nventuro left a comment

Choose a reason for hiding this comment

Giulio2002 commented May 11, 2019

cwhinfrey commented May 12, 2019

nventuro commented May 12, 2019

AugustoL commented May 14, 2019

nventuro commented May 14, 2019

AugustoL commented May 15, 2019

nventuro commented May 16, 2019 • edited Loading

stale bot commented Jun 7, 2019

stale bot commented Jun 22, 2019

nventuro commented Jun 24, 2019

marekkirejczyk commented Jun 26, 2019 • edited Loading

ngmachado commented Jun 29, 2019

ngmachado commented Jun 29, 2019

marekkirejczyk commented Jun 30, 2019

ngmachado commented Jun 30, 2019

Skyge Jul 5, 2019

Choose a reason for hiding this comment

Skyge commented Jul 5, 2019

Skyge commented Jul 5, 2019 • edited Loading

Skyge Jul 5, 2019

Choose a reason for hiding this comment

cwhinfrey Jul 5, 2019

Choose a reason for hiding this comment

ngmachado commented Jul 5, 2019

stale bot commented Aug 21, 2019

nventuro commented Sep 5, 2019

nventuro commented Sep 6, 2019

frangio left a comment

Choose a reason for hiding this comment

frangio Sep 11, 2019

Choose a reason for hiding this comment

nventuro Sep 12, 2019 • edited Loading

Choose a reason for hiding this comment

AugustoL Sep 12, 2019

Choose a reason for hiding this comment

stale bot commented Sep 27, 2019

frangio left a comment

Choose a reason for hiding this comment

nventuro commented May 16, 2019 •

edited

Loading

marekkirejczyk commented Jun 26, 2019 •

edited

Loading

Skyge commented Jul 5, 2019 •

edited

Loading

nventuro Sep 12, 2019 •

edited

Loading