-
Notifications
You must be signed in to change notification settings - Fork 18
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
contract clean up for distribution #67
Conversation
total += values[i]; | ||
} | ||
|
||
require(msg.value >= total, "Not enough Ether to distribute"); |
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.
Could just be a hardhat node thing, but this requirement breaks the current tests in the latest tests on the dev branch
require(token.transfer(recipients[i], values[i])); | ||
|
||
function distributeToken( | ||
IERC20 token, |
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.
ERC20 implements the internal method _transfer(address sender, address recipient, uint256 amount)
with requirements about spenders balances, null addresses, and allowances. Does that make these require
ments redundant?
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.
Minus the the tests that need to be written, this looks pretty solid! Check out the comments/questions about requirements.
Also worth noting, this contract is about 2X the gas to deploy.
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.
It makes sense to not rely on the implementation of ERC20 contracts and instead have our own checks. In fact, we should look into using raricaptitals/solmates erc20 implementation
No description provided.