erc721 mintable and bouncer updates

[shrugs]

Supercedes #957 and #950

fixes #962

🚀 Description

cc @vittominacori for review

• DefaultTokenURI.sol — implement tokenURI(uint256 _tokenId) and returns a default tokenURI if a specific token URI is not set. saves the developer from storing the same tokenURI for every token

• RBACMintable.sol — a super simple mintable role with onlyMinter modifier. Does not suggest role authorization, which is left up to the implementer. Pairs well with RBACOwnable

• RBACOwnable — an Ownable implementation that allows multiple owners that can add/remove themselves

• MintableERC721Token.sol — an ERC721Token that can be minted by anyone with the minter role.

• ERC721Burnable — an interface to support burnFrom() mechanism

• BurnableERC721Token — an implementation of that interface

• 📘 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).

[vittominacori]

@shrugs pushed in your repo

shrugs#1

It should solve test failing.

[frangio]

I see a few problems with this PR.

It's very complex, and doesn't feel very cohesive. There's additions to our ERC721 stack, SignatureBouncer and RBAC, as well as new stuff like NonceTracker and AutoIncrementing. It's very hard to review such disparate components. The huge new context in which they are presented results in a lot of overhead. We should aim for more cohesive and as small as possible PRs. I believe this will lead to better reviews by making better use of our time and focus. (In the possible case that a huge feature has to be developed, it should probably be done in an ongoing in progress PR with incremental reviews from the same reviewer.)

Additionally, I'm not quite convinced that ERC721Minter should be a part of OpenZeppelin. Although SignatureBouncer is very interesting, we shouldn't start writing bouncers for every feature in the library. ERC721Minter is a fine example of how a developer might build a system by composing a MintableERC721Token with a SignatureBouncer. What we should aim for is designing, writing, and documenting building blocks that can be safely composed by developers with little code. ERC721Minter is a success in this sense: it is very little code doing something quite powerful built out of OpenZeppelin components, but it is not something that I would include in the library.

What do you think about this?

My concrete proposal is to continue work on #957, and then merge ERC721Minter only as an example (i.e. in the examples/ directory).

[shrugs]

@frangio Keeping ERC721Minter inside examples is good 👍

This PR is super huge, and it totally is all over the place, yeah. With the removal of ERC721Minter we can split noncetracker, autoincrementing, and signature bouncer changes into separate PRs. Will do that unless you have extra thoughts.

[shrugs]

@frangio thanks for the feedback, I feel a lot better about the PR now. split them into

#1022
#1023
#1024

[nventuro]

@shrugs should we close this one then, since work continued on those other 3 PRs?

[shrugs]

@nventuro this PR will have to eventually be rebased on master once those commits are merged

[shrugs]

closing in favor of tracking via #1272