Merge branch 'master' into lib-834-use-governor-address-in-id-hash

ernestognw · ernestognw · commit bcb0b840c34e · 2023-07-12T09:41:11.000-06:00
diff --git a/.changeset/afraid-walls-smell.md b/.changeset/afraid-walls-smell.md
@@ -0,0 +1,5 @@
+---
+'openzeppelin-solidity': major
+---
+
+`ERC1155Receiver`: Removed in favor of `ERC1155Holder`.
diff --git a/.solhint.json b/.solhint.json
diff --git a/contracts/access/AccessControl.sol b/contracts/access/AccessControl.sol
@@ -5,7 +5,6 @@ pragma solidity ^0.8.19;
 
 import {IAccessControl} from "./IAccessControl.sol";
 import {Context} from "../utils/Context.sol";
-import {Strings} from "../utils/Strings.sol";
 import {ERC165} from "../utils/introspection/ERC165.sol";
 
 /**
diff --git a/contracts/governance/TimelockController.sol b/contracts/governance/TimelockController.sol
@@ -6,7 +6,6 @@ pragma solidity ^0.8.19;
 import {AccessControl} from "../access/AccessControl.sol";
 import {ERC721Holder} from "../token/ERC721/utils/ERC721Holder.sol";
 import {ERC1155Holder} from "../token/ERC1155/utils/ERC1155Holder.sol";
-import {ERC1155Receiver} from "../token/ERC1155/utils/ERC1155Receiver.sol";
 import {Address} from "../utils/Address.sol";
 
 /**
@@ -160,7 +159,7 @@ contract TimelockController is AccessControl, ERC721Holder, ERC1155Holder {
      */
     function supportsInterface(
         bytes4 interfaceId
-    ) public view virtual override(AccessControl, ERC1155Receiver) returns (bool) {
+    ) public view virtual override(AccessControl, ERC1155Holder) returns (bool) {
         return super.supportsInterface(interfaceId);
     }
 
diff --git a/contracts/governance/extensions/GovernorTimelockCompound.sol b/contracts/governance/extensions/GovernorTimelockCompound.sol
@@ -5,7 +5,6 @@ pragma solidity ^0.8.19;
 
 import {IGovernorTimelock} from "./IGovernorTimelock.sol";
 import {IGovernor, Governor} from "../Governor.sol";
-import {SafeCast} from "../../utils/math/SafeCast.sol";
 import {ICompoundTimelock} from "../../vendor/compound/ICompoundTimelock.sol";
 import {IERC165} from "../../interfaces/IERC165.sol";
 import {Address} from "../../utils/Address.sol";
diff --git a/contracts/mocks/token/ERC20PermitNoRevertMock.sol b/contracts/mocks/token/ERC20PermitNoRevertMock.sol
@@ -2,7 +2,6 @@
 
 pragma solidity ^0.8.19;
 
-import {ERC20} from "../../token/ERC20/ERC20.sol";
 import {ERC20Permit} from "../../token/ERC20/extensions/ERC20Permit.sol";
 
 abstract contract ERC20PermitNoRevertMock is ERC20Permit {
diff --git a/contracts/proxy/transparent/ProxyAdmin.sol b/contracts/proxy/transparent/ProxyAdmin.sol
@@ -3,7 +3,7 @@
 
 pragma solidity ^0.8.19;
 
-import {ITransparentUpgradeableProxy, TransparentUpgradeableProxy} from "./TransparentUpgradeableProxy.sol";
+import {ITransparentUpgradeableProxy} from "./TransparentUpgradeableProxy.sol";
 import {Ownable} from "../../access/Ownable.sol";
 
 /**
diff --git a/contracts/proxy/utils/Initializable.sol b/contracts/proxy/utils/Initializable.sol
@@ -3,8 +3,6 @@
 
 pragma solidity ^0.8.19;
 
-import {Address} from "../../utils/Address.sol";
-
 /**
  * @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 proxied contracts do not make use of a constructor, it's common to move constructor logic to an
diff --git a/contracts/proxy/utils/UUPSUpgradeable.sol b/contracts/proxy/utils/UUPSUpgradeable.sol
@@ -17,7 +17,7 @@ import {ERC1967Utils} from "../ERC1967/ERC1967Utils.sol";
  * The {_authorizeUpgrade} function must be overridden to include access restriction to the upgrade mechanism.
  */
 abstract contract UUPSUpgradeable is IERC1822Proxiable {
-    /// @custom:oz-upgrades-unsafe-allow state-variable-immutable state-variable-assignment
+    /// @custom:oz-upgrades-unsafe-allow state-variable-immutable
     address private immutable __self = address(this);
 
     /**
diff --git a/contracts/token/ERC1155/README.adoc b/contracts/token/ERC1155/README.adoc
@@ -26,8 +26,6 @@ NOTE: This core set of contracts is designed to be unopinionated, allowing devel
 
 {{IERC1155Receiver}}
 
-{{ERC1155Receiver}}
-
 == Extensions
 
 {{ERC1155Pausable}}
diff --git a/contracts/token/ERC1155/utils/ERC1155Holder.sol b/contracts/token/ERC1155/utils/ERC1155Holder.sol
@@ -3,15 +3,23 @@
 
 pragma solidity ^0.8.19;
 
-import {ERC1155Receiver} from "./ERC1155Receiver.sol";
+import {IERC165, ERC165} from "../../../utils/introspection/ERC165.sol";
+import {IERC1155Receiver} from "../IERC1155Receiver.sol";
 
 /**
- * @dev Simple implementation of `ERC1155Receiver` that will allow a contract to hold ERC1155 tokens.
+ * @dev Simple implementation of `IERC1155Receiver` that will allow a contract to hold ERC1155 tokens.
  *
  * IMPORTANT: When inheriting this contract, you must include a way to use the received tokens, otherwise they will be
  * stuck.
  */
-abstract contract ERC1155Holder is ERC1155Receiver {
+abstract contract ERC1155Holder is ERC165, IERC1155Receiver {
+    /**
+     * @dev See {IERC165-supportsInterface}.
+     */
+    function supportsInterface(bytes4 interfaceId) public view virtual override(ERC165, IERC165) returns (bool) {
+        return interfaceId == type(IERC1155Receiver).interfaceId || super.supportsInterface(interfaceId);
+    }
+
     function onERC1155Received(
         address,
         address,
diff --git a/contracts/token/ERC1155/utils/ERC1155Receiver.sol b/contracts/token/ERC1155/utils/ERC1155Receiver.sol
diff --git a/contracts/token/ERC20/extensions/ERC20Votes.sol b/contracts/token/ERC20/extensions/ERC20Votes.sol
@@ -5,7 +5,6 @@ pragma solidity ^0.8.19;
 
 import {ERC20} from "../ERC20.sol";
 import {Votes} from "../../../governance/utils/Votes.sol";
-import {SafeCast} from "../../../utils/math/SafeCast.sol";
 import {Checkpoints} from "../../../utils/structs/Checkpoints.sol";
 
 /**
diff --git a/contracts/token/ERC721/extensions/ERC721Royalty.sol b/contracts/token/ERC721/extensions/ERC721Royalty.sol
@@ -5,7 +5,6 @@ pragma solidity ^0.8.19;
 
 import {ERC721} from "../ERC721.sol";
 import {ERC2981} from "../../common/ERC2981.sol";
-import {ERC165} from "../../../utils/introspection/ERC165.sol";
 
 /**
  * @dev Extension of ERC721 with the ERC2981 NFT Royalty Standard, a standardized way to retrieve royalty payment
diff --git a/contracts/utils/cryptography/EIP712.sol b/contracts/utils/cryptography/EIP712.sol
@@ -29,7 +29,7 @@ import {IERC5267} from "../../interfaces/IERC5267.sol";
  * separator of the implementation contract. This will cause the {_domainSeparatorV4} function to always rebuild the
  * separator from the immutable values, which is cheaper than accessing a cached version in cold storage.
  *
- * @custom:oz-upgrades-unsafe-allow state-variable-immutable state-variable-assignment
+ * @custom:oz-upgrades-unsafe-allow state-variable-immutable
  */
 abstract contract EIP712 is IERC5267 {
     using ShortStrings for *;
diff --git a/package-lock.json b/package-lock.json
diff --git a/package.json b/package.json
@@ -84,6 +84,7 @@
     "rimraf": "^3.0.2",
     "semver": "^7.3.5",
     "solhint": "^3.3.6",
+    "solhint-plugin-openzeppelin": "file:scripts/solhint-custom",
     "solidity-ast": "^0.4.25",
     "solidity-coverage": "^0.8.0",
     "solidity-docgen": "^0.6.0-beta.29",
diff --git a/scripts/solhint-custom/index.js b/scripts/solhint-custom/index.js
@@ -0,0 +1,84 @@
+const path = require('path');
+const minimatch = require('minimatch');
+
+// Files matching these patterns will be ignored unless a rule has `static global = true`
+const ignore = ['contracts/mocks/**/*', 'test/**/*'];
+
+class Base {
+  constructor(reporter, config, source, fileName) {
+    this.reporter = reporter;
+    this.ignored = this.constructor.global || ignore.some(p => minimatch(path.normalize(fileName), p));
+    this.ruleId = this.constructor.ruleId;
+    if (this.ruleId === undefined) {
+      throw Error('missing ruleId static property');
+    }
+  }
+
+  error(node, message) {
+    if (!this.ignored) {
+      this.reporter.error(node, this.ruleId, message);
+    }
+  }
+}
+
+module.exports = [
+  class extends Base {
+    static ruleId = 'interface-names';
+
+    ContractDefinition(node) {
+      if (node.kind === 'interface' && !/^I[A-Z]/.test(node.name)) {
+        this.error(node, 'Interface names should have a capital I prefix');
+      }
+    }
+  },
+
+  class extends Base {
+    static ruleId = 'private-variables';
+
+    VariableDeclaration(node) {
+      const constantOrImmutable = node.isDeclaredConst || node.isImmutable;
+      if (node.isStateVar && !constantOrImmutable && node.visibility !== 'private') {
+        this.error(node, 'State variables must be private');
+      }
+    }
+  },
+
+  class extends Base {
+    static ruleId = 'leading-underscore';
+
+    VariableDeclaration(node) {
+      if (node.isDeclaredConst) {
+        if (/^_/.test(node.name)) {
+          // TODO: re-enable and fix
+          // this.error(node, 'Constant variables should not have leading underscore');
+        }
+      } else if (node.visibility === 'private' && !/^_/.test(node.name)) {
+        this.error(node, 'Non-constant private variables must have leading underscore');
+      }
+    }
+
+    FunctionDefinition(node) {
+      if (node.visibility === 'private' || (node.visibility === 'internal' && node.parent.kind !== 'library')) {
+        if (!/^_/.test(node.name)) {
+          this.error(node, 'Private and internal functions must have leading underscore');
+        }
+      }
+      if (node.visibility === 'internal' && node.parent.kind === 'library') {
+        if (/^_/.test(node.name)) {
+          this.error(node, 'Library internal functions should not have leading underscore');
+        }
+      }
+    }
+  },
+
+  // TODO: re-enable and fix
+  // class extends Base {
+  //   static ruleId = 'no-external-virtual';
+  //
+  //   FunctionDefinition(node) {
+  //     if (node.visibility == 'external' && node.isVirtual) {
+  //       this.error(node, 'Functions should not be external and virtual');
+  //     }
+  //   }
+  // },
+];
diff --git a/scripts/solhint-custom/package.json b/scripts/solhint-custom/package.json
@@ -0,0 +1,4 @@
+{
+  "name": "solhint-plugin-openzeppelin",
+  "private": true
+}
diff --git a/scripts/upgradeable/transpile.sh b/scripts/upgradeable/transpile.sh
@@ -33,3 +33,6 @@ npx @openzeppelin/upgrade-safe-transpiler@latest -D \
   -x '!contracts/proxy/utils/UUPSUpgradeable.sol' \
   -x '!contracts/proxy/beacon/IBeacon.sol' \
   -p 'contracts/**/presets/**/*'
+
+# delete compilation artifacts of vanilla code
+npm run clean
diff --git a/scripts/upgradeable/upgradeable.patch b/scripts/upgradeable/upgradeable.patch
@@ -142,7 +142,7 @@ index 36f076e5..90c1db78 100644
   * separator of the implementation contract. This will cause the {_domainSeparatorV4} function to always rebuild the
   * separator from the immutable values, which is cheaper than accessing a cached version in cold storage.
 - *
-- * @custom:oz-upgrades-unsafe-allow state-variable-immutable state-variable-assignment
+- * @custom:oz-upgrades-unsafe-allow state-variable-immutable
   */
  abstract contract EIP712 is IERC5267 {
 -    using ShortStrings for *;
diff --git a/solhint.config.js b/solhint.config.js
@@ -0,0 +1,20 @@
+const customRules = require('./scripts/solhint-custom');
+
+const rules = [
+  'no-unused-vars',
+  'const-name-snakecase',
+  'contract-name-camelcase',
+  'event-name-camelcase',
+  'func-name-mixedcase',
+  'func-param-name-mixedcase',
+  'modifier-name-mixedcase',
+  'var-name-mixedcase',
+  'imports-on-top',
+  'no-global-import',
+  ...customRules.map(r => `openzeppelin/${r.ruleId}`),
+];
+
+module.exports = {
+  plugins: ['openzeppelin'],
+  rules: Object.fromEntries(rules.map(r => [r, 'error'])),
+};
diff --git a/test/token/ERC20/extensions/ERC4626.t.sol b/test/token/ERC20/extensions/ERC4626.t.sol
@@ -3,7 +3,6 @@ pragma solidity ^0.8.19;
 
 import {ERC4626Test} from "erc4626-tests/ERC4626.test.sol";
 
-import {SafeCast} from "openzeppelin/utils/math/SafeCast.sol";
 import {ERC20} from "openzeppelin/token/ERC20/ERC20.sol";
 import {ERC4626} from "openzeppelin/token/ERC20/extensions/ERC4626.sol";
 

-Original file line number
+Diff line change
@@ @@ -0,0 +1,5 @@ @@
 +---
 +'openzeppelin-solidity': major
 +---
++
 +`ERC1155Receiver`: Removed in favor of `ERC1155Holder`.