Add Solhint linting + pre-commit hook #69
Conversation
❌ Deploy Preview for uniswap-hooks-docs failed. Why did it fail? →
|
❌ Deploy Preview for uniswap-hooks failed. Why did it fail? →
|
gonzaotc
changed the title
OpenZeppelin/contractsOpenZeppelin/contracts
|
If this PR get's approved I will fix all the current warnings from prettier and solhint rules on the current contracts, which is an easy lift but I wanted to get this approved first. Feel free to remove or add any rule of preference from https://protofire.github.io/solhint/docs/rules.html, I took the current configuration from @luiz-lvj thanks! |
There was a problem hiding this comment.
LGTM @gonzaotc!
The configs seems nice, I don't have a strong opinion on the different linting mechanisms, so I think it makes sense to follow openzeppelin-contracts.
Afaik by merging this, the checks would fail, right? Let's wait on the releases to merge these changes. Thanks!
Hey @luiz-lvj yes, your intuition is correct, after we merge this PR all the code that violate rules will be flagged. However, take into account that the Additionally, I already have other two PR's in line to get merged after this one: this fixing all prettier findings, and this fixing most solhint findings. I made them separate just to make them easier to review. I agree with you on proceeding with this after the incoming release. Thanks! |
gonzaotc
changed the title
OpenZeppelin/contracts
| class extends Base { | ||
| static ruleId = 'interface-only-external-functions'; | ||
|
|
||
| FunctionDefinition(node) { | ||
| if (node.parent.kind === 'interface' && node.visibility !== 'external') { | ||
| this.error(node, 'Interface functions must be external'); | ||
| } | ||
| } | ||
| }, |
There was a problem hiding this comment.
I see that instead of the rule interface-names coming from openzeppelin-contracts, we have interface-only-external-functions and leading-underscore is more strict. What's the reasoning?
There was a problem hiding this comment.
Good question, this update to the custom solhint rules is taken from a snapshot of a recent in-progress update for such that we did in openzeppelin-contracts with Hadrien: OpenZeppelin/openzeppelin-contracts#5794, those updates does better enforcement of our solidity guidelines. It wasn't applied yet since those fixes in the contracts library requires breaking changes that we may consider for the 6.0 release. On the other side, given the earliness of this library, we may be able to move faster and patch our code to our preferred practices.
There was a problem hiding this comment.
Particularly, we are looking to enforce:
- Only external functions in libraries
- Preventing leading underscore in constants and immutables, which was previously only prevented in private constants
- Preventing leading underscore in public and external functions.
- Preventing external virtual functions.
| 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'); | ||
| } | ||
| } | ||
| }, | ||
| ]; |
There was a problem hiding this comment.
Is this rule needed? It's not enabled in openzeppelin-contracts. Also, note that this function is an example that would fail on this rule.
There was a problem hiding this comment.
Preventing external virtual functions was in WIP for openzeppelin-contracts and part of our guidelines, the rationale is that if a function is virtual, it means that it is expected to be overwritten. However, an external virtual function cannot call .super, which effectively breaks inheritance. Therefore, we enforce public virtual over external virtual
There was a problem hiding this comment.
Additionally, all the required fixes for the solhint findings are done in this other PR! gonzaotc#8, which is dependent on this one
There was a problem hiding this comment.
That makes sense, thanks @gonzaotc!
Adding linting:
solhint,solhint-plugin-openzeppelinAdding two solhint configurations:
solhint.src.config.jsandsolhint.test.config.js. One forsrc/and other for/test, respectively. The rationale is that there are some rules such asfoundry-test-functionsthat we may only want to run on tests, andfunc-name-mixedcasethat we may want to run only on contracts.Added pre-commit linting via husky