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

feat: add contract creation execution functions #61

Merged
merged 13 commits into from
May 31, 2024

Conversation

Zer0dot
Copy link
Contributor

@Zer0dot Zer0dot commented May 24, 2024

Motivation

Currently, there isn't a way to deploy contracts directly from smart accounts. This PR addresses that by exposing two new top-level execution functions, one for create and one for create2.

Solution

Introduce two new functions create(bytes calldata initCode, uint256 value) and create2(bytes calldata initCode, bytes32 salt, uint256 value) which create a contract with initCode using their respective deployment methods.

These functions use the standard onlyAuthorized modifier.

Why not use a deployer contract?

Although practical, a deployer contract has a few limitations:

  • Overrides msg.sender throughout contract construction.
  • Prevents deterministic sequential create deployments (due to the nonce not being known until execution).
  • Adds gas to contract creation.

Note that this doesn't mean that using a deployer contract isn't possible-- it's still entirely possible to deploy via, for example, the Create2Factory contract, or any other factory contract via standard executions.

@Zer0dot Zer0dot marked this pull request as draft May 24, 2024 12:42
Copy link
Contributor

@adamegyed adamegyed left a comment

Choose a reason for hiding this comment

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

Nice! Had a couple of questions and comments.

src/common/BaseLightAccount.sol Outdated Show resolved Hide resolved
src/common/BaseLightAccount.sol Outdated Show resolved Hide resolved
src/common/BaseLightAccount.sol Outdated Show resolved Hide resolved
src/common/BaseLightAccount.sol Outdated Show resolved Hide resolved
src/common/BaseLightAccount.sol Outdated Show resolved Hide resolved
src/common/BaseLightAccount.sol Outdated Show resolved Hide resolved
src/common/BaseLightAccount.sol Outdated Show resolved Hide resolved
Copy link
Collaborator

@jaypaik jaypaik left a comment

Choose a reason for hiding this comment

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

Awesome! A few thoughts (don't feel particularly strongly about any of them):

  1. Do you think it's worth following the function signatures in https://github.com/safe-global/safe-smart-account/blob/main/contracts/libraries/CreateCall.sol for marginally easier compatibility with clients that already use Safes today?
  2. CreateCall also emits events. (as does https://github.com/pcaversaccio/createx). Is this worth doing for analytics? Or do we want to save gas?
  3. The examples above use a prefix, e.g., deployCreate2 or performCreate2. Is there an advantage to doing this? Perhaps makes for better grepability?

src/common/BaseLightAccount.sol Outdated Show resolved Hide resolved
@Zer0dot
Copy link
Contributor Author

Zer0dot commented May 29, 2024

Awesome! A few thoughts (don't feel particularly strongly about any of them):

  1. Do you think it's worth following the function signatures in https://github.com/safe-global/safe-smart-account/blob/main/contracts/libraries/CreateCall.sol for marginally easier compatibility with clients that already use Safes today?
  2. CreateCall also emits events. (as does https://github.com/pcaversaccio/createx). Is this worth doing for analytics? Or do we want to save gas?
  3. The examples above use a prefix, e.g., deployCreate2 or performCreate2. Is there an advantage to doing this? Perhaps makes for better grepability?

I'm not sure how compatibility with Safes will work-- as this is a feature built-in to the contract, but point 3 makes me want to modify the signatures anyway!

With respect to events, my rationale is that contracts that need to be indexed can emit their own construction events without enforcing the cost. But, it would make for easier analytics, I'm open to including this as well.

@Zer0dot Zer0dot marked this pull request as ready for review May 30, 2024 15:38
@Zer0dot Zer0dot requested a review from a team May 30, 2024 15:38
Copy link
Contributor

@adamegyed adamegyed left a comment

Choose a reason for hiding this comment

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

Looks good!

@Zer0dot Zer0dot merged commit ebe761a into develop May 31, 2024
2 checks passed
@Zer0dot Zer0dot deleted the feat/create-function branch May 31, 2024 17:17
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.

3 participants