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 resource config from standard config spec #200

Merged
merged 1 commit into from
May 20, 2024

Conversation

blmalone
Copy link
Contributor

@blmalone blmalone commented May 20, 2024

Should target post-L2Beat-Stage 1.

Resource config is now going to be hardcoded and updatable by L1PAO.

remove the setResourceConfig setter to prevent the System Config Owner, from misconfiguring the resource config and preventing force inclusions from L1 (e.g. by setting the maximum deposit gas too low). The resource config can now only be changed by upgrading the SystemConfig contract which is controlled by the L1 Proxy Admin (a 2-of-2 multisig with the Optimism Foundation and Security Council).

Copy link
Contributor

@tynes tynes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we have the default values elsewhere in the specs and eventually I want to remove the resource config completely via deposit queue

@tynes
Copy link
Contributor

tynes commented May 20, 2024

Looks like this needs a rebase

@blmalone blmalone force-pushed the bm/remove-resource-config branch from 664b080 to d1bcd5e Compare May 20, 2024 18:22
@blmalone blmalone merged commit 362db29 into main May 20, 2024
1 check passed
@blmalone blmalone deleted the bm/remove-resource-config branch May 20, 2024 18:24
blmalone added a commit that referenced this pull request Jun 18, 2024
blmalone added a commit that referenced this pull request Jun 21, 2024
* Revert "fix: removed resource config from standard config spec. (#200)"

This reverts commit 362db29.

* fix: removing resource config from the administers for system config owner.
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.

3 participants