validation: add global chainId check #153

geoknee · 2024-04-02T10:26:08Z

Closes #299

Test currently failing due to name mismatches.
It is likely and acceptable for a chain to already exist at chainId.network, so we need a way of checking whether it is the "same chain" or not. Using the name seems reasonable, since that it is required to be unique also .

=== RUN   TestGloballyUniqueChainId
    /Users/georgeknee/code/ethereum-optimism/superchain-registry/validation/chainId_test.go:22: Chain OP-Mainnet with ID 10 exists in global chain list with name OP Mainnet
    /Users/georgeknee/code/ethereum-optimism/superchain-registry/validation/chainId_test.go:22: Chain Zora Sepolia with ID 999999999 exists in global chain list with name Zora Sepolia Testnet
    /Users/georgeknee/code/ethereum-optimism/superchain-registry/validation/chainId_test.go:22: Chain Lyra with ID 957 exists in global chain list with name Lyra Chain
    /Users/georgeknee/code/ethereum-optimism/superchain-registry/validation/chainId_test.go:22: Chain Orderly Network with ID 291 exists in global chain list with name Orderly Mainnet
    /Users/georgeknee/code/ethereum-optimism/superchain-registry/validation/chainId_test.go:22: Chain PGN with ID 424 exists in global chain list with name PGN (Public Goods Network)
    /Users/georgeknee/code/ethereum-optimism/superchain-registry/validation/chainId_test.go:22: Chain Base Sepolia with ID 84532 exists in global chain list with name Base Sepolia Testnet
    /Users/georgeknee/code/ethereum-optimism/superchain-registry/validation/chainId_test.go:22: Chain OP-Sepolia with ID 11155420 exists in global chain list with name OP Sepolia Testnet
    /Users/georgeknee/code/ethereum-optimism/superchain-registry/validation/chainId_test.go:22: Chain PGN Sepolia with ID 58008 exists in global chain list with name Sepolia PGN (Public Goods Network)

Open to suggestions on how to resolve this. i think it seems reasonable to require the same name in both repos?

mds1 · 2024-04-02T13:40:29Z

My preferred solution would be:

If the chain is not in the upstream repo: we do the chain ID + name + short name uniqueness checks ourselves
If the chain is in the upstream repo: make sure everything added here matches

What do you think of that?

geoknee · 2024-04-03T21:06:53Z

@mds1 I updated the logic to match your suggestion.

I also went through the existing chains to add a short name and correct the existing names where discrepancies exist.

Further, the contributing guide has been updated.

Note that this will fail all in flight PRs, but they won't be hard to fix.

If the chain is not in the upstream repo: we do the chain ID + name uniqueness checks ourselves If the chain is in the upstream repo: make sure everything added here matches TODO for short names (we don't have any of these committed at present)

This reverts commit ba506c5.

if in the global list, it matches the short name from there

validation/uniqueness_test.go

geoknee · 2024-04-04T09:55:46Z

superchain/configs/mainnet/op.yaml

@@ -1,4 +1,5 @@
-name: OP-Mainnet
+name: OP Mainnet


We should verify if these changes will break something in the downstream dependencies (or even somewhere in this repo that I have missed).

ethereum-optimism/optimism#10043

ethereum-optimism/op-geth#287

to commit from ethereum-optimism/superchain-registry#153

circular dependencies, sigh

Closes #160

renaming on a mac, git doesn't detect the change otherwise

geoknee · 2024-04-04T16:48:20Z

@mds1 I'm not sure why chek-security-configs is failing. Do you have any insight?

mds1 · 2024-04-04T17:51:48Z

Checking OptimismPortalProxy 0x16Fc5058F25648194471939df75CF27A2fdC48BC
    -- Success: 0x189aBAAaa82DfC015A588A7dbaD6F13b1D3485Bc == 0x16Fc5058F25648194471939df75CF27A2fdC48BC.admin().
    -- Success: 0xDEe57160aAfCF04c34C887B5962D0a69676d3C8B == 0x16Fc5058F25648194471939df75CF27A2fdC48BC.GUARDIAN().
    !! Error calling 0x16Fc5058F25648194471939df75CF27A2fdC48BC.L2_ORACLE()
    !! Error: 0x90E9c4f8a994a250F6aEfd61CAFb4F2e895D458F != 0x16Fc5058F25648194471939df75CF27A2fdC48BC.L2_ORACLE(), 
             which is 0x0000000000000000000000000000000000000000

├─ [648] 0x16Fc5058F25648194471939df75CF27A2fdC48BC::L2_ORACLE() [staticcall]
    │   ├─ [192] 0x215b3A2d1766616d1C4701f3D117348711BaAe93::L2_ORACLE() [delegatecall]
    │   │   └─ ← EvmError: Revert

So it's reverting because L2_ORACLE method does not exist on the target contract. And the 0x215b3A2d1766616d1C4701f3D117348711BaAe93 contract is the FPAC OptimismPortal2 where the L2OO has been removed. So I think we need to use chainUpgradedToFPAC for basesep now. (I did not realize Base Sepolia upgraded to FPAC but seems they have)

geoknee · 2024-04-04T20:21:06Z

Thanks, need to get better at parsing those traces :-) Is there anything we can do to make that easier?

(I did not realize Base Sepolia upgraded to FPAC but seems they have)

I did run this check on main and it passes, so there must be something linked to my renaming changes on this branch which are causing the test to fail.

I think we actually just need to update chainUpgradedToFPAC for opsep (not basesep).

mds1 · 2024-04-04T20:26:33Z

I think we actually just need to update chainUpgradedToFPAC for opsep (not basesep).

Ah yes, foundry runs tests in parallel so the console.logs get a little confusing. But given the address you are correct

geoknee · 2024-04-04T20:28:12Z

scripts/CheckSecurityConfigs.s.sol

+ sliceString(addressesJsonPath, bytes(addressesJsonPath).length - 18, bytes(addressesJsonPath).length)
 )
- ) == keccak256(abi.encodePacked("sepolia/op.json"));


Really keen to replace this code (see #144).

Just pushed 3e61dba to clean this up, feel free to revert the commit if you prefer

geoknee · 2024-06-25T12:59:32Z

In favour of #320

geoknee self-assigned this Apr 3, 2024

geoknee requested a review from mds1 April 3, 2024 21:10

geoknee marked this pull request as ready for review April 3, 2024 21:10

geoknee requested review from a team as code owners April 3, 2024 21:10

geoknee requested a review from ajsutton April 3, 2024 21:10

geoknee added 12 commits April 3, 2024 22:11

validation: add global chainId check

aeac1d3

enhance checks:

6ad1718

If the chain is not in the upstream repo: we do the chain ID + name uniqueness checks ourselves If the chain is in the upstream repo: make sure everything added here matches TODO for short names (we don't have any of these committed at present)

remove short name check placeholder comments

6f24c76

harmonize chain names with chainid.network

044cf83

invert expected / actual

0bb5da2

rename file

7e98caa

Revert "remove short name check placeholder comments"

6b33055

This reverts commit ba506c5.

add a short name for every chain

ddb24d7

if in the global list, it matches the short name from there

update add-chain script to include short name

d3c2af9

remove todo

d0f037b

tidy up

05dd9dd

lint

730d61f

geoknee force-pushed the gk/global-chainid-check branch from 9190562 to 730d61f Compare April 3, 2024 21:11

geoknee commented Apr 3, 2024

View reviewed changes

validation/uniqueness_test.go Show resolved Hide resolved

geoknee mentioned this pull request Apr 4, 2024

adding a chain: allow filename to be distinct from chain name #160

Closed

geoknee commented Apr 4, 2024

View reviewed changes

zchn assigned mds1 Apr 4, 2024

geoknee added a commit to ethereum-optimism/optimism that referenced this pull request Apr 4, 2024

pin superchain module

fa994f4

to commit from ethereum-optimism/superchain-registry#153

geoknee mentioned this pull request Apr 4, 2024

WIP: pull in changes to chain names from superchain ethereum-optimism/optimism#10043

Closed

replace chainConfig.Chain with .ShortName

7e8a79f

geoknee mentioned this pull request Apr 4, 2024

WIP: pull in naming changes from registry ethereum-optimism/op-geth#287

Draft

update op-geth dependency

8c03187

circular dependencies, sigh

geoknee added 3 commits April 4, 2024 16:55

change filenames to reflect chain.ShortName

c362a26

set filename to chain short name

e40bca1

Closes #160

pnpm codegen

93e3e53

geoknee marked this pull request as draft April 4, 2024 16:14

use git mv to handle casing change for filenames

47b93f7

renaming on a mac, git doesn't detect the change otherwise

fix CheckSecurityConfigs.s.sol

36fe081

geoknee commented Apr 4, 2024

View reviewed changes

chore: cleanup string handling

3e61dba

geoknee closed this Jun 25, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

validation: add global chainId check #153

validation: add global chainId check #153

geoknee commented Apr 2, 2024 •

edited

Loading

mds1 commented Apr 2, 2024

geoknee commented Apr 3, 2024 •

edited

Loading

geoknee Apr 4, 2024

geoknee Apr 4, 2024

geoknee Apr 4, 2024

geoknee commented Apr 4, 2024

mds1 commented Apr 4, 2024

geoknee commented Apr 4, 2024 •

edited

Loading

mds1 commented Apr 4, 2024

geoknee Apr 4, 2024

mds1 Apr 4, 2024

geoknee commented Jun 25, 2024

validation: add global chainId check #153

validation: add global chainId check #153

Conversation

geoknee commented Apr 2, 2024 • edited Loading

mds1 commented Apr 2, 2024

geoknee commented Apr 3, 2024 • edited Loading

geoknee Apr 4, 2024

Choose a reason for hiding this comment

geoknee Apr 4, 2024

Choose a reason for hiding this comment

geoknee Apr 4, 2024

Choose a reason for hiding this comment

geoknee commented Apr 4, 2024

mds1 commented Apr 4, 2024

geoknee commented Apr 4, 2024 • edited Loading

mds1 commented Apr 4, 2024

geoknee Apr 4, 2024

Choose a reason for hiding this comment

mds1 Apr 4, 2024

Choose a reason for hiding this comment

geoknee commented Jun 25, 2024

geoknee commented Apr 2, 2024 •

edited

Loading

geoknee commented Apr 3, 2024 •

edited

Loading

geoknee commented Apr 4, 2024 •

edited

Loading