Improve encapsulation on SignatureBouncer, Whitelist and RBAC example (#1265)

* Improve encapsulation on Whitelist

* remove only

* update whitelisted crowdsale test

* Improve encapsulation on SignatureBouncer

* fix missing test

* Improve encapsulation on RBAC example

* Improve encapsulation on RBAC example

* Remove extra visibility

* Improve encapsulation on ERC20 Mintable

* Improve encapsulation on Superuser

* fix lint

* add missing constant

contracts/access/SignatureBouncer.sol

@@ -32,10 +32,13 @@ import "../cryptography/ECDSA.sol";
 contract SignatureBouncer is Ownable, RBAC {
   using ECDSA for bytes32;
 
-  string public constant ROLE_BOUNCER = "bouncer";
-  uint internal constant METHOD_ID_SIZE = 4;
-  // signature size is 65 bytes (tightly packed v + r + s), but gets padded to 96 bytes
-  uint internal constant SIGNATURE_SIZE = 96;
+  // Name of the bouncer role.
+  string private constant ROLE_BOUNCER = "bouncer";
+  // Function selectors are 4 bytes long, as documented in
+  // https://solidity.readthedocs.io/en/v0.4.24/abi-spec.html#function-selector
+  uint256 private constant METHOD_ID_SIZE = 4;
+  // Signature size is 65 bytes (tightly packed v + r + s), but gets padded to 96 bytes
+  uint256 private constant SIGNATURE_SIZE = 96;
 
   /**
    * @dev requires that a valid signature of a bouncer was provided
@@ -65,6 +68,14 @@ contract SignatureBouncer is Ownable, RBAC {
   }
 
   /**
+   * @dev Determine if an account has the bouncer role.
+   * @return true if the account is a bouncer, false otherwise.
+   */
+  function isBouncer(address _account) public view returns(bool) {
+    return hasRole(_account, ROLE_BOUNCER);
+  }
+
+  /**
    * @dev allows the owner to add additional bouncer addresses
    */
   function addBouncer(address _bouncer)
@@ -153,6 +164,6 @@ contract SignatureBouncer is Ownable, RBAC {
     address signer = _hash
       .toEthSignedMessageHash()
       .recover(_signature);
-    return hasRole(signer, ROLE_BOUNCER);
+    return isBouncer(signer);
   }
 }

contracts/access/Whitelist.sol

@@ -11,7 +11,9 @@ import "../access/rbac/RBAC.sol";
  * This simplifies the implementation of "user permissions".
  */
 contract Whitelist is Ownable, RBAC {
-  string public constant ROLE_WHITELISTED = "whitelist";
+
+  // Name of the whitelisted role.
+  string private constant ROLE_WHITELISTED = "whitelist";
 
   /**
    * @dev Throws if operator is not whitelisted.
@@ -35,9 +37,10 @@ contract Whitelist is Ownable, RBAC {
   }
 
   /**
-   * @dev getter to determine if address is in whitelist
+   * @dev Determine if an account is whitelisted.
+   * @return true if the account is whitelisted, false otherwise.
    */
-  function whitelist(address _operator)
+  function isWhitelisted(address _operator)
     public
     view
     returns (bool)

contracts/examples/RBACWithAdmin.sol

@@ -19,7 +19,7 @@ contract RBACWithAdmin is RBAC {
   /**
    * A constant role name for indicating admins.
    */
-  string public constant ROLE_ADMIN = "admin";
+  string private constant ROLE_ADMIN = "admin";
 
   /**
    * @dev modifier to scope access to admins
@@ -41,6 +41,13 @@ contract RBACWithAdmin is RBAC {
   }
 
   /**
+   * @return true if the account is admin, false otherwise.
+   */
+  function isAdmin(address _account) public view returns(bool) {
+    return hasRole(_account, ROLE_ADMIN);
+  }
+
+  /**
    * @dev add a role to an account
    * @param _account the account that will have the role
    * @param _roleName the name of the role

contracts/mocks/RBACMock.sol

@@ -10,7 +10,7 @@ contract RBACMock is RBACWithAdmin {
   modifier onlyAdminOrAdvisor()
   {
     require(
-      hasRole(msg.sender, ROLE_ADMIN) ||
+      isAdmin(msg.sender) ||
       hasRole(msg.sender, ROLE_ADVISOR)
     );
     _;

contracts/ownership/Superuser.sol

@@ -12,7 +12,7 @@ import "../access/rbac/RBAC.sol";
  * A superuser can transfer his role to a new address.
  */
 contract Superuser is Ownable, RBAC {
-  string public constant ROLE_SUPERUSER = "superuser";
+  string private constant ROLE_SUPERUSER = "superuser";
 
   constructor () public {
     _addRole(msg.sender, ROLE_SUPERUSER);

contracts/token/ERC20/RBACMintableToken.sol

@@ -13,7 +13,7 @@ contract RBACMintableToken is ERC20Mintable, RBAC {
   /**
    * A constant role name for indicating minters.
    */
-  string public constant ROLE_MINTER = "minter";
+  string private constant ROLE_MINTER = "minter";
 
   /**
    * @dev override the Mintable token modifier to add role based logic
@@ -24,6 +24,13 @@ contract RBACMintableToken is ERC20Mintable, RBAC {
   }
 
   /**
+   * @return true if the account is a minter, false otherwise.
+   */
+  function isMinter(address _account) public view returns(bool) {
+    return hasRole(_account, ROLE_MINTER);
+  }
+
+  /**
    * @dev add a minter role to an address
    * @param _minter address
    */

test/access/SignatureBouncer.test.js

@@ -16,7 +16,6 @@ const INVALID_SIGNATURE = '0xabcd';
 contract('Bouncer', function ([_, owner, anyone, bouncerAddress, authorizedUser]) {
   beforeEach(async function () {
     this.bouncer = await Bouncer.new({ from: owner });
-    this.roleBouncer = await this.bouncer.ROLE_BOUNCER();
   });
 
   context('management', function () {
@@ -26,7 +25,7 @@ contract('Bouncer', function ([_, owner, anyone, bouncerAddress, authorizedUser]
 
     it('allows the owner to add a bouncer', async function () {
       await this.bouncer.addBouncer(bouncerAddress, { from: owner });
-      (await this.bouncer.hasRole(bouncerAddress, this.roleBouncer)).should.equal(true);
+      (await this.bouncer.isBouncer(bouncerAddress)).should.equal(true);
     });
 
     it('does not allow adding an invalid address', async function () {
@@ -39,7 +38,7 @@ contract('Bouncer', function ([_, owner, anyone, bouncerAddress, authorizedUser]
       await this.bouncer.addBouncer(bouncerAddress, { from: owner });
 
       await this.bouncer.removeBouncer(bouncerAddress, { from: owner });
-      (await this.bouncer.hasRole(bouncerAddress, this.roleBouncer)).should.equal(false);
+      (await this.bouncer.isBouncer(bouncerAddress)).should.equal(false);
     });
 
     it('does not allow anyone to add a bouncer', async function () {

test/ownership/Whitelist.test.js → test/access/Whitelist.test.js

 const { expectThrow } = require('../helpers/expectThrow');
-const expectEvent = require('../helpers/expectEvent');
 
 const WhitelistMock = artifacts.require('WhitelistMock');
 
@@ -11,47 +10,30 @@ contract('Whitelist', function ([_, owner, whitelistedAddress1, whitelistedAddre
 
   beforeEach(async function () {
     this.mock = await WhitelistMock.new({ from: owner });
-    this.role = await this.mock.ROLE_WHITELISTED();
   });
 
   context('in normal conditions', function () {
     it('should add address to the whitelist', async function () {
-      await expectEvent.inTransaction(
-        this.mock.addAddressToWhitelist(whitelistedAddress1, { from: owner }),
-        'RoleAdded',
-        { role: this.role },
-      );
-      (await this.mock.whitelist(whitelistedAddress1)).should.equal(true);
+      await this.mock.addAddressToWhitelist(whitelistedAddress1, { from: owner });
+      (await this.mock.isWhitelisted(whitelistedAddress1)).should.equal(true);
     });
 
     it('should add addresses to the whitelist', async function () {
-      await expectEvent.inTransaction(
-        this.mock.addAddressesToWhitelist(whitelistedAddresses, { from: owner }),
-        'RoleAdded',
-        { role: this.role },
-      );
+      await this.mock.addAddressesToWhitelist(whitelistedAddresses, { from: owner });
       for (const addr of whitelistedAddresses) {
-        (await this.mock.whitelist(addr)).should.equal(true);
+        (await this.mock.isWhitelisted(addr)).should.equal(true);
       }
     });
 
     it('should remove address from the whitelist', async function () {
-      await expectEvent.inTransaction(
-        this.mock.removeAddressFromWhitelist(whitelistedAddress1, { from: owner }),
-        'RoleRemoved',
-        { role: this.role },
-      );
-      (await this.mock.whitelist(whitelistedAddress1)).should.equal(false);
+      await this.mock.removeAddressFromWhitelist(whitelistedAddress1, { from: owner });
+      (await this.mock.isWhitelisted(whitelistedAddress1)).should.equal(false);
     });
 
     it('should remove addresses from the the whitelist', async function () {
-      await expectEvent.inTransaction(
-        this.mock.removeAddressesFromWhitelist(whitelistedAddresses, { from: owner }),
-        'RoleRemoved',
-        { role: this.role },
-      );
+      await this.mock.removeAddressesFromWhitelist(whitelistedAddresses, { from: owner });
       for (const addr of whitelistedAddresses) {
-        (await this.mock.whitelist(addr)).should.equal(false);
+        (await this.mock.isWhitelisted(addr)).should.equal(false);
       }
     });
 

test/crowdsale/MintedCrowdsale.test.js

@@ -28,8 +28,6 @@ contract('MintedCrowdsale', function ([_, investor, wallet, purchaser]) {
   });
 
   describe('using RBACMintableToken', function () {
-    const ROLE_MINTER = 'minter';
-
     beforeEach(async function () {
       this.token = await RBACMintableToken.new();
       this.crowdsale = await MintedCrowdsale.new(rate, wallet, this.token.address);
@@ -37,7 +35,7 @@ contract('MintedCrowdsale', function ([_, investor, wallet, purchaser]) {
     });
 
     it('should have minter role on token', async function () {
-      (await this.token.hasRole(this.crowdsale.address, ROLE_MINTER)).should.equal(true);
+      (await this.token.isMinter(this.crowdsale.address)).should.equal(true);
     });
 
     shouldBehaveLikeMintedCrowdsale([_, investor, wallet, purchaser], rate, value);

test/crowdsale/WhitelistedCrowdsale.test.js

@@ -43,8 +43,8 @@ contract('WhitelistedCrowdsale', function ([_, wallet, authorized, unauthorized,
 
     describe('reporting whitelisted', function () {
       it('should correctly report whitelisted addresses', async function () {
-        (await this.crowdsale.whitelist(authorized)).should.equal(true);
-        (await this.crowdsale.whitelist(unauthorized)).should.equal(false);
+        (await this.crowdsale.isWhitelisted(authorized)).should.equal(true);
+        (await this.crowdsale.isWhitelisted(unauthorized)).should.equal(false);
       });
     });
   });
@@ -80,9 +80,9 @@ contract('WhitelistedCrowdsale', function ([_, wallet, authorized, unauthorized,
 
     describe('reporting whitelisted', function () {
       it('should correctly report whitelisted addresses', async function () {
-        (await this.crowdsale.whitelist(authorized)).should.equal(true);
-        (await this.crowdsale.whitelist(anotherAuthorized)).should.equal(true);
-        (await this.crowdsale.whitelist(unauthorized)).should.equal(false);
+        (await this.crowdsale.isWhitelisted(authorized)).should.equal(true);
+        (await this.crowdsale.isWhitelisted(anotherAuthorized)).should.equal(true);
+        (await this.crowdsale.isWhitelisted(unauthorized)).should.equal(false);
       });
     });
   });

test/token/ERC20/RBACMintableToken.behavior.js

 const { expectThrow } = require('../../helpers/expectThrow');
 
-const ROLE_MINTER = 'minter';
-
 function shouldBehaveLikeRBACMintableToken (owner, [anyone]) {
   describe('handle roles', function () {
     it('owner can add and remove a minter role', async function () {
       await this.token.addMinter(anyone, { from: owner });
-      (await this.token.hasRole(anyone, ROLE_MINTER)).should.equal(true);
+      (await this.token.isMinter(anyone)).should.equal(true);
 
       await this.token.removeMinter(anyone, { from: owner });
-      (await this.token.hasRole(anyone, ROLE_MINTER)).should.equal(false);
+      (await this.token.isMinter(anyone)).should.equal(false);
     });
 
     it('anyone can\'t add or remove a minter role', async function () {