From 9532d9db512a77f3bc01ade7a32a5363563863d5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ernani=20S=C3=A3o=20Thiago?= Date: Thu, 9 May 2024 16:35:11 -0600 Subject: [PATCH] ON-793: fixed _hasNonRevocableRole function --- contracts/ERC7432/NftRolesRegistryVault.sol | 23 ++++---- test/ERC7432/NftRolesRegistryVault.ts | 63 +++++++++++---------- 2 files changed, 47 insertions(+), 39 deletions(-) diff --git a/contracts/ERC7432/NftRolesRegistryVault.sol b/contracts/ERC7432/NftRolesRegistryVault.sol index 1fa5ae2..4facb3f 100644 --- a/contracts/ERC7432/NftRolesRegistryVault.sol +++ b/contracts/ERC7432/NftRolesRegistryVault.sol @@ -13,7 +13,7 @@ contract NftRolesRegistryVault is IERC7432 { bytes data; } - bytes32[] public allowedRoles = [keccak256('UNIQUE_ROLE')]; + bytes32[] public allowedRoles; // roleId => isAllowed mapping(bytes32 => bool) public isRoleAllowed; @@ -28,6 +28,7 @@ contract NftRolesRegistryVault is IERC7432 { 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; } @@ -98,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), @@ -159,10 +160,9 @@ contract NftRolesRegistryVault is IERC7432 { uint256 _tokenId, bytes32 _roleId ) external view returns (bool revocable_) { - if (roles[_tokenAddress][_tokenId][_roleId].expirationDate > block.timestamp) { - return roles[_tokenAddress][_tokenId][_roleId].revocable; - } - return false; + 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) { @@ -224,13 +224,16 @@ contract NftRolesRegistryVault is IERC7432 { revert('NftRolesRegistryVault: role does not exist or sender is not approved'); } - /// @notice Checks whether an NFT is locked. + /// @notice Checks whether an NFT has at least one non-revocable role. /// @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) { + /// @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]].expirationDate > block.timestamp) { + if ( + !roles[_tokenAddress][_tokenId][allowedRoles[i]].revocable && + roles[_tokenAddress][_tokenId][allowedRoles[i]].expirationDate > block.timestamp + ) { return true; } } 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) + }) }) })