-
Notifications
You must be signed in to change notification settings - Fork 7
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
feat: migrateTo #67
feat: migrateTo #67
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.
My
src/PolygonMigration.sol
Outdated
@@ -51,6 +51,14 @@ contract PolygonMigration is Ownable2StepUpgradeable, IPolygonMigration { | |||
polygon.safeTransfer(msg.sender, amount); | |||
} | |||
|
|||
/// @inheritdoc IPolygonMigration | |||
function migrateTo(address recipient, uint256 amount) external { | |||
emit Migrated(msg.sender, 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.
Just wondering whether or not we should follow the same standard used by Unmigrated where we do have both the origin account and recipient as part of the event
event Unmigrated(address indexed account, address indexed recipient, uint256 amount);
We should probably create a new event following the same pattern for MigrationTo in order to capture "msg.sender vs. recipient information" to differentiate from a normal migrate function.
wdyt?
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.
agreed and added.
Quality Gate passedIssues Measures |
/// @param amount the amount of MATIC that was migrated | ||
event Migrated(address indexed account, uint256 amount); | ||
event Migrated(address indexed account, address recipient, uint256 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.
Hi, thanks. I'd suggest we confirm if this event signature "change" is breaking any known relevant event listeners to agree on the best way to mitigate its impact (i.e. sometimes we do recommend to create new events instead).
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.
Let's just check the event signature change likely breaking any relevant listener to decide whether or not it is better to add a new event for this "migrateTo". The rest LGTM. Thanks!
Add
migrateTo
function to PolygonMigration contract.