Make view and pure functions virtual

CHANGELOG.md

@@ -15,6 +15,7 @@
  * `EnumerableMap`: fix a memory allocation issue by adding new `EnumerableMap.tryGet(uint)→(bool,address)` functions. `EnumerableMap.get(uint)→string` is now deprecated. ([#2462](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2462))
  * `ERC165Checker`: added batch `getSupportedInterfaces`. ([#2469](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2469))
  * `RefundEscrow`: `beneficiaryWithdraw` will forward all available gas to the beneficiary. ([#2480](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2480))
+ * Many view and pure functions have been made virtual to customize them via overrides. In many cases this will not imply that other functions in the contract will automatically adapt to the overridden definitions. People who wish to override should consult the source code to understand the impact and if they need to override any additional functions to achieve the desired behavior.
 
 ### Security Fixes
 

contracts/GSN/GSNRecipient.sol

@@ -35,7 +35,7 @@ abstract contract GSNRecipient is IRelayRecipient, Context {
     /**
      * @dev Returns the address of the {IRelayHub} contract for this recipient.
      */
-    function getHubAddr() public view override returns (address) {
+    function getHubAddr() public view virtual override returns (address) {
         return _relayHub;
     }
 
@@ -62,7 +62,7 @@ abstract contract GSNRecipient is IRelayRecipient, Context {
      */
     // This function is view for future-proofing, it may require reading from
     // storage in the future.
-    function relayHubVersion() public view returns (string memory) {
+    function relayHubVersion() public view virtual returns (string memory) {
         this; // silence state mutability warning without generating bytecode - see https://github.com/ethereum/solidity/issues/2691
         return "1.0.0";
     }
@@ -73,7 +73,7 @@ abstract contract GSNRecipient is IRelayRecipient, Context {
      * Derived contracts should expose this in an external interface with proper access control.
      */
     function _withdrawDeposits(uint256 amount, address payable payee) internal virtual {
-        IRelayHub(_relayHub).withdraw(amount, payee);
+        IRelayHub(getHubAddr()).withdraw(amount, payee);
     }
 
     // Overrides for Context's functions: when called from RelayHub, sender and
@@ -88,7 +88,7 @@ abstract contract GSNRecipient is IRelayRecipient, Context {
      * IMPORTANT: Contracts derived from {GSNRecipient} should never use `msg.sender`, and use {_msgSender} instead.
      */
     function _msgSender() internal view virtual override returns (address payable) {
-        if (msg.sender != _relayHub) {
+        if (msg.sender != getHubAddr()) {
             return msg.sender;
         } else {
             return _getRelayedCallSender();
@@ -102,7 +102,7 @@ abstract contract GSNRecipient is IRelayRecipient, Context {
      * IMPORTANT: Contracts derived from {GSNRecipient} should never use `msg.data`, and use {_msgData} instead.
      */
     function _msgData() internal view virtual override returns (bytes memory) {
-        if (msg.sender != _relayHub) {
+        if (msg.sender != getHubAddr()) {
             return msg.data;
         } else {
             return _getRelayedCallData();
@@ -162,7 +162,7 @@ abstract contract GSNRecipient is IRelayRecipient, Context {
      * @dev Return this in acceptRelayedCall to proceed with the execution of a relayed call. Note that this contract
      * will be charged a fee by RelayHub
      */
-    function _approveRelayedCall() internal pure returns (uint256, bytes memory) {
+    function _approveRelayedCall() internal pure virtual returns (uint256, bytes memory) {
         return _approveRelayedCall("");
     }
 
@@ -171,22 +171,22 @@ abstract contract GSNRecipient is IRelayRecipient, Context {
      *
      * This overload forwards `context` to _preRelayedCall and _postRelayedCall.
      */
-    function _approveRelayedCall(bytes memory context) internal pure returns (uint256, bytes memory) {
+    function _approveRelayedCall(bytes memory context) internal pure virtual returns (uint256, bytes memory) {
         return (_RELAYED_CALL_ACCEPTED, context);
     }
 
     /**
      * @dev Return this in acceptRelayedCall to impede execution of a relayed call. No fees will be charged.
      */
-    function _rejectRelayedCall(uint256 errorCode) internal pure returns (uint256, bytes memory) {
+    function _rejectRelayedCall(uint256 errorCode) internal pure virtual returns (uint256, bytes memory) {
         return (_RELAYED_CALL_REJECTED + errorCode, "");
     }
 
     /*
      * @dev Calculates how much RelayHub will charge a recipient for using `gas` at a `gasPrice`, given a relayer's
      * `serviceFee`.
      */
-    function _computeCharge(uint256 gas, uint256 gasPrice, uint256 serviceFee) internal pure returns (uint256) {
+    function _computeCharge(uint256 gas, uint256 gasPrice, uint256 serviceFee) internal pure virtual returns (uint256) {
         // The fee is expressed as a percentage. E.g. a value of 40 stands for a 40% fee, so the recipient will be
         // charged for 1.4 times the spent amount.
         return (gas * gasPrice * (100 + serviceFee)) / 100;

contracts/GSN/GSNRecipientERC20Fee.sol

@@ -37,15 +37,15 @@ contract GSNRecipientERC20Fee is GSNRecipient {
     /**
      * @dev Returns the gas payment token.
      */
-    function token() public view returns (IERC20) {
-        return IERC20(_token);
+    function token() public view virtual returns (__unstable__ERC20Owned) {
+        return _token;
     }
 
     /**
      * @dev Internal function that mints the gas payment token. Derived contracts should expose this function in their public API, with proper access control mechanisms.
      */
     function _mint(address account, uint256 amount) internal virtual {
-        _token.mint(account, amount);
+        token().mint(account, amount);
     }
 
     /**
@@ -68,7 +68,7 @@ contract GSNRecipientERC20Fee is GSNRecipient {
         override
         returns (uint256, bytes memory)
     {
-        if (_token.balanceOf(from) < maxPossibleCharge) {
+        if (token().balanceOf(from) < maxPossibleCharge) {
             return _rejectRelayedCall(uint256(GSNRecipientERC20FeeErrorCodes.INSUFFICIENT_BALANCE));
         }
 
@@ -85,7 +85,7 @@ contract GSNRecipientERC20Fee is GSNRecipient {
         (address from, uint256 maxPossibleCharge) = abi.decode(context, (address, uint256));
 
         // The maximum token charge is pre-charged from the user
-        _token.safeTransferFrom(from, address(this), maxPossibleCharge);
+        token().safeTransferFrom(from, address(this), maxPossibleCharge);
 
         return 0;
     }
@@ -104,7 +104,7 @@ contract GSNRecipientERC20Fee is GSNRecipient {
         actualCharge = actualCharge.sub(overestimation);
 
         // After the relayed call has been executed and the actual charge estimated, the excess pre-charge is returned
-        _token.safeTransfer(from, maxPossibleCharge.sub(actualCharge));
+        token().safeTransfer(from, maxPossibleCharge.sub(actualCharge));
     }
 }
 
@@ -121,12 +121,12 @@ contract __unstable__ERC20Owned is ERC20, Ownable {
     constructor(string memory name, string memory symbol) public ERC20(name, symbol) { }
 
     // The owner (GSNRecipientERC20Fee) can mint tokens
-    function mint(address account, uint256 amount) public onlyOwner {
+    function mint(address account, uint256 amount) public virtual onlyOwner {
         _mint(account, amount);
     }
 
     // The owner has 'infinite' allowance for all token holders
-    function allowance(address tokenOwner, address spender) public view override returns (uint256) {
+    function allowance(address tokenOwner, address spender) public view virtual override returns (uint256) {
         if (spender == owner()) {
             return _UINT256_MAX;
         } else {
@@ -135,15 +135,15 @@ contract __unstable__ERC20Owned is ERC20, Ownable {
     }
 
     // Allowance for the owner cannot be changed (it is always 'infinite')
-    function _approve(address tokenOwner, address spender, uint256 value) internal override {
+    function _approve(address tokenOwner, address spender, uint256 value) internal virtual override {
         if (spender == owner()) {
             return;
         } else {
             super._approve(tokenOwner, spender, value);
         }
     }
 
-    function transferFrom(address sender, address recipient, uint256 amount) public override returns (bool) {
+    function transferFrom(address sender, address recipient, uint256 amount) public virtual override returns (bool) {
         if (recipient == owner()) {
             _transfer(sender, recipient, amount);
             return true;

contracts/access/Ownable.sol

@@ -32,15 +32,15 @@ abstract contract Ownable is Context {
     /**
      * @dev Returns the address of the current owner.
      */
-    function owner() public view returns (address) {
+    function owner() public view virtual returns (address) {
         return _owner;
     }
 
     /**
      * @dev Throws if called by any account other than the owner.
      */
     modifier onlyOwner() {
-        require(_owner == _msgSender(), "Ownable: caller is not the owner");
+        require(owner() == _msgSender(), "Ownable: caller is not the owner");
         _;
     }
 

contracts/access/TimelockController.sol

@@ -93,33 +93,42 @@ contract TimelockController is AccessControl {
      */
     receive() external payable {}
 
+    /**
+     * @dev Returns whether an id correspond to a registered operation. This
+     * includes both Pending, Ready and Done operations.
+     */
+    function isOperation(bytes32 id) public view virtual returns (bool pending) {
+        return getTimestamp(id) > 0;
+    }
+
     /**
      * @dev Returns whether an operation is pending or not.
      */
-    function isOperationPending(bytes32 id) public view returns (bool pending) {
-        return _timestamps[id] > _DONE_TIMESTAMP;
+    function isOperationPending(bytes32 id) public view virtual returns (bool pending) {
+        return getTimestamp(id) > _DONE_TIMESTAMP;
     }
 
     /**
      * @dev Returns whether an operation is ready or not.
      */
-    function isOperationReady(bytes32 id) public view returns (bool ready) {
+    function isOperationReady(bytes32 id) public view virtual returns (bool ready) {
+        uint256 timestamp = getTimestamp(id);
         // solhint-disable-next-line not-rely-on-time
-        return _timestamps[id] > _DONE_TIMESTAMP && _timestamps[id] <= block.timestamp;
+        return timestamp > _DONE_TIMESTAMP && timestamp <= block.timestamp;
     }
 
     /**
      * @dev Returns whether an operation is done or not.
      */
-    function isOperationDone(bytes32 id) public view returns (bool done) {
-        return _timestamps[id] == _DONE_TIMESTAMP;
+    function isOperationDone(bytes32 id) public view virtual returns (bool done) {
+        return getTimestamp(id) == _DONE_TIMESTAMP;
     }
 
     /**
      * @dev Returns the timestamp at with an operation becomes ready (0 for
      * unset operations, 1 for done operations).
      */
-    function getTimestamp(bytes32 id) public view returns (uint256 timestamp) {
+    function getTimestamp(bytes32 id) public view virtual returns (uint256 timestamp) {
         return _timestamps[id];
     }
 
@@ -128,23 +137,23 @@ contract TimelockController is AccessControl {
      *
      * This value can be changed by executing an operation that calls `updateDelay`.
      */
-    function getMinDelay() public view returns (uint256 duration) {
+    function getMinDelay() public view virtual returns (uint256 duration) {
         return _minDelay;
     }
 
     /**
      * @dev Returns the identifier of an operation containing a single
      * transaction.
      */
-    function hashOperation(address target, uint256 value, bytes calldata data, bytes32 predecessor, bytes32 salt) public pure returns (bytes32 hash) {
+    function hashOperation(address target, uint256 value, bytes calldata data, bytes32 predecessor, bytes32 salt) public pure virtual returns (bytes32 hash) {
         return keccak256(abi.encode(target, value, data, predecessor, salt));
     }
 
     /**
      * @dev Returns the identifier of an operation containing a batch of
      * transactions.
      */
-    function hashOperationBatch(address[] calldata targets, uint256[] calldata values, bytes[] calldata datas, bytes32 predecessor, bytes32 salt) public pure returns (bytes32 hash) {
+    function hashOperationBatch(address[] calldata targets, uint256[] calldata values, bytes[] calldata datas, bytes32 predecessor, bytes32 salt) public pure virtual returns (bytes32 hash) {
         return keccak256(abi.encode(targets, values, datas, predecessor, salt));
     }
 
@@ -187,8 +196,8 @@ contract TimelockController is AccessControl {
      * @dev Schedule an operation that is to becomes valid after a given delay.
      */
     function _schedule(bytes32 id, uint256 delay) private {
-        require(_timestamps[id] == 0, "TimelockController: operation already scheduled");
-        require(delay >= _minDelay, "TimelockController: insufficient delay");
+        require(!isOperation(id), "TimelockController: operation already scheduled");
+        require(delay >= getMinDelay(), "TimelockController: insufficient delay");
         // solhint-disable-next-line not-rely-on-time
         _timestamps[id] = SafeMath.add(block.timestamp, delay);
     }

contracts/drafts/EIP712.sol

@@ -46,7 +46,7 @@ abstract contract EIP712 {
     constructor(string memory name, string memory version) internal {
         bytes32 hashedName = keccak256(bytes(name));
         bytes32 hashedVersion = keccak256(bytes(version));
-        bytes32 typeHash = keccak256("EIP712Domain(string name,string version,uint256 chainId,address verifyingContract)"); 
+        bytes32 typeHash = keccak256("EIP712Domain(string name,string version,uint256 chainId,address verifyingContract)");
         _HASHED_NAME = hashedName;
         _HASHED_VERSION = hashedVersion;
         _CACHED_CHAIN_ID = _getChainId();
@@ -57,7 +57,7 @@ abstract contract EIP712 {
     /**
      * @dev Returns the domain separator for the current chain.
      */
-    function _domainSeparatorV4() internal view returns (bytes32) {
+    function _domainSeparatorV4() internal view virtual returns (bytes32) {
         if (_getChainId() == _CACHED_CHAIN_ID) {
             return _CACHED_DOMAIN_SEPARATOR;
         } else {
@@ -92,7 +92,7 @@ abstract contract EIP712 {
      * address signer = ECDSA.recover(digest, signature);
      * ```
      */
-    function _hashTypedDataV4(bytes32 structHash) internal view returns (bytes32) {
+    function _hashTypedDataV4(bytes32 structHash) internal view virtual returns (bytes32) {
         return keccak256(abi.encodePacked("\x19\x01", _domainSeparatorV4(), structHash));
     }
 

contracts/introspection/ERC165.sol

@@ -32,7 +32,7 @@ abstract contract ERC165 is IERC165 {
      *
      * Time complexity O(1), guaranteed to always use less than 30 000 gas.
      */
-    function supportsInterface(bytes4 interfaceId) public view override returns (bool) {
+    function supportsInterface(bytes4 interfaceId) public view virtual override returns (bool) {
         return _supportedInterfaces[interfaceId];
     }
 

contracts/introspection/ERC1820Implementer.sol

@@ -20,7 +20,7 @@ contract ERC1820Implementer is IERC1820Implementer {
     /**
      * See {IERC1820Implementer-canImplementInterfaceForAddress}.
      */
-    function canImplementInterfaceForAddress(bytes32 interfaceHash, address account) public view override returns (bytes32) {
+    function canImplementInterfaceForAddress(bytes32 interfaceHash, address account) public view virtual override returns (bytes32) {
         return _supportedInterfaces[interfaceHash][account] ? _ERC1820_ACCEPT_MAGIC : bytes32(0x00);
     }
 

contracts/payment/escrow/RefundEscrow.sol

@@ -36,14 +36,14 @@ contract RefundEscrow is ConditionalEscrow {
     /**
      * @return The current state of the escrow.
      */
-    function state() public view returns (State) {
+    function state() public view virtual returns (State) {
         return _state;
     }
 
     /**
      * @return The beneficiary of the escrow.
      */
-    function beneficiary() public view returns (address) {
+    function beneficiary() public view virtual returns (address payable) {
         return _beneficiary;
     }
 
@@ -52,16 +52,16 @@ contract RefundEscrow is ConditionalEscrow {
      * @param refundee The address funds will be sent to if a refund occurs.
      */
     function deposit(address refundee) public payable virtual override {
-        require(_state == State.Active, "RefundEscrow: can only deposit while active");
+        require(state() == State.Active, "RefundEscrow: can only deposit while active");
         super.deposit(refundee);
     }
 
     /**
      * @dev Allows for the beneficiary to withdraw their funds, rejecting
      * further deposits.
      */
-    function close() public onlyOwner virtual {
-        require(_state == State.Active, "RefundEscrow: can only close while active");
+    function close() public virtual onlyOwner {
+        require(state() == State.Active, "RefundEscrow: can only close while active");
         _state = State.Closed;
         emit RefundsClosed();
     }
@@ -70,7 +70,7 @@ contract RefundEscrow is ConditionalEscrow {
      * @dev Allows for refunds to take place, rejecting further deposits.
      */
     function enableRefunds() public onlyOwner virtual {
-        require(_state == State.Active, "RefundEscrow: can only enable refunds while active");
+        require(state() == State.Active, "RefundEscrow: can only enable refunds while active");
         _state = State.Refunding;
         emit RefundsEnabled();
     }
@@ -79,15 +79,15 @@ contract RefundEscrow is ConditionalEscrow {
      * @dev Withdraws the beneficiary's funds.
      */
     function beneficiaryWithdraw() public virtual {
-        require(_state == State.Closed, "RefundEscrow: beneficiary can only withdraw while closed");
-        _beneficiary.sendValue(address(this).balance);
+        require(state() == State.Closed, "RefundEscrow: beneficiary can only withdraw while closed");
+        beneficiary().sendValue(address(this).balance);
     }
 
     /**
      * @dev Returns whether refundees can withdraw their deposits (be refunded). The overridden function receives a
      * 'payee' argument, but we ignore it here since the condition is global, not per-payee.
      */
     function withdrawalAllowed(address) public view override returns (bool) {
-        return _state == State.Refunding;
+        return state() == State.Refunding;
     }
 }