Skip to content
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.

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

[Mainnet] Hashbattle Gaming Contract #84

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
31 changes: 31 additions & 0 deletions Mainnet/HashBattle/HashBattle.sln
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@

Microsoft Visual Studio Solution File, Format Version 12.00
# Visual Studio Version 16
VisualStudioVersion = 16.0.31624.102
MinimumVisualStudioVersion = 10.0.40219.1
Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "HashBattle", "HashBattle\HashBattle.csproj", "{D711FA52-750E-481B-9BC5-2E07EBF58240}"
EndProject
Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "HashBattleTest", "HashBattleTest\HashBattleTest.csproj", "{2A7F2670-A17F-46E6-BF35-A2C55BC7DCB1}"
EndProject
Global
GlobalSection(SolutionConfigurationPlatforms) = preSolution
Debug|Any CPU = Debug|Any CPU
Release|Any CPU = Release|Any CPU
EndGlobalSection
GlobalSection(ProjectConfigurationPlatforms) = postSolution
{D711FA52-750E-481B-9BC5-2E07EBF58240}.Debug|Any CPU.ActiveCfg = Debug|Any CPU
{D711FA52-750E-481B-9BC5-2E07EBF58240}.Debug|Any CPU.Build.0 = Debug|Any CPU
{D711FA52-750E-481B-9BC5-2E07EBF58240}.Release|Any CPU.ActiveCfg = Release|Any CPU
{D711FA52-750E-481B-9BC5-2E07EBF58240}.Release|Any CPU.Build.0 = Release|Any CPU
{2A7F2670-A17F-46E6-BF35-A2C55BC7DCB1}.Debug|Any CPU.ActiveCfg = Debug|Any CPU
{2A7F2670-A17F-46E6-BF35-A2C55BC7DCB1}.Debug|Any CPU.Build.0 = Debug|Any CPU
{2A7F2670-A17F-46E6-BF35-A2C55BC7DCB1}.Release|Any CPU.ActiveCfg = Release|Any CPU
{2A7F2670-A17F-46E6-BF35-A2C55BC7DCB1}.Release|Any CPU.Build.0 = Release|Any CPU
EndGlobalSection
GlobalSection(SolutionProperties) = preSolution
HideSolutionNode = FALSE
EndGlobalSection
GlobalSection(ExtensibilityGlobals) = postSolution
SolutionGuid = {88A96B52-6F00-40C9-AA28-C3D79E3BC0DF}
EndGlobalSection
EndGlobal
194 changes: 194 additions & 0 deletions Mainnet/HashBattle/HashBattle/Arena.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,194 @@
using Stratis.SmartContracts;
using System;
using System.Text;

/// <summary>
/// A Stratis smart contract for running a game battle where owner will start the battle and maximum 4 users can enter a battle
/// </summary>
public class Arena : SmartContract
{
public Arena(ISmartContractState smartContractState)
: base(smartContractState)
{
BattleOwner = Message.Sender;
}

/// <summary>
/// Set the address deploying the contract as battle owner
/// </summary>
private Address BattleOwner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider adding the ability to migrate the battle owner address to a new address. This way, if the battle owner ever needs to migrate wallets, they can change the owner address of the contract and continue to retrieve battle fees. See this example of how to implement it.

{
get => PersistentState.GetAddress(nameof(BattleOwner));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After upgrading package to 2.0.0 then you have to use State

Suggested change
get => PersistentState.GetAddress(nameof(BattleOwner));
get => State.GetAddress(nameof(BattleOwner));

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not done.

set => PersistentState.SetAddress(nameof(BattleOwner), value);
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider making this public with a private setter.


/// <summary>
/// Battle owner will start the battle
/// </summary>
public bool StartBattle(ulong battleId, ulong fee)
{
Assert(Message.Sender == BattleOwner, "Only battle owner can start game.");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Avoid overflows when you process price at the end.

Suggested change
Assert(Message.Sender == BattleOwner, "Only battle owner can start game.");
Assert(fee < ulong.MaxValue / MaxUsers, "Fee is too high");


var battle = new BattleMain();
battle.BattleId = battleId;
battle.MaxUsers = 4;
battle.Fee = fee;
battle.Users = new Address[battle.MaxUsers];
SetBattle(battleId, battle);

Log(battle);
return true;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The return value of the methods returning bool has no purpose and they can return void instead.

}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider storing battle id as an incremental state value, otherwise the battle owner can overwrite the result of a battle by calling StartBattle. As an example, see NextProposalId in the Opdex vault contract.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your review. I have added all suggested changes. Please review the pull request again.


/// <summary>
/// 4 different user will enter the battle
/// </summary>
public bool EnterBattle(ulong battleId, uint userindex)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

User index/count should be stored in state on the battle struct and incremented, otherwise anyone can take the place of another user in an ongoing battle, by calling EnterBattle with the index of an existing entrant. It's also possible for a user to enter the same battle multiple times by calling EnterBattle twice with a different index each time.

{
var battle = GetBattle(battleId);

Assert(battle.Winner == Address.Zero, "Battle ended.");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Assert(battle.Winner == Address.Zero, "Battle ended.");
Assert(battle.Winner == Address.Zero, "Battle not found.");


Assert(battle.Fee == Message.Value, "Battle amount is not matching.");

var user = GetUser(battleId, Message.Sender);

Assert(!user.ScoreSubmitted, "The user already submitted score.");

user.Address = Message.Sender;

SetUser(battleId, Message.Sender, user);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are sending user address 2 times in here. One is Message.Sender and other is as user parameter with Address property. I question it is needed to add Address property for user ? Because you can not get/set user without knowing the address already.


battle.Users.SetValue(user.Address, userindex);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can set this way as well. It is same thing and up to you.

Suggested change
battle.Users.SetValue(user.Address, userindex);
battle.Users[userindex] = user.Address;

SetBattle(battleId, battle);

Log(battle);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You may want to consider logging events rather than the battle details. For example, you could define an event like so:

public struct HashBattleEnteredEvent
{
    public ulong BattleId;
    public Address Address;
}

Then here you would call

Log(new HashBattleEnteredEvent { BattleId = battleId, Address = Message.Sender }

This way it'd be easier to index and process the logs on your dApp. You're able to search the full node for specific events rather than having to analyze the entire sequence of events to determine which action happened at a point in time.

return true;
}

/// <summary>
/// 4 different user will end the battle and submit the score
/// </summary>
public bool EndBattle(Address userAddress, ulong battleId, uint score, bool IsBattleOver)
{
Assert(Message.Sender == BattleOwner, "Only battle owner can end game.");

var battle = GetBattle(battleId);

Assert(battle.Winner == Address.Zero, "Battle ended.");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Assert(battle.Winner == Address.Zero, "Battle ended.");
Assert(battle.Winner == Address.Zero, "Battle not found.");


var user = GetUser(battleId, userAddress);

Assert(!user.ScoreSubmitted, "The user already submitted score.");

user.Score = score;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What for we save Score on user ? It seems there is no use case for it.

user.ScoreSubmitted = true;

SetUser(battleId, userAddress, user);

if (IsBattleOver)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Appears that if all scores are submitted, yet IsBattleOver is false, then the fees will never be paid out to the winner or battle owner. Instead you should determine within the method whether all scores have been submitted and then process the winner accordingly.

Another way to go about it would be to submit all of the scores in a single method call, which would save a lot of gas fees for the battle owner.

ProcessWinner(battle);

Log(user);
return true;
}

/// <summary>
/// Get winner address
/// </summary>
public Address GetWinner(ulong battleId)
{
var battle = GetBattle(battleId);
Log(battle);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This log serves no purpose and should be removed. Logs should only be used for events, or in other words logging state changes, otherwise you're wasting gas and making it a lot harder to determine what happened on chain.

return battle.Winner;
}

/// <summary>
/// Process winner when all user scores are submitted
/// </summary>
private void ProcessWinner(BattleMain battle)
{
if (battle.Users.Length <= 4)
{

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This if statement is redundant and can be removed to save gas

foreach (Address userAddress in battle.Users)
{
var user = GetUser(battle.BattleId, userAddress);
if (!user.ScoreSubmitted)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This if statement is redundant as it is impossible for the winner to be processed if not all scores are submitted. Remove to save gas.

return;
}
}
uint winnerIndex = GetWinnerIndex(battle.BattleId, battle.Users);
if (battle.Winner == Address.Zero)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This if statement appears to be redundant also

{
battle.Winner = battle.Users[winnerIndex];
SetBattle(battle.BattleId, battle);
ProcessPrize(battle.BattleId);
}
}

/// <summary>
/// Get winner user index from battle users
/// </summary>
private uint GetWinnerIndex(ulong battleid, Address[] users)
{
uint winningScore = 0;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

var always

uint winningScoreIndex = 0;
for (uint i = 0; i < users.Length; i++)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Iterations are not recommended for contracts as general design guidelines.

My recommendation is on EndBattle method keep track of highest one in the state. Then you can remove Battle.Users property fully.

{
var user = GetUser(battleid, users[i]);
if (user.Score > winningScore)
{
winningScore = user.Score;
winningScoreIndex = i;
}
}
return winningScoreIndex;
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you need to consider what happens in the case of a tie? As it stands, the user with the lowest index will be determined as the winner.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Last indexed user will be winner in this case if users have same score. Just fyi


/// <summary>
/// Send 3/4 amount to winner and 1/4 amount to battle owner
/// </summary>
private void ProcessPrize(ulong battleid)
{
var battle = GetBattle(battleid);
ulong prize = battle.Fee * (battle.MaxUsers - 1);
Transfer(battle.Winner, prize);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If winner address is a contract address then this transfer has possibility to fail. So you can use withdrawal pattern(pull) in here. It will transfer funds immediately if destination is not a contract address and if it is then it will fallback to withdrawal pattern.

private bool SafeTransfer(Address to, ulong amount)

private bool SafeTransfer(Address to, ulong amount)
{
    if (State.IsContract(to))
    {
        var balance = GetBalance(to) + amount;

        SetBalance(to, balance);

        return true;
    }

    var result = Transfer(to, amount);

    return result.Success;
}

public bool Withdraw()
{
    EnsureNotPayable();

    var amount = GetBalance(Message.Sender);

    Assert(amount > 0);

    SetBalance(Message.Sender, 0);

    var transfer = Transfer(Message.Sender, amount);

    Assert(transfer.Success, "Transfer failed.");

    Log(new BalanceRefundedLog { To = Message.Sender, Amount = amount });

    return transfer.Success;
}

public ulong GetBalance(Address address)
{
    return State.GetUInt64($"Balance:{address}");
}

private void SetBalance(Address address, ulong balance)
{
    State.SetUInt64($"Balance:{address}", balance);
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Winner address will not be a contract address. 4 players will play the game with their wallet address.

Copy link
Member

@YakupIpek YakupIpek Jun 12, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how do you ensure that user addresses will not belong to a another contract ?

Transfer(BattleOwner, battle.Fee);
}

private void SetUser(ulong battleid, Address address, BattleUser user)
{
PersistentState.SetStruct($"user:{battleid}:{address}", user);
}

private BattleUser GetUser(ulong battleid, Address address)
{
return PersistentState.GetStruct<BattleUser>($"user:{battleid}:{address}");
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider making this public so anyone can determine users scores without having to analyze and parse logs


private void SetBattle(ulong battleid, BattleMain battle)
{
PersistentState.SetStruct($"battle:{battleid}", battle);
}

private BattleMain GetBattle(ulong battleid)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider making this public so anyone can determine the state of an ongoing battle without having to analyze and parse logs

{
return PersistentState.GetStruct<BattleMain>($"battle:{battleid}");
}

public struct BattleMain
{
public ulong BattleId;
public Address Winner;
public Address[] Users;
public uint MaxUsers;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Max users appears to be constant, so consider declaring it as a constant instead and exclude it from the struct.

public ulong Fee;
}

public struct BattleUser
{
public Address Address;
public uint Score;
public bool ScoreSubmitted;
}
}
11 changes: 11 additions & 0 deletions Mainnet/HashBattle/HashBattle/HashBattle.csproj
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
<Project Sdk="Microsoft.NET.Sdk">

<PropertyGroup>
<TargetFramework>netcoreapp2.1</TargetFramework>

<LangVersion>8.0</LangVersion>
</PropertyGroup>
<ItemGroup>
<PackageReference Include="Stratis.SmartContracts" Version="1.2.1" />
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
<PackageReference Include="Stratis.SmartContracts" Version="1.2.1" />
<PackageReference Include="Stratis.SmartContracts" Version="2.0.0" />

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Version is not upgraded. Also C#8 may not be supported fully.

</ItemGroup>
</Project>
145 changes: 145 additions & 0 deletions Mainnet/HashBattle/HashBattleTest/ArenaTest.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,145 @@
using System;
using Moq;
using Stratis.SmartContracts;
using Stratis.SmartContracts.CLR;
using Xunit;
using static Arena;

namespace HashBattleTest
{
public class ArenaTest
{
private readonly IPersistentState state;
private readonly Mock<ISmartContractState> mockContractState;
private readonly Mock<IPersistentState> mockPersistentState;
private readonly Mock<IContractLogger> mockContractLogger;
private readonly Mock<IInternalTransactionExecutor> mTransactionExecutor;
private Address contract;
private Address ownerAddress;
private Address playerAddress1;
private Address playerAddress2;
private Address playerAddress3;
private Address playerAddress4;

public ArenaTest()
{
this.state = new InMemoryState();
this.mockPersistentState = new Mock<IPersistentState>();
this.mockContractState = new Mock<ISmartContractState>();
this.mockContractLogger = new Mock<IContractLogger>();
this.mTransactionExecutor = new Mock<IInternalTransactionExecutor>();
this.mockContractState.Setup(s => s.PersistentState).Returns(this.state);
this.mockContractState.Setup(s => s.ContractLogger).Returns(this.mockContractLogger.Object);
this.mockContractState.Setup(s => s.InternalTransactionExecutor).Returns(this.mTransactionExecutor.Object);
this.contract = "0x0000000000000000000000000000000000000001".HexToAddress();
this.ownerAddress = "0x0000000000000000000000000000000000000002".HexToAddress();
this.playerAddress1 = "0x0000000000000000000000000000000000000003".HexToAddress();
this.playerAddress2 = "0x0000000000000000000000000000000000000004".HexToAddress();
this.playerAddress3 = "0x0000000000000000000000000000000000000005".HexToAddress();
this.playerAddress4 = "0x0000000000000000000000000000000000000006".HexToAddress();
}


[Fact]
public void TestBattle()
{
Arena arena = StartBattleTest();
Player1EnterGameTest(arena);
Player2EnterGameTest(arena);
Player3EnterGameTest(arena);
Player4EnterGameTest(arena);
Player1EndGameTest(arena);
Player2EndGameTest(arena);
Player3EndGameTest(arena);
Player4EndGameTest(arena);
GetGameWinnerTest(arena);
}

private Arena StartBattleTest()
{
this.mockContractState.Setup(m => m.Message).Returns(new Message(this.contract, this.ownerAddress, 0));
Arena arena = new Arena(this.mockContractState.Object);
arena.StartBattle(1, 1);

this.mockContractLogger.Verify(m => m.Log(this.mockContractState.Object, state.GetStruct<BattleMain>($"battle:{1}")));
return arena;
}

private void Player1EnterGameTest(Arena arena)
{
this.mockContractState.Setup(m => m.Message).Returns(new Message(this.contract, this.playerAddress1, 1));
arena.EnterBattle(1, 0);

Assert.Equal(this.playerAddress1, state.GetStruct<BattleUser>($"user:{1}:{this.playerAddress1}").Address);
this.mockContractLogger.Verify(m => m.Log(this.mockContractState.Object, state.GetStruct<BattleMain>($"battle:{1}")));
}

private void Player2EnterGameTest(Arena arena)
{
this.mockContractState.Setup(m => m.Message).Returns(new Message(this.contract, this.playerAddress2, 1));
arena.EnterBattle(1, 1);

Assert.Equal(this.playerAddress2, state.GetStruct<BattleUser>($"user:{1}:{this.playerAddress2}").Address);
this.mockContractLogger.Verify(m => m.Log(this.mockContractState.Object, state.GetStruct<BattleMain>($"battle:{1}")));
}

private void Player3EnterGameTest(Arena arena)
{
this.mockContractState.Setup(m => m.Message).Returns(new Message(this.contract, this.playerAddress3, 1));
arena.EnterBattle(1, 2);

Assert.Equal(this.playerAddress3, state.GetStruct<BattleUser>($"user:{1}:{this.playerAddress3}").Address);
this.mockContractLogger.Verify(m => m.Log(this.mockContractState.Object, state.GetStruct<BattleMain>($"battle:{1}")));
}

private void Player4EnterGameTest(Arena arena)
{
this.mockContractState.Setup(m => m.Message).Returns(new Message(this.contract, this.playerAddress4, 1));
arena.EnterBattle(1, 3);

Assert.Equal(this.playerAddress4, state.GetStruct<BattleUser>($"user:{1}:{this.playerAddress4}").Address);
this.mockContractLogger.Verify(m => m.Log(this.mockContractState.Object, state.GetStruct<BattleMain>($"battle:{1}")));
}

private void Player1EndGameTest(Arena arena)
{
this.mockContractState.Setup(m => m.Message).Returns(new Message(this.contract, this.ownerAddress, 0));
arena.EndBattle(this.playerAddress1, 1, 10, false);

this.mockContractLogger.Verify(m => m.Log(this.mockContractState.Object, state.GetStruct<BattleUser>($"user:{1}:{this.playerAddress1}")));
}

private void Player2EndGameTest(Arena arena)
{
this.mockContractState.Setup(m => m.Message).Returns(new Message(this.contract, this.ownerAddress, 0));
arena.EndBattle(this.playerAddress2, 1, 20, false);

this.mockContractLogger.Verify(m => m.Log(this.mockContractState.Object, state.GetStruct<BattleUser>($"user:{1}:{this.playerAddress2}")));
}

private void Player3EndGameTest(Arena arena)
{
this.mockContractState.Setup(m => m.Message).Returns(new Message(this.contract, this.ownerAddress, 0));
arena.EndBattle(this.playerAddress3, 1, 30, false);

this.mockContractLogger.Verify(m => m.Log(this.mockContractState.Object, state.GetStruct<BattleUser>($"user:{1}:{this.playerAddress3}")));
}

private void Player4EndGameTest(Arena arena)
{
this.mockContractState.Setup(m => m.Message).Returns(new Message(this.contract, this.ownerAddress, 0));
arena.EndBattle(this.playerAddress4, 1, 40, true);

this.mockContractLogger.Verify(m => m.Log(this.mockContractState.Object, state.GetStruct<BattleUser>($"user:{1}:{this.playerAddress4}")));
}

private void GetGameWinnerTest(Arena arena)
{
this.mockContractState.Setup(m => m.Message).Returns(new Message(this.contract, this.ownerAddress, 0));
Address winner = arena.GetWinner(1);

Assert.Equal(this.playerAddress4, winner);
this.mockContractLogger.Verify(m => m.Log(this.mockContractState.Object, state.GetStruct<BattleMain>($"battle:{1}")));
}
}
}
Loading