From 2224b1f7661869b2c255766474948f042e60bc8a Mon Sep 17 00:00:00 2001 From: Paul Razvan Berg Date: Wed, 5 Jul 2023 14:59:50 +0300 Subject: [PATCH] test: polish Iaro's changes --- test/Base.t.sol | 6 ++--- test/mocks/plugins/PluginReverter.sol | 2 +- test/mocks/targets/TargetReverter.sol | 2 +- test/proxy/execute/execute.t.sol | 4 +-- test/proxy/execute/execute.tree | 6 ++--- test/proxy/receive/receive.t.sol | 2 +- test/proxy/run-plugin/runPlugin.t.sol | 27 ++++++++++++++----- test/proxy/run-plugin/runPlugin.tree | 9 ++++--- .../install-plugin/installPlugin.t.sol | 3 ++- .../set-permission/setPermission.t.sol | 3 ++- .../uninstall-plugin/uninstallPlugin.t.sol | 20 +++++--------- 11 files changed, 47 insertions(+), 37 deletions(-) diff --git a/test/Base.t.sol b/test/Base.t.sol index 5cee8b5..73cdf3d 100644 --- a/test/Base.t.sol +++ b/test/Base.t.sol @@ -127,7 +127,7 @@ abstract contract Base_Test is Assertions, Events, StdCheats, StdUtils { } /// @dev Generates an address by hashing the name, labels the address and funds it with test assets. - function createUser(string memory name) private returns (address payable addr) { + function createUser(string memory name) internal returns (address payable addr) { addr = payable(makeAddr(name)); vm.deal({ account: addr, newBalance: 100 ether }); } @@ -138,7 +138,7 @@ abstract contract Base_Test is Assertions, Events, StdCheats, StdUtils { } /// @dev Conditionally deploy the registry either normally or from a source precompiled with `--via-ir`. - function deployRegistryConditionally() private { + function deployRegistryConditionally() internal { if (!isTestOptimizedProfile()) { registry = new PRBProxyRegistry(); } else { @@ -149,7 +149,7 @@ abstract contract Base_Test is Assertions, Events, StdCheats, StdUtils { } /// @dev Reads the proxy bytecode either normally or from precompiled source. - function getProxyBytecode() private returns (bytes memory) { + function getProxyBytecode() internal returns (bytes memory) { if (!isTestOptimizedProfile()) { return type(PRBProxy).creationCode; } else { diff --git a/test/mocks/plugins/PluginReverter.sol b/test/mocks/plugins/PluginReverter.sol index c9baa78..0b4adcf 100644 --- a/test/mocks/plugins/PluginReverter.sol +++ b/test/mocks/plugins/PluginReverter.sol @@ -13,7 +13,7 @@ contract PluginReverter is IPRBProxyPlugin, TargetReverter { methods[1] = this.withCustomError.selector; methods[2] = this.withRequire.selector; methods[3] = this.withReasonString.selector; - methods[4] = this.dueToNoPayableFunction.selector; + methods[4] = this.notPayable.selector; return methods; } diff --git a/test/mocks/targets/TargetReverter.sol b/test/mocks/targets/TargetReverter.sol index 06a9524..3ff7a4a 100644 --- a/test/mocks/targets/TargetReverter.sol +++ b/test/mocks/targets/TargetReverter.sol @@ -20,7 +20,7 @@ contract TargetReverter { revert("You shall not pass"); } - function dueToNoPayableFunction() external pure returns (uint256) { + function notPayable() external pure returns (uint256) { return 0; } } diff --git a/test/proxy/execute/execute.t.sol b/test/proxy/execute/execute.t.sol index 909ae34..caee1e8 100644 --- a/test/proxy/execute/execute.t.sol +++ b/test/proxy/execute/execute.t.sol @@ -143,13 +143,13 @@ contract Execute_Test is Proxy_Test { proxy.execute(address(targets.reverter), data); } - function test_RevertWhen_Error_NoPayableFunction() + function test_RevertWhen_Error_NotPayable() external whenCallerAuthorized whenTargetContract whenDelegateCallReverts { - bytes memory data = bytes.concat(targets.reverter.dueToNoPayableFunction.selector); + bytes memory data = bytes.concat(targets.reverter.notPayable.selector); vm.expectRevert(); proxy.execute{ value: 0.1 ether }(address(targets.reverter), data); } diff --git a/test/proxy/execute/execute.tree b/test/proxy/execute/execute.tree index 1ad67e3..ce60302 100644 --- a/test/proxy/execute/execute.tree +++ b/test/proxy/execute/execute.tree @@ -22,11 +22,11 @@ execute.t.sol │ ├── it should revert with a custom error │ ├── it should revert with a require │ ├── it should revert with a reason string - │ └── it should revert due to no payable function + │ └── it should revert when sending ETH to a non-payable function └── when the delegate call does not revert - ├── when Ether is sent + ├── when ETH is sent │ └── it should return the Ether amount - └── when no Ether is sent + └── when no ETH is sent ├── when the target self-destructs │ └── it should return an empty response and send the ETH to the SELFDESTRUCT recipient └── when the target does not self-destruct diff --git a/test/proxy/receive/receive.t.sol b/test/proxy/receive/receive.t.sol index 7df595a..f93e014 100644 --- a/test/proxy/receive/receive.t.sol +++ b/test/proxy/receive/receive.t.sol @@ -4,7 +4,7 @@ pragma solidity >=0.8.19 <=0.9.0; import { Proxy_Test } from "../Proxy.t.sol"; contract Receive_Test is Proxy_Test { - uint256 private value = 1 ether; + uint256 internal value = 1 ether; function setUp() public virtual override { Proxy_Test.setUp(); diff --git a/test/proxy/run-plugin/runPlugin.t.sol b/test/proxy/run-plugin/runPlugin.t.sol index 5641738..80e64bd 100644 --- a/test/proxy/run-plugin/runPlugin.t.sol +++ b/test/proxy/run-plugin/runPlugin.t.sol @@ -128,8 +128,8 @@ contract RunPlugin_Test is Proxy_Test { registry.installPlugin(plugins.selfDestructer); (bool success, bytes memory response) = address(proxy).call(abi.encodeWithSelector(plugins.selfDestructer.destroyMe.selector, users.bob)); - assertTrue(success); - assertEq(response.length, 0); + assertTrue(success, "selfDestructer.destroyMe failed"); + assertEq(response.length, 0, "selfDestructer.destroyMe response length mismatch"); // Assert that Bob's balance has increased by the contract's balance. uint256 actualBobBalance = users.bob.balance; @@ -148,6 +148,22 @@ contract RunPlugin_Test is Proxy_Test { whenDelegateCallDoesNotRevert whenNoEtherSent whenPluginDoesNotSelfDestruct + { + registry.installPlugin(plugins.basic); + (bool success, bytes memory actualResponse) = + address(proxy).call(abi.encodeWithSelector(plugins.basic.foo.selector)); + bytes memory expectedResponse = abi.encode(bytes("foo")); + assertTrue(success, "basic.foo failed"); + assertEq(actualResponse, expectedResponse, "basic.foo response mismatch"); + } + + function test_RunPlugin_Event() + external + whenPluginInstalled + whenDelegateCallReverts + whenDelegateCallDoesNotRevert + whenNoEtherSent + whenPluginDoesNotSelfDestruct { registry.installPlugin(plugins.basic); vm.expectEmit({ emitter: address(proxy) }); @@ -156,10 +172,7 @@ contract RunPlugin_Test is Proxy_Test { data: abi.encodeWithSelector(TargetBasic.foo.selector), response: abi.encode(bytes("foo")) }); - (bool success, bytes memory actualResponse) = - address(proxy).call(abi.encodeWithSelector(plugins.basic.foo.selector)); - assertTrue(success); - bytes memory expectedResponse = abi.encode(bytes("foo")); - assertEq(actualResponse, expectedResponse, "basic.foo response mismatch"); + (bool success,) = address(proxy).call(abi.encodeWithSelector(plugins.basic.foo.selector)); + success; } } diff --git a/test/proxy/run-plugin/runPlugin.tree b/test/proxy/run-plugin/runPlugin.tree index 0492bdd..690dc11 100644 --- a/test/proxy/run-plugin/runPlugin.tree +++ b/test/proxy/run-plugin/runPlugin.tree @@ -17,11 +17,12 @@ runPlugin.t.sol │ ├── it should revert with a require │ └── it should revert with a reason string └── when the delegate call does not revert - ├── when Ether is sent - │ └── it should return the Ether amount - └── when no Ether is sent + ├── when the plugin receives ETH + │ └── it should return the ETH amount + └── when the plugin does not receive ETH ├── when the plugin self-destructs │ └── it should return an empty response and send the ETH to the SELFDESTRUCT recipient └── when the plugin does not self-destruct - └── it should run the plugin and emit a {RunPlugin} event + ├── it should run the plugin + └── it should emit a {RunPlugin} event diff --git a/test/registry/install-plugin/installPlugin.t.sol b/test/registry/install-plugin/installPlugin.t.sol index c0d67c8..714802b 100644 --- a/test/registry/install-plugin/installPlugin.t.sol +++ b/test/registry/install-plugin/installPlugin.t.sol @@ -9,8 +9,9 @@ import { Registry_Test } from "../Registry.t.sol"; contract InstallPlugin_Test is Registry_Test { function test_RevertWhen_CallerDoesNotHaveProxy() external { vm.expectRevert( - abi.encodeWithSelector(IPRBProxyRegistry.PRBProxyRegistry_CallerDoesNotHaveProxy.selector, users.alice) + abi.encodeWithSelector(IPRBProxyRegistry.PRBProxyRegistry_CallerDoesNotHaveProxy.selector, users.bob) ); + changePrank({ msgSender: users.bob }); registry.installPlugin(plugins.empty); } diff --git a/test/registry/set-permission/setPermission.t.sol b/test/registry/set-permission/setPermission.t.sol index b64e34d..df80db5 100644 --- a/test/registry/set-permission/setPermission.t.sol +++ b/test/registry/set-permission/setPermission.t.sol @@ -8,8 +8,9 @@ import { Registry_Test } from "../Registry.t.sol"; contract SetPermission_Test is Registry_Test { function test_RevertWhen_CallerDoesNotHaveProxy() external { vm.expectRevert( - abi.encodeWithSelector(IPRBProxyRegistry.PRBProxyRegistry_CallerDoesNotHaveProxy.selector, users.alice) + abi.encodeWithSelector(IPRBProxyRegistry.PRBProxyRegistry_CallerDoesNotHaveProxy.selector, users.bob) ); + changePrank({ msgSender: users.bob }); registry.setPermission({ envoy: users.envoy, target: address(targets.basic), permission: true }); } diff --git a/test/registry/uninstall-plugin/uninstallPlugin.t.sol b/test/registry/uninstall-plugin/uninstallPlugin.t.sol index e12a349..090cbde 100644 --- a/test/registry/uninstall-plugin/uninstallPlugin.t.sol +++ b/test/registry/uninstall-plugin/uninstallPlugin.t.sol @@ -9,8 +9,9 @@ import { Registry_Test } from "../Registry.t.sol"; contract UninstallPlugin_Test is Registry_Test { function test_RevertWhen_CallerDoesNotHaveProxy() external { vm.expectRevert( - abi.encodeWithSelector(IPRBProxyRegistry.PRBProxyRegistry_CallerDoesNotHaveProxy.selector, users.alice) + abi.encodeWithSelector(IPRBProxyRegistry.PRBProxyRegistry_CallerDoesNotHaveProxy.selector, users.bob) ); + changePrank({ msgSender: users.bob }); registry.uninstallPlugin(plugins.empty); } @@ -20,13 +21,10 @@ contract UninstallPlugin_Test is Registry_Test { } function test_RevertWhen_PluginUnknown() external whenCallerHasProxy { - // Assert that every plugin method is uninstalled. - checkThatPluginMethodsArentInstalled(users.alice, plugins.basic); - vm.expectRevert( - abi.encodeWithSelector(IPRBProxyRegistry.PRBProxyRegistry_PluginUnknown.selector, plugins.basic) + abi.encodeWithSelector(IPRBProxyRegistry.PRBProxyRegistry_PluginUnknown.selector, plugins.empty) ); - registry.uninstallPlugin(plugins.basic); + registry.uninstallPlugin(plugins.empty); } modifier whenPluginKnown() { @@ -39,15 +37,11 @@ contract UninstallPlugin_Test is Registry_Test { registry.uninstallPlugin(plugins.basic); // Assert that every plugin method has been uninstalled. - checkThatPluginMethodsArentInstalled(users.alice, plugins.basic); - } - - function checkThatPluginMethodsArentInstalled(address owner, IPRBProxyPlugin plugin) private { - bytes4[] memory pluginMethods = plugin.getMethods(); + bytes4[] memory pluginMethods = plugins.basic.getMethods(); for (uint256 i = 0; i < pluginMethods.length; ++i) { - IPRBProxyPlugin actualPlugin = registry.getPluginByOwner({ owner: owner, method: pluginMethods[i] }); + IPRBProxyPlugin actualPlugin = registry.getPluginByOwner({ owner: users.alice, method: pluginMethods[i] }); IPRBProxyPlugin expectedPlugin = IPRBProxyPlugin(address(0)); - assertEq(actualPlugin, expectedPlugin, "plugin method installed"); + assertEq(actualPlugin, expectedPlugin, "plugin method still installed"); } }