This repository was archived by the owner on Oct 1, 2025. It is now read-only.
-
Notifications
You must be signed in to change notification settings - Fork 3
feat: added adr for burning or tterc721 token #26
Open
justussoh
wants to merge
3
commits into
master
Choose a base branch
from
feat/burning-erc721-tokens
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
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 hidden or 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 hidden or 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,74 @@ | ||||||
| # Burning TradeTrustERC721 Token | ||||||
|
|
||||||
| ## Status | ||||||
|
|
||||||
| Draft | ||||||
|
|
||||||
| ## Goal | ||||||
|
|
||||||
| ERC721 Burnable Token can be irreversibly burned (destroyed). The ERC721 implemented by OpenZeppelin allows this by transfering burn tokens to address(0) as shown below: | ||||||
|
|
||||||
| ```sol | ||||||
| function _burn(address owner, uint256 tokenId) internal { | ||||||
| require(ownerOf(tokenId) == owner, "ERC721: burn of token that is not own"); | ||||||
|
|
||||||
| _clearApproval(tokenId); | ||||||
|
|
||||||
| _ownedTokensCount[owner].decrement(); | ||||||
| _tokenOwner[tokenId] = address(0); | ||||||
|
|
||||||
| emit Transfer(owner, address(0), tokenId); | ||||||
| } | ||||||
| ``` | ||||||
|
|
||||||
| However, TradeTrust is still required to render and track burnt token. Since unminted tokens and destoryed token have the same owner address in the Erc721 Token Registry, we needed to find a way to differentiate a unminted and burnt token. This means that TradeTrust would have to modify the ERC721 implementation or modify its verifier method. | ||||||
|
|
||||||
| ## Options | ||||||
|
|
||||||
| After some discussion, 2 strategies stood out: | ||||||
|
|
||||||
| #### Continue using 0x0 as burn address | ||||||
|
|
||||||
| This strategy involve burning token normally. However, when verifying the token, the verifier would check if the document had previously been emitted a `Minted` event. | ||||||
|
|
||||||
| Pros: | ||||||
|
|
||||||
| - semantically correct because token is not owned by anyone anymore | ||||||
| - no need to inform consumers of a burn address | ||||||
|
|
||||||
| Cons: | ||||||
|
|
||||||
| - difficult to implement | ||||||
| - event emission might be lost in future | ||||||
| - state is not immediately available to be queried (as both unminted token and burnt tokens had `0x0` address, thus we need to query events) | ||||||
|
|
||||||
| #### using 0xknown as burn address (like [0x000000000000000000000000000000000000dead](https://etherscan.io/address/0x000000000000000000000000000000000000dead)) | ||||||
|
Contributor
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.
Suggested change
|
||||||
|
|
||||||
| This method instead would see that the token would be burn to `0xdead` address, a known burn address which no one has the keys for. Subsequently, in order to track the token, we will need to check the `ownerOf` token from the token registry. | ||||||
|
Contributor
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.
Suggested change
|
||||||
|
|
||||||
| Pros: | ||||||
|
|
||||||
| - easy to implement | ||||||
| - known dead address are available and labeled | ||||||
| - state is immediately available to be queried | ||||||
|
|
||||||
| Cons: | ||||||
|
|
||||||
| - burnt address a convention that needs to be propagated | ||||||
| - it's not possible to prove to another party that this address is uncontrolled | ||||||
|
|
||||||
| ## Implementation | ||||||
|
|
||||||
| In the end, a 0xknown burn address, `0x000000000000000000000000000000000000dead` was choosen. Since the logic of tracking token after burning is an app layer logic, there is no need to bloat the `TradeTrustERC721` smart contract with more methods. Also, scalability wise, if there were more intermediate state, other known burn address can be used to represent different states as opposed to more complex app layer logic on the verifier. | ||||||
|
|
||||||
| ## Review on Current State | ||||||
|
|
||||||
| For token-registry, a specificed function called `destroyToken` will be used to denote this function with the following implementaion. | ||||||
|
|
||||||
| ```sol | ||||||
| function destroyToken(uint256 _tokenId) public onlyMinter { | ||||||
| require(ownerOf(_tokenId) == address(this), "Cannot destroy token: Token not owned by token registry"); | ||||||
| _safeTransferFrom(ownerOf(_tokenId), 0x000000000000000000000000000000000000dEaD, _tokenId, ""); | ||||||
| emit TokenBurnt(_tokenId); | ||||||
| } | ||||||
| ``` | ||||||
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.