fixed revokeRoleFrom and included tests

contracts/RolesRegistry/SftRolesRegistry.sol

@@ -149,25 +149,37 @@ contract SftRolesRegistry is IERCXXXX, ERC1155Holder {
 
     function revokeRoleFrom(RevokeRoleData calldata _revokeRoleData) external override {
         LinkedLists.ListItem storage item = lists.items[_revokeRoleData.nonce];
-        require(item.data.hash == _hashRoleData(_revokeRoleData), "SftRolesRegistry: invalid revoke role data");
+        require(item.data.hash == _hashRoleData(_revokeRoleData), "SftRolesRegistry: could not find role assignment");
 
         address caller = _findCaller(_revokeRoleData);
-        if (!item.data.revocable) {
-            require(caller == _revokeRoleData.grantee, "SftRolesRegistry: Role is not revocable or caller is not the approved");
+        if (item.data.expirationDate > block.timestamp && !item.data.revocable) {
+            // if role is not expired and is not revocable, only the grantee can revoke it
+            require(caller == _revokeRoleData.grantee, "SftRolesRegistry: role is not revocable or caller is not the approved");
         }
 
+        uint256 tokensToReturn = item.data.tokenAmount;
         _transferFrom(
             address(this),
             _revokeRoleData.revoker,
             _revokeRoleData.tokenAddress,
             _revokeRoleData.tokenId,
-            item.data.tokenAmount
+            tokensToReturn
         );
 
         bytes32 rootKey = _getHeadKey(_revokeRoleData.grantee, _revokeRoleData.role, _revokeRoleData.tokenAddress, _revokeRoleData.tokenId);
 
         // remove from the list
         lists.remove(rootKey, _revokeRoleData.nonce);
+
+        emit RoleRevoked(
+            _revokeRoleData.nonce,
+            _revokeRoleData.role,
+            _revokeRoleData.tokenAddress,
+            _revokeRoleData.tokenId,
+            tokensToReturn,
+            _revokeRoleData.revoker,
+            _revokeRoleData.grantee
+        );
     }
 
     function setRoleApprovalForAll(address _tokenAddress, address _operator, bool _isApproved) external override {
@@ -239,16 +251,6 @@ contract SftRolesRegistry is IERCXXXX, ERC1155Holder {
         );
     }
 
-    function _hashRoleData(RoleAssignment calldata _roleAssignment) internal pure returns (bytes32) {
-        return _hashRoleData(
-            _roleAssignment.nonce,
-            _roleAssignment.role,
-            _roleAssignment.tokenAddress,
-            _roleAssignment.tokenId,
-            _roleAssignment.grantor
-        );
-    }
-
     function _hashRoleData(
         uint256 _nonce, bytes32 _role, address _tokenAddress, uint256 _tokenId, address _grantor
     ) internal pure returns (bytes32) {
@@ -262,7 +264,8 @@ contract SftRolesRegistry is IERCXXXX, ERC1155Holder {
             return _revokeRoleData.revoker;
         }
 
-        if (_revokeRoleData.grantee == msg.sender ||
+        if (
+            _revokeRoleData.grantee == msg.sender ||
             isRoleApprovedForAll(_revokeRoleData.tokenAddress, _revokeRoleData.grantee, msg.sender)
         ) {
             return _revokeRoleData.grantee;

test/SftRolesRegistry/SftRolesRegistry.spec.ts

@@ -2,10 +2,11 @@ import { ethers } from 'hardhat'
 import { Contract } from 'ethers'
 import { beforeEach } from 'mocha'
 import { expect } from 'chai'
+import { solidityKeccak256 } from 'ethers/lib/utils'
 import { loadFixture } from '@nomicfoundation/hardhat-network-helpers'
 import { SignerWithAddress } from '@nomiclabs/hardhat-ethers/signers'
-import { buildRoleAssignment, currentUnixTimestamp, ONE_DAY, generateRoleId } from './helpers'
-import { RoleAssignment } from './types'
+import { buildRoleAssignment, currentUnixTimestamp, ONE_DAY, generateRoleId, buildRevokeRoleData } from './helpers'
+import { RoleAssignment, RevokeRoleData } from './types'
 import { generateRandomInt } from '../helpers'
 
 const { AddressZero } = ethers.constants
@@ -14,6 +15,7 @@ describe('SftRolesRegistry', async () => {
   let SftRolesRegistry: Contract
   let MockToken: Contract
   let grantor: SignerWithAddress
+  let grantee: SignerWithAddress
   let anotherUser: SignerWithAddress
 
   async function deployContracts() {
@@ -23,7 +25,8 @@ describe('SftRolesRegistry', async () => {
     MockToken = await MockTokenFactory.deploy()
     const signers = await ethers.getSigners()
     grantor = signers[0]
-    anotherUser = signers[1]
+    grantee = signers[1]
+    anotherUser = signers[2]
     return { SftRolesRegistry, MockToken, signers }
   }
 
@@ -346,4 +349,188 @@ describe('SftRolesRegistry', async () => {
         .to.not.emit(MockToken, 'TransferSingle')
     })
   })
+
+  describe('revokeRole', async () => {
+    let RoleAssignment: RoleAssignment
+    let RevokeRoleData: RevokeRoleData
+
+    beforeEach(async () => {
+      RoleAssignment = await buildRoleAssignment({
+        tokenAddress: MockToken.address,
+        grantor: grantor.address,
+        grantee: grantee.address,
+      })
+      RevokeRoleData = buildRevokeRoleData(RoleAssignment)
+      await MockToken.connect(grantor).setApprovalForAll(SftRolesRegistry.address, true)
+      await MockToken.mint(grantor.address, RoleAssignment.tokenId, RoleAssignment.tokenAmount)
+      await expect(SftRolesRegistry.connect(grantor).grantRoleFrom(RoleAssignment)).to.not.be.reverted
+    })
+
+    it('should revert when hash is invalid', async () => {
+      // hash validates nonce, role, tokenAddress, tokenId and revoker
+
+      // nonce
+      await expect(
+        SftRolesRegistry.connect(grantor).revokeRoleFrom({
+          ...RevokeRoleData,
+          nonce: generateRandomInt(),
+        }),
+      ).to.be.revertedWith('SftRolesRegistry: could not find role assignment')
+
+      // role
+      await expect(
+        SftRolesRegistry.connect(grantor).revokeRoleFrom({
+          ...RevokeRoleData,
+          role: solidityKeccak256(['string'], ['Role(uint256 newArg)']),
+        }),
+      ).to.be.revertedWith('SftRolesRegistry: could not find role assignment')
+
+      // tokenAddress
+      await expect(
+        SftRolesRegistry.connect(grantor).revokeRoleFrom({
+          ...RevokeRoleData,
+          tokenAddress: AddressZero,
+        }),
+      ).to.be.revertedWith('SftRolesRegistry: could not find role assignment')
+
+      // tokenId
+      await expect(
+        SftRolesRegistry.connect(grantor).revokeRoleFrom({
+          ...RevokeRoleData,
+          tokenId: generateRandomInt(),
+        }),
+      ).to.be.revertedWith('SftRolesRegistry: could not find role assignment')
+
+      // revoker
+      await expect(
+        SftRolesRegistry.connect(grantor).revokeRoleFrom({
+          ...RevokeRoleData,
+          revoker: anotherUser.address,
+        }),
+      ).to.be.revertedWith('SftRolesRegistry: could not find role assignment')
+    })
+
+    it('should revert if nonce is not expired and is not revocable', async () => {
+      const newRoleAssignment = await buildRoleAssignment({
+        tokenAddress: MockToken.address,
+        grantor: grantor.address,
+        revocable: false,
+      })
+
+      const newRevokeRoleData = buildRevokeRoleData(newRoleAssignment)
+      await MockToken.mint(newRoleAssignment.grantor, newRoleAssignment.tokenId, newRoleAssignment.tokenAmount)
+      await expect(SftRolesRegistry.connect(grantor).grantRoleFrom(newRoleAssignment))
+
+      await expect(SftRolesRegistry.connect(grantor).revokeRoleFrom(newRevokeRoleData)).to.be.revertedWith(
+        'SftRolesRegistry: role is not revocable or caller is not the approved',
+      )
+    })
+
+    it('should revert if caller is not approved', async () => {
+      await expect(SftRolesRegistry.connect(anotherUser).revokeRoleFrom(RevokeRoleData)).to.be.revertedWith(
+        'SftRolesRegistry: sender must be approved',
+      )
+    })
+
+    it('should revoke role if sender is revoker', async () => {
+      await expect(SftRolesRegistry.connect(grantor).revokeRoleFrom(RevokeRoleData))
+        .to.emit(SftRolesRegistry, 'RoleRevoked')
+        .withArgs(
+          RevokeRoleData.nonce,
+          RevokeRoleData.role,
+          RevokeRoleData.tokenAddress,
+          RevokeRoleData.tokenId,
+          RoleAssignment.tokenAmount,
+          RevokeRoleData.revoker,
+          RevokeRoleData.grantee,
+        )
+        // transfer tokens back to owner
+        .to.emit(MockToken, 'TransferSingle')
+        .withArgs(
+          SftRolesRegistry.address,
+          SftRolesRegistry.address,
+          RevokeRoleData.revoker,
+          RevokeRoleData.tokenId,
+          RoleAssignment.tokenAmount,
+        )
+    })
+
+    it('should revoke role if sender is approved by grantor', async () => {
+      await SftRolesRegistry.connect(grantor).setRoleApprovalForAll(
+        RoleAssignment.tokenAddress,
+        anotherUser.address,
+        true,
+      )
+      await expect(SftRolesRegistry.connect(anotherUser).revokeRoleFrom(RevokeRoleData))
+        .to.emit(SftRolesRegistry, 'RoleRevoked')
+        .withArgs(
+          RevokeRoleData.nonce,
+          RevokeRoleData.role,
+          RevokeRoleData.tokenAddress,
+          RevokeRoleData.tokenId,
+          RoleAssignment.tokenAmount,
+          RevokeRoleData.revoker,
+          RevokeRoleData.grantee,
+        )
+        // transfer tokens back to owner
+        .to.emit(MockToken, 'TransferSingle')
+        .withArgs(
+          SftRolesRegistry.address,
+          SftRolesRegistry.address,
+          RevokeRoleData.revoker,
+          RevokeRoleData.tokenId,
+          RoleAssignment.tokenAmount,
+        )
+    })
+    it('should revoke role if sender is approved by grantee', async () => {
+      await SftRolesRegistry.connect(grantee).setRoleApprovalForAll(
+        RoleAssignment.tokenAddress,
+        anotherUser.address,
+        true,
+      )
+      await expect(SftRolesRegistry.connect(anotherUser).revokeRoleFrom(RevokeRoleData))
+        .to.emit(SftRolesRegistry, 'RoleRevoked')
+        .withArgs(
+          RevokeRoleData.nonce,
+          RevokeRoleData.role,
+          RevokeRoleData.tokenAddress,
+          RevokeRoleData.tokenId,
+          RoleAssignment.tokenAmount,
+          RevokeRoleData.revoker,
+          RevokeRoleData.grantee,
+        )
+        // transfer tokens back to owner
+        .to.emit(MockToken, 'TransferSingle')
+        .withArgs(
+          SftRolesRegistry.address,
+          SftRolesRegistry.address,
+          RevokeRoleData.revoker,
+          RevokeRoleData.tokenId,
+          RoleAssignment.tokenAmount,
+        )
+    })
+
+    it('should revoke role if sender is grantee', async () => {
+      await expect(SftRolesRegistry.connect(grantee).revokeRoleFrom(RevokeRoleData))
+        .to.emit(SftRolesRegistry, 'RoleRevoked')
+        .withArgs(
+          RevokeRoleData.nonce,
+          RevokeRoleData.role,
+          RevokeRoleData.tokenAddress,
+          RevokeRoleData.tokenId,
+          RoleAssignment.tokenAmount,
+          RevokeRoleData.revoker,
+          RevokeRoleData.grantee,
+        )
+        // transfer tokens back to owner
+        .to.emit(MockToken, 'TransferSingle')
+        .withArgs(
+          SftRolesRegistry.address,
+          SftRolesRegistry.address,
+          RevokeRoleData.revoker,
+          RevokeRoleData.tokenId,
+          RoleAssignment.tokenAmount,
+        )
+    })
+  })
 })

test/SftRolesRegistry/helpers.ts

@@ -1,5 +1,5 @@
 import { solidityKeccak256 } from 'ethers/lib/utils'
-import { RoleAssignment } from './types'
+import { RevokeRoleData, RoleAssignment } from './types'
 import { generateRandomInt } from '../helpers'
 import { ethers } from 'hardhat'
 import { time } from '@nomicfoundation/hardhat-network-helpers'
@@ -46,6 +46,17 @@ export async function buildRoleAssignment({
   }
 }
 
+export function buildRevokeRoleData(roleAssignment: RoleAssignment): RevokeRoleData {
+  return {
+    nonce: roleAssignment.nonce,
+    role: roleAssignment.role,
+    tokenAddress: roleAssignment.tokenAddress,
+    tokenId: roleAssignment.tokenId,
+    revoker: roleAssignment.grantor,
+    grantee: roleAssignment.grantee,
+  }
+}
+
 export function generateRoleId(role: string) {
   return solidityKeccak256(['string'], [role])
 }

test/SftRolesRegistry/types.ts

@@ -10,3 +10,12 @@ export interface RoleAssignment {
   revocable: boolean
   data: string
 }
+
+export interface RevokeRoleData {
+  nonce: number
+  role: string
+  tokenAddress: string
+  tokenId: number
+  revoker: string
+  grantee: string
+}