Add the missing test for ERC721Holder #1249
Conversation
|
|
||
| function shouldBehaveLikeERC721Holder (accounts) { | ||
| const tokenId = 1; | ||
| const creator = accounts[0]; |
There was a problem hiding this comment.
😱
Consider destructuring the accounts array (e.g. change the function declaration to function shouldBehaveLikeERC721Holder ([creator])), see #1137 for reference 😅
| require('chai') | ||
| .should(); | ||
|
|
||
| function shouldBehaveLikeERC721Holder (accounts) { |
There was a problem hiding this comment.
I'm not sure this name is accurate: the token isn't behaving like an ERC721 Holder, it is instead interacting with one.
I think we should have a test for the receiver itself, that calls the function and checks the return value, and one for the token, checking that safeTransferFrom works properly (i.e. rejects an invalid return value, accepts the correct one). @shrugs is the local ERC721 expert though, so I'll defer to his wisdom.
There was a problem hiding this comment.
maybe I can rename it to: shouldWorkWithERC721Holder.
There was a problem hiding this comment.
I checked the tests in some more detail: we're already testing that safeTransferFrom in the describe('via safeTransferFrom') block, what we need to test is that the ERC721Holder works with a compliant ERC721 token (i.e. it returns the correct value). Does that make sense?
There was a problem hiding this comment.
It does. But I think that the best way to test that the correct value is returned from the Holder contract is to check that safeTransferFrom succeeds. Just calling onReceived directly and verifying that a magic value is returned is too low level, IMO.
There was a problem hiding this comment.
My point was that it's not a token test, but a holder test - it belongs in a separate test file 😛
There was a problem hiding this comment.
Yes, that sounds right. I am just wondering if I should keep this style of tests/behavior, even if it's only one simple test.
|
@nventuro one more time, please. |
* Add the missing test for ERC721Holder * fix lint * Move the holder test to a separate file (cherry picked from commit 396680b)
This is one step towards #1147
🚀 Description
Add test coverage for ERC721Holder.
npm run lint:all:fix).