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

caller or token owner should be able to approve self as operator for … #407

Merged
merged 3 commits into from Aug 16, 2022

Conversation

jrkosinski
Copy link
Contributor

…own tokens.

Motivation:
As discussed, the EIP doesn't explicitly state that someone can't be their own operator for their own tokens. It has no effect per se, other than clearing any existing operator for the same tokens. An address being its own approver does no harm, and does not seem to be explicitly forbidden by the EIP.

approve(my_address, my_token_id) -> results in getApproved(my_token_id) returning the address of the owner. Now the token owner has all the same rights that they already had anyway.

Same for setApprovalForAll(my_address) -> the token owner is now the operator for all of its own tokens.

Changes

  • removed check for (operator == sender) in setApprovalForAll.
  • removed error ApproveToCaller (no longer being used)
  • modified unit test that was testing for that error, to check instead that the call is not reverted.
  • and then modified that same test to check that isApprovedForAll returns true for the token owner
  • added a new test in "setApprovalForAll" to check that similarly, an address can call approve to set itself as operator for its own tokens
  • added a further test that, once an address has called approve to set itself as operator, transferring will clear the approval (as it would for any other operator)

@Vectorized Vectorized linked an issue Aug 12, 2022 that may be closed by this pull request
Copy link
Collaborator

@Vectorized Vectorized left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:)

@Vectorized
Copy link
Collaborator

@jrkosinski Psst, can you help fix the linter issues for the test file?

@jrkosinski
Copy link
Contributor Author

jrkosinski commented Aug 12, 2022 via email

@Vectorized
Copy link
Collaborator

@cygaar LGTM, what do you think?

@cygaar cygaar merged commit 7fd5b84 into chiru-labs:main Aug 16, 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

Successfully merging this pull request may close these issues.

Self-approval allowed for approve
3 participants