From 3ff550e5e5160c0a1481c5b48dcf46e904cbc238 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nicol=C3=A1s=20Venturo?= Date: Wed, 5 Sep 2018 16:56:14 -0300 Subject: [PATCH 1/2] Improved Roles API. --- contracts/access/rbac/MinterRole.sol | 22 ++++------ contracts/access/rbac/Roles.sol | 22 ---------- contracts/examples/SampleCrowdsale.sol | 6 --- contracts/mocks/ERC20MintableMock.sol | 7 ---- contracts/mocks/RolesMock.sol | 12 ------ contracts/token/ERC20/ERC20Capped.sol | 3 +- contracts/token/ERC20/ERC20Mintable.sol | 6 --- test/access/rbac/PublicRole.behavior.js | 36 ++-------------- test/access/rbac/Roles.test.js | 55 +------------------------ test/crowdsale/MintedCrowdsale.test.js | 8 ++-- test/examples/SampleCrowdsale.test.js | 8 ++-- test/token/ERC20/ERC20Capped.test.js | 4 +- test/token/ERC20/ERC20Mintable.test.js | 10 ++--- test/token/ERC20/TokenTimelock.test.js | 2 +- test/token/ERC20/TokenVesting.test.js | 2 +- 15 files changed, 34 insertions(+), 169 deletions(-) diff --git a/contracts/access/rbac/MinterRole.sol b/contracts/access/rbac/MinterRole.sol index b01a41de6c5..889efd0d24a 100644 --- a/contracts/access/rbac/MinterRole.sol +++ b/contracts/access/rbac/MinterRole.sol @@ -8,29 +8,25 @@ contract MinterRole { Roles.Role private minters; - constructor(address[] _minters) public { - minters.addMany(_minters); + constructor() public { + minters.add(msg.sender); } - function transferMinter(address _account) public { - minters.transfer(_account); - } - - function renounceMinter() public { - minters.renounce(); + modifier onlyMinter() { + require(isMinter(msg.sender)); + _; } function isMinter(address _account) public view returns (bool) { return minters.has(_account); } - modifier onlyMinter() { - require(isMinter(msg.sender)); - _; + function addMinter(address _account) onlyMinter public { + minters.add(_account); } - function _addMinter(address _account) internal { - minters.add(_account); + function renounceMinter() public { + minters.remove(msg.sender); } function _removeMinter(address _account) internal { diff --git a/contracts/access/rbac/Roles.sol b/contracts/access/rbac/Roles.sol index f93187e6bd3..e3d9f867bf1 100644 --- a/contracts/access/rbac/Roles.sol +++ b/contracts/access/rbac/Roles.sol @@ -19,15 +19,6 @@ library Roles { _role.bearer[_account] = true; } - /** - * @dev give multiple accounts access to this role - */ - function addMany(Role storage _role, address[] _accounts) internal { - for (uint256 i = 0; i < _accounts.length; ++i) { - add(_role, _accounts[i]); - } - } - /** * @dev remove an account's access to this role */ @@ -35,19 +26,6 @@ library Roles { _role.bearer[_account] = false; } - function transfer(Role storage _role, address _account) internal { - require(_account != address(0)); - require(!has(_role, _account)); - require(has(_role, msg.sender)); - - remove(_role, msg.sender); - add(_role, _account); - } - - function renounce(Role storage _role) internal { - remove(_role, msg.sender); - } - /** * @dev check if an account has this role * @return bool diff --git a/contracts/examples/SampleCrowdsale.sol b/contracts/examples/SampleCrowdsale.sol index 61b82dc27f2..5773f5d8646 100644 --- a/contracts/examples/SampleCrowdsale.sol +++ b/contracts/examples/SampleCrowdsale.sol @@ -16,12 +16,6 @@ contract SampleCrowdsaleToken is ERC20Mintable { string public constant name = "Sample Crowdsale Token"; string public constant symbol = "SCT"; uint8 public constant decimals = 18; - - constructor(address[] _minters) - ERC20Mintable(_minters) - public - { - } } diff --git a/contracts/mocks/ERC20MintableMock.sol b/contracts/mocks/ERC20MintableMock.sol index 6ec365bc660..39345081f4d 100644 --- a/contracts/mocks/ERC20MintableMock.sol +++ b/contracts/mocks/ERC20MintableMock.sol @@ -5,13 +5,6 @@ import "../token/ERC20/ERC20Mintable.sol"; // Mock contract exposing internal methods contract ERC20MintableMock is ERC20Mintable { - constructor(address[] minters) ERC20Mintable(minters) public { - } - - function addMinter(address _account) public { - _addMinter(_account); - } - function removeMinter(address _account) public { _removeMinter(_account); } diff --git a/contracts/mocks/RolesMock.sol b/contracts/mocks/RolesMock.sol index 05cbbeaf2f5..22e14312652 100644 --- a/contracts/mocks/RolesMock.sol +++ b/contracts/mocks/RolesMock.sol @@ -12,22 +12,10 @@ contract RolesMock { dummyRole.add(_account); } - function addMany(address[] _accounts) public { - dummyRole.addMany(_accounts); - } - function remove(address _account) public { dummyRole.remove(_account); } - function renounce() public { - dummyRole.renounce(); - } - - function transfer(address _account) public { - dummyRole.transfer(_account); - } - function has(address _account) public view returns (bool) { return dummyRole.has(_account); } diff --git a/contracts/token/ERC20/ERC20Capped.sol b/contracts/token/ERC20/ERC20Capped.sol index a21a9107a62..289b6585ca9 100644 --- a/contracts/token/ERC20/ERC20Capped.sol +++ b/contracts/token/ERC20/ERC20Capped.sol @@ -11,9 +11,8 @@ contract ERC20Capped is ERC20Mintable { uint256 public cap; - constructor(uint256 _cap, address[] _minters) + constructor(uint256 _cap) public - ERC20Mintable(_minters) { require(_cap > 0); cap = _cap; diff --git a/contracts/token/ERC20/ERC20Mintable.sol b/contracts/token/ERC20/ERC20Mintable.sol index b746aa91783..ae9019b36dc 100644 --- a/contracts/token/ERC20/ERC20Mintable.sol +++ b/contracts/token/ERC20/ERC20Mintable.sol @@ -15,12 +15,6 @@ contract ERC20Mintable is ERC20, MinterRole { bool public mintingFinished = false; - constructor(address[] _minters) - MinterRole(_minters) - public - { - } - modifier canMint() { require(!mintingFinished); _; diff --git a/test/access/rbac/PublicRole.behavior.js b/test/access/rbac/PublicRole.behavior.js index 74012fe41b1..80d78e917b5 100644 --- a/test/access/rbac/PublicRole.behavior.js +++ b/test/access/rbac/PublicRole.behavior.js @@ -1,5 +1,3 @@ -const { assertRevert } = require('../../helpers/assertRevert'); - require('chai') .should(); @@ -20,17 +18,17 @@ function shouldBehaveLikePublicRole (authorized, otherAuthorized, [anyone], role describe('add', function () { it('adds role to a new account', async function () { - await this.contract[`add${rolename}`](anyone); + await this.contract[`add${rolename}`](anyone, { from: authorized }); (await this.contract[`is${rolename}`](anyone)).should.equal(true); }); it('adds role to an already-assigned account', async function () { - await this.contract[`add${rolename}`](authorized); + await this.contract[`add${rolename}`](authorized, { from: authorized }); (await this.contract[`is${rolename}`](authorized)).should.equal(true); }); it('doesn\'t revert when adding role to the null account', async function () { - await this.contract[`add${rolename}`](ZERO_ADDRESS); + await this.contract[`add${rolename}`](ZERO_ADDRESS, { from: authorized }); }); }); @@ -50,34 +48,6 @@ function shouldBehaveLikePublicRole (authorized, otherAuthorized, [anyone], role }); }); - describe('transfering', function () { - context('from account with role', function () { - const from = authorized; - - it('transfers to other account without the role', async function () { - await this.contract[`transfer${rolename}`](anyone, { from }); - (await this.contract[`is${rolename}`](anyone)).should.equal(true); - (await this.contract[`is${rolename}`](authorized)).should.equal(false); - }); - - it('reverts when transfering to an account with role', async function () { - await assertRevert(this.contract[`transfer${rolename}`](otherAuthorized, { from })); - }); - - it('reverts when transfering to the null account', async function () { - await assertRevert(this.contract[`transfer${rolename}`](ZERO_ADDRESS, { from })); - }); - }); - - context('from account without role', function () { - const from = anyone; - - it('reverts', async function () { - await assertRevert(this.contract[`transfer${rolename}`](anyone, { from })); - }); - }); - }); - describe('renouncing roles', function () { it('renounces an assigned role', async function () { await this.contract[`renounce${rolename}`]({ from: authorized }); diff --git a/test/access/rbac/Roles.test.js b/test/access/rbac/Roles.test.js index 57e932692e7..be13be072d1 100644 --- a/test/access/rbac/Roles.test.js +++ b/test/access/rbac/Roles.test.js @@ -1,5 +1,3 @@ -const { assertRevert } = require('../../helpers/assertRevert'); - const RolesMock = artifacts.require('RolesMock'); require('chai') @@ -32,17 +30,6 @@ contract('Roles', function ([_, authorized, otherAuthorized, anyone]) { (await this.roles.has(authorized)).should.equal(true); }); - it('adds roles to multiple accounts', async function () { - await this.roles.addMany([authorized, otherAuthorized]); - (await this.roles.has(authorized)).should.equal(true); - (await this.roles.has(otherAuthorized)).should.equal(true); - }); - - it('adds roles to multiple identical accounts', async function () { - await this.roles.addMany([authorized, authorized]); - (await this.roles.has(authorized)).should.equal(true); - }); - it('doesn\'t revert when adding roles to the null account', async function () { await this.roles.add(ZERO_ADDRESS); }); @@ -51,7 +38,8 @@ contract('Roles', function ([_, authorized, otherAuthorized, anyone]) { context('with added roles', function () { beforeEach(async function () { - await this.roles.addMany([authorized, otherAuthorized]); + await this.roles.add(authorized); + await this.roles.add(otherAuthorized); }); describe('removing roles', function () { @@ -69,44 +57,5 @@ contract('Roles', function ([_, authorized, otherAuthorized, anyone]) { await this.roles.remove(ZERO_ADDRESS); }); }); - - describe('transfering roles', function () { - context('from account with role', function () { - const from = authorized; - - it('transfers to other account with no role', async function () { - await this.roles.transfer(anyone, { from }); - (await this.roles.has(anyone)).should.equal(true); - (await this.roles.has(authorized)).should.equal(false); - }); - - it('reverts when transfering to an account with role', async function () { - await assertRevert(this.roles.transfer(otherAuthorized, { from })); - }); - - it('reverts when transfering to the null account', async function () { - await assertRevert(this.roles.transfer(ZERO_ADDRESS, { from })); - }); - }); - - context('from account without role', function () { - const from = anyone; - - it('reverts', async function () { - await assertRevert(this.roles.transfer(anyone, { from })); - }); - }); - }); - - describe('renouncing roles', function () { - it('renounces an assigned role', async function () { - await this.roles.renounce({ from: authorized }); - (await this.roles.has(authorized)).should.equal(false); - }); - - it('doesn\'t revert when renouncing unassigned role', async function () { - await this.roles.renounce({ from: anyone }); - }); - }); }); }); diff --git a/test/crowdsale/MintedCrowdsale.test.js b/test/crowdsale/MintedCrowdsale.test.js index 408300e3426..e254ddf1b93 100644 --- a/test/crowdsale/MintedCrowdsale.test.js +++ b/test/crowdsale/MintedCrowdsale.test.js @@ -8,15 +8,17 @@ const MintedCrowdsale = artifacts.require('MintedCrowdsaleImpl'); const ERC20Mintable = artifacts.require('ERC20Mintable'); const ERC20 = artifacts.require('ERC20'); -contract('MintedCrowdsale', function ([_, initialMinter, investor, wallet, purchaser]) { +contract('MintedCrowdsale', function ([_, deployer, investor, wallet, purchaser]) { const rate = new BigNumber(1000); const value = ether(5); describe('using ERC20Mintable', function () { beforeEach(async function () { - this.token = await ERC20Mintable.new([initialMinter]); + this.token = await ERC20Mintable.new({ from: deployer }); this.crowdsale = await MintedCrowdsale.new(rate, wallet, this.token.address); - await this.token.transferMinter(this.crowdsale.address, { from: initialMinter }); + + await this.token.addMinter(this.crowdsale.address, { from: deployer }); + await this.token.renounceMinter({ from: deployer }); }); it('crowdsale should be minter', async function () { diff --git a/test/examples/SampleCrowdsale.test.js b/test/examples/SampleCrowdsale.test.js index 6e9706d49d4..9dd48ba9553 100644 --- a/test/examples/SampleCrowdsale.test.js +++ b/test/examples/SampleCrowdsale.test.js @@ -16,7 +16,7 @@ const should = require('chai') const SampleCrowdsale = artifacts.require('SampleCrowdsale'); const SampleCrowdsaleToken = artifacts.require('SampleCrowdsaleToken'); -contract('SampleCrowdsale', function ([_, initialMinter, owner, wallet, investor]) { +contract('SampleCrowdsale', function ([_, deployer, owner, wallet, investor]) { const RATE = new BigNumber(10); const GOAL = ether(10); const CAP = ether(20); @@ -31,12 +31,14 @@ contract('SampleCrowdsale', function ([_, initialMinter, owner, wallet, investor this.closingTime = this.openingTime + duration.weeks(1); this.afterClosingTime = this.closingTime + duration.seconds(1); - this.token = await SampleCrowdsaleToken.new([initialMinter]); + this.token = await SampleCrowdsaleToken.new({ from: deployer }); this.crowdsale = await SampleCrowdsale.new( this.openingTime, this.closingTime, RATE, wallet, CAP, this.token.address, GOAL, { from: owner } ); - await this.token.transferMinter(this.crowdsale.address, { from: initialMinter }); + + await this.token.addMinter(this.crowdsale.address, { from: deployer }); + await this.token.renounceMinter({ from: deployer }); }); it('should create crowdsale with correct parameters', async function () { diff --git a/test/token/ERC20/ERC20Capped.test.js b/test/token/ERC20/ERC20Capped.test.js index ccbd920a2e6..7e0c35c13c6 100644 --- a/test/token/ERC20/ERC20Capped.test.js +++ b/test/token/ERC20/ERC20Capped.test.js @@ -10,13 +10,13 @@ contract('ERC20Capped', function ([_, minter, ...otherAccounts]) { it('requires a non-zero cap', async function () { await assertRevert( - ERC20Capped.new(0, [minter]) + ERC20Capped.new(0, { from: minter }) ); }); context('once deployed', async function () { beforeEach(async function () { - this.token = await ERC20Capped.new(cap, [minter]); + this.token = await ERC20Capped.new(cap, { from: minter }); }); shouldBehaveLikeERC20Capped(minter, otherAccounts, cap); diff --git a/test/token/ERC20/ERC20Mintable.test.js b/test/token/ERC20/ERC20Mintable.test.js index c838f0ccd02..4cd1c33253d 100644 --- a/test/token/ERC20/ERC20Mintable.test.js +++ b/test/token/ERC20/ERC20Mintable.test.js @@ -2,21 +2,21 @@ const { shouldBehaveLikeERC20Mintable } = require('./ERC20Mintable.behavior'); const { shouldBehaveLikePublicRole } = require('../../access/rbac/PublicRole.behavior'); const ERC20MintableMock = artifacts.require('ERC20MintableMock'); -contract('ERC20Mintable', function ([_, originalMinter, otherMinter, ...otherAccounts]) { +contract('ERC20Mintable', function ([_, minter, otherMinter, ...otherAccounts]) { beforeEach(async function () { - this.token = await ERC20MintableMock.new([originalMinter, otherMinter]); + this.token = await ERC20MintableMock.new({ from: minter }); }); context('with original minter', function () { - shouldBehaveLikeERC20Mintable(originalMinter, otherAccounts); + shouldBehaveLikeERC20Mintable(minter, otherAccounts); }); describe('minter role', function () { beforeEach(async function () { - await this.token.addMinter(otherMinter); + await this.token.addMinter(otherMinter, { from: minter }); this.contract = this.token; }); - shouldBehaveLikePublicRole(originalMinter, otherMinter, otherAccounts, 'minter'); + shouldBehaveLikePublicRole(minter, otherMinter, otherAccounts, 'minter'); }); }); diff --git a/test/token/ERC20/TokenTimelock.test.js b/test/token/ERC20/TokenTimelock.test.js index d7c80f0ab16..5b5609a5311 100644 --- a/test/token/ERC20/TokenTimelock.test.js +++ b/test/token/ERC20/TokenTimelock.test.js @@ -16,7 +16,7 @@ contract('TokenTimelock', function ([_, minter, beneficiary]) { context('with token', function () { beforeEach(async function () { - this.token = await ERC20Mintable.new([minter]); + this.token = await ERC20Mintable.new({ from: minter }); }); it('rejects a release time in the past', async function () { diff --git a/test/token/ERC20/TokenVesting.test.js b/test/token/ERC20/TokenVesting.test.js index 5138289a66b..2af7a1d3767 100644 --- a/test/token/ERC20/TokenVesting.test.js +++ b/test/token/ERC20/TokenVesting.test.js @@ -44,7 +44,7 @@ contract('TokenVesting', function ([_, owner, beneficiary, minter]) { beforeEach(async function () { this.vesting = await TokenVesting.new(beneficiary, this.start, this.cliff, this.duration, true, { from: owner }); - this.token = await ERC20Mintable.new([minter]); + this.token = await ERC20Mintable.new({ from: minter }); await this.token.mint(this.vesting.address, amount, { from: minter }); }); From 3ed932c139bb2c4167d930e318726500a24c3946 Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Wed, 5 Sep 2018 20:00:28 -0300 Subject: [PATCH 2/2] fix linter error --- contracts/access/rbac/MinterRole.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contracts/access/rbac/MinterRole.sol b/contracts/access/rbac/MinterRole.sol index 889efd0d24a..7fd7d8decaf 100644 --- a/contracts/access/rbac/MinterRole.sol +++ b/contracts/access/rbac/MinterRole.sol @@ -21,7 +21,7 @@ contract MinterRole { return minters.has(_account); } - function addMinter(address _account) onlyMinter public { + function addMinter(address _account) public onlyMinter { minters.add(_account); }