wighawag / hardhat-deploy

hardhat deployment plugin
MIT License
1.18k stars 286 forks source link

deterministic Deployment initCall #383

Closed noyyyy closed 1 year ago

noyyyy commented 1 year ago

Is your feature request related to a problem? Please describe.

Sometimes we need to deploy some contracts with privileges in deterministic deployments way, like Ownable, and AccessControl. By default, most library give the privilege to msg.sender rather tx.originand the factory contract get the privilege. On the other hand, we try to void tx.origin in the contract.

Describe the solution you'd like**

we need a new deterministicDeployment factory and extra args for deterministicDeployment.

factory contract can be like this(source: https://ethereum-magicians.org/t/erc-2470-singleton-factory/3933/27). Provide it on each network.

contract CounterfactualFactory
{
        constructor() internal {}

    function deploy(bytes memory _code, bytes32 _salt, uint256 _value, bytes memory _initCall)
    public returns(address)
    {
        bytes memory code = _code;
        bytes32      salt = keccak256(abi.encodePacked(_salt, _value));
        address      addr;
        // solium-disable-next-line security/no-inline-assembly
        assembly
        {
            addr := create2(_value, add(code, 0x20), mload(code), salt)
            if iszero(extcodesize(addr)) { revert(0, 0) }
        }
                if(_initCall.length > 0){
                        (bool success, bytes memory reason) = addr.call(_initCall);
                        require(success, string(reason));
                }
        return addr;
    }

    function predictAddress(bytes memory _code, bytes32 _salt, uint256 _value, bytes memory _initCall) 
    public view returns (address)
    {
        return address(bytes20(keccak256(abi.encodePacked(
            bytes1(0xff),
            address(this),
            keccak256(abi.encodePacked(_salt, _value)),
            keccak256(_code)
        )) << 0x60));
    }
}

And we need more args in deploy script, like this

  await deploy('MyContract', {
    from: deployer,
    args: [],
    log: true,
    deterministicDeployment: {
      salt: formatBytes32String('salt'),
      // transfers ownership to 0x4a9b5412bfa62c5556a7f9052335a57e5efd555e
      initCall: '0xb242e5340000000000000000000000000x4a9b5412bfa62c5556a7f9052335a57e5efd555e',
      value: '0',
    },
  });

For more convenience, we should support contract call auto encoding rather than calculate initcall bytes code manually.

Describe alternatives you've considered

No alternatives til now.

Additional context

How deterministic deployment works currently: https://github.com/wighawag/hardhat-deploy#4-deterministicdeployment-ability-to-specify-a-deployment-factory

TODO

This is not a tiny work and maybe there is already a solution. Just open this issue and expect more discussions.

wighawag commented 1 year ago

By default, most library give the privilege to msg.sender

They should definitely not do this, and if they do, they should change and let user provide the owner

Why can't you use the constructor arguments ?

noyyyy commented 1 year ago

By default, most library give the privilege to msg.sender

They should definitely not do this, and if they do, they should change and let user provide the owner

Why can't you use the constructor arguments ?

For example, the openzepplin ownable give owner to msg.sender https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/access/Ownable.sol#L28-L30. Maybe this way is simple and easy to use. And many developers(including me) may be used to inheriting the oz's ownable contract.

wighawag commented 1 year ago

For example, the openzepplin ownable give owner to msg.sender https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/access/Ownable.sol#L28-L30.

Yes unfortunately openzeppelin does it, see : https://github.com/OpenZeppelin/openzeppelin-contracts/issues/2639

noyyyy commented 1 year ago

For example, the openzepplin ownable give owner to msg.sender https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/access/Ownable.sol#L28-L30.

Yes unfortunately openzeppelin does it, see : OpenZeppelin/openzeppelin-contracts#2639

I agree with you but it'll be too late when openzeppelin changes this.

You're right, constructor can do anything to replace the initial call. Thanks for your time.

I will not import oz's ownable directly but copy and edit to resolve this problem.

noyyyy commented 1 year ago

Reopen this because I just realized that the owner in constructor arguments will affect the bytecode for deployments and these arguments shouldn't influence the exact bytecode for the create2 calculation. For example, Gnosis safe wallet address on different networks is different currently. So initial call is not useless.

wighawag commented 1 year ago

I disagree, the resulting address should absolutely change if the args changed. imagine if the address remained, someone could take control of the address on any other network, they could even frontrun you

pcaversaccio commented 1 year ago

absolutely agree with @wighawag - the address MUST change. For anyone looking for a solution to this problem, here you go: https://github.com/pcaversaccio/create2deployer/issues/81#issuecomment-1296206131. Constructor arguments are enough - what you must understand is the inheritance pattern!

noyyyy commented 1 year ago

Really thanks @wighawag @pcaversaccio