From ec2e4365d2140134088944a8a7b26858d81143ee Mon Sep 17 00:00:00 2001 From: OneTony Date: Tue, 3 Sep 2024 18:27:49 +0300 Subject: [PATCH 1/4] test: registry unit tests --- contracts/core/Registry.sol | 10 +- package.json | 2 +- test/mocks/smock-mocks/MockRegistry.sol | 39 +++ test/unit/core/RegistryUnit.t.sol | 346 ++++++++++++++++++++++++ test/unit/core/RegistryUnit.tree | 95 +++++++ 5 files changed, 486 insertions(+), 6 deletions(-) create mode 100644 test/mocks/smock-mocks/MockRegistry.sol create mode 100644 test/unit/core/RegistryUnit.t.sol create mode 100644 test/unit/core/RegistryUnit.tree diff --git a/contracts/core/Registry.sol b/contracts/core/Registry.sol index 1d3afced9..9c7d095e1 100644 --- a/contracts/core/Registry.sol +++ b/contracts/core/Registry.sol @@ -321,7 +321,7 @@ contract Registry is IRegistry, Initializable, AccessControlUpgradeable, Errors /// @notice Checks if the caller is the owner of the profile /// @dev Internal function used by modifier 'onlyProfileOwner' /// @param _profileId The ID of the profile - function _checkOnlyProfileOwner(bytes32 _profileId) internal view { + function _checkOnlyProfileOwner(bytes32 _profileId) internal view virtual { if (!_isOwnerOfProfile(_profileId, msg.sender)) revert UNAUTHORIZED(); } @@ -330,7 +330,7 @@ contract Registry is IRegistry, Initializable, AccessControlUpgradeable, Errors /// @param _profileId The ID of the profile /// @param _name The name of the profile /// @return anchor The address of the deployed anchor contract - function _generateAnchor(bytes32 _profileId, string memory _name) internal returns (address anchor) { + function _generateAnchor(bytes32 _profileId, string memory _name) internal virtual returns (address anchor) { bytes memory encodedData = abi.encode(_profileId, _name); bytes memory encodedConstructorArgs = abi.encode(_profileId, address(this)); @@ -356,7 +356,7 @@ contract Registry is IRegistry, Initializable, AccessControlUpgradeable, Errors /// @param _nonce Nonce provided by the caller to generate 'profileId' /// @param _owner The owner of the profile /// @return 'profileId' The ID of the profile - function _generateProfileId(uint256 _nonce, address _owner) internal pure returns (bytes32) { + function _generateProfileId(uint256 _nonce, address _owner) internal pure virtual returns (bytes32) { return keccak256(abi.encodePacked(_nonce, _owner)); } @@ -365,7 +365,7 @@ contract Registry is IRegistry, Initializable, AccessControlUpgradeable, Errors /// @param _profileId The 'profileId' of the profile /// @param _owner The address to check /// @return 'true' if the address is an owner of the profile, otherwise 'false' - function _isOwnerOfProfile(bytes32 _profileId, address _owner) internal view returns (bool) { + function _isOwnerOfProfile(bytes32 _profileId, address _owner) internal view virtual returns (bool) { return profilesById[_profileId].owner == _owner; } @@ -374,7 +374,7 @@ contract Registry is IRegistry, Initializable, AccessControlUpgradeable, Errors /// @param _profileId The 'profileId' of the profile /// @param _member The address to check /// @return 'true' if the address is a member of the profile, otherwise 'false' - function _isMemberOfProfile(bytes32 _profileId, address _member) internal view returns (bool) { + function _isMemberOfProfile(bytes32 _profileId, address _member) internal view virtual returns (bool) { return hasRole(_profileId, _member); } diff --git a/package.json b/package.json index d717cadf2..59174ef8e 100644 --- a/package.json +++ b/package.json @@ -30,7 +30,7 @@ "/////// deploy-test ///////": "echo 'deploy test scripts'", "create-profile": "npx hardhat run scripts/test/createProfile.ts --network", "create-pool": "npx hardhat run scripts/test/createPool.ts --network", - "smock": "smock-foundry --contracts contracts/core" + "smock": "smock-foundry --contracts contracts/core --contracts test/mocks/smock-mocks/" }, "devDependencies": { "@matterlabs/hardhat-zksync-deploy": "^1.2.1", diff --git a/test/mocks/smock-mocks/MockRegistry.sol b/test/mocks/smock-mocks/MockRegistry.sol new file mode 100644 index 000000000..27bba0c22 --- /dev/null +++ b/test/mocks/smock-mocks/MockRegistry.sol @@ -0,0 +1,39 @@ +// SPDX-License-Identifier: UNLICENSED +pragma solidity ^0.8.19; + +import {Registry} from "contracts/core/Registry.sol"; + +contract MockRegistry is Registry { + function _grantRole(bytes32 role, address account) internal virtual override { + super._grantRole(role, account); + } + + function _revokeRole(bytes32 role, address account) internal virtual override { + super._revokeRole(role, account); + } + + function _generateProfileId(uint256 _nonce, address _owner) internal pure virtual override returns (bytes32) { + return super._generateProfileId(_nonce, _owner); + } + + function _generateAnchor(bytes32 _profileId, string memory _name) + internal + virtual + override + returns (address anchor) + { + return super._generateAnchor(_profileId, _name); + } + + function _checkOnlyProfileOwner(bytes32 _profileId) internal view virtual override { + super._checkOnlyProfileOwner(_profileId); + } + + function _isOwnerOfProfile(bytes32 _profileId, address _owner) internal view virtual override returns (bool) { + return super._isOwnerOfProfile(_profileId, _owner); + } + + function _isMemberOfProfile(bytes32 _profileId, address _member) internal view virtual override returns (bool) { + return super._isMemberOfProfile(_profileId, _member); + } +} diff --git a/test/unit/core/RegistryUnit.t.sol b/test/unit/core/RegistryUnit.t.sol new file mode 100644 index 000000000..1178f676f --- /dev/null +++ b/test/unit/core/RegistryUnit.t.sol @@ -0,0 +1,346 @@ +// SPDX-License-Identifier: UNLICENSED +pragma solidity ^0.8.19; + +import {MockMockRegistry} from "test/smock/MockMockRegistry.sol"; +import {Metadata} from "contracts/core/libraries/Metadata.sol"; +import {Errors} from "contracts/core/libraries/Errors.sol"; +import {Test, stdStorage, StdStorage} from "forge-std/Test.sol"; + +contract RegistryUnit is Test { + using stdStorage for StdStorage; + + MockMockRegistry public registry; + + event ProfileCreated( + bytes32 indexed profileId, uint256 nonce, string name, Metadata metadata, address owner, address anchor + ); + + event ProfileNameUpdated(bytes32 indexed profileId, string name, address anchor); + + event ProfileMetadataUpdated(bytes32 indexed profileId, Metadata metadata); + + event ProfileOwnerUpdated(bytes32 indexed profileId, address owner); + + event ProfilePendingOwnerUpdated(bytes32 indexed profileId, address pendingOwner); + + function setUp() public virtual { + registry = new MockMockRegistry(); + } + + modifier givenCallerIsProfileOwner(bytes32 _profileId) { + registry.mock_call__checkOnlyProfileOwner(_profileId); + _; + } + + function test_InitializeRevertWhen_OwnerIsZeroAddress() external { + // it should revert + vm.expectRevert(Errors.ZERO_ADDRESS.selector); + registry.initialize(address(0)); + } + + function test_InitializeWhenOwnerIsNotZeroAddress(address _owner) external { + vm.assume(_owner != address(0)); + // it should call _grantRole + registry.expectCall__grantRole(keccak256("ALLO_OWNER"), _owner); + + vm.prank(_owner); + registry.initialize(_owner); + + assertTrue(registry.hasRole(keccak256("ALLO_OWNER"), _owner)); + } + + function test_CreateProfileRevertWhen_ProfileAlreadyExists( + uint256 _nonce, + address _owner, + address[] memory _members + ) external { + vm.skip(true); + // mock call _generateProfileId + bytes32 _expectedProfileId = keccak256(abi.encodePacked(_nonce, _owner)); + registry.mock_call__generateProfileId(_nonce, _owner, _expectedProfileId); + + address _fakeAnchor = makeAddr("fakeAnchor"); + // store a fake address on the profile's anchor + stdstore.target(address(registry)).sig("profilesById(bytes32)").with_key(_expectedProfileId).depth(5) + .checked_write(_fakeAnchor); + + // it should revert + vm.expectRevert(Errors.NONCE_NOT_AVAILABLE.selector); + registry.createProfile(_nonce, "test", Metadata({protocol: 1, pointer: "0x"}), _owner, _members); + } + + function test_CreateProfileRevertWhen_ProfileOwnerIsZeroAddress(uint256 _nonce, address[] memory _members) + external + { + // it should revert + vm.expectRevert(Errors.ZERO_ADDRESS.selector); + + registry.createProfile(_nonce, "test", Metadata({protocol: 1, pointer: "0x"}), address(0), _members); + } + + function test_CreateProfileRevertWhen_ProfileMembersAreHigherThanZeroAndOwnerIsNotTheCaller( + uint256 _nonce, + address _owner, + address[] memory members + ) external { + vm.assume(members.length > 0); + vm.assume(_owner != address(0)); + vm.assume(_owner != address(this)); + // it should revert + vm.expectRevert(Errors.UNAUTHORIZED.selector); + + registry.createProfile(_nonce, "test", Metadata({protocol: 1, pointer: "0x"}), _owner, members); + } + + function test_CreateProfileRevertWhen_ProfileMemberAddressIsZero(uint256 _nonce, address _owner) external { + vm.assume(_owner != address(0)); + + address[] memory _members = new address[](1); + _members[0] = address(0); + + vm.expectRevert(Errors.ZERO_ADDRESS.selector); + + vm.prank(_owner); + registry.createProfile(_nonce, "test", Metadata({protocol: 1, pointer: "0x"}), _owner, _members); + } + + function test_CreateProfileWhenCalledWithCorrectParams(uint256 _nonce, address _owner) external { + vm.assume(_owner != address(0)); + address[] memory _members = new address[](1); + _members[0] = makeAddr("member"); + + // it should call _generateProfileId + registry.expectCall__generateProfileId(_nonce, _owner); + // it should call _generateAnchor + bytes32 _expectedProfileId = keccak256(abi.encodePacked(_nonce, _owner)); + address _mockAnchor = makeAddr("anchor"); + + registry.mock_call__generateAnchor(_expectedProfileId, "test", _mockAnchor); + registry.expectCall__generateAnchor(_expectedProfileId, "test"); + // it should call _grantRole for members + registry.expectCall__grantRole(_expectedProfileId, _members[0]); + // it should emit ProfileCreated event + vm.expectEmit(true, true, true, true); + emit ProfileCreated( + _expectedProfileId, _nonce, "test", Metadata({protocol: 1, pointer: "0x"}), _owner, _mockAnchor + ); + + vm.prank(_owner); + registry.createProfile(_nonce, "test", Metadata({protocol: 1, pointer: "0x"}), _owner, _members); + } + + function test_UpdateProfileNameShouldCall_generateAnchor(string memory _name, bytes32 _profileId) + external + givenCallerIsProfileOwner(_profileId) + { + // it should call _generateAnchor + address _anchor = makeAddr("anchor"); + registry.expectCall__generateAnchor(_profileId, _name); + registry.mock_call__generateAnchor(_profileId, _name, _anchor); + + address _profileAnchor = registry.updateProfileName(_profileId, _name); + assertEq(_profileAnchor, _anchor); + } + + function test_UpdateProfileNameShouldEmitProfileNameUpdatedEvent(string memory _name, bytes32 _profileId) + external + givenCallerIsProfileOwner(_profileId) + { + // it should call _generateAnchor + address _anchor = makeAddr("anchor"); + registry.expectCall__generateAnchor(_profileId, _name); + registry.mock_call__generateAnchor(_profileId, _name, _anchor); + + // it should emit ProfileNameUpdated event + vm.expectEmit(true, true, true, true); + emit ProfileNameUpdated(_profileId, _name, _anchor); + + registry.updateProfileName(_profileId, _name); + } + + function test_UpdateProfileMetadataShouldEmitProfileMetadataUpdatedEvent( + bytes32 _profileId, + Metadata memory _metadata + ) external givenCallerIsProfileOwner(_profileId) { + // it should emit ProfileMetadataUpdated event + vm.expectEmit(true, true, true, true); + emit ProfileMetadataUpdated(_profileId, _metadata); + + registry.updateProfileMetadata(_profileId, _metadata); + } + + function test_IsOwnerOrMemberOfProfileWhenCalled(bytes32 _profileId, address _account) external { + // it should call _isOwnerOfProfile + registry.mock_call__isOwnerOfProfile(_profileId, _account, false); + registry.expectCall__isOwnerOfProfile(_profileId, _account); + // it should call _isMemberOfProfile + registry.mock_call__isMemberOfProfile(_profileId, _account, true); + registry.expectCall__isMemberOfProfile(_profileId, _account); + + assertTrue(registry.isOwnerOrMemberOfProfile(_profileId, _account)); + } + + function test_IsOwnerOfProfileWhenCalled(bytes32 _profileId, address _owner) external { + // it should call _isOwnerOfProfile + registry.mock_call__isOwnerOfProfile(_profileId, _owner, true); + registry.expectCall__isOwnerOfProfile(_profileId, _owner); + + assertTrue(registry.isOwnerOfProfile(_profileId, _owner)); + } + + function test_IsMemberOfProfileWhenCalled(bytes32 _profileId, address _member) external { + // it should call _isMemberOfProfile + registry.mock_call__isMemberOfProfile(_profileId, _member, true); + registry.expectCall__isMemberOfProfile(_profileId, _member); + + assertTrue(registry.isMemberOfProfile(_profileId, _member)); + } + + function test_UpdateProfilePendingOwnerWhenProfileOwnerIsTheCaller(bytes32 _profileId, address _pendingOwner) + external + givenCallerIsProfileOwner(_profileId) + { + // it should emit ProfilePendingOwnerUpdated + vm.expectEmit(true, true, true, true); + emit ProfilePendingOwnerUpdated(_profileId, _pendingOwner); + + registry.updateProfilePendingOwner(_profileId, _pendingOwner); + // it should store the pendingOwner to profileIdToPendingOwner + assertEq(registry.profileIdToPendingOwner(_profileId), _pendingOwner); + } + + function test_AcceptProfileOwnershipRevertWhen_CallerIsDifferentThanNewOwner(bytes32 _profileId) external { + address _pendingOwner = makeAddr("pendingOwner"); + stdstore.target(address(registry)).sig("profileIdToPendingOwner(bytes32)").with_key(_profileId).checked_write( + _pendingOwner + ); + + // it should revert + vm.expectRevert(Errors.NOT_PENDING_OWNER.selector); + registry.acceptProfileOwnership(_profileId); + } + + function test_AcceptProfileOwnershipWhenCalledWithCorrectParams(bytes32 _profileId) external { + address _pendingOwner = makeAddr("pendingOwner"); + stdstore.target(address(registry)).sig("profileIdToPendingOwner(bytes32)").with_key(_profileId).checked_write( + _pendingOwner + ); + + // it should emit ProfileOwnerUpdated + vm.expectEmit(true, true, true, true); + emit ProfileOwnerUpdated(_profileId, _pendingOwner); + + vm.prank(_pendingOwner); + registry.acceptProfileOwnership(_profileId); + + // it should delete the pendingOwner from profileIdToPendingOwner + assertEq(registry.profileIdToPendingOwner(_profileId), address(0)); + } + + function test_AddMembersRevertWhen_MemberIsZeroAddress(bytes32 _profileId) + external + givenCallerIsProfileOwner(_profileId) + { + address[] memory _members = new address[](1); + _members[0] = address(0); + + // it should revert + vm.expectRevert(Errors.ZERO_ADDRESS.selector); + + registry.addMembers(_profileId, _members); + } + + function test_AddMembersWhenCalledWithCorrectParams(bytes32 _profileId) + external + givenCallerIsProfileOwner(_profileId) + { + address[] memory _members = new address[](1); + _members[0] = makeAddr("member"); + // it should call _grantRole for members + registry.expectCall__grantRole(_profileId, _members[0]); + + registry.addMembers(_profileId, _members); + } + + function test_RemoveMembersWhenCalled(bytes32 _profileId) external givenCallerIsProfileOwner(_profileId) { + address[] memory _members = new address[](1); + _members[0] = makeAddr("member"); + + // it should call _revokeRole for members + registry.expectCall__revokeRole(_profileId, _members[0]); + + registry.removeMembers(_profileId, _members); + } + + function test__checkOnlyProfileOwnerRevertWhen_CallerIsNotProfileOwner(bytes32 _profileId) external { + // it should revert + vm.expectRevert(Errors.UNAUTHORIZED.selector); + + registry.call__checkOnlyProfileOwner(_profileId); + } + + function test__checkOnlyProfileOwnerWhenCallerIsProfileOwner(bytes32 _profileId) external { + registry.mock_call__isOwnerOfProfile(_profileId, address(this), true); + + // it should not revert + registry.call__checkOnlyProfileOwner(_profileId); + } + + function test__generateProfileIdShouldReturnTheKeccak256OfEncodedNonceAndOwnerAddress( + uint256 _nonce, + address _owner + ) external { + // it should return the keccak256 of encoded nonce and owner address + assertEq(keccak256(abi.encodePacked(_nonce, _owner)), registry.call__generateProfileId(_nonce, _owner)); + } + + function test__isOwnerOfProfileWhenProvidedAddressIsOwnerOfProfile(bytes32 _profileId, address _owner) external { + vm.skip(true); + // it should return true + stdstore.target(address(registry)).sig("profilesById(bytes32)").with_key(_profileId).depth(5).checked_write( + _owner + ); + assertTrue(registry.call__isOwnerOfProfile(_profileId, _owner)); + } + + function test__isOwnerOfProfileWhenProvidedAddressIsNotOwnerOfProfile(bytes32 _profileId, address _owner) + external + { + vm.assume(_owner != address(0)); + // it should return false + assertTrue(!registry.call__isOwnerOfProfile(_profileId, _owner)); + } + + function test__isMemberOfProfileWhenProvidedAddressIsMemberOfProfile(bytes32 _profileId, address _member) + external + { + vm.skip(true); + vm.mockCall( + address(registry), abi.encodeWithSelector(registry.hasRole.selector, _profileId, _member), abi.encode(true) + ); + // it should return true + assertTrue(registry.call__isMemberOfProfile(_profileId, _member)); + } + + function test__isMemberOfProfileWhenProvidedAddressIsNotMemberOfProfile(bytes32 _profileId, address _member) + external + { + // it should return false + assertTrue(!registry.call__isMemberOfProfile(_profileId, _member)); + } + + modifier givenCallerIsAlloOwner() { + _; + } + + function test_RecoverFundsRevertWhen_RecipientIsZeroAddress() external givenCallerIsAlloOwner { + // it should revert + vm.skip(true); + } + + function test_RecoverFundsWhenRecipientIsNotZeroAddress() external givenCallerIsAlloOwner { + // it should call getBalance + // it should call transfer + vm.skip(true); + } +} diff --git a/test/unit/core/RegistryUnit.tree b/test/unit/core/RegistryUnit.tree new file mode 100644 index 000000000..98982d496 --- /dev/null +++ b/test/unit/core/RegistryUnit.tree @@ -0,0 +1,95 @@ +Registry::initialize +├── when owner is zero address +│ └── it should revert +└── when owner is not zero address + └── it should call _grantRole + +Registry::createProfile +├── when profile already exists +│ └── it should revert +├── when profile owner is zero address +│ └── it should revert +├── when profile members are higher than zero and owner is not the caller +│ └── it should revert +├── when profile member address is zero +│ └── it should revert +└── when called with correct params + ├── it should call _generateProfileId + ├── it should call _generateAnchor + ├── it should call _grantRole for members + └── it should emit ProfileCreated event + +Registry::updateProfileName +├── it should call _generateAnchor +└── it should emit ProfileNameUpdated event + +Registry::updateProfileMetadata +└── it should emit ProfileMetadataUpdated event + +Registry::isOwnerOrMemberOfProfile +└── when called + ├── it should call _isOwnerOfProfile + └── it should call _isMemberOfProfile + +Registry::isOwnerOfProfile +└── when called + └── it should call _isOwnerOfProfile + +Registry::isMemberOfProfile +└── when called + └── it should call _isMemberOfProfile + +Registry::updateProfilePendingOwner +└── when profile owner is the caller + ├── it should store the pendingOwner to profileIdToPendingOwner + └── it should emit ProfilePendingOwnerUpdated + +Registry::acceptProfileOwnership +├── when caller is different than newOwner +│ └── it should revert +└── when called with correct params + ├── it should delete the pending owner from profileIdToPendingOwner + └── it should emit ProfileOwnerUpdated + +Registry::addMembers +└── given caller is profile owner + ├── when member is zero address + │ └── it should revert + └── when called with correct params + └── it should call _grantRole for members + +Registry::removeMembers +└── given caller is profile owner + └── when called + └── it should call _revokeRole for members + +Registry::_checkOnlyProfileOwner +├── when caller is not profile owner +│ └── it should revert +└── when caller is profile owner + └── it should not revert + +Registry::_generateAnchor + +Registry::_generateProfileId +└── it should return the keccak256 of encoded nonce and owner address + +Registry::_isOwnerOfProfile +├── when provided address is owner of profile +│ └── it should return true +└── when provided address is not owner of profile + └── it should return false + +Registry::_isMemberOfProfile +├── when provided address is member of profile +│ └── it should return true +└── when provided address is not member of profile + └── it should return false + +Registry::recoverFunds +└── given caller is allo owner + ├── when recipient is zero address + │ └── it should revert + └── when recipient is not zero address + ├── it should call getBalance + └── it should call transfer \ No newline at end of file From 2cdefd3588159236ca0d35f9834167e9e12cf95e Mon Sep 17 00:00:00 2001 From: OneTony Date: Thu, 5 Sep 2024 13:34:32 +0300 Subject: [PATCH 2/4] fix: tests --- package.json | 4 ---- test/mocks/smock-mocks/MockRegistry.sol | 4 ++++ test/unit/core/RegistryUnit.t.sol | 21 +++++++++++++-------- 3 files changed, 17 insertions(+), 12 deletions(-) diff --git a/package.json b/package.json index 23c573ac2..59174ef8e 100644 --- a/package.json +++ b/package.json @@ -30,11 +30,7 @@ "/////// deploy-test ///////": "echo 'deploy test scripts'", "create-profile": "npx hardhat run scripts/test/createProfile.ts --network", "create-pool": "npx hardhat run scripts/test/createPool.ts --network", -<<<<<<< HEAD "smock": "smock-foundry --contracts contracts/core --contracts test/mocks/smock-mocks/" -======= - "smock": "smock-foundry --contracts contracts/core --contracts test/utils/mocks/" ->>>>>>> 5e5ec81507abb02f02eb89db2db876a071f8b25e }, "devDependencies": { "@matterlabs/hardhat-zksync-deploy": "^1.2.1", diff --git a/test/mocks/smock-mocks/MockRegistry.sol b/test/mocks/smock-mocks/MockRegistry.sol index 27bba0c22..23c22bd5a 100644 --- a/test/mocks/smock-mocks/MockRegistry.sol +++ b/test/mocks/smock-mocks/MockRegistry.sol @@ -8,6 +8,10 @@ contract MockRegistry is Registry { super._grantRole(role, account); } + function _checkRole(bytes32 role, address account) internal view virtual override { + super._checkRole(role, account); + } + function _revokeRole(bytes32 role, address account) internal virtual override { super._revokeRole(role, account); } diff --git a/test/unit/core/RegistryUnit.t.sol b/test/unit/core/RegistryUnit.t.sol index 1178f676f..42ea8a036 100644 --- a/test/unit/core/RegistryUnit.t.sol +++ b/test/unit/core/RegistryUnit.t.sol @@ -5,6 +5,7 @@ import {MockMockRegistry} from "test/smock/MockMockRegistry.sol"; import {Metadata} from "contracts/core/libraries/Metadata.sol"; import {Errors} from "contracts/core/libraries/Errors.sol"; import {Test, stdStorage, StdStorage} from "forge-std/Test.sol"; +import {IAccessControlUpgradeable} from "openzeppelin-contracts-upgradeable/contracts/access/IAccessControlUpgradeable.sol"; contract RegistryUnit is Test { using stdStorage for StdStorage; @@ -32,6 +33,11 @@ contract RegistryUnit is Test { _; } + modifier givenCallerIsAlloOwner() { + registry.mock_call__checkRole(keccak256("ALLO_OWNER"), address(this), true); + _; + } + function test_InitializeRevertWhen_OwnerIsZeroAddress() external { // it should revert vm.expectRevert(Errors.ZERO_ADDRESS.selector); @@ -314,9 +320,8 @@ contract RegistryUnit is Test { function test__isMemberOfProfileWhenProvidedAddressIsMemberOfProfile(bytes32 _profileId, address _member) external { - vm.skip(true); vm.mockCall( - address(registry), abi.encodeWithSelector(registry.hasRole.selector, _profileId, _member), abi.encode(true) + address(registry), abi.encodeWithSelector(IAccessControlUpgradeable.hasRole.selector, _profileId, _member), abi.encode(true) ); // it should return true assertTrue(registry.call__isMemberOfProfile(_profileId, _member)); @@ -329,16 +334,16 @@ contract RegistryUnit is Test { assertTrue(!registry.call__isMemberOfProfile(_profileId, _member)); } - modifier givenCallerIsAlloOwner() { - _; - } - function test_RecoverFundsRevertWhen_RecipientIsZeroAddress() external givenCallerIsAlloOwner { + + function test_RecoverFundsRevertWhen_RecipientIsZeroAddress(address _token) external givenCallerIsAlloOwner { // it should revert - vm.skip(true); + vm.expectRevert(Errors.ZERO_ADDRESS.selector); + + registry.recoverFunds(_token, address(0)); } - function test_RecoverFundsWhenRecipientIsNotZeroAddress() external givenCallerIsAlloOwner { + function test_RecoverFundsWhenRecipientIsNotZeroAddress(address _token, address _recipient) external givenCallerIsAlloOwner { // it should call getBalance // it should call transfer vm.skip(true); From 37a03b16f4f671a97e33f78584ef865c9ac81275 Mon Sep 17 00:00:00 2001 From: OneTony Date: Thu, 5 Sep 2024 14:12:44 +0300 Subject: [PATCH 3/4] fix: tests --- package.json | 2 +- test/unit/core/RegistryUnit.t.sol | 39 +++++++++++++++++-------------- 2 files changed, 23 insertions(+), 18 deletions(-) diff --git a/package.json b/package.json index 317524e3c..35f0af8a3 100644 --- a/package.json +++ b/package.json @@ -31,7 +31,7 @@ "/////// deploy-test ///////": "echo 'deploy test scripts'", "create-profile": "npx hardhat run scripts/test/createProfile.ts --network", "create-pool": "npx hardhat run scripts/test/createPool.ts --network", - "smock": "smock-foundry --contracts contracts/core --contracts test/mocks/smock-mocks/" + "smock": "smock-foundry --contracts test/mocks/smock-mocks/" }, "devDependencies": { "@defi-wonderland/natspec-smells": "1.1.5", diff --git a/test/unit/core/RegistryUnit.t.sol b/test/unit/core/RegistryUnit.t.sol index 42ea8a036..308440763 100644 --- a/test/unit/core/RegistryUnit.t.sol +++ b/test/unit/core/RegistryUnit.t.sol @@ -5,7 +5,8 @@ import {MockMockRegistry} from "test/smock/MockMockRegistry.sol"; import {Metadata} from "contracts/core/libraries/Metadata.sol"; import {Errors} from "contracts/core/libraries/Errors.sol"; import {Test, stdStorage, StdStorage} from "forge-std/Test.sol"; -import {IAccessControlUpgradeable} from "openzeppelin-contracts-upgradeable/contracts/access/IAccessControlUpgradeable.sol"; +import {IAccessControlUpgradeable} from + "openzeppelin-contracts-upgradeable/contracts/access/IAccessControlUpgradeable.sol"; contract RegistryUnit is Test { using stdStorage for StdStorage; @@ -34,7 +35,7 @@ contract RegistryUnit is Test { } modifier givenCallerIsAlloOwner() { - registry.mock_call__checkRole(keccak256("ALLO_OWNER"), address(this), true); + registry.mock_call__checkRole(keccak256("ALLO_OWNER"), address(this)); _; } @@ -55,21 +56,22 @@ contract RegistryUnit is Test { assertTrue(registry.hasRole(keccak256("ALLO_OWNER"), _owner)); } - function test_CreateProfileRevertWhen_ProfileAlreadyExists( - uint256 _nonce, - address _owner, - address[] memory _members - ) external { - vm.skip(true); + function test_CreateProfileRevertWhen_ProfileAlreadyExists(uint256 _nonce, address _owner) external { + vm.assume(_owner != address(0)); + address[] memory _members = new address[](1); + _members[0] = makeAddr("member"); + // mock call _generateProfileId bytes32 _expectedProfileId = keccak256(abi.encodePacked(_nonce, _owner)); registry.mock_call__generateProfileId(_nonce, _owner, _expectedProfileId); - address _fakeAnchor = makeAddr("fakeAnchor"); - // store a fake address on the profile's anchor - stdstore.target(address(registry)).sig("profilesById(bytes32)").with_key(_expectedProfileId).depth(5) - .checked_write(_fakeAnchor); + address _mockAnchor = makeAddr("anchor"); + registry.mock_call__generateAnchor(_expectedProfileId, "test", _mockAnchor); + + vm.prank(_owner); + registry.createProfile(_nonce, "test", Metadata({protocol: 1, pointer: "0x"}), _owner, _members); + registry.mock_call__generateProfileId(_nonce, _owner, _expectedProfileId); // it should revert vm.expectRevert(Errors.NONCE_NOT_AVAILABLE.selector); registry.createProfile(_nonce, "test", Metadata({protocol: 1, pointer: "0x"}), _owner, _members); @@ -303,7 +305,7 @@ contract RegistryUnit is Test { function test__isOwnerOfProfileWhenProvidedAddressIsOwnerOfProfile(bytes32 _profileId, address _owner) external { vm.skip(true); // it should return true - stdstore.target(address(registry)).sig("profilesById(bytes32)").with_key(_profileId).depth(5).checked_write( + stdstore.target(address(registry)).sig("profilesById(bytes32)").with_key(_profileId).depth(4).checked_write( _owner ); assertTrue(registry.call__isOwnerOfProfile(_profileId, _owner)); @@ -321,7 +323,9 @@ contract RegistryUnit is Test { external { vm.mockCall( - address(registry), abi.encodeWithSelector(IAccessControlUpgradeable.hasRole.selector, _profileId, _member), abi.encode(true) + address(registry), + abi.encodeWithSignature("_isMemberOfProfile(bytes32,address)", _profileId, _member), + abi.encode(true) ); // it should return true assertTrue(registry.call__isMemberOfProfile(_profileId, _member)); @@ -334,8 +338,6 @@ contract RegistryUnit is Test { assertTrue(!registry.call__isMemberOfProfile(_profileId, _member)); } - - function test_RecoverFundsRevertWhen_RecipientIsZeroAddress(address _token) external givenCallerIsAlloOwner { // it should revert vm.expectRevert(Errors.ZERO_ADDRESS.selector); @@ -343,7 +345,10 @@ contract RegistryUnit is Test { registry.recoverFunds(_token, address(0)); } - function test_RecoverFundsWhenRecipientIsNotZeroAddress(address _token, address _recipient) external givenCallerIsAlloOwner { + function test_RecoverFundsWhenRecipientIsNotZeroAddress(address _token, address _recipient) + external + givenCallerIsAlloOwner + { // it should call getBalance // it should call transfer vm.skip(true); From b0a547b209700b4fded1fe004718d0b56a3adb66 Mon Sep 17 00:00:00 2001 From: OneTony Date: Thu, 5 Sep 2024 15:31:31 +0300 Subject: [PATCH 4/4] fix: add missing tests --- test/unit/core/RegistryUnit.t.sol | 42 +++++++++++++++++++++++++++++++ test/unit/core/RegistryUnit.tree | 6 +++++ 2 files changed, 48 insertions(+) diff --git a/test/unit/core/RegistryUnit.t.sol b/test/unit/core/RegistryUnit.t.sol index 308440763..1f759a69d 100644 --- a/test/unit/core/RegistryUnit.t.sol +++ b/test/unit/core/RegistryUnit.t.sol @@ -4,6 +4,7 @@ pragma solidity ^0.8.19; import {MockMockRegistry} from "test/smock/MockMockRegistry.sol"; import {Metadata} from "contracts/core/libraries/Metadata.sol"; import {Errors} from "contracts/core/libraries/Errors.sol"; +import {Anchor} from "contracts/core/Anchor.sol"; import {Test, stdStorage, StdStorage} from "forge-std/Test.sol"; import {IAccessControlUpgradeable} from "openzeppelin-contracts-upgradeable/contracts/access/IAccessControlUpgradeable.sol"; @@ -294,6 +295,45 @@ contract RegistryUnit is Test { registry.call__checkOnlyProfileOwner(_profileId); } + function test__generateAnchorWhenAnchorDoesNotExist(bytes32 _profileId, string memory _name) external { + /// Calculate anchor address + bytes memory encodedData = abi.encode(_profileId, _name); + bytes memory encodedConstructorArgs = abi.encode(_profileId, address(registry)); + + bytes memory bytecode = abi.encodePacked(type(Anchor).creationCode, encodedConstructorArgs); + + bytes32 salt = keccak256(encodedData); + + address preComputedAddress = address( + uint160(uint256(keccak256(abi.encodePacked(bytes1(0xff), address(registry), salt, keccak256(bytecode))))) + ); + + // it should deploy the anchor + address _anchor = registry.call__generateAnchor(_profileId, _name); + + assertEq(_anchor, preComputedAddress); + } + + function test__generateAnchorWhenAnchorExists(bytes32 _profileId, string memory _name) external { + // it should return the anchor address + address _anchor = registry.call__generateAnchor(_profileId, _name); + + // when called again with same params it should return the same address + assertEq(_anchor, registry.call__generateAnchor(_profileId, _name)); + } + + function test__generateAnchorRevertWhen_AnchorProfileIdIsNotTheSameAsProvidedProfileId( + bytes32 _profileId, + string memory _name + ) external { + address _anchor = registry.call__generateAnchor(_profileId, _name); + + vm.mockCall(_anchor, abi.encodeWithSignature("profileId()"), abi.encode(keccak256("wrong"))); + vm.expectRevert(Errors.ANCHOR_ERROR.selector); + // it should revert + registry.call__generateAnchor(_profileId, _name); + } + function test__generateProfileIdShouldReturnTheKeccak256OfEncodedNonceAndOwnerAddress( uint256 _nonce, address _owner @@ -304,6 +344,7 @@ contract RegistryUnit is Test { function test__isOwnerOfProfileWhenProvidedAddressIsOwnerOfProfile(bytes32 _profileId, address _owner) external { vm.skip(true); + // TODO: // it should return true stdstore.target(address(registry)).sig("profilesById(bytes32)").with_key(_profileId).depth(4).checked_write( _owner @@ -349,6 +390,7 @@ contract RegistryUnit is Test { external givenCallerIsAlloOwner { + // TODO: // it should call getBalance // it should call transfer vm.skip(true); diff --git a/test/unit/core/RegistryUnit.tree b/test/unit/core/RegistryUnit.tree index 98982d496..c4aeac453 100644 --- a/test/unit/core/RegistryUnit.tree +++ b/test/unit/core/RegistryUnit.tree @@ -70,6 +70,12 @@ Registry::_checkOnlyProfileOwner └── it should not revert Registry::_generateAnchor +├── when anchor does not exist +│ └── it should deploy the anchor +├── when anchor exists +│ └── it should return the anchor address +└── when anchor profileId is not the same as provided profileId + └── it should revert Registry::_generateProfileId └── it should return the keccak256 of encoded nonce and owner address