feat(smart-contracts): replace revert strings by error codes (#8867)

* replace revert strings by error struct * remove struct logic in mixin errors * make `UnlockErrors` a library * Revert "make `UnlockErrors` a library" This reverts commit f4b330e. * use solc 0.8.4 custom error patterns * small fixes * fix balance check on withdraw * better conditions for `purchase` * specify hook in error * public lock uses solc 0.8.11 * bump to 0.8.13 * fix condition in `transferFrom` * fix conditions and tests in `extend` and `renewMembershipFor` * fix tests and conditions * hooks revert strings in tests * condition if `_maxKeysPerAddress` * fix some more conditions * fix some more conditons and tests * Update smart-contracts/contracts/mixins/MixinErrors.sol Co-authored-by: Julien Genestoux <[email protected]> * Update smart-contracts/contracts/mixins/MixinPurchase.sol Co-authored-by: Julien Genestoux <[email protected]> Co-authored-by: Julien Genestoux <[email protected]>
unlock-protocol · Jun 3, 2022 · 3121df4 · 3121df4
1 parent 92507e1
commit 3121df4
Show file tree

Hide file tree

Showing 18 changed files with 266 additions and 107 deletions.
diff --git a/smart-contracts/contracts/mixins/MixinConvenienceOwnable.sol b/smart-contracts/contracts/mixins/MixinConvenienceOwnable.sol
@@ -2,13 +2,14 @@
 pragma solidity ^0.8.0;
 
 import './MixinLockCore.sol';
+import './MixinErrors.sol';
 
 /**
  * @title Mixin to add support for `ownable()`
  * @dev `Mixins` are a design pattern seen in the 0x contracts.  It simply
  * separates logically groupings of code to ease readability.
  */
-contract MixinConvenienceOwnable is MixinLockCore {
+contract MixinConvenienceOwnable is MixinErrors, MixinLockCore {
 
   // used for `owner()`convenience helper
   address private _convenienceOwner;
@@ -38,7 +39,9 @@ contract MixinConvenienceOwnable is MixinLockCore {
    */ 
   function setOwner(address account) public {
     _onlyLockManager();
-    require(account != address(0), 'OWNER_CANT_BE_ADDRESS_ZERO');
+    if(account == address(0)) {
+      revert OWNER_CANT_BE_ADDRESS_ZERO(); 
+    }
     address _previousOwner = _convenienceOwner;
     _convenienceOwner = account;
     emit OwnershipTransferred(_previousOwner, account);

diff --git a/smart-contracts/contracts/mixins/MixinERC721Enumerable.sol b/smart-contracts/contracts/mixins/MixinERC721Enumerable.sol
@@ -3,7 +3,7 @@ pragma solidity ^0.8.0;
 
 import './MixinKeys.sol';
 import './MixinLockCore.sol';
-// import '@openzeppelin/contracts-upgradeable/token/ERC721/extensions/IERC721EnumerableUpgradeable.sol';
+import './MixinErrors.sol';
 import '@openzeppelin/contracts-upgradeable/utils/introspection/ERC165StorageUpgradeable.sol';
 
 
@@ -12,6 +12,7 @@ import '@openzeppelin/contracts-upgradeable/utils/introspection/ERC165StorageUpg
  */
 contract MixinERC721Enumerable is
   ERC165StorageUpgradeable,
+  MixinErrors,
   MixinLockCore, // Implements totalSupply
   MixinKeys
 {
@@ -37,7 +38,9 @@ contract MixinERC721Enumerable is
   ) public view
     returns (uint256)
   {
-    require(_index < _totalSupply, 'OUT_OF_RANGE');
+    if(_index >= _totalSupply) {
+      revert OUT_OF_RANGE();
+    }
     return _index;
   }
 

diff --git a/smart-contracts/contracts/mixins/MixinErrors.sol b/smart-contracts/contracts/mixins/MixinErrors.sol
@@ -0,0 +1,70 @@
+// SPDX-License-Identifier: MIT
+pragma solidity ^0.8.0;
+
+/**
+ * @title List of all error messages 
+ * (replace string errors message to save on contract size)
+ */
+contract MixinErrors {
+
+  // generic
+  error OUT_OF_RANGE();
+  error NULL_VALUE();
+  error INVALID_ADDRESS();
+  error INVALID_TOKEN();
+  error INVALID_LENGTH();
+  error UNAUTHORIZED();
+
+  // erc 721
+  error NON_COMPLIANT_ERC721_RECEIVER();
+
+  // roles
+  error ONLY_LOCK_MANAGER_OR_KEY_GRANTER();
+  error ONLY_KEY_MANAGER_OR_APPROVED();
+  error UNAUTHORIZED_KEY_MANAGER_UPDATE();
+  error ONLY_LOCK_MANAGER_OR_BENEFICIARY();
+  error ONLY_LOCK_MANAGER();
+
+  // single key status
+  error KEY_NOT_VALID();
+  error NO_SUCH_KEY();
+
+  // single key operations
+  error CANT_EXTEND_NON_EXPIRING_KEY();
+  error NOT_ENOUGH_TIME();
+  error NOT_ENOUGH_FUNDS();
+
+  // migration & data schema
+  error SCHEMA_VERSION_NOT_CORRECT();
+  error MIGRATION_REQUIRED();
+
+  // lock status/settings
+  error OWNER_CANT_BE_ADDRESS_ZERO();
+  error MAX_KEYS_REACHED();
+  error KEY_TRANSFERS_DISABLED();
+  error CANT_BE_SMALLER_THAN_SUPPLY();
+
+  // transfers and approvals
+  error TRANSFER_TO_SELF();
+  error CANNOT_APPROVE_SELF();
+
+  // keys management 
+  error LOCK_SOLD_OUT();
+
+  // purchase
+  error INSUFFICIENT_ERC20_VALUE();
+  error INSUFFICIENT_VALUE();
+
+  // renewals
+  error NON_RENEWABLE_LOCK();
+  error LOCK_HAS_CHANGED();
+  error NOT_READY_FOR_RENEWAL();
+
+  // gas refund
+  error GAS_REFUND_FAILED();
+
+  // hooks
+  // NB: `hookIndex` designed the index of hook address in the params of `setEventHooks`
+  error INVALID_HOOK(uint8 hookIndex);
+
+}
diff --git a/smart-contracts/contracts/mixins/MixinFunds.sol b/smart-contracts/contracts/mixins/MixinFunds.sol
@@ -1,6 +1,7 @@
 // SPDX-License-Identifier: MIT
 pragma solidity ^0.8.0;
 
+import './MixinErrors.sol';
 import '@openzeppelin/contracts-upgradeable/token/ERC20/IERC20Upgradeable.sol';
 import '@openzeppelin/contracts-upgradeable/utils/AddressUpgradeable.sol';
 import '@openzeppelin/contracts-upgradeable/token/ERC20/utils/SafeERC20Upgradeable.sol';
@@ -9,7 +10,7 @@ import '@openzeppelin/contracts-upgradeable/token/ERC20/utils/SafeERC20Upgradeab
  * @title An implementation of the money related functions.
  * @author HardlyDifficult (unlock-protocol.com)
  */
-contract MixinFunds
+contract MixinFunds is MixinErrors
 {
   using AddressUpgradeable for address payable;
   using SafeERC20Upgradeable for IERC20Upgradeable;
@@ -34,10 +35,13 @@ contract MixinFunds
   internal 
   view
   {
-    require(
-      _tokenAddress == address(0) || IERC20Upgradeable(_tokenAddress).totalSupply() > 0,
-      'INVALID_TOKEN'
-    );
+    if(
+      _tokenAddress != address(0) 
+      && 
+      IERC20Upgradeable(_tokenAddress).totalSupply() < 0
+    ) {
+      revert INVALID_TOKEN();
+    }
   }
 
   /**

diff --git a/smart-contracts/contracts/mixins/MixinGrantKeys.sol b/smart-contracts/contracts/mixins/MixinGrantKeys.sol
@@ -3,6 +3,7 @@ pragma solidity ^0.8.0;
 
 import './MixinKeys.sol';
 import './MixinRoles.sol';
+import './MixinErrors.sol';
 
 
 /**
@@ -12,6 +13,7 @@ import './MixinRoles.sol';
  * separates logically groupings of code to ease readability.
  */
 contract MixinGrantKeys is
+  MixinErrors,
   MixinRoles,
   MixinKeys
 {
@@ -25,10 +27,14 @@ contract MixinGrantKeys is
     address[] calldata _keyManagers
   ) external {
     _lockIsUpToDate();
-    require(isKeyGranter(msg.sender) || isLockManager(msg.sender), 'ONLY_LOCK_MANAGER_OR_KEY_GRANTER');
+    if(!isKeyGranter(msg.sender) && !isLockManager(msg.sender)) {
+      revert ONLY_LOCK_MANAGER_OR_KEY_GRANTER();
+    }
 
     for(uint i = 0; i < _recipients.length; i++) {
-      require(_recipients[i] != address(0), 'INVALID_ADDRESS');
+      if(_recipients[i] == address(0)) { 
+        revert INVALID_ADDRESS();
+      }
 
       // an event is triggered
       _createNewKey(

diff --git a/smart-contracts/contracts/mixins/MixinKeys.sol b/smart-contracts/contracts/mixins/MixinKeys.sol
@@ -2,6 +2,7 @@
 pragma solidity ^0.8.0;
 
 import './MixinLockCore.sol';
+import './MixinErrors.sol';
 
 /**
  * @title Mixin for managing `Key` data, as well as the * Approval related functions needed to meet the ERC721
@@ -11,6 +12,7 @@ import './MixinLockCore.sol';
  * separates logically groupings of code to ease readability.
  */
 contract MixinKeys is
+  MixinErrors,
   MixinLockCore
 {
   // The struct for a key
@@ -96,12 +98,13 @@ contract MixinKeys is
   internal
   view
   {
-    require(
-      _isKeyManager(_tokenId, msg.sender) ||
-      approved[_tokenId] == msg.sender ||
-      isApprovedForAll(_ownerOf[_tokenId], msg.sender),
-      'ONLY_KEY_MANAGER_OR_APPROVED'
-    );
+    if(
+      !_isKeyManager(_tokenId, msg.sender)
+      && approved[_tokenId] != msg.sender
+      && !isApprovedForAll(_ownerOf[_tokenId], msg.sender)
+    ) {
+      revert ONLY_KEY_MANAGER_OR_APPROVED();
+    }
   }
 
   /**
@@ -114,10 +117,10 @@ contract MixinKeys is
   internal
   view
   {
-    require(
-      isValidKey(_tokenId),
-      'KEY_NOT_VALID'
-    );
+    if(!isValidKey(_tokenId)) {
+      revert KEY_NOT_VALID();
+    }
+
   }
 
   /**
@@ -130,9 +133,9 @@ contract MixinKeys is
   internal
   view 
   {
-    require(
-      _keys[_tokenId].expirationTimestamp != 0, 'NO_SUCH_KEY'
-    );
+    if(_keys[_tokenId].expirationTimestamp == 0) {
+      revert NO_SUCH_KEY();
+    }
   }
 
   /**
@@ -187,7 +190,9 @@ contract MixinKeys is
     view
     returns (uint256)
   {
-      require(_index < balanceOf(_keyOwner), "OWNER_INDEX_OUT_OF_BOUNDS");
+      if(_index >= balanceOf(_keyOwner)) {
+        revert OUT_OF_RANGE();
+      }
       return _ownedKeyIds[_keyOwner][_index];
   }
 
@@ -239,7 +244,9 @@ contract MixinKeys is
     uint expirationTimestamp = _keys[_tokenId].expirationTimestamp;
 
     // prevent extending a valid non-expiring key
-    require(expirationTimestamp != type(uint).max, 'CANT_EXTEND_NON_EXPIRING_KEY');
+    if(expirationTimestamp == type(uint).max){
+      revert CANT_EXTEND_NON_EXPIRING_KEY();
+    }
 
     // if non-expiring but not valid then extend
     if(expirationDuration == type(uint).max) {
@@ -271,7 +278,9 @@ contract MixinKeys is
     uint length = balanceOf(_recipient);
 
     // make sure address does not have more keys than allowed
-    require(length < _maxKeysPerAddress, 'MAX_KEYS');
+    if(length >= _maxKeysPerAddress) {
+      revert MAX_KEYS_REACHED();
+    }
 
     // record new owner
     _ownedKeysIndex[_tokenId] = length;
@@ -301,7 +310,11 @@ contract MixinKeys is
     _isKey(_tokenIdTo);
 
     // make sure there is enough time remaining
-    require(keyExpirationTimestampFor(_tokenIdFrom) - block.timestamp >= _amount, 'NOT_ENOUGH_TIME');
+    if(
+      _amount > keyExpirationTimestampFor(_tokenIdFrom) - block.timestamp
+    ) {
+      revert NOT_ENOUGH_TIME();
+    }
 
     // deduct time from parent key
     _timeMachine(_tokenIdFrom, _amount, false);
@@ -373,7 +386,9 @@ contract MixinKeys is
     view
     returns (uint)
   {
-    require(_keyOwner != address(0), 'INVALID_ADDRESS');
+    if(_keyOwner == address(0)) { 
+      revert INVALID_ADDRESS();
+    }
     return _balances[_keyOwner];
   }
 
@@ -463,11 +478,12 @@ contract MixinKeys is
   ) public
   {
     _isKey(_tokenId);
-    require(
-      _isKeyManager(_tokenId, msg.sender) ||
-      isLockManager(msg.sender),
-      'UNAUTHORIZED_KEY_MANAGER_UPDATE'
-    );
+    if(
+      !_isKeyManager(_tokenId, msg.sender) 
+      && !isLockManager(msg.sender)
+    ) {
+      revert UNAUTHORIZED_KEY_MANAGER_UPDATE();
+    }
     _setKeyManagerOf(_tokenId, _keyManager);
   }
 
@@ -495,7 +511,9 @@ contract MixinKeys is
     public
   {
     _onlyKeyManagerOrApproved(_tokenId);
-    require(msg.sender != _approved, 'APPROVE_SELF');
+    if(msg.sender == _approved) {
+      revert CANNOT_APPROVE_SELF();
+    }
 
     approved[_tokenId] = _approved;
     emit Approval(_ownerOf[_tokenId], _approved, _tokenId);
@@ -605,7 +623,9 @@ contract MixinKeys is
    */
   function setMaxNumberOfKeys (uint _maxNumberOfKeys) external {
      _onlyLockManager();
-     require (_maxNumberOfKeys >= _totalSupply, "SMALLER_THAN_SUPPLY");
+     if (_maxNumberOfKeys < _totalSupply) {
+       revert CANT_BE_SMALLER_THAN_SUPPLY();
+     }
      maxNumberOfKeys = _maxNumberOfKeys;
   }
 
@@ -627,7 +647,9 @@ contract MixinKeys is
    */
   function setMaxKeysPerAddress(uint _maxKeys) external {
      _onlyLockManager();
-     require(_maxKeys != 0, 'NULL_VALUE');
+     if(_maxKeys == 0) {
+       revert NULL_VALUE();
+     }
      _maxKeysPerAddress = _maxKeys;
   }