Adding Mintable, RBACMintable and Burnable ERC721 tokens

[vittominacori]

🚀 Description

This add Mintable, RBACMintable and Burnable tokens to the ERC721 token lists.

Features:

• Added BurnableERC721Token

  • Only token owner can burn his own token

• Added MintableERC721Token

  • Only contract owner can mint new tokens
  • Only contract owner can call the finishMinting function

• Added RBACMintableERC721Token

  • Only address with minter role can mint new tokens
  • Only owner can add/remove minter
  • All the MintableERC721Token stuffs

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

[shrugs]

@vittominacori how do you think these two PRs should be combined?

#950

[shrugs]

nitpick: formatting should look like

constructor(string _name, string _symbol)
  ERC721Token(_name, _symbol)
  public
{
}

[canhlinh]

I think so.

[vittominacori]

@shrugs

Regarding my code, I just wanted to replicate what I've already done with ERC20 RBACMintableToken.
So make the token able to be mint by more than an "owner".
And to do so I created MintableERC721Token.sol, RBACMintableERC721Token.sol and the behaviours to test them.

Your code is a little different, you allow the ERC721Minter to mint new tokens by validating signature.

About your code:

I think RBACOwnable should have only an owner and it is unuseful as the removed RBACWithAdmin contract. What's the difference between these 2 contracts and why one is being removed and the other should be created? In addition using the onlyOwner modifier to check Role can be confusing or maybe have conflict if people also extends from Ownable.

Also RBACMintable is more specific than expected. I mean why we should include all the possibile cases in the RBAC implementation and not only when requested? So I prefer to have the onlyMinter implementation in Token because of there are no other cases when the RBACMintable can be used so, for me, make no sense to have it into the RBAC folder.

PRs could be merged but, guy, you are the reviewer so feel free to ask for changes in my code if it sounds repetitive or unuseful.

[shrugs]

I'd very much like to include your contribution in OZ!

The reason we have MintableToken and RBACMintableToken is primarily for backwards compatibility; RBAC allows for either 1 or many addresses with a permission, so it pretty much covers the primary use-case of Ownable. I'm not sure we'd need both MintableERC721Token and RBACMintableERC721Token, since RBACMintableERC721Token just as useful.

If you agree with that, we can rename RBACMintableERC721Token to MintableERC721Token and then I can work on rebasing #950 on top of this one.

re: RBACMintable I think it's good to keep the definition of roles and their access logic within //access. I don't mind the implementation of that logic being deferred to the implementer (like it is in https://github.com/OpenZeppelin/openzeppelin-solidity/pull/950/files#diff-9404d7caf069d2c48bf2e2867b83ea7fR37 ) because someone may have a different implementation need.

RBACWithAdmin is super powerful, so we've removed it from the main contracts; it was designed to be an example, regardless: #936

[vittominacori]

Ok I agree to rename RBAC into Mintable.
Everyone will be enforced to add a minter role after the token creation.
The ability to mint like an ownable token is being removed.

If you prefer also to have RBACMintable into access feel free to keep it. I wouldn't imagine that this will open the road to a lot of RBAC[Something] contracts into //access.

Thanks.

[shrugs]

The owner of a mintable token will be automatically set as the deployer because of the constructor in RBACOwnable, so I think it's ok for us not to automatically set the owner as the minter as well (although it would be good to mention that in the comments).

Let me know if you think it'd be easier to rebase this branch on #950 or #950 on this branch.

[vittominacori]

I think it could be better to merge #950 here.
Keep MintableERC721 from my code, then rename and add the RBACMintableERC721 stuffs and then add your changes.

The others files should not have conflicts.

Take a look to the added test behaviours and adapt to your token and minter contracts.

[canhlinh]

Awasome. I very much appreciate if you guys continue working to finish this this PR.

[shrugs]

hey @vittominacori any update on your end? If you're unavailable, I can spend time this weekend and merge our PRs together. You'll still be the author of the original commits :)

[vittominacori]

Hi @shrugs I'm really busy these days. So it could be better if you can start merging PRs.
Don't worry about authorship :)

Feel free to ask for changes, I will try to work on when I can.

[shrugs]

closing in favor of work on #1273, since this will need to use the new roles refactor branch.