New Migration Contract #19
Conversation
afkbyte
requested review from
jasheal,
franck44,
Tri-stone,
aodhgan,
cateatpeanut and
agnarsong
…no longer accept eth
| return (_amount * TOKEN_SWAP_RATIO) / TOKEN_SWAP_SCALING_FACTOR; | ||
| } | ||
|
|
||
| function tokenMigrationAmountToRecieve(uint256 _amount) external returns (uint256) { |
There was a problem hiding this comment.
State modifier keyword here too
| uint256 amountToSwap = _tokenSwapCalculation(_amount); | ||
|
|
||
| // transfer user's BIT tokens to this contract | ||
| bool success = IERC20(BIT_TOKEN_ADDRESS).transferFrom(msg.sender, address(this), _amount); |
There was a problem hiding this comment.
So here we enforce that the accounts involved in the transfer both exist which is good 👍
If the call returns false it must be because of not enough tokens or out-of-gas, correct?
[I am not a Solidity expert, but if I am correct, transferring to a non-existing account returns true so it is good to prevent this from happening.]
There was a problem hiding this comment.
One more comment: if the call to transferFrom reverts, it bubbles up in Solidity and the caller reverts too.
I wonder whether it is possible to get a return value that is false in our setting. It depends on the implementation of transferFrom, right?
There was a problem hiding this comment.
gonna be replacing all transfer instances with safeTransfer so shouldn't be a concern, I'll remove the errors that shouldn't be reached anymore
There was a problem hiding this comment.
So at the end of the day, the body of this function could be:
IERC20(BIT_TOKEN_ADDRESS).transferFrom(...);
IERC20(MNT_TOKEN_ADDRESS).transfer(...);correct?
|
@afkbyte created this PR to resolve two nits, suggest merge and then lgtm main |
closes: #12 #11 #7 #6 #5 #4
TODO: add natspec comments