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

Update EIP-7873: Creator Contract - revert reason & magic value #9391

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

pdobacz
Copy link
Contributor

@pdobacz pdobacz commented Feb 20, 2025

Updates to the Creator Contract as done in ipsilon/eof#177 :

  • remove 0xff magic value from the final_salt calculation (CC @gumb0 @frangio)
  • pass on as return data the entire revert return data (presumably revert reason) in case deployment fails (CC @frangio )

@github-actions github-actions bot added c-update Modifies an existing proposal s-draft This EIP is a Draft t-core labels Feb 20, 2025
@eth-bot
Copy link
Collaborator

eth-bot commented Feb 20, 2025

✅ All reviewers have approved.

Copy link
Contributor

@pcaversaccio pcaversaccio left a comment

Choose a reason for hiding this comment

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

I have two questions:

  • Some use cases require a creation-and-initialise (can only happened after the bytecode is live) call; should this feature (i.e. the initialise call) also be offered as part of the creator contract? I offer this possibility in my CreateX factory for example. This would also complicate the callvalue handling, however, as there are now two native values to be considered: one for the creation tx as well as one for the initalise call.
  • How should we deal with ETH forced into the creator contract itself (can happen via selfdestruct send, set blockfee recipient address to CREATOR_CONTRACT_ADDRESS, or set withdrawal address on the Beacon chain to CREATOR_CONTRACT_ADDRESS). Should we just keep it locked or should we think about logic to handle it?

@gumb0
Copy link
Member

gumb0 commented Feb 21, 2025

  • Some use cases require a creation-and-initialise (can only happened after the bytecode is live) call; should this feature (i.e. the initialise call) also be offered as part of the creator contract? I offer this possibility in my CreateX factory for example. This would also complicate the callvalue handling, however, as there are now two native values to be considered: one for the creation tx as well as one for the initalise call.

If this initilize call can be achieved with a separate EXTCALL to deployed address, I don't see the reason to include this as a feature of Creator Contract.

  • How should we deal with ETH forced into the creator contract itself (can happen via selfdestruct send, set blockfee recipient address to CREATOR_CONTRACT_ADDRESS, or set withdrawal address on the Beacon chain to CREATOR_CONTRACT_ADDRESS). Should we just keep it locked or should we think about logic to handle it?

The same as with other system contracts, it will be locked.

@pcaversaccio
Copy link
Contributor

If this initilize call can be achieved with a separate EXTCALL to deployed address, I don't see the reason to include this as a feature of Creator Contract.

This can't happen from an EOA tho - this would require another contract to interact with CREATOR_CONTRACT_ADDRESS. Use case is: I have an EOA as a deployer, some init code and some data to initialise the contract.

@gumb0
Copy link
Member

gumb0 commented Feb 21, 2025

If this initilize call can be achieved with a separate EXTCALL to deployed address, I don't see the reason to include this as a feature of Creator Contract.

This can't happen from an EOA tho - this would require another contract to interact with CREATOR_CONTRACT_ADDRESS. Use case is: I have an EOA as a deployer, some init code and some data to initialise the contract.

InitcodeTransaction, unlike legacy creation transaction, has a separate data field for initialization data, so why couldn't this all create-and-initialize be done by a single TXCREATE, that takes initcode from one of tx.initcodes, and init data from tx.data ?

@pcaversaccio
Copy link
Contributor

pcaversaccio commented Feb 21, 2025

If this initilize call can be achieved with a separate EXTCALL to deployed address, I don't see the reason to include this as a feature of Creator Contract.

This can't happen from an EOA tho - this would require another contract to interact with CREATOR_CONTRACT_ADDRESS. Use case is: I have an EOA as a deployer, some init code and some data to initialise the contract.

InitcodeTransaction, unlike legacy creation transaction, has a separate data field for initialization data, so why couldn't this all create-and-initialize be done by a single TXCREATE, that takes initcode from one of tx.initcodes, and init data from tx.data ?

Maybe I'm misunderstanding something, but how can you call initialise on a contract that is in construction via TXCREATE but the initialise function itself of that contract requires existence of the bytecode itself (i.e. cannot be called in the constructor)?

@gumb0
Copy link
Member

gumb0 commented Feb 21, 2025

Maybe I'm misunderstanding something, but how can you call initialise on a contract that is in construction via TXCREATE but the initialise function itself of that contract requires existence of the bytecode itself (i.e. cannot be called in the constructor)?

If it cannot be called in the constructor (why?), then it won't work.

If it's rarely used optional feature, I'd say this should be part of a different TXCREATE-factory contract, which users are free to deploy.

@pcaversaccio
Copy link
Contributor

Maybe I'm misunderstanding something, but how can you call initialise on a contract that is in construction via TXCREATE but the initialise function itself of that contract requires existence of the bytecode itself (i.e. cannot be called in the constructor)?

If it cannot be called in the constructor (why?), then it won't work.

If it's rarely used optional feature, I'd say this should be part of a different TXCREATE-factory contract, which users are free to deploy.

So any use case that requires an external callback into the contract you create, would first need to be created otherwise it fails. An example would be using flashloans. Another use case are upgradeable contracts where you can't call constructors but you need to move it to an external initialiser function.

@frangio
Copy link
Contributor

frangio commented Feb 21, 2025

any use case that requires an external callback into the contract you create

This is a legitimate use case. Another one that we've already identified is creating a contract where the address is independent of some of the initialization parameters. However, I think the creator contract needs to be as simple as possible to achieve its goal and so I don't think it should accomodate any of these use cases. I think this would be the right choice even if they were the majority.

I'd say this should be part of a different TXCREATE-factory contract, which users are free to deploy.

This would be the answer. Note that once the creator contract is in place the guarantees about deterministic addresses can extend to any new factories, this is the only goal we should optimize for IMO.


let tx_initcode_hash := calldataload(0)
Copy link
Contributor

Choose a reason for hiding this comment

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

This could also be mload(0). Is there any reason to prefer one over the other? Are there any planned gas schedule changes that would impact here?


if iszero(ret) {
let ret_data_size := returndatasize()
returndatacopy(0, ret_data_size)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
returndatacopy(0, ret_data_size)
returndatacopy(0, 0, ret_data_size)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c-update Modifies an existing proposal s-draft This EIP is a Draft t-core
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants