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

🔒 Add Only owner Can Burn the ERC-721 Token #39

Closed
wants to merge 1 commit into from

Conversation

jaglinux
Copy link
Contributor

In current implementation, token id owner or approved owner can burn the respective token.
Limit the burn function to token id owner only.

Signed-off-by: Jagadish Krishnamoorthy jagdish.krishna@gmail.com

PR Checklist

  • Because this PR includes a bug fix, relevant tests have been included.
  • Because this PR includes a new feature, the change was previously discussed in an issue or in the discussions section.
  • I didn't do anything of this.

In current implementation, token id owner or approved owner can burn
the respective token.
Limit the burn function to token id owner only.

Signed-off-by: Jagadish Krishnamoorthy <jagdish.krishna@gmail.com>
@jaglinux
Copy link
Contributor Author

IMHO, burn feature should be restricted to owner of token id only.
Submitted PR to fix this. (maybe should have opened issue first ?)

@jaglinux
Copy link
Contributor Author

Should have rephrased it better. Who controls the right to burn should be left as an option for implementor/ package user to decide.
Given that inheritance methods are not supported, I am not sure of the approach to be taken.

@pcaversaccio pcaversaccio added the feature 💥 New feature or request label Dec 29, 2022
@pcaversaccio pcaversaccio self-assigned this Dec 29, 2022
@pcaversaccio
Copy link
Owner

pcaversaccio commented Dec 29, 2022

@jaglinux Thanks for this PR. However, I do not agree that this should be implemented because of the following reasoning:

  • The EIP-721 specification does not include how NFTs should be burned. So it's up to the author(s) how to implement it.
  • To your point "Who controls the right to burn should be left as an option for the implementor/ package user to decide." -> If you have given an allowance to another user, one can simply use safeTransferFrom/transferFrom & burn (given your implementation) which is the equivalent of what I implemented directly with my burn function. It does not offer any additional security features that are not already existent in the current code base.
  • My Vyper code is on purpose written in a composable way - if one desires to implement your feature, the complete logic in the internal function _burn can still be reused and just the external function burn can be adjusted. This will be very powerful once Vyper will support import via modules (VIP: Library Modules vyperlang/vyper#2431).

@jaglinux
Copy link
Contributor Author

@pcaversaccio Agree with your explanation wrt "import via modules" approach.

@pcaversaccio pcaversaccio changed the title ERC721: only owner can burn the token 🔒 Add Only owner Can Burn the ERC-721 Token Feb 1, 2023
@pcaversaccio pcaversaccio added the optimisation ⚡️ Code optimisations (e.g. gas improvements) label Feb 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature 💥 New feature or request optimisation ⚡️ Code optimisations (e.g. gas improvements)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants