-
Notifications
You must be signed in to change notification settings - Fork 82
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!: upstream to optimism v1.3.0 #239
Conversation
…evnet Enable Canyon in the devnet
op-bindings: regenerate
Bumps [github.com/google/uuid](https://github.com/google/uuid) from 1.3.1 to 1.4.0. - [Release notes](https://github.com/google/uuid/releases) - [Changelog](https://github.com/google/uuid/blob/master/CHANGELOG.md) - [Commits](google/uuid@v1.3.1...v1.4.0) --- updated-dependencies: - dependency-name: github.com/google/uuid dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com>
- add match check-values-match job that searches using grep - define workflow to verify initializer value matches between contract and genesis
fix: string encode error TS2345
…/github.com/google/uuid-1.4.0 build(deps): bump github.com/google/uuid from 1.3.1 to 1.4.0
fix: race condition updating last updated scorebook timestamp
…hain-mon_docker_build chore: Improve chain-mon docker build
ops: handle warm-up output case during docker publish correctly
…rn/types/node-20.8.9 build(deps-dev): bump @types/node from 20.8.8 to 20.8.9
…config op-node: Pull in Canyon Time from superchain registry
ci: Add filters so op-stack-go-docker-build-release builds on tags.
contracts-bedrock: modern import style in tests
Adds a new helper library called `EIP1967Helper` that can get the admin and impl storage slots from a `Proxy` implementing ERC1967. This is more helpful to use than hardcoded magic values such as `multisig` because it is not clear who the multisig is since its value is assigned in a different file. We want to decouple the value from a magic value and set it to exactly what we want it to be which is the admin. This will work in all cases no matter what the admin is since it dynamically pulls the value from storage for the tests.
88baeee
to
7ba1fb1
Compare
7ba1fb1
to
e00548a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
Currently, only Legolith and Spanbatch are applied to devnet environments, why not add the Canyon flag as well? |
And if we are going to include the Canyon hardfork in the next release, it would be nice to include the Canyon environment variable in the script to work in the e2e environment. (OP_E2E_USE_CANYON) |
kroma-chain-ops/genesis/setters.go
Outdated
|
||
// [Kroma: START] | ||
var ( | ||
L1ProxyCount = uint64(2048) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't this value now unnecessary because of the improvements?
e00548a
to
1059db8
Compare
"l1GenesisBlockTimestamp": "0x64c811bf", | ||
"l2GenesisRegolithTimeOffset": "0x0", | ||
"l2GenesisSpanBatchTimeOffset": "0x0", | ||
"l2GenesisCanyonTimeOffset": "0x40", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was wondering if there is a reason why it is 0x40 and not 0x00.
And for l2GenesisCanyonTimeOffset to be set, shouldn't eip1559DenominatorCanyon also be included?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
eip1559DenominatorCanyon added.
If I set the l2GenesisCanyonTimeOffset to 0x40, I think we can test the hard fork on the devnet.
op-node/p2p/gossip.go
Outdated
@@ -69,10 +70,14 @@ func blocksTopicV1(cfg *rollup.Config) string { | |||
return fmt.Sprintf("/kroma/%s/0/blocks", cfg.L2ChainID.String()) | |||
} | |||
|
|||
func blocksTopicV2(cfg *rollup.Config) string { | |||
return fmt.Sprintf("/optimism/%s/1/blocks", cfg.L2ChainID.String()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't it be nice to unify the format for v1, v2 topics?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why the legacy-bindings
and predeployes/address.go
are deleted in op-bindings
? I know the deleted things are not used in Kroma, but it would also be good to have in terms of less changes compared with Optimism
op-node/p2p/gossip.go
Outdated
@@ -69,10 +70,14 @@ func blocksTopicV1(cfg *rollup.Config) string { | |||
return fmt.Sprintf("/kroma/%s/0/blocks", cfg.L2ChainID.String()) | |||
} | |||
|
|||
func blocksTopicV2(cfg *rollup.Config) string { | |||
return fmt.Sprintf("/optimism/%s/1/blocks", cfg.L2ChainID.String()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just curiosity, this filename ends with tests, so I think it's not being recognized as a test file by golang.
Is this by design?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
l2_engine_test.go
will test it.
packages/contracts/scripts/differential-testing/differential-testing.go
Outdated
Show resolved
Hide resolved
1059db8
to
46cf288
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Before merging, have you tested running a node locally sync with live network (mainnet or sepolia)?
4c992f3
to
1c2755b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
1c2755b
to
b2e2a80
Compare
Description
This is a PR to upstream to Optimism v1.3.0.
Note that I have reduced diffs from Optimism as much as possible to make the upstream task easier.
As a result, there are some breaking changes.
BREAKING CHANGES
regolith_time
field must be added to the rollup.json andl2GenesisRegolithTimeOffset
to the deploy-config files. Note that Kroma is always post-regolith environment.kroma-
prefix.make devnet-allocs
command to create anallocs.json
before e2e testing.packages/contracts/deploy-config/devnetL1-template.json
.