lottery contract, tests and demo script #17
Conversation
| } | ||
|
|
||
| function enter() public payable { | ||
| require(contractPhase == Status.Open, "Not open yet"); |
There was a problem hiding this comment.
just checking - do you want to have one entry per wallet condition here?
contracts/lottery.sol
Outdated
| shuffle(); // Shuffle the players array | ||
| // The winner is the first player in the shuffled array | ||
| // The permutation is randomly generated so we can also take more winners if needed | ||
| players[0].transfer(address(this).balance); |
There was a problem hiding this comment.
while this isn't affected, it's a good practice to use call.value() instead of transfer(), so lets update that!
https://consensys.github.io/smart-contract-best-practices/development-recommendations/general/external-calls/#dont-use-transfer-or-send
| } | ||
|
|
||
|
|
||
| function getPlayers() public view returns (address payable[] memory) { |
There was a problem hiding this comment.
this method isn't necessary - lets lean on calling players() instead
There was a problem hiding this comment.
players() can only be called on each element. It is not convenient for reading the whole list.
There was a problem hiding this comment.
the ideal way is for the script to do the heavy lifting - and reducing the contract size hence. But since this is a demo feel free to use your preference.
| } | ||
|
|
||
| function pickWinner() public onlyOwner { | ||
| require(players.length > 0, "No players"); |
There was a problem hiding this comment.
lets add in a require(contractPhase == open) to avoid the option to re-call this method again?
No description provided.