[Mainnet] Hashbattle Gaming Contract #84
Conversation
mksmanish79
commented
Oct 26, 2022
•
mksmanish79
commented
| public bool StartBattle(ulong battleId, ulong fee) | ||
| { | ||
| Assert(Message.Sender == BattleOwner, "Only battle owner can start game."); | ||
|
|
||
| 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; | ||
| } |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Thanks for your review. I have added all suggested changes. Please review the pull request again.
| SetBattle(battleId, battle); | ||
|
|
||
| Log(battle); | ||
| return true; |
There was a problem hiding this comment.
The return value of the methods returning bool has no purpose and they can return void instead.
| public ulong BattleId; | ||
| public Address Winner; | ||
| public Address[] Users; | ||
| public uint MaxUsers; |
There was a problem hiding this comment.
Max users appears to be constant, so consider declaring it as a constant instead and exclude it from the struct.
| /// <summary> | ||
| /// 4 different user will enter the battle | ||
| /// </summary> | ||
| public bool EnterBattle(ulong battleId, uint userindex) |
There was a problem hiding this comment.
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.
| battle.Users.SetValue(user.Address, userindex); | ||
| SetBattle(battleId, battle); | ||
|
|
||
| Log(battle); |
There was a problem hiding this comment.
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.
| if (battle.Users.Length <= 4) | ||
| { |
There was a problem hiding this comment.
This if statement is redundant and can be removed to save gas
|
|
||
| SetUser(battleId, userAddress, user); | ||
|
|
||
| if (IsBattleOver) |
There was a problem hiding this comment.
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.
| foreach (Address userAddress in battle.Users) | ||
| { | ||
| var user = GetUser(battle.BattleId, userAddress); | ||
| if (!user.ScoreSubmitted) |
There was a problem hiding this comment.
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.
| } | ||
| } | ||
| uint winnerIndex = GetWinnerIndex(battle.BattleId, battle.Users); | ||
| if (battle.Winner == Address.Zero) |
There was a problem hiding this comment.
This if statement appears to be redundant also
| public Address GetWinner(ulong battleId) | ||
| { | ||
| var battle = GetBattle(battleId); | ||
| Log(battle); |
There was a problem hiding this comment.
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.
| /// <summary> | ||
| /// Set the address deploying the contract as battle owner | ||
| /// </summary> | ||
| private Address BattleOwner |
There was a problem hiding this comment.
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.
StratisIain
changed the title
| <LangVersion>8.0</LangVersion> | ||
| </PropertyGroup> | ||
| <ItemGroup> | ||
| <PackageReference Include="Stratis.SmartContracts" Version="1.2.1" /> |
There was a problem hiding this comment.
| <PackageReference Include="Stratis.SmartContracts" Version="1.2.1" /> | |
| <PackageReference Include="Stratis.SmartContracts" Version="2.0.0" /> |
There was a problem hiding this comment.
Version is not upgraded. Also C#8 may not be supported fully.
| /// </summary> | ||
| private Address BattleOwner | ||
| { | ||
| get => PersistentState.GetAddress(nameof(BattleOwner)); |
There was a problem hiding this comment.
After upgrading package to 2.0.0 then you have to use State
| get => PersistentState.GetAddress(nameof(BattleOwner)); | |
| get => State.GetAddress(nameof(BattleOwner)); |
| private set => PersistentState.SetAddress(nameof(PendingBattleOwner), value); | ||
| } | ||
|
|
||
| public void SetPendingBattleOwnership(Address pendingBattleOwner) |
There was a problem hiding this comment.
These are standard methods so they should not changed. Please follow this one which is common in our contracts.
| /// </summary> | ||
| public ulong NextBattleId | ||
| { | ||
| get => PersistentState.GetUInt64(ArenaStateKeys.NextBattleId); |
There was a problem hiding this comment.
Keep naming consistent for keys.
| get => PersistentState.GetUInt64(ArenaStateKeys.NextBattleId); | |
| get => PersistentState.GetUInt64(nameof(NextBattleId)); |
| var battle = new BattleMain(); | ||
| battle.BattleId = battleId; | ||
| battle.Fee = fee; | ||
| battle.Users = new Address[MaxUsers]; |
There was a problem hiding this comment.
| var battle = new BattleMain(); | |
| battle.BattleId = battleId; | |
| battle.Fee = fee; | |
| battle.Users = new Address[MaxUsers]; | |
| var battle = new BattleMain | |
| { | |
| BattleId = battleId, | |
| Fee = fee, | |
| Users = new Address[MaxUsers] | |
| }; |
| { | ||
| uint winningScore = 0; | ||
| uint winningScoreIndex = 0; | ||
| for (uint i = 0; i < users.Length; i++) |
There was a problem hiding this comment.
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.
| private uint GetWinnerIndex(ulong battleid, Address[] users) | ||
| { | ||
| uint winningScore = 0; | ||
| uint winningScoreIndex = 0; | ||
| for (uint i = 0; i < users.Length; i++) | ||
| { | ||
| var user = GetUser(battleid, users[i]); | ||
| if (user.Score > winningScore) | ||
| { | ||
| winningScore = user.Score; | ||
| winningScoreIndex = i; | ||
| } | ||
| } | ||
| return winningScoreIndex; | ||
| } |
There was a problem hiding this comment.
Last indexed user will be winner in this case if users have same score. Just fyi
| /// </summary> | ||
| private uint GetWinnerIndex(ulong battleid, Address[] users) | ||
| { | ||
| uint winningScore = 0; |
| uint winnerIndex = GetWinnerIndex(battle.BattleId, battle.Users); | ||
| battle.Winner = battle.Users[winnerIndex]; | ||
| SetBattle(battle.BattleId, battle); | ||
| ProcessPrize(battle.BattleId); |
There was a problem hiding this comment.
Why dont you send battle as paramter instead of battleId ?
| ProcessPrize(battle.BattleId); | |
| ProcessPrize(battle); |
| /// </summary> | ||
| public ulong StartBattle(ulong fee) | ||
| { | ||
| Assert(Message.Sender == BattleOwner, "Only battle owner can start game."); |
There was a problem hiding this comment.
Avoid overflows when you process price at the end.
| Assert(Message.Sender == BattleOwner, "Only battle owner can start game."); | |
| Assert(fee < ulong.MaxValue / MaxUsers, "Fee is too high"); |
| ulong battleId = NextBattleId; | ||
| NextBattleId += 1; |
There was a problem hiding this comment.
I recommend this syntax. Also use var or type consistently in the code. i recommend var
| ulong battleId = NextBattleId; | |
| NextBattleId += 1; | |
| var battleId = NextBattleId++; |
There was a problem hiding this comment.
I think this will assign next id to battleId instead of current.
There was a problem hiding this comment.
This will not change current behaviour.
But this statement would change it if it is var battleId = ++NextBattleId;
| { | ||
| var battle = GetBattle(battleid); | ||
| ulong prize = battle.Fee * (MaxUsers - 1); | ||
| Transfer(battle.Winner, prize); |
There was a problem hiding this comment.
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)
{
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);
}
There was a problem hiding this comment.
Winner address will not be a contract address. 4 players will play the game with their wallet address.
There was a problem hiding this comment.
how do you ensure that user addresses will not belong to a another contract ?
|
Added changes as suggested |
| SetHighestScorer(battleId, new BattleHighestScorer { Scorers = scorers, Score = score }); | ||
| } | ||
|
|
||
| if (ScoreSubmittedCount == MaxUsers) |
There was a problem hiding this comment.
You have to assert ScoreSubmittedCount > MaxUsers case at the beginning of the method ?
| /// </summary> | ||
| public class Arena : SmartContract | ||
| { | ||
| private const uint MaxUsers = 4; |
There was a problem hiding this comment.
If there is possibility in future that MaxUsers can be higher than 4 than you can make it constructor parameter.
| public uint Score; | ||
| public Address[] Scorers; | ||
| } | ||
| public struct ClaimPendingDeployerOwnershipLog |
There was a problem hiding this comment.
This log type name should be not changed. It should stays as in original.
| [Index] public Address From; | ||
| [Index] public Address To; | ||
| } | ||
| public struct SetPendingDeployerOwnershipLog |
There was a problem hiding this comment.
This log type name should be not changed. It should stays as in original.
| else if (score == highestScorer.Score) | ||
| { | ||
| var scorers = highestScorer.Scorers; | ||
| Array.Resize(ref scorers, scorers.Length + 1); |
There was a problem hiding this comment.
This allocate new array actually and iterate it by copying fully. We should avoid it. What you need to is highest score and how many times same highest score counted and highest user. You can keep last highest one always whenever equal score submited. So you can do something like this
public struct BattleHighestScorer
{
public uint Score;
public uint HighestScoreCount;
public Address HighestScorer;
}
if (score > highestScorer.Score)
{
highestScorer.Score = score;
highestScorer.HighestScorer = userAddress;
highestScorer.HighestScoreCount = 1;
SetHighestScorer(battleId, highestScorer);
}
else if (score == highestScorer.Score)
{
highestScorer.HighestScoreCount++;
SetHighestScorer(battleId, highestScorer);
}
if (ScoreSubmittedCount == MaxUsers)
{
highestScorer = GetHighestScorer(battleId);
if (highestScorer.HighestScoreCount > 1)
CancelBattle(battle);
else
ProcessWinner(battle, highestScorer.HighestScorer);
}
|
|
||
| SetBattle(battleId, battle); | ||
|
|
||
| Log(new BattleEventLog { Event = "Enter", BattleId = battleId, Address = Message.Sender }); |
There was a problem hiding this comment.
| Log(new BattleEventLog { Event = "Enter", BattleId = battleId, Address = Message.Sender }); | |
| Log(new BattleStartedLog { BattleId = battleId, Address = Message.Sender }); |
| var userindex = GetUserIndex(battleId); | ||
| Assert(userindex != MaxUsers, "Max user reached for this battle."); | ||
| battle.Users.SetValue(Message.Sender, userindex); | ||
| SetUserIndex(battleId, (userindex + 1)); |
There was a problem hiding this comment.
| SetUserIndex(battleId, (userindex + 1)); | |
| SetUserIndex(battleId, userindex + 1); |
| } | ||
| private void SetUser(ulong battleId, Address address, BattleUser user) | ||
| { | ||
| State.SetStruct($"user:{battleId}-{address}", user); |
There was a problem hiding this comment.
Always use : for multiple key seperator and do this for others.
| State.SetStruct($"user:{battleId}-{address}", user); | |
| State.SetStruct($"user:{battleId}:{address}", user); |
There was a problem hiding this comment.
Also these are getters and setters method over State so please move them to top of constructor.
|
|
||
| Assert(!user.ScoreSubmitted, "The user already submitted score."); | ||
|
|
||
| user.Score = score; |
There was a problem hiding this comment.
What for we save Score on user ? It seems there is no use case for it.
| public class Arena : SmartContract | ||
| { | ||
| private const uint MaxUsers = 4; | ||
| public struct BattleMain |
There was a problem hiding this comment.
All struct definitions should go to bottom of the class.
|
Added changes as suggested |
| Assert(fee < ulong.MaxValue / MaxUsers, "Fee is too high"); | ||
|
|
||
| var battleId = NextBattleId; | ||
| NextBattleId += 1; |
There was a problem hiding this comment.
You access value of NextBattleId twice in this code so please follow my recommandation.
| Assert(fee < ulong.MaxValue / MaxUsers, "Fee is too high"); | ||
|
|
||
| var battleId = NextBattleId; | ||
| NextBattleId += 1; |
There was a problem hiding this comment.
Or alternatively you can use below code.
| NextBattleId += 1; | |
| NextBattleId = battleId + 1; |
| public uint HighestScoreCount; | ||
| public Address HighestScorer; | ||
| } | ||
| public struct OwnershipTransferedLog |
There was a problem hiding this comment.
Typo
| public struct OwnershipTransferedLog | |
| public struct OwnershipTransferredLog |
|
@YakupIpek Added the additional feedback changes. Please merge the pull request. |
