Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor: Remove file I/O from deployment scripts #12015

Merged
merged 3 commits into from
Sep 20, 2024
Merged

refactor: Remove file I/O from deployment scripts #12015

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
44a89ec
fix: Removing all file I/O features
blmalone Sep 19, 2024
b322460
fix: comments in code.
blmalone Sep 19, 2024
23fcc6c
fix: pr comment
blmalone Sep 20, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 0 additions & 22 deletions packages/contracts-bedrock/scripts/DeployAuthSystem.s.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,13 +35,6 @@ contract DeployAuthSystemInput is CommonBase {
}
}

function loadInputFile(string memory _infile) public {
string memory toml = vm.readFile(_infile);

set(this.threshold.selector, toml.readUint(".safe.threshold"));
set(this.owners.selector, toml.readAddressArray(".safe.owners"));
}

function threshold() public view returns (uint256) {
require(_threshold != 0, "DeployAuthSystemInput: threshold not set");
return _threshold;
Expand All @@ -61,11 +54,6 @@ contract DeployAuthSystemOutput is CommonBase {
else revert("DeployAuthSystemOutput: unknown selector");
}

function writeOutputFile(string memory _outfile) public {
string memory out = vm.serializeAddress("outfile", "safe", address(this.safe()));
vm.writeToml(out, _outfile);
}

function checkOutput() public view {
address[] memory addrs = Solarray.addresses(address(this.safe()));
DeployUtils.assertValidContractAddresses(addrs);
Expand All @@ -78,16 +66,6 @@ contract DeployAuthSystemOutput is CommonBase {
}

contract DeployAuthSystem is Script {
function run(string memory _infile, string memory _outfile) public {
(DeployAuthSystemInput dasi, DeployAuthSystemOutput daso) = etchIOContracts();

dasi.loadInputFile(_infile);

run(dasi, daso);

daso.writeOutputFile(_outfile);
}

function run(DeployAuthSystemInput _dasi, DeployAuthSystemOutput _daso) public {
deploySafe(_dasi, _daso);
}
Expand Down
19 changes: 0 additions & 19 deletions packages/contracts-bedrock/scripts/DeployImplementations.s.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,11 +97,6 @@ contract DeployImplementationsInput is BaseDeployIO {
else revert("DeployImplementationsInput: unknown selector");
}

function loadInputFile(string memory _infile) public pure {
_infile;
require(false, "DeployImplementationsInput: not implemented");
}

function salt() public view returns (bytes32) {
// TODO check if implementations are deployed based on code+salt and skip deploy if so.
return _salt;
Expand Down Expand Up @@ -192,11 +187,6 @@ contract DeployImplementationsOutput is BaseDeployIO {
// forgefmt: disable-end
}

function writeOutputFile(string memory _outfile) public pure {
_outfile;
require(false, "DeployImplementationsOutput: not implemented");
}

function checkOutput(DeployImplementationsInput _dii) public {
address[] memory addrs = Solarray.addresses(
address(this.opsmProxy()),
Expand Down Expand Up @@ -441,15 +431,6 @@ contract DeployImplementationsOutput is BaseDeployIO {
contract DeployImplementations is Script {
// -------- Core Deployment Methods --------

function run(string memory _infile) public {
(DeployImplementationsInput dii, DeployImplementationsOutput dio) = etchIOContracts();
dii.loadInputFile(_infile);
run(dii, dio);
string memory outfile = ""; // This will be derived from input file name, e.g. `foo.in.toml` -> `foo.out.toml`
dio.writeOutputFile(outfile);
require(false, "DeployImplementations: run is not implemented");
}

function run(DeployImplementationsInput _dii, DeployImplementationsOutput _dio) public {
// Deploy the implementations.
deploySystemConfigImpl(_dii, _dio);
Expand Down
18 changes: 0 additions & 18 deletions packages/contracts-bedrock/scripts/DeployOPChain.s.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,11 +70,6 @@ contract DeployOPChainInput is BaseDeployIO {
}
}

function loadInputFile(string memory _infile) public pure {
_infile;
require(false, "DeployOPChainInput: not implemented");
}

function opChainProxyAdminOwner() public view returns (address) {
require(_opChainProxyAdminOwner != address(0), "DeployOPChainInput: not set");
return _opChainProxyAdminOwner;
Expand Down Expand Up @@ -167,11 +162,6 @@ contract DeployOPChainOutput is BaseDeployIO {
// forgefmt: disable-end
}

function writeOutputFile(string memory _outfile) public pure {
_outfile;
require(false, "DeployOPChainOutput: not implemented");
}

function checkOutput(DeployOPChainInput _doi) public view {
// With 16 addresses, we'd get a stack too deep error if we tried to do this inline as a
// single call to `Solarray.addresses`. So we split it into two calls.
Expand Down Expand Up @@ -406,14 +396,6 @@ contract DeployOPChainOutput is BaseDeployIO {

contract DeployOPChain is Script {
// -------- Core Deployment Methods --------
function run(string memory _infile) public {
(DeployOPChainInput doi, DeployOPChainOutput doo) = etchIOContracts();
doi.loadInputFile(_infile);
run(doi, doo);
string memory outfile = ""; // This will be derived from input file name, e.g. `foo.in.toml` -> `foo.out.toml`
doo.writeOutputFile(outfile);
require(false, "DeployOPChain: run is not implemented");
}

function run(DeployOPChainInput _doi, DeployOPChainOutput _doo) public {
OPStackManager opsmProxy = _doi.opsmProxy();
Expand Down
72 changes: 9 additions & 63 deletions packages/contracts-bedrock/scripts/DeploySuperchain.s.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,14 +19,14 @@ import { BaseDeployIO } from "scripts/utils/BaseDeployIO.sol";
// deployment script.
//
// There are three categories of users that are expected to interact with the scripts:
// 1. End users that want to run live contract deployments.
blmalone marked this conversation as resolved.
Show resolved Hide resolved
// 1. End users that want to run live contract deployments. These users are expected to run these scripts via
// 'op-deployer' which uses a go interface to interact with the scripts.
// 2. Solidity developers that want to use or test these scripts in a standard forge test environment.
// 3. Go developers that want to run the deploy scripts as part of e2e testing with other aspects of the OP Stack.
//
// We want each user to interact with the scripts in the way that's simplest for their use case:
// 1. End users: TOML input files that define config, and TOML output files with all output data.
// 2. Solidity developers: Direct calls to the script, with the input and output contracts configured.
// 3. Go developers: The forge scripts can be executed directly in Go.
// 1. Solidity developers: Direct calls to the script, with the input and output contracts configured.
// 2. Go developers: The forge scripts can be executed directly in Go.
//
// The following architecture is used to meet the requirements of each user. We use this file's
// `DeploySuperchain` script as an example, but it applies to other scripts as well.
Expand All @@ -39,7 +39,7 @@ import { BaseDeployIO } from "scripts/utils/BaseDeployIO.sol";
//
// Because the core script performs calls to the input and output contracts, Go developers can
// intercept calls to these addresses (analogous to how forge intercepts calls to the `Vm` address
// to execute cheatcodes), to avoid the need for file I/O or hardcoding the input/output values.
// to execute cheatcodes), to avoid the need for hardcoding the input/output values.
//
// Public getter methods on the input and output contracts allow individual fields to be accessed
// in a strong, type-safe manner (as opposed to a single struct getter where the caller may
Expand Down Expand Up @@ -110,27 +110,6 @@ contract DeploySuperchainInput is BaseDeployIO {
else revert("DeploySuperchainInput: unknown selector");
}

// Load the input from a TOML file.
// When setting inputs from a TOML file, we use the setter methods instead of writing directly
// to storage. This allows us to validate each input as it is set.
function loadInputFile(string memory _infile) public {
string memory toml = vm.readFile(_infile);

// Parse and set role inputs.
set(this.guardian.selector, toml.readAddress(".roles.guardian"));
set(this.protocolVersionsOwner.selector, toml.readAddress(".roles.protocolVersionsOwner"));
set(this.proxyAdminOwner.selector, toml.readAddress(".roles.proxyAdminOwner"));

// Parse and set other inputs.
set(this.paused.selector, toml.readBool(".paused"));

uint256 recVersion = toml.readUint(".recommendedProtocolVersion");
set(this.recommendedProtocolVersion.selector, ProtocolVersion.wrap(recVersion));

uint256 reqVersion = toml.readUint(".requiredProtocolVersion");
set(this.requiredProtocolVersion.selector, ProtocolVersion.wrap(reqVersion));
}

// Each input field is exposed via it's own getter method. Using public storage variables here
// would be less verbose, but would also be more error-prone, as it would require the caller to
// validate that each input is set before accessing it. With getter methods, we can automatically
Expand Down Expand Up @@ -195,21 +174,8 @@ contract DeploySuperchainOutput is BaseDeployIO {
else revert("DeploySuperchainOutput: unknown selector");
}

// Save the output to a TOML file.
// We fetch the output values using external calls to the getters to verify that all outputs are
// set correctly before writing them to the file.
function writeOutputFile(string memory _outfile) public {
string memory key = "dso-outfile";
vm.serializeAddress(key, "superchainProxyAdmin", address(this.superchainProxyAdmin()));
vm.serializeAddress(key, "superchainConfigImpl", address(this.superchainConfigImpl()));
vm.serializeAddress(key, "superchainConfigProxy", address(this.superchainConfigProxy()));
vm.serializeAddress(key, "protocolVersionsImpl", address(this.protocolVersionsImpl()));
string memory out = vm.serializeAddress(key, "protocolVersionsProxy", address(this.protocolVersionsProxy()));
vm.writeToml(out, _outfile);
}

// This function can be called to ensure all outputs are correct. Similar to `writeOutputFile`,
// it fetches the output values using external calls to the getter methods for safety.
// This function can be called to ensure all outputs are correct.
// It fetches the output values using external calls to the getter methods for safety.
function checkOutput(DeploySuperchainInput _dsi) public {
address[] memory addrs = Solarray.addresses(
address(this.superchainProxyAdmin()),
Expand Down Expand Up @@ -322,24 +288,6 @@ contract DeploySuperchainOutput is BaseDeployIO {
contract DeploySuperchain 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.
(DeploySuperchainInput dsi, DeploySuperchainOutput dso) = etchIOContracts();

// Load the input file into the input contract.
dsi.loadInputFile(_infile);

// Run the deployment script and write outputs to the DeploySuperchainOutput contract.
run(dsi, dso);

// Write the output data to a file.
dso.writeOutputFile(_outfile);
}

// This entrypoint is useful for testing purposes, as it doesn't use any file I/O.
function run(DeploySuperchainInput _dsi, DeploySuperchainOutput _dso) public {
// Notice that we do not do any explicit verification here that inputs are set. This is because
// the verification happens elsewhere:
Expand Down Expand Up @@ -449,10 +397,8 @@ contract DeploySuperchain is Script {

// -------- Utilities --------

// 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 `DeploySuperchain.run(file, file)` entrypoint
// handles it. But when interacting with the script programmatically (e.g. in a Solidity test),
// this must be called.
// This etches the IO contracts into memory so that we can use them in tests.
// When interacting with the script programmatically (e.g. in a Solidity test), this must be called.
function etchIOContracts() public returns (DeploySuperchainInput dsi_, DeploySuperchainOutput dso_) {
(dsi_, dso_) = getIOContracts();
vm.etch(address(dsi_), type(DeploySuperchainInput).runtimeCode);
Expand Down
45 changes: 0 additions & 45 deletions packages/contracts-bedrock/test/DeployAuthSystem.t.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,16 +30,6 @@ contract DeployAuthSystemInput_Test is Test {
}
}

function test_loadInputFile_succeeds() public {
string memory root = vm.projectRoot();
string memory path = string.concat(root, "/test/fixtures/test-deploy-auth-system-in.toml");

dasi.loadInputFile(path);

assertEq(threshold, dasi.threshold(), "100");
assertEq(owners.length, dasi.owners().length, "200");
}

function test_getters_whenNotSet_revert() public {
vm.expectRevert("DeployAuthSystemInput: threshold not set");
dasi.threshold();
Expand Down Expand Up @@ -88,27 +78,6 @@ contract DeployAuthSystemOutput_Test is Test {
vm.expectRevert(expectedErr);
daso.safe();
}

function test_writeOutputFile_succeeds() public {
string memory root = vm.projectRoot();

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");

vm.etch(expSafe, hex"01");

daso.set(daso.safe.selector, expSafe);

string memory actOutPath = string.concat(root, "/.testdata/test-deploy-auth-system-output.toml");
daso.writeOutputFile(actOutPath);
string memory actOutToml = vm.readFile(actOutPath);

vm.removeFile(actOutPath);

assertEq(expOutToml, actOutToml);
}
}

contract DeployAuthSystem_Test is Test {
Expand Down Expand Up @@ -166,20 +135,6 @@ contract DeployAuthSystem_Test is Test {
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"));

vm.removeFile(outpath);
assertEq(expOutToml, actOutToml);
}

function test_run_NullInput_reverts() public {
dasi.set(dasi.owners.selector, defaultOwners);
dasi.set(dasi.threshold.selector, defaultThreshold);
Expand Down
13 changes: 0 additions & 13 deletions packages/contracts-bedrock/test/DeployImplementations.t.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,19 +43,6 @@ contract DeployImplementationsInput_Test is Test {
dii = new DeployImplementationsInput();
}

function test_loadInputFile_succeeds() public {
// See `test_loadInputFile_succeeds` in `DeploySuperchain.t.sol` for a reference implementation.
// This test is currently skipped because loadInputFile is not implemented.
vm.skip(true);

// Compare the test inputs to the getter methods.
// assertEq(withdrawalDelaySeconds, dii.withdrawalDelaySeconds(), "100");
// assertEq(minProposalSizeBytes, dii.minProposalSizeBytes(), "200");
// assertEq(challengePeriodSeconds, dii.challengePeriodSeconds(), "300");
// assertEq(proofMaturityDelaySeconds, dii.proofMaturityDelaySeconds(), "400");
// assertEq(disputeGameFinalityDelaySeconds, dii.disputeGameFinalityDelaySeconds(), "500");
}

function test_getters_whenNotSet_revert() public {
vm.expectRevert("DeployImplementationsInput: not set");
dii.withdrawalDelaySeconds();
Expand Down
Loading