-
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
Added burn functionality #61
Merged
Merged
Changes from all commits
Commits
Show all changes
41 commits
Select commit
Hold shift + click to select a range
d92c3d0
Added burn functionality.
Vectorized 4a8bc00
Changed _initOneIndexed
Vectorized 00fe576
Moved burn function into ERC721ABurnable
Vectorized 2052461
Moved burn function into ERC721ABurnable
Vectorized 33d4b41
Remove redundant burn check in ownershipOf
Vectorized 5a6c851
Optimized ownershipOf
Vectorized a2ef813
Removed aux from AddressData for future PR
Vectorized 07573e9
Packed currentIndex and totalBurned for gas savings
Vectorized bfd6d97
Added gas optimizations
Vectorized 8fc600f
Added gas optimizations
Vectorized e23e803
Merge branch 'main' into feature/burnable
Vectorized 8ff5722
Added requested changes
Vectorized a66b24b
Merge branch 'chiru-labs:main' into feature/burnable
Vectorized e1df945
Edited comments.
Vectorized 9820bcb
Renamed totalBurned to burnedCounter
Vectorized 7a02958
Renamed to burnCounter
Vectorized 1cf3bc1
Updated comments.
Vectorized f25c639
Mark transferFrom/safeTransferFrom virtual
Vectorized 92c82e1
Mark transferFrom/safeTransferFrom virtual
Vectorized 66b488f
Updated comments.
Vectorized 39ff829
Tidy up tests
Vectorized 212912c
Inlined _exists for _burn and _transfer.
Vectorized bcbfb94
Merged custom errors
Vectorized 1f3bc9e
Merged custom errors
Vectorized a06b5bc
Merge branch 'main' into feature/burnable
Vectorized 64bc7e7
Fixed missing change from #59
Vectorized 01e7ade
Gas optimization
Vectorized 2e4857d
update specs for _beforeTokenTransfers and _afterTokenTransfers hooks
ahbanavi 5ac0204
Added #84
Vectorized 53ca717
Added #87
Vectorized d08c926
Merge branch 'main' into feature/burnable
Vectorized c493bf1
Added #85
Vectorized 492ec57
Merge branch 'main' into feature/burnable
Vectorized 7cf1fca
Merge branch 'feature/burnable' into feature/burnable
Vectorized 1aeb1b8
Merge pull request #1 from ahbanavi/feature/burnable
Vectorized 1bc34da
Added #89
Vectorized 6e1ab7a
Added comments on packing _currentIndex and _burnCounter
Vectorized 9ee026c
Removed overflow check for updatedIndex
Vectorized 314faba
Added requested test changes
Vectorized c539195
Removed unused variable in burn test
Vectorized ca9bb69
Removed unused variable in burn test
Vectorized File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
Large diffs are not rendered by default.
Oops, something went wrong.
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
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 || | ||
isApprovedForAll(prevOwnership.addr, _msgSender()) || | ||
getApproved(tokenId) == _msgSender()); | ||
|
||
if (!isApprovedOrOwner) revert TransferCallerNotOwnerNorApproved(); | ||
|
||
_burn(tokenId); | ||
} | ||
} |
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
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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,22 @@ | ||
// SPDX-License-Identifier: MIT | ||
// Creators: Chiru Labs | ||
|
||
pragma solidity ^0.8.4; | ||
|
||
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 getOwnershipAt(uint256 index) public view returns (TokenOwnership memory) { | ||
return _ownerships[index]; | ||
} | ||
} |
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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,27 @@ | ||
// SPDX-License-Identifier: MIT | ||
// Creators: Chiru Labs | ||
|
||
pragma solidity ^0.8.4; | ||
|
||
import '../extensions/ERC721ABurnable.sol'; | ||
import '../extensions/ERC721AOwnersExplicit.sol'; | ||
|
||
contract ERC721ABurnableOwnersExplicitMock is ERC721A, ERC721ABurnable, ERC721AOwnersExplicit { | ||
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 setOwnersExplicit(uint256 quantity) public { | ||
_setOwnersExplicit(quantity); | ||
} | ||
|
||
function getOwnershipAt(uint256 index) public view returns (TokenOwnership memory) { | ||
return _ownerships[index]; | ||
} | ||
} |
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Oops, something went wrong.
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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,157 @@ | ||
const { expect } = require('chai'); | ||
|
||
describe('ERC721ABurnable', function () { | ||
|
||
beforeEach(async function () { | ||
this.ERC721ABurnable = await ethers.getContractFactory('ERC721ABurnableMock'); | ||
this.token = await this.ERC721ABurnable.deploy('Azuki', 'AZUKI'); | ||
await this.token.deployed(); | ||
}); | ||
|
||
beforeEach(async function () { | ||
const [owner, addr1, addr2] = await ethers.getSigners(); | ||
this.owner = owner; | ||
this.addr1 = addr1; | ||
this.addr2 = addr2; | ||
this.numTestTokens = 10; | ||
this.burnedTokenId = 5; | ||
await this.token['safeMint(address,uint256)'](this.addr1.address, this.numTestTokens); | ||
await this.token.connect(this.addr1).burn(this.burnedTokenId); | ||
}); | ||
|
||
it('changes exists', async function () { | ||
expect(await this.token.exists(this.burnedTokenId)).to.be.false; | ||
}); | ||
|
||
it('cannot burn a non-existing token', async function () { | ||
const query = this.token.connect(this.addr1).burn(this.numTestTokens); | ||
await expect(query).to.be.revertedWith( | ||
'OwnerQueryForNonexistentToken' | ||
); | ||
}); | ||
|
||
it('cannot burn a burned token', async function () { | ||
const query = this.token.connect(this.addr1).burn(this.burnedTokenId); | ||
await expect(query).to.be.revertedWith( | ||
'OwnerQueryForNonexistentToken' | ||
); | ||
}) | ||
|
||
it('cannot transfer a burned token', async function () { | ||
const query = this.token.connect(this.addr1) | ||
.transferFrom(this.addr1.address, this.addr2.address, this.burnedTokenId); | ||
await expect(query).to.be.revertedWith( | ||
'OwnerQueryForNonexistentToken' | ||
); | ||
}) | ||
|
||
it('reduces totalSupply', async function () { | ||
const supplyBefore = await this.token.totalSupply(); | ||
for (let i = 0; i < 2; ++i) { | ||
await this.token.connect(this.addr1).burn(i); | ||
expect(supplyBefore - (await this.token.totalSupply())).to.equal(i + 1); | ||
} | ||
}) | ||
|
||
it('adjusts owners tokens by index', async function () { | ||
const n = await this.token.totalSupply(); | ||
for (let i = 0; i < this.burnedTokenId; ++i) { | ||
expect(await this.token.tokenByIndex(i)).to.be.equal(i); | ||
} | ||
for (let i = this.burnedTokenId; i < n; ++i) { | ||
expect(await this.token.tokenByIndex(i)).to.be.equal(i + 1); | ||
} | ||
// tokenIds of addr1: [0,1,2,3,4,6,7,8,9] | ||
expect(await this.token.tokenOfOwnerByIndex(this.addr1.address, 2)) | ||
.to.be.equal(2); | ||
await this.token.connect(this.addr1).burn(2); | ||
// tokenIds of addr1: [0,1,3,4,6,7,8,9] | ||
expect(await this.token.tokenOfOwnerByIndex(this.addr1.address, 2)) | ||
.to.be.equal(3); | ||
await this.token.connect(this.addr1).burn(0); | ||
// tokenIds of addr1: [1,3,4,6,7,8,9] | ||
expect(await this.token.tokenOfOwnerByIndex(this.addr1.address, 2)) | ||
.to.be.equal(4); | ||
await this.token.connect(this.addr1).burn(3); | ||
// tokenIds of addr1: [1,4,6,7,8,9] | ||
expect(await this.token.tokenOfOwnerByIndex(this.addr1.address, 2)) | ||
.to.be.equal(6); | ||
}) | ||
|
||
it('adjusts owners balances', async function () { | ||
expect(await this.token.balanceOf(this.addr1.address)) | ||
.to.be.equal(this.numTestTokens - 1); | ||
}); | ||
|
||
it('adjusts token by index', async function () { | ||
const n = await this.token.totalSupply(); | ||
for (let i = 0; i < this.burnedTokenId; ++i) { | ||
expect(await this.token.tokenByIndex(i)).to.be.equal(i); | ||
} | ||
for (let i = this.burnedTokenId; i < n; ++i) { | ||
expect(await this.token.tokenByIndex(i)).to.be.equal(i + 1); | ||
} | ||
await expect(this.token.tokenByIndex(n)).to.be.revertedWith( | ||
'TokenIndexOutOfBounds' | ||
); | ||
}); | ||
|
||
describe('ownerships correctly set', async function () { | ||
it('with token before previously burnt token transfered and burned', async function () { | ||
const tokenIdToBurn = this.burnedTokenId - 1; | ||
await this.token.connect(this.addr1) | ||
.transferFrom(this.addr1.address, this.addr2.address, tokenIdToBurn); | ||
expect(await this.token.ownerOf(tokenIdToBurn)).to.be.equal(this.addr2.address); | ||
await this.token.connect(this.addr2).burn(tokenIdToBurn); | ||
for (let i = 0; i < this.numTestTokens; ++i) { | ||
if (i == tokenIdToBurn || i == this.burnedTokenId) { | ||
await expect(this.token.ownerOf(i)).to.be.revertedWith( | ||
'OwnerQueryForNonexistentToken' | ||
); | ||
} else { | ||
expect(await this.token.ownerOf(i)).to.be.equal(this.addr1.address); | ||
} | ||
} | ||
}); | ||
|
||
it('with token after previously burnt token transfered and burned', async function () { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you add a test to validate that transferring a burnt token gets reverted? |
||
const tokenIdToBurn = this.burnedTokenId + 1; | ||
await this.token.connect(this.addr1) | ||
.transferFrom(this.addr1.address, this.addr2.address, tokenIdToBurn); | ||
expect(await this.token.ownerOf(tokenIdToBurn)).to.be.equal(this.addr2.address); | ||
await this.token.connect(this.addr2).burn(tokenIdToBurn); | ||
for (let i = 0; i < this.numTestTokens; ++i) { | ||
if (i == tokenIdToBurn || i == this.burnedTokenId) { | ||
await expect(this.token.ownerOf(i)).to.be.revertedWith( | ||
'OwnerQueryForNonexistentToken' | ||
) | ||
} else { | ||
expect(await this.token.ownerOf(i)).to.be.equal(this.addr1.address); | ||
} | ||
} | ||
}); | ||
|
||
it('with first token burned', async function () { | ||
await this.token.connect(this.addr1).burn(0); | ||
for (let i = 0; i < this.numTestTokens; ++i) { | ||
if (i == 0 || i == this.burnedTokenId) { | ||
await expect(this.token.ownerOf(i)).to.be.revertedWith( | ||
'OwnerQueryForNonexistentToken' | ||
) | ||
} else { | ||
expect(await this.token.ownerOf(i)).to.be.equal(this.addr1.address); | ||
} | ||
} | ||
}); | ||
|
||
it('with last token burned', async function () { | ||
await expect(this.token.ownerOf(this.numTestTokens)).to.be.revertedWith( | ||
'OwnerQueryForNonexistentToken' | ||
) | ||
await this.token.connect(this.addr1).burn(this.numTestTokens - 1); | ||
await expect(this.token.ownerOf(this.numTestTokens - 1)).to.be.revertedWith( | ||
'OwnerQueryForNonexistentToken' | ||
) | ||
}); | ||
}); | ||
}); |
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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,45 @@ | ||
const { expect } = require('chai'); | ||
const { constants } = require('@openzeppelin/test-helpers'); | ||
const { ZERO_ADDRESS } = constants; | ||
|
||
describe('ERC721ABurnableOwnersExplicit', function () { | ||
beforeEach(async function () { | ||
this.ERC721ABurnableOwnersExplicit = await ethers.getContractFactory('ERC721ABurnableOwnersExplicitMock'); | ||
this.token = await this.ERC721ABurnableOwnersExplicit.deploy('Azuki', 'AZUKI'); | ||
await this.token.deployed(); | ||
}); | ||
|
||
beforeEach(async function () { | ||
const [owner, addr1, addr2, addr3] = await ethers.getSigners(); | ||
this.owner = owner; | ||
this.addr1 = addr1; | ||
this.addr2 = addr2; | ||
this.addr3 = addr3; | ||
await this.token['safeMint(address,uint256)'](addr1.address, 1); | ||
await this.token['safeMint(address,uint256)'](addr2.address, 2); | ||
await this.token['safeMint(address,uint256)'](addr3.address, 3); | ||
await this.token.connect(this.addr1).burn(0); | ||
await this.token.connect(this.addr3).burn(4); | ||
await this.token.setOwnersExplicit(6); | ||
}); | ||
|
||
it('ownerships correctly set', async function () { | ||
for (let tokenId = 0; tokenId < 6; tokenId++) { | ||
let owner = await this.token.getOwnershipAt(tokenId); | ||
expect(owner[0]).to.not.equal(ZERO_ADDRESS); | ||
if (tokenId == 0 || tokenId == 4) { | ||
expect(owner[2]).to.equal(true); | ||
await expect(this.token.ownerOf(tokenId)).to.be.revertedWith( | ||
'OwnerQueryForNonexistentToken' | ||
) | ||
} else { | ||
expect(owner[2]).to.equal(false); | ||
if (tokenId < 1+2) { | ||
expect(await this.token.ownerOf(tokenId)).to.be.equal(this.addr2.address); | ||
} else { | ||
expect(await this.token.ownerOf(tokenId)).to.be.equal(this.addr3.address); | ||
} | ||
} | ||
} | ||
}); | ||
}); |
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.
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.
Is there any reason that you check for this here instead of internal
_burn
function?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.
To be consistent with OpenZeppelin’s implementation.