Foundation Drop contest - ReyAdmirado's results

Use fixed pragma

Disagree. We intentionally use a floating pragma in order to make integrating with contracts easier. Other contract developers are looking to interact with our contracts and they may be on a different version than we use. The pragma selected for our contracts is the minimum required in order to correctly compile and function. This way integration is easier if they lag a few versions behind, or if they use the latest but we don't bump our packages frequently enough, and when we do upgrade versions unless there was a breaking solidity change -- it should just swap in by incrementing our npm package version.

Use safeMint

Agree will fix - for context see our response here.

BuyReferralPaid event should index buyReferrer

Agree but won't fix at this time. We have already started emitting this event on mainnet, to simplify integrations such as subgraph we are going to continue with this event as is. However it's a good catch and I agree that buyReferrer ideally would have been indexed here.

For CreateFixedPriceSale I believe this is invalid. index should be reserved for params that are likely to be requested as a filter. In these examples those params are data not really filter candidates. And for the string specifically, indexed would prevent the full information from being communicated, requiring a second unindexed version which is a bit redundant and increases gas costs.

Good report, very easy to follow.

1. not using the named return variables when a function returns, wastes deployment gas

Agree for code consistency with other parts of our code. And this is a thorough list. Saves 0.013 bytes on the bytecode size in total.

2. can make the variable outside the loop to save gas

This technique is new to me.. but it makes sense that it could make a difference.

I tested it with royalties where 5 recipients were paid: Before: // 221331 · 230960 · 225544 After: // 221316 · 230945 · 225529 Savings: 15 gas

And when royalties are just 1 recipient (i.e. loop is not entered): Before: // 129153 · 136830 · 132512 After: // 129157 · 136834 · 132516 Cost: 4 gas

Since the vast majority of sales use just 1 recipient, and that it does complicate the code a tiny bit - I won't be making the change here.

However for your other example, that loop is always run for 20 iterations -- seems like an easy win. Will be making that change.

3. it costs more gas to initialize non-constant/non-immutable variables to zero than to let the default of zero be applied

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

++i/i++ should be unchecked{++i}

Forgot example links for this one - but I know what you mean from the other submissions :) 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.

5. require()/revert() strings longer than 32 bytes

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.

6. use custom errors rather than revert()/require() strings to save deployment gas

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.

7. using calldata instead of memory for read-only arguments in external functions saves gas

Updated startsWith, NFTCollection, and the AddressLibrary & call Invalid for capLength due to how that data is sourced. Same for replaceAtIf.

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

Not changing the return values to keep the ABI clean, particularly supportsInterface for example since that would be a violation of the standard.

9. usage of uint/int 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.

10. expressions for constant values such as a call to keccak256(), should use immutable rather than constant

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

11. using private rather than public for constants, saves gas

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

12. <array>.length should not be looked up in every loop of a for-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.