fix: improve solhint custom rules and remove TODOs

contracts/governance/Governor.sol

@@ -653,7 +653,7 @@ abstract contract Governor is Context, ERC165, EIP712, Nonces, IGovernor, IERC72
      * in a governance proposal to recover tokens or Ether that was sent to the governor contract by mistake.
      * Note that if the executor is simply the governor itself, use of `relay` is redundant.
      */
-    function relay(address target, uint256 value, bytes calldata data) external payable virtual onlyGovernance {
+    function relay(address target, uint256 value, bytes calldata data) public payable virtual onlyGovernance {
         (bool success, bytes memory returndata) = target.call{value: value}(data);
         Address.verifyCallResult(success, returndata);
     }

contracts/governance/TimelockController.sol

@@ -26,7 +26,7 @@ contract TimelockController is AccessControl, ERC721Holder, ERC1155Holder {
     bytes32 public constant PROPOSER_ROLE = keccak256("PROPOSER_ROLE");
     bytes32 public constant EXECUTOR_ROLE = keccak256("EXECUTOR_ROLE");
     bytes32 public constant CANCELLER_ROLE = keccak256("CANCELLER_ROLE");
-    uint256 internal constant _DONE_TIMESTAMP = uint256(1);
+    uint256 internal constant DONE_TIMESTAMP = uint256(1);
 
     mapping(bytes32 id => uint256) private _timestamps;
     uint256 private _minDelay;
@@ -207,7 +207,7 @@ contract TimelockController is AccessControl, ERC721Holder, ERC1155Holder {
         uint256 timestamp = getTimestamp(id);
         if (timestamp == 0) {
             return OperationState.Unset;
-        } else if (timestamp == _DONE_TIMESTAMP) {
+        } else if (timestamp == DONE_TIMESTAMP) {
             return OperationState.Done;
         } else if (timestamp > block.timestamp) {
             return OperationState.Waiting;
@@ -432,7 +432,7 @@ contract TimelockController is AccessControl, ERC721Holder, ERC1155Holder {
         if (!isOperationReady(id)) {
             revert TimelockUnexpectedOperationState(id, _encodeStateBitmap(OperationState.Ready));
         }
-        _timestamps[id] = _DONE_TIMESTAMP;
+        _timestamps[id] = DONE_TIMESTAMP;
     }
 
     /**
@@ -445,7 +445,7 @@ contract TimelockController is AccessControl, ERC721Holder, ERC1155Holder {
      * - the caller must be the timelock itself. This can only be achieved by scheduling and later executing
      * an operation where the timelock is the target and the data is the ABI-encoded call to this function.
      */
-    function updateDelay(uint256 newDelay) external virtual {
+    function updateDelay(uint256 newDelay) public virtual {
         address sender = _msgSender();
         if (sender != address(this)) {
             revert TimelockUnauthorizedCaller(sender);

contracts/governance/extensions/GovernorTimelockCompound.sol

@@ -154,7 +154,7 @@ abstract contract GovernorTimelockCompound is Governor {
 
      * CAUTION: It is not recommended to change the timelock while there are other queued governance proposals.
      */
-    function updateTimelock(ICompoundTimelock newTimelock) external virtual onlyGovernance {
+    function updateTimelock(ICompoundTimelock newTimelock) public virtual onlyGovernance {
         _updateTimelock(newTimelock);
     }
 

contracts/governance/extensions/GovernorTimelockControl.sol

@@ -146,7 +146,7 @@ abstract contract GovernorTimelockControl is Governor {
      *
      * CAUTION: It is not recommended to change the timelock while there are other queued governance proposals.
      */
-    function updateTimelock(TimelockController newTimelock) external virtual onlyGovernance {
+    function updateTimelock(TimelockController newTimelock) public virtual onlyGovernance {
         _updateTimelock(newTimelock);
     }
 

contracts/governance/extensions/GovernorVotesQuorumFraction.sol

@@ -73,7 +73,7 @@ abstract contract GovernorVotesQuorumFraction is GovernorVotes {
      * - Must be called through a governance proposal.
      * - New numerator must be smaller or equal to the denominator.
      */
-    function updateQuorumNumerator(uint256 newQuorumNumerator) external virtual onlyGovernance {
+    function updateQuorumNumerator(uint256 newQuorumNumerator) public virtual onlyGovernance {
         _updateQuorumNumerator(newQuorumNumerator);
     }
 

contracts/metatx/ERC2771Forwarder.sol

@@ -61,7 +61,7 @@ contract ERC2771Forwarder is EIP712, Nonces {
         bytes signature;
     }
 
-    bytes32 internal constant _FORWARD_REQUEST_TYPEHASH =
+    bytes32 internal constant FORWARD_REQUEST_TYPEHASH =
         keccak256(
             "ForwardRequest(address from,address to,uint256 value,uint256 gas,uint256 nonce,uint48 deadline,bytes data)"
         );
@@ -222,7 +222,7 @@ contract ERC2771Forwarder is EIP712, Nonces {
         (address recovered, ECDSA.RecoverError err, ) = _hashTypedDataV4(
             keccak256(
                 abi.encode(
-                    _FORWARD_REQUEST_TYPEHASH,
+                    FORWARD_REQUEST_TYPEHASH,
                     request.from,
                     request.to,
                     request.value,

contracts/mocks/proxy/UUPSUpgradeableMock.sol

@@ -29,7 +29,7 @@ contract UUPSUpgradeableUnsafeMock is UUPSUpgradeableMock {
 }
 
 contract UUPSUnsupportedProxiableUUID is UUPSUpgradeableMock {
-    function proxiableUUID() external pure override returns (bytes32) {
+    function proxiableUUID() public pure override returns (bytes32) {
         return keccak256("invalid UUID");
     }
 }

contracts/proxy/utils/UUPSUpgradeable.sol

@@ -69,7 +69,7 @@ abstract contract UUPSUpgradeable is IERC1822Proxiable {
      * bricking a proxy that upgrades to it, by delegating to itself until out of gas. Thus it is critical that this
      * function revert if invoked through a proxy. This is guaranteed by the `notDelegated` modifier.
      */
-    function proxiableUUID() external view virtual notDelegated returns (bytes32) {
+    function proxiableUUID() public view virtual notDelegated returns (bytes32) {
         return ERC1967Utils.IMPLEMENTATION_SLOT;
     }
 

contracts/token/ERC20/extensions/ERC20Permit.sol

@@ -71,7 +71,7 @@ abstract contract ERC20Permit is ERC20, IERC20Permit, EIP712, Nonces {
 
     /// @inheritdoc IERC20Permit
     // solhint-disable-next-line func-name-mixedcase
-    function DOMAIN_SEPARATOR() external view virtual returns (bytes32) {
+    function DOMAIN_SEPARATOR() external view returns (bytes32) {
         return _domainSeparatorV4();
     }
 }

contracts/utils/Base64.sol

@@ -11,22 +11,22 @@ library Base64 {
      * @dev Base64 Encoding/Decoding Table
      * See sections 4 and 5 of https://datatracker.ietf.org/doc/html/rfc4648
      */
-    string internal constant _TABLE = "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789+/";
-    string internal constant _TABLE_URL = "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789-_";
+    string internal constant TABLE = "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789+/";
+    string internal constant TABLE_URL = "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789-_";
 
     /**
      * @dev Converts a `bytes` to its Bytes64 `string` representation.
      */
     function encode(bytes memory data) internal pure returns (string memory) {
-        return _encode(data, _TABLE, true);
+        return _encode(data, TABLE, true);
     }
 
     /**
      * @dev Converts a `bytes` to its Bytes64Url `string` representation.
      * Output is not padded with `=` as specified in https://www.rfc-editor.org/rfc/rfc4648[rfc4648].
      */
     function encodeURL(bytes memory data) internal pure returns (string memory) {
-        return _encode(data, _TABLE_URL, false);
+        return _encode(data, TABLE_URL, false);
     }
 
     /**

contracts/utils/Multicall.sol

@@ -23,7 +23,7 @@ abstract contract Multicall is Context {
      * @dev Receives and executes a batch of function calls on this contract.
      * @custom:oz-upgrades-unsafe-allow-reachable delegatecall
      */
-    function multicall(bytes[] calldata data) external virtual returns (bytes[] memory results) {
+    function multicall(bytes[] calldata data) public virtual returns (bytes[] memory results) {
         bytes memory context = msg.sender == _msgSender()
             ? new bytes(0)
             : msg.data[msg.data.length - _contextSuffixLength():];

scripts/solhint-custom/index.js

@@ -48,8 +48,8 @@ module.exports = [
 
     VariableDeclaration(node) {
       if (node.isDeclaredConst) {
-        // TODO: expand visibility and fix
-        if (node.visibility === 'private' && /^_/.test(node.name)) {
+        // Check all visibility types for constants
+        if ((node.visibility === 'private' || node.visibility === 'internal') && /^_/.test(node.name)) {
           this.error(node, 'Constant variables should not have leading underscore');
         }
       } else if (node.visibility === 'private' && !/^_/.test(node.name)) {
@@ -71,14 +71,21 @@ module.exports = [
     }
   },
 
-  // 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');
-  //     }
-  //   }
-  // },
+  class extends Base {
+    static ruleId = 'no-external-virtual';
+
+    FunctionDefinition(node) {
+      // Skip interface definitions, constructors, receive, fallback functions
+      if (
+        node.parent.kind !== 'interface' &&
+        !node.isConstructor &&
+        !node.isReceiveEther &&
+        !node.isFallback &&
+        node.visibility === 'external' &&
+        node.isVirtual
+      ) {
+        this.error(node, 'External functions should not be virtual. Consider using public virtual instead.');
+      }
+    }
+  },
 ];