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

Provide a proxy factory library #2339

Closed
frangio opened this issue Aug 28, 2020 · 13 comments
Closed

Provide a proxy factory library #2339

frangio opened this issue Aug 28, 2020 · 13 comments

Comments

@frangio
Copy link
Contributor

frangio commented Aug 28, 2020

We should provide functionality to make it simple to create proxy instances at runtime. Creating a single proxy instance is simple using the proxy contracts provided, but it's worth providing something on top of that that implements a common pattern when the implementation contract needs to be reused. This pattern should be simple and not opinionated.

The proposed design is a library with a ProxyFactory "object" that would be used as in the following snippet.

contract MyFactory {
    using Proxies for Proxies.ProxyFactory;

    // This struct contains the address of the implementation that is
    // used for the proxies it creates.
    struct Proxies.ProxyFactory factory;

    constructor() public {
        // Initializes the factory struct by deploying the implementation
        // and storing its address.
        factory = Proxies.newFactory(FooUpgradeable.creationCode);
    }

    // initializerData is the encoded call to the initializer function.
    function deployInstance(address admin, bytes initializerData) public {
        factory.deploy(admin, initializerData);
    }
}

In this case the factory creates instances of TransparentUpgradeableProxy, but the same pattern could be offered for other kinds of proxies. We should think about this as it will affect naming.

@julianmrodri
Copy link
Contributor

Really cool. Will find some time early next week to give it a shot if you are ok.

@julianmrodri
Copy link
Contributor

@frangio a couple of questions. Would you use create2 for deploying the implementation code? Also should this library emit any events? (maybe when an instance is deployed?)

@frangio
Copy link
Contributor Author

frangio commented Sep 1, 2020

I don't think libraries can emit events so that wouldn't be an option unfortunately.

Using create2 sounds interesting, although I think there would be some extra storage cost to manage the salt and I don't think that cost would be worth it. Do you see any particularly compelling reasons?

@julianmrodri
Copy link
Contributor

julianmrodri commented Sep 1, 2020

I think libraries can emit events, but its true they dont have their own event log, they use the one of the caller contract but at the same time they are not included in the ABI so its not a good way to go.

No, no compelling reason wanted to confirm because is one of the options I considered and in that case i was wondering about the salt.

@abcoathup
Copy link
Contributor

There is a potential need for a MinimalProxyFactory which includes calculation of the address
https://forum.openzeppelin.com/t/how-to-compute-the-create2-address-for-a-minimal-proxy/3595

@frangio
Copy link
Contributor Author

frangio commented Sep 11, 2020

I do think that if the design proposed in this issue works well we should apply it to other kinds of proxies. I'm still undecided on the role of create2! Maybe it makes sense for the library to expose a function that uses create2 but accepts the salt as an argument, without having to keep itself track of the salt.

@julianmrodri
Copy link
Contributor

Submitted a PR with a bare bones implementation to kickoff discussions. I can certainly add the function that internally uses create2 if required. Im not happy at all about importing the Proxy code in the library. Im pretty sure theres a better way.

@0xTimepunk
Copy link

Is there a minimal, safe implementation of the Proxy Factory that could be used?

@frangio
Copy link
Contributor Author

frangio commented Dec 1, 2020

@J-Mars This is not necessarily minimal, but you may be able to adapt the ProxyFactory from OpenZeppelin SDK.

Can you share what are your requirements so we can consider offering something in OpenZeppelin Contracts?

@bonedaddy
Copy link

Is there a minimal, safe implementation of the Proxy Factory that could be used?

You can use the following, which is what I'm currently using

// import TransparentUpgradeableProxy here

/**
 * @dev Proxy Factory library to create multiple proxy instances at runtime
 *
 * @notice i modified this to use a contract instead of a library as it was causing errors
 *
 * These functions implement a simple Factory pattern that can be used to deploy
 * multiple proxy instances when the implementation contract needs to be reused. 
 */
contract Proxies {
  // Contains the address of the implementation used for the proxies it creates.
  struct ProxyFactory { address implementation; }

  /**
   * @dev Returns the ProxyFactory struct with the impementation contract address.
   * 
   * Initializes the factory struct by deploying the implementation contract and
   * storing its address.
  */
  function newFactory(bytes memory creationCode) internal returns (ProxyFactory memory) {
    address implementationAddr = _deployImplementation(creationCode);
    ProxyFactory memory factory = ProxyFactory(implementationAddr);
    return factory;
  }

  /**
   * @dev Deploys a Proxy instance with `admin` and initialized with `initializerData
   *
  */
  function deploy(ProxyFactory storage self,address admin, bytes memory initializerData) internal returns (address) {
    TransparentUpgradeableProxy prox = new TransparentUpgradeableProxy(self.implementation, admin, initializerData);
    return address(prox);
  }

  /**
   * @dev Returns the address of the deployed implementation contract.
   *
   * Creates the implementation contract with `creationCode`.
  */
  function _deployImplementation(bytes memory creationCode) private returns (address) {
    address payable addr;
    // solhint-disable-next-line no-inline-assembly
    assembly {
      addr := create(0, add(creationCode, 0x20), mload(creationCode))
      if iszero(extcodesize(addr)) {
        revert(0, 0)
      }
    }
    return addr;
  }
}

contract Factory is Proxies {

    // This struct contains the address of the implementation that is
    // used for the proxies it creates.
    ProxyFactory factory;

    constructor(bytes memory _creationCode) {
        // Initializes the factory struct by deploying the implementation
        // and storing its address.
        factory = Proxies.newFactory(_creationCode);
    }

    // initializerData is the encoded call to the initializer function.
    function deployInstance(address admin, bytes memory initializerData) public returns (address) {
        return deploy(factory, admin, initializerData);
    }
}

Example usage

contract StructAssignTest {
    struct numbers {
        uint256 a;
        uint256 b;
        uint256 c;
    }

    mapping (uint256 => numbers) public structs;

    function setMany() public returns (bool) {
        structs[block.number].a = 1;
        structs[block.number].b = 2;
        structs[block.number].c = 3;
        return true;
    }

    // cheaper than setMany by about 150 gas
    function setSingle() public returns (bool) {
        structs[block.number] = numbers({
            a: 1,
            b: 2,
            c: 3
        });
    }
}

contract TestProxyFactory is Factory {
    uint256 public numProxies;
    address[] public proxies;
    event ProxyCreated(address _proxy);

    constructor() Factory(type(StructAssignTest).creationCode) {}

    // initializerData is the encoded call to the initializer function.
    function deployProxyContract(address admin, bytes memory initializerData) public returns (address) {
        address prox = deploy(factory, admin, initializerData);
        emit ProxyCreated(prox);
        proxies.push(prox);
        numProxies += 1;
    }
}

@0xTimepunk
Copy link

Thank you both @frangio @bonedaddy

@frangio
Copy link
Contributor Author

frangio commented Dec 2, 2020

@J-Mars Can you still share your requirements so we can consider offering something in the library?

@frangio
Copy link
Contributor Author

frangio commented Dec 14, 2020

I'm no longer a fan of the interface I proposed in this issue, since it requires storing the implementation address in storage (structs can't be immutable). This is fine in some cases but people should use immutable whenever possible so our pattern should encourage that as the first option.

We can revisit this pattern if structs can ever be made immutable.

For a minimal proxy factory see #2437.

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

Successfully merging a pull request may close this issue.

5 participants