-
Notifications
You must be signed in to change notification settings - Fork 347
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
feat: add initial l1-contracts-foundry #267
Conversation
v1.8.0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Deniallugo I think we can discuss it later (since it is a rather small change to the PR and it is already somewhat big), but overall we should set the governance to the address rightaway (or at least as soon as possible). For localhost scripts it does not matter, but for the mainnet deployments we should have as little risk of human error as possible (i.e. the error when someone forgets to look onto the etherscan to double check that the governance was set correctly).
console.log("StateTransitionManager set in ValidatorTimelock"); | ||
} | ||
|
||
function deployDiamondProxy() internal { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I undertand correctly this one is only needed for tests and does not need to be always called
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe the approach we took with @Deniallugo for this script was to assume nothing was deployed previously. Therefore we're deploying it here. If necessary an option could be added to the config to pass the address manually. If it's present, then we don't deploy the DiamondProxy, otherwise we deploy it. Do you think this might work?
What ❔
Why ❔
Checklist
Notes
There are some points of discussion I wanted to address before finalizing this PR.
Preprocessing support
#273 removes the need for preprocessing.