Full review #13
Full review #13
Conversation
This should be reverted when I solve the upgradeability of ChainlinkClient.sol
Needed until there is no official upgradeable version of ChainlinkClient
This reverts commit 3ac23bc.
To be added in follow-up commits: - oracle fee compensation - proper tests - updated docs - updated deploy addresses
If the access is false, we still keep the fee - everything should be validated on frontend, so this shouldn't happen unintentionally. Minimizes the risk of someone draining the contract's link balance - it will cost more for them to make unsuccessful requests.
Accidentally moved in 476fa31
And start from one
Plus some refactor
* Add metadata-storage doc * Adjust wording
* Generate metadata in the contract, update signing * Add ranking * Revert changes to the deploy script * Document the process we are doing * Abitility to change name and symbol on upgrade With a test * Rename current metadata update function * Fix tests * Handle/upgrade existing metadata * Deploy new version to mumbai * Rank should be a string To appear better on Opensea * Update test * Rename to Guild Pin * Remove deployment on Sepolia It wasn't the same as v1 on other chains anyways * Update on Mumbai * Backfill all metadata * Upgrade on Mumbai * Improve documentation * Improve post stealth launch doc * Add migration plan * Update formatting * Initialize totalMintedPerGuild * Test deploy * Add test for pretty strings * Remove log
* Add chainId and contract address to the signature * Improve docs * Deploy to Mumbai * Upgrade OpenZeppelin contracts npm audit fix * Update docs * Deploy to Arbitrum * Deploy to BSC * Upgrade on Polygon
* Disallow claiming tokens twice for the same userId * Update docs * Really test what we intended
And rename mainnet to ethereum
* Implement EIP-5192 * Add tests * Locked function should throw for non-existent tokens * Update docs
There was a problem hiding this comment.
Looking really great! A lot of the same code as guildxyz/nft-reward-factory#5, so still looking good there. Just have this one comment on making error catching more explicit/immediately visible in the code but otherwise looks great!
| } | ||
|
|
||
| function burn(uint256 userId, GuildAction guildAction, uint256 guildId) external { | ||
| uint256 tokenId = claimedTokens[msg.sender][guildAction][guildId]; |
There was a problem hiding this comment.
Might want to consider having an explicit check or error here to make it clear what went wrong if someone tries to erroneously burn a token.
There was a problem hiding this comment.
Wrote a test to see better what happens here: 79d5059
I could revert with a custom error here (I already have a NonExistentToken error in the codebase, or come up with a new one), however, I don't think that would really be an improvement over the current situation (and not worth the ~8k gas extra).
You might say that since the revert comes from the underlying dependency, it might change just from a dependency update, but I will notice that now thanks to the test. Thanks for bringing this to my attention!
|
Closing the review session, thanks Sean! |
The whole codebase on main up until e7b5914 + #12. This should be the final form of the contract for v1.3.0.
Note: this branch should not be merged. Any change should go into a separate PR.