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
left a comment
Amxx
left a comment
There was a problem hiding this comment.
How efficient/explicit is /^_/.test(node.name) compared to node.name.startsWith('_')
Amxx
force-pushed
the
update-solhint-custom-rules
branch
from
d1b7fad to
ca766d7
Compare
Amxx
added
automation
Amxx
changed the title
ernestognw
left a comment
ernestognw
left a comment
There was a problem hiding this comment.
I'm curious about the external -> public changes. So far I thought we already made all functions public when needed, so I would assume they were intentional, can we confirm reviewing the history to make sure we're not missing a deliberate decision?
| * @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
|
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
|
Seems like the consensus on immutables is to be more relaxed and leave them as they are (since there are 20+ cases), and I’m fine with that. The only pending item I see is:
|
4 participants
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