Add a `sherif` monorepo linter to the CI #452

mmv08 · 2024-06-30T10:30:30Z

The PR introduces the 'sherif' linter, which is an opinionated, zero-config linter designed specifically for JavaScript monorepos. This addition aims to improve code quality and standardize development practices across the safe-modules project.

CI Integration:
The PR adds 'sherif' to the CI workflow. This ensures that the linter runs automatically on pull requests, helping to catch potential issues early in the development process.
Linter Features:

'Sherif' offers several features:

Zero-configuration setup, making it easy to integrate and use
Fast performance, as it doesn't require node_modules to be installed
Compatibility with various package managers (PNPM, NPM, Yarn)
Ability to prevent regressions by enforcing consistent dependency versions across the monorepo

Found and fixed issues:

2 issues found in ./package.json:

 ⨯ error The root package.json should be private to prevent accidentaly publishing it to a registry. root-package-private-field
  │ {
  +   "private": "true"   ← missing private field.
  │ }

 ⨯ error The root package.json should specify the package manager and version to use. Useful for tools like corepack. root-package-manager-field
  │ {
  +   "packageManager": "..."   ← missing packageManager field.
  │ }

1 issue found in ./packages/4337-provider/package.json:

 ⨯ error Private packages shouldn't have @types/* in dependencies. types-in-dependencies
  │ {
  │   "private": "true",     ← package is private...
  │   ...
  -   "dependencies": {      ← but has @types/* in dependencies...
  -      "@types/node": "...",
  -   },
  │   ...
  +   "devDependencies": {   ← instead of devDependencies.
  +      "@types/node": "...",
  +   }
  │ }

13 issues found in ./:

 ⨯ error Dependency @account-abstraction/contracts has multiple versions defined in the workspace. multiple-dependency-versions
  ./examples
      4337-passkeys             0.7.0   ↓ lowest
  ./modules
      4337                      ^0.7.0   ↑ highest
      passkey                   ^0.7.0   ↑ highest
  ./packages
      4337-local-bundler        ^0.7.0   ↑ highest

 ⨯ error Dependency @safe-global/safe-contracts has multiple versions defined in the workspace. multiple-dependency-versions
  ./examples
      4337-passkeys             ^1.4.1-build.0   ↑ highest
  ./modules
      4337                      ^1.4.1-build.0   ↑ highest
      passkey                   ^1.4.1-build.0   ↑ highest
      recovery                  =1.4.1-build.0   ∼ between
  ./packages
      4337-local-bundler        ^1.4.1-build.0   ↑ highest

 ⨯ error Dependency ethers has multiple versions defined in the workspace. multiple-dependency-versions
  ./examples
      4337-gas-metering         ^6.12.1   ↓ lowest
  ./modules
      allowances                ^6.12.1   ↓ lowest
      passkey                   ^6.12.1   ↓ lowest
      recovery                  ^6.12.1   ↓ lowest
  ./packages
      4337-local-bundler        ^6.12.1   ↓ lowest
      4337-provider             ^6.12.1   ↓ lowest
  ./examples
      4337-passkeys             ^6.13.1   ↑ highest
  ./modules
      4337                      ^6.13.1   ↑ highest

 ⨯ error Dependency hardhat has multiple versions defined in the workspace. multiple-dependency-versions
  ./modules
      allowances                ^2.22.3   ↓ lowest
      passkey                   ^2.22.3   ↓ lowest
      recovery                  ^2.22.3   ↓ lowest
  ./packages
      4337-local-bundler        ^2.22.3   ↓ lowest
  ./modules
      4337                      ^2.22.5   ↑ highest

 ⨯ error Dependency typescript has multiple versions defined in the workspace. multiple-dependency-versions
  ./examples
      4337-gas-metering         ^5.4.5   ↓ lowest
  ./modules
      allowances                5.4.5   ∼ between
      passkey                   ^5.4.5   ↓ lowest
      recovery                  ^5.4.5   ↓ lowest
  ./packages
      4337-local-bundler        ^5.4.5   ↓ lowest
      4337-provider             ^5.4.5   ↓ lowest
  ./examples
      4337-passkeys             ^5.5.2   ↑ highest
  ./modules
      4337                      ^5.5.2   ↑ highest

 ⨯ error Dependency @types/node has multiple versions defined in the workspace. multiple-dependency-versions
  ./examples
      4337-gas-metering         20.14.0   ↓ lowest
  ./modules
      allowances                ^20.14.0   ∼ between
      passkey                   ^20.14.0   ∼ between
      recovery                  ^20.14.0   ∼ between
  ./packages
      4337-provider             ^20.14.0   ∼ between
  ./modules
      4337                      ^20.14.8   ↑ highest

 ⨯ error Dependency @openzeppelin/contracts has multiple versions defined in the workspace. multiple-dependency-versions
  ./modules
      recovery                  =4.9.6   ↓ lowest
      passkey                   ^5.0.0   ∼ between
      4337                      ^5.0.2   ↑ highest
      allowances                ^5.0.2   ↑ highest

 ⨯ error Dependency @safe-global/safe-deployments has multiple versions defined in the workspace. multiple-dependency-versions
  ./modules
      allowances                ^1.36.0   ↓ lowest
  ./examples
      4337-passkeys             ^1.37.0   ↑ highest

 ⨯ error Dependency @types/mocha has multiple versions defined in the workspace. multiple-dependency-versions
  ./modules
      allowances                ^10.0.6   ↓ lowest
      4337                      ^10.0.7   ↑ highest

 ⨯ error Dependency dotenv has multiple versions defined in the workspace. multiple-dependency-versions
  ./examples
      4337-gas-metering         16.4.5   ↓ lowest
  ./modules
      4337                      ^16.4.5   ↑ highest
      allowances                ^16.4.5   ↑ highest
      passkey                   ^16.4.5   ↑ highest
      recovery                  ^16.4.5   ↑ highest

 ⨯ error Dependency solhint has multiple versions defined in the workspace. multiple-dependency-versions
  ./modules
      4337                      ^5.0.1   ↑ highest
      allowances                5.0.1   ∼ between
      passkey                   ^5.0.1   ↑ highest

 ⨯ error Dependency @nomicfoundation/hardhat-network-helpers has multiple versions defined in the workspace. multiple-dependency-versions
  ./modules
      passkey                   ^1.0.10   ↓ lowest
      4337                      ^1.0.11   ↑ highest

 ⨯ error Dependency @simplewebauthn/server has multiple versions defined in the workspace. multiple-dependency-versions
  ./modules
      4337                      10.0.0   ↓ lowest
      passkey                   ^10.0.0   ↑ highest

coveralls · 2024-07-05T10:43:45Z

Pull Request Test Coverage Report for Build 9807067578

Details

0 of 0 changed or added relevant lines in 0 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage remained the same at 100.0%

Totals
Change from base Build 9806149214:	0.0%
Covered Lines:	39
Relevant Lines:	39

💛 - Coveralls

coveralls · 2024-07-05T10:43:59Z

Pull Request Test Coverage Report for Build 9807067573

Details

0 of 0 changed or added relevant lines in 0 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage remained the same at 100.0%

Totals
Change from base Build 9806149214:	0.0%
Covered Lines:	90
Relevant Lines:	90

💛 - Coveralls

coveralls · 2024-07-05T10:47:40Z

Pull Request Test Coverage Report for Build 9807114818

Details

0 of 0 changed or added relevant lines in 0 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage remained the same at 100.0%

Totals
Change from base Build 9806149214:	0.0%
Covered Lines:	39
Relevant Lines:	39

💛 - Coveralls

coveralls · 2024-07-05T10:47:55Z

Pull Request Test Coverage Report for Build 9807114812

Details

0 of 0 changed or added relevant lines in 0 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage remained the same at 100.0%

Totals
Change from base Build 9806149214:	0.0%
Covered Lines:	90
Relevant Lines:	90

💛 - Coveralls

coveralls · 2024-07-05T11:01:25Z

Pull Request Test Coverage Report for Build 9807254097

Details

0 of 0 changed or added relevant lines in 0 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage remained the same at 100.0%

Totals
Change from base Build 9807171647:	0.0%
Covered Lines:	39
Relevant Lines:	39

💛 - Coveralls

coveralls · 2024-07-05T11:01:42Z

Pull Request Test Coverage Report for Build 9807254091

Details

0 of 0 changed or added relevant lines in 0 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage remained the same at 100.0%

Totals
Change from base Build 9807171647:	0.0%
Covered Lines:	90
Relevant Lines:	90

💛 - Coveralls

nlordell · 2024-07-05T11:10:21Z

modules/4337/package.json

@@ -47,15 +47,15 @@
    "url": "https://github.com/safe-global/safe-modules/issues"
  },
  "devDependencies": {
-    "@account-abstraction/contracts": "^0.7.0",
+    "@account-abstraction/contracts": "0.7.0",


Should this be a dependency? I think we import types from it when building the contract:

safe-modules/modules/4337/contracts/Safe4337Module.sol

Lines 6 to 9 in bb5e1f7

import {IAccount} from "@account-abstraction/contracts/interfaces/IAccount.sol";

import {PackedUserOperation} from "@account-abstraction/contracts/interfaces/PackedUserOperation.sol";

import {_packValidationData} from "@account-abstraction/contracts/core/Helpers.sol";

import {UserOperationLib} from "@account-abstraction/contracts/core/UserOperationLib.sol";

nlordell · 2024-07-05T11:15:19Z

modules/recovery/package.json

    "yargs": "^17.7.2"
  },
  "dependencies": {
    "candide-contracts": "github:5afe/CandideWalletContracts#113d3c059e039e332637e8f686d9cbd505f1e738",
    "@openzeppelin/contracts": "=4.9.6",
-    "@safe-global/safe-contracts": "=1.4.1-build.0"
+    "@safe-global/safe-contracts": "^1.4.1-build.0"


I wonder if we should use exact dependencies for these:

Suggested change

"@safe-global/safe-contracts": "^1.4.1-build.0"

"@safe-global/safe-contracts": "=1.4.1-build.0"

They affect the build and ultimately the bytecode - so not doing so could lead to issues where packages that depend on the contracts here build with different bytecode than what the contract build originally.

Same applies for other @safe-global/safe-contracts and @account-abstraction/contracts dependencies elsewhere.

So I've done this in the recovery package (technically, we shouldn't have this dependency here, but it's listed as a dev dependency in their repo) and the 4337 one. I proposed a change to them in our shared Slack channel.

nlordell · 2024-07-05T11:16:24Z

.github/workflows/ci.yml

+          pnpm install
+          pnpm run fmt:global-check
+          # -i is to ignore packages that are expected to have multiple versions across the workspace
+          npx sherif@latest -i @openzeppelin/contracts


I'm not a fan of this - it means that CI can break from one run to another because of new sherif rules.

Why not add it as a dependency to the root of the repository? It can even be part of the fmt:global-check in order to be easier to run locally.

This is their recommended way of running it: https://github.com/QuiiBz/sherif

I agree using latest is a mistake, I'll pin it to a specific version.

It can even be part of the fmt:global-check

It can be, but I am unsure if it's related to formatting 🤷

It can be, but I am unsure if it's related to formatting 🤷

😅 - it definitely isn't. What I meant more was that we can use NPM scripts to run both checks with a single command. A wise @mmv08 once taught me that this is the way :P

Pinned the version sir

I migrated it to the package.json file, and one thing that I missed straight away is the comments 🙈 I have no way to explain what the -i flag does and why certain packages are excluded

git blame 😛

True that JSON doesn't allow for comments which is annoying 😞

coveralls · 2024-07-05T12:46:27Z

Pull Request Test Coverage Report for Build 9808488628

Details

0 of 0 changed or added relevant lines in 0 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage remained the same at 100.0%

Totals
Change from base Build 9807171647:	0.0%
Covered Lines:	39
Relevant Lines:	39

💛 - Coveralls

nlordell · 2024-07-05T12:47:55Z

examples/4337-gas-metering/package.json

    "permissionless": "0.1.29",
    "viem": "2.12.5"
  },
  "devDependencies": {
-    "@types/node": "20.14.0",
+    "@types/node": "^20.14.8",
    "tsx": "4.11.0",


Unrelated nit that I'm just noticing now: it feels weird that we write dependencies differently for different packages in our repository. Here, we are using "x.y.z", while in others we are using "^x.y.z", even though there is nothing special about these dependencies for expressing them differently from a semver perspective.

Not for this PR, just something that I wanted to bring up.

Maybe there's a purpose, the problem is that there are no comments allowed in the package.json, so that knowledge may be lost. There are some hacky solutions though: https://stackoverflow.com/questions/14221579/how-do-i-add-comments-to-package-json-for-npm-install

These packages are getting pinned by the lock file. The places where versioning matters more are for dependencies, because those get transitively installed.

I think that git blame might be able to include documentation for these.

modules/passkey/package.json

modules/4337/package.json

nlordell · 2024-07-08T08:26:53Z

package.json

@@ -39,6 +42,7 @@
    "eslint-plugin-react-refresh": "^0.4.7",
    "prettier": "^3.3.0",
    "prettier-plugin-solidity": "^1.3.1",
-    "rimraf": "^5.0.7"
+    "rimraf": "^5.0.7",
+    "sherif": "0.9.0"


Suggested change

"sherif": "0.9.0"

"sherif": "^0.9.0"

They may not be following semver right?

Don't think that matters - we have a lock file, so the version gets pinned anyway. Also I think this version should follow semver (not super clear, but the docs say that it "must be parseable by node-semver", which itself is a semantic versioning package for NodeJS).

nlordell · 2024-07-08T08:28:30Z

One more question here: #452 (comment), but approving to reduce back-and-forth.

coveralls · 2024-07-08T15:24:57Z

Pull Request Test Coverage Report for Build 9842375074

Details

0 of 0 changed or added relevant lines in 0 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage remained the same at 100.0%

Totals
Change from base Build 9807474017:	0.0%
Covered Lines:	90
Relevant Lines:	90

💛 - Coveralls

nlordell · 2024-07-09T10:44:43Z

modules/passkey/package.json

-    "@openzeppelin/contracts": "^5.0.0",
-    "@safe-global/safe-contracts": "^1.4.1-build.0",
+    "@account-abstraction/contracts": "0.7.0",
+    "@openzeppelin/contracts": "^5.0.2",


This is a transitive dependency from account-abstraction (whose package has a bug and doesn't correctly include the contracts as a dependency) and uses version 5.0.0:

https://github.com/eth-infinitism/account-abstraction/blob/7af70c8993a6f42973f520ae0752386a5032abe7/yarn.lock#L901-L904

I would suggest marking this as:

Suggested change

"@openzeppelin/contracts": "^5.0.2",

"@openzeppelin/contracts": "=5.0.0",

To indicate this is an exact version on purpose.

Actually, they do include it as a dependency (I created a PR to fix it). We have it because hardhat can't resolve pnpm modules correctly: NomicFoundation/hardhat#4292

Ah, good find.

Should we use 5.0.0 though, so it matches the version in the repository?

We only use the AA dependency in tests and do not depend on the same bytecode, so I think it's fine. Openzeppelin package is also following semver so I think it's in the allowed version range.

Oops, I accidentally commited the fix. Still good with me though

Wait - if its only used in tests, then I think it should be a devDependency. I'm so confused 😭

Co-authored-by: Nicholas Rodrigues Lordello <nick@safe.global>

nlordell

Approving once again - I think some of the discussions around dependencies should happen, but they don't need to for this PR to merge since this is only about adding sherif for linting (so I'm sorry I keep on bringing them up 🙈).

add a monorepo linter to the ci

e74c7dd

mmv08 force-pushed the ci/add-monorepo-linter branch from 5babfdd to f16e4ff Compare July 5, 2024 10:40

mmv08 force-pushed the ci/add-monorepo-linter branch from f16e4ff to cf054ff Compare July 5, 2024 10:45

mmv08 changed the title ~~Add a monorepo linter to the ci~~ Add a sherif monorepo linter to the CI Jul 5, 2024

mmv08 marked this pull request as ready for review July 5, 2024 10:51

mmv08 requested a review from a team as a code owner July 5, 2024 10:51

mmv08 requested review from nlordell, akshay-ap and remedcu and removed request for a team July 5, 2024 10:51

Fix monorepo lint errors

2716468

mmv08 force-pushed the ci/add-monorepo-linter branch from cf054ff to 2716468 Compare July 5, 2024 10:58

nlordell reviewed Jul 5, 2024

View reviewed changes

pin dependencies that affect the bytecode

00f76b3

nlordell reviewed Jul 5, 2024

View reviewed changes

modules/passkey/package.json Outdated Show resolved Hide resolved

nlordell reviewed Jul 5, 2024

View reviewed changes

modules/4337/package.json Show resolved Hide resolved

mmv08 force-pushed the ci/add-monorepo-linter branch 2 times, most recently from 3bbf18c to d081be9 Compare July 5, 2024 15:43

nlordell reviewed Jul 8, 2024

View reviewed changes

nlordell approved these changes Jul 8, 2024

View reviewed changes

pin dependencies that affect the bytecode

1582795

mmv08 force-pushed the ci/add-monorepo-linter branch from d081be9 to 1582795 Compare July 8, 2024 15:21

nlordell reviewed Jul 9, 2024

View reviewed changes

nlordell approved these changes Jul 9, 2024

View reviewed changes

mmv08 and others added 2 commits July 9, 2024 14:09

Update modules/passkey/package.json

5084bef

Co-authored-by: Nicholas Rodrigues Lordello <nick@safe.global>

loosen up sheriff version

5ebd689

nlordell approved these changes Jul 9, 2024

View reviewed changes

mmv08 merged commit 0646e46 into main Jul 9, 2024
5 checks passed

mmv08 deleted the ci/add-monorepo-linter branch July 9, 2024 13:54

github-actions bot locked and limited conversation to collaborators Jul 9, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a `sherif` monorepo linter to the CI #452

Add a `sherif` monorepo linter to the CI #452

mmv08 commented Jun 30, 2024 •

edited

Loading

coveralls commented Jul 5, 2024 •

edited

Loading

coveralls commented Jul 5, 2024 •

edited

Loading

coveralls commented Jul 5, 2024 •

edited

Loading

coveralls commented Jul 5, 2024 •

edited

Loading

coveralls commented Jul 5, 2024 •

edited

Loading

coveralls commented Jul 5, 2024 •

edited

Loading

nlordell Jul 5, 2024

mmv08 Jul 5, 2024

nlordell Jul 5, 2024

mmv08 Jul 5, 2024

nlordell Jul 5, 2024 •

edited

Loading

mmv08 Jul 5, 2024

nlordell Jul 5, 2024

mmv08 Jul 5, 2024

mmv08 Jul 5, 2024

nlordell Jul 5, 2024

coveralls commented Jul 5, 2024 •

edited

Loading

nlordell Jul 5, 2024

mmv08 Jul 8, 2024

nlordell Jul 9, 2024

nlordell Jul 8, 2024

mmv08 Jul 8, 2024

nlordell Jul 9, 2024

nlordell commented Jul 8, 2024

coveralls commented Jul 8, 2024

nlordell Jul 9, 2024

mmv08 Jul 9, 2024

nlordell Jul 9, 2024

mmv08 Jul 9, 2024

mmv08 Jul 9, 2024

nlordell Jul 9, 2024 •

edited

Loading

nlordell left a comment

	import {IAccount} from "@account-abstraction/contracts/interfaces/IAccount.sol";
	import {PackedUserOperation} from "@account-abstraction/contracts/interfaces/PackedUserOperation.sol";
	import {_packValidationData} from "@account-abstraction/contracts/core/Helpers.sol";
	import {UserOperationLib} from "@account-abstraction/contracts/core/UserOperationLib.sol";

	"@safe-global/safe-contracts": "^1.4.1-build.0"
	"@safe-global/safe-contracts": "=1.4.1-build.0"

	"@openzeppelin/contracts": "^5.0.2",
	"@openzeppelin/contracts": "=5.0.0",

Add a sherif monorepo linter to the CI #452

Add a sherif monorepo linter to the CI #452

Conversation

mmv08 commented Jun 30, 2024 • edited Loading

coveralls commented Jul 5, 2024 • edited Loading

Pull Request Test Coverage Report for Build 9807067578

Details

💛 - Coveralls

coveralls commented Jul 5, 2024 • edited Loading

Pull Request Test Coverage Report for Build 9807067573

Details

💛 - Coveralls

coveralls commented Jul 5, 2024 • edited Loading

Pull Request Test Coverage Report for Build 9807114818

Details

💛 - Coveralls

coveralls commented Jul 5, 2024 • edited Loading

Pull Request Test Coverage Report for Build 9807114812

Details

💛 - Coveralls

coveralls commented Jul 5, 2024 • edited Loading

Pull Request Test Coverage Report for Build 9807254097

Details

💛 - Coveralls

coveralls commented Jul 5, 2024 • edited Loading

Pull Request Test Coverage Report for Build 9807254091

Details

💛 - Coveralls

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nlordell Jul 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Jul 5, 2024 • edited Loading

Pull Request Test Coverage Report for Build 9808488628

Details

💛 - Coveralls

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nlordell commented Jul 8, 2024

coveralls commented Jul 8, 2024

Pull Request Test Coverage Report for Build 9842375074

Details

💛 - Coveralls

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nlordell Jul 9, 2024 • edited Loading

Choose a reason for hiding this comment

nlordell left a comment

Choose a reason for hiding this comment

Add a `sherif` monorepo linter to the CI #452

Add a `sherif` monorepo linter to the CI #452

mmv08 commented Jun 30, 2024 •

edited

Loading

coveralls commented Jul 5, 2024 •

edited

Loading

coveralls commented Jul 5, 2024 •

edited

Loading

coveralls commented Jul 5, 2024 •

edited

Loading

coveralls commented Jul 5, 2024 •

edited

Loading

coveralls commented Jul 5, 2024 •

edited

Loading

coveralls commented Jul 5, 2024 •

edited

Loading

nlordell Jul 5, 2024 •

edited

Loading

coveralls commented Jul 5, 2024 •

edited

Loading

nlordell Jul 9, 2024 •

edited

Loading