search

OpenZeppelin / cairo-contracts

OpenZeppelin Contracts written in Cairo for Starknet, a decentralized ZK Rollup
https://docs.openzeppelin.com/contracts-cairo
MIT License
797 stars 320 forks source link

Add ERC721 hooks #978

Closed ericnordelo closed 1 month ago

ericnordelo commented 2 months ago

Partially fixes #965

PR Checklist

ericnordelo commented 1 month ago

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 commented 1 month ago

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?