ERC721 pausable token

[urvalla]

Fixes #1115

🚀 Description

ERC721PausableToken: ERC721BasicToken modified with pausable transfers.

• 📘 I've reviewed the OpenZeppelin Contributor Guidelines
• ✅ I've added tests where applicable to test my new functionality.
• 📖 I've made sure that my contracts are well-documented.
• 🎨 I've run the JS/Solidity linters and fixed any issues (npm run lint:all:fix).

[nventuro]

Great work @urvalla, thanks a lot! I left a couple comments, mainly regarding testing code style, for which admittedly we're still lacking documentation (sorry about that!).

[nventuro]

Please replace accounts with [_, owner] (see #1137 for reference)

[nventuro]

nit: remove this empty line

[nventuro]

nit: please add a newline character at the end of the file

[nventuro]

It's not on the testing code style guide yet, but please try to replicate the style shown in #1137: in these case, I'd have the behavior receive the owner account (which has been used to create the token in the calling test file), plus an array of other accounts that will be assigned to the different roles (i.e. recipient, operator). See the BurnableToken tests for reference.

[nventuro]

Considering changing the phrasing to 'is paused and then unpaused', to better reflect the sequential nature of this.

[nventuro]

Do we actually need to check this (in this and all other it blocks)? If the transaction reverted, the state wasn't changed, so I don't see why we'd perform these kinds of tests.

[nventuro]

Since we're only adding the pausable modifiers, couldn't we reuse the standard erc721 behavior and test against it when unpaused (see the ConditionalEscrow tests for a related example).

[urvalla]

Hello @nventuro, thank you for feedback!

I've made changes to pull request according to your comments.

Also, I've add checks for views, as their behavior shouldn't be changed while pause. Is it a good idea (in terms of #1148) to extract somthing like shouldBehaveLikeERC721BasicTokenViews and share it between ERC721 Basic & ERC721 Pausable test suites?

[urvalla]

Is there something that blocks merge?

[nventuro]

Sorry @urvalla - this PR must've slipped by! Thanks for letting me know. I'll review ASAP and get back to you :)

[nventuro]

Looking great @urvalla, thanks! Left a couple minor comments, mostly test-related.

Regarding your suggestion about ERC721 view methods, I think that's very interesting and worth exploring! I'd do it in a separate PR though (once this one is merged), to keep everything neat and tidy. I wonder how much repetition we'll be able to avoid, considering each test suite may require different values to be returned from the view methods.

[nventuro]

This looks a bit weird, since the test does not pause, it is the behavior that calls the pause function. Shouldn't we have the beforeEach here pausing the token?

[urvalla]

Should the behavior name be changed from shouldBehaveLikeERC721PausableToken to shouldBehaveLikeERC721PausedToken then?

Or maybe just leave 'when token is paused and then unpaused' context, and move out two first entries (as they don't have beforeEach'es).

[nventuro]

I'm not sure what you mean by 'moving out the two first entries' (which?), but the behavior renaming sounds like a neat idea!

[urvalla]

I mean just to remove two first context blocks:

shouldBehaveLikeERC721BasicToken(owner, [...otherAccounts]);
shouldBehaveLikeERC721PausableToken(owner, [...otherAccounts]);

context('when token is paused and then unpaused', function () {
   beforeEach(async function () {
      await this.token.pause({ from: owner });
      await this.token.unpause({ from: owner });
   });

   shouldBehaveLikeERC721BasicToken(owner, [...otherAccounts]);
});

But I'm not shure which option is more prefferable.

[nventuro]

I'd go with pausing outside, in a context, and renaming the behavior: in that second example you're testing both behaviors at the same time, as if they were the same thing (you could, in fact, test again for shouldBehaveLikeERC721PausableToken after pausing and unpausing :) )

[nventuro]

The behavior function should receive special accounts as their own argument, and then an array of all other accounts for the behavior to use as it pleases.

Therefore, this call should look like shouldBehaveLikeBlahBlah(owner, otherAccounts) (since recipient and operator have no special meaning here), and the behavior itself should look like shouldBehaveLikeBlahBlah(owner, [recipient, operator]).

(same for the other behavior call :))

[nventuro]

Shouldn't this read when trying to safeTransferFromWithData?

[nventuro]

Is this 1 coming from the fact that only one token was minted? Could we add a comment, or abstract it away to a constant (e.g. mintedTokens)?

[nventuro]

Do we actually need this helper? If not, I'd remove it entirely

[urvalla]

Do you mean to move helper's body to test (as it is not reused anywhere), or there is more simple way to call safeTransferFrom with extra arguments?

[nventuro]

Yes, sorry - I meant move it inside the test body. AFAIK truffle doesn't support overloading yet, even for different arities :/

[urvalla]

Thank you, @nventuro!

I've left shouldBehaveLikeERC721BasicToken(owner, ...otherAccounts) call, but not shouldBehaveLikeERC721BasicToken(owner, [...otherAccounts]) due to its interface. I'll fix it in a separate pull request.

[nventuro]

Awesome, thanks for keeping the PRs clean and concise :) Looking forward to that PR!

[nventuro]

Great work @urvalla, thank you very much for your contribution! We haven't seen many flavors of ERC721 come up, this is a nice addition.