HardlyDifficult
commented
1 year ago This was a very detailed report - thanks.
The result of a function call should be cached rather than re-calling the function
NFTCollectionFactory.sol.adminUpdateNFTCollectionImplementation(): versionNFTCollection should be cached(Saves ~261 gas)
NFTCollectionFactory.sol.adminUpdateNFTDropCollectionImplementation(): versionNFTDropCollection should be cached (Saves ~252 gas)
Agree, but optimizing for admin-only functions is not a goal. Keeping as is for simplicity.
Emitting storage values instead of the memory one.
Agree, fixed!
NFTDropCollection.sol.mintCountTo(): latestTokenId should be cached (Saves ~279 gas this might be higher as the storage value is also read inside a for loop repeatedly)
Agree, fixed.
NFTCollection.sol.baseURI(): baseURI should be cached (Saves 1 SLOAD - 94 gas)
Agree, fixed.
SequentialMintCollection.sol._updateMaxTokenId(): maxTokenId should be cached (Saves 1 SLOAD: ~94 gas)
Agree, fixed.
NFTDropMarketFixedPriceSale.sol.createFixedPriceSale(): IAccessControl(nftContract) should be cached
MarketFees.sol.internalGetMutableRoyalties(): IGetFees(nftContract) should be cached
Invalid - casting does not cause an SLOAD
NFTCollection.sol._mint(): cidToMinted[tokenCID] should be declared as cidToMinted storage _cidToMinted = cidToMinted[tokenCID] (Saves ~32gas)
Invalid - the data here is a bool which cannot be storaged as a storage variable.
NFTCollection.sol._burn(): _tokenCIDs[tokenId] should be declared as _tokenCIDs storage tokencid = _tokenCIDs[tokenId]
Potentially valid, but keeping the code as is for readability (so we can keep the delete keyword here).
inline
Won't fix. This function is shared with another out-of-scope contract.
calldata
Valid & will fix. This saves ~50 gas on createNFTCollection.
Using unchecked blocks to save gas
Agree and will fix.
unchecked loop in
getFeesAndRecipients
getFeesAndRecipients is a read only function not intended to be used on-chain, but as a best practice we will add unchecked there as well.
Cache Array Length Outside of Loop
May be theoretically valid, but won't fix. I tested this: gas-reporter and our gas-stories suite is reporting a small regression using this technique. It also hurts readability a bit so we wouldn't want to include it unless it was a clear win.
++i costs less than i++
Agree and will fix.
Use != 0 instead of > 0
Invalid. We tested the recommendation and got the following results:
createNFTDropCollection gas reporter results:
using > 0 (current):
- 319246 · 319578 · 319361
using != 0 (recommendation):
- 319252 · 319584 · 319367
impact: +6 gasUsing bools for storage incurs overhead.
Valid for cidToMinted, saving ~200 gas. Not seeing any benefit for assumePrimarySale, potentially because it's an immutable variable.
A modifier used only once and not being inherited should be inlined to save gas
Won't fix - I still like the modifier for readability.
uints/ints smaller than 32 bytes (256 bits) incurs overhead.
Potentially valid but won't fix. This is uint32 to allow packing storage for a significant gas savings. Accepting the input as uint256 would incur additional overhead in checking for overflow before saving these values. Other inputs are logically limited to the size exposed, huge values cannot be used. Accepting the input as uint256 would incur additional overhead in checking for overflow before using these values.
Use short error messages
Agree but won't fix. We use up to 64 bytes, aiming to respect the incremental cost but 32 bytes is a bit too short to provide descriptive error messages for our users.
Custom errors
Agree but won't fix at this time. We use these in the market but not in collections. Unfortunately custom errors are still not as good of an experience for users (e.g. on etherscan). We used them in the market originally because we were nearing the max contract size limit and this was a good way to reduce the bytecode. We'll consider this in the future as tooling continues to improve.
See the markdown file with the details of this report here.