From 0dfbc20969f7c99d61518ac82f4e5f8fc380904c Mon Sep 17 00:00:00 2001 From: Kirill Date: Tue, 13 Feb 2024 15:43:13 -0800 Subject: [PATCH] add comments and some tiny fixes --- contracts/zxp/Games.sol | 18 ++++++++- contracts/zxp/ObjectRegistry.sol | 11 +++++- contracts/zxp/ObjectRegistryClient.sol | 3 +- contracts/zxp/game/GameVault.sol | 15 +++++--- contracts/zxp/game/XP.sol | 8 ++++ contracts/zxp/game/seasons/Seasons.sol | 8 ++++ .../game/seasons/mechanics/PlayerRewards.sol | 9 +++++ .../game/seasons/mechanics/SecretRewards.sol | 15 +++++++- .../game/seasons/mechanics/StakerRewards.sol | 3 ++ test/zXP.test.ts | 38 +++++++++++++++++++ 10 files changed, 117 insertions(+), 11 deletions(-) diff --git a/contracts/zxp/Games.sol b/contracts/zxp/Games.sol index 4c488f5..1e6432c 100644 --- a/contracts/zxp/Games.sol +++ b/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); } } diff --git a/contracts/zxp/ObjectRegistry.sol b/contracts/zxp/ObjectRegistry.sol index 1129618..4cc876d 100644 --- a/contracts/zxp/ObjectRegistry.sol +++ b/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,7 +18,11 @@ 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]; @@ -23,6 +30,8 @@ contract ObjectRegistry is IObjectRegistry { } 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]; diff --git a/contracts/zxp/ObjectRegistryClient.sol b/contracts/zxp/ObjectRegistryClient.sol index 32910b7..e83f3d0 100644 --- a/contracts/zxp/ObjectRegistryClient.sol +++ b/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 diff --git a/contracts/zxp/game/GameVault.sol b/contracts/zxp/game/GameVault.sol index f0f22b3..06cb826 100644 --- a/contracts/zxp/game/GameVault.sol +++ b/contracts/zxp/game/GameVault.sol @@ -9,6 +9,7 @@ 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; @@ -16,28 +17,32 @@ contract GameVault is ERC721Wrapper, ObjectRegistryClient, IGameVault { 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); } } diff --git a/contracts/zxp/game/XP.sol b/contracts/zxp/game/XP.sol index 54815cf..7b7ea10 100644 --- a/contracts/zxp/game/XP.sol +++ b/contracts/zxp/game/XP.sol @@ -7,6 +7,8 @@ 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"; @@ -14,6 +16,7 @@ contract XP is ObjectRegistryClient, QuadraticLevelCurve, ERC20, IXP { 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); } diff --git a/contracts/zxp/game/seasons/Seasons.sol b/contracts/zxp/game/seasons/Seasons.sol index fa3c16d..28444e7 100644 --- a/contracts/zxp/game/seasons/Seasons.sol +++ b/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) diff --git a/contracts/zxp/game/seasons/mechanics/PlayerRewards.sol b/contracts/zxp/game/seasons/mechanics/PlayerRewards.sol index 00b46e6..9d65cfd 100644 --- a/contracts/zxp/game/seasons/mechanics/PlayerRewards.sol +++ b/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, diff --git a/contracts/zxp/game/seasons/mechanics/SecretRewards.sol b/contracts/zxp/game/seasons/mechanics/SecretRewards.sol index 77aef95..c132b00 100644 --- a/contracts/zxp/game/seasons/mechanics/SecretRewards.sol +++ b/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 diff --git a/contracts/zxp/game/seasons/mechanics/StakerRewards.sol b/contracts/zxp/game/seasons/mechanics/StakerRewards.sol index aebd316..ed81500 100644 --- a/contracts/zxp/game/seasons/mechanics/StakerRewards.sol +++ b/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; diff --git a/test/zXP.test.ts b/test/zXP.test.ts index f1e05da..20fc229 100644 --- a/test/zXP.test.ts +++ b/test/zXP.test.ts @@ -105,17 +105,20 @@ describe("ZXP", () => { s1 = staker1.address; s2 = staker2.address; }); + it("Registers GameVault", async () => { const gameVaultBytes = ethers.utils.formatBytes32String("GameVault"); let tx = await gameRegistry.connect(deployer).registerObjects([gameVaultBytes], [gameVault.address]); await tx.wait(); }); + it("Registers XP", async () => { const xpBytes = ethers.utils.formatBytes32String("XP"); await gameRegistry.connect(deployer).registerObjects([xpBytes], [xp.address]); //console.log(await gameRegistry.addressOf(xpBytes)); expect(await gameRegistry.addressOf(xpBytes)).to.equal(xp.address) }); + it("Registers Seasons", async () => { const seasonBytes = ethers.utils.formatBytes32String("Seasons"); await gameRegistry.connect(deployer).registerObjects([seasonBytes], [seasons.address]); @@ -123,26 +126,41 @@ describe("ZXP", () => { const numSeasons = 1 for (let i = 0; i < numSeasons; i++) { + // these are tests, but I don't see any specific checks here + // if you rely on reverts failing tests here, these reverts need to be checked + // to make sure it fails with the correct message, + // otherwise it's not a test it("Mints staker 1 NFT", async () => { s1nft = s1nft + 3; await mockErc721.connect(deployer).mint(s1, s1nft); }); + it("Staker 1 stakes NFT", async () => { + // this should in theory be resolved by the typechain type, so you could access it + // through dot notation + // just assign a proper type to `mockErc721` and you will be able to call methods with + // dot notation. there's no need to set all let vars to `Contract` type since it's not a correct type + // for these tests, it's a general type for all contracts + // use specific types here await mockErc721.connect(staker1)["safeTransferFrom(address,address,uint256)"](s1, gameVault.address, s1nft); }); + it("Mints Staker 2 NFT", async () => { s2nft = s2nft + 3; await mockErc721.connect(deployer).mint(s2, s2nft); }); + it("Player2 stakes NFT", async () => { await mockErc721.connect(staker2)["safeTransferFrom(address,address,uint256)"](s2, gameVault.address, s2nft); }); + it("Gets season registry", async () => { const storedSeason = await seasons.seasons(await seasons.currentSeason()); const storedRegistry = storedSeason.objects; const ObjectRegistryFactory = await hre.ethers.getContractFactory("ObjectRegistry"); seasonRegistry = await ObjectRegistryFactory.attach(storedRegistry); }); + it("Registers StakerRewards", async () => { const stakerRewardsFactory = await hre.ethers.getContractFactory("StakerRewards"); const stakerRewardsDeploy = await stakerRewardsFactory.deploy(mockErc20.address, "10", gameVault.address, gameVault.address, seasons.address); @@ -150,9 +168,12 @@ describe("ZXP", () => { stakerRewards = stakerRewardsDeploy; const sr = ethers.utils.formatBytes32String("StakerRewards"); + // why do we register this on 2 different contracts? + // why do we need 2 different registries here or in general? await gameRegistry.registerObjects([sr], [stakerRewards.address]); await seasonRegistry.registerObjects([sr], [stakerRewards.address]); }); + it("Registers PlayerRewards", async () => { const top3rewardsFactory = await hre.ethers.getContractFactory("PlayerRewards"); const top3deploy = await top3rewardsFactory.deploy(official.address, mockErc20.address, seasons.address, playerXPReward.toString()); @@ -160,8 +181,10 @@ describe("ZXP", () => { top3Rewards = top3deploy; const pr = ethers.utils.formatBytes32String("PlayerRewards"); + // but this one we register on one registry? why? await seasonRegistry.registerObjects([pr], [top3Rewards.address]); }); + it("Registers SecretRewards", async () => { const secretsFactory = await hre.ethers.getContractFactory("SecretRewards"); const secretsDeploy = await secretsFactory.deploy(seasons.address, "121"); @@ -190,6 +213,7 @@ describe("ZXP", () => { let tx = await secretRewards.connect(deployer).commitSecret(nonce, secretHash); await tx.wait(); }); + it("Commits correct guess", async () => { let guess = "verysecretwordshhh"; let nonce = 123123123; @@ -197,17 +221,20 @@ describe("ZXP", () => { let tx = await secretRewards.connect(player1).commitGuess(nonce, guessHash); await tx.wait(); }); + it("Reveals secret", async () => { let secret = "verysecretwordshhh"; let nonce = 123123123; let tx = await secretRewards.connect(deployer).revealSecret(nonce, secret); await tx.wait(); }); + it("Reveals correct guess, receives XP", async () => { let guess = "verysecretwordshhh"; let nonce = 123123123; let tx = await secretRewards.connect(player1).revealGuess(nonce, guess); await tx.wait(); + // can we not use hardcoded values anywhere? this makes maintenance difficult expect(await xp.balanceOf(p1)).to.equal(BigNumber.from("121")); previousP1XP = BigNumber.from("121"); let secret = "snow"; @@ -215,14 +242,20 @@ describe("ZXP", () => { let secretHash = await secretRewards.hashCommit(deployer.address, nonce, secret) await secretRewards.commitSecret(nonce, secretHash); }); + it("Funds player reward tokens", async () => { + // fix all these string based access calls await mockErc20.connect(deployer)["transfer(address,uint256)"](top3Rewards.address, "1000000000000000000000000"); await mockErc20.connect(deployer)["transfer(address,uint256)"](stakerRewards.address, "1000000000000000000000000"); + // how do we know here if anything is funded or not? where are the balance checks? + // none of these tests have any valid checks that make me believe this thing works }); + it("Funds staker reward tokens", async () => { await mockErc20.connect(deployer)["transfer(address,uint256)"](top3Rewards.address, "1000000000000000000000000"); await mockErc20.connect(deployer)["transfer(address,uint256)"](stakerRewards.address, "1000000000000000000000000"); }); + it("Starts the season", async () => { await seasons.startSeason(); }); @@ -230,6 +263,7 @@ describe("ZXP", () => { const numRounds = 10; for (let i = 0; i < numRounds; i++) { const str = "Submits round " + i.toString() + " results"; + it(str, async () => { firstReward = "1000000000000000000000"; secondReward = "100000000000000000000"; @@ -253,6 +287,7 @@ describe("ZXP", () => { previousP2Bal = newP2Bal; previousP3Bal = newP3Bal; }); + it("Awarded xp to winners", async () => { let reward1 = BigNumber.from(playerXPReward * 3); let reward2 = BigNumber.from(playerXPReward * 2); @@ -269,6 +304,7 @@ describe("ZXP", () => { previousP2XP = newP2XP; previousP3XP = newP3XP; }); + it("Levels up", async () => { let p1XP = await xp.balanceOf(p1); let playerLevel = await xp.levelAt(p1XP); @@ -278,9 +314,11 @@ describe("ZXP", () => { console.log(xpReq.toString()); }); } + it("Staker 1 unstakes and claims rewards", async () => { await gameVault.connect(staker1).withdrawTo(s1, [s1nft]); }); + it("Ends the season", async () => { await seasons.endSeason(); });