Update OZ solhint custom rules #5794
Conversation
|
|
For reference, there is already this PR that deals with linter rules: #5497 |
There was a problem hiding this comment.
Ideally I would like to add another rule to verify that Interfaces can only import other interfaces
AFAIK, this is already something solidity enforces. We don't need the linter to check taht if the compiler rejects it anyway.
Edit: that is true of "inheritance", importing is a different thing.
Amxx
force-pushed
the
update-solhint-custom-rules
branch
from
d1b7fad to
ca766d7
Compare
Amxx
added
automation
Amxx
changed the title
| * @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) { |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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:
- is there any reason we think overrides won't need to call super?
- 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
WalkthroughThis change set primarily updates function visibility from external to public across multiple contracts (Governor, TimelockController, GovernorTimelockCompound, GovernorTimelockControl, GovernorVotesQuorumFraction, UUPSUpgradeable, ERC20Permit, Multicall, and a mock). It also renames internal constants to drop leading underscores in TimelockController and ERC2771Forwarder, updating references and tests accordingly. The solhint custom rules are reworked: new require-based reporting, renamed interface rule to interface-only-external-functions, added no-external-virtual, tightened private-variables policy, and refined leading-underscore handling. Tests are adjusted to the new FORWARD_REQUEST_TYPEHASH constant usage. Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (12)
🧰 Additional context used🧠 Learnings (2)📓 Common learnings📚 Learning: 2025-08-29T13:17:07.068ZApplied to files:
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
🔇 Additional comments (13)
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
scripts/solhint-custom/index.js
Outdated
| module.exports = [ | ||
| class extends Base { | ||
| static ruleId = 'interface-names'; | ||
| static ruleId = 'interface-only-external-functions'; | ||
|
|
||
| ContractDefinition(node) { | ||
| if (node.kind === 'interface' && !/^I[A-Z]/.test(node.name)) { | ||
| this.error(node, 'Interface names should have a capital I prefix'); | ||
| FunctionDefinition(node) { | ||
| if (node.parent.kind === 'interface') { | ||
| this.require(node.visibility === 'external', node, 'Interface functions must be external'); | ||
| } | ||
| } | ||
| }, |
There was a problem hiding this comment.
Isn't this enforced by the compiler?
There was a problem hiding this comment.
indeed it is already checked.
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
|
All alerts resolved. Learn more about Socket for GitHub. This PR previously contained dependency changes with security issues that have been resolved, removed, or ignored. |
scripts/solhint-custom/index.js
Outdated
| // TODO: expand visibility and fix | ||
| if (node.visibility === 'private' && /^_/.test(node.name)) { | ||
| this.error(node, 'Constant variables should not have leading underscore'); | ||
| this.require(!node.name.startsWith('_'), node, 'Constant variables should not have leading underscore'); |
There was a problem hiding this comment.
should we require screaming snake case?
There was a problem hiding this comment.
There is already a const-name-snakecase in solhint. See line 5 of solhint.config.js
There was a problem hiding this comment.
If we want to do immutable snakecase, there is also a immutable-vars-naming we could enable.
…aotc/openzeppelin-contracts into update-solhint-custom-rules
interface-namescustom rule which is now covered by solhint's already includedinterface-starts-with-iinterface-only-external-functionsrule.Note that 2, 3, and 4 require breaking changes that can be potential candidates for 6.0
Ideally I would like to add another rule to verify that Interfaces can only import other interfaces