From 5b7ec56f283f12ca26ab4166e9096018aceaae6b Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Nicol=C3=A1s=20Venturo?= <nicolas.venturo@gmail.com>
Date: Tue, 28 Aug 2018 15:10:25 -0300
Subject: [PATCH] Roles can now be transfered. (#1235)

* Roles can now be transfered.

* Now explicitly checking support for the null address.

* Now rejecting transfer to a role-haver.

* Added renounce, roles can no longer be transfered to 0.

* Fixed linter errors.

* Fixed a Roles test.
---
 contracts/access/rbac/Roles.sol | 30 ++++++++++---------
 contracts/mocks/RolesMock.sol   |  8 ++++++
 test/access/rbac/Roles.test.js  | 51 ++++++++++++++++++++++++++++++++-
 3 files changed, 75 insertions(+), 14 deletions(-)

diff --git a/contracts/access/rbac/Roles.sol b/contracts/access/rbac/Roles.sol
index d478f6afa65..f5c95b89baf 100644
--- a/contracts/access/rbac/Roles.sol
+++ b/contracts/access/rbac/Roles.sol
@@ -15,18 +15,14 @@ library Roles {
   /**
    * @dev give an account access to this role
    */
-  function add(Role storage _role, address _account)
-    internal
-  {
+  function add(Role storage _role, address _account) internal {
     _role.bearer[_account] = true;
   }
 
   /**
    * @dev give multiple accounts access to this role
    */
-  function addMany(Role storage _role, address[] _accounts)
-    internal
-  {
+  function addMany(Role storage _role, address[] _accounts) internal {
     for (uint256 i = 0; i < _accounts.length; ++i) {
       add(_role, _accounts[i]);
     }
@@ -35,20 +31,28 @@ library Roles {
   /**
    * @dev remove an account's access to this role
    */
-  function remove(Role storage _role, address _account)
-    internal
-  {
+  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
    * // reverts
    */
-  function check(Role storage _role, address _account)
-    internal
-    view
-  {
+  function check(Role storage _role, address _account) internal view {
     require(has(_role, _account));
   }
 
diff --git a/contracts/mocks/RolesMock.sol b/contracts/mocks/RolesMock.sol
index cf20dfa45e1..96e0b64aaef 100644
--- a/contracts/mocks/RolesMock.sol
+++ b/contracts/mocks/RolesMock.sol
@@ -20,6 +20,14 @@ contract RolesMock {
     dummyRole.remove(_account);
   }
 
+  function renounce() public {
+    dummyRole.renounce();
+  }
+
+  function transfer(address _account) public {
+    dummyRole.transfer(_account);
+  }
+
   function check(address _account) public view {
     dummyRole.check(_account);
   }
diff --git a/test/access/rbac/Roles.test.js b/test/access/rbac/Roles.test.js
index d7269714d5f..743d6fe082c 100644
--- a/test/access/rbac/Roles.test.js
+++ b/test/access/rbac/Roles.test.js
@@ -6,6 +6,8 @@ require('chai')
   .should();
 
 contract('Roles', function ([_, authorized, otherAuthorized, anyone]) {
+  const ZERO_ADDRESS = '0x0000000000000000000000000000000000000000';
+
   beforeEach(async function () {
     this.roles = await RolesMock.new();
     this.testRole = async (account, expected) => {
@@ -49,6 +51,10 @@ contract('Roles', function ([_, authorized, otherAuthorized, anyone]) {
         await this.roles.addMany([authorized, authorized]);
         await this.testRole(authorized, true);
       });
+
+      it('doesn\'t revert when adding roles to the null account', async function () {
+        await this.roles.add(ZERO_ADDRESS);
+      });
     });
   });
 
@@ -64,9 +70,52 @@ contract('Roles', function ([_, authorized, otherAuthorized, anyone]) {
         await this.testRole(otherAuthorized, true);
       });
 
-      it('removes unassigned roles', async function () {
+      it('doesn\'t revert when removing unassigned roles', async function () {
         await this.roles.remove(anyone);
       });
+
+      it('doesn\'t revert when removing roles from the null account', async function () {
+        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.testRole(anyone, true);
+          await this.testRole(authorized, 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.testRole(authorized, false);
+      });
+
+      it('doesn\'t revert when renouncing unassigned role', async function () {
+        await this.roles.renounce({ from: anyone });
+      });
     });
   });
 });