Add a sherif monorepo linter to the CI
#452
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -53,14 +53,14 @@ | |
| "@alchemy/aa-core": "2.3.1", | ||
| "@gelatonetwork/relay-sdk": "^5.5.6", | ||
| "alchemy-sdk": "3.1.2", | ||
| "dotenv": "^16.4.5", | ||
| "ethers": "^6.13.1", | ||
| "permissionless": "0.1.29", | ||
| "viem": "2.12.5" | ||
| }, | ||
| "devDependencies": { | ||
| "@types/node": "^20.14.8", | ||
| "tsx": "4.11.0", | ||
|
There was a problem hiding this comment. 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. There was a problem hiding this comment. 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 There was a problem hiding this comment. These packages are getting pinned by the lock file. The places where versioning matters more are for I think that |
||
| "typescript": "^5.5.2" | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -32,4 +32,4 @@ | |
| "vite": "^5.3.1", | ||
| "vite-plugin-commonjs": "^0.10.1" | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -47,15 +47,15 @@ | |
| "url": "https://github.com/safe-global/safe-modules/issues" | ||
| }, | ||
| "devDependencies": { | ||
| "@account-abstraction/contracts": "0.7.0", | ||
|
There was a problem hiding this comment. 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 |
||
| "@noble/curves": "^1.4.0", | ||
| "@nomicfoundation/hardhat-ethers": "^3.0.6", | ||
| "@nomicfoundation/hardhat-network-helpers": "^1.0.11", | ||
| "@nomicfoundation/hardhat-toolbox": "^5.0.0", | ||
| "@openzeppelin/contracts": "^5.0.2", | ||
| "@safe-global/safe-4337-local-bundler": "workspace:^0.0.0", | ||
| "@safe-global/safe-4337-provider": "workspace:^0.0.0", | ||
| "@simplewebauthn/server": "^10.0.0", | ||
| "@types/chai": "^4.3.16", | ||
| "@types/mocha": "^10.0.7", | ||
| "@types/node": "^20.14.8", | ||
|
|
@@ -76,4 +76,4 @@ | |
| "dependencies": { | ||
| "@safe-global/safe-contracts": "^1.4.1-build.0" | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -48,24 +48,24 @@ | |||||
| "devDependencies": { | ||||||
| "@noble/curves": "^1.4.0", | ||||||
| "@nomicfoundation/hardhat-ethers": "^3.0.6", | ||||||
| "@nomicfoundation/hardhat-network-helpers": "^1.0.11", | ||||||
| "@nomicfoundation/hardhat-toolbox": "^5.0.0", | ||||||
| "@safe-global/mock-contract": "^4.1.0", | ||||||
| "@safe-global/safe-4337": "workspace:^0.3.0-1", | ||||||
| "@safe-global/safe-4337-local-bundler": "workspace:^0.0.0", | ||||||
| "@simplewebauthn/server": "^10.0.0", | ||||||
| "@types/node": "^20.14.8", | ||||||
| "dotenv": "^16.4.5", | ||||||
| "ethers": "^6.13.1", | ||||||
| "hardhat": "^2.22.5", | ||||||
| "hardhat-deploy": "^0.12.4", | ||||||
| "solhint": "^5.0.1", | ||||||
| "ts-node": "^10.9.2", | ||||||
| "typescript": "^5.5.2" | ||||||
| }, | ||||||
| "dependencies": { | ||||||
| "@account-abstraction/contracts": "0.7.0", | ||||||
| "@openzeppelin/contracts": "^5.0.2", | ||||||
|
There was a problem hiding this comment. 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: I would suggest marking this as:
Suggested change
To indicate this is an exact version on purpose. There was a problem hiding this comment. 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 There was a problem hiding this comment. Ah, good find. Should we use 5.0.0 though, so it matches the version in the repository? There was a problem hiding this comment. 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. There was a problem hiding this comment. Oops, I accidentally commited the fix. Still good with me though There was a problem hiding this comment. Wait - if its only used in tests, then I think it should be a |
||||||
| "@safe-global/safe-contracts": "^1.4.1-build.0", | ||||||
mmv08 marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
| "cbor": "^9.0.2" | ||||||
| } | ||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,6 +2,8 @@ | |
| "name": "@safe-global/safe-modules", | ||
| "version": "1.0.0", | ||
| "description": "A monorepo for Safe modules", | ||
| "private": true, | ||
| "packageManager": "pnpm@9.4.0", | ||
| "main": "dist/index.js", | ||
| "workspaces": [ | ||
| "packages/*", | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -15,14 +15,14 @@ | |
| "prepare": "pnpm run build" | ||
| }, | ||
| "dependencies": { | ||
| "@account-abstraction/contracts": "0.7.0", | ||
| "@nomicfoundation/hardhat-toolbox": "^5.0.0", | ||
| "@safe-global/safe-4337-provider": "workspace:^0.0.0", | ||
| "@safe-global/safe-contracts": "^1.4.1-build.0", | ||
| "ethers": "^6.13.1", | ||
| "hardhat": "^2.22.5", | ||
| "hardhat-deploy": "^0.12.4", | ||
| "node-fetch": "^3.3.2", | ||
| "typescript": "^5.5.2" | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -11,9 +11,11 @@ | |
| "prepare": "pnpm run build" | ||
| }, | ||
| "dependencies": { | ||
| "ethers": "^6.13.1", | ||
| "rimraf": "^5.0.7", | ||
| "typescript": "^5.5.2" | ||
| }, | ||
| "devDependencies": { | ||
| "@types/node": "^20.14.8" | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not a fan of this - it means that CI can break from one run to another because of new
sherifrules.Why not add it as a dependency to the root of the repository? It can even be part of the
fmt:global-checkin order to be easier to run locally.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is their recommended way of running it: https://github.com/QuiiBz/sherif
I agree using
latestis a mistake, I'll pin it to a specific version.It can be, but I am unsure if it's related to formatting 🤷
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😅 - 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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pinned the version sir
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
-iflag does and why certain packages are excludedThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
git blame😛True that JSON doesn't allow for comments which is annoying 😞