From 3985be8e0ad7099d3eb0e8901adf29f67383dcc3 Mon Sep 17 00:00:00 2001 From: Michael Amadi Date: Fri, 22 Nov 2024 22:32:33 +0100 Subject: [PATCH 1/3] add test for L1ChugSplashProxy --- .../test/legacy/L1ChugSplashProxy.t.sol | 166 ++++++++++++++++++ 1 file changed, 166 insertions(+) create mode 100644 packages/contracts-bedrock/test/legacy/L1ChugSplashProxy.t.sol diff --git a/packages/contracts-bedrock/test/legacy/L1ChugSplashProxy.t.sol b/packages/contracts-bedrock/test/legacy/L1ChugSplashProxy.t.sol new file mode 100644 index 00000000000..0b16460d177 --- /dev/null +++ b/packages/contracts-bedrock/test/legacy/L1ChugSplashProxy.t.sol @@ -0,0 +1,166 @@ +// SPDX-License-Identifier: UNLICENSED +pragma solidity 0.8.15; + +// Testing utilities +import { Test } from "forge-std/Test.sol"; + +// Target contract +import { L1ChugSplashProxy } from "src/legacy/L1ChugSplashProxy.sol"; + +contract L1ChugSplashProxyWrapper is L1ChugSplashProxy { + constructor(address admin) L1ChugSplashProxy(admin) { } + + function getDeployCodePrefix() public pure returns (bytes13) { + return DEPLOY_CODE_PREFIX; + } +} + +contract Owner { + bool public isUpgrading; + + function setIsUpgrading(bool _isUpgrading) public { + isUpgrading = _isUpgrading; + } +} + +contract Implementation { + function setCode(bytes memory) public pure returns (uint256) { + return 1; + } + + function setStorage(bytes32, bytes32) public pure returns (uint256) { + return 2; + } + + function setOwner(address) public pure returns (uint256) { + return 3; + } + + function getOwner() public pure returns (uint256) { + return 4; + } + + function getImplementation() public pure returns (uint256) { + return 5; + } +} + +contract L1ChugSplashProxy_Test is Test { + L1ChugSplashProxyWrapper proxy; + address impl; + address owner = makeAddr("owner"); + address alice = makeAddr("alice"); + + function setUp() public { + proxy = new L1ChugSplashProxyWrapper(owner); + vm.prank(owner); + assertEq(proxy.getOwner(), owner); + + vm.prank(owner); + proxy.setCode(type(Implementation).runtimeCode); + + vm.prank(owner); + impl = proxy.getImplementation(); + } + + function test_getDeployCodePrefix_works() public view { + assertTrue(proxy.getDeployCodePrefix() == 0x600D380380600D6000396000f3); + } + + function test_setCode_whenOwner_succeeds() public { + vm.prank(owner); + proxy.setCode(hex"604260005260206000f3"); + + vm.prank(owner); + assertNotEq(proxy.getImplementation(), impl); + } + + function test_setCode_whenNotOwner_works() public view { + uint256 ret = Implementation(address(proxy)).setCode(hex"604260005260206000f3"); + assertEq(ret, 1); + } + + function test_setCode_whenOwnerSameBytecode_works() public { + vm.prank(owner); + proxy.setCode(type(Implementation).runtimeCode); + + // does not deploy new implementation + vm.prank(owner); + assertEq(proxy.getImplementation(), impl); + } + + // If this solc version/settings change and modifying this proves time consuming, we can just remove it. + function test_setCode_whenOwnerAndDeployOutOfGas_reverts() public { + vm.prank(owner); + vm.expectRevert(bytes("L1ChugSplashProxy: code was not correctly deployed")); // Ran out of gas + proxy.setCode{ gas: 65_000 }( + hex"fefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefefe" + ); + } + + function test_calls_whenNotOwnerNoImplementation_reverts() public { + proxy = new L1ChugSplashProxyWrapper(owner); + + vm.expectRevert(bytes("L1ChugSplashProxy: implementation is not set yet")); + Implementation(address(proxy)).setCode(hex"604260005260206000f3"); + } + + function test_calls_whenUpgrading_reverts() public { + Owner ownerContract = new Owner(); + vm.prank(owner); + proxy.setOwner(address(ownerContract)); + + ownerContract.setIsUpgrading(true); + + vm.expectRevert(bytes("L1ChugSplashProxy: system is currently being upgraded")); + Implementation(address(proxy)).setCode(hex"604260005260206000f3"); + } + + function test_setStorage_whenOwner_works() public { + vm.prank(owner); + proxy.setStorage(bytes32(0), bytes32(uint256(42))); + assertEq(vm.load(address(proxy), bytes32(0)), bytes32(uint256(42))); + } + + function test_setStorage_whenNotOwner_works() public view { + uint256 ret = Implementation(address(proxy)).setStorage(bytes32(0), bytes32(uint256(42))); + assertEq(ret, 2); + assertEq(vm.load(address(proxy), bytes32(0)), bytes32(uint256(0))); + } + + function test_setOwner_whenOwner_works() public { + vm.prank(owner); + proxy.setOwner(alice); + + vm.prank(alice); + assertEq(proxy.getOwner(), alice); + } + + function test_setOwner_whenNotOwner_works() public { + uint256 ret = Implementation(address(proxy)).setOwner(alice); + assertEq(ret, 3); + + vm.prank(owner); + assertEq(proxy.getOwner(), owner); + } + + function test_getOwner_whenOwner_works() public { + vm.prank(owner); + assertEq(proxy.getOwner(), owner); + } + + function test_getOwner_whenNotOwner_works() public view { + uint256 ret = Implementation(address(proxy)).getOwner(); + assertEq(ret, 4); + } + + function test_getImplementation_whenOwner_works() public { + vm.prank(owner); + assertEq(proxy.getImplementation(), impl); + } + + function test_getImplementation_whenNotOwner_works() public view { + uint256 ret = Implementation(address(proxy)).getImplementation(); + assertEq(ret, 5); + } +} From 30017535f2819060d7ba5caa0d357f600a36247e Mon Sep 17 00:00:00 2001 From: Michael Amadi Date: Fri, 22 Nov 2024 22:40:56 +0100 Subject: [PATCH 2/3] fixes --- .../test/legacy/L1ChugSplashProxy.t.sol | 31 +++++++++---------- 1 file changed, 15 insertions(+), 16 deletions(-) diff --git a/packages/contracts-bedrock/test/legacy/L1ChugSplashProxy.t.sol b/packages/contracts-bedrock/test/legacy/L1ChugSplashProxy.t.sol index 0b16460d177..5611d641911 100644 --- a/packages/contracts-bedrock/test/legacy/L1ChugSplashProxy.t.sol +++ b/packages/contracts-bedrock/test/legacy/L1ChugSplashProxy.t.sol @@ -5,15 +5,8 @@ pragma solidity 0.8.15; import { Test } from "forge-std/Test.sol"; // Target contract -import { L1ChugSplashProxy } from "src/legacy/L1ChugSplashProxy.sol"; - -contract L1ChugSplashProxyWrapper is L1ChugSplashProxy { - constructor(address admin) L1ChugSplashProxy(admin) { } - - function getDeployCodePrefix() public pure returns (bytes13) { - return DEPLOY_CODE_PREFIX; - } -} +import { IL1ChugSplashProxy } from "src/legacy/interfaces/IL1ChugSplashProxy.sol"; +import { DeployUtils } from "scripts/libraries/DeployUtils.sol"; contract Owner { bool public isUpgrading; @@ -46,13 +39,18 @@ contract Implementation { } contract L1ChugSplashProxy_Test is Test { - L1ChugSplashProxyWrapper proxy; + IL1ChugSplashProxy proxy; address impl; address owner = makeAddr("owner"); address alice = makeAddr("alice"); function setUp() public { - proxy = new L1ChugSplashProxyWrapper(owner); + proxy = IL1ChugSplashProxy( + DeployUtils.create1({ + _name: "L1ChugSplashProxy", + _args: DeployUtils.encodeConstructor(abi.encodeCall(IL1ChugSplashProxy.__constructor__, (owner))) + }) + ); vm.prank(owner); assertEq(proxy.getOwner(), owner); @@ -63,10 +61,6 @@ contract L1ChugSplashProxy_Test is Test { impl = proxy.getImplementation(); } - function test_getDeployCodePrefix_works() public view { - assertTrue(proxy.getDeployCodePrefix() == 0x600D380380600D6000396000f3); - } - function test_setCode_whenOwner_succeeds() public { vm.prank(owner); proxy.setCode(hex"604260005260206000f3"); @@ -99,7 +93,12 @@ contract L1ChugSplashProxy_Test is Test { } function test_calls_whenNotOwnerNoImplementation_reverts() public { - proxy = new L1ChugSplashProxyWrapper(owner); + proxy = IL1ChugSplashProxy( + DeployUtils.create1({ + _name: "L1ChugSplashProxy", + _args: DeployUtils.encodeConstructor(abi.encodeCall(IL1ChugSplashProxy.__constructor__, (owner))) + }) + ); vm.expectRevert(bytes("L1ChugSplashProxy: implementation is not set yet")); Implementation(address(proxy)).setCode(hex"604260005260206000f3"); From c847f5f2a66a629262ca161372ce6d03461473a2 Mon Sep 17 00:00:00 2001 From: Michael Amadi Date: Fri, 22 Nov 2024 22:51:23 +0100 Subject: [PATCH 3/3] fixes --- .../test/legacy/L1ChugSplashProxy.t.sol | 19 ++++++++++++++++++- 1 file changed, 18 insertions(+), 1 deletion(-) diff --git a/packages/contracts-bedrock/test/legacy/L1ChugSplashProxy.t.sol b/packages/contracts-bedrock/test/legacy/L1ChugSplashProxy.t.sol index 5611d641911..04a73f4998a 100644 --- a/packages/contracts-bedrock/test/legacy/L1ChugSplashProxy.t.sol +++ b/packages/contracts-bedrock/test/legacy/L1ChugSplashProxy.t.sol @@ -61,6 +61,7 @@ contract L1ChugSplashProxy_Test is Test { impl = proxy.getImplementation(); } + /// @notice Tests that the owner can deploy a new implementation with a given runtime code function test_setCode_whenOwner_succeeds() public { vm.prank(owner); proxy.setCode(hex"604260005260206000f3"); @@ -69,11 +70,14 @@ contract L1ChugSplashProxy_Test is Test { assertNotEq(proxy.getImplementation(), impl); } + /// @notice Tests that when not the owner, `setCode` delegatecalls the implementation function test_setCode_whenNotOwner_works() public view { uint256 ret = Implementation(address(proxy)).setCode(hex"604260005260206000f3"); assertEq(ret, 1); } + /// @notice Tests that when the owner deploys the same bytecode as the existing implementation, + /// it does not deploy a new implementation function test_setCode_whenOwnerSameBytecode_works() public { vm.prank(owner); proxy.setCode(type(Implementation).runtimeCode); @@ -83,7 +87,9 @@ contract L1ChugSplashProxy_Test is Test { assertEq(proxy.getImplementation(), impl); } - // If this solc version/settings change and modifying this proves time consuming, we can just remove it. + /// @notice Tests that when the owner calls `setCode` with insufficient gas to complete the implementation + /// contract's deployment, it reverts. + /// @dev If this solc version/settings change and modifying this proves time consuming, we can just remove it. function test_setCode_whenOwnerAndDeployOutOfGas_reverts() public { vm.prank(owner); vm.expectRevert(bytes("L1ChugSplashProxy: code was not correctly deployed")); // Ran out of gas @@ -92,6 +98,7 @@ contract L1ChugSplashProxy_Test is Test { ); } + /// @notice Tests that when the caller is not the owner and the implementation is not set, all calls reverts function test_calls_whenNotOwnerNoImplementation_reverts() public { proxy = IL1ChugSplashProxy( DeployUtils.create1({ @@ -104,6 +111,8 @@ contract L1ChugSplashProxy_Test is Test { Implementation(address(proxy)).setCode(hex"604260005260206000f3"); } + /// @notice Tests that when the caller is not the owner but the owner has marked `isUpgrading` as true, the call + /// reverts function test_calls_whenUpgrading_reverts() public { Owner ownerContract = new Owner(); vm.prank(owner); @@ -115,18 +124,21 @@ contract L1ChugSplashProxy_Test is Test { Implementation(address(proxy)).setCode(hex"604260005260206000f3"); } + /// @notice Tests that the owner can set storage of the proxy function test_setStorage_whenOwner_works() public { vm.prank(owner); proxy.setStorage(bytes32(0), bytes32(uint256(42))); assertEq(vm.load(address(proxy), bytes32(0)), bytes32(uint256(42))); } + /// @notice Tests that when not the owner, `setStorage` delegatecalls the implementation function test_setStorage_whenNotOwner_works() public view { uint256 ret = Implementation(address(proxy)).setStorage(bytes32(0), bytes32(uint256(42))); assertEq(ret, 2); assertEq(vm.load(address(proxy), bytes32(0)), bytes32(uint256(0))); } + /// @notice Tests that the owner can set the owner of the proxy function test_setOwner_whenOwner_works() public { vm.prank(owner); proxy.setOwner(alice); @@ -135,6 +147,7 @@ contract L1ChugSplashProxy_Test is Test { assertEq(proxy.getOwner(), alice); } + /// @notice Tests that when not the owner, `setOwner` delegatecalls the implementation function test_setOwner_whenNotOwner_works() public { uint256 ret = Implementation(address(proxy)).setOwner(alice); assertEq(ret, 3); @@ -143,21 +156,25 @@ contract L1ChugSplashProxy_Test is Test { assertEq(proxy.getOwner(), owner); } + /// @notice Tests that the owner can get the owner of the proxy function test_getOwner_whenOwner_works() public { vm.prank(owner); assertEq(proxy.getOwner(), owner); } + /// @notice Tests that when not the owner, `getOwner` delegatecalls the implementation function test_getOwner_whenNotOwner_works() public view { uint256 ret = Implementation(address(proxy)).getOwner(); assertEq(ret, 4); } + /// @notice Tests that the owner can get the implementation of the proxy function test_getImplementation_whenOwner_works() public { vm.prank(owner); assertEq(proxy.getImplementation(), impl); } + /// @notice Tests that when not the owner, `getImplementation` delegatecalls the implementation function test_getImplementation_whenNotOwner_works() public view { uint256 ret = Implementation(address(proxy)).getImplementation(); assertEq(ret, 5);