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.

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

Modify Test Setup to use op-deployer rather than Deploy.s.sol (OPCM - Upgrade 13) #13331

Open
Tracked by #13069
maurelian opened this issue Dec 10, 2024 · 4 comments
Open
Tracked by #13069
Labels
A-op-deployer A-pkg-contracts-bedrock Area: packages/contracts-bedrock T-evm-safety Team: EVM Safety T-upgrades

Comments

@maurelian
Copy link
Contributor

maurelian commented Dec 10, 2024

In order to standardize on the use of op-deployer for deployment, we should enshrine in our test setup as the method for deploying contracts.

The invocation of op-deployer could be done in one of two ways:

  1. via the just test command.
  2. via the FFI calls in Deploy.s.sol.

My instinct is that the first will be cleaner.

Either way, the deployed contracts will then need to be saved by reading from the resulting state.json file created by op-deployer, and writing to the deploymentOutFile.

If the first option is chosen, then it should be possible to generate the deploymentOutfile using jq directly.

Otherwise that work will need to be done using forge-std's json parsing and fs utilities. An example of the latter approach (solidity driven), can be seen in #13323.

@tynes
Copy link
Contributor

tynes commented Dec 10, 2024

I am generally not in favor of reading in the state from a json file for the solidity unit tests. This is going to cause a massive devex headache, needing to regenerate the state dump before every little test. This is going to surprise solidity developers as its a very non standard flow. The abstractions should be set up so that whatever solidity file op-deployer calls into to do a deployment can be called in memory during a foundry invocation.

@maurelian
Copy link
Contributor Author

I tend to agree that resorting to FFI to call a go util is poor devex. Something that I'm starting to land on that might be a good middle ground:

My thinking is that op-deployer appears to have standardized on this WithScript() method that takes a DeployFoo solidity script. Those scripts have to satisfy the same constraints of the existing scripts (ie. DeploySuperchain.s.sol, DeployImplementations.s.sol etc.) scripts.

So what we can do is implement similar logic in solidity, which handles those scripts in a similar way by:

  1. etching the I/O contracts (DeployFooInput /DeployFooOutput )
  2. reading the intent file to get and pass the intput to DeployFooInput
  3. running the DeployFoo script
  4. reading the output from DeployFooOutput
  5. labelling the resulting addresses

This keeps the deploy/upgrade dev fully in solidity, while making it safe and easy to add new bootstrap commands.

@protolambda protolambda added A-op-deployer A-pkg-contracts-bedrock Area: packages/contracts-bedrock labels Dec 16, 2024
@tynes
Copy link
Contributor

tynes commented Dec 17, 2024

question: can we kill hundreds of lines of code by deleting the whole set interface and just use foundry vm.store + deterministic unstructured storage? seems overly complicated to need to write all this boilerplate in solidity. you can just write directly to StateDB in go and use vm.store in solidity if necessary

It could be way less work than it is to implement similar logic in solidity

@mslipper
Copy link
Collaborator

I don't think the Solidity should call out to OP Deployer at all. Instead, we should compose the various scripts that OP Deployer uses under the hood to create networks for the purpose of testing.

@BlocksOnAChain BlocksOnAChain changed the title Modify Test Setup to use op-deployer rather than Deploy.s.sol Modify Test Setup to use op-deployer rather than Deploy.s.sol (Upgrade 12) Jan 10, 2025
@maurelian maurelian changed the title Modify Test Setup to use op-deployer rather than Deploy.s.sol (Upgrade 12) Modify Test Setup to use op-deployer rather than Deploy.s.sol (Upgrade 13) Jan 30, 2025
@maurelian maurelian added T-upgrades T-evm-safety Team: EVM Safety labels Feb 4, 2025
@maurelian maurelian changed the title Modify Test Setup to use op-deployer rather than Deploy.s.sol (Upgrade 13) Modify Test Setup to use op-deployer rather than Deploy.s.sol (OPCM - Upgrade 13) Feb 5, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-op-deployer A-pkg-contracts-bedrock Area: packages/contracts-bedrock T-evm-safety Team: EVM Safety T-upgrades
Projects
Status: Todo
Development

No branches or pull requests

4 participants