Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Assembly mint loop #347

Merged
merged 5 commits into from
Jul 15, 2022
Merged

Assembly mint loop #347

merged 5 commits into from
Jul 15, 2022

Conversation

Vectorized
Copy link
Collaborator

@Vectorized Vectorized commented Jun 20, 2022

Just letting it sit here.

May or may not decide to merge.

OpenSea Rinkeby is able to detect the events as per normal.

Saves 36 gas for the first mint and 28 gas per additional iteration
(28 / 1958 ≈ 1.4% savings per additional iteration).

The variables and statements have been carefully arranged to ensure that the compiler is as well behaved as possible. (actually, it can perform better than 100% handwritten Yul cuz Solidity provides high level semantics that can aid optimizations).

Before:

|  ERC721AGasReporterMock  ·  mintOne          ·      56201  ·      90401  ·      59621  ·
···························|···················|·············|·············|·············|
|  ERC721AGasReporterMock  ·  mintTen          ·      73828  ·     108028  ·      97768  ·

After:

|  ERC721AGasReporterMock  ·  mintOne          ·      56165  ·      90365  ·      59585  ·
···························|···················|·············|·············|·············|
|  ERC721AGasReporterMock  ·  mintTen          ·      73540  ·     107740  ·      97480  ·

Some trivia:

  • iszero(eq(tokenId, end)) is more efficient than lt(tokenId, end)
    because of how the compiler implicitly adds a iszero for the lt, and removes the iszero for the eq.

  • Moving the zero address check after the loop helps avoid an implicit mask.

  • Rewriting the _packedAddressData[to] = ... in assembly actually costs more gas.

  • Moving uint256 toMasked; after uint256 end = startTokenId + quantity; actually costs more gas.

@Vectorized Vectorized requested a review from cygaar June 22, 2022 10:55
@Vectorized Vectorized changed the title [Experiment] Assembly mint loop Assembly mint loop Jun 28, 2022
@@ -72,6 +72,11 @@ contract ERC721A is IERC721A {
// is required to cause an overflow, which is unrealistic.
uint256 private constant MAX_MINT_ERC2309_QUANTITY_LIMIT = 5000;

// The `Transfer` event signature is given by:
// `keccak256(bytes("Transfer(address,address,uint256)"))`.
uint256 private constant TRANSFER_EVENT_SIGNATURE =
Copy link

@pcaversaccio pcaversaccio Jul 12, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as already discussed, internal/private constants (and immutables) should have a leading _ IMHO, e.g. uint256 private constant _TRANSFER_EVENT_SIGNATURE. OZ does this; for example: https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/utils/Strings.sol#L11

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also, to be a little bit picky here, keccak256 returns bytes32 and therefore you could change the type to bytes32 instead of uint256.

0, // Start of data (0, since no data).
0, // End of data (0, since no data).
TRANSFER_EVENT_SIGNATURE, // Signature.
0, // `address(0)`.
Copy link

@pcaversaccio pcaversaccio Jul 12, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe my OCD but LOG4 topics are defined in 32-byte values, i.e. the address(0) should be in 32-bytes IMHO for consistency..., maybe another solution could be the following:

bytes32 private constant _ZERO_ADDRESS = 0x0000000000000000000000000000000000000000000000000000000000000000;
log4(0, 0, _TRANSFER_EVENT_SIGNATURE, _ZERO_ADDRESS, toMasked, startTokenId)

Btw, the variable _ZERO_ADDRESS could also be used for example for _beforeTokenTransfers and _afterTokenTransfers since address(0) is involved.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think just 0 is ok here though. For brevity.

Cuz everything in Yul is a 32-byte word.

tokenId := add(tokenId, 1)
} {
// Emit the `Transfer` event. Similar to above.
log4(0, 0, TRANSFER_EVENT_SIGNATURE, 0, toMasked, tokenId)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see comment above regarding 32-bytes

log4(0, 0, TRANSFER_EVENT_SIGNATURE, 0, toMasked, tokenId)
}
}
if (toMasked == 0) revert MintToZeroAddress();

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

question here: does we really need this check? Some background: One of the reasons that the zero checks were put there in the first place was that initially, Solidity didn't verify the length of msg.data, and thus it was possible for a function to be called with missing arguments. If an address argument was missing it would be read as the zero address, and these checks served the purpose of protecting from that kind of accident. Nowadays Solidity inserts checks that msg.data is big enough, so this kind of error is not really possible anymore. Second, what if the address is one of e.g. the pre-compiles (0x00...1 etc). Why is this accepted? Just wanted to raise this issue quickly.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is due to the EIP-721 spec.
https://eips.ethereum.org/EIPS/eip-721

NFTs assigned to the zero address are considered invalid

So technically, according to the spec, an NFT can be owned by 0x000...1 and be considered valid.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do understand why the authors have chosen this but still my argument holds true. Since it's a general library, I assume it's best to exactly follow the specs, but yeah, am not really happy about the zero address checks since they indicate some artificial security which does actually not exist. So if you keep this check, we could move it into assembly, no?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moving it into assembly will mean assembloored reverts, which breaks aesthetics here (unlike Solmate's STL where everything is in assembly).

In this case, the gas numbers are the same for both assembloored reverts and plain ol' Solidity reverts.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand - my thoughts were not related to the gas but more to the consistency since the check belongs to the minting itself and if we aim for assembly-based minting it should be in the same Yul block.

@Vectorized Vectorized merged commit 5395acc into chiru-labs:main Jul 15, 2022
@Vectorized Vectorized deleted the mintLoop branch July 18, 2022 00:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants