hardhat config #116
hardhat config #116
Conversation
|
Warning Rate limit exceeded@robotoer has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 14 minutes and 20 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
WalkthroughThe pull request introduces several changes across four main files: Changes
Sequence DiagramsequenceDiagram
participant Deployer
participant EventEmitter
participant Strategy
participant Vault
Deployer->>EventEmitter: Deploy Event Emitter
Deployer->>Vault: Check Underlying Asset
Deployer->>Strategy: Deploy Strategy
Deployer->>Strategy: Configure Parameters
Deployer->>Strategy: Pair with Event Emitter
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
hardhat-scripts/deploy-strategy.ts (1)
13-16: Consider using BigNumber for large integers.
Using JavaScript’s native 'parseInt()' caps the numeric range, which may matter if large values are expected. If you only expect typical integer ranges, this is fine. Otherwise consider using BigNumber-based parsing.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
hardhat-scripts/deploy-strategy.ts(1 hunks)hardhat.config.ts(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
hardhat-scripts/deploy-strategy.ts
[error] 62-62: Do not shadow the global "Proxy" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
🔇 Additional comments (7)
hardhat-scripts/deploy-strategy.ts (4)
8-11: Good use of string splitting for address arrays.
The utility function handles trimming and splitting correctly, avoiding potential spacing issues.
18-24: Well-implemented asset verification.
Comparing underlying assets (case-insensitively) is a robust approach. The logic here is clear and correct.
76-104: Validate addresses before calling setters.
Environment variables like STRATEGY_MANAGEMENT_ADDRESS, KEEPER_ADDRESS, and FEE_RECIPIENT must be valid addresses. Consider adding lightweight runtime checks or fail-fast validations.
121-123: Governor role assignment appears consistent.
This step properly handles transferring governance. Ensure that the final acceptance (e.g., acceptGovernor call) is performed after deployment if required by the contract.
hardhat.config.ts (3)
1-3: Plugins and import structure look good.
No issues found with the plugin imports.
33-33: Exporting the Hardhat config is correct.
Configuration object is properly structured and exported.
4-31: Verify environment variables for network configurations.
For each network, confirm that the relevant RPC URLs and PRIVATE_KEY are valid.
Use this script to confirm references to environment variables in the codebase:
hardhat-scripts/deploy-strategy.ts
Outdated
|
|
||
| console.log("Deployed event emitter impl to:", eventEmitterImpl.address); | ||
|
|
||
| const Proxy = (await ethers.getContractFactory("ERC1967Proxy")).connect(managedSigner); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid shadowing the global "Proxy" property.
Rename this local variable (e.g., ERC1967ProxyFactory) to remove ambiguity.
- const Proxy = (await ethers.getContractFactory("ERC1967Proxy")).connect(managedSigner);
+ const ERC1967ProxyFactory = (await ethers.getContractFactory("ERC1967Proxy")).connect(managedSigner);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const Proxy = (await ethers.getContractFactory("ERC1967Proxy")).connect(managedSigner); | |
| const ERC1967ProxyFactory = (await ethers.getContractFactory("ERC1967Proxy")).connect(managedSigner); |
🧰 Tools
🪛 Biome (1.9.4)
[error] 62-62: Do not shadow the global "Proxy" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #116 +/- ##
=======================================
Coverage 89.23% 89.23%
=======================================
Files 5 5
Lines 604 604
Branches 90 90
=======================================
Hits 539 539
Misses 54 54
Partials 11 11 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
hardhat-scripts/deploy-strategy.ts (3)
35-64:⚠️ Potential issueAdd validation for required environment variables
The function uses TypeScript's non-null assertion operator (
!) but lacks proper validation of environment variables.async function buildStrategyParams( eventEmitter: string, deployer: string, managedSigner: Signer ) { + const requiredEnvVars = { + ASSET_ADDRESS: process.env.ASSET_ADDRESS, + YEARN_VAULT_ADDRESS: process.env.YEARN_VAULT_ADDRESS, + DISCOUNT_RATE_ADAPTER_ADDRESS: process.env.DISCOUNT_RATE_ADAPTER_ADDRESS, + TERM_CONTROLLER_ADDRESS: process.env.TERM_CONTROLLER_ADDRESS, + DISCOUNT_RATE_MARKUP: process.env.DISCOUNT_RATE_MARKUP, + TIME_TO_MATURITY_THRESHOLD: process.env.TIME_TO_MATURITY_THRESHOLD, + REPOTOKEN_CONCENTRATION_LIMIT: process.env.REPOTOKEN_CONCENTRATION_LIMIT, + NEW_REQUIRED_RESERVE_RATIO: process.env.NEW_REQUIRED_RESERVE_RATIO + }; + + const missingVars = Object.entries(requiredEnvVars) + .filter(([_, value]) => !value) + .map(([key]) => key); + + if (missingVars.length > 0) { + throw new Error(`Missing required environment variables: ${missingVars.join(', ')}`); + } const asset = process.env.ASSET_ADDRESS!; // ... rest of the function
78-81:⚠️ Potential issueAvoid shadowing the global "Proxy" property
Rename this local variable to remove ambiguity.
- const Proxy = (await hre.ethers.getContractFactory("ERC1967Proxy")).connect( + const ERC1967ProxyFactory = (await hre.ethers.getContractFactory("ERC1967Proxy")).connect( managedSigner );🧰 Tools
🪛 Biome (1.9.4)
[error] 78-78: Do not shadow the global "Proxy" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
157-162:⚠️ Potential issuePrevent mismatched array lengths in collateral token setup
When splitting environment variables, ensure both arrays have the same length.
+ if (collateralTokens.length !== minCollateralRatios.length) { + throw new Error("COLLATERAL_TOKEN_ADDRESSES and MIN_COLLATERAL_RATIOS must have the same number of entries"); + } for (let i = 0; i < collateralTokens.length; i++) { await strategy.setCollateralTokenParams( collateralTokens[i], minCollateralRatios[i] ); }
🧹 Nitpick comments (2)
hardhat-scripts/deploy-strategy.ts (2)
19-33: Enhance error message with more detailsThe error message could be more informative by including the actual and expected asset addresses.
if (underlyingAsset.toLowerCase() !== asset.toLowerCase()) { - throw new Error("Underlying asset does not match asset"); + throw new Error( + `Underlying asset mismatch. Expected: ${asset}, Got: ${underlyingAsset}` + ); }
168-173: Enhance error handling in script executionImprove error reporting to provide more context about deployment failures.
main() .then(() => process.exit(0)) .catch((error) => { - console.error(error); + console.error('Deployment failed:', { + message: error.message, + stack: error.stack, + // For contract deployment errors + transaction: error.transaction, + receipt: error.receipt + }); process.exit(1); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
hardhat-scripts/deploy-strategy.ts(1 hunks)tsconfig.json(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- tsconfig.json
🧰 Additional context used
🪛 Biome (1.9.4)
hardhat-scripts/deploy-strategy.ts
[error] 78-78: Do not shadow the global "Proxy" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (1)
package.json(2 hunks)
🔇 Additional comments (1)
package.json (1)
8-8: LGTM: env-cmd dependency addition
The addition of env-cmd is appropriate for managing environment-specific configurations during deployment.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
hardhat-scripts/deploy-strategy.ts (3)
32-46: Enhance error message with actual valuesThe error message should include the expected and actual asset addresses to help with debugging.
if (underlyingAsset.toLowerCase() !== asset.toLowerCase()) { - throw new Error("Underlying asset does not match asset"); + throw new Error( + `Underlying asset mismatch. Expected: ${asset}, Actual: ${underlyingAsset}` + ); }
79-112: Add deployment status verificationAdd verification steps after deployment to ensure the event emitter is properly initialized.
await eventEmitterProxy.deployed(); + + // Verify deployment + const deployedEmitter = await hre.ethers.getContractAt( + "TermVaultEventEmitter", + await eventEmitterProxy.getAddress(), + managedSigner + ); + + // Verify initialization + const actualAdmin = await deployedEmitter.admin(); + const actualDevops = await deployedEmitter.devops(); + if (actualAdmin.toLowerCase() !== admin.toLowerCase() || + actualDevops.toLowerCase() !== devops.toLowerCase()) { + throw new Error("Event emitter initialization verification failed"); + } console.log("Deployed event emitter proxy to:", eventEmitterProxy.address);🧰 Tools
🪛 Biome (1.9.4)
[error] 91-91: Do not shadow the global "Proxy" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
130-132: Remove redundant null checkThe
managedSigneris always defined at this point, making the null coalescing operator unnecessary.const Strategy = (await hre.ethers.getContractFactory("Strategy")).connect( - managedSigner ?? null + managedSigner );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
hardhat-scripts/deploy-strategy.ts(1 hunks)package.json(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- package.json
🧰 Additional context used
🪛 Biome (1.9.4)
hardhat-scripts/deploy-strategy.ts
[error] 91-91: Do not shadow the global "Proxy" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
[error] 25-25: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.
(lint/suspicious/noGlobalIsNan)
| function stringToUintArray(input: string): number[] { | ||
| if (!input) return []; | ||
| return input.split(",").map((num) => { | ||
| const trimmed = num.trim(); | ||
| const parsed = parseInt(trimmed); | ||
| if (isNaN(parsed) || parsed.toString() !== trimmed) { | ||
| throw new Error(`Invalid number: ${trimmed}`); | ||
| } | ||
| return parsed; | ||
| }); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace unsafe isNaN with Number.isNaN
The isNaN function performs type coercion which can lead to unexpected results. Use Number.isNaN for safer number validation.
return input.split(",").map((num) => {
const trimmed = num.trim();
const parsed = parseInt(trimmed);
- if (isNaN(parsed) || parsed.toString() !== trimmed) {
+ if (Number.isNaN(parsed) || parsed.toString() !== trimmed) {
throw new Error(`Invalid number: ${trimmed}`);
}
return parsed;
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function stringToUintArray(input: string): number[] { | |
| if (!input) return []; | |
| return input.split(",").map((num) => { | |
| const trimmed = num.trim(); | |
| const parsed = parseInt(trimmed); | |
| if (isNaN(parsed) || parsed.toString() !== trimmed) { | |
| throw new Error(`Invalid number: ${trimmed}`); | |
| } | |
| return parsed; | |
| }); | |
| } | |
| function stringToUintArray(input: string): number[] { | |
| if (!input) return []; | |
| return input.split(",").map((num) => { | |
| const trimmed = num.trim(); | |
| const parsed = parseInt(trimmed); | |
| if (Number.isNaN(parsed) || parsed.toString() !== trimmed) { | |
| throw new Error(`Invalid number: ${trimmed}`); | |
| } | |
| return parsed; | |
| }); | |
| } |
🧰 Tools
🪛 Biome (1.9.4)
[error] 25-25: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.
(lint/suspicious/noGlobalIsNan)
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores