Improved Roles API. (#1280)

* Improved Roles API.

* fix linter error

contracts/access/rbac/MinterRole.sol

@@ -8,29 +8,25 @@ contract MinterRole {
 
   Roles.Role private minters;
 
-  constructor(address[] _minters) public {
-    minters.addMany(_minters);
+  constructor() public {
+    minters.add(msg.sender);
   }
 
-  function transferMinter(address _account) public {
-    minters.transfer(_account);
-  }
-
-  function renounceMinter() public {
-    minters.renounce();
+  modifier onlyMinter() {
+    require(isMinter(msg.sender));
+    _;
   }
 
   function isMinter(address _account) public view returns (bool) {
     return minters.has(_account);
   }
 
-  modifier onlyMinter() {
-    require(isMinter(msg.sender));
-    _;
+  function addMinter(address _account) public onlyMinter {
+    minters.add(_account);
   }
 
-  function _addMinter(address _account) internal {
-    minters.add(_account);
+  function renounceMinter() public {
+    minters.remove(msg.sender);
   }
 
   function _removeMinter(address _account) internal {

contracts/access/rbac/Roles.sol

@@ -19,35 +19,13 @@ library Roles {
     _role.bearer[_account] = true;
   }
 
-  /**
-   * @dev give multiple accounts access to this role
-   */
-  function addMany(Role storage _role, address[] _accounts) internal {
-    for (uint256 i = 0; i < _accounts.length; ++i) {
-      add(_role, _accounts[i]);
-    }
-  }
-
   /**
    * @dev remove an account's access to this role
    */
   function remove(Role storage _role, address _account) internal {
     _role.bearer[_account] = false;
   }
 
-  function transfer(Role storage _role, address _account) internal {
-    require(_account != address(0));
-    require(!has(_role, _account));
-    require(has(_role, msg.sender));
-
-    remove(_role, msg.sender);
-    add(_role, _account);
-  }
-
-  function renounce(Role storage _role) internal {
-    remove(_role, msg.sender);
-  }
-
   /**
    * @dev check if an account has this role
    * @return bool

contracts/examples/SampleCrowdsale.sol

@@ -16,12 +16,6 @@ contract SampleCrowdsaleToken is ERC20Mintable {
   string public constant name = "Sample Crowdsale Token";
   string public constant symbol = "SCT";
   uint8 public constant decimals = 18;
-
-  constructor(address[] _minters)
-    ERC20Mintable(_minters)
-    public
-  {
-  }
 }
 
 

contracts/mocks/ERC20MintableMock.sol

@@ -5,13 +5,6 @@ import "../token/ERC20/ERC20Mintable.sol";
 
 // Mock contract exposing internal methods
 contract ERC20MintableMock is ERC20Mintable {
-  constructor(address[] minters) ERC20Mintable(minters) public {
-  }
-
-  function addMinter(address _account) public {
-    _addMinter(_account);
-  }
-
   function removeMinter(address _account) public {
     _removeMinter(_account);
   }

contracts/mocks/RolesMock.sol

@@ -12,22 +12,10 @@ contract RolesMock {
     dummyRole.add(_account);
   }
 
-  function addMany(address[] _accounts) public {
-    dummyRole.addMany(_accounts);
-  }
-
   function remove(address _account) public {
     dummyRole.remove(_account);
   }
 
-  function renounce() public {
-    dummyRole.renounce();
-  }
-
-  function transfer(address _account) public {
-    dummyRole.transfer(_account);
-  }
-
   function has(address _account) public view returns (bool) {
     return dummyRole.has(_account);
   }

contracts/token/ERC20/ERC20Capped.sol

@@ -11,9 +11,8 @@ contract ERC20Capped is ERC20Mintable {
 
   uint256 public cap;
 
-  constructor(uint256 _cap, address[] _minters)
+  constructor(uint256 _cap)
     public
-    ERC20Mintable(_minters)
   {
     require(_cap > 0);
     cap = _cap;

contracts/token/ERC20/ERC20Mintable.sol

@@ -15,12 +15,6 @@ contract ERC20Mintable is ERC20, MinterRole {
 
   bool public mintingFinished = false;
 
-  constructor(address[] _minters)
-    MinterRole(_minters)
-    public
-  {
-  }
-
   modifier canMint() {
     require(!mintingFinished);
     _;

test/access/rbac/PublicRole.behavior.js

@@ -1,5 +1,3 @@
-const { assertRevert } = require('../../helpers/assertRevert');
-
 require('chai')
   .should();
 
@@ -20,17 +18,17 @@ function shouldBehaveLikePublicRole (authorized, otherAuthorized, [anyone], role
 
     describe('add', function () {
       it('adds role to a new account', async function () {
-        await this.contract[`add${rolename}`](anyone);
+        await this.contract[`add${rolename}`](anyone, { from: authorized });
         (await this.contract[`is${rolename}`](anyone)).should.equal(true);
       });
 
       it('adds role to an already-assigned account', async function () {
-        await this.contract[`add${rolename}`](authorized);
+        await this.contract[`add${rolename}`](authorized, { from: authorized });
         (await this.contract[`is${rolename}`](authorized)).should.equal(true);
       });
 
       it('doesn\'t revert when adding role to the null account', async function () {
-        await this.contract[`add${rolename}`](ZERO_ADDRESS);
+        await this.contract[`add${rolename}`](ZERO_ADDRESS, { from: authorized });
       });
     });
 
@@ -50,34 +48,6 @@ function shouldBehaveLikePublicRole (authorized, otherAuthorized, [anyone], role
       });
     });
 
-    describe('transfering', function () {
-      context('from account with role', function () {
-        const from = authorized;
-
-        it('transfers to other account without the role', async function () {
-          await this.contract[`transfer${rolename}`](anyone, { from });
-          (await this.contract[`is${rolename}`](anyone)).should.equal(true);
-          (await this.contract[`is${rolename}`](authorized)).should.equal(false);
-        });
-
-        it('reverts when transfering to an account with role', async function () {
-          await assertRevert(this.contract[`transfer${rolename}`](otherAuthorized, { from }));
-        });
-
-        it('reverts when transfering to the null account', async function () {
-          await assertRevert(this.contract[`transfer${rolename}`](ZERO_ADDRESS, { from }));
-        });
-      });
-
-      context('from account without role', function () {
-        const from = anyone;
-
-        it('reverts', async function () {
-          await assertRevert(this.contract[`transfer${rolename}`](anyone, { from }));
-        });
-      });
-    });
-
     describe('renouncing roles', function () {
       it('renounces an assigned role', async function () {
         await this.contract[`renounce${rolename}`]({ from: authorized });

test/access/rbac/Roles.test.js

@@ -1,5 +1,3 @@
-const { assertRevert } = require('../../helpers/assertRevert');
-
 const RolesMock = artifacts.require('RolesMock');
 
 require('chai')
@@ -32,17 +30,6 @@ contract('Roles', function ([_, authorized, otherAuthorized, anyone]) {
         (await this.roles.has(authorized)).should.equal(true);
       });
 
-      it('adds roles to multiple accounts', async function () {
-        await this.roles.addMany([authorized, otherAuthorized]);
-        (await this.roles.has(authorized)).should.equal(true);
-        (await this.roles.has(otherAuthorized)).should.equal(true);
-      });
-
-      it('adds roles to multiple identical accounts', async function () {
-        await this.roles.addMany([authorized, authorized]);
-        (await this.roles.has(authorized)).should.equal(true);
-      });
-
       it('doesn\'t revert when adding roles to the null account', async function () {
         await this.roles.add(ZERO_ADDRESS);
       });
@@ -51,7 +38,8 @@ contract('Roles', function ([_, authorized, otherAuthorized, anyone]) {
 
   context('with added roles', function () {
     beforeEach(async function () {
-      await this.roles.addMany([authorized, otherAuthorized]);
+      await this.roles.add(authorized);
+      await this.roles.add(otherAuthorized);
     });
 
     describe('removing roles', function () {
@@ -69,44 +57,5 @@ contract('Roles', function ([_, authorized, otherAuthorized, anyone]) {
         await this.roles.remove(ZERO_ADDRESS);
       });
     });
-
-    describe('transfering roles', function () {
-      context('from account with role', function () {
-        const from = authorized;
-
-        it('transfers to other account with no role', async function () {
-          await this.roles.transfer(anyone, { from });
-          (await this.roles.has(anyone)).should.equal(true);
-          (await this.roles.has(authorized)).should.equal(false);
-        });
-
-        it('reverts when transfering to an account with role', async function () {
-          await assertRevert(this.roles.transfer(otherAuthorized, { from }));
-        });
-
-        it('reverts when transfering to the null account', async function () {
-          await assertRevert(this.roles.transfer(ZERO_ADDRESS, { from }));
-        });
-      });
-
-      context('from account without role', function () {
-        const from = anyone;
-
-        it('reverts', async function () {
-          await assertRevert(this.roles.transfer(anyone, { from }));
-        });
-      });
-    });
-
-    describe('renouncing roles', function () {
-      it('renounces an assigned role', async function () {
-        await this.roles.renounce({ from: authorized });
-        (await this.roles.has(authorized)).should.equal(false);
-      });
-
-      it('doesn\'t revert when renouncing unassigned role', async function () {
-        await this.roles.renounce({ from: anyone });
-      });
-    });
   });
 });

test/crowdsale/MintedCrowdsale.test.js

@@ -8,15 +8,17 @@ const MintedCrowdsale = artifacts.require('MintedCrowdsaleImpl');
 const ERC20Mintable = artifacts.require('ERC20Mintable');
 const ERC20 = artifacts.require('ERC20');
 
-contract('MintedCrowdsale', function ([_, initialMinter, investor, wallet, purchaser]) {
+contract('MintedCrowdsale', function ([_, deployer, investor, wallet, purchaser]) {
   const rate = new BigNumber(1000);
   const value = ether(5);
 
   describe('using ERC20Mintable', function () {
     beforeEach(async function () {
-      this.token = await ERC20Mintable.new([initialMinter]);
+      this.token = await ERC20Mintable.new({ from: deployer });
       this.crowdsale = await MintedCrowdsale.new(rate, wallet, this.token.address);
-      await this.token.transferMinter(this.crowdsale.address, { from: initialMinter });
+
+      await this.token.addMinter(this.crowdsale.address, { from: deployer });
+      await this.token.renounceMinter({ from: deployer });
     });
 
     it('crowdsale should be minter', async function () {

test/examples/SampleCrowdsale.test.js

@@ -16,7 +16,7 @@ const should = require('chai')
 const SampleCrowdsale = artifacts.require('SampleCrowdsale');
 const SampleCrowdsaleToken = artifacts.require('SampleCrowdsaleToken');
 
-contract('SampleCrowdsale', function ([_, initialMinter, owner, wallet, investor]) {
+contract('SampleCrowdsale', function ([_, deployer, owner, wallet, investor]) {
   const RATE = new BigNumber(10);
   const GOAL = ether(10);
   const CAP = ether(20);
@@ -31,12 +31,14 @@ contract('SampleCrowdsale', function ([_, initialMinter, owner, wallet, investor
     this.closingTime = this.openingTime + duration.weeks(1);
     this.afterClosingTime = this.closingTime + duration.seconds(1);
 
-    this.token = await SampleCrowdsaleToken.new([initialMinter]);
+    this.token = await SampleCrowdsaleToken.new({ from: deployer });
     this.crowdsale = await SampleCrowdsale.new(
       this.openingTime, this.closingTime, RATE, wallet, CAP, this.token.address, GOAL,
       { from: owner }
     );
-    await this.token.transferMinter(this.crowdsale.address, { from: initialMinter });
+
+    await this.token.addMinter(this.crowdsale.address, { from: deployer });
+    await this.token.renounceMinter({ from: deployer });
   });
 
   it('should create crowdsale with correct parameters', async function () {

test/token/ERC20/ERC20Capped.test.js

@@ -10,13 +10,13 @@ contract('ERC20Capped', function ([_, minter, ...otherAccounts]) {
 
   it('requires a non-zero cap', async function () {
     await assertRevert(
-      ERC20Capped.new(0, [minter])
+      ERC20Capped.new(0, { from: minter })
     );
   });
 
   context('once deployed', async function () {
     beforeEach(async function () {
-      this.token = await ERC20Capped.new(cap, [minter]);
+      this.token = await ERC20Capped.new(cap, { from: minter });
     });
 
     shouldBehaveLikeERC20Capped(minter, otherAccounts, cap);

test/token/ERC20/ERC20Mintable.test.js

@@ -2,21 +2,21 @@ const { shouldBehaveLikeERC20Mintable } = require('./ERC20Mintable.behavior');
 const { shouldBehaveLikePublicRole } = require('../../access/rbac/PublicRole.behavior');
 const ERC20MintableMock = artifacts.require('ERC20MintableMock');
 
-contract('ERC20Mintable', function ([_, originalMinter, otherMinter, ...otherAccounts]) {
+contract('ERC20Mintable', function ([_, minter, otherMinter, ...otherAccounts]) {
   beforeEach(async function () {
-    this.token = await ERC20MintableMock.new([originalMinter, otherMinter]);
+    this.token = await ERC20MintableMock.new({ from: minter });
   });
 
   context('with original minter', function () {
-    shouldBehaveLikeERC20Mintable(originalMinter, otherAccounts);
+    shouldBehaveLikeERC20Mintable(minter, otherAccounts);
   });
 
   describe('minter role', function () {
     beforeEach(async function () {
-      await this.token.addMinter(otherMinter);
+      await this.token.addMinter(otherMinter, { from: minter });
       this.contract = this.token;
     });
 
-    shouldBehaveLikePublicRole(originalMinter, otherMinter, otherAccounts, 'minter');
+    shouldBehaveLikePublicRole(minter, otherMinter, otherAccounts, 'minter');
   });
 });

test/token/ERC20/TokenTimelock.test.js

@@ -16,7 +16,7 @@ contract('TokenTimelock', function ([_, minter, beneficiary]) {
 
   context('with token', function () {
     beforeEach(async function () {
-      this.token = await ERC20Mintable.new([minter]);
+      this.token = await ERC20Mintable.new({ from: minter });
     });
 
     it('rejects a release time in the past', async function () {

test/token/ERC20/TokenVesting.test.js

@@ -44,7 +44,7 @@ contract('TokenVesting', function ([_, owner, beneficiary, minter]) {
     beforeEach(async function () {
       this.vesting = await TokenVesting.new(beneficiary, this.start, this.cliff, this.duration, true, { from: owner });
 
-      this.token = await ERC20Mintable.new([minter]);
+      this.token = await ERC20Mintable.new({ from: minter });
       await this.token.mint(this.vesting.address, amount, { from: minter });
     });