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 11 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
174 changes: 142 additions & 32 deletions contracts/ERC721A.sol
Original file line number Diff line number Diff line change
Expand Up @@ -16,27 +16,40 @@ import '@openzeppelin/contracts/utils/introspection/ERC165.sol';
* @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..).
* Assumes serials are sequentially minted starting at 0 (e.g. 0, 1, 2, 3..),
* or 1 (e.g. 1, 2, 3, 4..).
*
* 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 = 0;
Vectorized marked this conversation as resolved.
Show resolved Hide resolved

uint128 internal totalBurned = 0;

// Token name
string private _name;
Expand All @@ -62,19 +75,51 @@ contract ERC721A is Context, ERC165, IERC721, IERC721Metadata, IERC721Enumerable
_symbol = symbol_;
}

/**
* @dev Skips the zero index.
* This method must be called before any mints (e.g. in the constructor).
*/
function _initOneIndexed() internal {
Vectorized marked this conversation as resolved.
Show resolved Hide resolved
require(currentIndex == 0, 'ERC721A: 0 index already occupied');
currentIndex = 1;
totalBurned = 1;
_ownerships[0].burned = true;
}

/**
* @dev See {IERC721Enumerable-totalSupply}.
*/
function totalSupply() public view override returns (uint256) {
return currentIndex;
// Counter underflow is impossible as totalBurned cannot be incremented
// more than currentIndex times
unchecked {
return currentIndex - totalBurned;
}
}

/**
* @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) {
require(index < totalSupply(), 'ERC721A: global index out of bounds');
return index;
uint256 numMintedSoFar = currentIndex;
uint256 tokenIdsIdx = 0;
Vectorized marked this conversation as resolved.
Show resolved Hide resolved

// Counter overflow is impossible as the loop breaks when
// uint256 i is equal to another uint256 numMintedSoFar.
unchecked {
for (uint256 i = 0; i < numMintedSoFar; i++) {
Vectorized marked this conversation as resolved.
Show resolved Hide resolved
TokenOwnership memory ownership = _ownerships[i];
if (!ownership.burned) {
if (tokenIdsIdx == index) {
return i;
}
tokenIdsIdx++;
}
}
}
revert('ERC721A: global index out of bounds');
}

/**
Expand All @@ -84,17 +129,21 @@ contract ERC721A is Context, ERC165, IERC721, IERC721Metadata, IERC721Enumerable
*/
function tokenOfOwnerByIndex(address owner, uint256 index) public view override returns (uint256) {
require(index < balanceOf(owner), 'ERC721A: owner index out of bounds');
uint256 numMintedSoFar = totalSupply();
uint256 tokenIdsIdx;
address currOwnershipAddr;
uint256 numMintedSoFar = currentIndex;
uint256 tokenIdsIdx = 0;
address currOwnershipAddr = address(0);
Vectorized marked this conversation as resolved.
Show resolved Hide resolved

// 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++) {
for (uint256 i = 0; i < numMintedSoFar; i++) {
TokenOwnership memory ownership = _ownerships[i];
if (ownership.addr != address(0)) {
currOwnershipAddr = ownership.addr;
}
if (ownership.burned) {
Vectorized marked this conversation as resolved.
Show resolved Hide resolved
currOwnershipAddr = address(0);
}
if (currOwnershipAddr == owner) {
if (tokenIdsIdx == index) {
return i;
Expand All @@ -103,7 +152,6 @@ contract ERC721A is Context, ERC165, IERC721, IERC721Metadata, IERC721Enumerable
}
}
}

revert('ERC721A: unable to get token of owner by index');
}

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

function _numberBurned(address owner) internal view returns (uint256) {
require(owner != address(0), 'ERC721A: number burned query for the zero address');
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) {
require(_exists(tokenId), 'ERC721A: owner query for nonexistent token');
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('ERC721A: unable to determine the owner of token');
revert('ERC721A: owner query for nonexistent token');
}

/**
Expand Down Expand Up @@ -178,7 +240,7 @@ contract ERC721A is Context, ERC165, IERC721, IERC721Metadata, IERC721Enumerable
require(_exists(tokenId), 'ERC721Metadata: URI query for nonexistent token');

string memory baseURI = _baseURI();
return bytes(baseURI).length != 0 ? string(abi.encodePacked(baseURI, tokenId.toString())) : '';
return bytes(baseURI).length > 0 ? string(abi.encodePacked(baseURI, tokenId.toString())) : '';
}

/**
Expand Down Expand Up @@ -277,7 +339,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 @@ -320,37 +382,38 @@ contract ERC721A is Context, ERC165, IERC721, IERC721Metadata, IERC721Enumerable
) internal {
uint256 startTokenId = currentIndex;
require(to != address(0), 'ERC721A: mint to the zero address');
require(quantity != 0, 'ERC721A: quantity must be greater than 0');
// We know if the first token in the batch doesn't exist, the other ones don't as well, because of serial ordering.
// require(!_exists(startTokenId), 'ERC721A: token already minted');
require(quantity > 0, 'ERC721A: quantity must be greater than 0');

_beforeTokenTransfers(address(0), to, startTokenId, quantity);

// 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
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);

uint256 updatedIndex = startTokenId;

for (uint256 i; i < quantity; i++) {
for (uint256 i = 0; i < quantity; i++) {
emit Transfer(address(0), to, updatedIndex);
if (safe) {
require(
_checkOnERC721Received(address(0), to, updatedIndex, _data),
'ERC721A: transfer to non ERC721Receiver implementer'
);
}

updatedIndex++;
}

currentIndex = updatedIndex;
require(updatedIndex <= type(uint128).max, 'ERC721A: safecast overflow');
currentIndex = uint128(updatedIndex);
}

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

Expand Down Expand Up @@ -387,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 @@ -410,6 +473,53 @@ 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 transfer initiator owns it.
Vectorized marked this conversation as resolved.
Show resolved Hide resolved
// 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

if (_exists(nextTokenId)) {
_ownerships[nextTokenId].addr = prevOwnership.addr;
_ownerships[nextTokenId].startTimestamp = prevOwnership.startTimestamp;
}
}
}

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

totalBurned++;
}

/**
* @dev Approve `to` to operate on `tokenId`
*
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.0;

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()));

require(isApprovedOrOwner, 'ERC721A: caller is not owner nor approved');

_burn(tokenId);
}
}
4 changes: 2 additions & 2 deletions contracts/extensions/ERC721AOwnersExplicit.sol
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ pragma solidity ^0.8.0;
import '../ERC721A.sol';

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 @@ -28,7 +28,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
26 changes: 26 additions & 0 deletions contracts/mocks/ERC721ABurnableMock.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
// SPDX-License-Identifier: MIT
// Creators: Chiru Labs

pragma solidity ^0.8.0;

import '../extensions/ERC721ABurnable.sol';

contract ERC721ABurnableMock is ERC721A, ERC721ABurnable {
constructor(string memory name_, string memory symbol_) ERC721A(name_, symbol_) {}

function exists(uint256 tokenId) public view returns (bool) {
return _exists(tokenId);
}

function safeMint(address to, uint256 quantity) public {
_safeMint(to, quantity);
}

function initOneIndexed() public {
_initOneIndexed();
}

function getOwnershipAt(uint256 index) public view returns (TokenOwnership memory) {
return _ownerships[index];
}
}
Loading