remove constant declaration from ERC821Psi base classes

[nidhhoggr]

When investigating how to come up with a way to reduce log4 calls to only once per _mint call, I came up with the following code. The only problem here is that is winded up using ~16 more gas than before. Basically it emulates a do-while loop which is not supported un Yul yet.

        uint256 toMasked;
        uint256 end = nextTokenId + quantity;

        // Use assembly to loop and emit the `Transfer` event for gas savings.
        // The assembly, together with the surrounding Solidity code, have been
        // delicately arranged to nudge the compiler into producing optimized opcodes.
        assembly {
            // Mask `to` to the lower 160 bits, in case the upper bits somehow aren't clean.
            toMasked := and(to, sub(shl(0xA0, 1), 1))

            let tokenId := nextTokenId
            // The duplicated do/while removes an extra check and reduces stack juggling
            for {} 1 {} {
                // Emit the `Transfer` event. Similar to above.
                log4(0, 0, 0xddf252ad1be2c89b69c2b068fc378daa952ba7f163c4a11628f55a4df523b3ef, 0, toMasked, tokenId)
                tokenId := add(tokenId, 1)
                // The `eq(,))` check ensures that large values of `quantity`
                // that overflows uint256 will make the loop run out of gas.
                if eq(tokenId, end) { break }
            }
        }

I went back and checked ERC721As implementation and sure enough, they have also remedied this using a do while loop with native solidity. Implementing it this way result in aroun ~100 gas savings while providing more readability and still eliminating the constant declarations. https://github.com/chiru-labs/ERC721A/blob/main/contracts/ERC721A.sol#L773

        unchecked {
            // Mask `to` to the lower 160 bits, in case the upper bits somehow aren't clean.
            uint256 toMasked = uint256(uint160(to)) & (1 << 160) - 1;

            uint256 end = nextTokenId  + quantity;
            uint256 tokenId = nextTokenId ;

            do {
                assembly {
                    // Emit the `Transfer` event.
                    log4(
                        0, // Start of data (0, since no data).
                        0, // End of data (0, since no data).
                        0xddf252ad1be2c89b69c2b068fc378daa952ba7f163c4a11628f55a4df523b3ef, // Signature.
                        0, // `address(0)`.
                        toMasked, // `to`.
                        tokenId // `tokenId`.
                    )
                }
                // The `!=` check ensures that large values of `quantity`
                // that overflows uint256 will make the loop run out of gas.
            } while (++tokenId != end);
        }

At the end of the day constants are useful for code readability especially when the variable is used in multiple places. In our scenario, these variables are only used once in the code. Further, usage of constants in upgradable contracts introduce storage collision issues (as would any declared state variables) so the tradeoff of simply removing them are more advantageous. In scenarios where the constant would occur in multiple areas of the code, a library could be used with helper methods that contain the constants. e.g. a helper method for masking 160 bits. The commit below address the constants declared in our base contracts but there are still some declared in the Random extensions. It's up to you if you'd like to remedy those.

[nidhhoggr]

Gas Benchmarking https://github.com/nidhhoggr/ERC721PsiReports/compare/master...feature/baseContractConstantRemoval

[estarriolvetch]

@nidhhoggr Not sure if this is true, but I think this can overflow in your implementation.

uint256 end = nextTokenId  + quantity;

[nidhhoggr]

As mentioned previously, this code was grabbed from the latest commit to ERC721A where the same code is used. I've taken a look at the code and I don't think overflow is possible, here is why.

In the same function the following line is executed before the unchecked block.

_currentIndex += quantity;

At this point _currentIndex and nextTokenId are the same value. If quantity exceeded 2**256 the transaction would automatically revert before the unchecked block is reached. Looking more closely at the code in this commit, we could also get rid of end and just use _currentIndex because they are the same value.

https://github.com/nidhhoggr/ERC721Psi/blob/e0e3ddf43cde2bbec4e30a93caedbdfb7fe66e38/contracts/ERC721Psi.sol#L372