diff --git a/contracts/access/Roles.sol b/contracts/access/Roles.sol index 930a62066..e3b97ddb7 100644 --- a/contracts/access/Roles.sol +++ b/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; } diff --git a/test/access/Roles.test.js b/test/access/Roles.test.js index 869217776..4a3c7b09f 100644 --- a/test/access/Roles.test.js +++ b/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 () { diff --git a/test/access/roles/PublicRole.behavior.js b/test/access/roles/PublicRole.behavior.js index 57b94cf97..0ae8a19e6 100644 --- a/test/access/roles/PublicRole.behavior.js +++ b/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 })); }); }); });