-
Notifications
You must be signed in to change notification settings - Fork 1
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
feat: support create3 #3
Conversation
4069469
to
c8dfb2a
Compare
88188a4
to
a04dbdb
Compare
a04dbdb
to
1353edc
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wow great work 👏 👏 👏
bytes memory creationCode, | ||
uint256 creationFund, | ||
bytes calldata afterDeploymentExecutionPayload, | ||
uint256 afterDeploymentExecutionFund |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One suggestion , can we add one parameter about creationCode hash , so we can check whether the creationCode is correct.
For example, we already deployed in one chain , when we tried to deploy same contract in other chains , if we passed wrong creationCode , we will lose chance to deploy with same address.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For example, we already deployed in one chain , when we tried to deploy same contract in other chains , if we passed wrong creationCode , we will lose chance to deploy with same address.
would adding creationCodeHash
works to prevent this?
eg.
-
burger call `create3Factory.deploy("PCS_V4_VAULT", vault.creationCode, keccak256(vault.hash), ...);
-
mist can make the error calling `create3Factory.deploy("PCS_V4_VAULT", clPoolManager.creationCode, keccak256(clPoolManager.hash), ...);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
or can we have a concrete scenario example? thanks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
scenario: when someone is calling create3Factory.deploy
directly (not via our v4 deployment script).
Context: new-hire asked to deploy Vault in new chain (arb)
1/ step 1: new-hire see the transaction hash on how vault was deploy on eth
2/ step 2: new-hire copy the params and paste in etherscan calling create3Factory.deploy()
At step 2, somehow the new hire copy-pasted the wrong creation code, and this check will help to protect it
) external payable onlyWhitelisted nonReentrant returns (address deployed) { | ||
deployed = Create3.create3( | ||
salt, creationCode, creationFund, afterDeploymentExecutionPayload, afterDeploymentExecutionFund | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion: can add deploy event here.
src/Create3Factory.sol
Outdated
/// @dev ensure this contract is deployed on multiple chain with the same address | ||
contract Create3Factory is ICreate3Factory, Ownable2Step, ReentrancyGuard { | ||
event SetWhitelist(address indexed user, bool isWhitelist); | ||
event Deployed(address indexed deployed, bytes32 salt); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can add creationCodeHash here ?
so can verify codehash when tried to deploy in new chains.
08425e4
to
5425acf
Compare
No description provided.