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

ERC721 - Restricted zero address transfer issue in regards to how opensea treats dead addresses #3244

Closed
kanedaaaa opened this issue Mar 6, 2022 · 3 comments

Comments

@kanedaaaa
Copy link
Contributor

kanedaaaa commented Mar 6, 2022

In the last year or so, I had to work with many ERC721 projects and in almost every single one, I have to tackle the absence of a standard way of burning tokens, which is sending to black hole (0x00...000) address.

I'm pretty sure everyone is aware of this ongoing issue (from my POV it's a huge issue, will explain why below), but still, in ERC721.sol (Same goes for ERC20), zero address transfers are restricted in internal _transfer function (the only way to transfer assets):

require(to != address(0), "ERC721: transfer to the zero address");

I know, I know someone already made an issue about this years ago, that was closed and I decided to open a new one, just to make the old responses clear, exactly why do we avoid removing it, and making it optional? I also know that there is an extension, I also know that developers can expose internal _burn into public function, but why exactly? It's one line VS. extensions and function. (Not arguing here, just trying to understand exactly what is the reason)

Yes, I'm aware that 0x0 isn't only burning address, but the issue comes when we see how Opensea, currently N1 NFT marketplace doesn't recognize other burn addresses, as a black hole (0x0) and won't remove items from the collection page (neither from stats) if it's sent to those mentioned addresses.

Over time, I just keep telling myself that devs will learn, they will eventually start implementing burn functions or using extensions to cover that functionality, but it just keeps popping out.

Again, I don't want to create a huge problem from something simple like this, but since Opensea won't listen to us, I'm sure I can have a chance to make this point here.

P.S Can we at least outline the fact (somewhere in contract, or documentation) that devs must implement burn function for future use?

@kanedaaaa kanedaaaa changed the title ERC721 - Enable zero address transfers ERC721 - Restricted zero address transfer issue in regards to how opensea treats dead addresses Mar 6, 2022
@frangio
Copy link
Contributor

frangio commented Mar 8, 2022

Can you explain the importance of burning for NFTs? Why would we recommend that devs "must" implement a burn function? The way we see it burning is a feature that may or may not be relevant for a particular NFT, and a dev should make this choice when they are developing the contract. We don't include a burn function by default because it is not part of ERC721.

One of the things we could very easily do is enable "Burnable" by default in our ERC721 Wizard. A change like this to the contracts is going to require a lot more discussion.


I know someone already made an issue about this years ago

Can you share the link to this previous discussion?

@kanedaaaa
Copy link
Contributor Author

kanedaaaa commented Mar 8, 2022

Lately, it has become a trend, and usually marketing tool to offer some advantages, in exchange for the burn. And in general, due to the problem I described (Opensea), it's impossible (Not we can implement later, but literally impossible) to burn the tokens properly. Something that comes in need for many projects.

I do agree Must is an exaggerated term to use, I would prefer to call it Consider to add a given feature, due to the reasons mentioned above. For example, adding such a comment in Wizard. Since usually it is being used by less experienced people who do not realize the importance of such add-ons.

For example, pausable modification has this kind of explanation:
Privileged accounts will be able to pause the functionality marked as whenNotPaused. Useful for emergency response.

Would be nice to add that useful comment as well, such as Useful for future use-cases. (Not the best one, but you get the point)

Again, not asking for a change, just notice or comment, that will be sufficient. No need to alter the original implementation just because Opensea won't fix their stuff properly. BUT if it comes to modifying OG implementation, my vote would definitely go to removing require(to != address(0)) (Which i'm not asking for, since I understood that it poses some security risks from old discussion)

BTW, here is the old discussion about ERC20 burning

@frangio
Copy link
Contributor

frangio commented Dec 2, 2022

Closing due to unclear actionable. If there is a concrete suggestion for documentation please submit a pull request.

@frangio frangio closed this as completed Dec 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants