HardlyDifficult
commented
2 years ago FOR-LOOP LENGTH COULD BE CACHED
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.
OR-LOOP unchecked{++i}
Invalid - the example provided is inside an unchecked block already.
Don't initialize variables with default values.
Invalid. This optimization technique is no longer applicable with the current version of Solidity.
EXPRESSIONS FOR CONSTANT VALUES SUCH AS A CALL TO keccak256()
While it seems this should help, changing to immutable shows a regression in our test suite. Gas costs go up by ~50 gas.
Variable re-arrangement by storage packing
Invalid - packing does not apply to constants.
REDUCE THE SIZE OF 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.
DUPLICATED REQUIRE() CHECKS COULD BE REFACTORED TO A MODIFIER OR FUNCTION
Fair feedback - however the first two are separate classes due to different dependencies. We will be renaming one of these to make that more clear. The other is a copy paste from OZ with minimal modifications, we don't want to diverge from the source more than we need to. For the isContract references, many of those messages vary a bit which can be helpful for debugging.
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.
Unused modifier
Good catch. However they are used in other contracts which were out of scope for this contest.
USE CALLDATA INSTEAD OF MEMORY
Invalid for the return value.
Valid for the create call, saves ~60 gas on createNFTDropCollectionWithPaymentFactory
FOR-LOOP LENGTH COULD BE CACHED
The for loop length can be cached to memory before the loop, even for memory variables. The demo of the loop gas comparison can be seen here.
Suggestion: Cache the length before the loop.
FOR-LOOP
unchecked{++i}COSTS LESS GAS COMPARED TOi++ORi += 1Use
++iinstead ofi++to increment the value of an uint variable, and for guaranteed non-overflow/underflow, it can be unchecked. And the optimizer need to be turned on.The demo of the loop gas comparison can be seen here.
NO NEED TO EXPLICITLY INITIALIZE VARIABLES WITH DEFAULT VALUES
If a variable is not set/initialized, it is assumed to have the default value (0 for uint, false for bool, address(0) for address…). Explicitly initializing it with its default value is an anti-pattern and wastes gas.
The demo of the loop gas comparison can be seen here.
X = X + Y / X = X - YIS CHEAPER THANX += Y / X -= YThe demo of the gas comparison can be seen here.
Consider use
X = X + Y / X = X - Yto save gasEXPRESSIONS FOR CONSTANT VALUES SUCH AS A CALL TO
keccak256(),` SHOULD USE IMMUTABLE RATHER THAN CONSTANTreference: https://github.com/ethereum/solidity/issues/9232
Constant expressions are left as expressions, not constants.
a constant declared as:
It is expected that the value should be converted into a constant value at compile time. But the expression is re-calculated each time the constant is referenced.
consequences:
Suggestion: Change these expressions from constant to immutable and implement the calculation in the constructor or hardcode these values in the constants and add a comment to say how the value was calculated.
Variable re-arrangement by storage packing
In
DEFAULT_ADMIN_ROLEandROYALTY_IN_BASIS_POINTScan be placed next to each other, as a result, 1 slot storage can be saved. According to the currently layout, they both occupy 1 slot, but after re-arrangement, they can be packed into 1 slot.Reference: Layout of State Variables in Storage.
REDUCE THE SIZE OF ERROR MESSAGES
Shortening revert strings to fit in 32 bytes will decrease deployment time gas and will decrease runtime gas when the revert condition is met. Revert strings that are longer than 32 bytes require at least one additional mstore, along with additional overhead for computing memory offset, etc.
Many of these
require()statements are in a modifier which might be frequently called, optimizing the gas can be beneficial.Suggestion: Shortening the revert strings to fit in 32 bytes, or using custom errors as described next.
USE CUSTOM ERRORS INSTEAD OF REVERT STRINGS
Custom errors from Solidity 0.8.4 are more gas efficient than revert strings (lower deployment cost and runtime cost when the revert condition is met) reference
The demo of the gas comparison can be seen here.
Many of these
require()statements are in a modifier which might be frequently called, optimizing the gas can be beneficial.Just like in contracts/mixins/nftDropMarket/NFTDropMarketFixedPriceSale.sol, the following files have multiple
require()statements can use custom errors:DUPLICATED
REQUIRE()CHECKS COULD BE REFACTORED TO A MODIFIER OR FUNCTIONThe following
require()checks are the same, they can be made into a common modifier, and providing an error message as parameter.The followings all check
hasRole():The followings all check for
isContract():FUNCTIONS GUARANTEED TO REVERT WHEN CALLED BY NORMAL USERS CAN BE MARKED PAYABLE
If a function modifier such as
onlyOwner()is used, the function will revert if a normal user tries to pay the function. Marking the function as payable will lower the gas cost for legitimate callers because the compiler will not include checks for whether a payment was provided.The extra opcodes avoided are
which costs an average of about 21 gas per call to the function, in addition to the extra deployment cost.
Unused modifier
onlyFoundationAdmin()andonlyFoundationOperator()are never used across the contacts and can consider remove them.USE CALLDATA INSTEAD OF MEMORY
When arguments are read-only on external functions, the data location can be calldata.
The demo of the gas comparison can be seen here.