Roles.add and remove now require pre-conditions on the account. (#1421)

contracts/access/Roles.sol

@@ -14,6 +14,8 @@ library Roles {
    */
   function add(Role storage role, address account) internal {
     require(account != address(0));
+    require(!has(role, account));
+
     role.bearer[account] = true;
   }
 
@@ -22,6 +24,8 @@ library Roles {
    */
   function remove(Role storage role, address account) internal {
     require(account != address(0));
+    require(has(role, account));
+
     role.bearer[account] = false;
   }
 

test/access/Roles.test.js

@@ -29,10 +29,9 @@ contract('Roles', function ([_, authorized, otherAuthorized, anyone]) {
         (await this.roles.has(anyone)).should.equal(false);
       });
 
-      it('adds roles to an already-assigned account', async function () {
+      it('reverts when adding roles to an already assigned account', async function () {
         await this.roles.add(authorized);
-        await this.roles.add(authorized);
-        (await this.roles.has(authorized)).should.equal(true);
+        await shouldFail.reverting(this.roles.add(authorized));
       });
 
       it('reverts when adding roles to the null account', async function () {
@@ -54,8 +53,8 @@ contract('Roles', function ([_, authorized, otherAuthorized, anyone]) {
         (await this.roles.has(otherAuthorized)).should.equal(true);
       });
 
-      it('doesn\'t revert when removing unassigned roles', async function () {
-        await this.roles.remove(anyone);
+      it('reverts when removing unassigned roles', async function () {
+        await shouldFail.reverting(this.roles.remove(anyone));
       });
 
       it('reverts when removing roles from the null account', async function () {

test/access/roles/PublicRole.behavior.js

@@ -52,9 +52,8 @@ function shouldBehaveLikePublicRole (authorized, otherAuthorized, [anyone], role
         expectEvent.inLogs(logs, `${rolename}Added`, { account: anyone });
       });
 
-      it('adds role to an already-assigned account', async function () {
-        await this.contract[`add${rolename}`](authorized, { from: authorized });
-        (await this.contract[`is${rolename}`](authorized)).should.equal(true);
+      it('reverts when adding role to an already assigned account', async function () {
+        await shouldFail.reverting(this.contract[`add${rolename}`](authorized, { from: authorized }));
       });
 
       it('reverts when adding role to the null account', async function () {
@@ -74,8 +73,8 @@ function shouldBehaveLikePublicRole (authorized, otherAuthorized, [anyone], role
         expectEvent.inLogs(logs, `${rolename}Removed`, { account: authorized });
       });
 
-      it('doesn\'t revert when removing from an unassigned account', async function () {
-        await this.contract[`remove${rolename}`](anyone);
+      it('reverts when removing from an unassigned account', async function () {
+        await shouldFail.reverting(this.contract[`remove${rolename}`](anyone));
       });
 
       it('reverts when removing role from the null account', async function () {
@@ -94,8 +93,8 @@ function shouldBehaveLikePublicRole (authorized, otherAuthorized, [anyone], role
         expectEvent.inLogs(logs, `${rolename}Removed`, { account: authorized });
       });
 
-      it('doesn\'t revert when renouncing unassigned role', async function () {
-        await this.contract[`renounce${rolename}`]({ from: anyone });
+      it('reverts when renouncing unassigned role', async function () {
+        await shouldFail.reverting(this.contract[`renounce${rolename}`]({ from: anyone }));
       });
     });
   });