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

Should setApprovalForAll throw when operator == msg.sender? #121

Closed
mg6maciej opened this issue Jun 29, 2018 · 2 comments
Closed

Should setApprovalForAll throw when operator == msg.sender? #121

mg6maciej opened this issue Jun 29, 2018 · 2 comments

Comments

@mg6maciej
Copy link
Contributor

No description provided.

@MoMannn
Copy link
Collaborator

MoMannn commented Jun 29, 2018

Actually I would answer this the same way I did #122 .

@mg6maciej
Copy link
Contributor Author

I'm not sure here. Looking at different implementations shows different additions to what's written in eip-721. You have require(_operator != address(0));, OZ has require(_operator != msg.sender); in setApprovalForAll; both have require(_approved != tokenOwner); in approve.

I think require(_to != address(0)); in transferFrom is the single best check to avoid problems with bugs in frontend and that's why it's even written in eip-721. Possibly other checks not specified there might be almost as important, like approving or transfering to this, i.e. ERC721 contract.
It's probably important to look at it using gas cost / potential user confusion formula. These kind of checks use under 10 gas, which is nothing compared to each 5000 (or sometimes 20000) storage update.

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