rbac mintable erc721 token with signature bouncer

[shrugs]

I wrote all of these contracts to support mintable ERC721 using signature bouncer signatures.

• AutoIncrementing.sol — just keeps track of an ID and when you access it with nextId() it increments it on demand
• ERC721Minter.sol — a signature bouncer that lets you mint yourself a target MintablerERC721Token if you give it a valid signature (see tests for details, but a bouncer psigns the hash of contract.address + sender + tokenId + tokenURI
• NonceTracker — a simple method of tracking nonces per-address and imposing a maximum number of requests for a resource. Pairs well with a signature bouncer to make sure an address can only use the contract a single time
• 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.

fixes #962

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

[frangio]

Overall I think this is interesting. Given that the functionality is a bit more complex than what we're used to for OpenZeppelin, I'm wondering if it was motivated by a real need/use case?

I'm not sure about including the AutoIncrementing variant. Once a user has a signature, they can use it to mint many times. Is this a bug or is it by design? If by design, it feels like it duplicates the RBACMintable functionality, becase giving someone a signature is like making them a minter (minus being able to choose an id). Again, if this was motivated by a real use case I think that would help deciding whether we want to include it or not.

I don't see NonceTracker being used in the ERC721 parts. What is its purpose in this context? I would consider leaving it out of this PR.

[frangio]

We should make this private so that derived contracts cannot access/modify directly.

[frangio]

Nit: lowercase c.

[frangio]

The _sig argument is found in too many different argument positions. It's the first argument in mint, the second one in isValidMintSignature, and the last one in isValidSignature. It would be nice to make it consistent.

What do you think about making it always the last argument?

[shrugs]

Confirmed in #973 that it should always be last. Good catch.

Also this PR will be updated to use #973 when it's merged.

[frangio]

I'd prefer a more relevant name here. I can only think of _beneficiary but there may be better ones...

[shrugs]

@frangio

AutoIncrementing is designed for NFTs where the number isn't really important. The spec doesn't suggest or imply that token ids will be contiguous, but I expect most will want to be. By default this token minter AutoIncrements the token id, but one could easily override that and, combined with #973 allow the user to input the token id as an argument.

NonceTracker is actually what's stopping a user from using the ERC721Minter multiple times with the same signature; it only lets an address call the function one time. Once we check data arguments with #973 we'll be able to allow users to provide their own nonce (which could open up opportunities for a bouncer to issue a signature and then, for whatever reason, cancel it by increasing a user's nonce past the one they've been given and signed).