From 27de8f9286ed7a8c132ca0b217ca1a823c365478 Mon Sep 17 00:00:00 2001 From: 0xAustrian Date: Thu, 22 Aug 2024 13:01:30 -0300 Subject: [PATCH 01/10] test: add a few test cases --- remappings.txt | 3 +- test/foundry/core/AlloUnit.t.sol | 96 ++++++++++++++++++-- test/foundry/strategies/QVImpactStream.t.sol | 6 +- 3 files changed, 94 insertions(+), 11 deletions(-) diff --git a/remappings.txt b/remappings.txt index 0340fa5de..3cb2cad6d 100644 --- a/remappings.txt +++ b/remappings.txt @@ -13,4 +13,5 @@ hats-protocol/=lib/hats-protocol/src/ solady/=lib/solady/src/ @openzeppelin/contracts-upgradeable/=lib/openzeppelin-contracts-upgradeable/contracts/ lib/ERC1155/=lib/hats-protocol/lib/ERC1155/ -hedgey-vesting/=lib/hedgey-vesting/contracts/ \ No newline at end of file +hedgey-vesting/=lib/hedgey-vesting/contracts/ +smock/=test/smock/ \ No newline at end of file diff --git a/test/foundry/core/AlloUnit.t.sol b/test/foundry/core/AlloUnit.t.sol index 6bd8e21e8..2852f76f5 100644 --- a/test/foundry/core/AlloUnit.t.sol +++ b/test/foundry/core/AlloUnit.t.sol @@ -2,27 +2,109 @@ pragma solidity ^0.8.19; import {Test} from "forge-std/Test.sol"; +import {MockAllo} from "smock/MockAllo.sol"; +import {Errors} from "contracts/core/libraries/Errors.sol"; +import {Metadata} from "contracts/core/libraries/Metadata.sol"; +import {IBaseStrategy} from "contracts/core/interfaces/IBaseStrategy.sol"; contract Allo is Test { - function test_InitializeGivenUpgradeVersionIsCorrect() external { + MockAllo allo; + + function setUp() public virtual { + allo = new MockAllo(); + } + + function test_InitializeGivenUpgradeVersionIsCorrect( + address _owner, + address _registry, + address payable _treasury, + uint256 _percentFee, + uint256 _baseFee, + address _trustedForwarder + ) external { + allo.mock_call__updateRegistry(_registry); + allo.mock_call__updateTreasury(_treasury); + allo.mock_call__updatePercentFee(_percentFee); + allo.mock_call__updateBaseFee(_baseFee); + allo.mock_call__updateTrustedForwarder(_trustedForwarder); + // it should call _initializeOwner + // TODO: expect _initializeOwner + // it should call _updateRegistry + allo.expectCall__updateRegistry(_registry); + // it should call _updateTreasury + allo.expectCall__updateTreasury(_treasury); + // it should call _updatePercentFee + allo.expectCall__updatePercentFee(_percentFee); + // it should call _updateBaseFee + allo.expectCall__updateBaseFee(_baseFee); + // it should call _updateTrustedForwarder - vm.skip(true); + allo.expectCall__updateTrustedForwarder(_trustedForwarder); + + allo.initialize(_owner, _registry, _treasury, _percentFee, _baseFee, _trustedForwarder); } - function test_CreatePoolWithCustomStrategyRevertWhen_StrategyIsZeroAddress() external { + function test_CreatePoolWithCustomStrategyRevertWhen_StrategyIsZeroAddress( + bytes32 _profileId, + bytes memory _initStrategyData, + address _token, + uint256 _amount, + Metadata memory _metadata, + address[] memory _managers + ) external { // it should revert - vm.skip(true); - } + vm.expectRevert(Errors.ZERO_ADDRESS.selector); + + allo.createPoolWithCustomStrategy( + _profileId, address(0), _initStrategyData, _token, _amount, _metadata, _managers + ); + } + + function test_CreatePoolWithCustomStrategyWhenCallingWithProperParams( + address _creator, + uint256 _msgValue, + bytes32 _profileId, + IBaseStrategy _strategy, + bytes memory _initStrategyData, + address _token, + uint256 _amount, + Metadata memory _metadata, + address[] memory _managers, + uint256 _poolId + ) external { + vm.assume(address(_strategy) != address(0)); + vm.assume(_creator != address(0)); + allo.mock_call__createPool( + _creator, + _msgValue, + _profileId, + _strategy, + _initStrategyData, + _token, + _amount, + _metadata, + _managers, + _poolId + ); - function test_CreatePoolWithCustomStrategyWhenCallingWithProperParams() external { // it should call _createPool + allo.expectCall__createPool( + _creator, _msgValue, _profileId, _strategy, _initStrategyData, _token, _amount, _metadata, _managers + ); + + deal(_creator, _msgValue); + vm.prank(_creator); + uint256 poolId = allo.createPoolWithCustomStrategy{value: _msgValue}( + _profileId, address(_strategy), _initStrategyData, _token, _amount, _metadata, _managers + ); + // it should return poolId - vm.skip(true); + assertEq(poolId, _poolId); } function test_CreatePoolRevertWhen_StrategyIsZeroAddress() external { diff --git a/test/foundry/strategies/QVImpactStream.t.sol b/test/foundry/strategies/QVImpactStream.t.sol index 718cdd9c6..291483952 100644 --- a/test/foundry/strategies/QVImpactStream.t.sol +++ b/test/foundry/strategies/QVImpactStream.t.sol @@ -5,7 +5,7 @@ import {IERC20} from "@openzeppelin/contracts/token/ERC20/IERC20.sol"; import {StdStorage, Test, stdStorage} from "forge-std/Test.sol"; import {IAllo} from "../../../contracts/core/interfaces/IAllo.sol"; -import {IStrategy} from "../../../contracts/core/interfaces/IStrategy.sol"; +import {IBaseStrategy} from "../../../contracts/core/interfaces/IBaseStrategy.sol"; import {IRecipientsExtension} from "../../../contracts/extensions/interfaces/IRecipientsExtension.sol"; import {QVSimple} from "../../../contracts/strategies/QVSimple.sol"; import {QVImpactStream} from "../../../contracts/strategies/QVImpactStream.sol"; @@ -149,7 +149,7 @@ contract QVImpactStreamTest is Test { function test__distributeRevertWhen_PayoutAmountForRecipientIsZero() external callWithPoolManager { IAllo.Pool memory poolData = IAllo.Pool({ profileId: keccak256(abi.encodePacked(recipient1)), - strategy: IStrategy(address(qvImpactStream)), + strategy: IBaseStrategy(address(qvImpactStream)), token: address(0), metadata: Metadata({protocol: 0, pointer: ""}), managerRole: keccak256("MANAGER_ROLE"), @@ -175,7 +175,7 @@ contract QVImpactStreamTest is Test { IAllo.Pool memory poolData = IAllo.Pool({ profileId: keccak256(abi.encodePacked(recipient1)), - strategy: IStrategy(address(qvImpactStream)), + strategy: IBaseStrategy(address(qvImpactStream)), token: address(0), metadata: Metadata({protocol: 0, pointer: ""}), managerRole: keccak256("MANAGER_ROLE"), From c8a022076155c9f9aca6170ba1d20f655327876f Mon Sep 17 00:00:00 2001 From: 0xAustrian Date: Thu, 22 Aug 2024 15:39:43 -0300 Subject: [PATCH 02/10] test: add more unit test cases --- contracts/core/Allo.sol | 16 ++-- test/foundry/core/AlloUnit.t.sol | 129 +++++++++++++++++++++++++++---- 2 files changed, 122 insertions(+), 23 deletions(-) diff --git a/contracts/core/Allo.sol b/contracts/core/Allo.sol index 6e029df96..1e875b1af 100644 --- a/contracts/core/Allo.sol +++ b/contracts/core/Allo.sol @@ -54,7 +54,7 @@ contract Allo is /// percentage is 1e17 (10%), then 100 DAI will be deducted from the 1000 DAI and the pool will be /// funded with 900 DAI. The fee is then sent to the treasury address. /// @dev How the percentage is represented in our contracts: 1e18 = 100%, 1e17 = 10%, 1e16 = 1%, 1e15 = 0.1% - uint256 private percentFee; + uint256 internal percentFee; /// @notice Fee Allo charges for all pools on creation /// @dev This is different from the 'percentFee' in that this is a flat fee and not a percentage. So if you want to create a pool @@ -63,28 +63,28 @@ contract Allo is uint256 internal baseFee; /// @notice Incremental index to track the pools created - uint256 private _poolIndex; + uint256 internal _poolIndex; /// @notice Allo treasury - address payable private treasury; + address payable internal treasury; /// @notice Registry contract - IRegistry private registry; + IRegistry internal registry; /// @notice Maps the `_msgSender` to a `nonce` to prevent duplicates /// @dev '_msgSender' -> 'nonce' for cloning strategies - mapping(address => uint256) private _nonces; + mapping(address => uint256) internal _nonces; /// @notice Maps the pool ID to the pool details /// @dev 'Pool.id' -> 'Pool' - mapping(uint256 => Pool) private pools; + mapping(uint256 => Pool) internal pools; /// @custom:oz-upgrades-renamed-from cloneableStrategies - mapping(address => bool) private _unusedSlot; + mapping(address => bool) internal _unusedSlot; /// @notice The trusted forwarder contract address /// @dev Based on ERC2771ContextUpgradeable OZ contracts - address private _trustedForwarder; + address internal _trustedForwarder; // ==================================== // =========== Initializer ============= diff --git a/test/foundry/core/AlloUnit.t.sol b/test/foundry/core/AlloUnit.t.sol index 2852f76f5..8b4f18b75 100644 --- a/test/foundry/core/AlloUnit.t.sol +++ b/test/foundry/core/AlloUnit.t.sol @@ -6,10 +6,14 @@ import {MockAllo} from "smock/MockAllo.sol"; import {Errors} from "contracts/core/libraries/Errors.sol"; import {Metadata} from "contracts/core/libraries/Metadata.sol"; import {IBaseStrategy} from "contracts/core/interfaces/IBaseStrategy.sol"; +import {ClonesUpgradeable} from "openzeppelin-contracts-upgradeable/contracts/proxy/ClonesUpgradeable.sol"; +import {Ownable} from "solady/auth/Ownable.sol"; contract Allo is Test { MockAllo allo; + event PoolMetadataUpdated(uint256 indexed poolId, Metadata metadata); + function setUp() public virtual { allo = new MockAllo(); } @@ -107,42 +111,137 @@ contract Allo is Test { assertEq(poolId, _poolId); } - function test_CreatePoolRevertWhen_StrategyIsZeroAddress() external { + function test_CreatePoolRevertWhen_StrategyIsZeroAddress( + bytes32 _profileId, + bytes memory _initStrategyData, + address _token, + uint256 _amount, + Metadata memory _metadata, + address[] memory _managers) external { // it should revert - vm.skip(true); + vm.expectRevert(Errors.ZERO_ADDRESS.selector); + + allo.createPool( + _profileId, address(0), _initStrategyData, _token, _amount, _metadata, _managers + ); } - function test_CreatePoolWhenCallingWithProperParams() external { + function test_CreatePoolWhenCallingWithProperParams( + address _creator, + uint256 _msgValue, + bytes32 _profileId, + IBaseStrategy _strategy, + bytes memory _initStrategyData, + address _token, + uint256 _amount, + Metadata memory _metadata, + address[] memory _managers, + uint256 _poolId + ) external { + vm.skip(true); + + vm.assume(address(_strategy) != address(0)); + vm.assume(_creator != address(0)); + + address _expectedClonedStrategy = ClonesUpgradeable.predictDeterministicAddress( + address(_strategy), + keccak256(abi.encodePacked(address(allo), uint(0))), + address(allo) + ); + + allo.mock_call__createPool( + _creator, + _msgValue, + _profileId, + IBaseStrategy(_expectedClonedStrategy), + _initStrategyData, + _token, + _amount, + _metadata, + _managers, + _poolId + ); + // it should call _createPool + allo.expectCall__createPool( + _creator, _msgValue, _profileId, IBaseStrategy(_expectedClonedStrategy), _initStrategyData, _token, _amount, _metadata, _managers + ); + + deal(_creator, _msgValue); + vm.prank(_creator); + uint256 poolId = allo.createPool{value: _msgValue}( + _profileId, address(_strategy), _initStrategyData, _token, _amount, _metadata, _managers + ); + // it should return poolId - vm.skip(true); + assertEq(poolId, _poolId); } - function test_UpdatePoolMetadataGivenSenderIsManagerOfPool() external { + function test_UpdatePoolMetadataGivenSenderIsManagerOfPool(uint256 _poolId, Metadata memory _metadata) external { + allo.mock_call__checkOnlyPoolManager(_poolId, address(this)); + // it should call _checkOnlyPoolManager - // it should update metadata + allo.expectCall__checkOnlyPoolManager(_poolId, address(this)); + // it should emit event - vm.skip(true); + vm.expectEmit(); + emit PoolMetadataUpdated(_poolId, _metadata); + + allo.updatePoolMetadata(_poolId, _metadata); + + // it should update metadata + assertEq(allo.call_pools(_poolId).metadata.pointer, _metadata.pointer); + assertEq(allo.call_pools(_poolId).metadata.protocol, _metadata.protocol); } - function test_UpdateRegistryRevertWhen_SenderIsNotOwner() external { + function test_UpdateRegistryRevertWhen_SenderIsNotOwner(address _caller, address _registry) external { + vm.assume(_caller != address(0)); // owner is not set so is eq to 0 + // it should revert - vm.skip(true); + vm.expectRevert(Ownable.Unauthorized.selector); + + vm.prank(_caller); + allo.updateRegistry(_registry); } - function test_UpdateRegistryWhenSenderIsOwner() external { + function test_UpdateRegistryWhenSenderIsOwner(address _caller, address _registry) external { + vm.assume(_caller != address(0)); + + vm.prank(address(0)); + allo.transferOwnership(_caller); + + allo.mock_call__updateRegistry(_registry); + // it should call _updateRegistry - vm.skip(true); + allo.expectCall__updateRegistry(_registry); + + vm.prank(_caller); + allo.updateRegistry(_registry); } - function test_UpdateTreasuryRevertWhen_SenderIsNotOwner() external { + function test_UpdateTreasuryRevertWhen_SenderIsNotOwner(address _caller, address payable _treasury) external { + vm.assume(_caller != address(0)); // owner is not set so is eq to 0 + // it should revert - vm.skip(true); + vm.expectRevert(Ownable.Unauthorized.selector); + + vm.prank(_caller); + allo.updateTreasury(_treasury); } - function test_UpdateTreasuryWhenSenderIsOwner() external { + function test_UpdateTreasuryWhenSenderIsOwner(address _caller, address payable _treasury) external { + vm.assume(_caller != address(0)); + + vm.prank(address(0)); + allo.transferOwnership(_caller); + + allo.mock_call__updateTreasury(_treasury); + // it should call _updateTreasury - vm.skip(true); + allo.expectCall__updateTreasury(_treasury); + + vm.prank(_caller); + allo.updateTreasury(_treasury); } function test_UpdatePercentFeeRevertWhen_SenderIsNotOwner() external { From ff6ab30aebb90f0e8fa2fe2c36503bd5788acc38 Mon Sep 17 00:00:00 2001 From: 0xAustrian Date: Thu, 22 Aug 2024 19:55:04 -0300 Subject: [PATCH 03/10] test: add more test cases --- test/foundry/core/AlloUnit.t.sol | 147 +++++++++++++++++++++++++------ 1 file changed, 118 insertions(+), 29 deletions(-) diff --git a/test/foundry/core/AlloUnit.t.sol b/test/foundry/core/AlloUnit.t.sol index 8b4f18b75..4caad71a5 100644 --- a/test/foundry/core/AlloUnit.t.sol +++ b/test/foundry/core/AlloUnit.t.sol @@ -117,13 +117,12 @@ contract Allo is Test { address _token, uint256 _amount, Metadata memory _metadata, - address[] memory _managers) external { + address[] memory _managers + ) external { // it should revert vm.expectRevert(Errors.ZERO_ADDRESS.selector); - allo.createPool( - _profileId, address(0), _initStrategyData, _token, _amount, _metadata, _managers - ); + allo.createPool(_profileId, address(0), _initStrategyData, _token, _amount, _metadata, _managers); } function test_CreatePoolWhenCallingWithProperParams( @@ -144,9 +143,7 @@ contract Allo is Test { vm.assume(_creator != address(0)); address _expectedClonedStrategy = ClonesUpgradeable.predictDeterministicAddress( - address(_strategy), - keccak256(abi.encodePacked(address(allo), uint(0))), - address(allo) + address(_strategy), keccak256(abi.encodePacked(address(allo), uint256(0))), address(allo) ); allo.mock_call__createPool( @@ -164,7 +161,15 @@ contract Allo is Test { // it should call _createPool allo.expectCall__createPool( - _creator, _msgValue, _profileId, IBaseStrategy(_expectedClonedStrategy), _initStrategyData, _token, _amount, _metadata, _managers + _creator, + _msgValue, + _profileId, + IBaseStrategy(_expectedClonedStrategy), + _initStrategyData, + _token, + _amount, + _metadata, + _managers ); deal(_creator, _msgValue); @@ -229,7 +234,7 @@ contract Allo is Test { allo.updateTreasury(_treasury); } - function test_UpdateTreasuryWhenSenderIsOwner(address _caller, address payable _treasury) external { + function test_UpdateTreasuryWhenSenderIsOwner(address _caller, address payable _treasury) external { vm.assume(_caller != address(0)); vm.prank(address(0)); @@ -244,56 +249,140 @@ contract Allo is Test { allo.updateTreasury(_treasury); } - function test_UpdatePercentFeeRevertWhen_SenderIsNotOwner() external { + function test_UpdatePercentFeeRevertWhen_SenderIsNotOwner(address _caller, uint256 _percentFee) external { + vm.assume(_caller != address(0)); // owner is not set so is eq to 0 + // it should revert - vm.skip(true); + vm.expectRevert(Ownable.Unauthorized.selector); + + vm.prank(_caller); + allo.updatePercentFee(_percentFee); } - function test_UpdatePercentFeeWhenSenderIsOwner() external { + function test_UpdatePercentFeeWhenSenderIsOwner(address _caller, uint256 _percentFee) external { + vm.assume(_caller != address(0)); + + vm.prank(address(0)); + allo.transferOwnership(_caller); + + allo.mock_call__updatePercentFee(_percentFee); + // it should call _updatePercentFee - vm.skip(true); + allo.expectCall__updatePercentFee(_percentFee); + + vm.prank(_caller); + allo.updatePercentFee(_percentFee); } - function test_UpdateBaseFeeRevertWhen_SenderIsNotOwner() external { + function test_UpdateBaseFeeRevertWhen_SenderIsNotOwner(address _caller, uint256 _baseFee) external { + vm.assume(_caller != address(0)); // owner is not set so is eq to 0 + // it should revert - vm.skip(true); + vm.expectRevert(Ownable.Unauthorized.selector); + + vm.prank(_caller); + allo.updateBaseFee(_baseFee); } - function test_UpdateBaseFeeWhenSenderIsOwner() external { + function test_UpdateBaseFeeWhenSenderIsOwner(address _caller, uint256 _baseFee) external { + vm.assume(_caller != address(0)); + + vm.prank(address(0)); + allo.transferOwnership(_caller); + + allo.mock_call__updateBaseFee(_baseFee); + // it should call _updateBaseFee - vm.skip(true); + allo.expectCall__updateBaseFee(_baseFee); + + vm.prank(_caller); + allo.updateBaseFee(_baseFee); } - function test_UpdateTrustedForwarderRevertWhen_SenderIsNotOwner() external { + function test_UpdateTrustedForwarderRevertWhen_SenderIsNotOwner(address _caller, address _trustedForwarder) + external + { + vm.assume(_caller != address(0)); // owner is not set so is eq to 0 + // it should revert - vm.skip(true); + vm.expectRevert(Ownable.Unauthorized.selector); + + vm.prank(_caller); + allo.updateTrustedForwarder(_trustedForwarder); } - function test_UpdateTrustedForwarderWhenSenderIsOwner() external { + function test_UpdateTrustedForwarderWhenSenderIsOwner(address _caller, address _trustedForwarder) external { + vm.assume(_caller != address(0)); + + vm.prank(address(0)); + allo.transferOwnership(_caller); + + allo.mock_call__updateTrustedForwarder(_trustedForwarder); + // it should call _updateTrustedForwarder - vm.skip(true); + allo.expectCall__updateTrustedForwarder(_trustedForwarder); + + vm.prank(_caller); + allo.updateTrustedForwarder(_trustedForwarder); } - function test_AddPoolManagersGivenSenderIsAdminOfPoolId() external { + function test_AddPoolManagersGivenSenderIsAdminOfPoolId(uint256 _poolId, address[] memory _managers) external { + allo.mock_call__checkOnlyPoolAdmin(_poolId, address(this)); + for (uint256 i = 0; i < _managers.length; i++) { + allo.mock_call__addPoolManager(_poolId, _managers[i]); + } + // it should call _checkOnlyPoolAdmin + allo.expectCall__checkOnlyPoolAdmin(_poolId, address(this)); + // it should call _addPoolManager - vm.skip(true); + for (uint256 i = 0; i < _managers.length; i++) { + allo.expectCall__addPoolManager(_poolId, _managers[i]); + } + + allo.addPoolManagers(_poolId, _managers); } - function test_RemovePoolManagersGivenSenderIsAdminOfPoolId() external { + function test_RemovePoolManagersGivenSenderIsAdminOfPoolId(uint256 _poolId, address[] memory _managers) external { + allo.mock_call__checkOnlyPoolAdmin(_poolId, address(this)); + // it should call _checkOnlyPoolAdmin + allo.expectCall__checkOnlyPoolAdmin(_poolId, address(this)); + // it should call _revokeRole - vm.skip(true); + // TODO: expect _revokeRole + + allo.removePoolManagers(_poolId, _managers); } - function test_AddPoolManagersInMultiplePoolsGivenSenderIsAdminOfAllPoolIds() external { - // it should call addPoolManagers + function test_AddPoolManagersInMultiplePoolsGivenSenderIsAdminOfAllPoolIds( + uint256[] memory _poolIds, + address[] memory _managers + ) external { vm.skip(true); + for (uint256 i = 0; i < _poolIds.length; i++) { + allo.mock_call_addPoolManagers(_poolIds[0], _managers); + } + + // it should call addPoolManagers + // TODO: expect addPoolManagers + + allo.addPoolManagersInMultiplePools(_poolIds, _managers); } - function test_RemovePoolManagersInMultiplePoolsGivenSenderIsAdminOfAllPoolIds() external { - // it should call removePoolManagers + function test_RemovePoolManagersInMultiplePoolsGivenSenderIsAdminOfAllPoolIds( + uint256[] memory _poolIds, + address[] memory _managers + ) external { vm.skip(true); + for (uint256 i = 0; i < _poolIds.length; i++) { + allo.mock_call_removePoolManagers(_poolIds[0], _managers); + } + + // it should call removePoolManagers + // TODO: expect removePoolManagers + + allo.removePoolManagersInMultiplePools(_poolIds, _managers); } function test_RecoverFundsRevertWhen_SenderIsNotOwner() external { From 5ec2bece39fbe8a20978ec4d1fa2c55eb677e9b6 Mon Sep 17 00:00:00 2001 From: 0xAustrian Date: Fri, 23 Aug 2024 08:01:17 -0300 Subject: [PATCH 04/10] chore: add smock to CI --- .github/workflows/forge-test.yml | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/.github/workflows/forge-test.yml b/.github/workflows/forge-test.yml index e26237c24..e9b025fcd 100644 --- a/.github/workflows/forge-test.yml +++ b/.github/workflows/forge-test.yml @@ -28,6 +28,11 @@ jobs: with: version: nightly + - name: Run Smock + run: | + bun smock + id: smock + - name: Run Forge Format run: | forge fmt --check From 00b609597d9a2a6944d2af54768ffa55cb6098de Mon Sep 17 00:00:00 2001 From: 0xAustrian Date: Fri, 23 Aug 2024 08:51:43 -0300 Subject: [PATCH 05/10] test: fix broken test case --- test/foundry/core/AlloUnit.t.sol | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/test/foundry/core/AlloUnit.t.sol b/test/foundry/core/AlloUnit.t.sol index 4caad71a5..8db821fa0 100644 --- a/test/foundry/core/AlloUnit.t.sol +++ b/test/foundry/core/AlloUnit.t.sol @@ -137,13 +137,13 @@ contract Allo is Test { address[] memory _managers, uint256 _poolId ) external { - vm.skip(true); - vm.assume(address(_strategy) != address(0)); vm.assume(_creator != address(0)); address _expectedClonedStrategy = ClonesUpgradeable.predictDeterministicAddress( - address(_strategy), keccak256(abi.encodePacked(address(allo), uint256(0))), address(allo) + address(_strategy), + keccak256(abi.encodePacked(_creator, uint256(0))), + address(allo) ); allo.mock_call__createPool( From 9a4d4088dbb610b54dec7ec9ea7de7bd6515f8bf Mon Sep 17 00:00:00 2001 From: 0xAustrian Date: Fri, 23 Aug 2024 08:55:10 -0300 Subject: [PATCH 06/10] fix: format --- test/foundry/core/AlloUnit.t.sol | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/test/foundry/core/AlloUnit.t.sol b/test/foundry/core/AlloUnit.t.sol index 8db821fa0..32e296615 100644 --- a/test/foundry/core/AlloUnit.t.sol +++ b/test/foundry/core/AlloUnit.t.sol @@ -141,9 +141,7 @@ contract Allo is Test { vm.assume(_creator != address(0)); address _expectedClonedStrategy = ClonesUpgradeable.predictDeterministicAddress( - address(_strategy), - keccak256(abi.encodePacked(_creator, uint256(0))), - address(allo) + address(_strategy), keccak256(abi.encodePacked(_creator, uint256(0))), address(allo) ); allo.mock_call__createPool( From b97cbd393e3e501680817983aeda840c56e65893 Mon Sep 17 00:00:00 2001 From: 0xAustrian Date: Tue, 27 Aug 2024 10:11:45 -0300 Subject: [PATCH 07/10] test: create mock file for smock, fix TODOs --- package.json | 2 +- test/foundry/core/AlloUnit.t.sol | 34 ++++++----- test/utils/mocks/MockAllo.sol | 101 +++++++++++++++++++++++++++++++ 3 files changed, 121 insertions(+), 16 deletions(-) create mode 100644 test/utils/mocks/MockAllo.sol diff --git a/package.json b/package.json index d717cadf2..c0686b98f 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/utils/mocks/" }, "devDependencies": { "@matterlabs/hardhat-zksync-deploy": "^1.2.1", diff --git a/test/foundry/core/AlloUnit.t.sol b/test/foundry/core/AlloUnit.t.sol index 32e296615..7cd3baec1 100644 --- a/test/foundry/core/AlloUnit.t.sol +++ b/test/foundry/core/AlloUnit.t.sol @@ -2,7 +2,7 @@ pragma solidity ^0.8.19; import {Test} from "forge-std/Test.sol"; -import {MockAllo} from "smock/MockAllo.sol"; +import {MockMockAllo} from "test/smock/MockMockAllo.sol"; import {Errors} from "contracts/core/libraries/Errors.sol"; import {Metadata} from "contracts/core/libraries/Metadata.sol"; import {IBaseStrategy} from "contracts/core/interfaces/IBaseStrategy.sol"; @@ -10,12 +10,12 @@ import {ClonesUpgradeable} from "openzeppelin-contracts-upgradeable/contracts/pr import {Ownable} from "solady/auth/Ownable.sol"; contract Allo is Test { - MockAllo allo; + MockMockAllo allo; event PoolMetadataUpdated(uint256 indexed poolId, Metadata metadata); function setUp() public virtual { - allo = new MockAllo(); + allo = new MockMockAllo(); } function test_InitializeGivenUpgradeVersionIsCorrect( @@ -33,7 +33,7 @@ contract Allo is Test { allo.mock_call__updateTrustedForwarder(_trustedForwarder); // it should call _initializeOwner - // TODO: expect _initializeOwner + allo.expectCall__initializeOwner(_owner); // it should call _updateRegistry allo.expectCall__updateRegistry(_registry); @@ -193,8 +193,8 @@ contract Allo is Test { allo.updatePoolMetadata(_poolId, _metadata); // it should update metadata - assertEq(allo.call_pools(_poolId).metadata.pointer, _metadata.pointer); - assertEq(allo.call_pools(_poolId).metadata.protocol, _metadata.protocol); + assertEq(allo.getPool(_poolId).metadata.pointer, _metadata.pointer); + assertEq(allo.getPool(_poolId).metadata.protocol, _metadata.protocol); } function test_UpdateRegistryRevertWhen_SenderIsNotOwner(address _caller, address _registry) external { @@ -347,8 +347,12 @@ contract Allo is Test { // it should call _checkOnlyPoolAdmin allo.expectCall__checkOnlyPoolAdmin(_poolId, address(this)); + bytes32 _role = allo.getPool(_poolId).managerRole; + // it should call _revokeRole - // TODO: expect _revokeRole + for (uint256 i = 0; i < _managers.length; i++) { + allo.expectCall__revokeRole(_role, _managers[i]); + } allo.removePoolManagers(_poolId, _managers); } @@ -358,14 +362,14 @@ contract Allo is Test { address[] memory _managers ) external { vm.skip(true); - for (uint256 i = 0; i < _poolIds.length; i++) { - allo.mock_call_addPoolManagers(_poolIds[0], _managers); - } + // for (uint256 i = 0; i < _poolIds.length; i++) { + // allo.mock_call_addPoolManagers(_poolIds[0], _managers); + // } // it should call addPoolManagers // TODO: expect addPoolManagers - allo.addPoolManagersInMultiplePools(_poolIds, _managers); + // allo.addPoolManagersInMultiplePools(_poolIds, _managers); } function test_RemovePoolManagersInMultiplePoolsGivenSenderIsAdminOfAllPoolIds( @@ -373,14 +377,14 @@ contract Allo is Test { address[] memory _managers ) external { vm.skip(true); - for (uint256 i = 0; i < _poolIds.length; i++) { - allo.mock_call_removePoolManagers(_poolIds[0], _managers); - } + // for (uint256 i = 0; i < _poolIds.length; i++) { + // allo.mock_call_removePoolManagers(_poolIds[0], _managers); + // } // it should call removePoolManagers // TODO: expect removePoolManagers - allo.removePoolManagersInMultiplePools(_poolIds, _managers); + // allo.removePoolManagersInMultiplePools(_poolIds, _managers); } function test_RecoverFundsRevertWhen_SenderIsNotOwner() external { diff --git a/test/utils/mocks/MockAllo.sol b/test/utils/mocks/MockAllo.sol new file mode 100644 index 000000000..02113cd68 --- /dev/null +++ b/test/utils/mocks/MockAllo.sol @@ -0,0 +1,101 @@ +// SPDX-License-Identifier: UNLICENSED +pragma solidity ^0.8.19; + +import {Allo} from "contracts/core/Allo.sol"; +import {IBaseStrategy} from "contracts/core/interfaces/IBaseStrategy.sol"; +import {Metadata} from "contracts/core/libraries/Metadata.sol"; + +contract MockAllo is Allo { + constructor() Allo() {} + + function _initializeOwner(address newOwner) internal override virtual { + super._initializeOwner(newOwner); + } + + function _revokeRole(bytes32 role, address account) internal override virtual { + super._revokeRole(role, account); + } + + function _checkOnlyPoolManager(uint256 _poolId, address _address) internal view override virtual { + super._checkOnlyPoolManager(_poolId, _address); + } + + function _checkOnlyPoolAdmin(uint256 _poolId, address _address) internal view override virtual { + super._checkOnlyPoolAdmin(_poolId, _address); + } + + function _createPool( + address _creator, + uint256 _msgValue, + bytes32 _profileId, + IBaseStrategy _strategy, + bytes memory _initStrategyData, + address _token, + uint256 _amount, + Metadata memory _metadata, + address[] memory _managers + ) internal override virtual returns (uint256 poolId) { + return super._createPool( + _creator, + _msgValue, + _profileId, + _strategy, + _initStrategyData, + _token, + _amount, + _metadata, + _managers + ); + } + + function _allocate( + uint256 _poolId, + address[] memory _recipients, + uint256[] memory _amounts, + bytes memory _data, + uint256 _value, + address _allocator + ) internal virtual override { + super._allocate(_poolId, _recipients, _amounts, _data, _value, _allocator); + } + + function _fundPool(uint256 _amount, address _funder, uint256 _poolId, IBaseStrategy _strategy) internal virtual override { + super._fundPool(_amount, _funder, _poolId, _strategy); + } + + function _isPoolAdmin(uint256 _poolId, address _address) internal view virtual override returns (bool) { + return super._isPoolAdmin(_poolId, _address); + } + + function _isPoolManager(uint256 _poolId, address _address) internal view virtual override returns (bool) { + return super._isPoolManager(_poolId, _address); + } + + function _updateRegistry(address _registry) internal virtual override { + super._updateRegistry(_registry); + } + + function _updateTreasury(address payable _treasury) internal virtual override { + super._updateTreasury(_treasury); + } + + function _updatePercentFee(uint256 _percentFee) internal virtual override { + super._updatePercentFee(_percentFee); + } + + function _updateBaseFee(uint256 _baseFee) internal virtual override { + super._updateBaseFee(_baseFee); + } + + function _updateTrustedForwarder(address __trustedForwarder) internal virtual override { + super._updateTrustedForwarder(__trustedForwarder); + } + + function _addPoolManager(uint256 _poolId, address _manager) internal virtual override { + super._addPoolManager(_poolId, _manager); + } + + function _msgSender() internal view virtual override returns (address) { + return super._msgSender(); + } +} \ No newline at end of file From 35094325aab76a6c7ccff080fc96026425017127 Mon Sep 17 00:00:00 2001 From: 0xAustrian Date: Tue, 27 Aug 2024 14:06:37 -0300 Subject: [PATCH 08/10] test: solve all TODOs --- test/foundry/core/AlloUnit.t.sol | 53 ++++++++++++++++++++------------ 1 file changed, 33 insertions(+), 20 deletions(-) diff --git a/test/foundry/core/AlloUnit.t.sol b/test/foundry/core/AlloUnit.t.sol index 7cd3baec1..770594769 100644 --- a/test/foundry/core/AlloUnit.t.sol +++ b/test/foundry/core/AlloUnit.t.sol @@ -8,8 +8,11 @@ import {Metadata} from "contracts/core/libraries/Metadata.sol"; import {IBaseStrategy} from "contracts/core/interfaces/IBaseStrategy.sol"; import {ClonesUpgradeable} from "openzeppelin-contracts-upgradeable/contracts/proxy/ClonesUpgradeable.sol"; import {Ownable} from "solady/auth/Ownable.sol"; +import {LibString} from "solady/utils/LibString.sol"; contract Allo is Test { + using LibString for uint256; + MockMockAllo allo; event PoolMetadataUpdated(uint256 indexed poolId, Metadata metadata); @@ -357,34 +360,44 @@ contract Allo is Test { allo.removePoolManagers(_poolId, _managers); } - function test_AddPoolManagersInMultiplePoolsGivenSenderIsAdminOfAllPoolIds( - uint256[] memory _poolIds, - address[] memory _managers - ) external { - vm.skip(true); - // for (uint256 i = 0; i < _poolIds.length; i++) { - // allo.mock_call_addPoolManagers(_poolIds[0], _managers); - // } + function test_AddPoolManagersInMultiplePoolsGivenSenderIsAdminOfAllPoolIds(uint256[] memory _poolIds) external { + vm.assume(_poolIds.length > 0); + vm.assume(_poolIds.length < 100); + + address[] memory _managers = new address[](_poolIds.length); + for (uint256 i = 0; i < _poolIds.length; i++) { + allo.mock_call__checkOnlyPoolAdmin(_poolIds[i], address(this)); + _managers[i] = makeAddr((i + 1).toString()); + } // it should call addPoolManagers - // TODO: expect addPoolManagers + // NOTE: we cant expect addPoolManagers because is a public function. + // instead we expect the internal that is called inside addPoolManagers + for (uint256 i = 0; i < _poolIds.length; i++) { + allo.expectCall__addPoolManager(_poolIds[i], _managers[i]); + } - // allo.addPoolManagersInMultiplePools(_poolIds, _managers); + allo.addPoolManagersInMultiplePools(_poolIds, _managers); } - function test_RemovePoolManagersInMultiplePoolsGivenSenderIsAdminOfAllPoolIds( - uint256[] memory _poolIds, - address[] memory _managers - ) external { - vm.skip(true); - // for (uint256 i = 0; i < _poolIds.length; i++) { - // allo.mock_call_removePoolManagers(_poolIds[0], _managers); - // } + function test_RemovePoolManagersInMultiplePoolsGivenSenderIsAdminOfAllPoolIds(uint256[] memory _poolIds) external { + vm.assume(_poolIds.length > 0); + vm.assume(_poolIds.length < 100); + + address[] memory _managers = new address[](_poolIds.length); + for (uint256 i = 0; i < _poolIds.length; i++) { + allo.mock_call__checkOnlyPoolAdmin(_poolIds[i], address(this)); + _managers[i] = makeAddr((i + 1).toString()); + } // it should call removePoolManagers - // TODO: expect removePoolManagers + // NOTE: we cant expect removePoolManagers because is a public function. + // instead we expect the internal that is called inside removePoolManagers + for (uint256 i = 0; i < _poolIds.length; i++) { + allo.expectCall__revokeRole(allo.getPool(_poolIds[i]).managerRole, _managers[i]); + } - // allo.removePoolManagersInMultiplePools(_poolIds, _managers); + allo.removePoolManagersInMultiplePools(_poolIds, _managers); } function test_RecoverFundsRevertWhen_SenderIsNotOwner() external { From 0f0dfb79b5056ffa289ca501da55980e6d395b24 Mon Sep 17 00:00:00 2001 From: 0xAustrian Date: Thu, 29 Aug 2024 08:48:59 -0300 Subject: [PATCH 09/10] fix: fmt --- foundry.toml | 4 ++-- test/utils/mocks/MockAllo.sol | 28 ++++++++++++---------------- 2 files changed, 14 insertions(+), 18 deletions(-) diff --git a/foundry.toml b/foundry.toml index c231e728f..75d40cfea 100644 --- a/foundry.toml +++ b/foundry.toml @@ -39,8 +39,8 @@ allow_paths = ['node_modules'] [fmt] ignore = [ - 'contracts/strategies/_poc/qv-hackathon/SchemaResolver.sol', - 'contracts/strategies/_poc/direct-grants-simple/DirectGrantsSimpleStrategy.sol' + 'contracts/strategies/deprecated/_poc/qv-hackathon/SchemaResolver.sol', + 'contracts/strategies/deprecated/_poc/direct-grants-simple/DirectGrantsSimpleStrategy.sol' ] [rpc_endpoints] diff --git a/test/utils/mocks/MockAllo.sol b/test/utils/mocks/MockAllo.sol index 181053d76..e912043bd 100644 --- a/test/utils/mocks/MockAllo.sol +++ b/test/utils/mocks/MockAllo.sol @@ -8,19 +8,19 @@ import {Metadata} from "contracts/core/libraries/Metadata.sol"; contract MockAllo is Allo { constructor() Allo() {} - function _initializeOwner(address newOwner) internal override virtual { + function _initializeOwner(address newOwner) internal virtual override { super._initializeOwner(newOwner); } - function _revokeRole(bytes32 role, address account) internal override virtual { + function _revokeRole(bytes32 role, address account) internal virtual override { super._revokeRole(role, account); } - function _checkOnlyPoolManager(uint256 _poolId, address _address) internal view override virtual { + function _checkOnlyPoolManager(uint256 _poolId, address _address) internal view virtual override { super._checkOnlyPoolManager(_poolId, _address); } - function _checkOnlyPoolAdmin(uint256 _poolId, address _address) internal view override virtual { + function _checkOnlyPoolAdmin(uint256 _poolId, address _address) internal view virtual override { super._checkOnlyPoolAdmin(_poolId, _address); } @@ -34,17 +34,9 @@ contract MockAllo is Allo { uint256 _amount, Metadata memory _metadata, address[] memory _managers - ) internal override virtual returns (uint256 poolId) { + ) internal virtual override returns (uint256 poolId) { return super._createPool( - _creator, - _msgValue, - _profileId, - _strategy, - _initStrategyData, - _token, - _amount, - _metadata, - _managers + _creator, _msgValue, _profileId, _strategy, _initStrategyData, _token, _amount, _metadata, _managers ); } @@ -59,7 +51,11 @@ contract MockAllo is Allo { super._allocate(_poolId, _recipients, _amounts, _data, _value, _allocator); } - function _fundPool(uint256 _amount, address _funder, uint256 _poolId, IBaseStrategy _strategy) internal virtual override { + function _fundPool(uint256 _amount, address _funder, uint256 _poolId, IBaseStrategy _strategy) + internal + virtual + override + { super._fundPool(_amount, _funder, _poolId, _strategy); } @@ -98,4 +94,4 @@ contract MockAllo is Allo { function _msgSender() internal view virtual override returns (address) { return super._msgSender(); } -} \ No newline at end of file +} From e7ad851387b533bdbc72d80d95ff7aab42bcfcf5 Mon Sep 17 00:00:00 2001 From: 0xAustrian Date: Mon, 2 Sep 2024 07:15:46 -0300 Subject: [PATCH 10/10] fix: CI --- .github/workflows/forge-test.yml | 5 ----- 1 file changed, 5 deletions(-) diff --git a/.github/workflows/forge-test.yml b/.github/workflows/forge-test.yml index a1302408e..53c066cec 100644 --- a/.github/workflows/forge-test.yml +++ b/.github/workflows/forge-test.yml @@ -33,11 +33,6 @@ jobs: bun smock id: smock - - name: Run Smock - run: | - bun smock - id: smock - - name: Run Forge Format run: | forge fmt --check