diff --git a/contracts/ERC7432/NftRolesRegistryVault.sol b/contracts/ERC7432/NftRolesRegistryVault.sol index 9a97b3f..23a3527 100644 --- a/contracts/ERC7432/NftRolesRegistryVault.sol +++ b/contracts/ERC7432/NftRolesRegistryVault.sol @@ -13,6 +13,11 @@ contract NftRolesRegistryVault is IERC7432 { bytes data; } + bytes32[] public allowedRoles; + + // roleId => isAllowed + mapping(bytes32 => bool) public isRoleAllowed; + // tokenAddress => tokenId => owner mapping(address => mapping(uint256 => address)) public originalOwners; @@ -22,9 +27,21 @@ contract NftRolesRegistryVault is IERC7432 { // owner => tokenAddress => operator => isApproved mapping(address => mapping(address => mapping(address => bool))) public tokenApprovals; + constructor() { + allowedRoles = [keccak256('UNIQUE_ROLE')]; + for (uint256 i = 0; i < allowedRoles.length; i++) { + isRoleAllowed[allowedRoles[i]] = true; + } + } + + modifier onlyAllowedRole(bytes32 _roleId) { + require(isRoleAllowed[_roleId], 'NftRolesRegistryVault: role is not allowed'); + _; + } + /** External Functions **/ - function grantRole(IERC7432.Role calldata _role) external override { + function grantRole(IERC7432.Role calldata _role) external override onlyAllowedRole(_role.roleId) { require(_role.expirationDate > block.timestamp, 'NftRolesRegistryVault: expiration date must be in the future'); // deposit NFT if necessary @@ -57,7 +74,11 @@ contract NftRolesRegistryVault is IERC7432 { ); } - function revokeRole(address _tokenAddress, uint256 _tokenId, bytes32 _roleId) external override { + function revokeRole( + address _tokenAddress, + uint256 _tokenId, + bytes32 _roleId + ) external override onlyAllowedRole(_roleId) { address _recipient = roles[_tokenAddress][_tokenId][_roleId].recipient; address _caller = _getApprovedCaller(_tokenAddress, _tokenId, _recipient); @@ -78,7 +99,7 @@ contract NftRolesRegistryVault is IERC7432 { function unlockToken(address _tokenAddress, uint256 _tokenId) external override { address originalOwner = originalOwners[_tokenAddress][_tokenId]; - require(_isLocked(_tokenAddress, _tokenId), 'NftRolesRegistryVault: NFT is locked'); + require(!_hasNonRevocableRole(_tokenAddress, _tokenId), 'NftRolesRegistryVault: NFT is locked'); require( originalOwner == msg.sender || isRoleApprovedForAll(_tokenAddress, originalOwner, msg.sender), @@ -106,10 +127,7 @@ contract NftRolesRegistryVault is IERC7432 { uint256 _tokenId, bytes32 _roleId ) external view returns (address recipient_) { - if ( - _isTokenDeposited(_tokenAddress, _tokenId) && - roles[_tokenAddress][_tokenId][_roleId].expirationDate > block.timestamp - ) { + if (roles[_tokenAddress][_tokenId][_roleId].expirationDate > block.timestamp) { return roles[_tokenAddress][_tokenId][_roleId].recipient; } return address(0); @@ -120,10 +138,10 @@ contract NftRolesRegistryVault is IERC7432 { uint256 _tokenId, bytes32 _roleId ) external view returns (bytes memory data_) { - if (!_isTokenDeposited(_tokenAddress, _tokenId)) { - return ''; + if (roles[_tokenAddress][_tokenId][_roleId].expirationDate > block.timestamp) { + data_ = roles[_tokenAddress][_tokenId][_roleId].data; } - return roles[_tokenAddress][_tokenId][_roleId].data; + return data_; } function roleExpirationDate( @@ -131,10 +149,10 @@ contract NftRolesRegistryVault is IERC7432 { uint256 _tokenId, bytes32 _roleId ) external view returns (uint64 expirationDate_) { - if (!_isTokenDeposited(_tokenAddress, _tokenId)) { - return 0; + if (roles[_tokenAddress][_tokenId][_roleId].expirationDate > block.timestamp) { + return roles[_tokenAddress][_tokenId][_roleId].expirationDate; } - return roles[_tokenAddress][_tokenId][_roleId].expirationDate; + return 0; } function isRoleRevocable( @@ -142,10 +160,9 @@ contract NftRolesRegistryVault is IERC7432 { uint256 _tokenId, bytes32 _roleId ) external view returns (bool revocable_) { - if (!_isTokenDeposited(_tokenAddress, _tokenId)) { - return false; - } - return roles[_tokenAddress][_tokenId][_roleId].revocable; + return + roles[_tokenAddress][_tokenId][_roleId].expirationDate > block.timestamp && + roles[_tokenAddress][_tokenId][_roleId].revocable; } function isRoleApprovedForAll(address _tokenAddress, address _owner, address _operator) public view returns (bool) { @@ -207,21 +224,19 @@ contract NftRolesRegistryVault is IERC7432 { revert('NftRolesRegistryVault: role does not exist or sender is not approved'); } - /// @notice Checks if an NFT is locked. - /// @param _tokenAddress The token address. - /// @param _tokenId The token identifier. - /// @return True if the NFT is locked. - function _isLocked(address _tokenAddress, uint256 _tokenId) internal view returns (bool) { - // todo needs to implement a way to track expiration dates to make sure NFTs are not locked - // mocked result - return _isTokenDeposited(_tokenAddress, _tokenId); - } - - /// @notice Checks if the NFT is deposited on this contract. + /// @notice Checks whether an NFT has at least one non-revocable role. /// @param _tokenAddress The token address. /// @param _tokenId The token identifier. - /// @return deposited_ Whether the NFT is deposited or not. - function _isTokenDeposited(address _tokenAddress, uint256 _tokenId) internal view returns (bool) { - return originalOwners[_tokenAddress][_tokenId] != address(0); + /// @return true if the NFT is locked. + function _hasNonRevocableRole(address _tokenAddress, uint256 _tokenId) internal view returns (bool) { + for (uint256 i = 0; i < allowedRoles.length; i++) { + if ( + !roles[_tokenAddress][_tokenId][allowedRoles[i]].revocable && + roles[_tokenAddress][_tokenId][allowedRoles[i]].expirationDate > block.timestamp + ) { + return true; + } + } + return false; } } diff --git a/contracts/interfaces/IERC7432.sol b/contracts/interfaces/IERC7432.sol index beba086..af0c0d2 100644 --- a/contracts/interfaces/IERC7432.sol +++ b/contracts/interfaces/IERC7432.sol @@ -67,10 +67,12 @@ interface IERC7432 is IERC165 { /** External Functions **/ /// @notice Grants a role to a user. + /// @dev Reverts if sender is not approved or the NFT owner. /// @param _role The role attributes. function grantRole(Role calldata _role) external; /// @notice Revokes a role from a user. + /// @dev Reverts if sender is not approved or the original owner. /// @param _tokenAddress The token address. /// @param _tokenId The token identifier. /// @param _roleId The role identifier. diff --git a/test/ERC7432/NftRolesRegistryVault.ts b/test/ERC7432/NftRolesRegistryVault.ts index 3015561..e83f501 100644 --- a/test/ERC7432/NftRolesRegistryVault.ts +++ b/test/ERC7432/NftRolesRegistryVault.ts @@ -30,9 +30,9 @@ describe('NftRolesRegistryVault', () => { anotherUser = signers[2] } - async function depositNftAndGrantRole({ recipient = AddressZero }) { + async function depositNftAndGrantRole({ recipient = AddressZero, revocable = role.revocable }) { await MockErc721Token.connect(owner).approve(NftRolesRegistryVault.address, role.tokenId) - await expect(NftRolesRegistryVault.connect(owner).grantRole({ ...role, recipient })) + await expect(NftRolesRegistryVault.connect(owner).grantRole({ ...role, recipient, revocable })) .to.emit(NftRolesRegistryVault, 'RoleGranted') .withArgs( role.tokenAddress, @@ -41,7 +41,7 @@ describe('NftRolesRegistryVault', () => { owner.address, recipient, role.expirationDate, - role.revocable, + revocable, role.data, ) .to.emit(MockErc721Token, 'Transfer') @@ -265,37 +265,42 @@ describe('NftRolesRegistryVault', () => { }) describe('unlockToken', () => { - beforeEach(async () => { - await depositNftAndGrantRole({ recipient: recipient.address }) + describe('when NFT is not deposited', () => { + it('should revert if token is not deposited', async () => { + await depositNftAndGrantRole({ recipient: recipient.address, revocable: false }) + await expect( + NftRolesRegistryVault.connect(owner).unlockToken(role.tokenAddress, role.tokenId), + ).to.be.revertedWith('NftRolesRegistryVault: NFT is locked') + }) }) - it('should revert if token is not deposited', async () => { - await expect( - NftRolesRegistryVault.connect(owner).unlockToken(role.tokenAddress, role.tokenId + 1), - ).to.be.revertedWith('NftRolesRegistryVault: NFT is locked') - }) + describe('when NFT is deposited', () => { + beforeEach(async () => { + await depositNftAndGrantRole({ recipient: recipient.address }) + }) - it('should revert if sender is not original owner or approved', async () => { - await expect( - NftRolesRegistryVault.connect(anotherUser).unlockToken(role.tokenAddress, role.tokenId), - ).to.be.revertedWith('NftRolesRegistryVault: sender must be owner or approved') - }) + it('should revert if sender is not original owner or approved', async () => { + await expect( + NftRolesRegistryVault.connect(anotherUser).unlockToken(role.tokenAddress, role.tokenId), + ).to.be.revertedWith('NftRolesRegistryVault: sender must be owner or approved') + }) - it('should unlock token if sender is owner and NFT is not locked', async () => { - await expect(NftRolesRegistryVault.connect(owner).unlockToken(role.tokenAddress, role.tokenId)) - .to.emit(NftRolesRegistryVault, 'TokenUnlocked') - .withArgs(owner.address, role.tokenAddress, role.tokenId) - .to.emit(MockErc721Token, 'Transfer') - .withArgs(NftRolesRegistryVault.address, owner.address, role.tokenId) - }) + it('should unlock token if sender is owner and NFT is not locked', async () => { + await expect(NftRolesRegistryVault.connect(owner).unlockToken(role.tokenAddress, role.tokenId)) + .to.emit(NftRolesRegistryVault, 'TokenUnlocked') + .withArgs(owner.address, role.tokenAddress, role.tokenId) + .to.emit(MockErc721Token, 'Transfer') + .withArgs(NftRolesRegistryVault.address, owner.address, role.tokenId) + }) - it('should unlock token if sender is approved and NFT is not locked', async () => { - await NftRolesRegistryVault.connect(owner).setRoleApprovalForAll(role.tokenAddress, anotherUser.address, true) - await expect(NftRolesRegistryVault.connect(anotherUser).unlockToken(role.tokenAddress, role.tokenId)) - .to.emit(NftRolesRegistryVault, 'TokenUnlocked') - .withArgs(owner.address, role.tokenAddress, role.tokenId) - .to.emit(MockErc721Token, 'Transfer') - .withArgs(NftRolesRegistryVault.address, owner.address, role.tokenId) + it('should unlock token if sender is approved and NFT is not locked', async () => { + await NftRolesRegistryVault.connect(owner).setRoleApprovalForAll(role.tokenAddress, anotherUser.address, true) + await expect(NftRolesRegistryVault.connect(anotherUser).unlockToken(role.tokenAddress, role.tokenId)) + .to.emit(NftRolesRegistryVault, 'TokenUnlocked') + .withArgs(owner.address, role.tokenAddress, role.tokenId) + .to.emit(MockErc721Token, 'Transfer') + .withArgs(NftRolesRegistryVault.address, owner.address, role.tokenId) + }) }) }) diff --git a/test/ERC7432/mockData.ts b/test/ERC7432/mockData.ts index 293779c..347a310 100644 --- a/test/ERC7432/mockData.ts +++ b/test/ERC7432/mockData.ts @@ -14,8 +14,14 @@ export async function buildRole({ revocable = true, data = HashZero, }): Promise { + let actualRoleId = ROLE + if (roleId && !roleId.startsWith('0x')) { + actualRoleId = generateRoleId(roleId) + } else { + actualRoleId = roleId + } return { - roleId: generateRoleId(roleId), + roleId: actualRoleId, tokenAddress: ethers.utils.getAddress(tokenAddress), tokenId, recipient,