diff --git a/governance/scripts/deployments/publicLock.js b/governance/scripts/deployments/publicLock.js index 3518be08783..68fbbee3f5a 100644 --- a/governance/scripts/deployments/publicLock.js +++ b/governance/scripts/deployments/publicLock.js @@ -3,6 +3,7 @@ const { ADDRESS_ZERO, deployContract, copyAndBuildContractsAtVersion, + LOCK_MANAGER_ROLE, } = require('@unlock-protocol/hardhat-helpers') async function main({ publicLockVersion }) { @@ -39,7 +40,10 @@ async function main({ publicLockVersion }) { console.log(`PUBLIC LOCK > Template initialized (tx: ${txInitHash})`) // renounce the manager role that was added during initilization - const { hash: txRenounceHash } = await publicLock.renounceLockManager() + const { hash: txRenounceHash } = await publicLock.revokeRole( + LOCK_MANAGER_ROLE, + await signer.getAddress() + ) console.log(`PUBLIC LOCK > manager role revoked (tx: ${txRenounceHash})`) return publicLockAddress diff --git a/packages/hardhat-helpers/src/index.ts b/packages/hardhat-helpers/src/index.ts index b2828f23ced..f0c026bfef5 100644 --- a/packages/hardhat-helpers/src/index.ts +++ b/packages/hardhat-helpers/src/index.ts @@ -14,6 +14,7 @@ import upgrades from './upgrades' import localhost from './localhost' import events from './events' import deploy from './deploy' +import roles from './roles' module.exports = { ...balance, @@ -32,4 +33,5 @@ module.exports = { ...localhost, ...events, ...deploy, + ...roles, } diff --git a/packages/hardhat-helpers/src/roles.ts b/packages/hardhat-helpers/src/roles.ts new file mode 100644 index 00000000000..4025ea38c19 --- /dev/null +++ b/packages/hardhat-helpers/src/roles.ts @@ -0,0 +1,10 @@ +// keccak256('KEY_GRANTER_ROLE') +export const KEY_GRANTER_ROLE = `0xb309c40027c81d382c3b58d8de24207a34b27e1db369b1434e4a11311f154b5e` +export const DEFAULT_ADMIN_ROLE = `0x0000000000000000000000000000000000000000000000000000000000000000` +export const LOCK_MANAGER_ROLE = `0xb89cdd26cddd51301940bf2715f765b626b8a5a9e2681ac62dc83cc2db2530c0` + +export default { + KEY_GRANTER_ROLE, + DEFAULT_ADMIN_ROLE, + LOCK_MANAGER_ROLE, +} diff --git a/smart-contracts/contracts/Unlock.sol b/smart-contracts/contracts/Unlock.sol index a7411406083..75f3b276008 100644 --- a/smart-contracts/contracts/Unlock.sol +++ b/smart-contracts/contracts/Unlock.sol @@ -200,8 +200,8 @@ contract Unlock is UnlockInitializable, UnlockOwnable { // if the template has not been initialized, // claim the template so that no-one else could try IPublicLock(impl).initialize(address(this), 0, address(0), 0, 0, "") { - // renounce the lock manager role that was added during initialization - IPublicLock(impl).renounceLockManager(); + // renounce Unlock's lock manager role that was added during initialization + IPublicLock(impl).revokeRole(keccak256("LOCK_MANAGER"), address(this)); } catch { // failure means that the template is already initialized } diff --git a/smart-contracts/contracts/interfaces/IPublicLock.sol b/smart-contracts/contracts/interfaces/IPublicLock.sol index 6866e835c57..0d4c0590625 100644 --- a/smart-contracts/contracts/interfaces/IPublicLock.sol +++ b/smart-contracts/contracts/interfaces/IPublicLock.sol @@ -346,8 +346,6 @@ interface IPublicLock { uint _tokenId ) external view returns (uint refund); - function addLockManager(address account) external; - function isLockManager(address account) external view returns (bool); /** @@ -392,8 +390,6 @@ interface IPublicLock { */ function onKeyGrantHook() external view returns (address hookAddress); - function renounceLockManager() external; - /** * @return the maximum number of key allowed for a single address */ @@ -487,7 +483,7 @@ interface IPublicLock { * - `from`, `to` cannot be zero. * - `tokenId` must be owned by `from`. * - If the caller is not `from`, it must be have been allowed to move this - * NFT by either `approve` or `setApprovalForAll`. + * `approve` */ function safeTransferFrom(address from, address to, uint256 tokenId) external; @@ -504,7 +500,7 @@ interface IPublicLock { * @param to the address that will receive the token * @param tokenId the id of the token * @dev Requirements: if the caller is not `from`, it must be approved to move this token by - * either `approve` or `setApprovalForAll`. + * `approve` * The key manager will be reset to address zero after the transfer */ function transferFrom(address from, address to, uint256 tokenId) external; @@ -541,26 +537,6 @@ interface IPublicLock { uint256 _tokenId ) external view returns (address operator); - /** - * @dev Sets or unsets the approval of a given operator - * An operator is allowed to transfer all tokens of the sender on their behalf - * @param _operator operator address to set the approval - * @param _approved representing the status of the approval to be set - * @notice disabled when transfers are disabled - */ - function setApprovalForAll(address _operator, bool _approved) external; - - /** - * @dev Tells whether an operator is approved by a given keyManager - * @param _owner owner address which you want to query the approval of - * @param _operator operator address which you want to query the approval of - * @return bool whether the given operator is approved by the given owner - */ - function isApprovedForAll( - address _owner, - address _operator - ) external view returns (bool); - /** * Returns the total number of keys, including non-valid ones * @return _totalKeysCreated the total number of keys, valid or not @@ -609,20 +585,6 @@ interface IPublicLock { */ function migrate(bytes calldata _calldata) external; - /** - * Returns the version number of the data schema currently used by the lock - * @notice if this is different from `publicLockVersion`, then the ability to purchase, grant - * or extend keys is disabled. - * @dev will return 0 if no ;igration has ever been run - */ - function schemaVersion() external view returns (uint); - - /** - * Set the schema version to the latest - * @notice only lock manager call call this - */ - function updateSchemaVersion() external; - /** * Renew a given token * @notice only works for non-free, expiring, ERC20 locks diff --git a/smart-contracts/contracts/mixins/MixinKeys.sol b/smart-contracts/contracts/mixins/MixinKeys.sol index 0972f82427d..04655e54fb5 100644 --- a/smart-contracts/contracts/mixins/MixinKeys.sol +++ b/smart-contracts/contracts/mixins/MixinKeys.sol @@ -91,14 +91,10 @@ contract MixinKeys is MixinErrors, MixinLockCore { * @dev This is a modifier */ function _onlyKeyManagerOrApproved(uint _tokenId) internal view { - address realKeyManager = keyManagerOf[_tokenId] == address(0) - ? _ownerOf[_tokenId] - : keyManagerOf[_tokenId]; if ( !isLockManager(msg.sender) && !_isKeyManager(_tokenId, msg.sender) && - approved[_tokenId] != msg.sender && - !isApprovedForAll(realKeyManager, msg.sender) + approved[_tokenId] != msg.sender ) { revert ONLY_KEY_MANAGER_OR_APPROVED(); } @@ -135,18 +131,6 @@ contract MixinKeys is MixinErrors, MixinLockCore { "SCHEMA_VERSION_NOT_CORRECT" ); - // only for mainnet - if (block.chainid == 1) { - // Hardcoded address for the redeployed Unlock contract on mainnet - address newUnlockAddress = 0xe79B93f8E22676774F2A8dAd469175ebd00029FA; - - // trigger migration from the new Unlock - IUnlock(newUnlockAddress).postLockUpgrade(); - - // update unlock ref in this lock - unlockProtocol = IUnlock(newUnlockAddress); - } - // update data version schemaVersion = publicLockVersion(); } @@ -155,7 +139,7 @@ contract MixinKeys is MixinErrors, MixinLockCore { * Set the schema version to the latest * @notice only lock manager call call this */ - function updateSchemaVersion() public { + function updateSchemaVersion() internal { _onlyLockManager(); schemaVersion = publicLockVersion(); } @@ -494,19 +478,6 @@ contract MixinKeys is MixinErrors, MixinLockCore { return approvedRecipient; } - /** - * @dev Tells whether an operator is approved by a given keyManager - * @param _owner owner address which you want to query the approval of - * @param _operator operator address which you want to query the approval of - * @return bool whether the given operator is approved by the given owner - */ - function isApprovedForAll( - address _owner, - address _operator - ) public view returns (bool) { - return managerToOperatorApproved[_owner][_operator]; - } - /** * Returns true if _keyManager is explicitly set as key manager, or if the * address is the owner but no km is set. diff --git a/smart-contracts/contracts/mixins/MixinLockCore.sol b/smart-contracts/contracts/mixins/MixinLockCore.sol index 11bd556b8cd..feda0f17167 100644 --- a/smart-contracts/contracts/mixins/MixinLockCore.sol +++ b/smart-contracts/contracts/mixins/MixinLockCore.sol @@ -65,15 +65,6 @@ contract MixinLockCore is MixinRoles, MixinFunds, MixinDisable { address onKeyGrantHook ); - /** - * @dev Emitted when `owner` enables or disables (`approved`) `operator` to manage all of its assets. - */ - event ApprovalForAll( - address indexed owner, - address indexed operator, - bool approved - ); - // Unlock Protocol address IUnlock public unlockProtocol; @@ -103,7 +94,7 @@ contract MixinLockCore is MixinRoles, MixinFunds, MixinDisable { ILockTokenURIHook public onTokenURIHook; // use to check data version (added to v10) - uint public schemaVersion; + uint internal schemaVersion; // keep track of how many key a single address can use (added to v10) uint internal _maxKeysPerAddress; @@ -196,6 +187,12 @@ contract MixinLockCore is MixinRoles, MixinFunds, MixinDisable { emit PricingChanged(oldKeyPrice, keyPrice, oldTokenAddress, tokenAddress); } + function _isValidHook(address hookAddress, uint8 index) private view { + if (hookAddress != address(0) && !hookAddress.isContract()) { + revert INVALID_HOOK(index); + } + } + /** * @notice Allows a lock manager to add or remove an event hook */ @@ -210,27 +207,14 @@ contract MixinLockCore is MixinRoles, MixinFunds, MixinDisable { ) external { _onlyLockManager(); - if (_onKeyPurchaseHook != address(0) && !_onKeyPurchaseHook.isContract()) { - revert INVALID_HOOK(0); - } - if (_onKeyCancelHook != address(0) && !_onKeyCancelHook.isContract()) { - revert INVALID_HOOK(1); - } - if (_onValidKeyHook != address(0) && !_onValidKeyHook.isContract()) { - revert INVALID_HOOK(2); - } - if (_onTokenURIHook != address(0) && !_onTokenURIHook.isContract()) { - revert INVALID_HOOK(3); - } - if (_onKeyTransferHook != address(0) && !_onKeyTransferHook.isContract()) { - revert INVALID_HOOK(4); - } - if (_onKeyExtendHook != address(0) && !_onKeyExtendHook.isContract()) { - revert INVALID_HOOK(5); - } - if (_onKeyGrantHook != address(0) && !_onKeyGrantHook.isContract()) { - revert INVALID_HOOK(6); - } + // validate hooks + _isValidHook(_onKeyPurchaseHook, 0); + _isValidHook(_onKeyCancelHook, 1); + _isValidHook(_onValidKeyHook, 2); + _isValidHook(_onTokenURIHook, 3); + _isValidHook(_onKeyTransferHook, 4); + _isValidHook(_onKeyExtendHook, 5); + _isValidHook(_onKeyGrantHook, 6); onKeyPurchaseHook = ILockKeyPurchaseHook(_onKeyPurchaseHook); onKeyCancelHook = ILockKeyCancelHook(_onKeyCancelHook); diff --git a/smart-contracts/contracts/mixins/MixinRoles.sol b/smart-contracts/contracts/mixins/MixinRoles.sol index 1aafbdd9dec..68cbe2d4496 100644 --- a/smart-contracts/contracts/mixins/MixinRoles.sol +++ b/smart-contracts/contracts/mixins/MixinRoles.sol @@ -46,16 +46,5 @@ contract MixinRoles is AccessControlUpgradeable, MixinErrors { return hasRole(LOCK_MANAGER_ROLE, account); } - function addLockManager(address account) public { - _onlyLockManager(); - grantRole(LOCK_MANAGER_ROLE, account); - emit LockManagerAdded(account); - } - - function renounceLockManager() public { - renounceRole(LOCK_MANAGER_ROLE, msg.sender); - emit LockManagerRemoved(msg.sender); - } - uint256[1000] private __safe_upgrade_gap; } diff --git a/smart-contracts/contracts/mixins/MixinTransfer.sol b/smart-contracts/contracts/mixins/MixinTransfer.sol index fd470489409..665f8f0d038 100644 --- a/smart-contracts/contracts/mixins/MixinTransfer.sol +++ b/smart-contracts/contracts/mixins/MixinTransfer.sol @@ -113,7 +113,7 @@ contract MixinTransfer is * @param _recipient the address that will receive the token * @param _tokenId the id of the token * @dev Requirements: if the caller is not `from`, it must be approved to move this token by - * either `approve` or `setApprovalForAll`. + * `approve` * The key manager will be reset to address zero after the transfer */ function transferFrom( @@ -261,22 +261,6 @@ contract MixinTransfer is safeTransferFrom(_from, _to, _tokenId, ""); } - /** - * @dev Sets or unsets the approval of a given operator - * An operator is allowed to transfer all tokens of the sender on their behalf - * @param _to operator address to set the approval - * @param _approved representing the status of the approval to be set - * @notice disabled when transfers are disabled - */ - function setApprovalForAll(address _to, bool _approved) public { - _transferNotDisabled(); - if (_to == msg.sender) { - revert CANNOT_APPROVE_SELF(); - } - managerToOperatorApproved[msg.sender][_to] = _approved; - emit ApprovalForAll(msg.sender, _to, _approved); - } - /** * @notice Transfers the ownership of an NFT from one address to another address. * When transfer is complete, this functions diff --git a/smart-contracts/test/Kickback/kickback.js b/smart-contracts/test/Kickback/kickback.js index 32f5a731b52..aa1d61c4a9d 100644 --- a/smart-contracts/test/Kickback/kickback.js +++ b/smart-contracts/test/Kickback/kickback.js @@ -6,6 +6,7 @@ const { deployContracts, deployLock, purchaseKeys, + LOCK_MANAGER_ROLE, } = require('../helpers') describe('Kickback contract', () => { @@ -49,7 +50,7 @@ describe('Kickback contract', () => { it('should fail if callers is not a lock manager', async () => { const [, _randomSigner] = await ethers.getSigners() - await lock.addLockManager(await kickback.getAddress()) + await lock.grantRole(LOCK_MANAGER_ROLE, await kickback.getAddress()) await reverts( kickback.connect(_randomSigner).approveRefunds(lock, tree.root), `VM Exception while processing transaction: reverted with reason string 'You must be a lock manager to approve refunds.'` @@ -57,7 +58,7 @@ describe('Kickback contract', () => { }) it('should store the root of the merkle tree', async () => { const lockAddress = (await lock.getAddress()).toLowerCase() - await lock.addLockManager(await kickback.getAddress()) + await lock.grantRole(LOCK_MANAGER_ROLE, await kickback.getAddress()) await kickback.approveRefunds(lockAddress, tree.root) assert.equal(await kickback.roots(lockAddress), tree.root) }) @@ -66,7 +67,7 @@ describe('Kickback contract', () => { describe('refund', () => { let proof beforeEach(async () => { - await lock.addLockManager(await kickback.getAddress()) + await lock.grantRole(LOCK_MANAGER_ROLE, await kickback.getAddress()) await kickback.approveRefunds(lock, tree.root) for (const [i, v] of tree.entries()) { if (v[0] === keyOwners[0]) { diff --git a/smart-contracts/test/Lock/disableTransfers.js b/smart-contracts/test/Lock/disableTransfers.js index f3839307a47..02ed490acdc 100644 --- a/smart-contracts/test/Lock/disableTransfers.js +++ b/smart-contracts/test/Lock/disableTransfers.js @@ -51,18 +51,6 @@ describe('Lock / disableTransfers', () => { }) }) - describe('disabling setApprovalForAll', () => { - it('should prevent user from setting setApprovalForAll', async () => { - const [, , , , random] = await ethers.getSigners() - await reverts( - lock - .connect(keyOwner) - .setApprovalForAll(await random.getAddress(), true), - 'KEY_TRANSFERS_DISABLED' - ) - }) - }) - describe('disabling shareKey', () => { it('should prevent key sharing by reverting', async () => { // check owner has a key diff --git a/smart-contracts/test/Lock/erc721/approveForAll.js b/smart-contracts/test/Lock/erc721/approveForAll.js deleted file mode 100644 index f8e30433242..00000000000 --- a/smart-contracts/test/Lock/erc721/approveForAll.js +++ /dev/null @@ -1,192 +0,0 @@ -const assert = require('assert') -const { ethers } = require('hardhat') -const { deployLock, purchaseKey, reverts } = require('../../helpers') -const { getEvent } = require('@unlock-protocol/hardhat-helpers') - -let lock -let tokenId -let keyOwner, approvedAccount, newApprovedAccount, signers - -describe('Lock / erc721 / approveForAll', () => { - before(async () => { - lock = await deployLock() - ;[, keyOwner, approvedAccount, newApprovedAccount, ...signers] = - await ethers.getSigners() - await lock.updateTransferFee(0) // disable the transfer fee for this test - }) - - describe('when the key exists', () => { - before(async () => { - ;({ tokenId } = await purchaseKey(lock, await keyOwner.getAddress())) - }) - - it('isApprovedForAll defaults to false', async () => { - assert.equal( - await lock.isApprovedForAll( - await keyOwner.getAddress(), - await approvedAccount.getAddress() - ), - false - ) - }) - - describe('when the sender is self approving', () => { - it('should fail', async () => { - await reverts( - lock - .connect(keyOwner) - .setApprovalForAll(await keyOwner.getAddress(), true), - 'APPROVE_SELF' - ) - }) - }) - - describe('when the approval succeeds', () => { - let event - before(async () => { - const tx = await lock - .connect(keyOwner) - .setApprovalForAll(await approvedAccount.getAddress(), true) - const receipt = await tx.wait() - event = await getEvent(receipt, 'ApprovalForAll') - }) - - it('isApprovedForAll is true', async () => { - assert.equal( - await lock.isApprovedForAll( - await keyOwner.getAddress(), - await approvedAccount.getAddress() - ), - true - ) - }) - - it('should trigger the ApprovalForAll event', async () => { - assert.equal(event.event.fragment.name, 'ApprovalForAll') - assert.equal(event.args.owner, await keyOwner.getAddress()) - assert.equal(event.args.operator, await approvedAccount.getAddress()) - assert.equal(event.args.approved, true) - }) - - it('an authorized operator may set the approved address for an NFT', async () => { - await lock - .connect(approvedAccount) - .approve(await newApprovedAccount.getAddress(), tokenId) - assert.equal( - await lock.getApproved(tokenId), - await newApprovedAccount.getAddress() - ) - }) - - it('should allow the approved user to transferFrom', async () => { - await lock - .connect(approvedAccount) - .transferFrom( - await keyOwner.getAddress(), - await newApprovedAccount.getAddress(), - tokenId - ) - - // Transfer it back to the original keyOwner for other tests - await lock - .connect(newApprovedAccount) - .transferFrom( - await newApprovedAccount.getAddress(), - await keyOwner.getAddress(), - tokenId - ) - }) - - it('isApprovedForAll is still true (not lost after transfer)', async () => { - assert.equal( - await lock.isApprovedForAll( - await keyOwner.getAddress(), - await approvedAccount.getAddress() - ), - true - ) - }) - - describe('allows for multiple operators per keyOwner', () => { - before(async () => { - await lock - .connect(keyOwner) - .setApprovalForAll(await newApprovedAccount.getAddress(), true) - }) - - it('new operator is approved', async () => { - assert.equal( - await lock.isApprovedForAll( - await keyOwner.getAddress(), - await newApprovedAccount.getAddress() - ), - true - ) - }) - - it('original operator is still approved', async () => { - assert.equal( - await lock.isApprovedForAll( - await keyOwner.getAddress(), - await approvedAccount.getAddress() - ), - true - ) - }) - }) - }) - - describe('can cancel an outstanding approval', () => { - let event - - before(async () => { - // set key approval - await lock - .connect(keyOwner) - .setApprovalForAll(await approvedAccount.getAddress(), true) - // unset key approval - const tx = await lock - .connect(keyOwner) - .setApprovalForAll(await approvedAccount.getAddress(), false) - const receipt = await tx.wait() - event = await getEvent(receipt, 'ApprovalForAll') - }) - - it('isApprovedForAll is false again', async () => { - assert.equal( - await lock.isApprovedForAll( - await keyOwner.getAddress(), - await approvedAccount.getAddress() - ), - false - ) - }) - - it('This emits when an operator is (enabled or) disabled for an owner.', async () => { - assert.equal(event.event.fragment.name, 'ApprovalForAll') - assert.equal(event.args.owner, await keyOwner.getAddress()) - assert.equal(event.args.operator, await approvedAccount.getAddress()) - assert.equal(event.args.approved, false) - }) - }) - }) - - describe('when the owner does not have a key', () => { - it('allows the owner to call approveForAll', async () => { - const ownerWithoutAKey = signers[8] - // owner has no key - assert.equal(await lock.balanceOf(await ownerWithoutAKey.getAddress()), 0) - // approval works - await lock - .connect(ownerWithoutAKey) - .setApprovalForAll(await approvedAccount.getAddress(), true) - assert.equal( - await lock.isApprovedForAll( - await ownerWithoutAKey.getAddress(), - await approvedAccount.getAddress() - ), - true - ) - }) - }) -}) diff --git a/smart-contracts/test/Lock/erc721/transferFrom.js b/smart-contracts/test/Lock/erc721/transferFrom.js index d29598f6f1e..a7d95aa0fca 100644 --- a/smart-contracts/test/Lock/erc721/transferFrom.js +++ b/smart-contracts/test/Lock/erc721/transferFrom.js @@ -294,46 +294,6 @@ describe('Lock / erc721 / transferFrom', () => { }) }) - describe('when the sender is approved', async () => { - beforeEach(async () => { - await lock - .connect(keyOwner) - .setApprovalForAll(await approvedAllAccount.getAddress(), true) - assert.equal( - await lock.isApprovedForAll( - await keyOwner.getAddress(), - await approvedAllAccount.getAddress() - ), - true - ) - }) - it('should allow the transfer', async () => { - await lock - .connect(approvedAllAccount) - .transferFrom( - await keyOwner.getAddress(), - await keyRecipient.getAddress(), - tokenId - ) - assert.equal( - await lock.isApprovedForAll( - await keyOwner.getAddress(), - await approvedAllAccount.getAddress() - ), - true - ) - await reverts( - lock - .connect(approvedAllAccount) - .transferFrom( - await keyRecipient.getAddress(), - await approvedAllAccount.getAddress(), - tokenId - ) - ) - }) - }) - describe('when the lock is sold out', () => { it('should still allow the transfer of keys', async () => { // first we create a lock with only 1 key diff --git a/smart-contracts/test/Lock/gasRefund.js b/smart-contracts/test/Lock/gasRefund.js index 41008087c79..c678343b4ce 100644 --- a/smart-contracts/test/Lock/gasRefund.js +++ b/smart-contracts/test/Lock/gasRefund.js @@ -8,6 +8,7 @@ const { compareBigNumbers, purchaseKey, increaseTimeTo, + LOCK_MANAGER_ROLE, } = require('../helpers') const { getEvent } = require('@unlock-protocol/hardhat-helpers') @@ -118,7 +119,10 @@ describe('Lock / GasRefund', () => { }) it('can be set by lock manager', async () => { - await lock.addLockManager(await lockManager.getAddress()) + await lock.grantRole( + LOCK_MANAGER_ROLE, + await lockManager.getAddress() + ) await lock.connect(lockManager).setGasRefundValue(gasRefundAmount) compareBigNumbers(await lock.gasRefundValue(), gasRefundAmount) }) diff --git a/smart-contracts/test/Lock/initializers.js b/smart-contracts/test/Lock/initializers.js index 3ebec479270..cfcba745624 100644 --- a/smart-contracts/test/Lock/initializers.js +++ b/smart-contracts/test/Lock/initializers.js @@ -7,12 +7,13 @@ const { deployLock, deployContracts, parseInterface, + LOCK_MANAGER_ROLE, } = require('../helpers') describe('Lock / initializers', () => { - let unlockOwner, caller + let deployer, lockOwner, caller before(async () => { - ;[unlockOwner, , , , , , , caller] = await ethers.getSigners() + ;[deployer, lockOwner, , , , , , caller] = await ethers.getSigners() }) describe('initialize()', () => { @@ -28,14 +29,7 @@ describe('Lock / initializers', () => { it('may not be called again after deploying a lock', async () => { const lock = await deployLock() await reverts( - lock.initialize( - await unlockOwner.getAddress(), - 0, - ADDRESS_ZERO, - 0, - 0, - '' - ), + lock.initialize(await deployer.getAddress(), 0, ADDRESS_ZERO, 0, 0, ''), 'Initializable: contract is already initialized' ) }) @@ -50,7 +44,9 @@ describe('Lock / initializers', () => { const template = await PublicLock.deploy() await template.initialize(callerAddress, 0, ADDRESS_ZERO, 0, 0, '') await assert.equal(await template.isLockManager(callerAddress), true) - await template.connect(caller).renounceLockManager() + await template + .connect(caller) + .renounceRole(LOCK_MANAGER_ROLE, await caller.getAddress()) await assert.equal(await template.isLockManager(callerAddress), false) }) }) @@ -70,13 +66,15 @@ describe('Lock / initializers', () => { await template.publicLockVersion() ) + // unlock should not be lock manager anymore await assert.equal( await template.isLockManager(await unlock.getAddress()), false ) + await reverts( template.initialize( - await unlockOwner.getAddress(), + await lockOwner.getAddress(), 0, ADDRESS_ZERO, 0, @@ -101,7 +99,9 @@ describe('Lock / initializers', () => { 0, '' ) - await template.connect(caller).renounceLockManager() + await template + .connect(caller) + .renounceRole(LOCK_MANAGER_ROLE, await caller.getAddress()) await assert.equal( await template.isLockManager(await caller.getAddress()), false @@ -127,7 +127,7 @@ describe('Lock / initializers', () => { // cant be re-initialized await reverts( template.initialize( - await unlockOwner.getAddress(), + await deployer.getAddress(), 0, ADDRESS_ZERO, 0, diff --git a/smart-contracts/test/Lock/lendKey.js b/smart-contracts/test/Lock/lendKey.js index 112713f1d8a..d8962024a17 100644 --- a/smart-contracts/test/Lock/lendKey.js +++ b/smart-contracts/test/Lock/lendKey.js @@ -99,29 +99,6 @@ describe('Lock / lendKey', () => { 'UNAUTHORIZED' ) }) - - it('should fail if the sender has been approved for all owner keys', async () => { - await lock - .connect(keyOwner) - .setApprovalForAll(await accountApproved.getAddress(), true) - assert.equal( - await lock.isApprovedForAll( - await keyOwner.getAddress(), - await accountApproved.getAddress() - ), - true - ) - await reverts( - lock - .connect(accountApproved) - .lendKey( - await keyOwner.getAddress(), - await random.getAddress(), - tokenId - ), - 'UNAUTHORIZED' - ) - }) }) describe('when the sender is the key owner', () => { @@ -303,22 +280,6 @@ describe('Lock / lendKey', () => { 'ONLY_KEY_MANAGER_OR_APPROVED' ) }) - - it('can not used an "approved for all" account to transfer the key', async () => { - await lock - .connect(receiver) - .setApprovalForAll(await accountApproved.getAddress(), true) - await reverts( - lock - .connect(receiver) - .transferFrom( - await receiver.getAddress(), - await random.getAddress(), - tokenId - ), - 'ONLY_KEY_MANAGER_OR_APPROVED' - ) - }) }) describe('a lent key', () => { diff --git a/smart-contracts/test/Lock/transferFee.js b/smart-contracts/test/Lock/transferFee.js index 3bec10fdfac..21c78e19cd7 100644 --- a/smart-contracts/test/Lock/transferFee.js +++ b/smart-contracts/test/Lock/transferFee.js @@ -7,6 +7,7 @@ const { purchaseKey, compareBigNumbers, increaseTimeTo, + LOCK_MANAGER_ROLE, } = require('../helpers') describe('Lock / transferFee', () => { @@ -162,7 +163,7 @@ describe('Lock / transferFee', () => { before(async () => { // Change the fee to 0.25% await lock.updateTransferFee(25) - await lock.addLockManager(await lockManager.getAddress()) + await lock.grantRole(LOCK_MANAGER_ROLE, await lockManager.getAddress()) ;({ tokenId } = await purchaseKey(lock, await keyOwner.getAddress())) }) diff --git a/smart-contracts/test/Lock/updateKeyPricing.js b/smart-contracts/test/Lock/updateKeyPricing.js index 05855ab7832..92375012b67 100644 --- a/smart-contracts/test/Lock/updateKeyPricing.js +++ b/smart-contracts/test/Lock/updateKeyPricing.js @@ -7,6 +7,7 @@ const { deployERC20, ADDRESS_ZERO, compareBigNumbers, + LOCK_MANAGER_ROLE, } = require('../helpers') let lock @@ -101,7 +102,7 @@ describe('Lock / updateKeyPricing', () => { it('should allow a lock manager who is not the owner to make changes', async () => { await lock .connect(lockCreator) - .addLockManager(await lockManager.getAddress()) + .grantRole(LOCK_MANAGER_ROLE, await lockManager.getAddress()) assert.notEqual( await lockManager.getAddress(), await lockCreator.getAddress() @@ -125,7 +126,9 @@ describe('Lock / updateKeyPricing', () => { }) it('should allow a lockManager to renounce their role', async () => { - await lock.connect(lockManager).renounceLockManager() + await lock + .connect(lockManager) + .renounceRole(LOCK_MANAGER_ROLE, await lockManager.getAddress()) assert.equal( await lock.isLockManager(await lockManager.getAddress()), false diff --git a/smart-contracts/test/Unlock/createLockAtVersion.js b/smart-contracts/test/Unlock/createLockAtVersion.js index 519b90d9145..ad8fe159847 100644 --- a/smart-contracts/test/Unlock/createLockAtVersion.js +++ b/smart-contracts/test/Unlock/createLockAtVersion.js @@ -4,7 +4,7 @@ const { createLockCalldata, getEvent, } = require('@unlock-protocol/hardhat-helpers') -const { ADDRESS_ZERO, reverts } = require('../helpers') +const { ADDRESS_ZERO, reverts, LOCK_MANAGER_ROLE } = require('../helpers') const { keccak256, toUtf8Bytes } = require('ethers') // lock args const args = [ @@ -114,7 +114,7 @@ describe('Unlock / createUpgradeableLockAtVersion', () => { describe('with extra transactiions', () => { it('executes the extra transactions', async () => { - const [, lockManager] = await ethers.getSigners() + const [deployer, lockManager] = await ethers.getSigners() const calldata = await createLockCalldata({ args, @@ -127,12 +127,13 @@ describe('Unlock / createUpgradeableLockAtVersion', () => { ) const addLockManager = await publicLock.interface.encodeFunctionData( - 'addLockManager(address)', - [await lockManager.getAddress()] + 'grantRole(bytes32, address)', + [LOCK_MANAGER_ROLE, await lockManager.getAddress()] ) const renounceLockManager = await publicLock.interface.encodeFunctionData( - 'renounceLockManager()' + 'renounceRole(bytes32, address)', + [LOCK_MANAGER_ROLE, await unlock.getAddress()] ) const tx = await unlock[ @@ -171,6 +172,7 @@ describe('Unlock / createUpgradeableLockAtVersion', () => { ).to.equal(false) }) it('fails to deploy if one of the transaction fails', async () => { + const [deployer] = await ethers.getSigners() const calldata = await createLockCalldata({ args, from: await unlock.getAddress(), @@ -182,7 +184,8 @@ describe('Unlock / createUpgradeableLockAtVersion', () => { ) const renounceLockManager = await publicLock.interface.encodeFunctionData( - 'renounceLockManager()' + 'renounceRole(bytes32, address)', + [LOCK_MANAGER_ROLE, await unlock.getAddress()] ) // Renouncing first will fail the 2nd call!