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

Revert "Replace Create3 with ZeframLou/create3-factory" #1388

Merged
merged 1 commit into from
Sep 18, 2023

Conversation

trajan0x
Copy link
Contributor

@trajan0x trajan0x commented Sep 18, 2023

Reverts #1387

Undoing this for now, when I checked manually to see if #1387 broke anything, I was on the wrong branch

Summary by CodeRabbit

New Features:

  • Added CREATE3Factory contract for deploying contracts to deterministic addresses using the CREATE3 library.
  • Introduced ICREATE3Factory interface that outlines the structure for deploying and retrieving contracts with CREATE3Factory.
  • Updated DeployCREATE3 script to utilize the new CREATE3Factory contract.

Chore:

  • Updated submodule URL for solmate library.
  • Adjusted file paths for forge-std and ds-test libraries in remappings.txt.
  • Modified import paths for CREATE3Factory in various scripts.

@trajan0x trajan0x marked this pull request as ready for review September 18, 2023 15:55
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Sep 18, 2023

Walkthrough

This pull request introduces a new contract CREATE3Factory for deploying contracts to deterministic addresses using the CREATE3 library. It also includes an interface ICREATE3Factory, and updates import paths in scripts. The submodule URL for solmate has been updated.

Changes

File(s) Summary
.gitmodules Updated the submodule URL for solmate.
.../contracts/create3/CREATE3Factory.sol, .../contracts/create3/ICREATE3Factory.sol Introduced a new contract CREATE3Factory and its interface ICREATE3Factory for deterministic contract deployment.
packages/contracts-core/lib/solmate Single commit hash provided, specific changes unknown.
packages/contracts-core/remappings.txt Updated file paths for forge-std and ds-test libraries.
.../script/DeployCREATE3.sol, .../script/utils/DeployerUtils.sol Updated import statements for CREATE3Factory and renamed contract from DeployCREATE3Factory to DeployCREATE3.

🐇💻

In the land of code, where logic is king,
A rabbit hops forth, changes to bring.
With a nibble here, and a nibble there,
He crafts with care, his wisdom to share.

Contracts deployed, paths redefined,
In the maze of code, clarity we find.
So raise your cups, let's toast this feat,
To the rhythm of keys, our joyful beat! 🥂🎉


Tips

Chat with CodeRabbit Bot (@coderabbitai)

  • Mention @coderabbitai in any review comment for bot assistance.
  • Note: Review comments are made on code diffs or files, not on the PR overview.

Pause Incremental Reviews

  • Insert @coderabbitai: ignore in the PR description to halt the bot's ongoing reviews. Remove the line to resume.

@trajan0x trajan0x merged commit 6b100fd into feat/devnet-upper Sep 18, 2023
@trajan0x trajan0x deleted the revert-1387-feat/remove-create3 branch September 18, 2023 15:55
trajan0x added a commit that referenced this pull request Sep 18, 2023
@github-actions github-actions bot added M-contracts Module: Contracts 2-reviewers C-Protocol-Critical PRs that modify protocol-critical code. size/s labels Sep 18, 2023
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Review Status

Actionable comments generated: 1

Commits Files that changed from the base of the PR and between 616c5ab and 7c13e7d commits.
Files selected for processing (7)
  • .gitmodules (1 hunks)
  • packages/contracts-core/contracts/create3/CREATE3Factory.sol (1 hunks)
  • packages/contracts-core/contracts/create3/ICREATE3Factory.sol (1 hunks)
  • packages/contracts-core/lib/solmate (1 hunks)
  • packages/contracts-core/remappings.txt (1 hunks)
  • packages/contracts-core/script/DeployCREATE3.sol (1 hunks)
  • packages/contracts-core/script/utils/DeployerUtils.sol (1 hunks)
Files skipped from review due to trivial changes (4)
  • .gitmodules
  • packages/contracts-core/lib/solmate
  • packages/contracts-core/remappings.txt
  • packages/contracts-core/script/utils/DeployerUtils.sol
Additional comments (Suppressed): 4
packages/contracts-core/script/DeployCREATE3.sol (2)
  • 13-13: The contract name has been changed from DeployCREATE3Factory to DeployCREATE3. Ensure that all references to this contract throughout the codebase have been updated to match the new name.

  • 10-10: The import path for CREATE3Factory has been changed. Please verify if the new path is correct and the contract is accessible at this location.

packages/contracts-core/contracts/create3/CREATE3Factory.sol (2)
  • 14-18: The deploy function is correctly overriding the interface method and using the CREATE3 library to deploy contracts. However, it's important to ensure that the creationCode passed to this function is valid and will result in a successful contract deployment. If the creationCode is invalid, the CREATE3.deploy call will fail and revert the transaction.

  • 21-25: Similar to the deploy function, the getDeployed function is also correctly overriding the interface method and using the CREATE3 library to predict the address of a deployed contract. Again, ensure that the salt and deployer parameters are valid and correspond to an actual deployed contract. If they don't, the CREATE3.getDeployed call will return an incorrect address.

Comment on lines +1 to +22
// SPDX-License-Identifier: AGPL-3.0
pragma solidity >=0.6.0;

/// @title Factory for deploying contracts to deterministic addresses via CREATE3
/// @author zefram.eth
/// @notice Enables deploying contracts using CREATE3. Each deployer (msg.sender) has
/// its own namespace for deployed addresses.
interface ICREATE3Factory {
/// @notice Deploys a contract using CREATE3
/// @dev The provided salt is hashed together with msg.sender to generate the final salt
/// @param salt The deployer-specific salt for determining the deployed contract's address
/// @param creationCode The creation code of the contract to deploy
/// @return deployed The address of the deployed contract
function deploy(bytes32 salt, bytes memory creationCode) external payable returns (address deployed);

/// @notice Predicts the address of a deployed contract
/// @dev The provided salt is hashed together with the deployer address to generate the final salt
/// @param deployer The deployer account that will call deploy()
/// @param salt The deployer-specific salt for determining the deployed contract's address
/// @return deployed The address of the contract that will be deployed
function getDeployed(address deployer, bytes32 salt) external view returns (address deployed);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

The interface ICREATE3Factory is well defined with clear function signatures for deploy and getDeployed. The comments provide a good understanding of what each function does. However, it would be beneficial to include more information about the parameters, especially creationCode in the deploy function. What format should this code be in? Is there any specific requirement or limitation that the user should be aware of?

dwasse added a commit that referenced this pull request Nov 10, 2023
* docker first pass

* fix omnirpc port default behavior

* Docker-compose updates

* add guide for adding new go modules

* typo fix

* indentation fix

* kink fix

* remove vanity

* devnet fix

* backup for max attempt change

* hi

* make retry defaults more sensible

* lint fix

* Revert "hi"

This reverts commit 60dd2b7.

* test

* omnirpc chains endpoint

* close response handle

* actually close response

* more fixes

* wip

* forge install: solmate

* newmates

* create3

* create3factory

* custom config path

* Revert "custom config path"

This reverts commit 47ee5f9.

* deploy config defaults

* Revert "deploy config defaults"

This reverts commit 969a6cd.

* devnet check

* basic updates

* factory fixes

* cleanup

* devnet up/down

* first pass

* guard it up

* guard

* 1 notary

* more updates

* fix wrong domain in deploy script

* diff fix

* comment fix

* agent proof updates

* synapse domain fix

* generic linter fix

* get guard to boot [goreleaser]

* more yaml lint

* some nice debugging tools

* generic linter fix

* finish devnet

* fix lint commandS

* d

* wrap up rudimentary devneT

* create3

* deployer/create fix

* fix ubuntu

* lowercase non-constants

* icreate3 import

* Revert "icreate3 import"

This reverts commit bcb2522.

* move to internal

* another internal change

* update installs on dev containers for easier debugging

* Update packages/contracts-core/README.md

Co-authored-by: χ² <[email protected]>

* address #1381 (comment)

* Replace Create3 with ZeframLou/create3-factory (#1387)

* create3

* forge install: create3-factory

* replace create3 w/ external module

* fix naming issues

* create3 factory re-addition to address #1387 (comment)

---------

Co-authored-by: Trajan0x <[email protected]>

* Revert "Replace Create3 with ZeframLou/create3-factory (#1387)" (#1388)

This reverts commit 616c5ab.

* Replace Create3 with ZeframLou/create3-factory

This reverts commit 6b100fd.

* fix package issue

---------

Co-authored-by: Trajan0x <[email protected]>
Co-authored-by: χ² <[email protected]>
Co-authored-by: Daniel Wasserman <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2-reviewers C-Protocol-Critical PRs that modify protocol-critical code. M-contracts Module: Contracts size/s
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant