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

Added burn functionality #61

Merged
merged 41 commits into from
Feb 12, 2022
Merged
Show file tree
Hide file tree
Changes from 25 commits
Commits
Show all changes
41 commits
Select commit Hold shift + click to select a range
d92c3d0
Added burn functionality.
Vectorized Feb 1, 2022
4a8bc00
Changed _initOneIndexed
Vectorized Feb 1, 2022
00fe576
Moved burn function into ERC721ABurnable
Vectorized Feb 1, 2022
2052461
Moved burn function into ERC721ABurnable
Vectorized Feb 1, 2022
33d4b41
Remove redundant burn check in ownershipOf
Vectorized Feb 1, 2022
5a6c851
Optimized ownershipOf
Vectorized Feb 1, 2022
a2ef813
Removed aux from AddressData for future PR
Vectorized Feb 1, 2022
07573e9
Packed currentIndex and totalBurned for gas savings
Vectorized Feb 1, 2022
bfd6d97
Added gas optimizations
Vectorized Feb 2, 2022
8fc600f
Added gas optimizations
Vectorized Feb 2, 2022
e23e803
Merge branch 'main' into feature/burnable
Vectorized Feb 2, 2022
8ff5722
Added requested changes
Vectorized Feb 3, 2022
a66b24b
Merge branch 'chiru-labs:main' into feature/burnable
Vectorized Feb 3, 2022
e1df945
Edited comments.
Vectorized Feb 3, 2022
9820bcb
Renamed totalBurned to burnedCounter
Vectorized Feb 4, 2022
7a02958
Renamed to burnCounter
Vectorized Feb 5, 2022
1cf3bc1
Updated comments.
Vectorized Feb 7, 2022
f25c639
Mark transferFrom/safeTransferFrom virtual
Vectorized Feb 7, 2022
92c82e1
Mark transferFrom/safeTransferFrom virtual
Vectorized Feb 7, 2022
66b488f
Updated comments.
Vectorized Feb 7, 2022
39ff829
Tidy up tests
Vectorized Feb 7, 2022
212912c
Inlined _exists for _burn and _transfer.
Vectorized Feb 7, 2022
bcbfb94
Merged custom errors
Vectorized Feb 8, 2022
1f3bc9e
Merged custom errors
Vectorized Feb 8, 2022
a06b5bc
Merge branch 'main' into feature/burnable
Vectorized Feb 8, 2022
64bc7e7
Fixed missing change from #59
Vectorized Feb 9, 2022
01e7ade
Gas optimization
Vectorized Feb 9, 2022
2e4857d
update specs for _beforeTokenTransfers and _afterTokenTransfers hooks
ahbanavi Feb 9, 2022
5ac0204
Added #84
Vectorized Feb 9, 2022
53ca717
Added #87
Vectorized Feb 9, 2022
d08c926
Merge branch 'main' into feature/burnable
Vectorized Feb 9, 2022
c493bf1
Added #85
Vectorized Feb 9, 2022
492ec57
Merge branch 'main' into feature/burnable
Vectorized Feb 10, 2022
7cf1fca
Merge branch 'feature/burnable' into feature/burnable
Vectorized Feb 10, 2022
1aeb1b8
Merge pull request #1 from ahbanavi/feature/burnable
Vectorized Feb 10, 2022
1bc34da
Added #89
Vectorized Feb 10, 2022
6e1ab7a
Added comments on packing _currentIndex and _burnCounter
Vectorized Feb 11, 2022
9ee026c
Removed overflow check for updatedIndex
Vectorized Feb 11, 2022
314faba
Added requested test changes
Vectorized Feb 12, 2022
c539195
Removed unused variable in burn test
Vectorized Feb 12, 2022
ca9bb69
Removed unused variable in burn test
Vectorized Feb 12, 2022
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,6 @@ contract Azuki is ERC721A {

## Roadmap

- [] Add burn function
- [] Add flexibility for the first token id to not start at 0
- [] Support ERC721 Upgradeable
- [] Add more documentation on benefits of using ERC721A
Expand Down
174 changes: 140 additions & 34 deletions contracts/ERC721A.sol
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ error ApproveToCaller();
error ApprovalToCurrentOwner();
error BalanceQueryForZeroAddress();
error MintedQueryForZeroAddress();
error BurnedQueryForZeroAddress();
error MintToZeroAddress();
error MintZeroQuantity();
error OwnerIndexOutOfBounds();
Expand All @@ -27,35 +28,47 @@ error TransferCallerNotOwnerNorApproved();
error TransferFromIncorrectOwner();
error TransferToNonERC721ReceiverImplementer();
error TransferToZeroAddress();
error UnableDetermineTokenOwner();
error UnableGetTokenOwnerByIndex();
error URIQueryForNonexistentToken();
error SafecastOverflow();

/**
* @dev Implementation of https://eips.ethereum.org/EIPS/eip-721[ERC721] Non-Fungible Token Standard, including
* the Metadata and Enumerable extension. Built to optimize for lower gas during batch mints.
*
* Assumes serials are sequentially minted starting at 0 (e.g. 0, 1, 2, 3..).
*
* Does not support burning tokens to address(0).
* Assumes that an owner cannot have more than 2**64 - 1 (max value of uint64) of supply.
*
* Assumes that an owner cannot have more than the 2**128 - 1 (max value of uint128) of supply
* Assumes that the maximum token id cannot exceed 2**128 - 1 (max value of uint128).
*/
contract ERC721A is Context, ERC165, IERC721, IERC721Metadata, IERC721Enumerable {
using Address for address;
using Strings for uint256;

// Compiler will pack this into a single 256bit word.
struct TokenOwnership {
// The address of the owner.
address addr;
// Keeps track of the start time of ownership with minimal overhead for tokenomics.
uint64 startTimestamp;
// Whether the token has been burned.
bool burned;
}

// Compiler will pack this into a single 256bit word.
struct AddressData {
uint128 balance;
uint128 numberMinted;
// Realistically, 2**64-1 is more than enough.
uint64 balance;
// Keeps track of mint count with minimal overhead for tokenomics.
uint64 numberMinted;
// Keeps track of burn count with minimal overhead for tokenomics.
uint64 numberBurned;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Any reason why you decided to include this?

Copy link
Collaborator Author

@Vectorized Vectorized Feb 8, 2022

Choose a reason for hiding this comment

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

Keeping track of numberMinted and the block timestamps for ownerships are not in the spec, but are useful to squeeze the most out of each SSTORE. Going by the same spirit, numberBurned is included.

If you prefer, it can be removed or moved to a future PR. Let me know what you think.

Copy link
Collaborator

Choose a reason for hiding this comment

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

We can keep this in for now - was just curious. We're still going through this PR, just been a super busy week. Will try to get this done soon!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Of the currently opened feature PRs, which ones likely to be merged before this PR?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Merged the custom errors from #73.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This is the next PR I'm reviewing

}

uint256 internal currentIndex;
uint128 internal currentIndex;

uint128 internal burnCounter;

// Token name
string private _name;
Expand Down Expand Up @@ -85,15 +98,36 @@ contract ERC721A is Context, ERC165, IERC721, IERC721Metadata, IERC721Enumerable
* @dev See {IERC721Enumerable-totalSupply}.
*/
function totalSupply() public view override returns (uint256) {
return currentIndex;
// Counter underflow is impossible as burnCounter cannot be incremented
// more than currentIndex times
unchecked {
return currentIndex - burnCounter;
}
}

/**
* @dev See {IERC721Enumerable-tokenByIndex}.
* This read function is O(totalSupply). If calling from a separate contract, be sure to test gas first.
* It may also degrade with extremely large collection sizes (e.g >> 10000), test for your use case.
*/
function tokenByIndex(uint256 index) public view override returns (uint256) {
if (index >= totalSupply()) revert TokenIndexOutOfBounds();
return index;
uint256 numMintedSoFar = currentIndex;
uint256 tokenIdsIdx;

// Counter overflow is impossible as the loop breaks when
// uint256 i is equal to another uint256 numMintedSoFar.
unchecked {
for (uint256 i; i < numMintedSoFar; i++) {
TokenOwnership memory ownership = _ownerships[i];
if (!ownership.burned) {
if (tokenIdsIdx == index) {
return i;
}
tokenIdsIdx++;
}
}
}
revert TokenIndexOutOfBounds();
}

/**
Expand All @@ -103,14 +137,18 @@ contract ERC721A is Context, ERC165, IERC721, IERC721Metadata, IERC721Enumerable
*/
function tokenOfOwnerByIndex(address owner, uint256 index) public view override returns (uint256) {
if (index >= balanceOf(owner)) revert OwnerIndexOutOfBounds();
uint256 numMintedSoFar = totalSupply();
uint256 numMintedSoFar = currentIndex;
uint256 tokenIdsIdx;
address currOwnershipAddr;

// Counter overflow is impossible as the loop breaks when uint256 i is equal to another uint256 numMintedSoFar.
// Counter overflow is impossible as the loop breaks when
// uint256 i is equal to another uint256 numMintedSoFar.
unchecked {
for (uint256 i; i < numMintedSoFar; i++) {
TokenOwnership memory ownership = _ownerships[i];
if (ownership.burned) {
continue;
}
if (ownership.addr != address(0)) {
currOwnershipAddr = ownership.addr;
}
Expand All @@ -122,7 +160,6 @@ contract ERC721A is Context, ERC165, IERC721, IERC721Metadata, IERC721Enumerable
}
}
}

revert UnableGetTokenOwnerByIndex();
}

Expand Down Expand Up @@ -150,23 +187,37 @@ contract ERC721A is Context, ERC165, IERC721, IERC721Metadata, IERC721Enumerable
return uint256(_addressData[owner].numberMinted);
}

function _numberBurned(address owner) internal view returns (uint256) {
if (owner == address(0)) revert BurnedQueryForZeroAddress();
return uint256(_addressData[owner].numberBurned);
}

/**
* Gas spent here starts off proportional to the maximum mint batch size.
* It gradually moves to O(1) as tokens get transferred around in the collection over time.
*/
function ownershipOf(uint256 tokenId) internal view returns (TokenOwnership memory) {
if (!_exists(tokenId)) revert OwnerQueryForNonexistentToken();
uint256 curr = tokenId;

// Underflow is impossible because curr must be > 0 before decrement.
unchecked {
for (uint256 curr = tokenId; curr >= 0; curr--) {
if (curr < currentIndex) {
TokenOwnership memory ownership = _ownerships[curr];
if (ownership.addr != address(0)) {
return ownership;
if (!ownership.burned) {
if (ownership.addr != address(0)) {
return ownership;
}
Comment on lines +210 to +213

Choose a reason for hiding this comment

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

If we generally don't expect too many burns, it might be more optimal to check burned after L207

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Burn check has to be done for all cases.

  • Start of search is burned. (revert)
  • Start of search has address and not burned. (early return ownership)
  • Start of search has no address and not burned. (backward search)

while (curr > 0) {
curr--;
ownership = _ownerships[curr];
if (ownership.addr != address(0)) {
return ownership;
}
}
}
}
}

revert UnableDetermineTokenOwner();
revert OwnerQueryForNonexistentToken();
}

/**
Expand Down Expand Up @@ -216,8 +267,10 @@ contract ERC721A is Context, ERC165, IERC721, IERC721Metadata, IERC721Enumerable
address owner = ERC721A.ownerOf(tokenId);
if (to == owner) revert ApprovalToCurrentOwner();

if (_msgSender() != owner && !isApprovedForAll(owner, _msgSender())) revert ApprovalCallerNotOwnerNorApproved();

if (_msgSender() != owner && !isApprovedForAll(owner, _msgSender())) {
revert ApprovalCallerNotOwnerNorApproved();
}

_approve(to, tokenId, owner);
}

Expand Down Expand Up @@ -277,9 +330,11 @@ contract ERC721A is Context, ERC165, IERC721, IERC721Metadata, IERC721Enumerable
address to,
uint256 tokenId,
bytes memory _data
) public override {
) public virtual override {
_transfer(from, to, tokenId);
if (!_checkOnERC721Received(from, to, tokenId, _data)) revert TransferToNonERC721ReceiverImplementer();
if (!_checkOnERC721Received(from, to, tokenId, _data)) {
revert TransferToNonERC721ReceiverImplementer();
}
}

/**
Expand All @@ -290,7 +345,7 @@ contract ERC721A is Context, ERC165, IERC721, IERC721Metadata, IERC721Enumerable
* Tokens start existing when they are minted (`_mint`),
*/
function _exists(uint256 tokenId) internal view returns (bool) {
return tokenId < currentIndex;
return tokenId < currentIndex && !_ownerships[tokenId].burned;
Vectorized marked this conversation as resolved.
Show resolved Hide resolved
}

function _safeMint(address to, uint256 quantity) internal {
Expand Down Expand Up @@ -339,10 +394,10 @@ contract ERC721A is Context, ERC165, IERC721, IERC721Metadata, IERC721Enumerable

// Overflows are incredibly unrealistic.
// balance or numberMinted overflow if current value of either + quantity > 3.4e38 (2**128) - 1
// updatedIndex overflows if currentIndex + quantity > 1.56e77 (2**256) - 1
// updatedIndex overflows if currentIndex + quantity > 3.4e38 (2**128) - 1
unchecked {
_addressData[to].balance += uint128(quantity);
_addressData[to].numberMinted += uint128(quantity);
_addressData[to].balance += uint64(quantity);
_addressData[to].numberMinted += uint64(quantity);

_ownerships[startTokenId].addr = to;
_ownerships[startTokenId].startTimestamp = uint64(block.timestamp);
Expand All @@ -354,13 +409,12 @@ contract ERC721A is Context, ERC165, IERC721, IERC721Metadata, IERC721Enumerable
if (safe && !_checkOnERC721Received(address(0), to, updatedIndex, _data)) {
revert TransferToNonERC721ReceiverImplementer();
}

updatedIndex++;
}

currentIndex = updatedIndex;
if (updatedIndex > type(uint128).max) revert SafecastOverflow();

Choose a reason for hiding this comment

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

we make assumptions a lot that we won't encounter overflow, is there a reason we're explicitly checking for it here? If we do want to check i would prefer a separate PR that does similar checks everywhere and allows for discussion

Copy link
Contributor

@ahbanavi ahbanavi Feb 10, 2022

Choose a reason for hiding this comment

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

I think the same, also I think all the changes in the _mint function should be in another PR if they are irrelevant to burn functionality.
Edit:
I see now it's for changing _currentIndex type from unit256 to uint128.
Think we should keep using unit256 in this PR for both _currentIndex and _burnCounter and after merge, we open another PR to discuss changing the types. It's out of burn functionality context.

Copy link
Collaborator Author

@Vectorized Vectorized Feb 11, 2022

Choose a reason for hiding this comment

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

I think we can remove the overflow check.

Most if not all users won't need the check. Even if the startIndex can be customized (in the future), most users will choose either 0 or 1, so overflow will still be very unrealistic. I think leaving a cautionary comment will suffice in that case.

As for packing the _currentIndex and _burnCounter, I'm kinda leaning towards including it in this PR.
It was suggested by @locationtba in the spirit of ensuring that the added functionality is gas efficient too.

currentIndex = uint128(updatedIndex);
}

_afterTokenTransfers(address(0), to, startTokenId, quantity);
}

Expand Down Expand Up @@ -396,10 +450,10 @@ contract ERC721A is Context, ERC165, IERC721, IERC721Metadata, IERC721Enumerable

// Underflow of the sender's balance is impossible because we check for
// ownership above and the recipient's balance can't realistically overflow.
// Counter overflow is incredibly unrealistic as tokenId would have to be 2**256.
// Counter overflow is incredibly unrealistic as tokenId would have to be 2**128.
unchecked {
_addressData[from].balance -= 1;
_addressData[to].balance += 1;
_addressData[to].balance += 1;

_ownerships[tokenId].addr = to;
_ownerships[tokenId].startTimestamp = uint64(block.timestamp);
Expand All @@ -408,7 +462,9 @@ contract ERC721A is Context, ERC165, IERC721, IERC721Metadata, IERC721Enumerable
// Set the slot of tokenId+1 explicitly in storage to maintain correctness for ownerOf(tokenId+1) calls.
uint256 nextTokenId = tokenId + 1;
if (_ownerships[nextTokenId].addr == address(0)) {
if (_exists(nextTokenId)) {
// This will suffice for checking _exists(nextTokenId),
// as a burned slot cannot contain the zero address.
if (nextTokenId < currentIndex) {
_ownerships[nextTokenId].addr = prevOwnership.addr;
_ownerships[nextTokenId].startTimestamp = prevOwnership.startTimestamp;
}
Expand All @@ -419,6 +475,55 @@ contract ERC721A is Context, ERC165, IERC721, IERC721Metadata, IERC721Enumerable
_afterTokenTransfers(from, to, tokenId, 1);
}

/**
* @dev Destroys `tokenId`.
* The approval is cleared when the token is burned.
*
* Requirements:
*
* - `tokenId` must exist.
*
* Emits a {Transfer} event.
*/
function _burn(uint256 tokenId) internal virtual {
TokenOwnership memory prevOwnership = ownershipOf(tokenId);

_beforeTokenTransfers(prevOwnership.addr, address(0), tokenId, 1);

// Clear approvals from the previous owner
_approve(address(0), tokenId, prevOwnership.addr);

// Underflow of the sender's balance is impossible because we check for
// ownership above and the recipient's balance can't realistically overflow.
// Counter overflow is incredibly unrealistic as tokenId would have to be 2**128.
unchecked {
_addressData[prevOwnership.addr].balance -= 1;
_addressData[prevOwnership.addr].numberBurned += 1;

// Keep track of who burnt the token, and when is it burned.
_ownerships[tokenId].addr = prevOwnership.addr;
_ownerships[tokenId].startTimestamp = uint64(block.timestamp);
_ownerships[tokenId].burned = true;

// If the ownership slot of tokenId+1 is not explicitly set, that means the burn initiator owns it.
// Set the slot of tokenId+1 explicitly in storage to maintain correctness for ownerOf(tokenId+1) calls.
uint256 nextTokenId = tokenId + 1;
if (_ownerships[nextTokenId].addr == address(0)) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could be worth making this a helper function and re-use it here and in the _transfer function, but I don't have strong opinions here since you only have it in two places

// This will suffice for checking _exists(nextTokenId),
// as a burned slot cannot contain the zero address.
if (nextTokenId < currentIndex) {
_ownerships[nextTokenId].addr = prevOwnership.addr;
_ownerships[nextTokenId].startTimestamp = prevOwnership.startTimestamp;
}
}
}

emit Transfer(prevOwnership.addr, address(0), tokenId);
_afterTokenTransfers(prevOwnership.addr, address(0), tokenId, 1);

burnCounter++;
}

/**
* @dev Approve `to` to operate on `tokenId`
*
Expand Down Expand Up @@ -453,8 +558,9 @@ contract ERC721A is Context, ERC165, IERC721, IERC721Metadata, IERC721Enumerable
try IERC721Receiver(to).onERC721Received(_msgSender(), from, tokenId, _data) returns (bytes4 retval) {
return retval == IERC721Receiver(to).onERC721Received.selector;
} catch (bytes memory reason) {
if (reason.length == 0) revert TransferToNonERC721ReceiverImplementer();
else {
if (reason.length == 0) {
revert TransferToNonERC721ReceiverImplementer();
} else {
assembly {
revert(add(32, reason), mload(reason))
}
Expand Down
33 changes: 33 additions & 0 deletions contracts/extensions/ERC721ABurnable.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
// SPDX-License-Identifier: MIT
// Creator: Chiru Labs

pragma solidity ^0.8.4;

import '../ERC721A.sol';
import '@openzeppelin/contracts/utils/Context.sol';

/**
* @title ERC721A Burnable Token
* @dev ERC721A Token that can be irreversibly burned (destroyed).
*/
abstract contract ERC721ABurnable is Context, ERC721A {

/**
* @dev Burns `tokenId`. See {ERC721A-_burn}.
*
* Requirements:
*
* - The caller must own `tokenId` or be an approved operator.
*/
function burn(uint256 tokenId) public virtual {
TokenOwnership memory prevOwnership = ownershipOf(tokenId);

bool isApprovedOrOwner = (_msgSender() == prevOwnership.addr ||
getApproved(tokenId) == _msgSender() ||
isApprovedForAll(prevOwnership.addr, _msgSender()));

if (!isApprovedOrOwner) revert TransferCallerNotOwnerNorApproved();

_burn(tokenId);
}
}
4 changes: 2 additions & 2 deletions contracts/extensions/ERC721AOwnersExplicit.sol
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ error QuantityMustBeNonZero();
error NoTokensMintedYet();

abstract contract ERC721AOwnersExplicit is ERC721A {
uint256 public nextOwnerToExplicitlySet;
uint256 public nextOwnerToExplicitlySet = 0;
Vectorized marked this conversation as resolved.
Show resolved Hide resolved

/**
* @dev Explicitly set `owners` to eliminate loops in future calls of ownerOf().
Expand All @@ -32,7 +32,7 @@ abstract contract ERC721AOwnersExplicit is ERC721A {
}

for (uint256 i = _nextOwnerToExplicitlySet; i <= endIndex; i++) {
if (_ownerships[i].addr == address(0)) {
if (_ownerships[i].addr == address(0) && !_ownerships[i].burned) {
TokenOwnership memory ownership = ownershipOf(i);
_ownerships[i].addr = ownership.addr;
_ownerships[i].startTimestamp = ownership.startTimestamp;
Expand Down
Loading