Skip to content
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

#6 airdrop #15

Open
wants to merge 13 commits into
base: main
Choose a base branch
from
Open

#6 airdrop #15

wants to merge 13 commits into from

Conversation

Ddorsmt
Copy link

@Ddorsmt Ddorsmt commented Jun 14, 2022

No description provided.

@huy525
Copy link

huy525 commented Jun 15, 2022

Overall everything is good! Nice use of modifiers, code is easy to understand and error-free (from what I see). Just a few small thoughts:

  • address(0) convention for balanceToken()/balanceETH() and claimAllToken() is a bit unusual, but OK. Should there be a test for balanceETH(address(0))?
  • Maybe shouldn't use the word "approve" because it's in ERC20 (I think so not really sure). And the "true" boolean return value isn't necessary as code would revert otherwise. It's mentioned in yesterday's impromptu session, but I guess you missed the first part lol.
  • Small typo in tests, should be "test approveETH: too poor" instead of "test approveETH: not owner".
  • I guess your code doesn't support direct ERC20 transfer the native transfer() function, huh. Will the funds be lost forever if someone accidentally used transfer()? Maybe instead of _undistributedToken and _undistributed just call IERC20().balanceOf() and address(this).getBalance()? Just my two cents tho lol.
  • Brilliant gamble() function! Add tests to ensure it works perfectly ... as expected ...?

Edit: Oh, and Duong commented on my PR that gas can be explicitly calculated in testing, so that might help.

@hruiyang
Copy link

Interesting gamble function! Not sure if there's really a gambling element or it's a 100% sure-lose tho LMAO.

That's aside, the code is mostly fine, it is quite sound!

  1. I think you can use Ownable or BoringOwnable for ownership of smart contracts!
  2. Also, you can use SafeERC20 or TransferHelper to do transfers, this way you don't need to worry about whether or not there is enough balance for the user (in your require statements)
  3. I think you can change some of the public functions to external

@Ddorsmt Ddorsmt changed the title airdrop #6 airdrop Jun 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants