From e3eddd093a70dfaa91e2e4c6389ef9823c8abbcc Mon Sep 17 00:00:00 2001 From: Daksh Miglani Date: Mon, 10 May 2021 14:19:45 +0530 Subject: [PATCH] =?UTF-8?q?feat:=20=F0=9F=8E=B8=20access=20controller=20->?= =?UTF-8?q?=20mapping=20controller=20bytes32=20->=20addr?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- ...Control.sol => InstaMappingController.sol} | 30 ++-- contracts/mapping/InstaMappings.sol | 37 ----- test/01_insta_access_control.js | 113 ++++++------- test/02_insta_mapping.js | 148 ------------------ 4 files changed, 67 insertions(+), 261 deletions(-) rename contracts/mapping/{InstaAccessControl.sol => InstaMappingController.sol} (82%) delete mode 100644 contracts/mapping/InstaMappings.sol delete mode 100644 test/02_insta_mapping.js diff --git a/contracts/mapping/InstaAccessControl.sol b/contracts/mapping/InstaMappingController.sol similarity index 82% rename from contracts/mapping/InstaAccessControl.sol rename to contracts/mapping/InstaMappingController.sol index 68303a27..c7287aaf 100644 --- a/contracts/mapping/InstaAccessControl.sol +++ b/contracts/mapping/InstaMappingController.sol @@ -10,11 +10,11 @@ interface IndexInterface { function master() external view returns (address); } -contract InstaAccessControl is Context { +contract InstaMappingController is Context { using EnumerableSet for EnumerableSet.AddressSet; using Address for address; - mapping(bytes32 => EnumerableSet.AddressSet) private _roles; + mapping(address => EnumerableSet.AddressSet) private _roles; IndexInterface public constant instaIndex = IndexInterface(0x2971AdFa57b20E5a416aE5a708A8655A9c74f723); @@ -22,7 +22,7 @@ contract InstaAccessControl is Context { /** * @dev Emitted when `account` is granted `role`. */ - event RoleGranted(bytes32 indexed role, address indexed account); + event RoleGranted(address indexed role, address indexed account); /** * @dev Emitted when `account` is revoked `role`. @@ -32,7 +32,7 @@ contract InstaAccessControl is Context { * - if using `renounceRole`, it is the role bearer (i.e. `account`) */ event RoleRevoked( - bytes32 indexed role, + address indexed role, address indexed account, address indexed sender ); @@ -40,7 +40,7 @@ contract InstaAccessControl is Context { modifier onlyMaster { require( instaIndex.master() == _msgSender(), - "AccessControl: sender must be master" + "MappingController: sender must be master" ); _; } @@ -48,7 +48,7 @@ contract InstaAccessControl is Context { /** * @dev Returns `true` if `account` has been granted `role`. */ - function hasRole(bytes32 role, address account) public view returns (bool) { + function hasRole(address role, address account) public view returns (bool) { return _roles[role].contains(account); } @@ -56,7 +56,7 @@ contract InstaAccessControl is Context { * @dev Returns the number of accounts that have `role`. Can be used * together with {getRoleMember} to enumerate all bearers of a role. */ - function getRoleMemberCount(bytes32 role) public view returns (uint256) { + function getRoleMemberCount(address role) public view returns (uint256) { return _roles[role].length(); } @@ -72,7 +72,7 @@ contract InstaAccessControl is Context { * https://forum.openzeppelin.com/t/iterating-over-elements-on-enumerableset-in-openzeppelin-contracts/2296[forum post] * for more information. */ - function getRoleMember(bytes32 role, uint256 index) + function getRoleMember(address role, uint256 index) public view returns (address) @@ -90,7 +90,7 @@ contract InstaAccessControl is Context { * * - the caller must be the master. */ - function grantRole(bytes32 role, address account) + function grantRole(address role, address account) public virtual onlyMaster @@ -107,7 +107,7 @@ contract InstaAccessControl is Context { * * - the caller must be the master. */ - function revokeRole(bytes32 role, address account) + function revokeRole(address role, address account) public virtual onlyMaster @@ -129,10 +129,10 @@ contract InstaAccessControl is Context { * * - the caller must be `account`. */ - function renounceRole(bytes32 role, address account) public virtual { + function renounceRole(address role, address account) public virtual { require( account == _msgSender(), - "AccessControl: can only renounce roles for self" + "MappingController: can only renounce roles for self" ); _revokeRole(role, account); @@ -154,17 +154,17 @@ contract InstaAccessControl is Context { * system imposed by {AccessControl}. * ==== */ - function _setupRole(bytes32 role, address account) internal virtual { + function _setupRole(address role, address account) internal virtual { _grantRole(role, account); } - function _grantRole(bytes32 role, address account) private { + function _grantRole(address role, address account) private { if (_roles[role].add(account)) { emit RoleGranted(role, account); } } - function _revokeRole(bytes32 role, address account) private { + function _revokeRole(address role, address account) private { if (_roles[role].remove(account)) { emit RoleRevoked(role, account, _msgSender()); } diff --git a/contracts/mapping/InstaMappings.sol b/contracts/mapping/InstaMappings.sol deleted file mode 100644 index 17147510..00000000 --- a/contracts/mapping/InstaMappings.sol +++ /dev/null @@ -1,37 +0,0 @@ -pragma solidity ^0.7.0; -pragma experimental ABIEncoderV2; - -import {InstaAccessControl} from "./InstaAccessControl.sol"; - -contract InstaMappings is InstaAccessControl { - function getMappingContractRole(address mappingContract) - public - pure - returns (bytes32 role) - { - bytes memory encoded = abi.encode(mappingContract); - assembly { - role := mload(add(encoded, 32)) - } - } - - function hasRole(address mappingAddr, address account) - public - view - returns (bool) - { - return super.hasRole(getMappingContractRole(mappingAddr), account); - } - - function grantRole(address mappingAddr, address account) public { - super.grantRole(getMappingContractRole(mappingAddr), account); - } - - function revokeRole(address mappingAddr, address account) public { - super.revokeRole(getMappingContractRole(mappingAddr), account); - } - - function renounceRole(address mappingAddr, address account) public { - super.renounceRole(getMappingContractRole(mappingAddr), account); - } -} diff --git a/test/01_insta_access_control.js b/test/01_insta_access_control.js index cd24a164..4702d54e 100644 --- a/test/01_insta_access_control.js +++ b/test/01_insta_access_control.js @@ -8,22 +8,22 @@ chai.use(solidity); const { expect } = chai; -const getAccessControl = (address, signer) => { - return ethers.getContractAt("InstaAccessControl", address, signer); +const getMapping = (address, signer) => { + return ethers.getContractAt("InstaMappingController", address, signer); }; -describe("Test InstaAccessControl contract", () => { +describe("Test InstaMapping contract", () => { let account, instaMaster; - let accessControlAddress; - let masterAccessControl; + let mappingAddress; + let masterMapping; const indexInterfaceAddress = "0x2971AdFa57b20E5a416aE5a708A8655A9c74f723"; - const TEST_ROLE = ethers.utils.formatBytes32String("test"); + const testRoleAddress = "0x2971AdFa57b20E5a416aE5a708A8655A9c74f723"; before("get signers", async () => { [account] = await ethers.getSigners(); const IndexContract = await ethers.getContractAt( - "contracts/mapping/InstaAccessControl.sol:IndexInterface", + "contracts/mapping/InstaMappingController.sol:IndexInterface", indexInterfaceAddress ); const masterAddress = await IndexContract.master(); @@ -44,130 +44,121 @@ describe("Test InstaAccessControl contract", () => { }); beforeEach("deploy contract", async () => { - const accessControlFactory = await ethers.getContractFactory( - "InstaAccessControl" + const mappingFactory = await ethers.getContractFactory( + "InstaMappingController" ); - const accessControl = await accessControlFactory.deploy(); + const mapping = await mappingFactory.deploy(); - await accessControl.deployed(); - accessControlAddress = accessControl.address; + await mapping.deployed(); + mappingAddress = mapping.address; - masterAccessControl = await getAccessControl( - accessControlAddress, - instaMaster - ); + masterMapping = await getMapping(mappingAddress, instaMaster); }); it("grant,revoke role should fail with non master signer", async () => { - const selfAccessControl = await getAccessControl( - accessControlAddress, - account - ); + const selfMapping = await getMapping(mappingAddress, account); await expect( - selfAccessControl.grantRole(TEST_ROLE, account.address) - ).to.rejectedWith(/AccessControl: sender must be master/); + selfMapping.grantRole(testRoleAddress, account.address) + ).to.rejectedWith(/MappingController: sender must be master/); await expect( - selfAccessControl.revokeRole(TEST_ROLE, account.address) - ).to.rejectedWith(/AccessControl: sender must be master/); + selfMapping.revokeRole(testRoleAddress, account.address) + ).to.rejectedWith(/MappingController: sender must be master/); }); it("hasRole should return false for roles not assigned to users", async () => { - expect(await masterAccessControl.hasRole(TEST_ROLE, account.address)).to.eq( + expect(await masterMapping.hasRole(testRoleAddress, account.address)).to.eq( false ); }); it("should grant roles", async () => { - await expect(masterAccessControl.grantRole(TEST_ROLE, account.address)) - .to.emit(masterAccessControl, "RoleGranted") - .withArgs(TEST_ROLE, account.address); + await expect(masterMapping.grantRole(testRoleAddress, account.address)) + .to.emit(masterMapping, "RoleGranted") + .withArgs(testRoleAddress, account.address); - expect(await masterAccessControl.hasRole(TEST_ROLE, account.address)).to.eq( + expect(await masterMapping.hasRole(testRoleAddress, account.address)).to.eq( true ); }); it("should revoke role", async () => { // add a role first - await masterAccessControl.grantRole(TEST_ROLE, account.address); - expect(await masterAccessControl.hasRole(TEST_ROLE, account.address)).to.eq( + await masterMapping.grantRole(testRoleAddress, account.address); + expect(await masterMapping.hasRole(testRoleAddress, account.address)).to.eq( true ); // then remove the role - await expect(masterAccessControl.revokeRole(TEST_ROLE, account.address)) - .to.emit(masterAccessControl, "RoleRevoked") - .withArgs(TEST_ROLE, account.address, instaMaster.address); + await expect(masterMapping.revokeRole(testRoleAddress, account.address)) + .to.emit(masterMapping, "RoleRevoked") + .withArgs(testRoleAddress, account.address, instaMaster.address); - expect(await masterAccessControl.hasRole(TEST_ROLE, account.address)).to.eq( + expect(await masterMapping.hasRole(testRoleAddress, account.address)).to.eq( false ); }); it("should renounce role only with the account not master", async () => { // add a role first - await masterAccessControl.grantRole(TEST_ROLE, account.address); - expect(await masterAccessControl.hasRole(TEST_ROLE, account.address)).to.eq( + await masterMapping.grantRole(testRoleAddress, account.address); + expect(await masterMapping.hasRole(testRoleAddress, account.address)).to.eq( true ); // then renounce the the role await expect( - masterAccessControl.renounceRole(TEST_ROLE, account.address) - ).to.rejectedWith(/AccessControl: can only renounce roles for self/); + masterMapping.renounceRole(testRoleAddress, account.address) + ).to.rejectedWith(/MappingController: can only renounce roles for self/); - const selfAccessControl = await getAccessControl( - accessControlAddress, - account - ); - expect(await selfAccessControl.renounceRole(TEST_ROLE, account.address)) - .to.emit(masterAccessControl, "RoleRevoked") - .withArgs(TEST_ROLE, account.address, account.address); + const selfMapping = await getMapping(mappingAddress, account); + expect(await selfMapping.renounceRole(testRoleAddress, account.address)) + .to.emit(masterMapping, "RoleRevoked") + .withArgs(testRoleAddress, account.address, account.address); - expect(await masterAccessControl.hasRole(TEST_ROLE, account.address)).to.eq( + expect(await masterMapping.hasRole(testRoleAddress, account.address)).to.eq( false ); }); it("should do role count properly", async () => { - expect(await masterAccessControl.getRoleMemberCount(TEST_ROLE)).to.eq(0); + expect(await masterMapping.getRoleMemberCount(testRoleAddress)).to.eq(0); - await masterAccessControl.grantRole(TEST_ROLE, account.address); + await masterMapping.grantRole(testRoleAddress, account.address); - expect(await masterAccessControl.getRoleMemberCount(TEST_ROLE)).to.eq(1); + expect(await masterMapping.getRoleMemberCount(testRoleAddress)).to.eq(1); - await masterAccessControl.grantRole(TEST_ROLE, instaMaster.address); + await masterMapping.grantRole(testRoleAddress, instaMaster.address); - expect(await masterAccessControl.getRoleMemberCount(TEST_ROLE)).to.eq(2); + expect(await masterMapping.getRoleMemberCount(testRoleAddress)).to.eq(2); - await masterAccessControl.revokeRole(TEST_ROLE, instaMaster.address); + await masterMapping.revokeRole(testRoleAddress, instaMaster.address); - expect(await masterAccessControl.getRoleMemberCount(TEST_ROLE)).to.eq(1); + expect(await masterMapping.getRoleMemberCount(testRoleAddress)).to.eq(1); }); it("should get member correctly by index", async () => { await expect( - masterAccessControl.getRoleMember(TEST_ROLE, 0) + masterMapping.getRoleMember(testRoleAddress, 0) ).to.rejectedWith(/EnumerableSet: index out of bounds/); - await masterAccessControl.grantRole(TEST_ROLE, account.address); + await masterMapping.grantRole(testRoleAddress, account.address); - expect(await masterAccessControl.getRoleMember(TEST_ROLE, 0)).to.eq( + expect(await masterMapping.getRoleMember(testRoleAddress, 0)).to.eq( account.address ); - await masterAccessControl.grantRole(TEST_ROLE, instaMaster.address); + await masterMapping.grantRole(testRoleAddress, instaMaster.address); - expect(await masterAccessControl.getRoleMember(TEST_ROLE, 1)).to.eq( + expect(await masterMapping.getRoleMember(testRoleAddress, 1)).to.eq( instaMaster.address ); - await masterAccessControl.revokeRole(TEST_ROLE, instaMaster.address); + await masterMapping.revokeRole(testRoleAddress, instaMaster.address); await expect( - masterAccessControl.getRoleMember(TEST_ROLE, 1) + masterMapping.getRoleMember(testRoleAddress, 1) ).to.rejectedWith(/EnumerableSet: index out of bounds/); }); }); diff --git a/test/02_insta_mapping.js b/test/02_insta_mapping.js deleted file mode 100644 index 295ef5f7..00000000 --- a/test/02_insta_mapping.js +++ /dev/null @@ -1,148 +0,0 @@ -const { ethers, network } = require("hardhat"); -const chai = require("chai"); -const chaiPromise = require("chai-as-promised"); -const { solidity } = require("ethereum-waffle"); - -chai.use(chaiPromise); -chai.use(solidity); - -const { expect } = chai; - -const getInstaMappings = (address, signer) => { - return ethers.getContractAt("InstaMappings", address, signer); -}; - -describe("Test InstaMapping contract", () => { - let account, instaMaster; - let instaMappingAddress; - let masterInstaMapping; - const indexInterfaceAddress = "0x2971AdFa57b20E5a416aE5a708A8655A9c74f723"; - const TEST_CONTRACT = "0x1f9840a85d5af5bf1d1762f925bdaddc4201f984"; - let TEST_CONTRACT_ENCODED; - const keys = { - grant: "grantRole(address,address)", - revoke: "revokeRole(address,address)", - renounce: "renounceRole(address,address)", - has: "hasRole(address,address)", - }; - - before("get signers", async () => { - [account] = await ethers.getSigners(); - - const IndexContract = await ethers.getContractAt( - "contracts/mapping/InstaAccessControl.sol:IndexInterface", - indexInterfaceAddress - ); - const masterAddress = await IndexContract.master(); - - await network.provider.request({ - method: "hardhat_impersonateAccount", - params: [masterAddress], - }); - - instaMaster = await ethers.getSigner(masterAddress); - }); - - after(async () => { - await network.provider.request({ - method: "hardhat_stopImpersonatingAccount", - params: [instaMaster.address], - }); - }); - - beforeEach("deploy contract", async () => { - const instaMappingFactory = await ethers.getContractFactory( - "InstaMappings" - ); - const instaMapping = await instaMappingFactory.deploy(); - - await instaMapping.deployed(); - instaMappingAddress = instaMapping.address; - - masterInstaMapping = await getInstaMappings( - instaMappingAddress, - instaMaster - ); - - TEST_CONTRACT_ENCODED = await instaMapping.getMappingContractRole( - TEST_CONTRACT - ); - }); - - it("grant,revoke role should fail with non master signer", async () => { - const selfInstaMapping = await getInstaMappings( - instaMappingAddress, - account - ); - - await expect( - selfInstaMapping[keys.grant](TEST_CONTRACT, account.address) - ).to.rejectedWith(/AccessControl: sender must be master/); - - await expect( - selfInstaMapping[keys.revoke](TEST_CONTRACT, account.address) - ).to.rejectedWith(/AccessControl: sender must be master/); - }); - - it("hasRole should return false for roles not assigned to users", async () => { - expect( - await masterInstaMapping[keys.has](TEST_CONTRACT, account.address) - ).to.eq(false); - }); - - it("should grant roles", async () => { - await expect(masterInstaMapping[keys.grant](TEST_CONTRACT, account.address)) - .to.emit(masterInstaMapping, "RoleGranted") - .withArgs(TEST_CONTRACT_ENCODED, account.address); - - expect( - await masterInstaMapping[keys.has](TEST_CONTRACT, account.address) - ).to.eq(true); - }); - - it("should revoke role", async () => { - // add a role first - await masterInstaMapping[keys.grant](TEST_CONTRACT, account.address); - expect( - await masterInstaMapping[keys.has](TEST_CONTRACT, account.address) - ).to.eq(true); - - // then remove the role - await expect( - masterInstaMapping[keys.revoke](TEST_CONTRACT, account.address) - ) - .to.emit(masterInstaMapping, "RoleRevoked") - .withArgs(TEST_CONTRACT_ENCODED, account.address, instaMaster.address); - - expect( - await masterInstaMapping[keys.has](TEST_CONTRACT, account.address) - ).to.eq(false); - }); - - it("should renounce role only with the account not master", async () => { - // add a role first - await masterInstaMapping[keys.grant](TEST_CONTRACT, account.address); - expect( - await masterInstaMapping[keys.has](TEST_CONTRACT, account.address) - ).to.eq(true); - - // then renounce the the role - await expect( - masterInstaMapping[keys.renounce](TEST_CONTRACT, account.address) - ).to.rejectedWith(/AccessControl: can only renounce roles for self/); - - const selfInstaMapping = await getInstaMappings( - instaMappingAddress, - account - ); - expect( - await selfInstaMapping[keys.renounce](TEST_CONTRACT, account.address) - ) - .to.emit(masterInstaMapping, "RoleRevoked") - .withArgs(TEST_CONTRACT_ENCODED, account.address, account.address); - - expect( - await masterInstaMapping[keys.has](TEST_CONTRACT, account.address) - ).to.eq(false); - }); -});