victortzy_airdrop #17
Conversation
contracts/Distributor.sol
Outdated
| constructor(address[] memory accounts_, uint256[] memory shares_) payable { | ||
| require(accounts_.length == shares_.length, "Distributor: Mismatch of addresses and shares."); | ||
| for(uint256 i = 0; i < accounts_.length; i++ ){ | ||
| _registerPayeesForETH(accounts_[i], shares_[i]); | ||
| } | ||
| } |
There was a problem hiding this comment.
Why you only distribute shares for eth in the constructor =)))
You can do the same thing with erc20, assign in the constructor, or just do both outside the constructor.
this is weird to me since it is not consistent
There was a problem hiding this comment.
Haha yeah it may be abit weird, but my primary consideration was to only allow the owner to register payees with their respective shares once for each ERC20 token, starting with the native token ETH as the constructor so yeah. I could add in features to allow amendments to shares and addition of payees too
contracts/Distributor.sol
Outdated
| require(address(this).balance >= ethClaimable, "Distributor: Insufficient ETH balance in contract."); | ||
|
|
||
| _totalETHDistributed += ethClaimable; | ||
| _distributedETH[account] = ethClaimable; | ||
| (bool success,) = account.call{value: ethClaimable}(""); | ||
| require(success, "Distributor: Unable to receive ETH from contract."); |
There was a problem hiding this comment.
consider using address.send(value) to limit gas in case the fallback account is malicious.
or you can use transfer to auto-revert if unsuccessfully transfer
There was a problem hiding this comment.
Yup okay thanks for the suggestion, sounds good 👍
contracts/Distributor.sol
Outdated
| function distributeDemAllETH() external onlyOwner { | ||
| uint totalPayees = numRegisteredForETH(); | ||
| for(uint256 i = 0; i < totalPayees;){ | ||
| if(_sharesForETH[_payeesForETH[i]] > 0) { | ||
| payoutETH(payable(_payeesForETH[i])); | ||
| } | ||
| unchecked {i++; } | ||
| } | ||
| } |
There was a problem hiding this comment.
this will not revert if there is a payee does not have ETH allocated, unlike your below function for distributeDemAllToken will revert if this happens => inconsistent
There was a problem hiding this comment.
Oh yes sorry I overlooked this part, I could remove that line of code too because totalPayee refers to the actual number of accounts registered with shares for ETH, will amend it accordingly thanks!
There was a problem hiding this comment.
May be that they will not able to claim exactly amount they want ?
There was a problem hiding this comment.
They will be able to claim the proportion of funds deposited inside the contract based on their shares relative to the total shares, regardless of how many times it is being topped up
contracts/Distributor.sol
Outdated
| function distributeDemAllToken(IERC20 token_) external onlyOwner { | ||
| IERC20 token = token_; | ||
| uint totalPayees = numRegisteredForToken(token); | ||
| for(uint256 i = 0; i < totalPayees;){ | ||
| payoutToken(_payeesForToken[token][i], token); | ||
| unchecked {i++; } | ||
| } | ||
| } |
There was a problem hiding this comment.
will revert if the share of one payee is 0
There was a problem hiding this comment.
It won't, because I am using a nested mapping to keep track of accounts registered under a particular token (means every account looped through will have shares > 0 for that token) itself
contracts/Distributor.sol
Outdated
| require(account != address(0), "Distributor: Null address not allowed."); | ||
| require(share > 0, "Distributor: Invalid shares specified."); | ||
| require(_sharesForETH[account_] == 0, "Distributor: Account registered for ETH distribution."); |
There was a problem hiding this comment.
duplicated require statement, consider to declare modifiers
There was a problem hiding this comment.
Oh yeah thanks alot, refracted both line 138 and 139 as modifiers!
contracts/Distributor.sol
Outdated
| function _registerPayeesForETH(address account_, uint256 share_) private { | ||
| address account = account_; | ||
| uint share = share_; | ||
| require(account != address(0), "Distributor: Null address not allowed."); | ||
| require(share > 0, "Distributor: Invalid shares specified."); | ||
| require(_sharesForETH[account_] == 0, "Distributor: Account registered for ETH distribution."); | ||
|
|
||
| // Save account as a payee in the mapping | ||
| _payeesForETH[_numRegisteredForETH++] = account_; | ||
| _sharesForETH[account_] = share_; | ||
| _totalETHShares += share_; | ||
|
|
||
| emit RegisterPayeeForETH(account, share); | ||
| } |
There was a problem hiding this comment.
Nope, I thought of implementing that at first but then I felt maybe it would be better if the proportion of shares were fixed when launched to preserve perpetuity of relative shares for each payee. Yeah, it would be bad if someone made a mistake in the shares breakdown and there would be many who would prefer to have it amendable also. So yeah I just didn't implement it first haha but its needed do I will gladly do it 👍
| constructor(address[] memory accounts_, uint256[] memory shares_) payable { | ||
| require(accounts_.length == shares_.length, "Distributor: Mismatch of addresses and shares."); | ||
| for(uint256 i = 0; i < accounts_.length; i++ ){ | ||
| _registerPayeesForETH(accounts_[i], shares_[i]); |
There was a problem hiding this comment.
I think not exposing this function to public makes it that if the owner want to airdrop for some users in the future, he won't be able to
There was a problem hiding this comment.
Hence, strongly recommend to move it out of the constructor & expose it to be a public/external function
There was a problem hiding this comment.
Yeah actually should make it flexible. Thanks a lot and will make the necessary amends!
|
|
||
| // @Desc: To batch register accounts with a specified share for a new ERC20 Token | ||
| function registerPayeesForToken(address[] memory accounts_, IERC20 token_, uint256[] memory shares_) external onlyOwner { | ||
| require(accounts_.length == shares_.length, "Distributor: Mismatch of addresses and shares."); |
There was a problem hiding this comment.
Good error message, I like it!
contracts/Distributor.sol
Outdated
| /*/////////////////////////////////////////////////////////////// | ||
| Pure/View Functions | ||
| //////////////////////////////////////////////////////////////*/ | ||
| function numRegisteredForETH() public view returns(uint256){ |
There was a problem hiding this comment.
Is there a reason for you to make the variable private & write public functions?
First draft of the distributor contract. Note I purposely on restricted the registration rights to the owner but not the claim functions for possible future integration of external contracts.