From 917a91cc9fef027d9be67783f7d83c5b482ba332 Mon Sep 17 00:00:00 2001 From: Ryan Sauge Date: Mon, 11 Sep 2023 16:13:56 +0200 Subject: [PATCH 1/4] Add test for AccessControlDefaultAdminRules + add functiont transferAdminDirectly --- .../modules/security/AuthorizationModule.sol | 23 ++++-- .../AuthorizationModuleCommon.js | 79 ++++++++++++++++--- test/common/PauseModuleCommon.js | 1 - test/deploymentUtils.js | 29 +++---- 4 files changed, 103 insertions(+), 29 deletions(-) diff --git a/contracts/modules/security/AuthorizationModule.sol b/contracts/modules/security/AuthorizationModule.sol index 39e2a370..aad968b7 100644 --- a/contracts/modules/security/AuthorizationModule.sol +++ b/contracts/modules/security/AuthorizationModule.sol @@ -10,6 +10,11 @@ import "../../libraries/Errors.sol"; abstract contract AuthorizationModule is AccessControlDefaultAdminRulesUpgradeable { // BurnModule bytes32 public constant BURNER_ROLE = keccak256("BURNER_ROLE"); + // CreditEvents + bytes32 public constant DEBT_CREDIT_EVENT_ROLE = + keccak256("DEBT_CREDIT_EVENT_ROLE"); + // DebtModule + bytes32 public constant DEBT_ROLE = keccak256("DEBT_ROLE"); // EnforcementModule bytes32 public constant ENFORCER_ROLE = keccak256("ENFORCER_ROLE"); // MintModule @@ -18,11 +23,8 @@ abstract contract AuthorizationModule is AccessControlDefaultAdminRulesUpgradeab bytes32 public constant PAUSER_ROLE = keccak256("PAUSER_ROLE"); // SnapshotModule bytes32 public constant SNAPSHOOTER_ROLE = keccak256("SNAPSHOOTER_ROLE"); - // DebtModule - bytes32 public constant DEBT_ROLE = keccak256("DEBT_ROLE"); - // CreditEvents - bytes32 public constant DEBT_CREDIT_EVENT_ROLE = - keccak256("DEBT_CREDIT_EVENT_ROLE"); + + function __AuthorizationModule_init( address admin, @@ -64,5 +66,16 @@ abstract contract AuthorizationModule is AccessControlDefaultAdminRulesUpgradeab return AccessControlUpgradeable.hasRole(role, account); } + /** + @notice + Warning: this function should be called only in case of necessity (e.g private key leak) + Its goal is to transfer the adminship of the contract to a new admin, whithout delay. + The prefer way is to use the workflow of AccessControlDefaultAdminRulesUpgradeable + */ + function transferAdminshipDirectly(address newAdmin) public virtual onlyRole(DEFAULT_ADMIN_ROLE) { + // we revoke first the admin since we can only have one admin + _revokeRole(DEFAULT_ADMIN_ROLE, _msgSender()); + _grantRole(DEFAULT_ADMIN_ROLE, newAdmin); + } uint256[50] private __gap; } diff --git a/test/common/AuthorizationModule/AuthorizationModuleCommon.js b/test/common/AuthorizationModule/AuthorizationModuleCommon.js index ee5d7ef4..dc0584cb 100644 --- a/test/common/AuthorizationModule/AuthorizationModuleCommon.js +++ b/test/common/AuthorizationModule/AuthorizationModuleCommon.js @@ -1,17 +1,17 @@ -const { expectEvent } = require('@openzeppelin/test-helpers') +const { expectEvent, time } = require('@openzeppelin/test-helpers') const { expectRevertCustomError } = require('../../../openzeppelin-contracts-upgradeable/test/helpers/customError') const { PAUSER_ROLE, DEFAULT_ADMIN_ROLE } = require('../../utils') const chai = require('chai') const should = chai.should() - -function AuthorizationModuleCommon (owner, address1, address2) { +const { DEFAULT_ADMIN_DELAY_WEB3 } = require('../../deploymentUtils') +function AuthorizationModuleCommon (admin, address1, address2) { context('Authorization', function () { it('testAdminCanGrantRole', async function () { // Act this.logs = await this.cmtat.grantRole(PAUSER_ROLE, address1, { - from: owner + from: admin }); // Assert (await this.cmtat.hasRole(PAUSER_ROLE, address1)).should.equal(true) @@ -19,18 +19,18 @@ function AuthorizationModuleCommon (owner, address1, address2) { expectEvent(this.logs, 'RoleGranted', { role: PAUSER_ROLE, account: address1, - sender: owner + sender: admin }) }) it('testAdminCanRevokeRole', async function () { // Arrange - await this.cmtat.grantRole(PAUSER_ROLE, address1, { from: owner }); + await this.cmtat.grantRole(PAUSER_ROLE, address1, { from: admin }); // Arrange - Assert (await this.cmtat.hasRole(PAUSER_ROLE, address1)).should.equal(true) // Act this.logs = await this.cmtat.revokeRole(PAUSER_ROLE, address1, { - from: owner + from: admin }); // Assert (await this.cmtat.hasRole(PAUSER_ROLE, address1)).should.equal(false) @@ -38,7 +38,7 @@ function AuthorizationModuleCommon (owner, address1, address2) { expectEvent(this.logs, 'RoleRevoked', { role: PAUSER_ROLE, account: address1, - sender: owner + sender: admin }) }) @@ -58,7 +58,7 @@ function AuthorizationModuleCommon (owner, address1, address2) { it('testCannotNonAdminRevokeRole', async function () { // Arrange (await this.cmtat.hasRole(PAUSER_ROLE, address1)).should.equal(false) - await this.cmtat.grantRole(PAUSER_ROLE, address1, { from: owner }); + await this.cmtat.grantRole(PAUSER_ROLE, address1, { from: admin }); // Arrange - Assert (await this.cmtat.hasRole(PAUSER_ROLE, address1)).should.equal(true) // Act @@ -70,6 +70,67 @@ function AuthorizationModuleCommon (owner, address1, address2) { // Assert (await this.cmtat.hasRole(PAUSER_ROLE, address1)).should.equal(true) }) + + it('testCanTransferAdminship', async function () { + // Arrange + (await this.cmtat.hasRole(PAUSER_ROLE, address1)).should.equal(false) + await this.cmtat.grantRole(PAUSER_ROLE, address1, { from: admin }); + // Arrange - Assert + (await this.cmtat.hasRole(PAUSER_ROLE, address1)).should.equal(true) + // Act + await expectRevertCustomError( + this.cmtat.revokeRole(PAUSER_ROLE, address1, { from: address2 }), + 'AccessControlUnauthorizedAccount', + [address2, DEFAULT_ADMIN_ROLE] + ); + // Assert + (await this.cmtat.hasRole(PAUSER_ROLE, address1)).should.equal(true) + }) + + it('testCanAdminTransferAdminship', async function () { + // Arrange - Assert + (await this.cmtat.owner()).should.equal(admin) + // Starts an admin transfer + await this.cmtat.beginDefaultAdminTransfer(address1, { from: admin }); + + // Wait for acceptance + const acceptSchedule = web3.utils.toBN(await time.latest()).add(DEFAULT_ADMIN_DELAY_WEB3); + // We jump into the future + await time.increase(acceptSchedule.addn(1)) + + // Act + await this.cmtat.acceptDefaultAdminTransfer({ from: address1 }); + + // Assert + (await this.cmtat.owner()).should.equal(address1) + }); + + it('testCanAdminTransferAdminshipDirectly', async function () { + // Arrange - Assert + (await this.cmtat.owner()).should.equal(admin) + + // Act + // Transfer the rights + await this.cmtat.transferAdminshipDirectly(address1, { from: admin }); + + // Assert + (await this.cmtat.owner()).should.equal(address1) + }); + + + it('testCannnotNonAdminTransferAdminshipDirectly', async function () { + // Arrange - Assert + (await this.cmtat.owner()).should.equal(admin) + // Transfer the rights + await expectRevertCustomError( + this.cmtat.transferAdminshipDirectly(address1, { from: address2 }), + 'AccessControlUnauthorizedAccount', + [address2, DEFAULT_ADMIN_ROLE] + ); + + // Assert + (await this.cmtat.owner()).should.equal(admin) + }); }) } module.exports = AuthorizationModuleCommon diff --git a/test/common/PauseModuleCommon.js b/test/common/PauseModuleCommon.js index 608eceb2..77eabace 100644 --- a/test/common/PauseModuleCommon.js +++ b/test/common/PauseModuleCommon.js @@ -45,7 +45,6 @@ function PauseModuleCommon (admin, address1, address2, address3) { 'CMTAT_InvalidTransfer', [address1, address2, AMOUNT_TO_TRANSFER] ) - }) it('testCannotBePausedByNonPauser', async function () { diff --git a/test/deploymentUtils.js b/test/deploymentUtils.js index adf4fbec..b5f29542 100644 --- a/test/deploymentUtils.js +++ b/test/deploymentUtils.js @@ -8,13 +8,14 @@ const { time } = require('@openzeppelin/test-helpers') const { ethers, upgrades } = require('hardhat') const DEPLOYMENT_FLAG = 5 const DEPLOYMENT_DECIMAL = 0 - +const DEFAULT_ADMIN_DELAY = 1 +const DEFAULT_ADMIN_DELAY_WEB3 = web3.utils.toBN(time.duration.days(DEFAULT_ADMIN_DELAY)) +const DEFAULT_ADMIN_DELAY_HARDHAT = BigInt(time.duration.days(DEFAULT_ADMIN_DELAY)) async function deployCMTATStandalone (_, admin, deployerAddress) { - const delay = web3.utils.toBN(time.duration.days(3)) const cmtat = await CMTAT_STANDALONE.new( _, admin, - delay, + DEFAULT_ADMIN_DELAY_WEB3, 'CMTA Token', 'CMTAT', DEPLOYMENT_DECIMAL, @@ -32,7 +33,7 @@ async function deployCMTATStandaloneWithParameter ( deployerAddress, forwarderIrrevocable, admin, - initialDelayToAcceptAdminRole, + defaultAdminDelay, nameIrrevocable, symbolIrrevocable, decimalsIrrevocable, @@ -45,7 +46,7 @@ async function deployCMTATStandaloneWithParameter ( const cmtat = await CMTAT_STANDALONE.new( forwarderIrrevocable, admin, - initialDelayToAcceptAdminRole, + defaultAdminDelay, nameIrrevocable, symbolIrrevocable, decimalsIrrevocable, @@ -60,11 +61,11 @@ async function deployCMTATStandaloneWithParameter ( } async function deployCMTATStandaloneWithSnapshot (_, admin, deployerAddress) { - const delay = web3.utils.toBN(time.duration.days(3)) + const DEFAULT_ADMIN_DELAY_WEB3_ = web3.utils.toBN(time.duration.days(3)) const cmtat = await CMTAT_STANDALONE_SNAPSHOT.new( _, admin, - delay, + DEFAULT_ADMIN_DELAY_WEB3_, 'CMTA Token', 'CMTAT', DEPLOYMENT_DECIMAL, @@ -87,7 +88,7 @@ async function deployCMTATProxy (_, admin, deployerAddress) { ETHERS_CMTAT_PROXY_FACTORY, [ admin, - BigInt(time.duration.days(3)), + DEFAULT_ADMIN_DELAY_HARDHAT, 'CMTA Token', 'CMTAT', DEPLOYMENT_DECIMAL, @@ -115,12 +116,11 @@ async function deployCMTATProxyWithSnapshot (_, admin, deployerAddress) { const ETHERS_CMTAT_PROXY_FACTORY = await ethers.getContractFactory( 'CMTATSnapshotProxyTest' ) - const delayTime = BigInt(time.duration.days(3)) const ETHERS_CMTAT_PROXY = await upgrades.deployProxy( ETHERS_CMTAT_PROXY_FACTORY, [ admin, - delayTime, + DEFAULT_ADMIN_DELAY_HARDHAT, 'CMTA Token', 'CMTAT', DEPLOYMENT_DECIMAL, @@ -152,7 +152,7 @@ async function deployCMTATProxyWithKillTest (_, admin, deployerAddress) { ETHERS_CMTAT_PROXY_FACTORY, [ admin, - BigInt(time.duration.days(3)), + DEFAULT_ADMIN_DELAY_HARDHAT, 'CMTA Token', 'CMTAT', DEPLOYMENT_DECIMAL, @@ -179,7 +179,7 @@ async function deployCMTATProxyWithParameter ( deployerAddress, forwarderIrrevocable, admin, - initialDelayToAcceptAdminRole, + defaultAdminDelay, nameIrrevocable, symbolIrrevocable, decimalsIrrevocable, @@ -197,7 +197,7 @@ async function deployCMTATProxyWithParameter ( ETHERS_CMTAT_PROXY_FACTORY, [ admin, - initialDelayToAcceptAdminRole, + defaultAdminDelay, nameIrrevocable, symbolIrrevocable, decimalsIrrevocable, @@ -229,5 +229,6 @@ module.exports = { deployCMTATProxyWithParameter, deployCMTATStandaloneWithParameter, DEPLOYMENT_FLAG, - DEPLOYMENT_DECIMAL + DEPLOYMENT_DECIMAL, + DEFAULT_ADMIN_DELAY_WEB3 } From 5d51cd5a9a9cbb0dcb9748c1b89920a455c7e814 Mon Sep 17 00:00:00 2001 From: Ryan Sauge Date: Tue, 12 Sep 2023 09:31:32 +0200 Subject: [PATCH 2/4] Rename contract and init function for ERC20BurnModule, ERC20MintModule, ERC20SnapshotModule --- contracts/modules/CMTAT_BASE.sol | 18 +++++++++--------- .../internal/ERC20SnapshotModuleInternal.sol | 8 ++++---- .../modules/wrapper/core/ERC20BurnModule.sol | 8 ++++---- .../modules/wrapper/core/ERC20MintModule.sol | 8 ++++---- .../wrapper/extensions/ERC20SnapshotModule.sol | 12 ++++++------ .../CMTATSnapshot/CMTAT_BASE_SnapshotTest.sol | 18 +++++++++--------- 6 files changed, 36 insertions(+), 36 deletions(-) diff --git a/contracts/modules/CMTAT_BASE.sol b/contracts/modules/CMTAT_BASE.sol index 916df323..29b81b71 100644 --- a/contracts/modules/CMTAT_BASE.sol +++ b/contracts/modules/CMTAT_BASE.sol @@ -32,13 +32,13 @@ abstract contract CMTAT_BASE is ContextUpgradeable, BaseModule, PauseModule, - MintModule, - BurnModule, + ERC20MintModule, + ERC20BurnModule, EnforcementModule, ValidationModule, MetaTxModule, ERC20BaseModule, - // SnapshotModule, + // ERC20SnapshotModule, DebtBaseModule, CreditEventsModule { @@ -113,15 +113,15 @@ abstract contract CMTAT_BASE is /* SnapshotModule: Add this call in case you add the SnapshotModule - __Snapshot_init_unchained(); + __ERC20Snapshot_init_unchained(); */ __Validation_init_unchained(ruleEngine_); /* Wrapper */ // AuthorizationModule_init_unchained is called firstly due to inheritance __AuthorizationModule_init_unchained(); - __BurnModule_init_unchained(); - __MintModule_init_unchained(); + __ERC20BurnModule_init_unchained(); + __ERC20MintModule_init_unchained(); // EnforcementModule_init_unchained is called before ValidationModule_init_unchained due to inheritance __EnforcementModule_init_unchained(); __ERC20Module_init_unchained(decimalsIrrevocable); @@ -132,7 +132,7 @@ abstract contract CMTAT_BASE is /* SnapshotModule: Add this call in case you add the SnapshotModule - __SnasphotModule_init_unchained(); + __ERC20SnasphotModule_init_unchained(); */ /* Other modules */ @@ -178,7 +178,7 @@ abstract contract CMTAT_BASE is * @dev * SnapshotModule: * - override SnapshotModuleInternal if you add the SnapshotModule - * e.g. override(SnapshotModuleInternal, ERC20Upgradeable) + * e.g. override(ERC20SnapshotModuleInternal, ERC20Upgradeable) * - remove the keyword view */ function _update( @@ -194,7 +194,7 @@ abstract contract CMTAT_BASE is /* SnapshotModule: Add this call in case you add the SnapshotModule - SnapshotModuleInternal._update(from, to, amount); + ERC20SnapshotModuleInternal._update(from, to, amount); */ } diff --git a/contracts/modules/internal/ERC20SnapshotModuleInternal.sol b/contracts/modules/internal/ERC20SnapshotModuleInternal.sol index 8e1022da..765cfd4a 100644 --- a/contracts/modules/internal/ERC20SnapshotModuleInternal.sol +++ b/contracts/modules/internal/ERC20SnapshotModuleInternal.sol @@ -18,7 +18,7 @@ import "../../libraries/Errors.sol"; because overriding this function can break the contract. */ -abstract contract SnapshotModuleInternal is ERC20Upgradeable { +abstract contract ERC20SnapshotModuleInternal is ERC20Upgradeable { using ArraysUpgradeable for uint256[]; /** @@ -64,16 +64,16 @@ abstract contract SnapshotModuleInternal is ERC20Upgradeable { /** * @dev Initializes the contract */ - function __Snapshot_init( + function __ERC20Snapshot_init( string calldata name_, string calldata symbol_ ) internal onlyInitializing { __Context_init_unchained(); __ERC20_init(name_, symbol_); - __Snapshot_init_unchained(); + __ERC20Snapshot_init_unchained(); } - function __Snapshot_init_unchained() internal onlyInitializing { + function __ERC20Snapshot_init_unchained() internal onlyInitializing { _currentSnapshotTime = 0; _currentSnapshotIndex = 0; } diff --git a/contracts/modules/wrapper/core/ERC20BurnModule.sol b/contracts/modules/wrapper/core/ERC20BurnModule.sol index 6a617911..eb66e0d6 100644 --- a/contracts/modules/wrapper/core/ERC20BurnModule.sol +++ b/contracts/modules/wrapper/core/ERC20BurnModule.sol @@ -6,13 +6,13 @@ import "../../../../openzeppelin-contracts-upgradeable/contracts/token/ERC20/ERC import "../../../../openzeppelin-contracts-upgradeable/contracts/proxy/utils/Initializable.sol"; import "../../security/AuthorizationModule.sol"; -abstract contract BurnModule is ERC20Upgradeable, AuthorizationModule { +abstract contract ERC20BurnModule is ERC20Upgradeable, AuthorizationModule { /** * @notice Emitted when the specified `value` amount of tokens owned by `owner`are destroyed with the given `reason` */ event Burn(address indexed owner, uint256 value, string reason); - function __BurnModule_init( + function __ERC20BurnModule_init( string memory name_, string memory symbol_, address admin, @@ -31,10 +31,10 @@ abstract contract BurnModule is ERC20Upgradeable, AuthorizationModule { __AuthorizationModule_init_unchained(); // own function - __BurnModule_init_unchained(); + __ERC20BurnModule_init_unchained(); } - function __BurnModule_init_unchained() internal onlyInitializing { + function __ERC20BurnModule_init_unchained() internal onlyInitializing { // no variable to initialize } diff --git a/contracts/modules/wrapper/core/ERC20MintModule.sol b/contracts/modules/wrapper/core/ERC20MintModule.sol index 40ad36ff..c8abe018 100644 --- a/contracts/modules/wrapper/core/ERC20MintModule.sol +++ b/contracts/modules/wrapper/core/ERC20MintModule.sol @@ -6,14 +6,14 @@ import "../../../../openzeppelin-contracts-upgradeable/contracts/token/ERC20/ERC import "../../../../openzeppelin-contracts-upgradeable/contracts/proxy/utils/Initializable.sol"; import "../../security/AuthorizationModule.sol"; -abstract contract MintModule is ERC20Upgradeable, AuthorizationModule { +abstract contract ERC20MintModule is ERC20Upgradeable, AuthorizationModule { /** * @notice Emitted when the specified `value` amount of new tokens are created and * allocated to the specified `account`. */ event Mint(address indexed account, uint256 value); - function __MintModule_init( + function __ERC20MintModule_init( string memory name_, string memory symbol_, address admin, @@ -32,10 +32,10 @@ abstract contract MintModule is ERC20Upgradeable, AuthorizationModule { __AuthorizationModule_init_unchained(); // own function - __MintModule_init_unchained(); + __ERC20MintModule_init_unchained(); } - function __MintModule_init_unchained() internal onlyInitializing { + function __ERC20MintModule_init_unchained() internal onlyInitializing { // no variable to initialize } diff --git a/contracts/modules/wrapper/extensions/ERC20SnapshotModule.sol b/contracts/modules/wrapper/extensions/ERC20SnapshotModule.sol index ce06d379..ac37850e 100644 --- a/contracts/modules/wrapper/extensions/ERC20SnapshotModule.sol +++ b/contracts/modules/wrapper/extensions/ERC20SnapshotModule.sol @@ -12,11 +12,11 @@ import "../../internal/ERC20SnapshotModuleInternal.sol"; * Useful to take a snapshot of token holder balance and total supply at a specific time */ -abstract contract SnapshotModule is - SnapshotModuleInternal, +abstract contract ERC20SnapshotModule is + ERC20SnapshotModuleInternal, AuthorizationModule { - function __SnasphotModule_init( + function __ERC20SnasphotModule_init( string memory name_, string memory symbol_, address admin, @@ -33,16 +33,16 @@ abstract contract SnapshotModule is __AccessControlDefaultAdminRules_init_unchained(initialDelayToAcceptAdminRole, admin); /* CMTAT modules */ // Internal - __Snapshot_init_unchained(); + __ERC20Snapshot_init_unchained(); // Security __AuthorizationModule_init_unchained(); // own function - __SnasphotModule_init_unchained(); + __ERC20SnasphotModule_init_unchained(); } - function __SnasphotModule_init_unchained() internal onlyInitializing { + function __ERC20SnasphotModule_init_unchained() internal onlyInitializing { // no variable to initialize } diff --git a/contracts/test/CMTATSnapshot/CMTAT_BASE_SnapshotTest.sol b/contracts/test/CMTATSnapshot/CMTAT_BASE_SnapshotTest.sol index 29c7ab05..85aab463 100644 --- a/contracts/test/CMTATSnapshot/CMTAT_BASE_SnapshotTest.sol +++ b/contracts/test/CMTATSnapshot/CMTAT_BASE_SnapshotTest.sol @@ -32,13 +32,13 @@ abstract contract CMTAT_BASE_SnapshotTest is ContextUpgradeable, BaseModule, PauseModule, - MintModule, - BurnModule, + ERC20MintModule, + ERC20BurnModule, EnforcementModule, ValidationModule, MetaTxModule, ERC20BaseModule, - SnapshotModule, + ERC20SnapshotModule, DebtBaseModule, CreditEventsModule { @@ -104,7 +104,7 @@ abstract contract CMTAT_BASE_SnapshotTest is SnapshotModule: Add this call in case you add the SnapshotModule */ - __Snapshot_init_unchained(); + __ERC20Snapshot_init_unchained(); __Validation_init_unchained(ruleEngine_); @@ -112,8 +112,8 @@ abstract contract CMTAT_BASE_SnapshotTest is // AuthorizationModule_init_unchained is called firstly due to inheritance __AuthorizationModule_init_unchained(); __AccessControlDefaultAdminRules_init_unchained(initialDelayToAcceptAdminRole, admin); - __BurnModule_init_unchained(); - __MintModule_init_unchained(); + __ERC20BurnModule_init_unchained(); + __ERC20MintModule_init_unchained(); // EnforcementModule_init_unchained is called before ValidationModule_init_unchained due to inheritance __EnforcementModule_init_unchained(); __ERC20Module_init_unchained(decimalsIrrevocable); @@ -125,7 +125,7 @@ abstract contract CMTAT_BASE_SnapshotTest is SnapshotModule: Add this call in case you add the SnapshotModule */ - __SnasphotModule_init_unchained(); + __ERC20SnasphotModule_init_unchained(); /* Other modules */ __DebtBaseModule_init_unchained(); @@ -177,7 +177,7 @@ abstract contract CMTAT_BASE_SnapshotTest is address from, address to, uint256 amount - ) internal override(SnapshotModuleInternal, ERC20Upgradeable) { + ) internal override(ERC20SnapshotModuleInternal, ERC20Upgradeable) { // We call the SnapshotModule only if the transfer is valid if (!ValidationModule.validateTransfer(from, to, amount)) revert Errors.CMTAT_InvalidTransfer(from, to, amount); @@ -188,7 +188,7 @@ abstract contract CMTAT_BASE_SnapshotTest is SnapshotModule: Add this call in case you add the SnapshotModule */ - SnapshotModuleInternal._update(from, to, amount); + ERC20SnapshotModuleInternal._update(from, to, amount); } /** From 28301d07a35c662d31ed72bdddc8dce3c2dc3fd8 Mon Sep 17 00:00:00 2001 From: Ryan Sauge Date: Tue, 12 Sep 2023 13:41:04 +0200 Subject: [PATCH 3/4] AuthorizationModule: improve test --- .../AuthorizationModuleCommon.js | 29 +++++++++++++++++-- 1 file changed, 27 insertions(+), 2 deletions(-) diff --git a/test/common/AuthorizationModule/AuthorizationModuleCommon.js b/test/common/AuthorizationModule/AuthorizationModuleCommon.js index dc0584cb..c9319e92 100644 --- a/test/common/AuthorizationModule/AuthorizationModuleCommon.js +++ b/test/common/AuthorizationModule/AuthorizationModuleCommon.js @@ -42,6 +42,9 @@ function AuthorizationModuleCommon (admin, address1, address2) { }) }) + /* + Already tested by OpenZeppelin library + */ it('testCannotNonAdminGrantRole', async function () { // Arrange - Assert (await this.cmtat.hasRole(PAUSER_ROLE, address1)).should.equal(false) @@ -55,6 +58,9 @@ function AuthorizationModuleCommon (admin, address1, address2) { (await this.cmtat.hasRole(PAUSER_ROLE, address1)).should.equal(false) }) + /* + Already tested by OpenZeppelin library + */ it('testCannotNonAdminRevokeRole', async function () { // Arrange (await this.cmtat.hasRole(PAUSER_ROLE, address1)).should.equal(false) @@ -71,6 +77,9 @@ function AuthorizationModuleCommon (admin, address1, address2) { (await this.cmtat.hasRole(PAUSER_ROLE, address1)).should.equal(true) }) + /* + Already tested by OpenZeppelin library + */ it('testCanTransferAdminship', async function () { // Arrange (await this.cmtat.hasRole(PAUSER_ROLE, address1)).should.equal(false) @@ -87,6 +96,7 @@ function AuthorizationModuleCommon (admin, address1, address2) { (await this.cmtat.hasRole(PAUSER_ROLE, address1)).should.equal(true) }) + it('testCanAdminTransferAdminship', async function () { // Arrange - Assert (await this.cmtat.owner()).should.equal(admin) @@ -105,6 +115,22 @@ function AuthorizationModuleCommon (admin, address1, address2) { (await this.cmtat.owner()).should.equal(address1) }); + /* + Already tested by OpenZeppelin library + */ + it('testCannotNonAdminTransferAdminship', async function () { + // Arrange - Assert + (await this.cmtat.owner()).should.equal(admin) + // Starts an admin transfer + await expectRevertCustomError( + this.cmtat.beginDefaultAdminTransfer(address1, { from: address1 }), + 'AccessControlUnauthorizedAccount', + [address1, DEFAULT_ADMIN_ROLE] + ); + // Assert + (await this.cmtat.owner()).should.equal(admin) + }); + it('testCanAdminTransferAdminshipDirectly', async function () { // Arrange - Assert (await this.cmtat.owner()).should.equal(admin) @@ -117,8 +143,7 @@ function AuthorizationModuleCommon (admin, address1, address2) { (await this.cmtat.owner()).should.equal(address1) }); - - it('testCannnotNonAdminTransferAdminshipDirectly', async function () { + it('testCannotNonAdminTransferAdminshipDirectly', async function () { // Arrange - Assert (await this.cmtat.owner()).should.equal(admin) // Transfer the rights From 198e8697b4c01d3b2d39cd3ebb3aae8d6efbd44a Mon Sep 17 00:00:00 2001 From: Ryan Sauge Date: Tue, 12 Sep 2023 13:47:00 +0200 Subject: [PATCH 4/4] AuthorizationModule: improve test --- .../AuthorizationModuleCommon.js | 20 ------------------- 1 file changed, 20 deletions(-) diff --git a/test/common/AuthorizationModule/AuthorizationModuleCommon.js b/test/common/AuthorizationModule/AuthorizationModuleCommon.js index c9319e92..9077b048 100644 --- a/test/common/AuthorizationModule/AuthorizationModuleCommon.js +++ b/test/common/AuthorizationModule/AuthorizationModuleCommon.js @@ -77,26 +77,6 @@ function AuthorizationModuleCommon (admin, address1, address2) { (await this.cmtat.hasRole(PAUSER_ROLE, address1)).should.equal(true) }) - /* - Already tested by OpenZeppelin library - */ - it('testCanTransferAdminship', async function () { - // Arrange - (await this.cmtat.hasRole(PAUSER_ROLE, address1)).should.equal(false) - await this.cmtat.grantRole(PAUSER_ROLE, address1, { from: admin }); - // Arrange - Assert - (await this.cmtat.hasRole(PAUSER_ROLE, address1)).should.equal(true) - // Act - await expectRevertCustomError( - this.cmtat.revokeRole(PAUSER_ROLE, address1, { from: address2 }), - 'AccessControlUnauthorizedAccount', - [address2, DEFAULT_ADMIN_ROLE] - ); - // Assert - (await this.cmtat.hasRole(PAUSER_ROLE, address1)).should.equal(true) - }) - - it('testCanAdminTransferAdminship', async function () { // Arrange - Assert (await this.cmtat.owner()).should.equal(admin)