Full review #5
Full review #5
Conversation
Most chains - including Polygon - have not applied the Shanghai hardfork yet, thus they don't support PUSH0 (which is heavily used by Solidity ^0.8.20)
* Add userId check to claim and burn * Update tests * Update docs * Deploy to Mumbai
* Add factory * Remove redundant initializer * Make validSigner not payable, add initialize to interface * Split variable storage locations * Separate factory and implementation * Remove test from irrelevant location * Take care of tests * Set owner on clones * Update deploy scripts * Fix/cleanup tests * Make validSigner internal on the NFT * Add note to deploy script * Update factory interface * Update Mumbai RPC * Update docs * Store multiple token contracts per guild * Update docs * Update upgrade script * Update deployment on Mumbai * Fix GuildRewardNFT tests * Make nft implementation non-upgradeable * Remove intermediary deployments * Prepare factory for deploying multiple types of contracts * Add tests for setNFTImplementation * Update docs
Also delete previous incompatible deployments, since they are irrelevant now
And track deployments by sender address
And rename mainnet to ethereum
There was a problem hiding this comment.
This all looks really clean! Great thinking with how the signer setup works - although I think that is probably the biggest attack vector you have: logic is super clean in the smart contract code, just have to make sure noone gets access to the env vars on the site that are being used to create the signatures.
| /// @param token The address of the token to send. | ||
| /// @param from The source of the tokens. | ||
| /// @param amount The amount of the token to send in base units. | ||
| function sendTokenFrom(address to, address from, address token, uint256 amount) internal { |
There was a problem hiding this comment.
This function isn't used at all, is it here for future-proofing?
There was a problem hiding this comment.
You are right, I use this lib file in more projects and I tend to keep it as it is. Probably it would be even better to just import it from somewhere but I feel like creating a package just for this is overkill :D
It's not included in the compiled code so I'm leaving it here for now
| /// @param to The recipient of the tokens. | ||
| /// @param token The address of the token to send. | ||
| /// @param amount The amount of the token to send in base units. | ||
| function sendToken(address to, address token, uint256 amount) internal { |
There was a problem hiding this comment.
This function isn't used at all, is it here for future-proofing?
Creating signatures is handled by our backend - I'm convinced we store the key securely, but you are absolutely right, someone might be even more incentivized to try to hack us because of this. |
|
Closing this as the review session is over, thanks Sean! |
|
100%! |
The whole codebase up until ef8bbee.
This branch should not be merged. Any change should go into a separate PR.