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

deterministic Deployment initCall #383

Closed
2 tasks
noyyyy opened this issue Oct 26, 2022 · 8 comments
Closed
2 tasks

deterministic Deployment initCall #383

noyyyy opened this issue Oct 26, 2022 · 8 comments

Comments

@noyyyy
Copy link

noyyyy commented Oct 26, 2022

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
Copy link
Owner

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
Copy link
Author

noyyyy commented Oct 29, 2022

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
Copy link
Owner

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

@noyyyy
Copy link
Author

noyyyy commented Oct 29, 2022

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 noyyyy closed this as completed Oct 29, 2022
@noyyyy
Copy link
Author

noyyyy commented Oct 29, 2022

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
Copy link
Owner

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
Copy link

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

@noyyyy
Copy link
Author

noyyyy commented Oct 30, 2022

Really thanks @wighawag @pcaversaccio

@noyyyy noyyyy closed this as completed Oct 30, 2022
@noyyyy noyyyy closed this as not planned Won't fix, can't repro, duplicate, stale Oct 30, 2022
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

No branches or pull requests

3 participants