Gas Optimizations

[code423n4]

See the markdown file with the details of this report here.

[HardlyDifficult]

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 gas

Using 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.