Skip to content

Stateless airdrops #432

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

Merged
merged 10 commits into from
Jul 20, 2023
Merged

Stateless airdrops #432

merged 10 commits into from
Jul 20, 2023

Conversation

kumaryash90
Copy link
Member

No description provided.

@kumaryash90 kumaryash90 marked this pull request as draft July 19, 2023 13:34
@kumaryash90 kumaryash90 requested a review from nkrishang July 19, 2023 13:35
@nkrishang nkrishang marked this pull request as ready for review July 20, 2023 16:59
@nkrishang
Copy link
Contributor

Re: this native token amount check: https://github.com/thirdweb-dev/contracts/blob/yash/stateless-airdrops/contracts/airdrop/AirdropERC20.sol#L119C9-L119C82

This check happens outside the for loop, however we should try to revert as early as possible, since the for loop is likely to have many runs (due to many recipients). What about this pattern:

uint256 msgValue = msg.value;
uint256 nativeTokenAmount = 0;
bool insufficientValue = false;

for(uint256 i = 0; i < len; ) {
   // .. whatever logic
  
    if (_tokenAddress == CurrencyTransferLib.NATIVE_TOKEN) {
        nativeTokenAmount += _contents[i].amount;
       
        if(nativeTokenAmount > msgValue) {
            insufficientValue = true;
            break;
        }
    }
}

require(!insufficientValue && msgValue == nativeTokenAmount);

@nkrishang
Copy link
Contributor

Re: this native token amount check: https://github.com/thirdweb-dev/contracts/blob/yash/stateless-airdrops/contracts/airdrop/AirdropERC20.sol#L119C9-L119C82

This check happens outside the for loop, however we should try to revert as early as possible, since the for loop is likely to have many runs (due to many recipients). What about this pattern:

uint256 msgValue = msg.value;
uint256 nativeTokenAmount = 0;
bool insufficientValue = false;

for(uint256 i = 0; i < len; ) {
   // .. whatever logic
  
    if (_tokenAddress == CurrencyTransferLib.NATIVE_TOKEN) {
        nativeTokenAmount += _contents[i].amount;
       
        if(nativeTokenAmount > msgValue) {
            insufficientValue = true;
            break;
        }
    }
}

require(!insufficientValue && msgValue == nativeTokenAmount);

To clarify, the break should happen before the _transferCurrencyWithReturnVal, and not after.

This is because if the msg.value sent in the airdrop call is truly less than the cumulative nativeTokenValue, then at some point, _transferCurrencyWithReturnVal will start returning false for native token transfers. In such a case, we want to revert as soon as possible, and not spend the gas to perform the remaining for loop runs, since the function is going to revert anyway after the loop ends.

@nkrishang
Copy link
Contributor

Let's not inherit Ownable in the AirdropERCxxx contracts; the presence of Permissions is sufficient.

@kumaryash90 kumaryash90 merged commit a6c89a5 into main Jul 20, 2023
@kumaryash90 kumaryash90 deleted the yash/stateless-airdrops branch July 20, 2023 21:28
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.

2 participants