Gas Optimizations - Githubissues

constants expressions are expressions, not constants

While it seems this should help, changing to immutable shows a regression in our test suite. Gas costs go up by ~50 gas.

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.

duplicated require() check should be refactored

Agree, will consider changing here.

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

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.

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.

Store using Struct over multiple mappings

Potentially valid but these variables support different use cases, since they are not read together it may not be beneficial.

Not using the named return variables when a function returns, wastes deployment gas

Agree for code consistency with other parts of our code. Saves 0.013 bytes on the bytecode size.

calldata

Valid & will fix. This saves ~50 gas on createNFTCollection and ~60 gas on createNFTDropCollectionWithPaymentFactory

Using private rather than public for constants to saves gas.

Agree but won't fix. For ease of use and consistency we will continue to expose some constants publicly.

Don't initialize variables with default values.

Invalid. This optimization technique is no longer applicable with the current version of Solidity.

++i costs less than i++

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.

Variables should be cached when used several times

Agree, will fix.

= cheaper than >

Invalid. This rec changes logic and violates requirements.

code-423n4 / 2022-08-foundation-findings

Gas Optimizations #261