Skip to content

Commit

Permalink
ON-483: Add tests
Browse files Browse the repository at this point in the history
  • Loading branch information
Daniel Lima committed Sep 25, 2023
1 parent 131bdb3 commit 684e226
Show file tree
Hide file tree
Showing 2 changed files with 113 additions and 37 deletions.
84 changes: 48 additions & 36 deletions contracts/RolesRegistry/RolesRegistry.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,11 @@ contract RolesRegistry is IERC7432 {
_;
}

modifier onlyApproved(address _tokenAddress, uint256 _tokenId, address _account) {
modifier onlyApproved(
address _tokenAddress,
uint256 _tokenId,
address _account
) {
require(
_isRoleApproved(_tokenAddress, _tokenId, _account, msg.sender),
"RolesRegistry: sender must be approved"
Expand All @@ -34,19 +38,7 @@ contract RolesRegistry is IERC7432 {
}

modifier onlyTokenOwner(address _tokenAddress, uint256 _tokenId) {
if (isERC1155(_tokenAddress)) {
require(
IERC1155(_tokenAddress).balanceOf(msg.sender, _tokenId) > 0,
"OriumMarketplace: only token owner can call this function"
);
} else if(isERC721(_tokenAddress)) {
require(
msg.sender == IERC721(_tokenAddress).ownerOf(_tokenId),
"OriumMarketplace: only token owner can call this function"
);
} else {
revert("OriumMarketplace: token address is not ERC1155 or ERC721");
}
require(_isOwner(_tokenAddress, _tokenId, msg.sender), "RolesRegistry: sender must be token owner");
_;
}

Expand All @@ -72,6 +64,7 @@ contract RolesRegistry is IERC7432 {
bool _revocable,
bytes calldata _data
) external override onlyApproved(_tokenAddress, _tokenId, _grantor) {
require(_isOwner(_tokenAddress, _tokenId, _grantor), "RolesRegistry: account must be owner");
_grantRole(_role, _tokenAddress, _tokenId, _grantor, _grantee, _expirationDate, _revocable, _data);
}

Expand All @@ -85,13 +78,18 @@ contract RolesRegistry is IERC7432 {
bool _revocable,
bytes calldata _data
) internal validExpirationDate(_expirationDate) {
// TODO: How to handle the case where the role is already granted and not expired?
// TODO: How to handle the case where the role is already granted and not expired ?
roleAssignments[_grantee][_tokenAddress][_tokenId][_role] = RoleData(_expirationDate, _revocable, _data);
latestGrantees[_tokenAddress][_tokenId][_role] = _grantee;
emit RoleGranted(_role, _tokenAddress, _tokenId, _grantor, _grantee, _expirationDate, _revocable, _data);
}

function revokeRole(bytes32 _role, address _tokenAddress, uint256 _tokenId, address _grantee) external onlyTokenOwner(_tokenAddress, _tokenId) {
function revokeRole(
bytes32 _role,
address _tokenAddress,
uint256 _tokenId,
address _grantee
) external onlyTokenOwner(_tokenAddress, _tokenId) {
_revokeRole(_role, _tokenAddress, _tokenId, msg.sender, _grantee, msg.sender);
}

Expand All @@ -102,14 +100,20 @@ contract RolesRegistry is IERC7432 {
address _revoker,
address _grantee
) external override {
require(_isOwner(_tokenAddress, _tokenId, _revoker), "RolesRegistry: revoker must be token owner");
address _caller = _getApprovedCaller(_tokenAddress, _tokenId, _revoker, _grantee);
_revokeRole(_role, _tokenAddress, _tokenId, _revoker, _grantee, _caller);
}

function _getApprovedCaller(address _tokenAddress, uint256 _tokenId, address _revoker, address _grantee) internal view returns (address) {
if(_isRoleApproved(_tokenAddress, _tokenId, _grantee, msg.sender)){
function _getApprovedCaller(
address _tokenAddress,
uint256 _tokenId,
address _revoker,
address _grantee
) internal view returns (address) {
if (_isRoleApproved(_tokenAddress, _tokenId, _grantee, msg.sender)) {
return _grantee;
} else if(_isRoleApproved(_tokenAddress, _tokenId, _revoker, msg.sender)){
} else if (_isRoleApproved(_tokenAddress, _tokenId, _revoker, msg.sender)) {
return _revoker;
} else {
revert("RolesRegistry: sender must be approved");
Expand All @@ -125,7 +129,10 @@ contract RolesRegistry is IERC7432 {
address _caller
) internal {
bool _isRevocable = roleAssignments[_grantee][_tokenAddress][_tokenId][_role].revocable;
require(_isRevocable || _caller == _grantee, "RolesRegistry: Role is not revocable or caller is not the grantee");
require(
_isRevocable || _caller == _grantee,
"RolesRegistry: Role is not revocable or caller is not the grantee"
);
delete roleAssignments[_grantee][_tokenAddress][_tokenId][_role];
delete latestGrantees[_tokenAddress][_tokenId][_role];
emit RoleRevoked(_role, _tokenAddress, _tokenId, _revoker, _grantee);
Expand All @@ -148,7 +155,9 @@ contract RolesRegistry is IERC7432 {
address _grantor, // not used, but needed for compatibility with ERC7432
address _grantee
) external view returns (bool) {
return latestGrantees[_tokenAddress][_tokenId][_role] == _grantee && roleAssignments[_grantee][_tokenAddress][_tokenId][_role].expirationDate > block.timestamp;
return
latestGrantees[_tokenAddress][_tokenId][_role] == _grantee &&
roleAssignments[_grantee][_tokenAddress][_tokenId][_role].expirationDate > block.timestamp;
}

function roleData(
Expand Down Expand Up @@ -177,21 +186,12 @@ contract RolesRegistry is IERC7432 {
return interfaceId == type(IERC7432).interfaceId;
}

function setRoleApprovalForAll(
address _tokenAddress,
address _operator,
bool _isApproved
) external override {
function setRoleApprovalForAll(address _tokenAddress, address _operator, bool _isApproved) external override {
tokenApprovals[msg.sender][_tokenAddress][_operator] = _isApproved;
emit RoleApprovalForAll(_tokenAddress, _operator, _isApproved);
}

function approveRole(
address _tokenAddress,
uint256 _tokenId,
address _operator,
bool _approved
) external override {
function approveRole(address _tokenAddress, uint256 _tokenId, address _operator, bool _approved) external override {
tokenIdApprovals[msg.sender][_tokenAddress][_tokenId][_operator] = _approved;
emit RoleApproval(_tokenAddress, _tokenId, _operator, _approved);
}
Expand Down Expand Up @@ -219,14 +219,26 @@ contract RolesRegistry is IERC7432 {
address _grantor,
address _operator
) internal view returns (bool) {
return isRoleApprovedForAll(_tokenAddress, _grantor, _operator) || getApprovedRole(_tokenAddress, _tokenId, _grantor, _operator);
return
isRoleApprovedForAll(_tokenAddress, _grantor, _operator) ||
getApprovedRole(_tokenAddress, _tokenId, _grantor, _operator);
}

function isERC1155(address _tokenAddress) public view returns (bool) {
function _isERC1155(address _tokenAddress) internal view returns (bool) {
return ERC165Checker.supportsInterface(_tokenAddress, type(IERC1155).interfaceId);
}

function isERC721(address _tokenAddress) public view returns (bool) {
function _isERC721(address _tokenAddress) internal view returns (bool) {
return ERC165Checker.supportsInterface(_tokenAddress, type(IERC721).interfaceId);
}
}

function _isOwner(address _tokenAddress, uint256 _tokenId, address _account) internal view returns (bool) {
if (_isERC1155(_tokenAddress)) {
return IERC1155(_tokenAddress).balanceOf(_account, _tokenId) > 0;
} else if (_isERC721(_tokenAddress)) {
return _account == IERC721(_tokenAddress).ownerOf(_tokenId);
} else {
revert("OriumMarketplace: token address is not ERC1155 or ERC721");
}
}
}
66 changes: 65 additions & 1 deletion test/RolesRegistry.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import axios from 'axios'
import { defaultAbiCoder, solidityKeccak256 } from 'ethers/lib/utils'
import { NftMetadata, Role } from './types'

const { HashZero, AddressZero } = ethers.constants
const { HashZero } = ethers.constants
const ONE_DAY = 60 * 60 * 24

describe('RolesRegistry', () => {
Expand Down Expand Up @@ -737,5 +737,69 @@ describe('RolesRegistry', () => {
})
}
})

describe.only('Transfers', async function () {
beforeEach(async function () {
await RolesRegistry.connect(grantor).grantRole(
PROPERTY_MANAGER,
mockERC721.address,
tokenId,
userTwo.address,
expirationDate,
revocable,
HashZero,
)

await mockERC721.connect(grantor).transferFrom(grantor.address, userTwo.address, tokenId)
})
it('Should keep the role when transferring the NFT', async function () {
expect(
await RolesRegistry.hasRole(PROPERTY_MANAGER, mockERC721.address, tokenId, grantor.address, userTwo.address),
).to.be.equal(true)
})
it('Should revoke the role after transferring the NFT', async function () {
expect(
await RolesRegistry.hasRole(PROPERTY_MANAGER, mockERC721.address, tokenId, grantor.address, userTwo.address),
).to.be.equal(true)

await RolesRegistry.connect(userTwo).revokeRole(PROPERTY_MANAGER, mockERC721.address, tokenId, userTwo.address)

expect(
await RolesRegistry.hasRole(PROPERTY_MANAGER, mockERC721.address, tokenId, grantor.address, userTwo.address),
).to.be.equal(false)
})
it('Should NOT revoke role from if operator is only approved by previous NFT owner', async () => {
await RolesRegistry.connect(grantor).approveRole(mockERC721.address, tokenId, userOne.address, true)
await expect(
RolesRegistry.connect(userOne).revokeRoleFrom(
PROPERTY_MANAGER,
mockERC721.address,
tokenId,
grantor.address,
userTwo.address,
),
).to.be.revertedWith(`RolesRegistry: revoker must be token owner`)
})
it('Should revoke role from if operator is approved by grantee', async () => {
await RolesRegistry.connect(userTwo).approveRole(mockERC721.address, tokenId, userOne.address, true)
expect(
await RolesRegistry.hasRole(PROPERTY_MANAGER, mockERC721.address, tokenId, grantor.address, userTwo.address),
).to.be.equal(true)
await expect(
RolesRegistry.connect(userOne).revokeRoleFrom(
PROPERTY_MANAGER,
mockERC721.address,
tokenId,
userTwo.address,
userTwo.address,
),
)
.to.emit(RolesRegistry, 'RoleRevoked')
.withArgs(PROPERTY_MANAGER, mockERC721.address, tokenId, userTwo.address, userTwo.address)
expect(
await RolesRegistry.hasRole(PROPERTY_MANAGER, mockERC721.address, tokenId, grantor.address, userTwo.address),
).to.be.equal(false)
})
})
})
})

0 comments on commit 684e226

Please sign in to comment.