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

feat(superchain): add GovernedByOptimism field to chain config #718

Merged
merged 29 commits into from
Dec 10, 2024

Conversation

edobry
Copy link
Contributor

@edobry edobry commented Nov 27, 2024

Description

This PR adds a boolean GovernedByOptimism field to the chain config, which indicates if a certain chain is subject to Optimism governance. It also adds a GovernedByOptimismTest test to the validation package, which ensures that any chain which has the governed_by_optimism field set to true in its config also has its ProxyAdmin address set to the Optimism multisig.

It also sets the new governed_by_optimism field on all chain configs as appropriate

Metadata

@edobry edobry requested a review from a team as a code owner November 27, 2024 21:23
@edobry edobry changed the title feat: add GovernedByOptimism field to chain config feat(superchain: add GovernedByOptimism field to chain config Nov 27, 2024
@edobry edobry changed the title feat(superchain: add GovernedByOptimism field to chain config feat(superchain): add GovernedByOptimism field to chain config Nov 27, 2024
@edobry edobry force-pushed the edobry/governed-by-optimism branch from 1942d41 to b1e4932 Compare November 27, 2024 21:27
@edobry edobry changed the base branch from edobry/refactor-flags to main November 27, 2024 21:28
@geoknee
Copy link
Collaborator

geoknee commented Nov 28, 2024

Two main questions:

  1. If we declare a chain as governed by optimism, but it actually isn't, how would we tell?

I am wondering if this change be accompanied by some edits to this validation check:

func testKeyHandover(t *testing.T, chain *ChainConfig) {
chainID := chain.ChainID
superchain := OPChains[chainID].Superchain
rpcEndpoint := Superchains[superchain].Config.L1.PublicRPC
require.NotEmpty(t, rpcEndpoint, "no rpc specified")
client, err := ethclient.Dial(rpcEndpoint)
require.NoErrorf(t, err, "could not dial rpc endpoint %s", rpcEndpoint)
// L1 Proxy Admin
checkResolutions(t, standard.Config.MultisigRoles[superchain].KeyHandover.L1.Universal, chainID, client)
rpcEndpoint = OPChains[chainID].PublicRPC
require.NotEmpty(t, rpcEndpoint, "no rpc specified")
client, err = ethclient.Dial(rpcEndpoint)
require.NoErrorf(t, err, "could not dial rpc endpoint %s", rpcEndpoint)
// L2 Proxy Admin
checkResolutions(t, standard.Config.MultisigRoles[superchain].KeyHandover.L2.Universal, chainID, client)
}

EDIT: just spotted the other PR #719. I think it would make sense to commit the changes in a single PR, if possible.

  1. When does this boolean get written / updated? During the add-chain tool? Manually? During promotion to a standard chain? Do we need to update the .env schema / add-chain tool at all?

@edobry
Copy link
Contributor Author

edobry commented Dec 2, 2024

we already have a "key handover test" and I would like to understand how this new test relates to that one.

My understanding is that the existing "key handover test" is intended only for standard chains, while the Governed by Optimism field is a bit more loose:

(Note that superchainLevel requires that chains both have their ProxyAdminOwner role set to the 2/2 and that the chain uses permissionless FPs, e.g., that the chain is on the latest gov-approved version. "Governed by Optimism" should refer only to the key handover status.)

source: #676

More context here

It may be possible that the correct path of action could be to reuse the key handover test for this purpose and instead add logic to also run it for chains that have governed_by_optimism: true set, I'm not really clear on this. Would welcome some guidance here @geoknee @tessr

@edobry edobry force-pushed the edobry/governed-by-optimism branch 2 times, most recently from aedca82 to 3568b11 Compare December 3, 2024 16:06
@bitwiseguy
Copy link
Collaborator

bitwiseguy commented Dec 4, 2024

Looks like you'll need to run just codegen locally to propagate any changes from chain config *.toml files to the summary files. Then when you push the edits to the summary files, the codegen ci check should pass

@tessr
Copy link
Contributor

tessr commented Dec 5, 2024

Just chiming in to confirm that this can be the same logic as the key handover test. We are functionally checking the same thing (whether the proxy admin owner is set to the correct address).

@edobry edobry force-pushed the edobry/governed-by-optimism branch 2 times, most recently from 6fad3ba to 9538f19 Compare December 6, 2024 19:57
@edobry
Copy link
Contributor Author

edobry commented Dec 6, 2024

I've updated this PR with several changes:

1. restructured the validation/standard/*.toml files to improve DevX
2. added the negative test case suggested here
3. added standard configs for the sepolia-dev-0 chain to account for newly-failing tests caused by 2.

I explored whether we can reuse the Key_Handover test for this usecase but this doesn't seem possible, as it doesn't check the configured address directly but rather calls a contract method; I tested this by making changes in the config file which should've caused it to fail, but it didn't, not fully sure why. Please LMK if I've misunderstood something here.

cc @bitwiseguy

@edobry edobry requested a review from bitwiseguy December 6, 2024 20:06
@edobry edobry force-pushed the edobry/governed-by-optimism branch from 76cf719 to f478121 Compare December 9, 2024 17:30
@edobry edobry force-pushed the edobry/governed-by-optimism branch 2 times, most recently from fa008b6 to db26141 Compare December 10, 2024 18:15
@edobry edobry force-pushed the edobry/governed-by-optimism branch from 46862bb to ad7e77b Compare December 10, 2024 22:13
Copy link
Collaborator

@bitwiseguy bitwiseguy left a comment

Choose a reason for hiding this comment

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

LGTM

@edobry edobry enabled auto-merge December 10, 2024 23:22
@edobry edobry added this pull request to the merge queue Dec 10, 2024
Merged via the queue into main with commit ad6f08a Dec 10, 2024
14 checks passed
@edobry edobry deleted the edobry/governed-by-optimism branch December 10, 2024 23:36
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.

4 participants