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

Remove duplication of deployment steps in executeFork, make a function that can do the genesis deploy from the ForkingManager #265

Draft
wants to merge 16 commits into
base: main
Choose a base branch
from

Conversation

edmundedgar
Copy link
Contributor

@edmundedgar edmundedgar commented Mar 20, 2024

We'd ended up with a lot of duplication of all the initialize steps passing a lot of similar arguments into various contracts:

  • Twice in executeFork
  • In lots of tests
  • Again in the deployment scripts, which we also don't have tests for (although they're thorough about assert checks)

This PR channels all initialization through a single function, and creates a spawnInstance function that can be called against the ForkingManager implementation to do the initial deployment of all the other forkable parts.

Next step (apart from tidying up) would be to replace parts of the JavaScript deployment scripts with a call to spawnInstance.

…ation from executeFork, replace with AddressPair with a struct per fork.
…n the original genesis deploy. (Haven't changed the deployment script yet).
@edmundedgar edmundedgar requested a review from josojo March 20, 2024 22:21
@josojo
Copy link
Collaborator

josojo commented Mar 21, 2024

yeah I think generally its nicer to do it that way.

Maybe the gas price for such big transaction are a little bit higher, but i think its fine.
In the original deployment scripts, they did some deployments in a particular manner such that the bytecode of both bridge contracts (one in the L1, and L2) are the same. I never fully understood the reaosn for it. I think wiht your method, it might be that they are different. Not sure how much a deal that really is. Just a small heads-up of a potentially an issue.

@edmundedgar
Copy link
Contributor Author

Great, I'll have a go at hacking this into the deploy and see if it works in practice.

I think the gas cost should be OK because it's only adding the proxies and initializing them. The part where you deploy the implementation contracts is still done from JavaScript and it's simple so I should be able to get the ongoing_deployments stuff working properly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants