1

I have the following code for a simple gambling game. The code works fine except for getPlayers function

pragma solidity >=0.4.22 <0.6.0;

contract Lottery {
address payable public manager;
address payable [] public players; 

constructor () public {
    manager = msg.sender;
}
function enter() public payable {

    require(msg.value > 0.01 ether); //condition
    players.push(msg.sender);
}

function random() private view returns(uint) {
    return uint(keccak256(abi.encodePacked(block.difficulty, now, 
players)));
}

function pickWinner() public restricted{
    uint index = random() % players.length;
    players[index].transfer(address(this).balance);
    players = new address payable[](0); // new dynamic array

}

modifier restricted() {
    require(msg.sender == manager); // security
    _;
}

function getPlayers() public payable returns (address[] memory) {
    return players;
    // Error in the above line saying
    // "Return argument type address payable[] storage ref is not implicitly convertible to expected type(type of first return variable)  address[] memory."

}
}
  • 1
    "The code works fine" - impossible. This is a compilation error, which means that you couldn't have possibly tested the code as of yet. – goodvibration Dec 15 '18 at 9:13
  • You can fix this error by removing the memory keyword that you've placed in the declaration of function getPlayers. Side note: since players is a public state variable, the compiler automatically creates a getter function for this variable, which you can invoke from the off-chain via players(). So this function seems redundant, and you may as well get rid of it altogether. – goodvibration Dec 15 '18 at 9:16
  • By the way, I gotta admit I've never seen the use of payable in the declaration of variables (as you do at the beginning of your contract). – goodvibration Dec 15 '18 at 9:17
  • @goodvibration Yes, the recent Solidity versions have added the address payable type to the language. – Jesse Busman Dec 15 '18 at 9:52
  • @Anukool Srivastava Please accept my answer if it fixed your problem :) – Jesse Busman Dec 15 '18 at 12:07
0

There are quite a few problems here.

I'm not sure where pragma solidity >=0.4.22 <0.6.0; originates but it's picked up by a lot of people and appears in many examples. The problem is there are too many breaking changes in successive compiler versions for it to be right very often. In my opinion it's always better to specify exactly which compiler the code is written for.

I realize it's not your question, but the enter() function is set up to swallow funds with no means of retrieval. No accounting is being done and no check for excessive amounts (error). At a minimum, require the precise amount and reject user errors.

require(msg.value = ticketPrice);

The players function is returning the full array. This is an anti-pattern because it will stop working when it exceeds the block gasLimit. It should not be necessary to implement such a function.

First, emit an event in the enter() function to inform observers there has been a state change. This makes it reasonable to assume that any interested party already has the complete list of players.

event LogNewPlayer(address newPlayer);

...

emit LogNewPlayer(playerAddress);

Second, make the state discoverable at a fixed cost per transaction, independent of scale. Iteration should be a client-side concern. Provided the individual transactions always complete at a reasonable cost, a client can iterate endlessly. Better to have lots of cheap operations that one costly operation.

pragma solidity 0.5.1;

contract Lottery {

    address payable[] public players;

    // above is just enough wrapper to compiler and test these.

    function getPlayerAtIndex(uint index) public view returns(address player) {
        return players[index];
    }

    function getPlayerCount() public view returns(uint count) {
        return players.length;
    }
}

The address array is storage but the returns of a function have to be memory. That means iterating over the stored list and copying all of that data to memory.

A more permissive compiler might make that seem like an okay thing to do. In Solidity 5.x I believe you would have to explicitly handle the conversion so it's clear you are conscious of what you're doing. It would be awful owing to the scalability concerns. Something like:

function grossDoNotUse() public view returns(address[] memory) {
    address[] memory playerList;
    for(uint i=0; i<getPlayerCount(); i++) {
        playerList[i] = players[i];
    }
    return playerList;
}

Don't do that!

Hope it helps.

  • I am just a beginner in this and all the explanation above was a bit over my head! I'll be extrmely greatful if you could tell me how can i overcome the error im getting above. since im just working on a testing network using ganach, I dont see an issue with gas limit for now . However I have a note of it when i proceed with the logic at a later stage. Thank you – Anukool Srivastava Feb 7 at 6:41
  • grossDoNotUse() address your issue with getPlayers() without changing the approach. Since you're just starting out, that should be fine. Rename it and replace your implementation. – Rob Hitchens - B9lab Feb 7 at 6:55
  • The two functions laid out earlier get around the dangers slightly altering your approach. Instead of expecting a whole list, you will get one entry at a time as you ask for them (by row). You can find a fuller explanation here: blog.b9lab.com/getting-loopy-with-solidity-1d51794622ad – Rob Hitchens - B9lab Feb 7 at 6:57
  • i clearly got your approach @Rob, thanks a lot. However the function above is still not working. It's giving me invalid opcode error . Function is returning an empty array. – Anukool Srivastava Feb 7 at 11:31
0

As of version 0.5.0 Solidity has two address types: address and address payable.

The problem is that they cannot be implicitly converted when in memory array form, but you are trying to assign a address payable storage[] to a address memory[].

You can solve this by changing the return type of getPlayers to address payable memory[]

  • i tried your suggestion but im getting an ParserError on Remix. Error says " Expected ',' but got '[' " Its not accepting a payable return type – Anukool Srivastava Dec 17 '18 at 6:01
  • Here is the changed function definition /* function getPlayers() public payable returns (address[] payable memory) { return players; } */ – Anukool Srivastava Dec 17 '18 at 6:03
  • @AnukoolSrivastava The correct declaration should be function getPlayers() public view returns (address payable[] memory) { – Ismael Dec 18 '18 at 20:40
  • tried that, but in my local editor 'visual studio code' im getting an error with this address payable public manager;* it gives me an error saying expected identifier but got 'payable' the code is running perfectly fine on remix but when im trying to plug it with reactjs project, im getting the above error – Anukool Srivastava Jan 15 at 13:26
  • @AnukoolSrivastava Then you are compiling with a Solidity version lower than 0.5.0. To use address payable you need at least version 0.5.0 – Jesse Busman Jan 15 at 14:13

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.