Kirill Review and some small fixes #1
Conversation
| 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? |
There was a problem hiding this comment.
Yes, compatibility for contracts so they can run this and set someone as the owner
|
|
||
|
|
||
| // why does this need to be ObjectRegistry? where is this functionality used? |
There was a problem hiding this comment.
Inheritance of the ObjectRegistry and its use below are all remnants of a rework.
Fixed, and it will give me an opportunity to discuss why this change was made.
| contract Games is IGames, ObjectRegistry { | ||
| struct Game { | ||
| string metadata; | ||
| // this should be named better. it's confusing what this is |
There was a problem hiding this comment.
Change to registeredObjects?
| address owner, | ||
| string calldata metadata, | ||
| // who deploys these contracts and who pays for each deployment of | ||
| // these sets of objects per EVERY game? |
There was a problem hiding this comment.
Not us. Users, who want to launch game utility for an NFT collection. These are the "game creators"
| 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? |
There was a problem hiding this comment.
Inherited ObjectRegistry is removed.
It was changed to one registry per game because this allows the data association with the mapping -> struct to work generically with the ObjectRegistry, which wasn't it's own contract previously. Instead, code was repeated with very slight variation on the Games and the Seasons, in order to achieve the spec. However this is bad practice - this change consolidates the repeated code and makes the ObjectRegistry reusable.
| @@ -33,6 +35,7 @@ contract SecretRewards is ObjectRegistryClient, ISecretRewards { | |||
| string reveal; | |||
| } | |||
|
|
|||
| // what is the reason there's no nonce counter on the contract? | |||
There was a problem hiding this comment.
No, could store it, just no good reason to on-contract since it is not getting used anywhere.
This does mean you have to store these offchain, but even if you had them on-chain you'd still have some work to do there managing these.
And I don't think we'd want to spend gas on making them like enumerable or something.
| ); | ||
| 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? |
There was a problem hiding this comment.
Simple compare does not work due to differing data types storage vs mem
| // 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 |
There was a problem hiding this comment.
Aw but then, only one person would be able to guess and then everybody else would know that it's the right answer
without having to do the quest.
Actually there is an issue with the contract related to this though. that is WIP. either salt is needed here, or the nonce needs to be abstracted in the guess. in order to prevent copying.
| @@ -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? | |||
| @@ -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 ? | |||
There was a problem hiding this comment.
because this is a Season object, it doesn't actually know about the game vault. it is not on the same registry as it.
No description provided.