diff --git a/contracts/ownership/rbac/RBAC.sol b/contracts/ownership/rbac/RBAC.sol index 7f3d3b81e..4c175c65b 100644 --- a/contracts/ownership/rbac/RBAC.sol +++ b/contracts/ownership/rbac/RBAC.sol @@ -11,14 +11,16 @@ import './Roles.sol'; * See //contracts/examples/RBACExample.sol for an example of usage. * This RBAC method uses strings to key roles. It may be beneficial * for you to write your own implementation of this interface using Enums or similar. + * It's also recommended that you define constants in the contract, like ROLE_ADMIN below, + * to avoid typos. */ contract RBAC { using Roles for Roles.Role; mapping (string => Roles.Role) private roles; - event LogRoleAdded(address addr, string roleName); - event LogRoleRemoved(address addr, string roleName); + event RoleAdded(address addr, string roleName); + event RoleRemoved(address addr, string roleName); /** * A constant role name for indicating admins. @@ -43,7 +45,7 @@ contract RBAC { internal { roles[roleName].add(addr); - LogRoleAdded(addr, roleName); + RoleAdded(addr, roleName); } /** @@ -55,7 +57,7 @@ contract RBAC { internal { roles[roleName].remove(addr); - LogRoleRemoved(addr, roleName); + RoleRemoved(addr, roleName); } /** diff --git a/test/RBAC.js b/test/RBAC.test.js similarity index 77% rename from test/RBAC.js rename to test/RBAC.test.js index ef3866132..19f72e1f5 100644 --- a/test/RBAC.js +++ b/test/RBAC.test.js @@ -1,17 +1,21 @@ -const RBACMock = artifacts.require('./helpers/RBACMock.sol') +const RBACMock = artifacts.require('./mocks/RBACMock.sol') import expectThrow from './helpers/expectThrow' +import expectEvent from './helpers/expectEvent' require('chai') .use(require('chai-as-promised')) .should() +const ROLE_ADVISOR = 'advisor'; + contract('RBAC', function(accounts) { let mock const [ admin, anyone, + futureAdvisor, ...advisors ] = accounts @@ -60,9 +64,23 @@ contract('RBAC', function(accounts) { .should.be.fulfilled }) it('allows admins to #adminRemoveRole', async () => { - await mock.adminRemoveRole(advisors[3], 'advisor', { from: admin }) + await mock.adminRemoveRole(advisors[3], ROLE_ADVISOR, { from: admin }) .should.be.fulfilled }) + + it('announces a RoleAdded event on addRole', async () => { + expectEvent.inTransaction( + mock.adminAddRole(futureAdvisor, ROLE_ADVISOR, { from: admin }), + 'RoleAdded' + ) + }) + + it('announces a RoleRemoved event on removeRole', async () => { + expectEvent.inTransaction( + mock.adminRemoveRole(futureAdvisor, ROLE_ADVISOR, { from: admin }), + 'RoleRemoved' + ) + }) }) context('in adversarial conditions', () => { diff --git a/test/helpers/expectEvent.js b/test/helpers/expectEvent.js new file mode 100644 index 000000000..8a9502c36 --- /dev/null +++ b/test/helpers/expectEvent.js @@ -0,0 +1,16 @@ +const assert = require('chai').assert; + +const inLogs = async (logs, eventName) => { + const event = logs.find(e => e.event === eventName); + assert.exists(event); +}; + +const inTransaction = async (tx, eventName) => { + const { logs } = await tx; + return inLogs(logs, eventName); +}; + +module.exports = { + inLogs, + inTransaction, +}; diff --git a/test/helpers/RBACMock.sol b/test/mocks/RBACMock.sol similarity index 72% rename from test/helpers/RBACMock.sol rename to test/mocks/RBACMock.sol index aff1374d9..ae768a96a 100644 --- a/test/helpers/RBACMock.sol +++ b/test/mocks/RBACMock.sol @@ -5,11 +5,13 @@ import '../../contracts/ownership/rbac/RBAC.sol'; contract RBACMock is RBAC { + string constant ROLE_ADVISOR = "advisor"; + modifier onlyAdminOrAdvisor() { require( - hasRole(msg.sender, "admin") || - hasRole(msg.sender, "advisor") + hasRole(msg.sender, ROLE_ADMIN) || + hasRole(msg.sender, ROLE_ADVISOR) ); _; } @@ -17,23 +19,22 @@ contract RBACMock is RBAC { function RBACMock(address[] _advisors) public { - addRole(msg.sender, "admin"); - addRole(msg.sender, "advisor"); + addRole(msg.sender, ROLE_ADVISOR); for (uint256 i = 0; i < _advisors.length; i++) { - addRole(_advisors[i], "advisor"); + addRole(_advisors[i], ROLE_ADVISOR); } } function onlyAdminsCanDoThis() - onlyRole("admin") + onlyAdmin view external { } function onlyAdvisorsCanDoThis() - onlyRole("advisor") + onlyRole(ROLE_ADVISOR) view external { @@ -60,9 +61,9 @@ contract RBACMock is RBAC { { // revert if the user isn't an advisor // (perhaps you want to soft-fail here instead?) - checkRole(_addr, "advisor"); + checkRole(_addr, ROLE_ADVISOR); // remove the advisor's role - removeRole(_addr, "advisor"); + removeRole(_addr, ROLE_ADVISOR); } }