Add gas optimizations #59
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
added 6 commits
January 31, 2022 21:28
[source:](https://medium.com/coinmonks/gas-optimization-in-solidity-part-i-variables-9d5775e43dde) > Initialization > Every variable assignment in Solidity costs gas. When initializing variables, we often waste gas by assigning default values that will never be used. > uint256 value; is cheaper than uint256 value = 0;.
chiru-labs
requested changes
Feb 1, 2022
| } | ||
|
|
||
| for (uint256 i = oldNextOwnerToSet; i <= endIndex; i++) { |
There was a problem hiding this comment.
Seems like we don't need oldNextOwnerToSet anymore and can just use _nextOwnerToExplicitlySet here
| require(_nextOwnerToExplicitlySet < currentIndex, 'all ownerships have been set'); | ||
|
|
||
| // Index underflow is impossible. | ||
| // Counter or index overflow is incredibly unrealistic. |
There was a problem hiding this comment.
Please elaborate a bit more on this line (e.g.- "incredibly unrealistic, collection size would have to be ___")
contracts/ERC721A.sol
Outdated
| uint256 tokenIdsIdx; | ||
| address currOwnershipAddr; | ||
|
|
||
| // Counter overflow is incredibly unrealistic. |
There was a problem hiding this comment.
Please elaborate a bit more on this line (e.g.- "incredibly unrealistic, collection size would have to be ___")
|
Thanks for that! I missed the last one. Also added an unchecked block to |
added 3 commits
February 1, 2022 04:43
chiru-labs
approved these changes
Feb 1, 2022
cygaar
approved these changes
Feb 2, 2022
|
Love this |
Merged
Vectorized
added a commit
to Vectorized/ERC721A
that referenced
this pull request
Feb 9, 2022
cygaar
pushed a commit
that referenced
this pull request
Feb 12, 2022
* Added burn functionality. * Changed _initOneIndexed * Moved burn function into ERC721ABurnable * Moved burn function into ERC721ABurnable * Remove redundant burn check in ownershipOf * Optimized ownershipOf * Removed aux from AddressData for future PR * Packed currentIndex and totalBurned for gas savings * Added gas optimizations * Added gas optimizations * Added requested changes * Edited comments. * Renamed totalBurned to burnedCounter * Renamed to burnCounter * Updated comments. * Mark transferFrom/safeTransferFrom virtual * Mark transferFrom/safeTransferFrom virtual * Updated comments. * Tidy up tests * Inlined _exists for _burn and _transfer. * Merged custom errors * Merged custom errors * Fixed missing change from #59 * Gas optimization * update specs for _beforeTokenTransfers and _afterTokenTransfers hooks * Added #84 * Added #87 * Added #85 * Added #89 * Added comments on packing _currentIndex and _burnCounter * Removed overflow check for updatedIndex * Added requested test changes * Removed unused variable in burn test * Removed unused variable in burn test Co-authored-by: Amirhossein Banavi <ahbanavi@gmail.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Wrote up a few optimizations on the main contracts, open to feedback especially for the unchecked blocks if you think that over/underflow checks are needed in those functions.
1b32797: _exists(uint256 id) returns id < currentIndex, and we call it with currentIndex, which always results in it returning false.
ae1cdcf: Explanations in the code comments.
5fca737: Initializing a variable with a 0 is more expensive than not doing so, even if both results are the same. Source:
c23e2e0: All values are integers which cannot be negative, saves a few opcodes.
ee2abe2: Previous changes applied to
ERC721AOwnersExplicit.81daf25: Loading
nextOwnerToExplicitlySetinto memory saves an SLOAD opcode later on line 23.