Skip to content
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.

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

[Step 1 of 2] feat: add create2Factory #1

Merged
merged 16 commits into from
Oct 15, 2024
Merged

Conversation

ChefMist
Copy link
Collaborator

@ChefMist ChefMist commented Oct 10, 2024

Based on option 2c

This is a 2 steps process:

  1. [PR] Step 1: Add create2 factory
  2. [No PR, just executing] Step 2: Deploy create2Factory across multiple chains

Original goal: have all deployment script here. However we cannot do that as this means all contract will deploy from the same foundry.toml config

@ChefMist ChefMist changed the title [Step 1 of 2] feat: add create2Factory [Step 1 of 3] feat: add create2Factory Oct 10, 2024
@ChefMist ChefMist changed the base branch from master to main October 10, 2024 10:35
@ChefMist ChefMist changed the title [Step 1 of 3] feat: add create2Factory [Step 1 of 2] feat: add create2Factory Oct 11, 2024
@ChefMist
Copy link
Collaborator Author

ready for review, no more change will be added

test/Create2Factory.t.sol Outdated Show resolved Hide resolved
test/Create2Factory.t.sol Show resolved Hide resolved
test/Create2Factory.t.sol Show resolved Hide resolved
Copy link
Contributor

@chefburger chefburger left a comment

Choose a reason for hiding this comment

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

lgtm

nonReentrant
returns (address deployed)
{
deployed = Create2.deploy(msg.value, salt, creationCode);

Choose a reason for hiding this comment

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

Suggestion: can we add deploy event here , so we can know which contract deployed by this factory

@chefburger chefburger force-pushed the feat/add-create2-factory branch from ee79f71 to d77c1c3 Compare October 15, 2024 04:52

if (newOwner != address(0)) {
// transfer ownership
Ownable(addr).transferOwnership(newOwner);

Choose a reason for hiding this comment

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

I think the only issue with this is what if the addr is not Ownable? like if it is IAccessControl?

Copy link
Contributor

@chefburger chefburger Oct 15, 2024

Choose a reason for hiding this comment

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

i guess then we might need to have a separate ProxyChild for that contract. In my sense the "ProxyChild" is more like a on-chain "deployScript" with fixed addr and nonce = 0. It needs to be customized according to the contract we want to deploy

Choose a reason for hiding this comment

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

then maybe we can just generalized it by deploy(creationCode, calldata) and then addr.call(calldata)

Copy link
Contributor

@chefburger chefburger Oct 15, 2024

Choose a reason for hiding this comment

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

will update this part if this approach is taken

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

same thoughts, agree with Omelette since some of his farming contract is not using Ownable 😆

src/Create3.sol Outdated
//SPDX-License-Identifier: Unlicense
pragma solidity ^0.8.0;

import {CustomizedProxyChild} from "./CustomizedProxyChild.sol";
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

would there also be a Create3Factory.sol ?

src/Create3.sol Outdated
// TODO: update desc

/**
* @title A library for deploying contracts EIP-3171 style.
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

maybe can put the reference to where this was 'referenced' from

@chefburger chefburger force-pushed the feat/add-create2-factory branch from d77c1c3 to f6a5133 Compare October 15, 2024 08:21
@chefburger chefburger merged commit 9e648d1 into main Oct 15, 2024
4 checks passed
@ChefMist ChefMist deleted the feat/add-create2-factory branch October 16, 2024 01:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants