From f4eb51a7e978365dbf0237062f014ffb261ac406 Mon Sep 17 00:00:00 2001 From: Leo Arias Date: Mon, 3 Sep 2018 16:06:43 -0600 Subject: [PATCH] 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 | 21 +++++++++--- contracts/access/Whitelist.sol | 9 +++-- contracts/examples/RBACWithAdmin.sol | 9 ++++- contracts/mocks/RBACMock.sol | 2 +- contracts/ownership/Superuser.sol | 2 +- contracts/token/ERC20/RBACMintableToken.sol | 9 ++++- test/access/SignatureBouncer.test.js | 5 ++- test/{ownership => access}/Whitelist.test.js | 34 +++++-------------- test/crowdsale/MintedCrowdsale.test.js | 4 +-- test/crowdsale/WhitelistedCrowdsale.test.js | 10 +++--- .../token/ERC20/RBACMintableToken.behavior.js | 6 ++-- 11 files changed, 58 insertions(+), 53 deletions(-) rename test/{ownership => access}/Whitelist.test.js (62%) diff --git a/contracts/access/SignatureBouncer.sol b/contracts/access/SignatureBouncer.sol index 03ad5cca4..0b91dcdb3 100644 --- a/contracts/access/SignatureBouncer.sol +++ b/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 @@ -64,6 +67,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 */ @@ -153,6 +164,6 @@ contract SignatureBouncer is Ownable, RBAC { address signer = _hash .toEthSignedMessageHash() .recover(_signature); - return hasRole(signer, ROLE_BOUNCER); + return isBouncer(signer); } } diff --git a/contracts/access/Whitelist.sol b/contracts/access/Whitelist.sol index 40a2f5663..3d9b3da5c 100644 --- a/contracts/access/Whitelist.sol +++ b/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) diff --git a/contracts/examples/RBACWithAdmin.sol b/contracts/examples/RBACWithAdmin.sol index ca2d5ed31..83c2dcef7 100644 --- a/contracts/examples/RBACWithAdmin.sol +++ b/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 @@ -40,6 +40,13 @@ contract RBACWithAdmin is RBAC { _addRole(msg.sender, ROLE_ADMIN); } + /** + * @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 diff --git a/contracts/mocks/RBACMock.sol b/contracts/mocks/RBACMock.sol index dea62d8f8..f4acc9279 100644 --- a/contracts/mocks/RBACMock.sol +++ b/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) ); _; diff --git a/contracts/ownership/Superuser.sol b/contracts/ownership/Superuser.sol index afacc1934..d8d081d03 100644 --- a/contracts/ownership/Superuser.sol +++ b/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); diff --git a/contracts/token/ERC20/RBACMintableToken.sol b/contracts/token/ERC20/RBACMintableToken.sol index 7a50c69f0..ad6ab19ca 100644 --- a/contracts/token/ERC20/RBACMintableToken.sol +++ b/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 @@ -23,6 +23,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 diff --git a/test/access/SignatureBouncer.test.js b/test/access/SignatureBouncer.test.js index 20bc85427..798ab0ac9 100644 --- a/test/access/SignatureBouncer.test.js +++ b/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 () { diff --git a/test/ownership/Whitelist.test.js b/test/access/Whitelist.test.js similarity index 62% rename from test/ownership/Whitelist.test.js rename to test/access/Whitelist.test.js index 411978d6e..f1113ffc3 100644 --- a/test/ownership/Whitelist.test.js +++ b/test/access/Whitelist.test.js @@ -1,5 +1,4 @@ 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); } }); diff --git a/test/crowdsale/MintedCrowdsale.test.js b/test/crowdsale/MintedCrowdsale.test.js index 27f64e6ed..50a37eaef 100644 --- a/test/crowdsale/MintedCrowdsale.test.js +++ b/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); diff --git a/test/crowdsale/WhitelistedCrowdsale.test.js b/test/crowdsale/WhitelistedCrowdsale.test.js index a20af2846..3cf9b3206 100644 --- a/test/crowdsale/WhitelistedCrowdsale.test.js +++ b/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); }); }); }); diff --git a/test/token/ERC20/RBACMintableToken.behavior.js b/test/token/ERC20/RBACMintableToken.behavior.js index 57325728b..8906c58db 100644 --- a/test/token/ERC20/RBACMintableToken.behavior.js +++ b/test/token/ERC20/RBACMintableToken.behavior.js @@ -1,15 +1,13 @@ 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 () {