0

In following code I've implemented two functions to set and get Token holders.

 function setTokenHolders(address _to)internal returns(bool){
    for(uint i = 0; i<= tokenHolders.length; i++){
        if(_to != tokenHolders[i]){
            tokenHolders.push(_to);
            return true;
        }
        else{
            require(_to == tokenHolders[i],"Redundant token holder addresses.");
        }
    }
}
function getTokenHolders()public view returns(address[] memory){
    return tokenHolders;
}

My array's 0th element is initialized with my msg.sender. The logic of if is not worked as assumed.

  • Your else part doesn't do what you want it to do (you want to revert the transaction, but the require condition is always true instead of always false). – goodvibration Feb 12 at 11:39
  • And the <= part inside the for loop is disastrous, because you always end up accessing the array at an illegal index (so the transaction will always revert). – goodvibration Feb 12 at 11:40
  • okay, so what will I do? – Rudrika Feb 12 at 11:41
  • And your logic in general won't work more than two times. For example, consider you've added "1", then "2", then "2" again. You need a mapping here, not an array!!! – goodvibration Feb 12 at 11:42
  • Is there any way to do this without using mapping? – Rudrika Feb 12 at 11:43
1

Wow, your code and logic are full of errors!

First of all, with for(uint i = 0; i<= tokenHolders.length; i++), you'll always end up attempting to access the array at an illegal index, so the transaction will always revert. To begin with, you need to change the <= to <.

Second, you are trying to maintain an array with unique values, but your logic of checking _to != tokenHolders[i] will generally not work more than twice. For example, consider the following sequence:

  • After adding 0x1, the array is [0x1]
  • After adding 0x2, the array is [0x1, 0x2]
  • After adding 0x2, the array is [0x1, 0x2, 0x2], because 0x2 != tokenHolders[0]

Third, your else part doesn't do what you want it to do (you want to revert the transaction, but the require condition is always true instead of always false).

Fourth, function setTokenHolders is declared to return a bool value, which I don't see why it needs to, since you apparently want it to succeed or revert (but never to fail silently). In any case, in its current implementation, you did not bother to return a value in all cases.

Fifth, as soon as the array becomes too long, you'll end up in a scenario where the transaction doesn't fit in a block (the gas required for executing it is larger than the block gas-limit). At this point, you will no longer be able to successfully execute the function (i.e., it will always revert due to exceeding block gas-limit).

In short, you need a mapping here, not an array:

mapping(address => bool) public tokenHolders;

function setTokenHolders(address _to) internal {
    require(tokenHolders[_to] == false);
    tokenHolders[_to] = true;
}

Your Answer

By clicking "Post Your Answer", you acknowledge that you have read our updated terms of service, privacy policy and cookie policy, and that your continued use of the website is subject to these policies.

Not the answer you're looking for? Browse other questions tagged or ask your own question.