-
Notifications
You must be signed in to change notification settings - Fork 0
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: Added migrator contract and tests (SC-4769) #1
Conversation
This pull request has been linked to Shortcut Story #4769: [mpl-migration] Create repo for MPL token migration. |
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.
Looks good! Small changes
contracts/Migrator.sol
Outdated
require(ERC20Helper.transfer(newToken, msg.sender, amount_), "M:M:TRANSFER_FAILED"); | ||
} | ||
|
||
} |
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.
New line
contracts/Migrator.sol
Outdated
require(ERC20Helper.transferFrom(oldToken, msg.sender, address(this), amount_), "M:M:TRANSFER_FROM_FAILED"); | ||
require(ERC20Helper.transfer(newToken, msg.sender, amount_), "M:M:TRANSFER_FAILED"); |
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.
require(ERC20Helper.transferFrom(oldToken, msg.sender, address(this), amount_), "M:M:TRANSFER_FROM_FAILED");
require(ERC20Helper.transfer(newToken, msg.sender, amount_), "M:M:TRANSFER_FAILED");
contracts/Migrator.sol
Outdated
address immutable oldToken; | ||
address immutable newToken; | ||
|
||
constructor(address old_, address new_) { |
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.
oldToken_
, newToken_
contracts/test/Migrator.t.sol
Outdated
function test_migration(uint256 amount_) external { | ||
amount_ = constrictToRange(amount_, 1, OLD_SUPPLY); | ||
|
||
//Mint new token to migrator |
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.
// Mint
contracts/test/Migrator.t.sol
Outdated
amount_ = constrictToRange(amount_, 1, OLD_SUPPLY); | ||
|
||
//Mint new token to migrator | ||
newToken.mint(address(migrator), OLD_SUPPLY); |
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.
This step should be in the setUp I think
contracts/test/Migrator.t.sol
Outdated
try migrator.migrate(amount_) { | ||
assertTrue(false, "Able to migrate without new token"); | ||
} catch Error(string memory reason) { | ||
assertEq(reason, "M:M:TRANSFER_FAILED"); | ||
} |
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.
vm.expectRevert
contracts/test/Migrator.t.sol
Outdated
|
||
|
||
|
||
} |
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.
New line
contracts/test/Migrator.t.sol
Outdated
|
||
|
||
|
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.
One line break
contracts/test/Migrator.t.sol
Outdated
//Mint new token to migrator | ||
newToken.mint(address(migrator), OLD_SUPPLY); |
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.
Lets move this to the setup block and then burn OLD_SUPPLY - amount_ + 1, and then mint 1 for the success case
test.sh
Outdated
forge test --match "$match" -vvv --lib-paths "modules" --contracts "contracts" |
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.
New line
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.
Dont need --contracts flag
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.
Also lets try removing --lib-paths and see what happens, last time it didn't work for me but maybe new forge works
7c6acd8
to
14ba45a
Compare
No description provided.