Add documentation for proxies (#2344)

.editorconfig

@@ -8,7 +8,7 @@ charset = utf-8
 end_of_line = lf
 indent_style = space
 insert_final_newline = true
-trim_trailing_whitespace = true
+trim_trailing_whitespace = false
 max_line_length = 120
 
 [*.sol]
@@ -16,3 +16,6 @@ indent_size = 4
 
 [*.js]
 indent_size = 2
+
+[*.adoc]
+max_line_length = 0

contracts/proxy/Initializable.sol

@@ -4,16 +4,16 @@ pragma solidity >=0.4.24 <0.7.0;
 
 
 /**
- * @title Initializable
- *
- * @dev Helper contract to support initializer functions. To use it, replace
- * the constructor with a function that has the `initializer` modifier.
- * WARNING: Unlike constructors, initializer functions must be manually
- * invoked. This applies both to deploying an Initializable contract, as well
- * as extending an Initializable contract via inheritance.
- * WARNING: When used with inheritance, manual care must be taken to not invoke
- * a parent initializer twice, or ensure that all initializers are idempotent,
- * because this is not dealt with automatically as with constructors.
+ * @dev This is a base contract to aid in writing upgradeable contracts, or any kind of contract that will be deployed
+ * behind a proxy. Since a proxied contract can't have a constructor, it's common to move constructor logic to an
+ * external initializer function, usually called `initialize`. It then becomes necessary to protect this initializer
+ * function so it can only be called once. The {initializer} modifier provided by this contract will have this effect.
+ * 
+ * TIP: To avoid leaving the proxy in an uninitialized state, the initializer function should be called as early as
+ * possible by providing the encoded function call as the `_data` argument to {UpgradeableProxy-constructor}.
+ * 
+ * CAUTION: When used with inheritance, manual care must be taken to not invoke a parent initializer twice, or to ensure
+ * that all initializers are idempotent. This is not verified automatically as constructors are by Solidity.
  */
 contract Initializable {
 
@@ -28,7 +28,7 @@ contract Initializable {
     bool private _initializing;
 
     /**
-     * @dev Modifier to use in the initializer function of a contract.
+     * @dev Modifier to protect an initializer function from being invoked twice.
      */
     modifier initializer() {
         require(_initializing || _isConstructor() || !_initialized, "Initializable: contract is already initialized");

contracts/proxy/Proxy.sol

@@ -3,39 +3,20 @@
 pragma solidity ^0.6.0;
 
 /**
- * @title Proxy
- * @dev Implements delegation of calls to other contracts, with proper
- * forwarding of return values and bubbling of failures.
- * It defines a fallback function that delegates all calls to the address
- * returned by the abstract _implementation() internal function.
+ * @dev This abstract contract provides a fallback function that delegates all calls to another contract using the EVM
+ * instruction `delegatecall`. We refer to the second contract as the _implementation_ behind the proxy, and it has to
+ * be specified by overriding the virtual {_implementation} function.
+ * 
+ * Additionally, delegation to the implementation can be triggered manually through the {_fallback} function, or to a
+ * different contract through the {_delegate} function.
+ * 
+ * The success and return data of the delegated call will be returned back to the caller of the proxy.
  */
 abstract contract Proxy {
     /**
-     * @dev Fallback function.
-     * Implemented entirely in `_fallback`.
-     */
-    fallback () payable external {
-        _fallback();
-    }
-
-    /**
-     * @dev Receive function.
-     * Implemented entirely in `_fallback`.
-     */
-    receive () payable external {
-        _fallback();
-    }
-
-    /**
-     * @return The Address of the implementation.
-     */
-    function _implementation() internal virtual view returns (address);
-
-    /**
-     * @dev Delegates execution to an implementation contract.
-     * This is a low level function that doesn't return to its internal call site.
-     * It will return to the external caller whatever the implementation returns.
-     * @param implementation Address to delegate.
+     * @dev Delegates the current call to `implementation`.
+     * 
+     * This function does not return to its internall call site, it will return directly to the external caller.
      */
     function _delegate(address implementation) internal {
         // solhint-disable-next-line no-inline-assembly
@@ -60,19 +41,43 @@ abstract contract Proxy {
     }
 
     /**
-     * @dev Function that is run as the first thing in the fallback function.
-     * Can be redefined in derived contracts to add functionality.
-     * Redefinitions must call super._willFallback().
+     * @dev This is a virtual function that should be overriden so it returns the address to which the fallback function
+     * and {_fallback} should delegate.
      */
-    function _willFallback() internal virtual {
-    }
+    function _implementation() internal virtual view returns (address);
 
     /**
-     * @dev fallback implementation.
-     * Extracted to enable manual triggering.
+     * @dev Delegates the current call to the address returned by `_implementation()`.
+     * 
+     * This function does not return to its internall call site, it will return directly to the external caller.
      */
     function _fallback() internal {
         _willFallback();
         _delegate(_implementation());
     }
+
+    /**
+     * @dev Fallback function that delegates calls to the address returned by `_implementation()`. Will run if no other
+     * function in the contract matches the call data.
+     */
+    fallback () payable external {
+        _fallback();
+    }
+
+    /**
+     * @dev Fallback function that delegates calls to the address returned by `_implementation()`. Will run if call data
+     * is empty.
+     */
+    receive () payable external {
+        _fallback();
+    }
+
+    /**
+     * @dev Hook that is called before falling back to the implementation. Can happen as part of a manual `_fallback`
+     * call, or as part of the Solidity `fallback` or `receive` functions.
+     * 
+     * If overriden should call `super._willFallback()`.
+     */
+    function _willFallback() internal virtual {
+    }
 }

contracts/proxy/ProxyAdmin.sol

@@ -6,16 +6,17 @@ import "../access/Ownable.sol";
 import "./TransparentUpgradeableProxy.sol";
 
 /**
- * @title ProxyAdmin
- * @dev This contract is the admin of a proxy, and is in charge
- * of upgrading it as well as transferring it to another admin.
+ * @dev This is an auxiliary contract meant to be assigned as the admin of a {TransparentUpgradeableProxy}. For an
+ * explanation of why you would want to use this see the documentation for {TransparentUpgradeableProxy}.
  */
 contract ProxyAdmin is Ownable {
 
     /**
-     * @dev Returns the current implementation of a proxy.
-     * This is needed because only the proxy admin can query it.
-     * @return The address of the current implementation of the proxy.
+     * @dev Returns the current implementation of `proxy`.
+     * 
+     * Requirements:
+     * 
+     * - This contract must be the admin of `proxy`.
      */
     function getProxyImplementation(TransparentUpgradeableProxy proxy) public view returns (address) {
         // We need to manually run the static call since the getter cannot be flagged as view
@@ -26,8 +27,11 @@ contract ProxyAdmin is Ownable {
     }
 
     /**
-     * @dev Returns the admin of a proxy. Only the admin can query it.
-     * @return The address of the current admin of the proxy.
+     * @dev Returns the current admin of `proxy`.
+     * 
+     * Requirements:
+     * 
+     * - This contract must be the admin of `proxy`.
      */
     function getProxyAdmin(TransparentUpgradeableProxy proxy) public view returns (address) {
         // We need to manually run the static call since the getter cannot be flagged as view
@@ -38,31 +42,34 @@ contract ProxyAdmin is Ownable {
     }
 
     /**
-     * @dev Changes the admin of a proxy.
-     * @param proxy Proxy to change admin.
-     * @param newAdmin Address to transfer proxy administration to.
+     * @dev Changes the admin of `proxy` to `newAdmin`.
+     * 
+     * Requirements:
+     * 
+     * - This contract must be the current admin of `proxy`.
      */
     function changeProxyAdmin(TransparentUpgradeableProxy proxy, address newAdmin) public onlyOwner {
         proxy.changeAdmin(newAdmin);
     }
 
     /**
-     * @dev Upgrades a proxy to the newest implementation of a contract.
-     * @param proxy Proxy to be upgraded.
-     * @param implementation the address of the Implementation.
+     * @dev Upgrades `proxy` to `implementation`. See {TransparentUpgradeableProxy-upgradeTo}.
+     * 
+     * Requirements:
+     * 
+     * - This contract must be the admin of `proxy`.
      */
     function upgrade(TransparentUpgradeableProxy proxy, address implementation) public onlyOwner {
         proxy.upgradeTo(implementation);
     }
 
     /**
-     * @dev Upgrades a proxy to the newest implementation of a contract and forwards a function call to it.
-     * This is useful to initialize the proxied contract.
-     * @param proxy Proxy to be upgraded.
-     * @param implementation Address of the Implementation.
-     * @param data Data to send as msg.data in the low level call.
-     * It should include the signature and the parameters of the function to be called, as described in
-     * https://solidity.readthedocs.io/en/v0.4.24/abi-spec.html#function-selector-and-argument-encoding.
+     * @dev Upgrades `proxy` to `implementation` and calls a function on the new implementation. See
+     * {TransparentUpgradeableProxy-upgradeToAndCall}.
+     * 
+     * Requirements:
+     * 
+     * - This contract must be the admin of `proxy`.
      */
     function upgradeAndCall(TransparentUpgradeableProxy proxy, address implementation, bytes memory data) public payable onlyOwner {
         proxy.upgradeToAndCall{value: msg.value}(implementation, data);

contracts/proxy/README.adoc

@@ -0,0 +1,26 @@
+= Proxies
+
+[.readme-notice]
+NOTE: This document is better viewed at https://docs.openzeppelin.com/contracts/api/proxy
+
+This is a low-level set of contracts implementing the proxy pattern for upgradeability. For an in-depth overview of this pattern check out the xref:upgrades-plugins::proxies.adoc[Proxy Upgrade Pattern] page.
+
+The abstract {Proxy} contract implements the core delegation functionality. If the concrete proxies that we provide below are not suitable, we encourage building on top of this base contract since it contains an assembly block that may be hard to get right.
+
+Upgradeability is implemented in the {UpgradeableProxy} contract, although it provides only an internal upgrade interface. For an upgrade interface exposed externally to an admin, we provide {TransparentUpgradeableProxy}. Both of these contracts use the storage slots specified in https://eips.ethereum.org/EIPS/eip-1967[EIP1967] to avoid clashes with the storage of the implementation contract behind the proxy.
+
+CAUTION: Using upgradeable proxies correctly and securely is a difficult task that requires deep knowledge of the proxy pattern, Solidity, and the EVM. Unless you want a lot of low level control, we recommend using the xref:upgrades-plugins::index.adoc[OpenZeppelin Upgrades Plugins] for Truffle and Buidler.
+
+== Core
+
+{{Proxy}}
+
+{{UpgradeableProxy}}
+
+{{TransparentUpgradeableProxy}}
+
+== Utilities
+
+{{Initializable}}
+
+{{ProxyAdmin}}

contracts/proxy/TransparentUpgradeableProxy.sol

@@ -5,22 +5,30 @@ pragma solidity ^0.6.0;
 import "./UpgradeableProxy.sol";
 
 /**
- * @title TransparentUpgradeableProxy
- * @dev This contract combines an upgradeability proxy with an authorization
- * mechanism for administrative tasks.
- * All external functions in this contract must be guarded by the
- * `ifAdmin` modifier. See ethereum/solidity#3864 for a Solidity
- * feature proposal that would enable this to be done automatically.
+ * @dev This contract implements a proxy that is upgradeable by an admin.
+ * 
+ * To avoid https://medium.com/nomic-labs-blog/malicious-backdoors-in-ethereum-proxies-62629adf3357[proxy selector
+ * clashing], which can potentially be used in an attack, this contract uses the
+ * https://blog.openzeppelin.com/the-transparent-proxy-pattern/[transparent proxy pattern]. This pattern implies two
+ * things that go hand in hand:
+ * 
+ * 1. If any account other than the admin calls the proxy, the call will be forwarded to the implementation, even if
+ * that call matches one of the admin functions exposed by the proxy itself.
+ * 2. If the admin calls the proxy, it can access the admin functions, but its calls will never be forwarded to the
+ * implementation. If the admin tries to call a function on the implementation it will fail with an error that says
+ * "admin cannot fallback to proxy target".
+ * 
+ * These properties mean that the admin account can only be used for admin actions like upgrading the proxy or changing
+ * the admin, so it's best if it's a dedicated account that is not used for anything else. This will avoid headaches due
+ * to sudden errors when trying to call a function from the proxy implementation.
+ * 
+ * Our recommendation is for the dedicated account to be an instance of the {ProxyAdmin} contract. If set up this way,
+ * you should think of the `ProxyAdmin` instance as the real administrative inerface of your proxy.
  */
 contract TransparentUpgradeableProxy is UpgradeableProxy {
     /**
-     * Contract constructor.
-     * @param _logic address of the initial implementation.
-     * @param _admin Address of the proxy administrator.
-     * @param _data Data to send as msg.data to the implementation to initialize the proxied contract.
-     * It should include the signature and the parameters of the function to be called, as described in
-     * https://solidity.readthedocs.io/en/v0.4.24/abi-spec.html#function-selector-and-argument-encoding.
-     * This parameter is optional, if no data is given the initialization call to proxied contract will be skipped.
+     * @dev Initializes an upgradeable proxy managed by `_admin`, backed by the implementation at `_logic`, and
+     * optionally initialized with `_data` as explained in {UpgradeableProxy-constructor}.
      */
     constructor(address _logic, address _admin, bytes memory _data) public payable UpgradeableProxy(_logic, _data) {
         assert(_ADMIN_SLOT == bytes32(uint256(keccak256("eip1967.proxy.admin")) - 1));
@@ -28,9 +36,7 @@ contract TransparentUpgradeableProxy is UpgradeableProxy {
     }
 
     /**
-     * @dev Emitted when the administration has been transferred.
-     * @param previousAdmin Address of the previous admin.
-     * @param newAdmin Address of the new admin.
+     * @dev Emitted when the admin account has changed.
      */
     event AdminChanged(address previousAdmin, address newAdmin);
 
@@ -39,13 +45,10 @@ contract TransparentUpgradeableProxy is UpgradeableProxy {
      * This is the keccak-256 hash of "eip1967.proxy.admin" subtracted by 1, and is
      * validated in the constructor.
      */
-
     bytes32 private constant _ADMIN_SLOT = 0xb53127684a568b3173ae13b9f8a6016e243e63b6e8ee1178d6a717850b5d6103;
 
     /**
-     * @dev Modifier to check whether the `msg.sender` is the admin.
-     * If it is, it will run the function. Otherwise, it will delegate the call
-     * to the implementation.
+     * @dev Modifier used internally that will delegate the call to the implementation unless the sender is the admin.
      */
     modifier ifAdmin() {
         if (msg.sender == _admin()) {
@@ -56,14 +59,26 @@ contract TransparentUpgradeableProxy is UpgradeableProxy {
     }
 
     /**
-     * @return The address of the proxy admin.
+     * @dev Returns the current admin.
+     * 
+     * NOTE: Only the admin can call this function. See {ProxyAdmin-getProxyAdmin}.
+     * 
+     * TIP: To get this value clients can read directly from the storage slot shown below (specified by EIP1967) using the
+     * https://eth.wiki/json-rpc/API#eth_getstorageat[`eth_getStorageAt`] RPC call.
+     * `0xb53127684a568b3173ae13b9f8a6016e243e63b6e8ee1178d6a717850b5d6103`
      */
     function admin() external ifAdmin returns (address) {
         return _admin();
     }
 
     /**
-     * @return The address of the implementation.
+     * @dev Returns the current implementation.
+     * 
+     * NOTE: Only the admin can call this function. See {ProxyAdmin-getProxyImplementation}.
+     * 
+     * TIP: To get this value clients can read directly from the storage slot shown below (specified by EIP1967) using the
+     * https://eth.wiki/json-rpc/API#eth_getstorageat[`eth_getStorageAt`] RPC call.
+     * `0x360894a13ba1a3210667c828492db98dca3e2076cc3735a920a3ca505d382bbc`
      */
     function implementation() external ifAdmin returns (address) {
         return _implementation();
@@ -71,8 +86,10 @@ contract TransparentUpgradeableProxy is UpgradeableProxy {
 
     /**
      * @dev Changes the admin of the proxy.
-     * Only the current admin can call this function.
-     * @param newAdmin Address to transfer proxy administration to.
+     * 
+     * Emits an {AdminChanged} event.
+     * 
+     * NOTE: Only the admin can call this function. See {ProxyAdmin-changeProxyAdmin}.
      */
     function changeAdmin(address newAdmin) external ifAdmin {
         require(newAdmin != address(0), "TransparentUpgradeableProxy: new admin is the zero address");
@@ -81,22 +98,20 @@ contract TransparentUpgradeableProxy is UpgradeableProxy {
     }
 
     /**
-     * @dev Upgrade the backing implementation of the proxy.
-     * Only the admin can call this function.
-     * @param newImplementation Address of the new implementation.
+     * @dev Upgrade the implementation of the proxy.
+     * 
+     * NOTE: Only the admin can call this function. See {ProxyAdmin-upgrade}.
      */
     function upgradeTo(address newImplementation) external ifAdmin {
         _upgradeTo(newImplementation);
     }
 
     /**
-     * @dev Upgrade the backing implementation of the proxy and call a function
-     * on the new implementation.
-     * This is useful to initialize the proxied contract.
-     * @param newImplementation Address of the new implementation.
-     * @param data Data to send as msg.data in the low level call.
-     * It should include the signature and the parameters of the function to be called, as described in
-     * https://solidity.readthedocs.io/en/v0.4.24/abi-spec.html#function-selector-and-argument-encoding.
+     * @dev Upgrade the implementation of the proxy, and then call a function from the new implementation as specified
+     * by `data`, which should be an encoded function call. This is useful to initialize new storage variables in the
+     * proxied contract.
+     * 
+     * NOTE: Only the admin can call this function. See {ProxyAdmin-upgradeAndCall}.
      */
     function upgradeToAndCall(address newImplementation, bytes calldata data) external payable ifAdmin {
         _upgradeTo(newImplementation);
@@ -106,7 +121,7 @@ contract TransparentUpgradeableProxy is UpgradeableProxy {
     }
 
     /**
-     * @return adm The admin slot.
+     * @dev Returns the current admin.
      */
     function _admin() internal view returns (address adm) {
         bytes32 slot = _ADMIN_SLOT;
@@ -117,8 +132,7 @@ contract TransparentUpgradeableProxy is UpgradeableProxy {
     }
 
     /**
-     * @dev Sets the address of the proxy admin.
-     * @param newAdmin Address of the new proxy admin.
+     * @dev Stores a new address in the EIP1967 admin slot.
      */
     function _setAdmin(address newAdmin) internal {
         bytes32 slot = _ADMIN_SLOT;
@@ -130,7 +144,7 @@ contract TransparentUpgradeableProxy is UpgradeableProxy {
     }
 
     /**
-     * @dev Only fallback when the sender is not the admin.
+     * @dev Makes sure the admin cannot access the fallback function. See {Proxy-_willFallback}.
      */
     function _willFallback() internal override virtual {
         require(msg.sender != _admin(), "TransparentUpgradeableProxy: admin cannot fallback to proxy target");

contracts/proxy/UpgradeableProxy.sol

@@ -6,19 +6,20 @@ import "./Proxy.sol";
 import "../utils/Address.sol";
 
 /**
- * @title UpgradeableProxy
- * @dev This contract implements a proxy that allows to change the
- * implementation address to which it will delegate.
- * Such a change is called an implementation upgrade.
+ * @dev This contract implements an upgradeable proxy. It is upgradeable because calls are delegated to an
+ * implementation address that can be changed. This address is stored in storage in the location specified by
+ * https://eips.ethereum.org/EIPS/eip-1967[EIP1967], so that it doesn't conflict with the storage layout of the
+ * implementation behind the proxy.
+ * 
+ * Upgradeability is only provided internally through {_upgradeTo}. For an externally upgradeable proxy see
+ * {TransparentUpgradeableProxy}.
  */
 contract UpgradeableProxy is Proxy {
     /**
-     * @dev Contract constructor.
-     * @param _logic Address of the initial implementation.
-     * @param _data Data to send as msg.data to the implementation to initialize the proxied contract.
-     * It should include the signature and the parameters of the function to be called, as described in
-     * https://solidity.readthedocs.io/en/v0.4.24/abi-spec.html#function-selector-and-argument-encoding.
-     * This parameter is optional, if no data is given the initialization call to proxied contract will be skipped.
+     * @dev Initializes the upgradeable proxy with an initial implementation specified by `_logic`.
+     * 
+     * If `_data` is nonempty, it's used as data in a delegate call to `_logic`. This will typically be an encoded
+     * function call, and allows initializating the storage of the proxy like a Solidity constructor.
      */
     constructor(address _logic, bytes memory _data) public payable {
         assert(_IMPLEMENTATION_SLOT == bytes32(uint256(keccak256("eip1967.proxy.implementation")) - 1));
@@ -28,11 +29,10 @@ contract UpgradeableProxy is Proxy {
             (bool success,) = _logic.delegatecall(_data);
             require(success);
         }
-    }  
+    }
 
     /**
      * @dev Emitted when the implementation is upgraded.
-     * @param implementation Address of the new implementation.
      */
     event Upgraded(address indexed implementation);
 
@@ -44,8 +44,7 @@ contract UpgradeableProxy is Proxy {
     bytes32 private constant _IMPLEMENTATION_SLOT = 0x360894a13ba1a3210667c828492db98dca3e2076cc3735a920a3ca505d382bbc;
 
     /**
-     * @dev Returns the current implementation.
-     * @return impl Address of the current implementation
+     * @dev Returns the current implementation address.
      */
     function _implementation() internal override view returns (address impl) {
         bytes32 slot = _IMPLEMENTATION_SLOT;
@@ -57,7 +56,8 @@ contract UpgradeableProxy is Proxy {
 
     /**
      * @dev Upgrades the proxy to a new implementation.
-     * @param newImplementation Address of the new implementation.
+     * 
+     * Emits an {Upgraded} event.
      */
     function _upgradeTo(address newImplementation) internal {
         _setImplementation(newImplementation);
@@ -65,8 +65,7 @@ contract UpgradeableProxy is Proxy {
     }
 
     /**
-     * @dev Sets the implementation address of the proxy.
-     * @param newImplementation Address of the new implementation.
+     * @dev Stores a new address in the EIP1967 implementation slot.
      */
     function _setImplementation(address newImplementation) internal {
         require(Address.isContract(newImplementation), "UpgradeableProxy: new implementation is not a contract");