Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ON-793: refactor NftRolesRegistryVault #32

Merged
merged 3 commits into from
May 10, 2024
Merged

ON-793: refactor NftRolesRegistryVault #32

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c77f75f
ON-793: refactor NftRolesRegistryVault
ernanirst May 9, 2024
9532d9d
ON-793: fixed _hasNonRevocableRole function
ernanirst May 9, 2024
c549f49
ON-793: returning variable instead of constant
ernanirst May 9, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
77 changes: 46 additions & 31 deletions contracts/ERC7432/NftRolesRegistryVault.sol
View file
Open in desktop
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;
}
ernanirst marked this conversation as resolved.
Show resolved Hide resolved
}

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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Loading