Add ERC721 hooks

[ericnordelo]

Partially fixes #965

PR Checklist

• Tests
• Documentation
• Added entry to CHANGELOG.md
• Tried the feature on a public network

[andrew-fleming]

The changes look great, Eric! I left some comments, questions, and suggestions

[andrew-fleming]

Hm the requirement isn't correct, it should be "operator cannot be the owner".

Also, should this be a requirement? I don't see anything against self approvals in the EIP and the current Solidity implementation only has a check that the operator isn't the zero address (here). On top of that, the proposed _approve_with_optional_event already allows for self approvals (here). Thoughts?

[ericnordelo]

After giving it some time, I think we can remove the requirement.

[ericnordelo]

And we don't need the check for the operator != zero either, since the zero address is always unauthorized in the check_authorized method.

[andrew-fleming]

Right, but this would allow is_approved_for_all to potentially return true when the operator is zero, no?

[ericnordelo]

Yes, but I don't think there's a real issue with that. Even if people approve the zero address, if for some reason the zero address approval causes issues, there must be a big bug somewhere in the process, right?

[andrew-fleming]

Even if people approve the zero address, if for some reason the zero address approval causes issues, there must be a big bug somewhere in the process, right?

Totally agree. My uneasiness with removing the check is the fact that it allows the getter to return an incorrect value. To me, that's a bug (a very small one in this context but a bug nonetheless). Do you disagree?

[ericnordelo]

Fair point. I think keeping it as consistent as possible with Solidity is good anyway. Let's keep the operator not zero check.

[andrew-fleming]

Changes look good! I think we should also add tests for the new internals (_require_owned, _check_authorized, _update). And we should also test the behavior of hooks. Since ERC20 should have hooks tests as well, maybe we can create a separate issue/PR to handle hooks testing (and not further block this PR). WDYT?

[ericnordelo]

I added the tests for the internal methods that were missing, but not sure what's the best approach to test hooks, since the only implementation we provide is empty, and how can we test an empty behavior? If we were to test that functions don't do things that's infinite possibilities right? Any ideas?

[andrew-fleming]

not sure what's the best approach to test hooks, since the only implementation we provide is empty, and how can we test an empty behavior? If we were to test that functions don't do things that's infinite possibilities right? Any ideas?

We must test ALL possibilities!!!

No, there's no point in testing empty behaviors. What we can test is the side effects of the before and after hook with a mock to confirm they're called in the correct order i.e. before_update is called before _update. A Cairo mocking library would be super useful :(

Unless you have another idea, I say let's get this merged and regroup on this. WDYT?

[andrew-fleming]

I left a couple tiny suggestions on the new tests. We just need to conclude on the open discussion and this should be good to go!

[andrew-fleming]

Last suggestion: let's add the non-zero operator requirement for _set_approval_for_all in the API doc and then this should be good to go!