From 5f3fbef6be2f19f2637921e70b39b87163fc8d11 Mon Sep 17 00:00:00 2001 From: Maurelian Date: Thu, 12 Sep 2024 14:21:50 -0400 Subject: [PATCH 1/3] feat: Scaffolding for DeployAuthSystem Script --- .../scripts/DeployAuthSystem.s.sol | 68 ++++++++++++ .../test/DeployAuthSystem.t.sol | 104 +++++++++++++++++- 2 files changed, 170 insertions(+), 2 deletions(-) diff --git a/packages/contracts-bedrock/scripts/DeployAuthSystem.s.sol b/packages/contracts-bedrock/scripts/DeployAuthSystem.s.sol index 13ac34ea48c8..cca7f054afcd 100644 --- a/packages/contracts-bedrock/scripts/DeployAuthSystem.s.sol +++ b/packages/contracts-bedrock/scripts/DeployAuthSystem.s.sol @@ -48,6 +48,7 @@ contract DeployAuthSystemInput is CommonBase { } function owners() public view returns (address[] memory) { + // expecting to trigger this require(_owners.length != 0, "DeployAuthSystemInput: owners not set"); return _owners; } @@ -76,3 +77,70 @@ contract DeployAuthSystemOutput is CommonBase { return _safe; } } + +// For all broadcasts in this script we explicitly specify the deployer as `msg.sender` because for +// testing we deploy this script from a test contract. If we provide no argument, the foundry +// default sender would be the broadcaster during test, but the broadcaster needs to be the deployer +// since they are set to the initial proxy admin owner. +contract DeployAuthSystem is Script { + // -------- Core Deployment Methods -------- + + // This entrypoint is for end-users to deploy from an input file and write to an output file. + // In this usage, we don't need the input and output contract functionality, so we deploy them + // here and abstract that architectural detail away from the end user. + function run(string memory _infile, string memory _outfile) public { + // End-user without file IO, so etch the IO helper contracts. + (DeployAuthSystemInput dasi, DeployAuthSystemOutput daso) = etchIOContracts(); + + // Load the input file into the input contract. + dasi.loadInputFile(_infile); + + // Run the deployment script and write outputs to the DeployAuthSystemOutput contract. + run(dasi, daso); + + // Write the output data to a file. + daso.writeOutputFile(_outfile); + } + + // This entrypoint is useful for testing purposes, as it doesn't use any file I/O. + function run(DeployAuthSystemInput _dasi, DeployAuthSystemOutput _daso) public { + deploySafe(_dasi, _daso); + } + + function deploySafe(DeployAuthSystemInput _dasi, DeployAuthSystemOutput _daso) public { + address[] memory owners = _dasi.owners(); + uint256 threshold = _dasi.threshold(); + // Silence unused variable warnings + owners; + threshold; + + // TODO: replace with a real deployment. The safe deployment logic is fairly complex, so for the purposes of + // this scaffolding PR we'll just etch the code. + // makeAddr("safe") = 0xDC93f9959c0F9c3849461B6468B4592a19567E09 + address safe = 0xDC93f9959c0F9c3849461B6468B4592a19567E09; + vm.label(safe, "Safe"); + vm.etch(safe, type(Safe).runtimeCode); + vm.store(safe, bytes32(uint256(3)), bytes32(uint256(owners.length))); + vm.store(safe, bytes32(uint256(4)), bytes32(uint256(threshold))); + + _daso.set(_daso.safe.selector, safe); + } + + // This etches the IO contracts into memory so that we can use them in tests. When using file IO + // we don't need to call this directly, as the `DeployAuthSystem.run(file, file)` entrypoint + // handles it. But when interacting with the script programmatically (e.g. in a Solidity test), + // this must be called. + function etchIOContracts() public returns (DeployAuthSystemInput dasi_, DeployAuthSystemOutput daso_) { + (dasi_, daso_) = getIOContracts(); + vm.etch(address(dasi_), type(DeployAuthSystemInput).runtimeCode); + vm.etch(address(daso_), type(DeployAuthSystemOutput).runtimeCode); + vm.allowCheatcodes(address(dasi_)); + vm.allowCheatcodes(address(daso_)); + } + + // This returns the addresses of the IO contracts for this script. + function getIOContracts() public view returns (DeployAuthSystemInput dasi_, DeployAuthSystemOutput daso_) { + dasi_ = DeployAuthSystemInput(DeployUtils.toIOAddress(msg.sender, "optimism.DeployAuthSystemInput")); + daso_ = DeployAuthSystemOutput(DeployUtils.toIOAddress(msg.sender, "optimism.DeployAuthSystemOutput")); + } +} diff --git a/packages/contracts-bedrock/test/DeployAuthSystem.t.sol b/packages/contracts-bedrock/test/DeployAuthSystem.t.sol index 60eb27ba1c3a..2e4a9876dd60 100644 --- a/packages/contracts-bedrock/test/DeployAuthSystem.t.sol +++ b/packages/contracts-bedrock/test/DeployAuthSystem.t.sol @@ -1,11 +1,11 @@ // SPDX-License-Identifier: MIT pragma solidity 0.8.15; -import { Test } from "forge-std/Test.sol"; +import { Test, stdStorage, StdStorage } from "forge-std/Test.sol"; import { stdToml } from "forge-std/StdToml.sol"; import { Solarray } from "scripts/libraries/Solarray.sol"; -import { DeployAuthSystemInput, DeployAuthSystemOutput } from "scripts/DeployAuthSystem.s.sol"; +import { DeployAuthSystemInput, DeployAuthSystem, DeployAuthSystemOutput } from "scripts/DeployAuthSystem.s.sol"; contract DeployAuthSystemInput_Test is Test { DeployAuthSystemInput dasi; @@ -116,3 +116,103 @@ contract DeployAuthSystemOutput_Test is Test { assertEq(expOutToml, actOutToml); } } + +contract DeployAuthSystem_Test is Test { + using stdStorage for StdStorage; + + DeployAuthSystem deployAuthSystem; + DeployAuthSystemInput dasi; + DeployAuthSystemOutput daso; + + // Define default input variables for testing. + uint256 defaultThreshold = 5; + uint256 defaultOwnersLength = 7; + address[] defaultOwners; + + function setUp() public { + deployAuthSystem = new DeployAuthSystem(); + (dasi, daso) = deployAuthSystem.etchIOContracts(); + for (uint256 i = 0; i < defaultOwnersLength; i++) { + defaultOwners.push(makeAddr(string.concat("owner", vm.toString(i)))); + } + } + + function hash(bytes32 _seed, uint256 _i) internal pure returns (bytes32) { + return keccak256(abi.encode(_seed, _i)); + } + + function testFuzz_run_memory_succeeds(bytes32 _seed) public { + // Generate random input values from the seed. This doesn't give us the benefit of the forge + // fuzzer's dictionary, but that's ok because we are just testing that values are set and + // passed correctly. + address[] memory _owners = Solarray.addresses( + address(uint160(uint256(hash(_seed, 0)))), + address(uint160(uint256(hash(_seed, 1)))), + address(uint160(uint256(hash(_seed, 2)))), + address(uint160(uint256(hash(_seed, 3)))), + address(uint160(uint256(hash(_seed, 4)))), + address(uint160(uint256(hash(_seed, 5)))), + address(uint160(uint256(hash(_seed, 6)))) + ); + + uint256 threshold = bound(uint256(_seed), 1, _owners.length - 1); + + dasi.set(dasi.owners.selector, _owners); + dasi.set(dasi.threshold.selector, threshold); + + // Run the deployment script. + deployAuthSystem.run(dasi, daso); + + // Assert inputs were properly passed through to the contract initializers. + assertNotEq(address(daso.safe()), address(0), "100"); + assertEq(daso.safe().getThreshold(), threshold, "200"); + // TODO: the getOwners() method requires iterating over the owners linked list. + // Since we're not yet performing a proper deployment of the Safe, this call will revert. + // assertEq(daso.safe().getOwners().length, _owners.length, "300"); + + // Architecture assertions. + // TODO: these will become relevant as we add more contracts to the auth system, and need to test their + // relationships. + + daso.checkOutput(); + } + + function test_run_io_succeeds() public { + string memory root = vm.projectRoot(); + string memory inpath = string.concat(root, "/test/fixtures/test-deploy-auth-system-in.toml"); + string memory outpath = string.concat(root, "/.testdata/test-deploy-auth-system-out.toml"); + + deployAuthSystem.run(inpath, outpath); + + string memory actOutToml = vm.readFile(outpath); + string memory expOutToml = vm.readFile(string.concat(root, "/test/fixtures/test-deploy-auth-system-out.toml")); + + // Clean up before asserting so that we don't leave any files behind. + vm.removeFile(outpath); + assertEq(expOutToml, actOutToml); + } + + function test_run_NullInput_reverts() public { + // Set default values for all inputs. + dasi.set(dasi.owners.selector, defaultOwners); + dasi.set(dasi.threshold.selector, defaultThreshold); + + // Zero out the owners length slot + uint256 slot = 9; + vm.store(address(dasi), bytes32(uint256(9)), bytes32(0)); + vm.expectRevert("DeployAuthSystemInput: owners not set"); + deployAuthSystem.run(dasi, daso); + vm.store(address(dasi), bytes32(uint256(9)), bytes32(defaultOwnersLength)); + + // Zero out the threshold slot + slot = zeroOutSlotForSelector(dasi.threshold.selector); + vm.expectRevert("DeployAuthSystemInput: threshold not set"); + deployAuthSystem.run(dasi, daso); + vm.store(address(dasi), bytes32(slot), bytes32(defaultThreshold)); + } + + function zeroOutSlotForSelector(bytes4 _selector) internal returns (uint256 slot_) { + slot_ = stdstore.enable_packed_slots().target(address(dasi)).sig(_selector).find(); + vm.store(address(dasi), bytes32(slot_), bytes32(0)); + } +} From 20f70038a1974d2a87c8cd150864fb33a974d06c Mon Sep 17 00:00:00 2001 From: Maurelian Date: Fri, 13 Sep 2024 13:30:27 -0400 Subject: [PATCH 2/3] feat: Remove redundant documentation --- .../scripts/DeployAuthSystem.s.sol | 23 ------------------- .../test/DeployAuthSystem.t.sol | 14 ----------- 2 files changed, 37 deletions(-) diff --git a/packages/contracts-bedrock/scripts/DeployAuthSystem.s.sol b/packages/contracts-bedrock/scripts/DeployAuthSystem.s.sol index cca7f054afcd..66f5da20b740 100644 --- a/packages/contracts-bedrock/scripts/DeployAuthSystem.s.sol +++ b/packages/contracts-bedrock/scripts/DeployAuthSystem.s.sol @@ -48,7 +48,6 @@ contract DeployAuthSystemInput is CommonBase { } function owners() public view returns (address[] memory) { - // expecting to trigger this require(_owners.length != 0, "DeployAuthSystemInput: owners not set"); return _owners; } @@ -78,31 +77,17 @@ contract DeployAuthSystemOutput is CommonBase { } } -// For all broadcasts in this script we explicitly specify the deployer as `msg.sender` because for -// testing we deploy this script from a test contract. If we provide no argument, the foundry -// default sender would be the broadcaster during test, but the broadcaster needs to be the deployer -// since they are set to the initial proxy admin owner. contract DeployAuthSystem is Script { - // -------- Core Deployment Methods -------- - - // This entrypoint is for end-users to deploy from an input file and write to an output file. - // In this usage, we don't need the input and output contract functionality, so we deploy them - // here and abstract that architectural detail away from the end user. function run(string memory _infile, string memory _outfile) public { - // End-user without file IO, so etch the IO helper contracts. (DeployAuthSystemInput dasi, DeployAuthSystemOutput daso) = etchIOContracts(); - // Load the input file into the input contract. dasi.loadInputFile(_infile); - // Run the deployment script and write outputs to the DeployAuthSystemOutput contract. run(dasi, daso); - // Write the output data to a file. daso.writeOutputFile(_outfile); } - // This entrypoint is useful for testing purposes, as it doesn't use any file I/O. function run(DeployAuthSystemInput _dasi, DeployAuthSystemOutput _daso) public { deploySafe(_dasi, _daso); } @@ -110,9 +95,6 @@ contract DeployAuthSystem is Script { function deploySafe(DeployAuthSystemInput _dasi, DeployAuthSystemOutput _daso) public { address[] memory owners = _dasi.owners(); uint256 threshold = _dasi.threshold(); - // Silence unused variable warnings - owners; - threshold; // TODO: replace with a real deployment. The safe deployment logic is fairly complex, so for the purposes of // this scaffolding PR we'll just etch the code. @@ -126,10 +108,6 @@ contract DeployAuthSystem is Script { _daso.set(_daso.safe.selector, safe); } - // This etches the IO contracts into memory so that we can use them in tests. When using file IO - // we don't need to call this directly, as the `DeployAuthSystem.run(file, file)` entrypoint - // handles it. But when interacting with the script programmatically (e.g. in a Solidity test), - // this must be called. function etchIOContracts() public returns (DeployAuthSystemInput dasi_, DeployAuthSystemOutput daso_) { (dasi_, daso_) = getIOContracts(); vm.etch(address(dasi_), type(DeployAuthSystemInput).runtimeCode); @@ -138,7 +116,6 @@ contract DeployAuthSystem is Script { vm.allowCheatcodes(address(daso_)); } - // This returns the addresses of the IO contracts for this script. function getIOContracts() public view returns (DeployAuthSystemInput dasi_, DeployAuthSystemOutput daso_) { dasi_ = DeployAuthSystemInput(DeployUtils.toIOAddress(msg.sender, "optimism.DeployAuthSystemInput")); daso_ = DeployAuthSystemOutput(DeployUtils.toIOAddress(msg.sender, "optimism.DeployAuthSystemOutput")); diff --git a/packages/contracts-bedrock/test/DeployAuthSystem.t.sol b/packages/contracts-bedrock/test/DeployAuthSystem.t.sol index 2e4a9876dd60..f06e9237cb86 100644 --- a/packages/contracts-bedrock/test/DeployAuthSystem.t.sol +++ b/packages/contracts-bedrock/test/DeployAuthSystem.t.sol @@ -68,13 +68,10 @@ contract DeployAuthSystemOutput_Test is Test { function test_set_succeeds() public { address safeAddr = makeAddr("safe"); - // Ensure the address has code, since it's expected to be a contract vm.etch(safeAddr, hex"01"); - // Set the output data daso.set(daso.safe.selector, safeAddr); - // Compare the test data to the getter method assertEq(safeAddr, address(daso.safe()), "100"); } @@ -95,13 +92,11 @@ contract DeployAuthSystemOutput_Test is Test { function test_writeOutputFile_succeeds() public { string memory root = vm.projectRoot(); - // Use the expected data from the test fixture. string memory expOutPath = string.concat(root, "/test/fixtures/test-deploy-auth-system-out.toml"); string memory expOutToml = vm.readFile(expOutPath); address expSafe = expOutToml.readAddress(".safe"); - // Etch code at each address so the code checks pass when settings values. vm.etch(expSafe, hex"01"); daso.set(daso.safe.selector, expSafe); @@ -110,7 +105,6 @@ contract DeployAuthSystemOutput_Test is Test { daso.writeOutputFile(actOutPath); string memory actOutToml = vm.readFile(actOutPath); - // Clean up before asserting so that we don't leave any files behind. vm.removeFile(actOutPath); assertEq(expOutToml, actOutToml); @@ -142,9 +136,6 @@ contract DeployAuthSystem_Test is Test { } function testFuzz_run_memory_succeeds(bytes32 _seed) public { - // Generate random input values from the seed. This doesn't give us the benefit of the forge - // fuzzer's dictionary, but that's ok because we are just testing that values are set and - // passed correctly. address[] memory _owners = Solarray.addresses( address(uint160(uint256(hash(_seed, 0)))), address(uint160(uint256(hash(_seed, 1)))), @@ -160,10 +151,8 @@ contract DeployAuthSystem_Test is Test { dasi.set(dasi.owners.selector, _owners); dasi.set(dasi.threshold.selector, threshold); - // Run the deployment script. deployAuthSystem.run(dasi, daso); - // Assert inputs were properly passed through to the contract initializers. assertNotEq(address(daso.safe()), address(0), "100"); assertEq(daso.safe().getThreshold(), threshold, "200"); // TODO: the getOwners() method requires iterating over the owners linked list. @@ -187,13 +176,11 @@ contract DeployAuthSystem_Test is Test { string memory actOutToml = vm.readFile(outpath); string memory expOutToml = vm.readFile(string.concat(root, "/test/fixtures/test-deploy-auth-system-out.toml")); - // Clean up before asserting so that we don't leave any files behind. vm.removeFile(outpath); assertEq(expOutToml, actOutToml); } function test_run_NullInput_reverts() public { - // Set default values for all inputs. dasi.set(dasi.owners.selector, defaultOwners); dasi.set(dasi.threshold.selector, defaultThreshold); @@ -204,7 +191,6 @@ contract DeployAuthSystem_Test is Test { deployAuthSystem.run(dasi, daso); vm.store(address(dasi), bytes32(uint256(9)), bytes32(defaultOwnersLength)); - // Zero out the threshold slot slot = zeroOutSlotForSelector(dasi.threshold.selector); vm.expectRevert("DeployAuthSystemInput: threshold not set"); deployAuthSystem.run(dasi, daso); From 7a309ff88fe8acbfbafc86ce57ad9867f31c5e3f Mon Sep 17 00:00:00 2001 From: Maurelian Date: Sat, 14 Sep 2024 08:34:37 -0400 Subject: [PATCH 3/3] Update DeployAuthSystem.s.sol --- packages/contracts-bedrock/scripts/DeployAuthSystem.s.sol | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/packages/contracts-bedrock/scripts/DeployAuthSystem.s.sol b/packages/contracts-bedrock/scripts/DeployAuthSystem.s.sol index 66f5da20b740..ac2f51ee4be1 100644 --- a/packages/contracts-bedrock/scripts/DeployAuthSystem.s.sol +++ b/packages/contracts-bedrock/scripts/DeployAuthSystem.s.sol @@ -98,9 +98,7 @@ contract DeployAuthSystem is Script { // TODO: replace with a real deployment. The safe deployment logic is fairly complex, so for the purposes of // this scaffolding PR we'll just etch the code. - // makeAddr("safe") = 0xDC93f9959c0F9c3849461B6468B4592a19567E09 - address safe = 0xDC93f9959c0F9c3849461B6468B4592a19567E09; - vm.label(safe, "Safe"); + address safe = makeAddr("safe"); vm.etch(safe, type(Safe).runtimeCode); vm.store(safe, bytes32(uint256(3)), bytes32(uint256(owners.length))); vm.store(safe, bytes32(uint256(4)), bytes32(uint256(threshold)));