Skip to content

Add the missing test for ERC721Holder #1249

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.

Sign up for GitHub

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

Merged
nventuro merged 4 commits into from
Sep 26, 2018
Merged

Add the missing test for ERC721Holder #1249

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
7279fc7
Add the missing test for ERC721Holder
Aug 29, 2018
1ccfba9
fix lint
Aug 29, 2018
9cb251d
Merge branch 'master' into test/ERC721Holder
Sep 26, 2018
9ade13a
Move the holder test to a separate file
Sep 26, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions test/token/ERC721/ERC721BasicToken.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
const { shouldBehaveLikeERC721BasicToken } = require('./ERC721BasicToken.behavior');
const { shouldBehaveLikeMintAndBurnERC721Token } = require('./ERC721MintBurn.behavior');
const { shouldBehaveLikeERC721Holder } = require('./ERC721Holder.behavior');

const BigNumber = web3.BigNumber;
const ERC721BasicToken = artifacts.require('ERC721BasicTokenMock.sol');
Expand All @@ -15,4 +16,5 @@ contract('ERC721BasicToken', function (accounts) {

shouldBehaveLikeERC721BasicToken(accounts);
shouldBehaveLikeMintAndBurnERC721Token(accounts);
shouldBehaveLikeERC721Holder(accounts);
});
25 changes: 25 additions & 0 deletions test/token/ERC721/ERC721Holder.behavior.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
const ERC721Holder = artifacts.require('ERC721Holder.sol');

require('chai')
.should();

function shouldBehaveLikeERC721Holder (accounts) {
Copy link
Contributor

@nventuro nventuro Aug 29, 2018

Choose a reason for hiding this comment

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

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.

Copy link
Author

@come-maiz come-maiz Aug 29, 2018

Choose a reason for hiding this comment

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

maybe I can rename it to: shouldWorkWithERC721Holder.

Copy link
Contributor

@nventuro nventuro Aug 29, 2018

Choose a reason for hiding this comment

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

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?

Copy link
Author

@come-maiz come-maiz Aug 29, 2018

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@nventuro nventuro Aug 29, 2018

Choose a reason for hiding this comment

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

My point was that it's not a token test, but a holder test - it belongs in a separate test file 😛

Copy link
Author

@come-maiz come-maiz Aug 29, 2018

Choose a reason for hiding this comment

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

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.

const tokenId = 1;
const creator = accounts[0];
Copy link
Contributor

@nventuro nventuro Aug 29, 2018

Choose a reason for hiding this comment

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

😱

Consider destructuring the accounts array (e.g. change the function declaration to function shouldBehaveLikeERC721Holder ([creator])), see #1137 for reference 😅


describe('like an ERC721Holder', function () {
it('safe transfers to a holder contract', async function () {
await this.token.mint(creator, tokenId, { from: creator });
const receiver = await ERC721Holder.new();
await this.token.approve(receiver.address, tokenId, { from: creator });

await this.token.safeTransferFrom(creator, receiver.address, tokenId);

(await this.token.ownerOf(tokenId)).should.be.equal(receiver.address);
});
});
}

module.exports = {
shouldBehaveLikeERC721Holder,
};