0

I am programming a lottery. I am storing the players (addresses) who enter in a public array, and I only want a person to be able to enter ONCE per lottery. Meaning if they have sent in the required amount of ether, their address is stored in the array, and if they try to enter again, it rejects and refunds their ether. I set up a for loop in the receive function, but it does not seem to be working when I deploy on remix, meaning I can use the same address to enter over and over. Here is my code:

pragma solidity ^ 0.8.10;

contract Test_Lottery {

    address payable[] public Players;
    address payable public admin;


    constructor() {
        admin = payable(msg.sender);
    }

    modifier onlyAdmin {
        require(admin == msg.sender);
        _;
    }

    receive() external payable {
        require(msg.value >= 0.001 ether , "You haven't entered enough into the pot!");
        require(msg.sender != admin , "The admin cannot play!");
        for (uint i=0; i < Players.length; i++) {
            if (msg.sender == Players[i]) {
                { revert() ; }
            } else {
                Players.push(payable(msg.sender));
            }          
        }
    }

    function getLotteryValue() public view returns (uint) {
        return address(this).balance;
    }

    function getNumberOfPlayers() public view returns (uint) {
        return Players.length;
    }

    function random() internal view returns (uint) {
        return uint(keccak256(abi.encodePacked(block.difficulty, block.timestamp, Players.length)));
    }

    function pickWinner() public onlyAdmin {
        address payable winner;
        require(Players.length >= 3 && address(this).balance >= 10 ether , "Not enough people have entered or the pot isn't big enough!" );
        winner = Players[random() % Players.length];
        winner.transfer(getLotteryValue());
        Players = new address payable[](0);
        }
        
    }

Is this a correct way to implement this functionality? I understand for loops are relatively naive, so I am open to other implementations of this one-entry functionality. Perhaps mappings and bools?

1 Answer 1

1

To check the address, add mapping PlayersCheck as shown below - it's faster and cheaper. And try to avoid large-dimensional arrays altogether, since working with them may require an excessive amount of gas.

If you want to exclude the possibility of sending a transaction through a proxying contract, then you should use tx.origin instead of msg.sender


contract Test_Lottery {

    address payable[] public Players;
    mapping (address => bytes32) public PlayersCheck;

    address payable public admin;

    receive() external payable {
        require(msg.value >= 0.001 ether , "You haven't entered enough into the pot!");
        require(msg.sender != admin , "The admin cannot play!");

            if (PlayersCheck[msg.sender]=="Y") {
                 revert() ; 
            } else {
                Players.push(payable(msg.sender));
                PlayersCheck[msg.sender]="Y" ;
            }          
    }

Or with using 'requiry' operator :

    receive() external payable {
        require(msg.value >= 0.001 ether , "You haven't entered enough into the pot!");
        require(msg.sender != admin , "The admin cannot play!");

        require(PlayersCheck[msg.sender]!="Y" , "You have already entered!");

                Players.push(payable(msg.sender));
                PlayersCheck[msg.sender]="Y" ;

    }

8
  • Hi there! Thank you for your reply. I implemented this but the contract still lets me send from the same address over and over on remix. Any idea why this might be?
    – e n
    Dec 7, 2021 at 7:00
  • And the sender is re-added to Players? Does the value given by getNumberOfPlayers() change? Perhaps the Remix does not preserve the context of the contract. Have you tried the test or your private network?
    – Mad Jackal
    Dec 7, 2021 at 7:14
  • In your corrected code, you incorrectly wrote the assignment operation (not in if operator): Players Check[msg.sender] == "Y"; must be Players Check[msg.sender] = "Y";
    – Mad Jackal
    Dec 7, 2021 at 7:20
  • DUDE!!!!! It worked!! Thank you so much! Could you explain again why I need two equal signs for the if statement but not for the PlayersCheck[msg.sender] in the else statement? Also, how would I add an error statment saying "You have already entered!"? Would I just add that after the revert?
    – e n
    Dec 7, 2021 at 7:49
  • Add to answer. '==' is the logical comparison operator 'equals', and '=' is the assignment operator. As in C++
    – Mad Jackal
    Dec 7, 2021 at 8:14

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.