diff --git a/contracts/RockPaperScissors.sol b/contracts/RockPaperScissors.sol index 79820df..5cc8f8a 100644 --- a/contracts/RockPaperScissors.sol +++ b/contracts/RockPaperScissors.sol @@ -1,4 +1,4 @@ -pragma solidity ^0.4.24; +pragma solidity ^0.5.0; contract RockPaperScissors { enum Choice { @@ -21,16 +21,15 @@ contract RockPaperScissors { bytes32 commitment; Choice choice; } - - // events + event Payout(address player, uint amount); - //initialisation args + // Initialisation args uint public bet; uint public deposit; uint public revealSpan; - // state vars + // State vars CommitChoice[2] public players; uint public revealDeadline; Stage public stage = Stage.FirstCommit; @@ -41,166 +40,145 @@ contract RockPaperScissors { revealSpan = _revealSpan; } - // TODO: go through and write explicit 'stored' and 'memory' everywhere - function commit(bytes32 commitment) payable public { - // only allow commit stages + function commit(bytes32 commitment) public payable { + // Only run during commit stages uint playerIndex; if(stage == Stage.FirstCommit) playerIndex = 0; else if(stage == Stage.SecondCommit) playerIndex = 1; - else revert(); + else revert("both players have already played"); - //TODO: possible overflow uint commitAmount = bet + deposit; - require(msg.value >= commitAmount); - - // return any excess - if(msg.value > commitAmount) msg.sender.transfer(msg.value - commitAmount); - - // store the commitment + require(commitAmount >= bet, "overflow error"); + require(msg.value >= commitAmount, "value must be greater than commit amount"); + + // Return additional funds transferred + if(msg.value > commitAmount) { + (bool success, ) = msg.sender.call.value(msg.value - commitAmount)(""); + require(success, "call failed"); + } + + // Store the commitment players[playerIndex] = CommitChoice(msg.sender, commitment, Choice.None); - // if we're on the first commit, then move to the second + // If we're on the first commit, then move to the second if(stage == Stage.FirstCommit) stage = Stage.SecondCommit; - // otherwise we must already be on the second, move to first reveal + // Otherwise we must already be on the second, move to first reveal else stage = Stage.FirstReveal; } - + function reveal(Choice choice, bytes32 blindingFactor) public { - require(stage == Stage.FirstReveal || stage == Stage.SecondReveal); - // only valid choices - require(choice == Choice.Rock || choice == Choice.Paper || choice == Choice.Scissors); - - // find the player index + // Only run during reveal stages + require(stage == Stage.FirstReveal || stage == Stage.SecondReveal, "not at reveal stage"); + // Only accept valid choices + require(choice == Choice.Rock || choice == Choice.Paper || choice == Choice.Scissors, "invalid choice"); + + // Find the player index uint playerIndex; if(players[0].playerAddress == msg.sender) playerIndex = 0; else if (players[1].playerAddress == msg.sender) playerIndex = 1; - // unknown player - else revert(); + // Revert if unknown player + else revert("unknown player"); + + // Find player data + CommitChoice storage commitChoice = players[playerIndex]; - // find the player data - CommitChoice storage commitChoice = players[playerIndex]; + // Check the hash to ensure the commitment is correct + require(keccak256(abi.encodePacked(msg.sender, choice, blindingFactor)) == commitChoice.commitment, "invalid hash"); - // check the hash, we have a hash of sender, choice, blind so that players cannot learn anything from a committment - // if it were just choice, blind the other player could view this and submit it themselves to reliably achieve a draw - require(keccak256(abi.encodePacked(msg.sender, choice, blindingFactor)) == commitChoice.commitment); - - // update if correct + // Update choice if correct commitChoice.choice = choice; - if(stage == Stage.FirstReveal) { - // TODO: possible overflow - // if this is the first reveal we set the deadline for the second one - revealDeadline = block.number + revealSpan; - // if we're on first reveal, move to the second + if(stage == Stage.FirstReveal) { + // If this is the first reveal, set the deadline for the second one + revealDeadline = block.number + revealSpan; + require(revealDeadline >= block.number, "overflow error"); + // Move to second reveal stage = Stage.SecondReveal; } - // if we're on second reveal, move to distribute + // If we're on second reveal, move to distribute stage else stage = Stage.Distribute; } function distribute() public { - // to distribute we need: - // a) to be in the distribute stage OR b) still in the second reveal stage but past the deadline - require(stage == Stage.Distribute || (stage == Stage.SecondReveal && revealDeadline <= block.number)); + // To distribute we need: + // a) To be in the distribute stage OR + // b) Still in the second reveal stage but past the deadline + require(stage == Stage.Distribute || (stage == Stage.SecondReveal && revealDeadline <= block.number), "cannot yet distribute"); - // calulate value of payouts for players - //TODO: possible overflow + // Calculate value of payouts for players uint player0Payout; uint player1Payout; uint winningAmount = deposit + 2 * bet; + require(winningAmount / deposit == 2 * bet, "overflow error"); - // we always draw with the same choices, and we dont lose our deposit even if neither revealed + // If both players picked the same choice, return their deposits and bets if(players[0].choice == players[1].choice) { player0Payout = deposit + bet; player1Payout = deposit + bet; } - // at least one person has made a choice, otherwise we wouldn't be here - // in the situation that only one person chose that person wins, and the person - // who did not will lose their deposit + // If only one player made a choice, they win else if(players[0].choice == Choice.None) { player1Payout = winningAmount; } else if(players[1].choice == Choice.None) { player0Payout = winningAmount; } - // both players have made a choice, and they did not draw else if(players[0].choice == Choice.Rock) { + assert(players[1].choice == Choice.Paper || players[1].choice == Choice.Scissors); if(players[1].choice == Choice.Paper) { - // rock loses to paper + // Rock loses to paper player0Payout = deposit; player1Payout = winningAmount; } else if(players[1].choice == Choice.Scissors) { - // rock beats scissors + // Rock beats scissors player0Payout = winningAmount; player1Payout = deposit; - } - else revert(); + } } else if(players[0].choice == Choice.Paper) { + assert(players[1].choice == Choice.Rock || players[1].choice == Choice.Scissors); if(players[1].choice == Choice.Rock) { - // paper beats rock + // Paper beats rock player0Payout = winningAmount; player1Payout = deposit; } else if(players[1].choice == Choice.Scissors) { - // paper loses to scissors + // Paper loses to scissors player0Payout = deposit; player1Payout = winningAmount; } - else revert(); } else if(players[0].choice == Choice.Scissors) { + assert(players[1].choice == Choice.Paper || players[1].choice == Choice.Rock); if(players[1].choice == Choice.Rock) { - // scissors lose to paper + // Scissors lose to rock player0Payout = deposit; player1Payout = winningAmount; } else if(players[1].choice == Choice.Paper) { - // scissors beats paper + // Scissors beats paper player0Payout = winningAmount; player1Payout = deposit; } - else revert(); - } - else revert(); - - // send the payouts - if(player0Payout != 0 && players[0].playerAddress.send(player0Payout)){ - emit Payout(players[0].playerAddress, player0Payout); } - if(player1Payout != 0 && players[1].playerAddress.send(player1Payout)){ - emit Payout(players[1].playerAddress, player1Payout); + else revert("invalid choice"); + + // Send the payouts + if(player0Payout > 0) { + (bool success, ) = players[0].playerAddress.call.value(player0Payout)(""); + require(success, 'call failed'); + emit Payout(players[0].playerAddress, player0Payout); + } else if (player1Payout > 0) { + (bool success, ) = players[1].playerAddress.call.value(player1Payout)(""); + require(success, 'call failed'); + emit Payout(players[1].playerAddress, player1Payout); } - //reset the state to play again + // Reset the state to play again delete players; revealDeadline = 0; stage = Stage.FirstCommit; } } - -// ISSUES - -// 1. We should pass in the address of the other player to 'commit', -// then adding a second commit should start the timer - -// concerns - assymetry in setting timeout -// concerns - timeout possibly not greatly reduced by addition of deposit - -// TODO: look at all the access modifiers for all members and functions -// TODO: what are the consequences? - -// TODO: checkout all the integer operations for possible overflows -// TODO: consider event logging - - -// IMPROVMENTS: - -// 1. Allow second player to reveal without committing. -// 2. Allow re-use of the contract? Or allow a self destruct to occur? -// 3. Choose where to send lost deposits. -// 4. Allow a player to forfeit for a cheaper gas cost? - -