-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
base: master
Are you sure you want to change the base?
Conversation
✅ All reviewers have approved. |
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.
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 thecallvalue
handling, however, as there are now two nativevalue
s 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 toCREATOR_CONTRACT_ADDRESS
, or set withdrawal address on the Beacon chain toCREATOR_CONTRACT_ADDRESS
). Should we just keep it locked or should we think about logic to handle it?
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.
The same as with other system contracts, it will be locked. |
This can't happen from an EOA tho - this would require another contract to interact with |
InitcodeTransaction, unlike legacy creation transaction, has a separate |
Maybe I'm misunderstanding something, but how can you call initialise on a contract that is in construction via |
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. |
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.
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) |
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.
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?
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.
calldataload(0)
is simpler/safer, as it reads from an immutable buffer, independent from code that wrote that data into memory.
Not sure about gas schedule changes, but anyway I'd prefer simplicity today over performance in the future.
Co-authored-by: Francisco Giordano <fg@frang.io>
This made me think about whether the Creator Contract doesn't accidentally implement standards we would not like it to. It seems we're ok for some examples I have in mind:
Can you think of any other? Whoops. I think it allows for reentrancy - the initcode can call into the Creator. One potential issue I can identify is that it could attempt to deploy at the same Otherwise, the reentrancy can be ok and useful in order for multiple dependent contracts be deployed out of transaction data, rather than EOFCREATE. Related to the above, I'm thinking whether we shouldn't include |
I think reentrancy is a feature here (I also allow for reentrancy in |
What exactly is circumvented?
There are other forms of malleability, from reading block and transaction parameters to reading other accounts' state. Ultimately to trust a contract deployed at a deterministic address you still need to check the code hash or to inspect the source code to determine it's not malleable. How should we decide which ones should impact on the address? I actually think the more serious issue is that a deployment transaction can be frontrun and sent from a different account. Most of the motivating use cases for this feature need the address to be independent from the caller, but I think most users in general today do care about deployment frontrunning and assume it cannot happen. |
Sorry, I might have ended up conflating 2 subjects: 1/ Creator Contract reentrancy - as pcaversaccio notes, it's OK
Yes, you're right. I'd risk to draw that line at "ones which one normally uses to variate the deployed contract", so code and init_data are ok to be in the hash, callvalue and other - not. A factory which factors in callvalue can always be deployed later, if it's useful. As long as we can ensure that deployment will happen using the Creator Contract, we're fine (noted by chfast). So I guess we're good here.
A factory which factors in |
That's exactly what |
Yes, what I was thinking was that "transaction to creator contract" would become the new default "deployment transaction", replacing those with |
Updates to the Creator Contract as done in ipsilon/eof#177 :
0xff
magic value from thefinal_salt
calculation (CC @gumb0 @frangio)