💥 Discussion: Comments on Current Version

[mds1]

Comments on the current version, which is commit bbbd299:

• [x] 1. Can't tell what the guard modifier is just from it's name, maybe something like preventCrossChainRedeploy but that's verbose. (https://github.com/pcaversaccio/createx/pull/4)
• [x] 2. For the goal mentioned in the guard modifier comments, you only need to hash with chain ID, not with msg.sender. I'd suggest removing msg.sender for that reason. (https://github.com/pcaversaccio/createx/pull/4)
• [x] 3. onlyMsgSender modifier: Wondering if this is better than hashing with msg.sender? Trying to think of tradeoffs. I know 0age uses this approach in his factory, but hashing might be more consistent (https://github.com/pcaversaccio/createx/pull/4)
• [x] 4. Any objections to shortening the name to CreateX? Feels catchier which I think is good for helping people rally around it. Also means the interface that devs will use can be ICreateX which is shorter (https://github.com/pcaversaccio/createx/pull/2)
• [x] 5. Regarding (bool refunded,) = msg.sender.call{value: balance}("") lines (https://github.com/pcaversaccio/createx/pull/4):
  • If deployer is a one-time key (e.g. nick's method) this can result in struck funds
  • Cost to send the ETH may be greater than the amount being sent
  • Given above, I'd suggest removing this to keep the logic simpler, and we can add claim or skim methods something to claim any stray ETH/tokens
• [x] 6. Regarding (bool success,) = implementation.call{value: msg.value}(abi.encodeWithSignature("initializer()")) lines
  • [x] 6.1 The data to pass here should be an input—we don't know what the sig will be and often it needs inputs, so we shouldn't hardcode initializer() as the sig (https://github.com/pcaversaccio/createx/pull/4)
  • [x] 6.2 We're calling it on the implementation but should be calling it on the newly deployed proxy (https://github.com/pcaversaccio/createx/pull/4)
• [x] 7. deployCreate2(initCode): maybe include block.difficulty/prevrandao there also? (https://github.com/pcaversaccio/createx/pull/4)
• [x] 8. Missing methods (https://github.com/pcaversaccio/createx/pull/4):
  • [x] 8.1 The create2 section is missing methods that have onlyMsgSender without guard. Sample use case: I want the same address on all chains, but don't want others to be able to deploy (https://github.com/pcaversaccio/createx/pull/4)
  • [x] 8.2 Similarly the create3 section is missing versions with only the guard no onlyMsgSender modifier. Sample use case: I want the same address on all chains and anyone should be able to deploy it (https://github.com/pcaversaccio/createx/pull/4)
  • [x] 8.3 We should make sure each deployment opcode (create/create2/create3) supports any combination of modifiers (none, guard, onlyMsgSender, both) (https://github.com/pcaversaccio/createx/pull/4)
• [x] 9. Pretty sure all input bytes will be copied to memory anyway when solidity prepares the call, so changing calldata params to memory should be a bit cheaper/smaller bytecode (https://github.com/pcaversaccio/createx/pull/3)
• [x] 10. What's your plan for formatting? I see // prettier-ignore comments but no package.json with prettier as a dep. I'd just use forge fmt here and define the config in foundry.toml to remove the need for a dep (https://github.com/pcaversaccio/createx/pull/3)
• [x] 11. Similar question on linting, I see the // solhint-disable comments but no dep. Personally I think we can remove those also (https://github.com/pcaversaccio/createx/pull/3)

[pcaversaccio]

[ ] 1. Can't tell what the guard modifier is just from it's name, maybe something like preventCrossChainRedeploy but that's verbose.

Yeah, I agree. What about xchainRedeployGuard?

[ ] 7. deployCreate2(initCode): maybe include block.difficulty/prevrandao there also?

So your proposal would be like that (or should I keep block.timestamp)? I mean in any case it doesn't increase the entropy much, and is anyways still pseudo-randomly:

function deployCreate2(bytes memory initCode) public payable returns (address newContract) {
    return
        deployCreate2(
            keccak256(abi.encode(block.prevrandao, blockhash(block.number))),
            initCode
        );
}

[mds1]

Yeah, I agree. What about xchainRedeployGuard?

works for me!

[ ] 7. deployCreate2(initCode): maybe include block.difficulty/prevrandao there also?

So your proposal would be like that (or should I keep block.timestamp)? I mean in any case it doesn't increase the entropy much, and is anyways still pseudo-randomly:

function deployCreate2(bytes memory initCode) public payable returns (address newContract) {
    return
        deployCreate2(
            keccak256(abi.encode(block.prevrandao, blockhash(block.number))),
            initCode
        );
}

I think ideally we should try to ensure the pseudo-random number is always unique, regardless of chain. For example on arbitrum block.prevrandao always returns a constant of 1, and blockhash is just a cryptographically insecure, pseudo-random hash for x, and it wouldn't surprise me if some chains just have a constant value there. Since this will be deployed on many chains, we might want to load up the pseudo-random salt with various params to ensure as much uniqueness as possible. All the block property opcodes are really cheap anyway, so maybe we do:

bytes32 salt = keccak256(
    abi.encode(
        blockhash(block.number),
        block.coinbase,
        block.number,
        block.timestamp,
        block.prevrandao,
        block.chainid,
        msg.sender
    )
);

I can see an argument where this feels like overkill, but it does only cost 32 gas to use all those opcodes (blockhash is 20 gas) so maybe this is the safest way to do it

[pcaversaccio]

works for me!

cool, will use xChainRedeployGuard

I think ideally we should try to ensure the pseudo-random number is always unique, regardless of chain. For example on arbitrum block.prevrandao always returns a constant of 1, and blockhash is just a cryptographically insecure, pseudo-random hash for x, and it wouldn't surprise me if some chains just have a constant value there. Since this will be deployed on many chains, we might want to load up the pseudo-random salt with various params to ensure as much uniqueness as possible. All the block property opcodes are really cheap anyway, so maybe we do:

bytes32 salt = keccak256(
    abi.encode(
        blockhash(block.number),
        block.coinbase,
        block.number,
        block.timestamp,
        block.prevrandao,
        block.chainid,
        msg.sender
    )
);

I can see an argument where this feels like overkill, but it does only cost 32 gas to use all those opcodes (blockhash is 20 gas) so maybe this is the safest way to do it

Ok, yeah better safe than sorry here indeed.

[pcaversaccio]

Missing methods:

• [ ] 8.1 The create2 section is missing methods that have onlyMsgSender without guard. Sample use case: I want the same address on all chains, but don't want others to be able to deploy
• [ ] 8.2 Similarly the create3 section is missing versions with only the guard no onlyMsgSender modifier. Sample use case: I want the same address on all chains and anyone should be able to deploy it
• [ ] 8.3 We should make sure each deployment opcode (create/create2/create3) supports any combination of modifiers (none, guard, onlyMsgSender, both)

• I don't understand the value-add of the modifiers xChainRedeployGuard and onlyMsgSender for the CREATE functions. Since the contract creation address is based on the account nonce of CreateX and the corresponding address, we can't allow anyone to block a certain contract creation on a different chain. But maybe I misunderstand your intentions here. Can you elaborate?

• Actually, I think a made a stupid mistake in implementing the CREATE2 functions (thanks Vyper 0-day for the sleep deprivation). We can't combine functions with different modifier levels since there is always the non-modifier version which would allow us to circumvent this protection. Let me make an example. I call deployCreate2Guarded on Ethereum, but could redeploy on Optimism using deployCreate2 since I can reverse engineer the salt. Thus, we might only use one CREATE2 method using the onlyMsgSender using 0age's approach to bypass the check if the first 20 bytes are zero. Thoughts?