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: op-program supports custom chain config #12310

Merged
merged 6 commits into from
Oct 17, 2024

Conversation

qizhou
Copy link
Contributor

@qizhou qizhou commented Oct 4, 2024

Description

This PR enables custom chain config in op-program-client to be part of the target binary (MIPS, RISCV64, etc) using go:embed. As a result, such custom config will be reflected in the absolute prestate hash, and thus be applied to the on-chain FDG.

Tests

After spinning up a devnet via make devnet-up, pass make verify-devnet under op-program folder.

Additional context

See https://discord.com/channels/1244729134312198194/1290708296939999242/1291598392853790752

Metadata

Copy link
Contributor

@ajsutton ajsutton left a comment

Choose a reason for hiding this comment

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

Sorry for the delay in reviewing this.

op-program/Makefile Outdated Show resolved Hide resolved
op-program/chainconfig/chaincfg.go Outdated Show resolved Hide resolved
op-program/chainconfig/configs/rollup.json Outdated Show resolved Hide resolved
op-program/verify/verify.go Show resolved Hide resolved
op-program/chainconfig/chaincfg.go Outdated Show resolved Hide resolved
op-program/chainconfig/chaincfg.go Outdated Show resolved Hide resolved
Copy link

codecov bot commented Oct 8, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 64.04%. Comparing base (18732db) to head (ffd1f4a).
Report is 46 commits behind head on develop.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop   #12310      +/-   ##
===========================================
- Coverage    64.23%   64.04%   -0.19%     
===========================================
  Files           52       52              
  Lines         4353     4353              
===========================================
- Hits          2796     2788       -8     
- Misses        1381     1390       +9     
+ Partials       176      175       -1     
Flag Coverage Δ
cannon-go-tests 64.04% <ø> (-0.19%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

see 1 file with indirect coverage changes

@ajsutton ajsutton self-requested a review October 10, 2024 22:53
Copy link
Contributor

@ajsutton ajsutton left a comment

Choose a reason for hiding this comment

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

Looking good. Maybe the best way forward is to do the couple of nits around providing better error messages and leave this PR running op-program in process and merge it. Then do a follow up PR to make --network support a chain ID. That way we don't wind up creating too big a PR.

op-program/chainconfig/chaincfg.go Outdated Show resolved Hide resolved
op-program/chainconfig/chaincfg.go Outdated Show resolved Hide resolved
op-program/verify/verify.go Show resolved Hide resolved
@qizhou
Copy link
Contributor Author

qizhou commented Oct 12, 2024

Hi @ajsutton, let me know if you have further comments on this. Meanwhile, I am preparing the PR for supporting chain ID as network ID.

Copy link
Contributor

@ajsutton ajsutton left a comment

Choose a reason for hiding this comment

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

Sorry I lost track of what I'd said on this and was thinking we'd land the load-by-chain-id change first. But I'd said merge this and that probably does make more sense. I think it looks good so will get it merged in. The CI setup has changed a little in the mean time so I may have to jump a couple of hurdles but we'll get there. :)

@ajsutton ajsutton added this pull request to the merge queue Oct 17, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Oct 17, 2024
@ajsutton ajsutton added this pull request to the merge queue Oct 17, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to no response for status checks Oct 17, 2024
@ajsutton ajsutton added this pull request to the merge queue Oct 17, 2024
Merged via the queue into ethereum-optimism:develop with commit 527bf6c Oct 17, 2024
58 checks passed
samlaf pushed a commit to samlaf/optimism that referenced this pull request Nov 10, 2024
* feat: op-program supports custom chain config

* address comments

* address comments

* update mainnet.go/sepolia.go

* Update op-program/chainconfig/chaincfg.go

Co-authored-by: Adrian Sutton <[email protected]>

* more error info on config file not found

---------

Co-authored-by: Qi Zhou <[email protected]>
Co-authored-by: Adrian Sutton <[email protected]>
ajsutton added a commit that referenced this pull request Dec 1, 2024
* feat: op-program supports custom chain config

* address comments

* address comments

* update mainnet.go/sepolia.go

* Update op-program/chainconfig/chaincfg.go

Co-authored-by: Adrian Sutton <[email protected]>

* more error info on config file not found

---------

Co-authored-by: Qi Zhou <[email protected]>
Co-authored-by: Adrian Sutton <[email protected]>
ajsutton added a commit that referenced this pull request Dec 8, 2024
* feat: op-program supports custom chain config

* address comments

* address comments

* update mainnet.go/sepolia.go

* Update op-program/chainconfig/chaincfg.go

Co-authored-by: Adrian Sutton <[email protected]>

* more error info on config file not found

---------

Co-authored-by: Qi Zhou <[email protected]>
Co-authored-by: Adrian Sutton <[email protected]>
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.

2 participants