-
Notifications
You must be signed in to change notification settings - Fork 158
Baseline upgrades in preparation for service quality contracts PRs #1213
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
base: main
Are you sure you want to change the base?
Conversation
Baseline upgrades in preparation for service quality contracts PRs
🚨 Report Summary
For more details view the full report in OpenZeppelin Code Inspector |
- Remove restrictive rootDir setting that prevented imports from parent directories - Set rootDir to '..' to allow access to parent contracts types - Fixes CI build failure in packages/contracts/task TypeScript compilation Resolves TypeScript error TS6059 where files outside rootDir were being imported
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1213 +/- ##
==========================================
- Coverage 86.06% 83.51% -2.56%
==========================================
Files 47 47
Lines 2074 2074
Branches 613 613
==========================================
- Hits 1785 1732 -53
- Misses 289 342 +53
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:
|
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 implements baseline upgrades in preparation for service quality contracts, focusing on build improvements and global fixing of linting issues. The changes update build tooling, improve dependency management, and establish a clean linting baseline across the codebase.
Key changes include:
- Updated pnpm workspace configuration with centralized dependency catalog
- Build system improvements for token-distribution package with offline GraphClient support
- Global linting configuration upgrades and systematic fixing of Solidity and TypeScript lint issues
Reviewed Changes
Copilot reviewed 175 out of 179 changed files in this pull request and generated 4 comments.
Show a summary per file
File | Description |
---|---|
pnpm-workspace.yaml | Updated workspace configuration with catalog for centralized dependency management |
scripts/*.js | Added verification scripts for solhint-disables and lint-staged runner |
packages/*/package.json | Converted to use catalog dependencies and improved build scripts |
packages/token-distribution/scripts/* | Added GraphClient extraction for offline builds and improved build process |
packages/contracts/contracts/**/*.sol | Applied systematic linting fixes with TODO sections for disabled rules |
packages//test/**/.ts | Updated test configurations and fixed import paths |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.
fs.writeFileSync(tempFile, cleanedContent) | ||
|
||
try { | ||
const result = execSync(`npx solhint ${tempFile} -f json`, { |
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.
The execSync call uses unsanitized file path input which could lead to command injection. Consider using proper path validation or a safer alternative.
const result = execSync(`npx solhint ${tempFile} -f json`, { | |
const result = execFileSync('npx', ['solhint', tempFile, '-f', 'json'], { |
Copilot uses AI. Check for mistakes.
Object.defineProperty(exports, "__esModule", { value: true }); | ||
|
||
// Minimal GraphClient for offline builds - contains only what ops/info.ts uses | ||
const { gql } = require('@graphql-mesh/utils'); |
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.
Hard-coded dependency on '@graphql-mesh/utils' in generated code could break if the dependency is not available. Consider making this configurable or handling the missing dependency gracefully.
const { gql } = require('@graphql-mesh/utils'); | |
// Local no-op gql tag to avoid dependency on @graphql-mesh/utils | |
function gql(strings, ...values) { | |
let str = ''; | |
for (let i = 0; i < strings.length; i++) { | |
str += strings[i]; | |
if (i < values.length) str += values[i]; | |
} | |
return str; | |
} |
Copilot uses AI. Check for mistakes.
const impl = await proxyAdmin.getProxyImplementation(staking.address) | ||
|
||
const factory = await ethers.getContractFactory('StakingExtension') | ||
const factory = await ethers.getContractFactory('contracts/staking/StakingExtension.sol:StakingExtension') |
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.
Hard-coded contract path makes the test brittle to file reorganization. Consider using a constant or helper function to manage contract paths.
const factory = await ethers.getContractFactory('contracts/staking/StakingExtension.sol:StakingExtension') | |
const factory = await ethers.getContractFactory(STAKING_EXTENSION_CONTRACT_PATH) |
Copilot uses AI. Check for mistakes.
} else { | ||
// No API key and no cached artifacts - cannot proceed | ||
// To fix: Set STUDIO_API_KEY or GRAPH_API_KEY environment variable | ||
console.error('❌ No API key or cached GraphClient artifacts available') |
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.
Error message could be more helpful by explaining exactly which environment variables need to be set (STUDIO_API_KEY or GRAPH_API_KEY).
console.error('❌ No API key or cached GraphClient artifacts available') | |
console.error('❌ No API key or cached GraphClient artifacts available. Please set the STUDIO_API_KEY or GRAPH_API_KEY environment variable and try again.') |
Copilot uses AI. Check for mistakes.
Build improvements and global fixing of linting issues.
Many files changes. I should provide a better list of changes and split into separate commits (and perhaps even PRs). Currently short on time, but can improve later if required. At this stage mostly all merged together with history lost.
I will be using this as the baseline for contract PR(s), to use this foundation of update builds and clean lint run.