Skip to content

Conversation

fvictorio
Copy link
Contributor

@fvictorio fvictorio commented Sep 5, 2025

Hi, for the last couple of months I've been working on a new linter for Solidity, and I think it's already more flexible and powerful than Solhint: Slippy. This PR is meant to show how moving to Slippy would look like. I hope you consider doing it!

I split this PR into four commits:

  • The first commit removes some unused imports that were caught by Slippy's no-unused-vars rule. This rule is more powerful than Solhint's equivalent (it checks unused imports and unused private variables and functions) and more flexible (you can configure a pattern like "starts with underscore" to mark some variables as intentionally unused).
  • The second commit adds names to a couple of return params. These were caught by Slippy's named-return-params rule.
  • The third commit installs Slippy, creates a config, and adds a couple of comment directives that were necessary to make it pass. I enabled all the non-naming related rules that matched the Solhint rules that OZ is using. I also added a couple more that passed fine.
  • And the fourth commit enables Slippy's powerful naming-convention rule. This rule replaces the usage of multiple Solhint rules like const-name-snakecase, contract-name-capwords, etc. It also replaces OZ's interface-names custom rule (which is redundant with Solhint's interface-starts-with-i, but anyway). Most of the changes in this commit are related to adding equivalent slippy-disable-* comments; the interesting changes are in the Slippy config themselves.

Why Slippy

I wrote a detailed comparison with Solhint in the repo, but here are some highlights:

  • A flexible config file. Notice how slippy.config.js easily lets you configure some rules for every Solidity file, some for every non-mock and non-test file, and some only for test files. This is inspired by eslint's flat configuration files.
  • A better no-unused-vars rule. As mentioned above, this rule is both more powerful (catches more things) and lets you set naming patterns to mark as exceptions.
  • The naming-convention rule lets you define whatever naming convention you want. I admit that its config is quite verbose in the case of OZ, given that it has a mix of Solhint rules and custom rules, but it also shows that it can be adapted to any convention, no matter how complex it is.
  • Unlike Solhint, Slippy will warn you about unused // slippy-disable-* comments. In OZ's case, I don't think you have any stale directives, but I've seen multiple projects that have // solhint-disable-* that don't do anything.
  • Powered by Slang. You are already using Slang in OZ upgrades, so you probably already know how good it is. In the case of Slippy, it not only means building it on top of a better foundation: it also allows rules that are very hard otherwise, like compatible-pragma or no-uninitialized-immutable-references.
  • Speaking of Slang, Slippy also has a no-restricted-syntax rule that lets you easily forbid patterns using Slang queries.

Why not migrate

I also want to be honest about Slippy's current limitations:

  • No IDE support yet, but I want to add it to Hardhat's VSCode extension soon.
  • No --fix. This is also high on my list of things to work on.
  • No support for plugins. Again, this is on the roadmap, but I don't know when I'll add it. On the other hand, I think naming-convention and no-restricted-syntax should make this less urgent.
  • It's still in 0.x. I'm not going to break things unnecessarily, but it's early.
  • No browser support, but I think this is not relevant for you.

Summary by Sourcery

Replace Solhint with Slippy, updating dependencies, adding a flat Slippy configuration, and adjusting the codebase to adhere to Slippy’s no-unused-vars, named-return-params, and naming-convention rules.

New Features:

  • Integrate Slippy as the project's new Solidity linter

Enhancements:

  • Remove unused imports caught by Slippy’s no-unused-vars rule
  • Add missing return parameter names to comply with Slippy’s named-return-params rule
  • Insert slippy-disable comments throughout legacy code to satisfy the naming-convention rule

Build:

  • Install @slippy-lint/slippy and add slippy.config.js with custom rule sets for sources, mocks, and tests

Summary by CodeRabbit

  • Style
    • Added lint-suppression comments across governance, token, utils, and mock contracts to align naming conventions. No runtime behavior changes.
  • Chores
    • Introduced Slippy lint configuration and added the Slippy linter as a dev dependency.
    • Cleaned up unused imports across several contracts to reduce noise.
  • Tests
    • Added lint directives in test files to suppress naming-convention and console warnings without affecting test behavior.

@fvictorio fvictorio requested a review from a team as a code owner September 5, 2025 14:27
Copy link

changeset-bot bot commented Sep 5, 2025

⚠️ No Changeset found

Latest commit: 55122e8

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

Copy link
Contributor

sourcery-ai bot commented Sep 5, 2025

Reviewer's Guide

This PR replaces Solhint with Slippy by installing Slippy, introducing a detailed Slippy config, and updating code to satisfy its rules (unused-vars, named-return-params, naming-convention) via removals, parameter naming, and inline disable directives.

Entity relationship diagram for linter configuration files

erDiagram
    PACKAGE_JSON ||--o| SLIPPY_CONFIG : "uses"
    SLIPPY_CONFIG {
      string[] rules
      string[] ignores
      string[] files
    }
    PACKAGE_JSON {
      string dependencies
    }
Loading

Class diagram for updated ERC7579FallbackHandlerMock return params

classDiagram
    class ERC7579FallbackHandlerMock {
      +callView() : (address account, address sender)
    }
Loading

File-Level Changes

Change Details Files
Remove unused imports and variables using Slippy's no-unused-vars rule
  • Removed obsolete imports not referenced elsewhere
  • Added top-level slippy-disable for unavoidable unused imports
contracts/account/extensions/draft-AccountERC7579.sol
contracts/mocks/token/ERC20BridgeableMock.sol
contracts/mocks/utils/cryptography/ERC7739Mock.sol
contracts/mocks/account/AccountMock.sol
Add explicit names to return parameters under named-return-params rule
  • Converted unnamed tuple returns to named return parameters
contracts/mocks/account/modules/ERC7579Mock.sol
Install Slippy and configure rules
  • Added @slippy-lint/slippy dependency
  • Created comprehensive slippy.config.js mirroring and extending Solhint rules
package.json
slippy.config.js
Enable Slippy's naming-convention rule and inline-disable violations
  • Inserted slippy-disable naming-convention comments on legacy mixed-case initializers and overrides
  • Appended slippy-disable directives to override and clock functions
  • Disabled naming-convention for utility modules where needed
contracts/mocks/MultipleInheritanceInitializableMocks.sol
test/governance/Governor.t.sol
test/governance/extensions/GovernorSuperQuorumGreaterThanQuorum.t.sol
contracts/governance/Governor.sol
contracts/governance/IGovernor.sol
contracts/interfaces/IERC6372.sol
contracts/mocks/token/ERC20VotesTimestampMock.sol
contracts/mocks/token/ERC721VotesTimestampMock.sol
contracts/proxy/utils/UUPSUpgradeable.sol
contracts/token/ERC20/extensions/IERC20Permit.sol
contracts/utils/cryptography/EIP712.sol
contracts/governance/extensions/GovernorCountingFractional.sol
contracts/governance/extensions/GovernorCountingOverridable.sol
contracts/governance/extensions/GovernorCountingSimple.sol
contracts/governance/extensions/GovernorVotes.sol
contracts/governance/utils/Votes.sol
contracts/mocks/VotesExtendedMock.sol
contracts/mocks/VotesMock.sol
contracts/mocks/docs/governance/MyTokenTimestampBased.sol
scripts/generate/templates/Packing.js
test/account/utils/draft-ERC7579Utils.t.sol

Tips and commands

Interacting with Sourcery

  • Trigger a new review: Comment @sourcery-ai review on the pull request.
  • Continue discussions: Reply directly to Sourcery's review comments.
  • Generate a GitHub issue from a review comment: Ask Sourcery to create an
    issue from a review comment by replying to it. You can also reply to a
    review comment with @sourcery-ai issue to create an issue from it.
  • Generate a pull request title: Write @sourcery-ai anywhere in the pull
    request title to generate a title at any time. You can also comment
    @sourcery-ai title on the pull request to (re-)generate the title at any time.
  • Generate a pull request summary: Write @sourcery-ai summary anywhere in
    the pull request body to generate a PR summary at any time exactly where you
    want it. You can also comment @sourcery-ai summary on the pull request to
    (re-)generate the summary at any time.
  • Generate reviewer's guide: Comment @sourcery-ai guide on the pull
    request to (re-)generate the reviewer's guide at any time.
  • Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
    pull request to resolve all Sourcery comments. Useful if you've already
    addressed all the comments and don't want to see them anymore.
  • Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
    request to dismiss all existing Sourcery reviews. Especially useful if you
    want to start fresh with a new review - don't forget to comment
    @sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

  • Enable or disable review features such as the Sourcery-generated pull request
    summary, the reviewer's guide, and others.
  • Change the review language.
  • Add, remove or edit custom review instructions.
  • Adjust other review settings.

Getting Help

Copy link

coderabbitai bot commented Sep 5, 2025

Walkthrough

Project-wide linting integration: adds Slippy linter config/dependency and inserts suppression comments across contracts, mocks, and tests for naming conventions and related rules. Removes several unused imports. One function now uses named return parameters. A codegen template gains a matching lint directive. No runtime logic changes.

Changes

Cohort / File(s) Change summary
Lint suppressions (naming/console/unused vars)
contracts/governance/Governor.sol, contracts/governance/IGovernor.sol, contracts/governance/extensions/GovernorCounting*.sol, contracts/governance/extensions/GovernorVotes.sol, contracts/governance/utils/Votes.sol, contracts/interfaces/IERC6372.sol, contracts/token/ERC20/extensions/ERC20Permit.sol, contracts/token/ERC20/extensions/IERC20Permit.sol, contracts/utils/Packing.sol, contracts/utils/cryptography/EIP712.sol, contracts/proxy/utils/UUPSUpgradeable.sol, contracts/vendor/compound/ICompoundTimelock.sol, contracts/mocks/Votes*.sol, contracts/mocks/token/ERC20Votes*Mock.sol, contracts/mocks/docs/governance/MyTokenTimestampBased.sol, contracts/mocks/MultipleInheritanceInitializableMocks.sol, contracts/mocks/Stateless.sol, test/governance/*.t.sol, test/.../draft-ERC7579Utils.t.sol
Inserted Slippy lint directives (mainly naming-convention; also no-console, no-unused-vars) as comments. No functional or API changes.
Removed unused imports
contracts/account/extensions/draft-AccountERC7579.sol, contracts/mocks/account/AccountMock.sol, contracts/mocks/governance/GovernorNoncesKeyedMock.sol, contracts/mocks/token/ERC20BridgeableMock.sol, contracts/mocks/utils/cryptography/ERC7739Mock.sol, contracts/utils/cryptography/signers/draft-ERC7739.sol
Dropped unused imports (Address, ERC4337Utils, GovernorProposalGuardian, ERC20, ECDSA, ShortStrings). No behavior or interface changes.
Named returns update
contracts/mocks/account/modules/ERC7579Mock.sol
callView() now declares named returns (address account, address sender); return types/order unchanged.
Tooling: Slippy integration
package.json, slippy.config.js
Added @slippy-lint/slippy devDependency and introduced slippy.config.js with project/test/mocks naming-convention rules and other lints.
Template lint header
scripts/generate/templates/Packing.js
Added // slippy-disable naming-convention to generated header template only.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Poem

I twitch my ears at tidy code,
Hop-hop, the linters share the load.
A name suppressed, imports trimmed,
Configs laid and comments primmed.
With carrots cached in devDependencies,
I bound through builds with breezy ease. 🥕✨

✨ Finishing Touches
  • 📝 Generate Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment

🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.

Support

Need help? Create a ticket on our support page for assistance with any issues or questions.

CodeRabbit Commands (Invoked using PR/Issue comments)

Type @coderabbitai help to get the list of available commands.

Other keywords and placeholders

  • Add @coderabbitai ignore or @coderabbit ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Status, Documentation and Community

  • Visit our Status Page to check the current availability of CodeRabbit.
  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link

Review the following changes in direct dependencies. Learn more about Socket for GitHub.

Diff Package Supply Chain
Security
Vulnerability Quality Maintenance License
Addednpm/​@​slippy-lint/​slippy@​0.1.2771009694100
Updatednpm/​semver@​7.7.1 ⏵ 7.7.2100 +1100100 +185100

View full report

Copy link

Caution

Review the following alerts detected in dependencies.

According to your organization's Security Policy, you must resolve all "Block" alerts before proceeding. Learn more about Socket for GitHub.

Action Severity Alert  (click "▶" to expand/collapse)
Block Low
npm/zod@4.1.5 is a AI-detected potential code anomaly.

Notes: No explicit network exfiltration, reverse shell, or credential theft is present in this fragment. However, the code assembles and compiles arbitrary code via the Function constructor and invokes passed-in functions immediately (twice). That behavior constitutes a strong dangerous primitive (arbitrary code execution) which can be abused if any inputs (strings or args) are attacker-controlled. Treat this module as risky in threat models where inputs are not fully trusted; review call sites and sanitize/validate inputs or avoid dynamic evaluation.

Confidence: 1.00

Severity: 0.60

From: package-lock.jsonnpm/@slippy-lint/slippy@0.1.2npm/zod@4.1.5

ℹ Read more on: This package | This alert | What is an AI-detected potential code anomaly?

Next steps: Take a moment to review the security alert above. Review the linked package source code to understand the potential risk. Ensure the package is not malicious before proceeding. If you're unsure how to proceed, reach out to your security team or ask the Socket team for help at support@socket.dev.

Suggestion: An AI system found a low-risk anomaly in this package. It may still be fine to use, but you should check that it is safe before proceeding.

Mark the package as acceptable risk. To ignore this alert only in this pull request, reply with the comment @SocketSecurity ignore npm/zod@4.1.5. You can also ignore all packages with @SocketSecurity ignore-all. To ignore an alert for all future pull requests, use Socket's Dashboard to change the triage state of this alert.

View full report

Copy link
Contributor

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

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

Hey there - I've reviewed your changes - here's some feedback:

  • The naming-convention rule is triggering a lot of per-line disables; consider adding pattern-based exceptions (e.g. for __.*_init or CLOCK_MODE/COUNTING_MODE functions) in slippy.config.js so you don’t need to sprinkle disable comments everywhere.
  • Where possible, group slippy disables at the file or directory level (especially in tests or mocks) rather than on each line to cut down on noise and make the code more maintainable.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The naming-convention rule is triggering a lot of per-line disables; consider adding pattern-based exceptions (e.g. for __.*_init or CLOCK_MODE/COUNTING_MODE functions) in slippy.config.js so you don’t need to sprinkle disable comments everywhere.
- Where possible, group slippy disables at the file or directory level (especially in tests or mocks) rather than on each line to cut down on noise and make the code more maintainable.

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🧹 Nitpick comments (25)
package.json (1)

67-67: Wire Slippy into npm scripts/CI and ignore "slippy" in Codespell

  • Add a "lint:slippy": "slippy" script in package.json and invoke it from your existing lint and lint:sol chains.
  • Extend your lint-staged config for *.sol to run slippy after Solhint.
  • Create or update a Codespell config (e.g. .codespellrc or .codespellignore) to whitelist “slippy” so CI checks no longer flag it.
contracts/mocks/token/ERC20VotesAdditionalCheckpointsMock.sol (1)

29-29: Place the suppression before the signature and use -next-line for clarity.

Using slippy-disable-previous-line inside the body is easy to miss. Put it above the function with -next-line alongside solhint for consistency.

Apply within the selected line to remove the in-body directive:

-        // slippy-disable-previous-line naming-convention

Then adjust above (outside selected lines):

// solhint-disable-next-line func-name-mixedcase
// slippy-disable-next-line naming-convention
function CLOCK_MODE() public view virtual override returns (string memory) {
contracts/mocks/docs/governance/MyTokenTimestampBased.sol (1)

20-20: Mirror the suppression placement pattern used elsewhere.

Move the directive above the signature and switch to -next-line to make the intent unambiguous.

Apply within the selected line to remove the in-body directive:

-        // slippy-disable-previous-line naming-convention

Then adjust above (outside selected lines):

// solhint-disable-next-line func-name-mixedcase
// slippy-disable-next-line naming-convention
function CLOCK_MODE() public pure override returns (string memory) {
contracts/vendor/compound/ICompoundTimelock.sol (1)

41-47: Consistent suppression style (optional).

Using inline // slippy-disable-line naming-convention is fine. For consistency with the preceding Solhint directive and to reduce line length noise, consider switching to slippy-disable-next-line above each declaration.

 // solhint-disable-next-line func-name-mixedcase
-function GRACE_PERIOD() external view returns (uint256); // slippy-disable-line naming-convention
+// slippy-disable-next-line naming-convention
+function GRACE_PERIOD() external view returns (uint256);

 // solhint-disable-next-line func-name-mixedcase
-function MINIMUM_DELAY() external view returns (uint256); // slippy-disable-line naming-convention
+// slippy-disable-next-line naming-convention
+function MINIMUM_DELAY() external view returns (uint256);

 // solhint-disable-next-line func-name-mixedcase
-function MAXIMUM_DELAY() external view returns (uint256); // slippy-disable-line naming-convention
+// slippy-disable-next-line naming-convention
+function MAXIMUM_DELAY() external view returns (uint256);
contracts/mocks/Stateless.sol (1)

5-6: Fix minor grammar; suppression placement is good.

Change “just to we can run” → “just so we can run”. Optional: re-enable after imports to limit the disabled span.

-// We keep these imports and a dummy contract just to we can run the test suite after transpilation.
+// We keep these imports and a dummy contract just so we can run the test suite after transpilation.

Optional snippet (near the contract declaration):

// slippy-enable no-unused-vars
contracts/governance/utils/Votes.sol (1)

66-68: Nit: prefer trailing disable on the signature for consistency

Use slippy-disable-line on the declaration itself (matches IGovernor style) and drop the body-line directive.

-    function CLOCK_MODE() public view virtual returns (string memory) {
-        // slippy-disable-previous-line naming-convention
+    function CLOCK_MODE() public view virtual returns (string memory) { // slippy-disable-line naming-convention
contracts/governance/extensions/GovernorCountingFractional.sol (1)

58-60: Nit: inline the suppression on the signature for uniformity

Inline slippy-disable-line on the declaration and remove the extra line.

-    function COUNTING_MODE() public pure virtual override returns (string memory) {
-        // slippy-disable-previous-line naming-convention
+    function COUNTING_MODE() public pure virtual override returns (string memory) { // slippy-disable-line naming-convention
contracts/mocks/VotesMock.sol (1)

39-41: Nit: match the inline style used elsewhere

Move the suppression to the function line.

-    function CLOCK_MODE() public view virtual override returns (string memory) {
-        // slippy-disable-previous-line naming-convention
+    function CLOCK_MODE() public view virtual override returns (string memory) { // slippy-disable-line naming-convention
contracts/mocks/VotesExtendedMock.sol (1)

39-41: Nit: adopt inline suppression for consistency

Apply the trailing form and remove the extra comment line.

-    function CLOCK_MODE() public view virtual override returns (string memory) {
-        // slippy-disable-previous-line naming-convention
+    function CLOCK_MODE() public view virtual override returns (string memory) { // slippy-disable-line naming-convention
contracts/governance/IGovernor.sol (1)

202-202: Optional: centralize standard exceptions in Slippy config

To reduce scattered disable comments, consider allowlisting ERC-required identifiers (e.g., CLOCK_MODE, COUNTING_MODE, DOMAIN_SEPARATOR, __self) in slippy.config.js for function-name checks in contracts/mocks/tests.

contracts/utils/cryptography/EIP712.sol (2)

146-149: Scope linter suppression to the signature line for consistency

Inline the directive to reduce vertical noise and match usages elsewhere.

-// solhint-disable-next-line func-name-mixedcase
-function _EIP712Name() internal view returns (string memory) {
-        // slippy-disable-previous-line naming-convention
+// solhint-disable-next-line func-name-mixedcase
+function _EIP712Name() internal view returns (string memory) { // slippy-disable-line naming-convention

158-161: Same here: inline the naming suppression on the declaration

Keeps style uniform with other files.

-// solhint-disable-next-line func-name-mixedcase
-function _EIP712Version() internal view returns (string memory) {
-        // slippy-disable-previous-line naming-convention
+// solhint-disable-next-line func-name-mixedcase
+function _EIP712Version() internal view returns (string memory) { // slippy-disable-line naming-convention
contracts/governance/extensions/GovernorCountingOverridable.sol (1)

52-55: Prefer trailing // slippy-disable-line on the declaration

Matches the intent (suppress rule for this signature only) and reduces clutter.

-// solhint-disable-next-line func-name-mixedcase
-function COUNTING_MODE() public pure virtual override returns (string memory) {
-        // slippy-disable-previous-line naming-convention
+// solhint-disable-next-line func-name-mixedcase
+function COUNTING_MODE() public pure virtual override returns (string memory) { // slippy-disable-line naming-convention
contracts/governance/extensions/GovernorVotes.sol (1)

45-47: Inline the suppression on the signature

Consistent with other occurrences and avoids an extra line.

-// solhint-disable-next-line func-name-mixedcase
-function CLOCK_MODE() public view virtual override returns (string memory) {
-        // slippy-disable-previous-line naming-convention
+// solhint-disable-next-line func-name-mixedcase
+function CLOCK_MODE() public view virtual override returns (string memory) { // slippy-disable-line naming-convention
scripts/generate/templates/Packing.js (1)

39-40: Consider narrowing the disable or encoding it in config

File-wide disable is pragmatic for generated numeric-suffixed functions, but you could:

  • Encode an allowlist pattern for pack_/extract_/replace_ names in slippy.config.js, or
  • Prepend // slippy-disable-next-line naming-convention only before generated function declarations to limit scope.

If you want, I can draft a config override targeting the generated Packing.sol path with a regex for these function names.

contracts/mocks/token/ERC20VotesTimestampMock.sol (2)

15-18: Inline the suppression on the declaration

For consistency with other files.

-// solhint-disable-next-line func-name-mixedcase
-function CLOCK_MODE() public view virtual override returns (string memory) {
-        // slippy-disable-previous-line naming-convention
+// solhint-disable-next-line func-name-mixedcase
+function CLOCK_MODE() public view virtual override returns (string memory) { // slippy-disable-line naming-convention

27-30: Apply the same in the ERC721 variant

Keep both variants styled the same way.

-// solhint-disable-next-line func-name-mixedcase
-function CLOCK_MODE() public view virtual override returns (string memory) {
-        // slippy-disable-previous-line naming-convention
+// solhint-disable-next-line func-name-mixedcase
+function CLOCK_MODE() public view virtual override returns (string memory) { // slippy-disable-line naming-convention
contracts/mocks/MultipleInheritanceInitializableMocks.sol (1)

30-30: Directive placement looks correct; consider eliminating repeated disables via config.

Using // slippy-disable-previous-line naming-convention immediately after the function header neatly targets the mixed-case init names, including the multi-line signature at Line 124. To reduce noise across many init helpers, consider adding a narrow exception in slippy.config.js for ^__.*_init(_unchained)?$ function names in sources/mocks instead of per-callsite disables.

I can draft a config override that whitelists only that pattern without loosening other rules.

Also applies to: 36-36, 53-53, 61-61, 77-77, 84-84, 101-101, 108-108, 125-125, 138-138

slippy.config.js (1)

77-87: Consider scoping named-return-params to sources only.

With 'named-return-params': 'error' in the global block, tests/mocks also require naming. If that’s intentional, fine; otherwise move it to the sources block to cut test churn and comment directives.

I can prepare a patch that enables it only for contracts/**/* while leaving tests/mocks unchanged.

contracts/token/ERC20/extensions/ERC20Permit.sol (1)

75-75: Scoped suppression for EIP-mandated name is fine; consider moving to config to avoid inline churn.

The one-line disable is appropriate here. Optionally, add an allowlist pattern for DOMAIN_SEPARATOR in slippy.config.js (e.g., naming-convention exceptions for specific selectors) to remove inline comments across the repo.

contracts/governance/Governor.sol (1)

795-795: Consolidate inline linter disables and remove legacy Solhint directives
• Repo-wide, pick one Slippy disable form—e.g. replace all // slippy-disable-previous-line naming-convention with // slippy-disable-line naming-convention (currently both appear in contracts, extensions, mocks, and tests).
• Remove every // solhint-disable-* directive (several leftovers in scripts, tests, and contracts) if Solhint is no longer part of the linting pipeline.

contracts/governance/extensions/GovernorCountingSimple.sol (1)

33-33: OK to suppress here; prefer config-based exceptions for COUNTING_MODE.

Given COUNTING_MODE is part of a public API surface across multiple files, consider a config regex exception instead of per-site disables to keep sources clean.

test/governance/extensions/GovernorSuperQuorumGreaterThanQuorum.t.sol (3)

47-49: Test helper naming suppression is reasonable; you can relax naming rules for tests in config.

Add a tests-only override in slippy.config.js (pattern: test/**) allowing leading $/underscores to avoid inline directives.


53-55: Same as above for $_updateQuorumNumerator.

Prefer a tests-scope naming exception over inline disables to reduce noise.


83-85: Centralize naming-convention overrides in test config

  • Configure Slippy to ignore naming-convention under test/** and remove the 5 inline // slippy-disable-(previous-line|line) naming-convention directives.
  • Once Solhint is removed or scoped for tests, drop the adjacent // solhint-disable-next-line here.
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

💡 Knowledge Base configuration:

  • MCP integration is disabled by default for public repositories
  • Jira integration is disabled by default for public repositories
  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 0cb4888 and 55122e8.

⛔ Files ignored due to path filters (1)
  • package-lock.json is excluded by !**/package-lock.json
📒 Files selected for processing (34)
  • contracts/account/extensions/draft-AccountERC7579.sol (0 hunks)
  • contracts/governance/Governor.sol (1 hunks)
  • contracts/governance/IGovernor.sol (1 hunks)
  • contracts/governance/extensions/GovernorCountingFractional.sol (1 hunks)
  • contracts/governance/extensions/GovernorCountingOverridable.sol (1 hunks)
  • contracts/governance/extensions/GovernorCountingSimple.sol (1 hunks)
  • contracts/governance/extensions/GovernorVotes.sol (1 hunks)
  • contracts/governance/utils/Votes.sol (1 hunks)
  • contracts/interfaces/IERC6372.sol (1 hunks)
  • contracts/mocks/MultipleInheritanceInitializableMocks.sol (6 hunks)
  • contracts/mocks/Stateless.sol (1 hunks)
  • contracts/mocks/VotesExtendedMock.sol (1 hunks)
  • contracts/mocks/VotesMock.sol (1 hunks)
  • contracts/mocks/account/AccountMock.sol (0 hunks)
  • contracts/mocks/account/modules/ERC7579Mock.sol (1 hunks)
  • contracts/mocks/docs/governance/MyTokenTimestampBased.sol (1 hunks)
  • contracts/mocks/governance/GovernorNoncesKeyedMock.sol (0 hunks)
  • contracts/mocks/token/ERC20BridgeableMock.sol (1 hunks)
  • contracts/mocks/token/ERC20VotesAdditionalCheckpointsMock.sol (1 hunks)
  • contracts/mocks/token/ERC20VotesTimestampMock.sol (2 hunks)
  • contracts/mocks/utils/cryptography/ERC7739Mock.sol (0 hunks)
  • contracts/proxy/utils/UUPSUpgradeable.sol (1 hunks)
  • contracts/token/ERC20/extensions/ERC20Permit.sol (1 hunks)
  • contracts/token/ERC20/extensions/IERC20Permit.sol (1 hunks)
  • contracts/utils/Packing.sol (1 hunks)
  • contracts/utils/cryptography/EIP712.sol (2 hunks)
  • contracts/utils/cryptography/signers/draft-ERC7739.sol (0 hunks)
  • contracts/vendor/compound/ICompoundTimelock.sol (1 hunks)
  • package.json (1 hunks)
  • scripts/generate/templates/Packing.js (1 hunks)
  • slippy.config.js (1 hunks)
  • test/account/utils/draft-ERC7579Utils.t.sol (1 hunks)
  • test/governance/Governor.t.sol (1 hunks)
  • test/governance/extensions/GovernorSuperQuorumGreaterThanQuorum.t.sol (2 hunks)
💤 Files with no reviewable changes (5)
  • contracts/mocks/governance/GovernorNoncesKeyedMock.sol
  • contracts/account/extensions/draft-AccountERC7579.sol
  • contracts/mocks/utils/cryptography/ERC7739Mock.sol
  • contracts/utils/cryptography/signers/draft-ERC7739.sol
  • contracts/mocks/account/AccountMock.sol
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-29T13:16:08.640Z
Learnt from: Amxx
PR: OpenZeppelin/openzeppelin-contracts#5904
File: contracts/mocks/crosschain/ERC7786RecipientMock.sol:12-14
Timestamp: 2025-08-29T13:16:08.640Z
Learning: In OpenZeppelin contracts, mock contracts (like ERC7786RecipientMock) don't require input validation such as zero-address checks in constructors, as they are only used for testing purposes in controlled environments.

Applied to files:

  • contracts/mocks/token/ERC20BridgeableMock.sol
🪛 GitHub Check: codespell
package.json

[failure] 67-67:
slippy ==> slippery

⏰ 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). (5)
  • GitHub Check: tests-foundry
  • GitHub Check: tests
  • GitHub Check: slither
  • GitHub Check: coverage
  • GitHub Check: halmos
🔇 Additional comments (15)
contracts/utils/Packing.sol (1)

36-36: LGTM: file-wide naming-convention suppression for generated code.

Appropriate for auto-generated mixedcase/underscored helpers; matches the existing solhint disable.

contracts/mocks/token/ERC20BridgeableMock.sol (1)

5-5: Nice cleanup: removed unused ERC20 import.

No behavior change; keeps the mock lean.

contracts/proxy/utils/UUPSUpgradeable.sol (1)

21-21: LGTM: targeted naming suppression for __self.

Keeps semantics intact and matches local convention for private immutables with double underscores.

contracts/interfaces/IERC6372.sol (1)

16-16: LGTM: inline naming-convention suppression for CLOCK_MODE.

No interface or runtime impact.

test/account/utils/draft-ERC7579Utils.t.sol (1)

8-8: LGTM: added Slippy no-console suppression mirroring Solhint.

Keeps test logging pragmas explicit.

contracts/governance/utils/Votes.sol (1)

67-67: LGTM: appropriate Slippy suppression for ERC-6372 name

Using slippy-disable-previous-line to exempt CLOCK_MODE from naming-convention is appropriate here.

contracts/governance/extensions/GovernorCountingFractional.sol (1)

59-61: LGTM: suppression limited to the nonconforming COUNTING_MODE identifier

The localized Slippy disable is appropriate and keeps scope tight.

contracts/mocks/VotesMock.sol (1)

40-41: LGTM: scoped suppression for CLOCK_MODE

Looks good; no behavioral impact.

contracts/mocks/VotesExtendedMock.sol (1)

40-42: LGTM: minimal, localized disable

OK to suppress naming rule for this ERC-style function.

contracts/governance/IGovernor.sol (1)

202-202: LGTM: trailing suppression on declaration is clear and contained

The inline disable is the cleanest approach on interfaces.

contracts/mocks/account/modules/ERC7579Mock.sol (1)

80-82: Named return params: ABI-safe and clearer.

Good switch to named returns; no ABI change and it satisfies the linter rule.

slippy.config.js (2)

45-64: Rule precedence: internal vs library-internal functions.

You correctly forbid leading underscores for internal library functions (Lines 60-64) while requiring them for other internal functions (Lines 53-57). Ensure Slippy applies the most specific rule last; keeping the library-specific rule after the general internal rule (as you have) should give the desired precedence.

Run Slippy once on a representative library with an internal function named _foo to confirm it’s flagged, while a non-library internal _foo passes.


80-106: Sanity-check rule enablement without options (id-denylist).

'id-denylist': 'error' is enabled but no deny list is provided in this config. If Slippy expects an explicit list, this will be a no-op (or could error unexpectedly). Either configure denylisted ids or turn it off to avoid confusion.

You can confirm behavior by running the linter once and checking whether any identifiers are reported without a configured list. I can help wire a minimal deny list if desired.

contracts/token/ERC20/extensions/IERC20Permit.sol (1)

89-89: Targeted suppression is fine.

Inline disable on DOMAIN_SEPARATOR() is appropriate given the mandated mixed-case name; no functional impact.

test/governance/Governor.t.sol (1)

39-39: Consistent, minimal suppressions for constant-style accessors.

The per-line disables on CLOCK_MODE and COUNTING_MODE keep tests clean without broad exemptions.

Also applies to: 42-42

Comment on lines +33 to +44
selector: 'stateVariable',
format: ['camelCase', 'UPPER_CASE'],
modifiers: ['private', 'constant'],
leadingUnderscore: 'forbid',
},
// non-constant private state variables should have a leading underscore
{
selector: 'stateVariable',
format: ['camelCase', 'UPPER_CASE'],
modifiers: ['private'],
leadingUnderscore: 'require',
},
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Resolve conflicting stateVariable rules (constant/private vs. private non-constant).

As written, private non-constant state vars can be UPPER_CASE, and private constants allow camelCase, which contradicts typical conventions and the comment intent. Tighten formats as below.

   {
     selector: 'stateVariable',
-    format: ['camelCase', 'UPPER_CASE'],
+    format: ['UPPER_CASE'],
     modifiers: ['private', 'constant'],
     leadingUnderscore: 'forbid',
   },
   // non-constant private state variables should have a leading underscore
   {
     selector: 'stateVariable',
-    format: ['camelCase', 'UPPER_CASE'],
+    format: ['camelCase'],
     modifiers: ['private'],
     leadingUnderscore: 'require',
   },
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
selector: 'stateVariable',
format: ['camelCase', 'UPPER_CASE'],
modifiers: ['private', 'constant'],
leadingUnderscore: 'forbid',
},
// non-constant private state variables should have a leading underscore
{
selector: 'stateVariable',
format: ['camelCase', 'UPPER_CASE'],
modifiers: ['private'],
leadingUnderscore: 'require',
},
{
selector: 'stateVariable',
format: ['UPPER_CASE'],
modifiers: ['private', 'constant'],
leadingUnderscore: 'forbid',
},
// non-constant private state variables should have a leading underscore
{
selector: 'stateVariable',
format: ['camelCase'],
modifiers: ['private'],
leadingUnderscore: 'require',
},
🤖 Prompt for AI Agents
In slippy.config.js around lines 33 to 44, the two stateVariable rules conflict:
the private+constant rule allows camelCase and UPPER_CASE (but should only allow
UPPER_CASE for constants) and the private (non-constant) rule allows UPPER_CASE
(but should only allow camelCase). Update the formats so the private+constant
rule uses format: ['UPPER_CASE'] only, and the private (non-constant) rule uses
format: ['camelCase'] only, keeping leadingUnderscore: 'require' for the
non-constant private rule and leadingUnderscore: 'forbid' for the constant
private rule.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant