Kirill Review and some small fixes

contracts/zxp/Games.sol

@@ -1,29 +1,43 @@
 // SPDX-License-Identifier: MIT
 pragma solidity ^0.8.19;
 
-import {IGames} from "./interfaces/IGames.sol";
-import {ObjectRegistry} from "./ObjectRegistry.sol";
+import { IGames } from "./interfaces/IGames.sol";
+import { ObjectRegistry } from "./ObjectRegistry.sol";
 
+
+// why does this need to be ObjectRegistry? where is this functionality used?
+// mb also change the inheritance order. go with interface last
 contract Games is IGames, ObjectRegistry {
     struct Game {
         string metadata;
+        // this should be named better. it's confusing what this is
         ObjectRegistry objects;
     }
+
     mapping(bytes32 name => Game game) public games;
 
     constructor() ObjectRegistry(msg.sender) {}
 
     function createGame(
         bytes32 name,
+        // why is this not an msg.sender? to set someone else as an owner?
         address owner,
         string calldata metadata,
+        // who deploys these contracts and who pays for each deployment of
+        // these sets of objects per EVERY game?
         bytes32[] calldata objectNames,
         address[] calldata objectAddresses
     ) external override {
+        // what is the difference between this ObjectRegistry and the one we inherited here, initialized in the constructor?
+        // also, does there HAVE to be a new registry for every game?
+        // what are the advantages of this?
         ObjectRegistry newRegistry = new ObjectRegistry(owner);
         games[name].metadata = metadata;
         games[name].objects = newRegistry;
         if (objectNames.length > 0) {
+            // this call will always fail because msg.sender is the contract and not the owner
+            // that is set above on L31
+            // this is also never tested, and it seems to be a crucial function
             newRegistry.registerObjects(objectNames, objectAddresses);
         }
     }

contracts/zxp/ObjectRegistry.sol

@@ -1,10 +1,13 @@
 // SPDX-License-Identifier: MIT
 pragma solidity ^0.8.19;
 
-import {IObjectRegistry} from "./interfaces/IObjectRegistry.sol";
+import { IObjectRegistry } from "./interfaces/IObjectRegistry.sol";
+
 
 contract ObjectRegistry is IObjectRegistry {
     bytes32 internal constant OWNER = "Owner";
+    // what is an object? is it a contract?
+    // the name is too generic and is not clear what it is
     mapping(bytes32 name => address object) public objects;
 
     constructor(address owner) {
@@ -15,14 +18,20 @@ contract ObjectRegistry is IObjectRegistry {
         bytes32[] calldata objectNames,
         address[] calldata objectAddresses
     ) public override {
+        // does this work? if it comes from Games.createGame() then msg.sender is that contract and not the owner wallet.
         require(msg.sender == objects[OWNER], "ZXP Not game owner");
+        // should we check that the length of both of these arrays are equal?
+        // otherwise we can get wrong opcode error. but it can be a way to prevent passing arrays of different lengths...
+        // need to test this
         require(objectNames.length > 0, "ZXP Objects empty");
         for (uint256 i = 0; i < objectNames.length; i++) {
             objects[objectNames[i]] = objectAddresses[i];
         }
     }
 
     function addressOf(
+    // where are these names stored?
+    // is it up to the owner of the game to store them somewhere off-chain?
         bytes32 objectName
     ) external view override returns (address) {
         return objects[objectName];

contracts/zxp/ObjectRegistryClient.sol

@@ -1,7 +1,8 @@
 // SPDX-License-Identifier: MIT
 pragma solidity ^0.8.19;
 
-import {IObjectRegistry} from "./interfaces/IObjectRegistry.sol";
+import { IObjectRegistry } from "./interfaces/IObjectRegistry.sol";
+
 
 /**
  * @dev Base contract for Registry clients

contracts/zxp/game/GameVault.sol

@@ -9,35 +9,40 @@ import {IObjectRegistry} from "../interfaces/IObjectRegistry.sol";
 import {ObjectRegistryClient} from "../ObjectRegistryClient.sol";
 import {ISeasons} from "./interfaces/ISeasons.sol";
 
+
 contract GameVault is ERC721Wrapper, ObjectRegistryClient, IGameVault {
     bytes32 internal constant SEASONS = "Seasons";
     mapping(uint id => uint block) public stakedAt;
 
     constructor(
         IERC721 underlyingToken,
         IERC20 _rewardToken,
-        string memory name,
-        string memory symbol,
+        string memory underlyingTokenName,
+        string memory underlyingTokenSymbol,
         IObjectRegistry registry,
         bytes32 game
     )
         ObjectRegistryClient(registry)
-        ERC721(name, symbol)
+        ERC721(underlyingTokenName, underlyingTokenSymbol)
         ERC721Wrapper(underlyingToken)
     {}
 
+    // what is this `id` for? how do we get it? maybe a clearer name?
     function _mint(address to, uint id) internal override {
         stakedAt[id] = block.number;
         super._mint(to, id);
     }
 
     function _burn(uint id) internal override {
+        // moved this for checks-effects pattern to avoid reentrancy
+        uint stakedAt = stakedAt[id];
+        stakedAt[id] = 0;
+
         ISeasons(registry.addressOf(SEASONS)).onUnstake(
             id,
             msg.sender,
-            block.number - stakedAt[id]
+            block.number - stakedAt
         );
-        stakedAt[id] = 0;
         super._burn(id);
     }
 }

contracts/zxp/game/XP.sol

@@ -7,13 +7,16 @@ import {QuadraticLevelCurve} from "./QuadraticLevelCurve.sol";
 import {ERC20} from "@openzeppelin/contracts/token/ERC20/ERC20.sol";
 import {IXP} from "./interfaces/IXP.sol";
 
+
+// where is QuadraticLevelCurve logic used here? why do we inherit it?
 contract XP is ObjectRegistryClient, QuadraticLevelCurve, ERC20, IXP {
     bytes32 internal constant SEASONS = "Seasons";
 
     constructor(
         string memory name,
         string memory symbol,
         IObjectRegistry registry,
+        // why is this passed if it's not used?
         bytes32 game
     ) ObjectRegistryClient(registry) ERC20(name, symbol) {}
 
@@ -22,10 +25,15 @@ contract XP is ObjectRegistryClient, QuadraticLevelCurve, ERC20, IXP {
         address to,
         uint amount
     ) internal virtual override {
+        // does this need to support burning and minting? is that why it checks for `from` and `to`?
+        // can minting be even done through transfer()? if not, then do we need `from` check?
+        // could it just be a single `revert("ZXP: Token soulbound")`?
+        // or it may even be on `transfer()` method instead of `_beforeTokenTransfer()`
         require(from == address(0) || to == address(0), "ZXP: Token soulbound");
         super._beforeTokenTransfer(from, to, amount);
     }
 
+    // does this mean this will not work if Seasons contract is not used?
     function awardXP(address to, uint amount) external override only(SEASONS) {
         _mint(to, amount);
     }

contracts/zxp/game/seasons/Seasons.sol

@@ -13,6 +13,8 @@ contract Seasons is ObjectRegistryClient, ISeasons, Ownable {
     bytes32 internal constant STAKER_REWARDS = "StakerRewards";
     bytes32 internal constant XP = "XP";
     uint public currentSeason;
+    // is this a constant or a var? if a constant, make it a constant to save gas in tx,
+    // if a var, we need a setter for it
     uint public stakerXPReward = 100;
 
     struct Season {
@@ -21,6 +23,7 @@ contract Seasons is ObjectRegistryClient, ISeasons, Ownable {
         uint end;
         ObjectRegistry objects;
     }
+
     mapping(uint season => Season data) public seasons;
 
     modifier preseason(uint season) {
@@ -65,6 +68,8 @@ contract Seasons is ObjectRegistryClient, ISeasons, Ownable {
         require(seasons[currentSeason].start != 0, "ZXP season not started");
         seasons[currentSeason].end = block.number;
         currentSeason++;
+        // will this always be the case? we assume here that the next season will always start at the end of the current one
+        // user also pays to start a new season when he may not need it
         seasons[currentSeason].objects = new ObjectRegistry(msg.sender);
     }
 
@@ -73,8 +78,11 @@ contract Seasons is ObjectRegistryClient, ISeasons, Ownable {
         address to,
         uint stakedAt
     ) external override only(GAME_VAULT) {
+        // what if this type of contract is not in the .objects? can other contracts be passed there?
+        // and if it always have to be present, where are the checks that a correct contract has been set?
         IStakerRewards(seasons[currentSeason].objects.addressOf(STAKER_REWARDS))
             .onUnstake(id, to, stakedAt);
+        // same here, what if this contract is not added or other contract is added that has a different ABI?
         IXP(registry.addressOf(XP)).awardXP(
             to,
             stakerXPReward * (block.number - stakedAt)

contracts/zxp/game/seasons/mechanics/PlayerRewards.sol

@@ -9,6 +9,7 @@ import {ObjectRegistryClient} from "../../../ObjectRegistryClient.sol";
 import {IObjectRegistry} from "../../../interfaces/IObjectRegistry.sol";
 import {ISeasons} from "../../interfaces/ISeasons.sol";
 
+
 contract PlayerRewards is ObjectRegistryClient, Ownable, IPlayerRewards {
     bytes32 internal constant name = "PlayerRewards";
     IERC20 public rewardToken;
@@ -22,6 +23,8 @@ contract PlayerRewards is ObjectRegistryClient, Ownable, IPlayerRewards {
         ISeasons seasonManager,
         uint xpRewarded
     )
+    // this seems to be called on every single contract. what is the point of it?
+    // if we already have access to this value, why do we need to write it in every single state?
         ObjectRegistryClient(
             IObjectRegistry(
                 seasonManager.getRegistryAddress(seasonManager.currentSeason())
@@ -38,6 +41,12 @@ contract PlayerRewards is ObjectRegistryClient, Ownable, IPlayerRewards {
         rewardToken.transfer(to, rewards[to]);
     }
 
+    // when is this called? why 3 results?
+    // why not addresses and uints arrays?
+    // also, what is the point of this contract if the decision to disperse
+    // rewards is made by the caller by an external call at arbitrary time?
+    // we can just do this all off-chain and the result seems the same
+    // from the trust standpoint
     function submitTop3Results(
         address first,
         address second,

contracts/zxp/game/seasons/mechanics/SecretRewards.sol

@@ -18,8 +18,10 @@ contract SecretRewards is ObjectRegistryClient, ISecretRewards {
         ISeasons seasonManager,
         uint xpRewarded
     )
+    // here another double-external call to set the variable that's already available
         ObjectRegistryClient(
             IObjectRegistry(
+            // is there a different registry contract for every season?
                 seasonManager.getRegistryAddress(seasonManager.currentSeason())
             )
         )
@@ -33,6 +35,7 @@ contract SecretRewards is ObjectRegistryClient, ISecretRewards {
         string reveal;
     }
 
+    // what is the reason there's no nonce counter on the contract?
     mapping(uint nonce => Commitment commit) public secrets;
     mapping(address player => mapping(uint nonce => Commitment commit))
         public guesses;
@@ -47,7 +50,7 @@ contract SecretRewards is ObjectRegistryClient, ISecretRewards {
     function commitGuess(uint nonce, bytes32 guessHash) public override {
         require(
             bytes(secrets[nonce].reveal).length == 0,
-            "No overwrite after reveal"
+            "No overwrite after reveal" // seems like a wrong message
         );
         guesses[msg.sender][nonce] = Commitment({
             secretHash: guessHash,
@@ -62,13 +65,15 @@ contract SecretRewards is ObjectRegistryClient, ISecretRewards {
 
         require(
             guessHash == guesses[msg.sender][nonce].secretHash,
-            "Invalid reveal"
+            "Invalid reveal" // unclear message
         );
         require(
             bytes(secrets[nonce].reveal).length != 0,
             "Answer not revealed"
         );
         require(
+            // why is this necessary to hash these here and not just compare string directly?
+            // what do we miss when just comparing strings?
             keccak256(abi.encode(guess)) ==
                 keccak256(abi.encode(secrets[nonce].reveal)),
             "Wrong answer"
@@ -88,6 +93,12 @@ contract SecretRewards is ObjectRegistryClient, ISecretRewards {
         emit SecretCommitted(nonce, secretHash);
     }
 
+    // this whole flow seems redundant. an user that has the hash
+    // (that is available on the contract by the time a user commits a guess,
+    // he can locally keep hashing his guesses to get the same hash,
+    // once he gets the same hash he (and we) already know that his guess is correct,
+    // so we should be able to reward him at guess commit time and this whole reveal secret flow
+    // seems redundant
     function revealSecret(
         uint nonce,
         string memory secret

contracts/zxp/game/seasons/mechanics/StakerRewards.sol

@@ -9,6 +9,7 @@ import {ISeasons} from "../../interfaces/ISeasons.sol";
 import {ObjectRegistryClient} from "../../../ObjectRegistryClient.sol";
 import {IObjectRegistry} from "../../../interfaces/IObjectRegistry.sol";
 
+
 contract StakerRewards is ObjectRegistryClient, IStakerRewards {
     IERC20 public rewardToken;
     uint public rewardPerBlock;
@@ -19,6 +20,7 @@ contract StakerRewards is ObjectRegistryClient, IStakerRewards {
     mapping(address awardee => uint amount) public rewards;
     mapping(uint nft => uint block) public claimedAt;
 
+    // incorrect naming here. what is even registry in zXP? is it seasons? is it ObjectRegistry?
     modifier onlyRegistry(address checked) {
         require(checked == address(seasons), "ZXP: not registry");
         _;
@@ -49,6 +51,7 @@ contract StakerRewards is ObjectRegistryClient, IStakerRewards {
     function onUnstake(
         uint id,
         address to,
+        // why don't we check this value against a mapping in GameVault ?
         uint stakedAt
     ) external override onlyRegistry(msg.sender) {
         uint numBlocks;