Implement feedback on Mint Access Control

tokens/contracts/access/MinterAccessControl.sol

@@ -10,10 +10,7 @@ contract MinterAccessControl is Initializable, OwnableUpgradeable {
     using EnumerableSetUpgradeable for EnumerableSetUpgradeable.AddressSet;
 
     EnumerableSetUpgradeable.AddressSet private _minters;
-    bool public minterAccessControlEnabled;
-
-    event MinterAccessControlEnabled();
-    event MinterAccessControlDisabled();
+
     event MinterGranted(address indexed account);
     event MinterRevoked(address indexed account);
 
@@ -25,25 +22,6 @@ contract MinterAccessControl is Initializable, OwnableUpgradeable {
     function __MinterAccessControl_init_unchained() internal initializer {
     }
 
-    /**
-     * @dev Enable minter control
-     * When enabled, only addresses added to `grantMinter` will be allowed to mint
-     */
-    function enableMinterAccessControl() external onlyOwner {
-        require(!minterAccessControlEnabled, "MinterAccessControl: Already enabled");
-        minterAccessControlEnabled = true;
-        emit MinterAccessControlEnabled();
-    }
-
-    /**
-     * @dev Disable minter control
-     */
-    function disableMinterAccessControl() external onlyOwner  {
-        require(minterAccessControlEnabled, "MinterAccessControl: Already disabled");
-        minterAccessControlEnabled = false;
-        emit MinterAccessControlDisabled();
-    }
-
     /**
      * @dev Add `_minter` to the list of allowed minters.
      */
@@ -63,10 +41,10 @@ contract MinterAccessControl is Initializable, OwnableUpgradeable {
     }
 
     /**
-     * @dev Returns `true` if minterControl is not enabled or `account` has been granted to minters.
+     * @dev Returns `true` if `account` has been granted to minters.
      */
     function isValidMinter(address account) public view returns (bool) {
-        return !minterAccessControlEnabled || _minters.contains(account);
+        return _minters.contains(account);
     }
 
     uint256[50] private __gap;

tokens/contracts/erc-1155/ERC1155Lazy.sol

@@ -9,9 +9,8 @@ import "@rarible/royalties-upgradeable/contracts/RoyaltiesV2Upgradeable.sol";
 import "@rarible/lazy-mint/contracts/erc-1155/IERC1155LazyMint.sol";
 import "./Mint1155Validator.sol";
 import "./ERC1155BaseURI.sol";
-import "../access/MinterAccessControl.sol";
 
-abstract contract ERC1155Lazy is IERC1155LazyMint, ERC1155BaseURI, Mint1155Validator, RoyaltiesV2Upgradeable, RoyaltiesV2Impl, MinterAccessControl  {
+abstract contract ERC1155Lazy is IERC1155LazyMint, ERC1155BaseURI, Mint1155Validator, RoyaltiesV2Upgradeable, RoyaltiesV2Impl {
     using SafeMathUpgradeable for uint;
 
     bytes4 private constant _INTERFACE_ID_ERC165 = 0x01ffc9a7;
@@ -60,7 +59,6 @@ abstract contract ERC1155Lazy is IERC1155LazyMint, ERC1155BaseURI, Mint1155Valid
         address sender = _msgSender();
 
         require(minter == sender || isApprovedForAll(minter, sender), "ERC1155: transfer caller is not approved");
-        require(isValidMinter(minter), "ERC1155: minter not granted");
         require(_amount > 0, "amount incorrect");
 
         if (supply[data.tokenId] == 0) {

tokens/contracts/erc-1155/ERC1155Rarible.sol

@@ -4,8 +4,9 @@ pragma solidity 0.7.6;
 pragma abicoder v2;
 
 import "./ERC1155Base.sol";
+import "../access/MinterAccessControl.sol";
 
-contract ERC1155Rarible is ERC1155Base {
+contract ERC1155Rarible is ERC1155Base, MinterAccessControl {
     /// @dev true if collection is private, false if public
     bool isPrivate;
 
@@ -50,7 +51,7 @@ contract ERC1155Rarible is ERC1155Base {
 
     function mintAndTransfer(LibERC1155LazyMint.Mint1155Data memory data, address to, uint256 _amount) public override {
         if (isPrivate){
-          require(owner() == data.creators[0].account, "minter is not the owner");
+          require(owner() == data.creators[0].account || isValidMinter(data.creators[0].account), "minter not granted or not owner");
         }
         super.mintAndTransfer(data, to, _amount);
     }

tokens/contracts/erc-721-minimal/ERC721LazyMinimal.sol

@@ -10,9 +10,8 @@ import "@rarible/lazy-mint/contracts/erc-721/IERC721LazyMint.sol";
 import "../Mint721Validator.sol";
 import "@openzeppelin/contracts-upgradeable/math/SafeMathUpgradeable.sol";
 import "./ERC721URI.sol";
-import "../access/MinterAccessControl.sol";
 
-abstract contract ERC721LazyMinimal is IERC721LazyMint, ERC721UpgradeableMinimal, Mint721Validator, RoyaltiesV2Upgradeable, RoyaltiesV2Impl, ERC721URI, MinterAccessControl {
+abstract contract ERC721LazyMinimal is IERC721LazyMint, ERC721UpgradeableMinimal, Mint721Validator, RoyaltiesV2Upgradeable, RoyaltiesV2Impl, ERC721URI {
     using SafeMathUpgradeable for uint;
 
     bytes4 private constant _INTERFACE_ID_ERC165 = 0x01ffc9a7;
@@ -55,7 +54,6 @@ abstract contract ERC721LazyMinimal is IERC721LazyMint, ERC721UpgradeableMinimal
         require(minter == data.creators[0].account, "tokenId incorrect");
         require(data.creators.length == data.signatures.length);
         require(minter == sender || isApprovedForAll(minter, sender), "ERC721: transfer caller is not owner nor approved");
-        require(isValidMinter(minter), "ERC721: minter not granted");
 
         bytes32 hash = LibERC721LazyMint.hash(data);
         for (uint i = 0; i < data.creators.length; i++) {

tokens/contracts/erc-721-minimal/ERC721RaribleMinimal.sol

@@ -4,8 +4,9 @@ pragma solidity 0.7.6;
 pragma abicoder v2;
 
 import "./ERC721BaseMinimal.sol";
+import "../access/MinterAccessControl.sol";
 
-contract ERC721RaribleMinimal is ERC721BaseMinimal {
+contract ERC721RaribleMinimal is ERC721BaseMinimal, MinterAccessControl {
     /// @dev true if collection is private, false if public
     bool isPrivate;
 
@@ -50,7 +51,7 @@ contract ERC721RaribleMinimal is ERC721BaseMinimal {
 
     function mintAndTransfer(LibERC721LazyMint.Mint721Data memory data, address to) public override virtual {
         if (isPrivate){
-            require(owner() == data.creators[0].account, "minter is not the owner");
+            require(owner() == data.creators[0].account || isValidMinter(data.creators[0].account), "minter not granted or not owner");
         }
         super.mintAndTransfer(data, to);
     }

tokens/contracts/erc-721/ERC721Lazy.sol

@@ -8,9 +8,8 @@ import "@rarible/royalties/contracts/impl/RoyaltiesV2Impl.sol";
 import "@rarible/royalties-upgradeable/contracts/RoyaltiesV2Upgradeable.sol";
 import "@rarible/lazy-mint/contracts/erc-721/IERC721LazyMint.sol";
 import "../Mint721Validator.sol";
-import "../access/MinterAccessControl.sol";
 
-abstract contract ERC721Lazy is IERC721LazyMint, ERC721Upgradeable, Mint721Validator, RoyaltiesV2Upgradeable, RoyaltiesV2Impl, MinterAccessControl {
+abstract contract ERC721Lazy is IERC721LazyMint, ERC721Upgradeable, Mint721Validator, RoyaltiesV2Upgradeable, RoyaltiesV2Impl {
     using SafeMathUpgradeable for uint;
     using EnumerableSetUpgradeable for EnumerableSetUpgradeable.UintSet;
     using EnumerableMapUpgradeable for EnumerableMapUpgradeable.UintToAddressMap;
@@ -55,7 +54,6 @@ abstract contract ERC721Lazy is IERC721LazyMint, ERC721Upgradeable, Mint721Valid
         require(minter == data.creators[0].account, "tokenId incorrect");
         require(data.creators.length == data.signatures.length);
         require(minter == sender || isApprovedForAll(minter, sender), "ERC721: transfer caller is not owner nor approved");
-        require(isValidMinter(minter), "ERC721: minter not granted");
 
         bytes32 hash = LibERC721LazyMint.hash(data);
         for (uint i = 0; i < data.creators.length; i++) {

tokens/contracts/erc-721/ERC721Rarible.sol

@@ -23,7 +23,6 @@ contract ERC721Rarible is ERC721Base {
         __Ownable_init_unchained();
         __ERC721Burnable_init_unchained();
         __Mint721Validator_init_unchained();
-        __MinterAccessControl_init_unchained();
         __HasContractURI_init_unchained(contractURI);
         __ERC721_init_unchained(_name, _symbol);
 

tokens/test/MinterAccessControl.test.js

@@ -0,0 +1,33 @@
+const TestingV1 = artifacts.require("MinterAccessControlTestV1.sol");
+const TestingV2 = artifacts.require("MinterAccessControlTestV2.sol");
+
+const { expectThrow } = require('@daonomic/tests-common');
+const { deployProxy, upgradeProxy } = require('@openzeppelin/truffle-upgrades');
+
+function creators(list) {
+  const value = 10000 / list.length
+  return list.map(account => ({ account, value }))
+}
+
+contract("MinterAccessControl", accounts => {
+  let token;
+  let tokenOwner = accounts[9];
+
+  beforeEach(async () => {
+    token = await deployProxy(TestingV1, [], { initializer: 'initialize' });
+    await token.transferOwnership(tokenOwner);
+  });
+
+  it("conserve minter access control after upgrade", async () => {
+    const minter = accounts[1];
+
+    await token.grantMinter(minter, {from: tokenOwner})
+    assert.equal(await token.isValidMinter(minter), true);
+
+    // upgrade contract
+    const newInstance = await upgradeProxy(token.address, TestingV2);
+    assert.equal(await newInstance.version(), 2);
+
+    assert.equal(await newInstance.isValidMinter(minter), true);
+  });
+});

tokens/test/contracts/access/MinterAccessControlTestV1.sol

@@ -0,0 +1,15 @@
+// SPDX-License-Identifier: MIT
+
+pragma solidity >=0.6.0 <0.8.0;
+
+import "../../../contracts/access/MinterAccessControl.sol";
+
+contract MinterAccessControlTestV1 is MinterAccessControl {
+
+    function initialize() external initializer {
+        __Ownable_init_unchained();
+        __MinterAccessControl_init_unchained();
+    }
+
+    uint256[50] private __gap;
+}

tokens/test/contracts/access/MinterAccessControlTestV2.sol

@@ -0,0 +1,19 @@
+// SPDX-License-Identifier: MIT
+
+pragma solidity >=0.6.0 <0.8.0;
+
+import "../../../contracts/access/MinterAccessControl.sol";
+
+contract MinterAccessControlTestV2 is MinterAccessControl {
+
+    function initialize() external initializer {
+        __Ownable_init_unchained();
+        __MinterAccessControl_init_unchained();
+    }
+
+    function version() public view returns (uint256) {
+        return 2;
+    }
+
+    uint256[50] private __gap;
+}

tokens/test/erc-1155/ERC1155Rarible.test.js

@@ -382,93 +382,6 @@ contract("ERC1155Rarible", accounts => {
     );
   });
 
-  it("mint and transfer with minter access control", async () => {
-    const minter = accounts[1];
-    let transferTo = accounts[2];
-
-    const tokenId = minter + "b00000000000000000000001";
-    const tokenURI = "//uri";
-    let supply = 5;
-    let mint = 2;
-
-    await token.enableMinterAccessControl({from: tokenOwner});
-    assert.equal(await token.minterAccessControlEnabled(), true);
-
-    await expectThrow(
-      token.mintAndTransfer([tokenId, tokenURI, supply, creators([minter]), [], [zeroWord]], transferTo, mint, {from: minter})
-    );
-
-    await token.grantMinter(minter, {from: tokenOwner});
-    assert.equal(await token.isValidMinter(minter), true);
-    assert.equal(await token.isValidMinter(transferTo), false);
-
-    await token.mintAndTransfer([tokenId, tokenURI, supply, creators([minter]), [], [zeroWord]], transferTo, mint, {from: minter});
-		assert.equal(await token.uri(tokenId), "ipfs:/" + tokenURI);
-    assert.equal(await token.balanceOf(transferTo, tokenId), mint);
-    assert.equal(await token.balanceOf(minter, tokenId), 0);
-  });
-
-  it("mint and transfer with minter access control and minter signature", async () => {
-    const minter = accounts[1];
-    let transferTo = accounts[2];
-
-    const tokenId = minter + "b00000000000000000000001";
-    const tokenURI = "//uri";
-    let supply = 5;
-    let mint = 2;
-
-    const signature = await getSignature(tokenId, tokenURI, supply, creators([minter]), [], minter);
-
-    let whiteListProxy = accounts[5];
-    await token.setDefaultApproval(whiteListProxy, true, {from: tokenOwner});
-
-    await token.enableMinterAccessControl({from: tokenOwner});
-    assert.equal(await token.minterAccessControlEnabled(), true);
-
-    await expectThrow(
-      token.mintAndTransfer([tokenId, tokenURI, supply, creators([minter]), [], [signature]], transferTo, mint, {from: whiteListProxy})
-    );
-
-    await token.grantMinter(minter, {from: tokenOwner});
-    assert.equal(await token.isValidMinter(minter), true);
-    assert.equal(await token.isValidMinter(whiteListProxy), false);
-
-    await token.mintAndTransfer([tokenId, tokenURI, supply, creators([minter]), [], [signature]], transferTo, mint, {from: whiteListProxy})
-		assert.equal(await token.uri(tokenId), "ipfs:/" + tokenURI);
-    assert.equal(await token.balanceOf(transferTo, tokenId), mint);
-    assert.equal(await token.balanceOf(minter, tokenId), 0);
-  });
-
-  it("mint and transfer with minter access control and wrong minter signature", async () => {
-    const minter = accounts[1];
-    let transferTo = accounts[2];
-
-    const tokenId = minter + "b00000000000000000000001";
-    const tokenURI = "//uri";
-    let supply = 5;
-    let mint = 2;
-
-    const signature = await getSignature(tokenId, tokenURI, supply, creators([minter]), [], transferTo);
-
-    let whiteListProxy = accounts[5];
-    await token.setDefaultApproval(whiteListProxy, true, {from: tokenOwner});
-
-    await token.enableMinterAccessControl({from: tokenOwner});
-    assert.equal(await token.minterAccessControlEnabled(), true);
-
-    await expectThrow(
-      token.mintAndTransfer([tokenId, tokenURI, supply, creators([minter]), [], [signature]], transferTo, mint, {from: whiteListProxy})
-    );
-
-    await token.grantMinter(minter, {from: tokenOwner});
-    assert.equal(await token.isValidMinter(minter), true);
-    assert.equal(await token.isValidMinter(whiteListProxy), false);
-
-    await expectThrow(
-      token.mintAndTransfer([tokenId, tokenURI, supply, creators([minter]), [], [signature]], transferTo, mint, {from: whiteListProxy})
-    );
-  });
-
   it("standard transfer from owner", async () => {
     let minter = accounts[1];
     const tokenId = minter + "b00000000000000000000001";