Skip to content

Commit

Permalink
ON-793: refactor NftRolesRegistryVault (#32)
Browse files Browse the repository at this point in the history 
* ON-793: refactor NftRolesRegistryVault

* ON-793: fixed _hasNonRevocableRole function

* ON-793: returning variable instead of constant
  • Loading branch information
@ernanirst
ernanirst authored May 10, 2024
1 parent 1c7560f commit 8e380ba
Show file tree
Hide file tree
Showing 4 changed files with 89 additions and 61 deletions.
77 changes: 46 additions & 31 deletions contracts/ERC7432/NftRolesRegistryVault.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand All @@ -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
Expand Down Expand Up @@ -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);

Expand All @@ -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),
Expand Down Expand Up @@ -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);
Expand All @@ -120,32 +138,31 @@ 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(
address _tokenAddress,
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(
address _tokenAddress,
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) {
Expand Down Expand Up @@ -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;
}
}
2 changes: 2 additions & 0 deletions contracts/interfaces/IERC7432.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
63 changes: 34 additions & 29 deletions test/ERC7432/NftRolesRegistryVault.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -41,7 +41,7 @@ describe('NftRolesRegistryVault', () => {
owner.address,
recipient,
role.expirationDate,
role.revocable,
revocable,
role.data,
)
.to.emit(MockErc721Token, 'Transfer')
Expand Down Expand Up @@ -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)
})
})
})

Expand Down
8 changes: 7 additions & 1 deletion test/ERC7432/mockData.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,14 @@ export async function buildRole({
revocable = true,
data = HashZero,
}): Promise<Role> {
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,
Expand Down

0 comments on commit 8e380ba

Please sign in to comment.