Added burn functionality #61

Vectorized · 2022-02-01T11:42:14Z

An internal _burn function which behaves like the OpenZeppelin's ERC721 implementation.
- Burns to the zero address.
- Decreases totalSupply.
- Relevant functions and extensions updated to support burn function.
An public burn function which behaves like the OpenZeppelin's ERC721Burnable implementation.
- Moved to a separate extension extensions/ERC721ABurnable.sol
Tests for ERC721ABurnable and ERC721ABurnable + ERC721AOwnersExplicit.
Changed AddressData values from uint128 to uint64 to pack more information per SSTORE.
- It is extremely unlikely that a typical NFT project can use 2**64-1 slots.
- Added numberBurned.

This implementation is designed for future extensibility, for example:

An additional aux variable under AddressData to store miscellaneous variable(s) to allow users to make use of the remaining 64-bits
(e.g. to store number of pre-sale mints for each address).

contracts/ERC721A.sol

locationtba · 2022-02-01T18:42:09Z

was just about to start this PR good thing i checked first lol

contracts/ERC721A.sol

locationtba

thanks for putting this together, def a common ask for burn. think we could squeeze a bit more optimization out of this

contracts/ERC721A.sol

Vectorized · 2022-02-02T23:07:41Z

Added the optimizations from 1129166

cygaar

Try to keep the gas optimizations we added in #59 - mostly just need to remove the initializations to 0.

contracts/ERC721A.sol

test/extensions/ERC721ABurnable.test.js

contracts/ERC721A.sol

Vectorized · 2022-02-09T22:28:13Z

Added #84, #85, #87.

#84 _currentIndex instead of _currentindex.
#85 revert() instead of assert(false) to remove compilation warning.

@cygaar

cygaar · 2022-02-10T06:04:49Z

@Vectorized sorry for the delay I've been trying to clear out these smaller PRs to shorten the backlog, will look through this PR tomorrow

update specs for _beforeTokenTransfers and _afterTokenTransfers hooks

locationtba

looks pretty awesome! just some small nits, thanks for pushing this froward

locationtba · 2022-02-10T18:37:53Z

contracts/ERC721A.sol

+    uint128 internal _currentIndex;
+
+    uint128 internal _burnCounter;


can we put this in a struct?

I’d suggest simply leaving a comment stating that the compiler will pack the variables into a single 256 bit word.

Otherwise we have the great burden of coming up with another meaningful variable name and struct name.

locationtba · 2022-02-10T18:47:39Z

contracts/ERC721A.sol

+                if (!ownership.burned) {
+                    if (ownership.addr != address(0)) {
+                        return ownership;
+                    }


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

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)

locationtba · 2022-02-10T18:55:09Z

contracts/ERC721A.sol

                updatedIndex++;
            }

-            _currentIndex = updatedIndex;
+            if (updatedIndex > type(uint128).max) revert SafecastOverflow();


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

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.

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.

cygaar

Just a couple small notes on tests and then I'm ready to approve

cygaar · 2022-02-11T21:18:58Z

test/extensions/ERC721ABurnable.test.js

+      }
+    });
+
+    it('with token after previously burnt token transfered and burned', async function () {


Can you add a test to validate that transferring a burnt token gets reverted?

cygaar · 2022-02-11T21:28:10Z

contracts/ERC721A.sol

+            // 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)) {


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

cygaar · 2022-02-11T21:31:56Z

test/extensions/ERC721ABurnable.test.js

+    this.addr1 = addr1;
+    this.addr2 = addr2;
+    await this.token['safeMint(address,uint256)'](this.addr1.address, 10);
+    await this.token.connect(this.addr1).burn(5);


can you make 5 here a constant called burnedTokenId and use that in the tests below to make it clear what you're testing for? I want to avoid magic numbers where we can

cygaar · 2022-02-12T00:58:33Z

I also merged your other PR in, so you'll need to rebase

ahbanavi · 2022-02-12T08:37:52Z

contracts/extensions/ERC721ABurnable.sol

+        bool isApprovedOrOwner = (_msgSender() == prevOwnership.addr ||
+            isApprovedForAll(prevOwnership.addr, _msgSender()) ||
+            getApproved(tokenId) == _msgSender());
+
+        if (!isApprovedOrOwner) revert TransferCallerNotOwnerNorApproved();


Is there any reason that you check for this here instead of internal _burn function?

To be consistent with OpenZeppelin’s implementation.

ahbanavi · 2022-02-12T16:53:04Z

Congrats! 🎉 Kudos to @Vectorized and everyone else who worked on this feature. 🚀

@Vectorized

#66) * replaces currentIndex with _nextTokenId which starts at 1 instead of 0 * adapts test to new starting index of 1 * makes `ERC721AOwnersExplicit` test work with new starting index * explicitly sets `nextOwnerToExplicitlySet` to 1 because it's initialised with zero * remove vscode files * implements @jpegditials suggestions * adds tests for `tokenByIndex` and `tokenOfOwnerByIndex` * prevents tokenId 0 to be an existing token * adapts `tokenOfOwnerByIndex` for starting token index 1 and making tests for it more elaborate. * renames variable to make it more clear, it's index by the tokenId * adds unchecked scopes to save on gas as suggested by @Vectorized * more gas optimizations by @Vectorized * implements suggested `_startTokenId` function to override to set starting index * minor adaptions to work with _startTokenId() properly * brings back original tests * creates copy of original test with startTokenId set to 1 * adapts comment to reflect flexibility of the starting tokenId * adapts solidity version of tests * Replaces revert messages with new contract errors * fix merge hiccup * optimisations to save on gas used during deployment * Added #61 * Edited comments * Edited README roadmap * Removed burn test with one-index mock * Changed _startTokenId to _currentIndex * adapting test and mock related to index starting at 1 to make them work with recent changes on `main` * add _totalMinted function * add tests for _totalMinted * removes uint128 cast, simplifies comment and lints file * rename `ERC721AExplicitOwnershipMock` to `ERC721AOwnersExplicitMock` to be aligned with the naming of the contract * extend from already mocked ERC721A versions to dry code. * aligns file naming and contract naming * creates new ERC721A mock which enables the user to specify the startTokenId dynamically * converts ERC721A test to a parameterized test to work with other mocks as well. * extracts Helper contract to it's own file * parameterises ERC721A Explicit Owner Tests to also test a Version of the Contract with a different start token id. * makes it more transparent where the startTokenId value is coming from * creates a ERC721ABurnable Mock with custom startTokenId and uses it in a parameterised test suite testing the burnable extension. * removes code used during development * unifies code style across all tests Co-authored-by: webby1111 <webby1111@hotmail.com> Co-authored-by: Amirhossein Banavi <ahbanavi@gmail.com>

Vectorized added 2 commits February 1, 2022 11:29

Added burn functionality.

d92c3d0

Changed _initOneIndexed

4a8bc00

naomsa reviewed Feb 1, 2022

View reviewed changes

contracts/ERC721A.sol Outdated Show resolved Hide resolved

naomsa reviewed Feb 1, 2022

View reviewed changes

contracts/ERC721A.sol Outdated Show resolved Hide resolved

Vectorized added 3 commits February 1, 2022 15:00

Moved burn function into ERC721ABurnable

00fe576

Moved burn function into ERC721ABurnable

2052461

Remove redundant burn check in ownershipOf

33d4b41

Vectorized changed the title ~~Added burn functionality.~~ Added burn functionality Feb 1, 2022

locationtba reviewed Feb 1, 2022

View reviewed changes

contracts/ERC721A.sol Outdated Show resolved Hide resolved

locationtba reviewed Feb 1, 2022

View reviewed changes

contracts/ERC721A.sol Outdated Show resolved Hide resolved

locationtba reviewed Feb 1, 2022

View reviewed changes

contracts/ERC721A.sol Outdated Show resolved Hide resolved

locationtba reviewed Feb 1, 2022

View reviewed changes

contracts/ERC721A.sol Outdated Show resolved Hide resolved

locationtba reviewed Feb 1, 2022

View reviewed changes

Vectorized added 2 commits February 1, 2022 20:31

Optimized ownershipOf

5a6c851

Removed aux from AddressData for future PR

a2ef813

locationtba reviewed Feb 1, 2022

View reviewed changes

contracts/ERC721A.sol Outdated Show resolved Hide resolved

Vectorized added 2 commits February 1, 2022 22:09

Packed currentIndex and totalBurned for gas savings

07573e9

Added gas optimizations

bfd6d97

Vectorized changed the title ~~Added burn functionality~~ Added burn functionality + optional id starting from 1 Feb 2, 2022

Vectorized changed the title ~~Added burn functionality + optional id starting from 1~~ Added burn functionality + optional id starting from 1 via burn Feb 2, 2022

Added gas optimizations

8fc600f

Vectorized changed the title ~~Added burn functionality + optional id starting from 1 via burn~~ Added burn functionality + optional id starting from 1 Feb 2, 2022

Merge branch 'main' into feature/burnable

e23e803

Vectorized changed the title ~~Added burn functionality + optional id starting from 1~~ Added burn functionality Feb 2, 2022

cygaar requested changes Feb 3, 2022

View reviewed changes

Vectorized added 3 commits February 3, 2022 09:06

Added requested changes

8ff5722

Merge branch 'chiru-labs:main' into feature/burnable

a66b24b

Edited comments.

e1df945

Vectorized and others added 7 commits February 9, 2022 08:15

Fixed missing change from chiru-labs#59

64bc7e7

Gas optimization

01e7ade

update specs for _beforeTokenTransfers and _afterTokenTransfers hooks

2e4857d

Added chiru-labs#84

5ac0204

Added chiru-labs#87

53ca717

Merge branch 'main' into feature/burnable

d08c926

Added chiru-labs#85

c493bf1

Vectorized added 4 commits February 10, 2022 16:01

Merge branch 'main' into feature/burnable

492ec57

Merge branch 'feature/burnable' into feature/burnable

7cf1fca

Merge pull request #1 from ahbanavi/feature/burnable

1aeb1b8

update specs for _beforeTokenTransfers and _afterTokenTransfers hooks

Added chiru-labs#89

1bc34da

locationtba reviewed Feb 10, 2022

View reviewed changes

Vectorized added 2 commits February 11, 2022 05:50

Added comments on packing _currentIndex and _burnCounter

6e1ab7a

Removed overflow check for updatedIndex

9ee026c

cygaar reviewed Feb 11, 2022

View reviewed changes

Vectorized added 3 commits February 12, 2022 02:20

Added requested test changes

314faba

Removed unused variable in burn test

c539195

Removed unused variable in burn test

ca9bb69

ahbanavi reviewed Feb 12, 2022

View reviewed changes

ahbanavi mentioned this pull request Feb 12, 2022

Add burn feature #2

Closed

cygaar approved these changes Feb 12, 2022

View reviewed changes

cygaar merged commit fb22621 into chiru-labs:main Feb 12, 2022

Vectorized added a commit to Vectorized/ERC721A that referenced this pull request Feb 13, 2022

Added chiru-labs#61

d34d4ac

ahbanavi mentioned this pull request Feb 18, 2022

One user can burn another user's tokens? #117

Closed

Vectorized deleted the feature/burnable branch April 12, 2022 13:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added burn functionality #61

Added burn functionality #61

Vectorized commented Feb 1, 2022 •

edited

Loading

locationtba commented Feb 1, 2022

locationtba left a comment

Vectorized commented Feb 2, 2022

cygaar left a comment

Vectorized commented Feb 9, 2022 •

edited

Loading

cygaar commented Feb 10, 2022

locationtba left a comment

locationtba Feb 10, 2022

Vectorized Feb 10, 2022 •

edited

Loading

locationtba Feb 10, 2022

Vectorized Feb 11, 2022

locationtba Feb 10, 2022

ahbanavi Feb 10, 2022 •

edited

Loading

Vectorized Feb 11, 2022 •

edited

Loading

cygaar left a comment

cygaar Feb 11, 2022

cygaar Feb 11, 2022

cygaar Feb 11, 2022

cygaar commented Feb 12, 2022

ahbanavi Feb 12, 2022

Vectorized Feb 12, 2022 •

edited

Loading

ahbanavi commented Feb 12, 2022

		uint128 internal _currentIndex;

		uint128 internal _burnCounter;

Added burn functionality #61

Added burn functionality #61

Conversation

Vectorized commented Feb 1, 2022 • edited Loading

locationtba commented Feb 1, 2022

locationtba left a comment

Choose a reason for hiding this comment

Vectorized commented Feb 2, 2022

cygaar left a comment

Choose a reason for hiding this comment

Vectorized commented Feb 9, 2022 • edited Loading

cygaar commented Feb 10, 2022

locationtba left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Vectorized Feb 10, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahbanavi Feb 10, 2022 • edited Loading

Choose a reason for hiding this comment

Vectorized Feb 11, 2022 • edited Loading

Choose a reason for hiding this comment

cygaar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cygaar commented Feb 12, 2022

Choose a reason for hiding this comment

Vectorized Feb 12, 2022 • edited Loading

Choose a reason for hiding this comment

ahbanavi commented Feb 12, 2022

Vectorized commented Feb 1, 2022 •

edited

Loading

Vectorized commented Feb 9, 2022 •

edited

Loading

Vectorized Feb 10, 2022 •

edited

Loading

ahbanavi Feb 10, 2022 •

edited

Loading

Vectorized Feb 11, 2022 •

edited

Loading

Vectorized Feb 12, 2022 •

edited

Loading