0

I know a similar question has been asked before but I want to understand where am I going wrong in terms of logic for the code below. I am building a lottery contract where if an address has been added once then that address can no longer play again. I am using a for loop to check if the address is present.
When I am deploying the contract and running the addPlayer contract I am getting an error statig -- transact to lottery.addPlayer errored: VM error: invalid opcode. invalid opcode The execution might have thrown. Debug the transaction to get more information.

contract lottery
{
        //Defining the Owner
            address Owner;
            constructor() public
            {   
                 Owner = msg.sender;    
                 
            }
    
        // Defining players
        
            struct person
            {
                string name;
                uint age;
            }
    
        // One to one mapping of player and its address    
            mapping (address => person) players;
            mapping(string => bool) userExists;


        // creating a list/array of players    
        address[] playersAddressList;


        // Tracking balance;
            uint public balance=0;
            
                   
        //Function to add a player to the lottery
        bool public flag=false;
        
        
        function addPlayer(string memory a, uint b) public payable
        {
               //Checking if the player already exists in the list
                
                for(uint i=0; i<=playersAddressList.length; i++)
                {
                    if(playersAddressList[i] == msg.sender)
                    {
                       flag=true;
                       break;
                    }
                }
         
                require (flag==false);
                
                address creator = msg.sender;
                playersAddressList.push(creator);
                
                balance = balance + msg.value;
                require (msg.value >= 1 ether);
                
                person memory newPerson;
    
                newPerson.name = a;
                newPerson.age = b;
        
                players[creator] = newPerson;
        }
     // Function to get the player details'
      function getPlayerDetails(uint a) public view returns(string memory, uint)
        {
            address playersAddress = playersAddressList[a];
            return (players[playersAddress].name, players[playersAddress].age);
       }
}
0

You problem is with line: for(uint i=0; i<=playersAddressList.length; i++). Arrays are zero-based so you are iterating too far.

If for example your array has 3 entries the length will be 3. But your loop checks for 4 entries (at indexes 0, 1, 2 and 3) - the last index doesn't exist so it crashes.

Just change your condition to for(uint i = 0; i < playersAddressList.length; i++) and it should work.

| improve this answer | |
0

As the amount of players grows, that loop will consume more and more gas. If you get a lot of players, it will start to cost too much in transaction fees.

If the amount of players should be unlimited, you should maintain a mapping to track which addresses are already participating:

mapping(address => bool) public playersMapping;

That way you can run one if-statement in O(1) gas complexity instead of the loop in O(n) gas complexity:

if (playersMapping[playerAddress]) {

You might be able to use the players mapping that you already have

| improve this answer | |
  • Thanks @Jesbus that was very helpful. – Rwiju Pal Sep 29 at 2:18

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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