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 684e226 commit 380d938
Show file tree
Hide file tree
Showing 2 changed files with 74 additions and 31 deletions.
16 changes: 7 additions & 9 deletions contracts/RolesRegistry/RolesRegistry.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -37,8 +37,8 @@ contract RolesRegistry is IERC7432 {
_;
}

modifier onlyTokenOwner(address _tokenAddress, uint256 _tokenId) {
require(_isOwner(_tokenAddress, _tokenId, msg.sender), "RolesRegistry: sender must be token owner");
modifier onlyTokenOwner(address _tokenAddress, uint256 _tokenId, address _account) {
require(_isOwner(_tokenAddress, _tokenId, _account), "RolesRegistry: account must be token owner");
_;
}

Expand All @@ -50,7 +50,7 @@ contract RolesRegistry is IERC7432 {
uint64 _expirationDate,
bool _revocable,
bytes calldata _data
) external onlyTokenOwner(_tokenAddress, _tokenId) {
) external onlyTokenOwner(_tokenAddress, _tokenId, msg.sender) {
_grantRole(_role, _tokenAddress, _tokenId, msg.sender, _grantee, _expirationDate, _revocable, _data);
}

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

Expand All @@ -78,7 +77,7 @@ 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: [Part3] 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);
Expand All @@ -89,7 +88,7 @@ contract RolesRegistry is IERC7432 {
address _tokenAddress,
uint256 _tokenId,
address _grantee
) external onlyTokenOwner(_tokenAddress, _tokenId) {
) external onlyTokenOwner(_tokenAddress, _tokenId, msg.sender) {
_revokeRole(_role, _tokenAddress, _tokenId, msg.sender, _grantee, msg.sender);
}

Expand All @@ -99,8 +98,7 @@ contract RolesRegistry is IERC7432 {
uint256 _tokenId,
address _revoker,
address _grantee
) external override {
require(_isOwner(_tokenAddress, _tokenId, _revoker), "RolesRegistry: revoker must be token owner");
) external override onlyTokenOwner(_tokenAddress, _tokenId, _revoker) {
address _caller = _getApprovedCaller(_tokenAddress, _tokenId, _revoker, _grantee);
_revokeRole(_role, _tokenAddress, _tokenId, _revoker, _grantee, _caller);
}
Expand Down
89 changes: 67 additions & 22 deletions test/RolesRegistry.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -142,6 +142,19 @@ describe('RolesRegistry', () => {
),
).to.be.revertedWith('RolesRegistry: expiration date must be in the future')
})
it('should NOT grant role if caller is not the token owner', async () => {
await expect(
RolesRegistry.connect(userOne).grantRole(
PROPERTY_MANAGER,
mockERC721.address,
tokenId,
userOne.address,
expirationDate,
revocable,
HashZero,
),
).to.be.revertedWith(`RolesRegistry: account must be token owner`)
})
})

describe('Revoke role', async () => {
Expand Down Expand Up @@ -203,6 +216,11 @@ describe('RolesRegistry', () => {
RolesRegistry.connect(grantor).revokeRole(PROPERTY_MANAGER, mockERC721.address, tokenId, userOne.address),
).to.be.revertedWith(`RolesRegistry: Role is not revocable or caller is not the grantee`)
})
it('should NOT revoke role if caller is not the token owner', async () => {
await expect(
RolesRegistry.connect(userOne).revokeRole(PROPERTY_MANAGER, mockERC721.address, tokenId, userOne.address),
).to.be.revertedWith(`RolesRegistry: account must be token owner`)
})
})

describe('Has role', async () => {
Expand Down Expand Up @@ -493,6 +511,21 @@ describe('RolesRegistry', () => {
),
).to.be.revertedWith('RolesRegistry: sender must be approved')
})
it('should NOT grant role from if grantor is not the token owner', async () => {
await mockERC721.connect(grantor).transferFrom(grantor.address, userOne.address, tokenId)
await expect(
RolesRegistry.connect(operator).grantRoleFrom(
PROPERTY_MANAGER,
mockERC721.address,
tokenId,
grantor.address,
userOne.address,
expirationDate,
revocable,
HashZero,
),
).to.be.revertedWith(`RolesRegistry: account must be token owner`)
})
})

describe('Revoke role from', async () => {
Expand Down Expand Up @@ -522,26 +555,6 @@ describe('RolesRegistry', () => {
.to.emit(RolesRegistry, 'RoleRevoked')
.withArgs(PROPERTY_MANAGER, mockERC721.address, tokenId, grantor.address, userOne.address)
})
it('should NOT revoke role from if operator is not approved', async () => {
if (approval === 'Approval for TokenId') {
await RolesRegistry.connect(grantor).approveRole(mockERC721.address, tokenId, operator.address, false)
} else {
await RolesRegistry.connect(grantor).setRoleApprovalForAll(
mockERC721.address,
operator.address,
false,
)
}
await expect(
RolesRegistry.connect(operator).revokeRoleFrom(
PROPERTY_MANAGER,
mockERC721.address,
tokenId,
grantor.address,
userOne.address,
),
).to.be.revertedWith('RolesRegistry: sender must be approved')
})
it('should revoke role from if operator is only approved by grantee', async () => {
await RolesRegistry.connect(grantor).approveRole(mockERC721.address, tokenId, operator.address, false)
await RolesRegistry.connect(userOne).approveRole(mockERC721.address, tokenId, operator.address, true)
Expand Down Expand Up @@ -640,6 +653,38 @@ describe('RolesRegistry', () => {
),
).to.be.equal(false)
})
it('should NOT revoke role from if operator is not approved', async () => {
if (approval === 'Approval for TokenId') {
await RolesRegistry.connect(grantor).approveRole(mockERC721.address, tokenId, operator.address, false)
} else {
await RolesRegistry.connect(grantor).setRoleApprovalForAll(
mockERC721.address,
operator.address,
false,
)
}
await expect(
RolesRegistry.connect(operator).revokeRoleFrom(
PROPERTY_MANAGER,
mockERC721.address,
tokenId,
grantor.address,
userOne.address,
),
).to.be.revertedWith('RolesRegistry: sender must be approved')
})
it('should NOT revoke role from if revoker is not the token owner', async () => {
await mockERC721.connect(grantor).transferFrom(grantor.address, userOne.address, tokenId)
await expect(
RolesRegistry.connect(operator).revokeRoleFrom(
PROPERTY_MANAGER,
mockERC721.address,
tokenId,
grantor.address,
userOne.address,
),
).to.be.revertedWith(`RolesRegistry: account must be token owner`)
})
})
describe('Non-Revocable roles', async () => {
beforeEach(async () => {
Expand Down Expand Up @@ -738,7 +783,7 @@ describe('RolesRegistry', () => {
}
})

describe.only('Transfers', async function () {
describe('Transfers', async function () {
beforeEach(async function () {
await RolesRegistry.connect(grantor).grantRole(
PROPERTY_MANAGER,
Expand Down Expand Up @@ -778,7 +823,7 @@ describe('RolesRegistry', () => {
grantor.address,
userTwo.address,
),
).to.be.revertedWith(`RolesRegistry: revoker must be token owner`)
).to.be.revertedWith(`RolesRegistry: account 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)
Expand Down

0 comments on commit 380d938

Please sign in to comment.