Skip to content

Update OZ solhint custom rules #5794

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 22 commits into from
Sep 23, 2025
Merged

Update OZ solhint custom rules #5794

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
37c7a52
up rules
gonzaotc Jul 10, 2025
6e13ca7
Update index.js
Amxx Jul 10, 2025
56b120f
Update index.js
Amxx Jul 10, 2025
ba351f3
Update index.js
Amxx Jul 10, 2025
bee7bc3
Update index.js
Amxx Jul 16, 2025
f859dd7
Update index.js
Amxx Jul 16, 2025
d807d85
Update index.js
Amxx Jul 16, 2025
add0ff0
fix lint
Amxx Jul 17, 2025
101f119
improve
Amxx Jul 24, 2025
ca766d7
fix code to match new rules
Amxx Jul 24, 2025
9192838
Merge branch 'master' into update-solhint-custom-rules
Amxx Jul 24, 2025
ac17893
Merge branch 'master' into update-solhint-custom-rules
Amxx Aug 27, 2025
a9560bc
Merge branch 'master' into update-solhint-custom-rules
Amxx Sep 19, 2025
1525a7b
Remove interface-only-external-functions that is already enforced by …
Amxx Sep 19, 2025
34e6f7d
Update scripts/solhint-custom/index.js
Amxx Sep 19, 2025
1c5046b
js lint
Amxx Sep 19, 2025
01d53b1
add no-public-library rule
Amxx Sep 19, 2025
00550a7
update solhint
Amxx Sep 19, 2025
728bb00
Update scripts/solhint-custom/index.js
arr00 Sep 19, 2025
47b57c0
use solhint commons
Amxx Sep 19, 2025
b73b564
Merge branch 'update-solhint-custom-rules' of https://github.com/gonz…
Amxx Sep 19, 2025
eb66607
external non virtual
Amxx Sep 22, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion contracts/governance/Governor.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Expand Down
8 changes: 4 additions & 4 deletions contracts/governance/TimelockController.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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;
}

/**
Expand All @@ -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);
Expand Down
4 changes: 2 additions & 2 deletions contracts/governance/extensions/GovernorTimelockCompound.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,7 +136,7 @@ abstract contract GovernorTimelockCompound is Governor {
/**
* @dev Accept admin right over the timelock.
*/
// solhint-disable-next-line private-vars-leading-underscore
// solhint-disable-next-line openzeppelin/leading-underscore
function __acceptAdmin() public {
_timelock.acceptAdmin();
}
Expand All @@ -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);
}

Expand Down
2 changes: 1 addition & 1 deletion contracts/governance/extensions/GovernorTimelockControl.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}

Expand Down
2 changes: 1 addition & 1 deletion contracts/governance/extensions/GovernorVotesQuorumFraction.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}

Expand Down
4 changes: 2 additions & 2 deletions contracts/metatx/ERC2771Forwarder.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)"
);
Expand Down Expand Up @@ -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,
Expand Down
4 changes: 2 additions & 2 deletions contracts/mocks/proxy/UUPSUpgradeableMock.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,8 +28,8 @@ contract UUPSUpgradeableUnsafeMock is UUPSUpgradeableMock {
}
}

contract UUPSUnsupportedProxiableUUID is UUPSUpgradeableMock {
function proxiableUUID() external pure override returns (bytes32) {
contract UUPSUnsupportedProxiableUUIDMock is NonUpgradeableMock {
function proxiableUUID() external pure returns (bytes32) {
return keccak256("invalid UUID");
}
}
2 changes: 1 addition & 1 deletion contracts/proxy/utils/UUPSUpgradeable.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,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() external view notDelegated returns (bytes32) {
return ERC1967Utils.IMPLEMENTATION_SLOT;
}

Expand Down
2 changes: 1 addition & 1 deletion contracts/token/ERC20/extensions/ERC20Permit.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();
}
}
2 changes: 1 addition & 1 deletion contracts/utils/Multicall.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I remember correctly, this is purposely external since it doesn't have an internal use. Users could just call the functions they want directly

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By default, all public facing functions should be public so that overrides can call super.

If we have exceptions, the questions we need to answer are:

  1. is there any reason we think overrides won't need to call super?
  2. is there any risks in making that internally callable?

I'm not sure what override we would want to do, but I'd say calling super would be reasonnable here ... so really the question that remains is 2.

If function A internally calls multicall(...) (instead of doing this.multicall(...)) that would allow arbitrary function execution with the same caller as A's initial context. That is also what we would achieve by directly calling the corresponding function (assuming they are public and not external).

But yes, let review all the external->public changes, and we can disable the rule selectivelly if we have a good reason to keep the functions externals

bytes memory context = msg.sender == _msgSender()
? new bytes(0)
: msg.data[msg.data.length - _contextSuffixLength():];
Expand Down
56 changes: 8 additions & 48 deletions package-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@
"prettier-plugin-solidity": "^2.0.0",
"rimraf": "^6.0.0",
"semver": "^7.3.5",
"solhint": "^6.0.0",
"solhint": "^6.0.1",
"solhint-plugin-openzeppelin": "file:scripts/solhint-custom",
"solidity-ast": "^0.4.50",
"solidity-coverage": "^0.8.14",
Expand Down
Loading