LF-12515 Improved Solhint #1025
Conversation
WalkthroughThis pull request introduces extensive updates to the Solidity linting configuration in the Changes
Suggested reviewers
Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (2)
🔇 Additional comments (8)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
package.json (1)
43-43: Solhint Dependency Version Upgrade
The dependency forsolhinthas been updated from^3.3.7to^5.0.5. This significant upgrade is expected to support the new and stricter linting rules defined in the Solhint configuration. Please verify that there are no breaking changes introduced by the new version by reviewing Solhint's migration guide.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (3)
.solhint.json(1 hunks).solhintignore(1 hunks)package.json(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- .solhintignore
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: run-unit-tests
🔇 Additional comments (1)
.solhint.json (1)
18-31: New Solidity Linting Rules Added
The new rules (e.g.,explicit-types,gas-custom-errors,max-states-count,quotes,avoid-sha3,avoid-suicide,avoid-throw,no-empty-blocks,no-global-import,no-unused-import,no-unused-vars,payable-fallback, andcontract-name-capwords) effectively enforce stricter coding standards as per the latest best practices. Ensure that current Solidity contracts comply with these rules and consider any necessary code refactoring if violations occur.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
solhint-filtered.ts (5)
6-52: Consider storing contract versions in a separate config file.The hardcoded
contractVersionsobject contains many entries that would be better maintained in a separate JSON configuration file. This would make it easier to update and would separate configuration from logic.-// load stored versions from contracts.json -const contractVersions = { - 'src/LiFiDiamond.sol': '1.0.0', - 'src/Facets/CalldataVerificationFacet.sol': '1.2.0', - // ... many more entries -} +// load stored versions from a separate JSON file +const contractVersions = JSON.parse( + fs.readFileSync(path.join(process.cwd(), 'contract-versions.json'), 'utf8') +)
58-91: Add type validation for version strings.The script assumes that all version strings follow semantic versioning format, but doesn't validate this assumption. Invalid version strings could cause unexpected behavior.
+ // function to validate semver format + function isValidSemver(version: string): boolean { + return /^\d+\.\d+\.\d+$/.test(version) + } files.forEach((file) => { // ... const versionMatch = content.match(/@custom:version\s+([\d.]+)/) const currentVersion = versionMatch ? versionMatch[1] : null + // Validate version format + if (currentVersion && !isValidSemver(currentVersion)) { + console.log(`❌ ${relativeFile} has invalid version format: ${currentVersion}. Including for linting.`) + filesToLint.push(file) + return + } // ... })
72-89: Improve error handling for file path mismatches.The script doesn't handle the case where a file exists but isn't recorded in the
contractVersionsobject clearly. While it does include the file for linting, it would be better to explicitly log this situation.if (!currentVersion) { console.log( `⚠️ ${relativeFile} has no @custom:version. Including for linting.` ) filesToLint.push(file) } else if ( - !storedVersion || + !storedVersion) { + console.log( + `⚠️ ${relativeFile} not found in version registry. Including for linting.` + ) + filesToLint.push(file) + } else if ( isVersionNewer(currentVersion, storedVersion) ) { console.log( `✅ ${relativeFile} has a newer version (${currentVersion}). Linting.` ) filesToLint.push(file) } else {
100-104: Improve error reporting in catch block.The catch block discards error details that might be useful for debugging, only preserving the exit code.
} catch (error: any) { // in case of deeper solhint error you print just error.message if needed: - // console.error(error.message) + console.error('Solhint execution failed:'); + if (error.message) { + console.error(error.message); + } process.exit(error.status || 1) }
110-120: Version comparison function could be more robust.The current version comparison function works for standard semantic versioning, but doesn't handle pre-release versions or missing components.
Consider using a dedicated semver library for more robust comparison:
-// function to compare versions -function isVersionNewer(current: string, stored: string): boolean { - const parseVersion = (v: string) => v.split('.').map(Number) - const [cMajor, cMinor, cPatch] = parseVersion(current) - const [sMajor, sMinor, sPatch] = parseVersion(stored) - - return ( - cMajor > sMajor || - (cMajor === sMajor && cMinor > sMinor) || - (cMajor === sMajor && cMinor === sMinor && cPatch > sPatch) - ) -} +// function to compare versions using semantic versioning rules +function isVersionNewer(current: string, stored: string): boolean { + const parseVersion = (v: string) => { + const parts = v.split('.').map(Number); + return { + major: parts[0] || 0, + minor: parts[1] || 0, + patch: parts[2] || 0 + }; + }; + + const currentVer = parseVersion(current); + const storedVer = parseVersion(stored); + + return ( + currentVer.major > storedVer.major || + (currentVer.major === storedVer.major && currentVer.minor > storedVer.minor) || + (currentVer.major === storedVer.major && currentVer.minor === storedVer.minor && currentVer.patch > storedVer.patch) + ); +}.solhint.json (1)
35-45: Consider adding explanatory comments for disabled rules.Several security-related rules like
"avoid-call-value","avoid-low-level-calls", and"no-inline-assembly"are turned off. It would be helpful to add comments explaining why these potentially important security checks are disabled.- "avoid-call-value": "off", - "avoid-low-level-calls": "off", + // Disabled because the project needs to use low-level calls for cross-chain interactions + "avoid-call-value": "off", + "avoid-low-level-calls": "off", ... - "no-inline-assembly": "off", + // Disabled because inline assembly is needed for gas optimizations in critical paths + "no-inline-assembly": "off",
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.solhint.json(1 hunks)package.json(2 hunks)solhint-filtered.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- package.json
🔇 Additional comments (4)
solhint-filtered.ts (1)
97-99: Use--fixflag with caution in automated workflows.The script uses
--fixflag with Solhint, which automatically fixes issues. This is potentially risky in automated contexts as it might introduce unintended changes.Consider documenting this behavior clearly or making it configurable:
- execSync(`npx solhint --noPrompt --fix ${filesToLint.join(' ')}`, { + const fixFlag = process.env.SOLHINT_AUTOFIX === 'true' ? '--fix' : ''; + execSync(`npx solhint --noPrompt ${fixFlag} ${filesToLint.join(' ')}`, { stdio: 'inherit', }).solhint.json (3)
12-47: Comprehensive linting rules added for better code quality.The updated configuration adds numerous rules for Solidity code quality, naming conventions, security practices, and best practices. This will help enforce consistent code standards across the codebase.
However, consider documenting the rationale for specific rule choices, particularly for rules like "max-states-count" where the limit of 15 might need justification. A comment in the file or separate documentation would help developers understand the reasoning.
34-34:❓ Verification inconclusive
Verify double quotes consistency in the codebase.
The new rule
"quotes": ["error","double"]enforces double quotes for strings. Ensure that this matches the existing style in the codebase, or be prepared for many changes during the initial run.
🏁 Script executed:
#!/bin/bash # Check how many files use single quotes vs double quotes in Solidity files echo "Files using single quotes:" grep -l "'" --include="*.sol" -r . echo "Files using double quotes:" grep -l "\"" --include="*.sol" -r .Length of output: 17752
Action: Confirm enforcement of double quotes across the codebase
The new rule
"quotes": ["error", "double"]in.solhint.jsonwill force all string literals to use double quotes. Our grep results show that many Solidity files—especially in tests and some source contracts—currently contain single quotes (e.g.,./src/Periphery/TokenWrapper.sol,./test/solidity/utils/TestBase.sol, etc.).
- Verify if the codebase should be updated to use double quotes uniformly.
- Consider automating the migration or creating exemptions for tests if the scope of changes is too large.
- Be prepared for a significant diff when applying these style changes.
42-42:✅ Verification successful
Consider making compiler version more flexible.
The rule
"compiler-version": ["error", "^0.8.17"]might be too restrictive. If you plan to upgrade to newer Solidity versions in the future, a more flexible pattern like"^0.8.0"might be better.
🏁 Script executed:
#!/bin/bash # Check current compiler pragma versions in the codebase grep -r "pragma solidity" --include="*.sol" .Length of output: 26191
Compiler version rule is consistent with the codebase
Based on the grep results, nearly all Solidity files (both production and tests) explicitly use^0.8.17—ensuring consistent and deterministic behavior—aside from one utility file (test/solidity/utils/Console.sol) which intentionally uses a broader>=0.8.0pattern. Given this consistency and the clear commitment to a specific patch version, the linter rule in.solhint.json:"compiler-version": ["error", "^0.8.17"],remains appropriate for the project. If you decide in the future to allow a broader range (for example, to facilitate upgrades to newer 0.8.x versions), please update both the linter configuration and all Solidity files accordingly to maintain uniformity.
Which Jira task belongs to this PR?
LF-12515
Why did I implement it this way?
Solhint provides a set of best practices and security rules for Solidity code linting. However, our current configuration does not fully enforce the latest best practices as recommended in Solhint Rules.
Checklist before requesting a review
Checklist for reviewer (DO NOT DEPLOY and contracts BEFORE CHECKING THIS!!!)