-
Notifications
You must be signed in to change notification settings - Fork 0
Migration setup v2 #14
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
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 aims to update the migration setup by moving tests to Foundry, introducing deployment scripts, and separating utility functions and configuration. Key changes include:
- Updating Merkle tree functions to remove unused parameters.
- Integrating configuration for deployment and event fetching (including a START_BLOCK parameter).
- Updating test utilities and simulation scripts to reflect the new structure and deployment addresses.
Reviewed Changes
Copilot reviewed 31 out of 31 changed files in this pull request and generated 3 comments.
Show a summary per file
File | Description |
---|---|
script/utils/merkle.js | Removed the unused “id” parameter from hashing, proof, and verify functions. |
script/utils/getRoot.js | Updated to load claims using a configurable path. |
script/utils/fetchAndStoreEvents.js | Replaced hardcoded values with config values and introduced event aggregation. |
script/utils/config.js | Added config options for locker and output paths. |
script/testUtils/*.js | Fixed import paths to reference updated utils directory. |
script/testSimulations/*.js | Updated simulation scripts with additional lock amounts and revised approval amounts. |
script/deploy/*.s.sol | Deployment scripts for MigrationRelease and MigrationLocker using a transparent proxy. |
README.md | Updated instructions and release ratio details to match the new setup. |
Comments suppressed due to low confidence (2)
script/testUtils/getPoof.js:1
- The file name 'getPoof.js' appears to be a typo. Consider renaming it to 'getProof.js' for consistency with the function it imports.
const whitelist = require("../../output/claims.json");
README.md:75
- The documented migration ratios in this section appear inconsistent. Please verify that the 'Total migration ratio: 1:15' (locked:received) is correct and aligns with the intended distribution.
**Instant Release**: 50% of the locked amount is immediately available
events.forEach(event => { | ||
const address = event.args.recipient; | ||
const amount = BigInt(event.args.amount.toString()); | ||
|
||
if (addressAmounts[address]) { | ||
// Address already exists, add to existing amount | ||
addressAmounts[address] = addressAmounts[address] + amount; | ||
console.log(`📝 Combined amount for ${address}`); | ||
} else { | ||
// First occurrence of this address | ||
addressAmounts[address] = amount; | ||
} | ||
}); |
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.
[nitpick] Consider removing or limiting debugging log outputs within loops to avoid potential performance issues in production environments.
events.forEach(event => { | |
const address = event.args.recipient; | |
const amount = BigInt(event.args.amount.toString()); | |
if (addressAmounts[address]) { | |
// Address already exists, add to existing amount | |
addressAmounts[address] = addressAmounts[address] + amount; | |
console.log(`📝 Combined amount for ${address}`); | |
} else { | |
// First occurrence of this address | |
addressAmounts[address] = amount; | |
} | |
}); | |
const combinedLogs = []; | |
events.forEach(event => { | |
const address = event.args.recipient; | |
const amount = BigInt(event.args.amount.toString()); | |
if (addressAmounts[address]) { | |
// Address already exists, add to existing amount | |
addressAmounts[address] = addressAmounts[address] + amount; | |
combinedLogs.push(`📝 Combined amount for ${address}`); | |
} else { | |
// First occurrence of this address | |
addressAmounts[address] = amount; | |
} | |
}); | |
combinedLogs.forEach(log => console.log(log)); |
Copilot uses AI. Check for mistakes.
await tx.wait(); | ||
const after = await ethers.provider.getBalance(address); | ||
|
||
const received = after - before; | ||
const expected = BigInt(amount) * 10n; | ||
const expected = (BigInt(amount) * 75n) / 10n; |
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.
Review the vested release expected amount calculation to ensure it aligns with the intended release ratio described in the updated release model.
const expected = (BigInt(amount) * 75n) / 10n; | |
const VESTED_RELEASE_RATIO_NUMERATOR = 75n; // Replace with actual numerator from updated release model | |
const VESTED_RELEASE_RATIO_DENOMINATOR = 10n; // Replace with actual denominator from updated release model | |
const expected = (BigInt(amount) * VESTED_RELEASE_RATIO_NUMERATOR) / VESTED_RELEASE_RATIO_DENOMINATOR; |
Copilot uses AI. Check for mistakes.
ABI: [ | ||
"event Locked(address caller, address recipient, uint256 amount)" | ||
], | ||
START_BLOCK: 0, // Block number where to start fetching events from ( DON't MARK THIS AS ZERO) |
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.
[nitpick] Consider correcting the comment to 'DON'T MARK THIS AS ZERO' to improve clarity.
START_BLOCK: 0, // Block number where to start fetching events from ( DON't MARK THIS AS ZERO) | |
START_BLOCK: 0, // Block number where to start fetching events from (DON'T MARK THIS AS ZERO) |
Copilot uses AI. Check for mistakes.
List of changes: