-
Notifications
You must be signed in to change notification settings - Fork 164
feat: modernize build system and linting infrastructure #1237
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
Conversation
## Build System - Add consistent build:self/build:dep/test:self script patterns across packages - Root builds now call build:self on each package for efficiency - Update CI workflows to use test:self and test:coverage:self - Add codecov.yml configuration for coverage reporting - Move coverage output from reports/coverage to coverage/ ## Linting - Migrate ESLint to flat config (eslint.config.mjs) - Add .solhint.json to all Solidity packages with hierarchical config - Add prettier.config.cjs to contracts/task and contracts/test - Remove natspec-smells linter and configs - Add TODO checking script (check-todos.mjs) - Add solhint disable verification script - Add lint-staged filter for generated files - Improve ignore patterns in .gitignore, .prettierignore, .markdownlintignore ## New Tooling - Add interface ID generation to interfaces package - Add GraphClient extraction to token-distribution - Add bytecode comparison utility script ## Documentation - Add comprehensive build/test script patterns documentation - Add detailed linting configuration guide - Update package table with issuance and badges ## Maintenance - Remove obsolete storage location calculation scripts - Remove remappings.txt (unused) - Standardize author field to "Edge & Node" - Use pnpm catalog references for shared dependencies
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
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.
Pull Request Overview
This PR modernizes the monorepo's build pipeline and linting infrastructure to improve consistency, efficiency, and maintainability across all packages.
- Standardized build/test script patterns with
build:self,build:dep,test:selfpattern - Migrated from legacy
.eslintrc.jsto flat config and removed deprecated natspec-smells linter - Added comprehensive solhint-disable validation with test suite and new utility scripts
Reviewed Changes
Copilot reviewed 76 out of 80 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| scripts/verify-storage-slots.js | Removed obsolete storage verification script |
| scripts/verify-solhint-disables.mjs | Added new validation script for solhint disable comments |
| scripts/verify-solhint-disables.test.mjs | Added comprehensive test suite for solhint validation |
| scripts/utils/storage-locations.js | Removed obsolete storage location utilities |
| scripts/lint-staged-run.sh | Added script to filter generated files from lint-staged |
| scripts/filter-natspec.js | Removed deprecated natspec-smells filter script |
| scripts/compare-repo-contract-bytecode-excluding-metadata.mjs | Added bytecode comparison utility |
| scripts/check-todos.mjs | Added TODO comment detection script |
| scripts/calculate-storage-locations.js | Removed obsolete storage calculation script |
| pnpm-workspace.yaml | Added pnpm catalog for shared dependencies |
| packages/*/package.json | Standardized script patterns and migrated to catalog dependencies |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
feat: modernize build system and linting infrastructure
🚨 Report Summary
For more details view the full report in OpenZeppelin Code Inspector |
Security fixes: - Replace shell command string interpolation with safer argument construction in verify-solhint-disables.mjs - Replace `find` shell command with Node.js fs API (walkDir) to eliminate command injection risk Code quality improvements: - Make metadata stripping regex more precise in bytecode comparison - Replace double negation with clearer null check in build script
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1237 +/- ##
==========================================
+ Coverage 82.84% 84.05% +1.21%
==========================================
Files 47 42 -5
Lines 2093 2070 -23
Branches 620 615 -5
==========================================
+ Hits 1734 1740 +6
+ Misses 359 330 -29
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
The previous attempt to make the metadata regex more precise used an
incorrect pattern that expected 64 hex characters for the IPFS hash,
but the actual CBOR format contains 68 hex characters (34 bytes).
Solidity metadata structure (CBOR-encoded):
- a264697066735822 = {"ipfs": bytes(
- [68 hex chars] = 34-byte IPFS multihash (0x1220 prefix + 32-byte hash)
- 64736f6c63 = "solc"
- [variable] = version bytes
The incorrect regex failed to match, causing metadata to remain in the
bytecode and producing false positive differences (106 contracts
reported as changed when only 2 mock contracts actually changed).
Verified with full comparison: 150 contracts functionally identical,
only 2 test mocks have legitimate changes (added 'indexed' to events).
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.
Pull Request Overview
Copilot reviewed 76 out of 80 changed files in this pull request and generated 1 comment.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Replace InterfaceIdExtractor contract deployment with direct artifact reading. Benefits: - 20x faster: ~0.1s vs ~2-3s (no contract deployment) - Incremental: only regenerates when artifacts change - Simpler: config object instead of maintaining Solidity contract - Succinct output: silent when up-to-date Implementation uses ethers-v5 keccak256 to calculate ERC-165 interface IDs by XORing function selectors from compiled ABI.
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.
Pull Request Overview
Copilot reviewed 75 out of 79 changed files in this pull request and generated 6 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
This PR modernizes the monorepo's build pipeline and linting configuration, improving consistency, efficiency, and maintainability across all packages.
Build System Improvements
build:self/build:dep/test:selfscript namingbuild:selfon each package instead of recursivepnpm -r build, avoiding redundant dependency rebuildstest:selfandtest:coverage:selffor targeted testingreports/coveragetocoverage/and addedcodecov.ymlconfigurationLinting Infrastructure
.eslintrc.jsto flat config (eslint.config.mjs).solhint.jsonto all Solidity packages with hierarchical configuration inheritanceprettier.config.cjsto contracts/task and contracts/test packagescheck-todos.mjs: Detects TODO comments in codeverify-solhint-disables.mjs: Validates solhint disable comments (with comprehensive test suite)lint-staged-run.sh: Filters generated files from lint-staged runs.gitignore,.prettierignore, and.markdownlintignorefor better coverageNew Tooling & Features
Documentation
Maintenance & Cleanup
remappings.txt