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

Superchain Registry 2.0 #816

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Superchain Registry 2.0 #816

wants to merge 4 commits into from

Conversation

mslipper
Copy link
Contributor

@mslipper mslipper commented Dec 28, 2024

See here for context.

@mslipper mslipper requested a review from a team as a code owner December 28, 2024 02:46
@mslipper mslipper marked this pull request as draft December 28, 2024 02:46
@mslipper mslipper added the F-do-not-merge Flag: Do Not Merge label Dec 28, 2024
@OptimismBot
Copy link

OptimismBot commented Dec 28, 2024

Standards Compliance Report

L1 Deployment

Release: op-contracts/v1.6.0

Deployment Transaction Hash: 0x0193884dc77ba74ca9ae23079edf91b81d49b0243c486fab5bbecd415d2dad68

Semvers
Contract Std Version Got Version
SystemConfig 2.2.0 2.2.0
⚠️ PermissionedDisputeGame 1.3.0 1.3.1-beta.3
OptimismPortal 3.10.0 3.10.0
⚠️ AnchorStateRegistry 2.0.0 2.0.1-beta.3
DisputeGameFactory 1.0.0 1.0.0
L1CrossDomainMessenger 2.3.0 2.3.0
L1StandardBridge 2.1.0 2.1.0
L1ERC721Bridge 2.1.0 2.1.0
OptimismMintableERC20Factory 1.9.0 1.9.0
Ownership
Contract Std Address Got Address
Guardian 0x7a50f00e8d05b95f98fe38d8bee366a7324dcf7e 0x7a50f00e8D05b95F98fE38d8BeE366a7324dCf7E
Challenger 0xfd1d2e729ae8eee2e146c033bf4400fe75284301 0xfd1D2e729aE8eEe2E146c033bf4400fE75284301
ProxyAdminOwner 0x1eb2ffc903729a0f03966b917003800b145f56e2 0x1Eb2fFc903729a0F03966B917003800b145F56E2
Permissioned Proofs
Contract Std Param Got Param
GameType 1 1
AbsolutePrestate 0x038512e02c4c3f7bdaec27d00edf55b7155e0905301e1a88083e4e0a6764d54c 0x038512e02c4c3f7bdaec27d00edf55b7155e0905301e1a88083e4e0a6764d54c
MaxGameDepth 73 73
SplitDepth 30 30
MaxClockDuration 302400 302400
ClockExtension 10800 10800

L2 Deployment

✅ Genesis matches standard!

Report generated on 2025-01-03T06:45:38Z for commit ce926bd763e432d7a25dbb0b722178364d055c93

@mslipper mslipper force-pushed the feat/new-workflow branch 11 times, most recently from 72aa7ef to 7f81b55 Compare December 31, 2024 21:18
@codecov-commenter
Copy link

Welcome to Codecov 🎉

Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests.

Thanks for integrating Codecov - We've got you covered ☂️

@mslipper mslipper force-pushed the feat/new-workflow branch 6 times, most recently from e9d20d6 to dbee18d Compare December 31, 2024 21:52
@mslipper mslipper force-pushed the feat/new-workflow branch 2 times, most recently from 195557b to d286b89 Compare January 3, 2025 22:38
@mslipper mslipper changed the title [DO NOT MERGE] testing new workflow Superchain Registry 2.0 Jan 3, 2025
@mslipper mslipper marked this pull request as ready for review January 3, 2025 22:49
[optimism]
eip1559_elasticity = 10
eip1559_denominator = 50
eip1559_denominator_canyon = 250

[alt_da]
da_challenge_contract_address = "0x08c5DCDD5e46d31CC1591ee15b084663507597f3"
da_challenge_contract_address = "0x08c5dcdd5e46d31cc1591ee15b084663507597f3"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we store the check-summed version of the address here as we did before?

Copy link
Collaborator

Choose a reason for hiding this comment

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

We should probably also reinstate a CI check that all addresses are checksummed? Previously, we checked this via the go bindings at init-time.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If the checksums are important I can bring them back.

public_rpc = "https://rpc.arena-z.gg"
sequencer_rpc = "https://rpc.arena-z.gg"
explorer = "https://explorer.arena-z.gg"
superchain_level = 0
governed_by_optimism = true
standard_chain_candidate = true # This is a temporary field which causes most of the standard validation checks to run on this chain
data_availability_type = "eth-da"
chain_id = 7897
Copy link
Collaborator

Choose a reason for hiding this comment

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

From memory we may have wanted the chain_id nearer the top of the file (for easier human readability). I don't feel strongly but wanted to flag for @tessr's input.

SystemConfigOwner = "0xBeA2Bc852a160B8547273660E22F4F08C2fa9Bbb"
ProxyAdminOwner = "0x5a0Aae59D09fccBdDb6C6CcEB07B7279367C3d2A"
Guardian = "0x09f7150D8c019BeF34450d6920f6B3608ceFdAf2"
Challenger = "0x9BA6e03D8B90dE867373Db8cF1A58d2F7F006b3A"
Proposer = "0x5f16E66D8736B689a430564a31c8d887ca357CD8"
UnsafeBlockSigner = "0xb774Ca8438319d2a97B9925F4CD248e4C470Ac5B"
BatchSubmitter = "0x2b8733E8c60A928b19BB7db1D79b918e8E09AC8c"

[addresses]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could we use the term contracts instead of addresses here to disambiguate between this and roles (since roles are also addresses)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm OK with that.

public_rpc = "https://rpc.ata.network"
sequencer_rpc = "https://automata-mainnet.alt.technology/"
explorer = "https://explorer.ata.network"
superchain_level = 0
governed_by_optimism = false
data_availability_type = "alt-da"
chain_id = 65536
batch_inbox_addr = "0xff00000000000000000000000000000001111111"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could be saved for future work but while we're in here -- we could moved batch inbox address and gas paying token to addresses table? Or the addresses table gets renamed to something more specific such as l1_contract_addresses.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Related to #816 (comment)

Comment on lines -99 to -100
gsutil cp ./generate-genesis/output-${COMMIT_HASH}/${CHAIN}.json \
gs://${GCS_BUCKET}/${FOLDER}/${CHAIN}/${COMMIT_HASH}/genesis.json
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are we comfortable removing this? Is it unused / what is the plan for migrating it if it is still used? (I'm a bit out of touch with this workflow so I'm not sure mysefl).

Copy link
Contributor Author

@mslipper mslipper Jan 6, 2025

Choose a reason for hiding this comment

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

I have no idea if this was used anywhere. I couldn't find any references to it.

Comment on lines 1 to -2
[
{
Copy link
Collaborator

Choose a reason for hiding this comment

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

Was it intentional to make changes / reorder this file?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The order was not consistently applied, so I updated the file to list all chains by name.


for _, cfg := range cfgs {
chainCfg := cfg.Config
if chainCfg.SuperchainTime != nil && int64(*chainCfg.SuperchainTime) < time.Now().Unix() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it necessary to restrict this to when SuperchainTime is in the past? I think chains ought to be able to set it to the the future and things work as expected? I think in general we shouldn't depend on Now at all.

Reference for superchain_time semantics: https://github.com/ethereum-optimism/superchain-registry/blob/main/docs/hardfork-activation-inheritance.md

Copy link
Contributor Author

Choose a reason for hiding this comment

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

According to the linked doc, this field functions like a hardfork activation timestamp:

All superchain-wide hardfork activations will be inherited starting from this timestamp.

If we don't check time.Now() then we will be applying superchain-wide hardfork activations prior to the timestamp in superchain_time.

Comment on lines +3 to +6
use (
.
../validation
)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not sure what the purpose of this file is, haven't seen it used in this way before. We did have one previously but it was in the root of the repo. Can you clarify?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ops needs it so that local changes in the validation package carry through without requiring manual version bumps.

Comment on lines +46 to +50
a := &Hardforks{
CanyonTime: NewHardforkTime(1),
DeltaTime: NewHardforkTime(2),
}
b := &Hardforks{
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: if we used src and dest this would be slightly easier to read since there is an implicit direction here.

readGen, err := DecompressGenesis(wd, superchain, shortName)
require.NoError(t, err)

require.Equal(t, testGen, readGen)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe overkill, but we could check that the result decompresses again to the input every time we invoke the compression? Unit test looks good but we may as well put extra runtime protection in.


ops/bin

.staging
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm confused by this. If this directory is gitignored, how does the user add their intent.toml and state.json file to it and push it as part of a pr? Or is that not the intended user flow for generating the report?

Comment on lines +14 to +16
// include regolith to demonstrate how unknown destination fields
// are not copied
L2GenesisRegolithTimeOffset: new(hexutil.Uint64),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we want to silently ignore fields like this? I want to make sure we would catch a situation where the next hardfork is missing from the SCR structs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can update this so that it panics.

| Chain Name | OP Governed | Upgradeable | Explorer | Public RPC | Sequencer RPC
|---|---|---|---|---|---|
{{- range index $.ChainData $index }}
| {{ .Name }} | {{ checkmark .GovernedByOptimism }} | {{ optedInSuperchain .SuperchainTime }} | {{ .Explorer }} | `{{ .PublicRPC }}` | `{{ .SequencerRPC }}` |
Copy link
Collaborator

@bitwiseguy bitwiseguy Jan 6, 2025

Choose a reason for hiding this comment

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

How does the chainConfig.GovernedByOptimism value get set now? Will that be part of the chain's intent.toml or state.json file? I'm wondering if we should remove that boolean from the ChainConfig struct and dynamically determine if the chain is currently governed by optimism (since the value can change after initial deployment) when generating the CHAINS.md file instead of relying on a static field

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's set in meta.toml.

alias t := test-all
alias l := lint-all

# Adding a chain
add-chain:
Copy link
Collaborator

Choose a reason for hiding this comment

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

What is the complete user flow that replaces this command? I think the answer to that will help me review this pr overall. Is there any op-deployer in-flight work that needs to be merged to make it compatible with the SR 2.0? I noticed this op-deployer pr was closed but not sure if there is a replacement

return nil, fmt.Errorf("failed to get OPCM address: %w", err)
}

receipt, err := client.TransactionReceipt(ctx, deploymentTx)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Instead of parsing the DeployedEvent, which just contains information about the addresses at a previous fixed point in time, would it be better for the report command to query the SystemConfig contract and gather all the contract addresses and config info needed through that? That way the report would give info for the current state of the chain instead of potentially outdated information. Could grab the SystemConfigProxy address from the chain config stored in the superchain-registry.

A few advantages to removing dependence on deployment tx hash:

  1. Chains that did not deploy via op-deployer can still generate a report
  2. Consumers of the report (e.g. a superchain health dashboard) can continually generate real-time reports based on current chain config params (read from on-chain) and show a current standard compliance status across all chains

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The deployment event proves that the system was deployed via op-deployer, whereas querying the systemconfig does not. This allows us to skip certain checks altogether as long as we assume that OPCM is correct.

Copy link
Collaborator

@bitwiseguy bitwiseguy Jan 7, 2025

Choose a reason for hiding this comment

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

But almost all chains (or maybe all?) that are currently in the registry did not use op-deployer. So does that mean we just won't run the reports for those chains? Or can those chains be onboarded onto OPCM and future contract upgrades for those chains will emit a Deployed event, at which time the registry could start to generate the report? Or are you only concerned with running the report once with the sole purpose to be a gatekeeper when the chain tries to enter the registry, and then never again so it won't matter for chains that are already in the registry?

Copy link
Contributor Author

@mslipper mslipper Jan 7, 2025

Choose a reason for hiding this comment

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

My strawman is that we grandfather those chains in, and focus on the chains that use our governance system. Future contract upgrades will also use the OPCM, so there's less of a need to continually check each chain.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
F-do-not-merge Flag: Do Not Merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants