HardlyDifficult
commented
1 year ago 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.
The other 3 examples provided are already unchecked -- Invalid.
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.
G-03 Using storage instead of memory for structs/arrays saves gas
We've seen this technique work before, but for the fixed price sale it actually causes gas costs to go up. Maybe because we are using all fields from the struct.
Invalid for the BytesLibrary - those cannot be storage variables.
G-04 Comparison operators
Invalid - changing these to > instead of >= changes the logic and would violate requirements.
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.
calldata
Valid & will fix. This saves ~50 gas on createNFTCollection and ~60 gas on createNFTDropCollectionWithPaymentFactory
Updated startsWith, NFTCollection, and the AddressLibrary & call
Invalid for capLength due to how that data is sourced. Same for replaceAtIf.
G-07 internal functions only called
Invalid - these cannot be inlined due to the inheritance pattern we have used.
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 gasG-09 Empty blocks should be removed or emit something
Invalid, those examples are required in order to compile.
G-10
Not seeing any benefit for assumePrimarySale, potentially because it's an immutable variable.
G-11 Using storage instead of memory for structs/arrays saves gas
Invalid - storage cannot be used for the examples provided.
+= COSTS MORE GAS THAN = + FOR STATE VARIABLES
No impact. We made the recommended change and ran gas-reporter for our entire test suite. 0 impact detected. Even if there were a small benefit, we feel this hurts readability and would not be worth trivial savings.
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.
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.
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.
Functions guaranteed to revert when called by normal users can be marked payable
Disagree. This could save gas but 1) this seems like a poor practice to encourage and could be error prone. and 2) we don't care about optimizing admin-only actions.
Don't initialize variables with default values.
Invalid. This optimization technique is no longer applicable with the current version of Solidity.
G-18 Tight variable packing
Invalid - packing does not apply to function inputs.
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.
calldata
Valid & will fix. This saves ~60 gas on createNFTDropCollectionWithPaymentFactory
See the markdown file with the details of this report here.