Skip to content

RockPaperScissors contract improvements #1

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
kadenzipfel wants to merge 6 commits into
base: master
Choose a base branch
from
Open

RockPaperScissors contract improvements #1

Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
3fe624a
Fix compiler erros
kadenzipfel Jan 12, 2020
2e96f8c
Overflow protection
kadenzipfel Jan 12, 2020
ab111c5
Replace transfer and send with low-level calls
kadenzipfel Jan 12, 2020
d8fbba4
Add assert statements
kadenzipfel Jan 12, 2020
549daff
Change version and fix errors
kadenzipfel Jan 12, 2020
956fe9f
Modify comments
kadenzipfel Jan 12, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
162 changes: 70 additions & 92 deletions contracts/RockPaperScissors.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
pragma solidity ^0.4.24;
pragma solidity ^0.5.0;

contract RockPaperScissors {
enum Choice {
Expand All @@ -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;
Expand All @@ -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?