-
Notifications
You must be signed in to change notification settings - Fork 842
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
Move _setOwnersExplicit to separate extension #34
Conversation
@@ -46,7 +46,7 @@ contract ERC721A is Context, ERC165, IERC721, IERC721Metadata, IERC721Enumerable | |||
|
|||
// Mapping from token ID to ownership details | |||
// An empty struct value does not necessarily mean the token is unowned. See ownershipOf implementation for details. | |||
mapping(uint256 => TokenOwnership) private _ownerships; | |||
mapping(uint256 => TokenOwnership) internal _ownerships; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Had to convert this to internal in order for ERC721AOwnersExplicit to read the variable. We can also redeclare ownerships in that file, I have no strong preference here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same with currentIndex above
contracts/ERC721A.sol
Outdated
endIndex = currentIndex - 1; | ||
} | ||
// We know if the last one in the group exists, all in the group exist, due to serial ordering. | ||
require(_exists(endIndex), "not enough minted yet for this cleanup"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line will never evaluate to true due to line 408 so I removed this check in the refactored function
function _setOwnersExplicit(uint256 quantity) internal { | ||
require(quantity > 0, "quantity must be nonzero"); | ||
require(currentIndex > 0, "no tokens minted yet"); | ||
require(nextOwnerToExplicitlySet < currentIndex, "all ownerships have been set"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added this to prevent unnecessary calls to this function
35d8f9b
to
8063078
Compare
_setOwnersExplicit doesn't need to be in the base level contract as most users won't need it. However, we want to provide it as an optional extension in case some users want to explicitly set ownerships after a mint is over.