Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improved Roles API. #1280

Merged
merged 2 commits into from
Sep 6, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 9 additions & 13 deletions contracts/access/rbac/MinterRole.sol
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a bit reluctant to remove transfer because it means in order to transfer now one has to do two transactions. On the other hand, this may be the 2-step transfer we had been wanting. (!!) 🙂

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can always add it later 🎉

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) public onlyMinter {
minters.add(_account);
}

function _addMinter(address _account) internal {
minters.add(_account);
function renounceMinter() public {
minters.remove(msg.sender);
}

function _removeMinter(address _account) internal {
Expand Down
22 changes: 0 additions & 22 deletions contracts/access/rbac/Roles.sol
Original file line number Diff line number Diff line change
Expand Up @@ -19,35 +19,13 @@ 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
*/
function remove(Role storage _role, address _account) internal {
_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
Expand Down
6 changes: 0 additions & 6 deletions contracts/examples/SampleCrowdsale.sol
Original file line number Diff line number Diff line change
Expand Up @@ -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
{
}
}


Expand Down
7 changes: 0 additions & 7 deletions contracts/mocks/ERC20MintableMock.sol
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Expand Down
12 changes: 0 additions & 12 deletions contracts/mocks/RolesMock.sol
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Expand Down
3 changes: 1 addition & 2 deletions contracts/token/ERC20/ERC20Capped.sol
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
6 changes: 0 additions & 6 deletions contracts/token/ERC20/ERC20Mintable.sol
Original file line number Diff line number Diff line change
Expand Up @@ -15,12 +15,6 @@ contract ERC20Mintable is ERC20, MinterRole {

bool public mintingFinished = false;

constructor(address[] _minters)
MinterRole(_minters)
public
{
}

modifier canMint() {
require(!mintingFinished);
_;
Expand Down
36 changes: 3 additions & 33 deletions test/access/rbac/PublicRole.behavior.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,3 @@
const { assertRevert } = require('../../helpers/assertRevert');

require('chai')
.should();

Expand All @@ -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 });
});
});

Expand All @@ -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 });
Expand Down
55 changes: 2 additions & 53 deletions test/access/rbac/Roles.test.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,3 @@
const { assertRevert } = require('../../helpers/assertRevert');

const RolesMock = artifacts.require('RolesMock');

require('chai')
Expand Down Expand Up @@ -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);
});
Expand All @@ -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 () {
Expand All @@ -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 });
});
});
});
});
8 changes: 5 additions & 3 deletions test/crowdsale/MintedCrowdsale.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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 () {
Expand Down
8 changes: 5 additions & 3 deletions test/examples/SampleCrowdsale.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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 () {
Expand Down
4 changes: 2 additions & 2 deletions test/token/ERC20/ERC20Capped.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
10 changes: 5 additions & 5 deletions test/token/ERC20/ERC20Mintable.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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');
});
});
2 changes: 1 addition & 1 deletion test/token/ERC20/TokenTimelock.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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 () {
Expand Down
2 changes: 1 addition & 1 deletion test/token/ERC20/TokenVesting.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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 });
});

Expand Down