Skip to content

Modernise and cleanup lint configuration and build process #1177

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 24 commits into
base: main
Choose a base branch
from

Conversation

RembrandtK
Copy link
Contributor

@RembrandtK RembrandtK commented May 21, 2025

NOT READY YET, trying to resolve build failures, due to version conflicts I think.

  1. Migrated eslint-graph-config and solhint-graph-config to be configuration files in repo root that are inherited from for packages. It is presumed we do not need to maintain these config packages for external dependencies, and they have been deleted.
  2. Upgraded linting engines (ESLint, Solhint, Prettier) and added linting of Markdown, JSON, and YAML.
  3. Upgraded to Node version 20 (from 18). Might not work anymore when using Node version 18.
  4. Consistently made Prettier the last to process during linting, upgrading Prettier config to match ESLint config and avoid conflicts.
  5. Changed import ordering to be automated (by running yarn lint) using eslint-plugin-simple-import-sort. Also sorts exports.
  6. Dropped no-secrets ESLint plugin due to version conflicts and low signal to noise ratio.
  7. Upgraded Yarn and resolved dependency version clashes.
  8. Upgraded and simplified Husky pre-commit behaviour.
  9. Currently using repo root .gitignore as single source for lint files to ignore. This is imperfect, and might need to be subsequently adjusted and revisited.
  10. Attempted to preserve Lint/Prettier config parameters, however I think import and export sorting have changed, and in some cases dangling commas will be added where they were previously removed (an improvement IMO).
  11. Upgraded linting engines mean more issues are detected, and likely some changes in behaviour.
  12. Added incremental and cached linting and building in various places.
  13. Bumping versions of contracts and token-distribution as the TS interfaces are not backwards compatible, and minor of other packages as should be considered a different version now.
  14. GitHub workflow to run lints. (Likely needs some fixes at time of writing, being tested with this PR.)
  15. (Ideally some of the configuration pattern should be documented, not yet part of this PR.)

All tests still pass, but I had to make changes to token-distribution tests to get them to pass. I suspect the new version of Hardhat does not allow negative changes to block time, causing exceptions in test execution. The tests log warnings that, if we need to revisist token-distribution in the future, should be investigated to ensure the tests are effective.

Although the tests pass, due to multiple substantial version upgrades and build changes this PR could cause subtle issues that have not been discovered. I expect this upgrade is not perfect, please contact me if you notice an introduced issue, however I recommend that (after sanity testing by others in their environment) we make the upgrade and seek to 'fail-forward' on

Copy link

socket-security bot commented May 21, 2025

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

Diff Package Supply Chain
Security
Vulnerability Quality Maintenance License
Added@​types/​chai@​4.3.201001007588100
Added@​openzeppelin/​contracts@​3.4.2100759589100
Added@​types/​mocha@​10.0.101001007580100
Added@​types/​mocha@​9.1.11001007680100
Addeddotenv@​16.5.010010010086100
Added@​openzeppelin/​contracts-upgradeable@​3.4.2100889789100
Addedgraphql@​16.11.010010010098100

View full report

Copy link

openzeppelin-code bot commented May 21, 2025

Modernise and cleanup lint configuration and build process

Generated at commit: 6c9aae44505ca3bac58372e9a9502393ef7e1484

🚨 Report Summary

Severity Level Results
Contracts Critical
High
Medium
Low
Note
Total
2
4
0
15
37
58
Dependencies Critical
High
Medium
Low
Note
Total
0
0
0
0
0
0

For more details view the full report in OpenZeppelin Code Inspector

@RembrandtK RembrandtK requested a review from Copilot May 22, 2025 17:09
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR modernises the repository’s lint and build setup by consolidating config files at the root, upgrading linting engines and Node, and streamlining scripts and workflows.

  • Centralise ESLint, Prettier, Solhint, Markdown and YAML configs in the monorepo root
  • Upgrade to Node 20, modernise package.json scripts, Yarn workspaces and Husky hooks
  • Refactor Hardhat configuration, import sorting, and incremental lint/build caching

Reviewed Changes

Copilot reviewed 84 out of 84 changed files in this pull request and generated no comments.

Show a summary per file
File Description
packages/contracts/scripts/analyze Updated Slither’s artifacts directory to ./artifacts
packages/contracts/prettier.config.js Removed inherited config; now uses root config
packages/contracts/prettier.config.cjs Added local wrapper extending root Prettier config
packages/contracts/package.json Bumped version, overhauled scripts, deps and files
packages/contracts/hardhat.config.ts Refactored task loading, networks, compiler and plugins
packages/contracts/eslint.config.js Deleted legacy ESLint wrapper (now handled at root)
packages/contracts/contracts/tests/arbitrum/ArbSysMock.sol Added ArbSys mock for L2 testing
packages/contracts/addresses.json Added local 1337 addresses, inlined initArgs
packages/contracts/.solhintignore Removed obsolete Solhint ignore file
packages/contracts/.markdownlint.json Added package-specific MarkdownLint config
package.json Upgraded workspace scripts, devDeps, lint-staged
eslint.config.mjs Added root ESLint monorepo config with ignore logic
.yarnrc.yml Adjusted Yarn settings
.yarn/patches/typechain-npm-8.3.2-b02e27439e.patch Patched TypeChain to await async output in IO step
.husky/pre-commit Simplified hook to run top-level lint-staged
.github/workflows/lint.yml Introduced comprehensive multi-linter GitHub Action
.github/actions/setup/action.yml Upgraded Node.js setup to v20
.solhint.json Removed prettier/prettier rule from Solhint config
.markdownlint.json Added root MarkdownLint rules
.yamllint Added YAMLLint configuration
Comments suppressed due to low confidence (3)

packages/contracts/hardhat.config.ts:116

  • The network key localnitrol1 appears to have a typo and is inconsistent—consider renaming it to something like localNitro1 or a clearer name (e.g. localArbitrumOne).
localnitrol1: {

packages/contracts/.solhintignore:1

  • Removing the .solhintignore file means Solhint will no longer ignore these directories; consider migrating those ignore patterns into your root .solhint.json to prevent unintended lint failures.
node_modules

package.json:19

  • The lint:md script references .markdownlintignore, but that file isn't added to the repo. Either remove the --ignore-path .markdownlintignore option or add the corresponding ignore file.
"lint:md": "markdownlint --fix --ignore-path .gitignore --ignore-path .markdownlintignore '**/*.md'; prettier -w --cache --log-level warn '**/*.md'",

tmigone
tmigone previously approved these changes May 23, 2025
Copy link
Member

@tmigone tmigone left a comment

Choose a reason for hiding this comment

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

Shared some comments over slack but looks good to me.

Copy link

codecov bot commented May 24, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 86.06%. Comparing base (03d8d2a) to head (6c9aae4).
Report is 18 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1177      +/-   ##
==========================================
- Coverage   92.83%   86.06%   -6.78%     
==========================================
  Files          47       47              
  Lines        2415     2074     -341     
  Branches      438      613     +175     
==========================================
- Hits         2242     1785     -457     
- Misses        173      289     +116     
Flag Coverage Δ
unittests 86.06% <ø> (-6.78%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@RembrandtK
Copy link
Contributor Author

After messy fiddling around, all workflows now pass.

Code coverage has gone down as a result of skipping some problematic tests for coverage. This is a shame, but I think good enough for now, can be improved again over time and when (and if) we need to work on those areas again.

Some more refinements pending from other branches, but if reviewed again I hope good enough to merge now.

@RembrandtK RembrandtK marked this pull request as ready for review May 24, 2025 17:03
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.

2 participants