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

Add SEAL chains for exercise #225

Merged
merged 16 commits into from
May 14, 2024
Merged

Conversation

ipatka
Copy link
Contributor

@ipatka ipatka commented May 8, 2024

Description

This PR contains the contract addresses and superchain configs for the SEAL drill

The files were not all generated using the standard scripts as the deployment was done outside of the normal flow in the OP monorepo. Therefore there may be some missing information which should normally go in the bytecodes and genesis folders.

Some implementation addresses and admin addresses like Proxy Admin Owner, Guardian, Challenger may change between now and the drill to align them with other Sepolia OP deployments

@ipatka ipatka requested review from a team, sbvegan, ZakAyesh and OPMattie as code owners May 8, 2024 21:10
@ipatka ipatka marked this pull request as draft May 8, 2024 21:10
@zchn zchn marked this pull request as ready for review May 9, 2024 18:16
@Ethnical
Copy link
Collaborator

Ethnical commented May 10, 2024

FYI,
The contributing guide (https://github.com/ethereum-optimism/superchain-registry/blob/main/CONTRIBUTING.md) is not followed properly during this PR. Thus, this is leading to CI issues. The team asked to make a new PR by following the contributing guide (as we discuss on telegram).
We need to have the CI tests succeeding to ensure a chain can be added with the correct standard chain level.

@rholterhus rholterhus requested a review from a team as a code owner May 10, 2024 15:21
Copy link
Collaborator

@Ethnical Ethnical left a comment

Choose a reason for hiding this comment

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

A small changes has to be made there is the name of 2 chains (op-seal & base-seal) to ensure the expectation and alignement of the exercise are met and no confusion is made here.
We can rename there as seal1 and seal2 to remove name confusion into the current code.
Overall this is great and will approve it after the changes are made nice work! 🚀

Copy link
Collaborator

@Ethnical Ethnical left a comment

Choose a reason for hiding this comment

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

Looks good to me. Perfect! Thanks.

Copy link
Collaborator

@sbvegan sbvegan left a comment

Choose a reason for hiding this comment

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

@Ethnical I left a few comments for things that caught my eye. I'm not sure what's going into this drill or if the missing components are needed on this devnet. Let me know if they should be ignored and I can approve this PR

superchain/configs/sepolia-seal-0/sealchain-1.yaml Outdated Show resolved Hide resolved
superchain/configs/sepolia-seal-0/sealchain-2.yaml Outdated Show resolved Hide resolved
superchain/configs/sepolia-seal-0/semver.yaml Outdated Show resolved Hide resolved
superchain/configs/sepolia-seal-0/superchain.yaml Outdated Show resolved Hide resolved
@zchn
Copy link
Contributor

zchn commented May 13, 2024

@Ethnical could you look into the checks and make sure we are actually running all standard chain checks on these two chains? For example, I don't see seal in "check-security-config": https://app.circleci.com/pipelines/github/ethereum-optimism/superchain-registry/3051/workflows/92a4049f-8e3b-485b-ac73-7d34370cdfd8/jobs/9340/parallel-runs/0/steps/0-102

@ipatka
Copy link
Contributor Author

ipatka commented May 13, 2024

@Ethnical could you look into the checks and make sure we are actually running all standard chain checks on these two chains? For example, I don't see seal in "check-security-config": https://app.circleci.com/pipelines/github/ethereum-optimism/superchain-registry/3051/workflows/92a4049f-8e3b-485b-ac73-7d34370cdfd8/jobs/9340/parallel-runs/0/steps/0-102

it is not currently checking them so i had modified it locally to run them, i can update this so it checks these chains as well in the PR

@Ethnical
Copy link
Collaborator

Hey @ipatka,
Some mandatory requirements from the pipeline are silently failing (we should fix this issue in the future already discussed with @geoknee).
This is the script that should pass is here CheckSecurityConfigs.s.sol that is called from https://github.com/security-alliance/superchain-registry/blob/f14dc47551fdc4a5b48548a1b5bc8530d74f8ecd/scripts/check-security-configs.sh

This is because of the name of the network that is incorrect thus making the CI test silently failing.

The current workaround is to rename the network to sepolia.

For example, inside the file superchain/configs/chainids.json the correct value should be sepolia/sealchain-1 instead of the sepolia-seal-0/sealchain-1.

The path name : superchain/extra/addresses/sepolia-seal-0/sealchain-1.json should be instead superchain/extra/addresses/sepolia/sealchain-1.json.
This also apply for sealchain-2.

 string memory jsonDir = string.concat("superchain/extra/addresses/", network);

We need to have this test that pass correctly, if you can make it works would be amazing! :)

@rholterhus
Copy link
Contributor

Hi @Ethnical to make sure the CheckSecurityConfigs.s.sol script runs, we've updated locally to use sepolia as the SUPERCHAIN_TARGET instead of sepolia-seal-0. The CheckSecurityConfigs.s.sol script seems to succeed on our chain so that's good. I'll push the commit for this.

However there are two new errors when running go test in the validation folder. I think we can solve the first one but might need help on the second. Could you let me know your thoughts?

  1. Our challengePeriodSeconds we've planned to be 7 days, but in sepolia it looks like it's required to be 12s. Do you know if there's any way around this issue? Worst case scenario we can update our chain to be 12s, and then we can just pretend it's actually longer during the drill.

  2. At least one of our contracts won't have an implementation that's defined in implementations/networks/sepolia.yaml. Even if we add this contract under a new semver, it's not the "desired" version and we are getting an error like: Messages: OptimismPortalProxy.version=2.5.1 (UNACCEPTABLE desired version 3.8.0). Is there any way for us to get around this error? Maybe just having our new implementation match the desired semver would work (even if it's a different address)?

Other potential thoughts (we're happy to do whatever's easiest/quickest for you):

  • We can revert back to using own SUPERCHAIN_TARGET called sepolia-seal-0, but it seems like it's blocked on Update CheckSecurityConfigs to cover all superchain targets #230, is there any way we can help progress this, or is this mostly internal changes to the pipeline?
  • We can probably make our chains match the sepolia standard to get it through the PR pipeline, and then we just upgrade our implementation once the monitoring is already running, would that work?

Thanks!

rholterhus and others added 2 commits May 13, 2024 21:09
- Finalization period
- Challenger
- Guardian
- Superchain Config

Exclude seal chains from version check
@ipatka
Copy link
Contributor Author

ipatka commented May 14, 2024

Hi @Ethnical to make sure the CheckSecurityConfigs.s.sol script runs, we've updated locally to use sepolia as the SUPERCHAIN_TARGET instead of sepolia-seal-0. The CheckSecurityConfigs.s.sol script seems to succeed on our chain so that's good. I'll push the commit for this.

However there are two new errors when running go test in the validation folder. I think we can solve the first one but might need help on the second. Could you let me know your thoughts?

1. Our `challengePeriodSeconds` we've planned to be 7 days, but in `sepolia` it looks like it's required to be 12s. Do you know if there's any way around this issue? Worst case scenario we can update our chain to be 12s, and then we can just pretend it's actually longer during the drill.

2. At least one of our contracts won't have an implementation that's defined in `implementations/networks/sepolia.yaml`. Even if we add this contract under a new semver, it's  not the "desired" version and we are getting an error like: `Messages:       OptimismPortalProxy.version=2.5.1 (UNACCEPTABLE desired version 3.8.0)`. Is there any way for us to get around this error? Maybe just having our new implementation match the desired semver would work (even if it's a different address)?

Other potential thoughts (we're happy to do whatever's easiest/quickest for you):

* We can revert back to using own `SUPERCHAIN_TARGET` called `sepolia-seal-0`, but it seems like it's blocked on [Update CheckSecurityConfigs to cover all superchain targets #230](https://github.com/ethereum-optimism/superchain-registry/issues/230), is there any way we can help progress this, or is this mostly internal changes to the pipeline?

* We can probably make our chains match the `sepolia` standard to get it through the PR pipeline, and then we just upgrade our implementation once the monitoring is already running, would that work?

Thanks!

In the latest commit I updated the challengers, guardians, superchain config to match sepolia. Also excluded seal chains from version checks in validation following the pattern for other dev chains

I have not yet transferred proxy admin & system config owner until we confirm everything else is passing

@Ethnical
Copy link
Collaborator

@rholterhus Thanks! As always nice work 🔥

Our challengePeriodSeconds we've planned to be 7 days, but in sepolia it looks like it's required to be 12s. Do you know if there's any way around this issue? Worst case scenario we can update our chain to be 12s, and then we can just pretend it's actually longer during the drill.

Clearly! The idea of having something greater here make completely sense especially in this kind of exercise (This will be fixed fast -> #232)

At least one of our contracts won't have an implementation that's defined in implementations/networks/sepolia.yaml. Even if we add this contract under a new semver, it's not the "desired" version and we are getting an error like: Messages: OptimismPortalProxy.version=2.5.1 (UNACCEPTABLE desired version 3.8.0). Is there any way for us to get around this error? Maybe just having our new implementation match the desired semver would work (even if it's a different address)?

As the drill will be happening before pre-faults Proofs that make sense that the version is set before 3.8.0. The idea is too obviously allow this one here ( This PR should also fixed our issues as mentioned with @geoknee #232)

The current issues with CheckSecurityConfigs.s.sol is due an recent add from our side here if you could just rebase 1d3b7d5

@ipatka
Copy link
Contributor Author

ipatka commented May 14, 2024

@rholterhus Thanks! As always nice work 🔥

Our challengePeriodSeconds we've planned to be 7 days, but in sepolia it looks like it's required to be 12s. Do you know if there's any way around this issue? Worst case scenario we can update our chain to be 12s, and then we can just pretend it's actually longer during the drill.

Clearly! The idea of having something greater here make completely sense especially in this kind of exercise (This will be fixed fast -> #232)

At least one of our contracts won't have an implementation that's defined in implementations/networks/sepolia.yaml. Even if we add this contract under a new semver, it's not the "desired" version and we are getting an error like: Messages: OptimismPortalProxy.version=2.5.1 (UNACCEPTABLE desired version 3.8.0). Is there any way for us to get around this error? Maybe just having our new implementation match the desired semver would work (even if it's a different address)?

As the drill will be happening before pre-faults Proofs that make sense that the version is set before 3.8.0. The idea is too obviously allow this one here ( This PR should also fixed our issues as mentioned with @geoknee #232)

The current issues with CheckSecurityConfigs.s.sol is due an recent add from our side here if you could just rebase 1d3b7d5

Thanks! We've included all of these changes and looks like we're passing now

@Ethnical Ethnical merged commit 3f08c3f into ethereum-optimism:main May 14, 2024
7 checks passed
geoknee added a commit that referenced this pull request May 16, 2024
geoknee added a commit that referenced this pull request May 16, 2024
geoknee added a commit that referenced this pull request May 20, 2024
geoknee added a commit that referenced this pull request May 20, 2024
geoknee added a commit that referenced this pull request May 20, 2024
Ethnical pushed a commit that referenced this pull request May 20, 2024
geoknee added a commit that referenced this pull request May 20, 2024
geoknee added a commit that referenced this pull request May 20, 2024
)

* chore: removing the `sealchain1` & `sealchain2`

* chore: remove from the exclusion and the files from the configs

* chore: `}` add at the of the line to close it.

* remove: remove the rest of the git revert

* pnpm codegen

* Revert "Add SEAL chains for exercise (#225)" (#247)

This reverts commit 3f08c3f.

---------

Co-authored-by: George C. Knee <georgeknee@googlemail.com>
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.

6 participants