chore(edr_op): remove the unsupported OpHardfork::OSAKA variant - #1603
Conversation
|
OpHardfork::OSAKA variant
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Removes the OSAKA hardfork variant from the local OpHardfork model and related conversions, aligning supported hardforks with current OP needs and updating tests accordingly.
Changes:
- Removed
OSAKAfromOpHardfork, including string identifiers andop_revm::OpSpecIdconversion. - Updated
FromStr/&'static strconversions to no longer accept/emit"Osaka". - Adjusted test variant list and updated revm parity module documentation.
Suppressed comments (1)
crates/edr_op/src/hardfork.rs:124
- Removing support for parsing
\"Osaka\"is an externally-observable behavior change: inputs that previously parsed will now returnUnknownHardfork. If this string can appear in persisted configs/fixtures, consider keeping\"Osaka\"as a deprecated alias (either mapping to a chosen replacement or returning a more actionable error) or documenting/migrating callers accordingly.
impl FromStr for OpHardfork {
type Err = UnknownHardfork;
fn from_str(s: &str) -> Result<Self, Self::Err> {
match s {
name::BEDROCK => Ok(Self::BEDROCK),
name::REGOLITH => Ok(Self::REGOLITH),
name::CANYON => Ok(Self::CANYON),
name::ECOTONE => Ok(Self::ECOTONE),
name::FJORD => Ok(Self::FJORD),
name::GRANITE => Ok(Self::GRANITE),
name::HOLOCENE => Ok(Self::HOLOCENE),
name::ISTHMUS => Ok(Self::ISTHMUS),
name::JOVIAN => Ok(Self::JOVIAN),
name::INTEROP => Ok(Self::INTEROP),
_ => Err(UnknownHardfork),
}
}
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /// guarantee that the owned enum matches op-revm for every variant it | ||
| /// defines during the transition; DELETE THIS MODULE when an op-revm | ||
| /// upgrade removes variants from `OpSpecId`. |
| name::OSAKA => Ok(Self::OSAKA), | ||
| _ => Err(UnknownHardfork), | ||
| } | ||
| } |
There was a problem hiding this comment.
I don't think this is necessary
…ponding associated type names Two sites got narrower bounds: chain/spec/evm/src/lib.rs:27 — ContextForChainSpec only needs the EVM-level type, so it now projects through EvmHardforkChainSpec. edr_napi_core/src/solidity/config.rs:224-225 — the combined bound decomposed into EvmHardforkChainSpec<EvmHardfork: HardforkTr> + ProtocolHardforkChainSpec<ProtocolHardfork: FromStr<Err = UnknownHardfork>>.
…spec `PartialHeader::new` required `HardforkT: ProtocolHardfork` solely to reach `ProtocolParams::bomb_delay` for the Ethash fallback, which pushed L1 proof-of-work policy onto every chain: `OpHardfork::bomb_delay` was an `unreachable!()` and `OpChainSpec::MIN_ETHASH_DIFFICULTY` a meaningless 0. Chains now supply the computation as `ProviderChainSpec::default_block_difficulty`, carried down to the header layer as a `fn` pointer on `BlockConfig`. A pointer rather than a closure parameter because the reservation path resolves lazily behind `RwLock`s, arbitrarily far from `reserve_blocks`, with only the cloned `BlockConfig` crossing that boundary - and both it and `Reservation` derive `Clone`/`Debug`. No trait bound had to be widened to make this reachable. The Ethash formula moves to `edr_chain_l1::difficulty`, its only consumer. Its `assert!(spec_id >= BYZANTIUM)` and the `unreachable!()` in `bomb_delay` are both replaced by `PreMergeL1Hardfork`, whose `TryFrom<L1Hardfork>` failure *is* the post-merge branch, leaving `bomb_delay` total over an exhaustive match. `MIN_ETHASH_DIFFICULTY` and `BlockConfig::min_ethash_difficulty` are gone, and `ProtocolParams` keeps only `miner_reward`. `PartialHeader::new` and the storage/blockchain layers that forward to it now require just `Copy + Into<EvmSpecId> + PartialOrd`.
`BlockBuilder::finalize_block` took a `rewards` vector that every caller built identically, out of values the builder already owned: the amount from the blockchain's hardfork, and the recipient read straight off `block_builder.header().beneficiary`. The amount was threaded down from `edr_provider` as a `u128` so the builder could be told a number it could compute itself. `EthBlockBuilder` used to be both the shared Ethereum block-building machinery and L1's `BlockBuilder` implementation, while OP wrapped it. That dual role is what made the reward awkward to place, since the reward is L1-only but the engine is instantiated with `OpHardfork` too. It now loses its `BlockBuilder` impl and keeps only the engine role: the reward loop becomes `apply_rewards`, and `finalize` is reward-free. A new `L1BlockBuilder` wraps it and implements `BlockBuilder`, pinned to L1's hardfork type so it can pay the reward; `OpBlockBuilder` simply doesn't. Behavior is unchanged for OP, whose reward was already zero and skipped. With the reward L1-local, `ProtocolParams` has no members left. Deleting it reduces `ProtocolHardfork` to `Copy + Into<EvmSpecId> + PartialOrd`, which is purely derivable, and removes the `miner_reward` stub OP only carried to answer `None`.
`ProtocolHardfork::to_evm_spec_id` was a named `Into<EvmSpecId>` conversion. Remove it: call sites that can infer the target become a bare `.into()`; the rest annotate `let evm_spec_id: EvmSpecId = ...`, because protocol hardforks carry two `Into` bounds — `Into<Self::EvmHardfork>` and `Into<EvmSpecId>` — which coincide for L1 but not for a chain with its own EVM hardfork type. That ambiguity is now documented on the associated type. `ProtocolHardfork` itself stays, reduced to a blanket-implemented alias for `Copy + Into<EvmSpecId> + PartialOrd`: the hardfork-generic block and blockchain crates have no chain spec in scope to carry these bounds through an associated type, so the alias names the bundle once instead of repeating it at thirteen sites.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1603 +/- ##
==========================================
- Coverage 80.00% 79.99% -0.01%
==========================================
Files 453 453
Lines 79293 79292 -1
Branches 79293 79292 -1
==========================================
- Hits 63440 63432 -8
- Misses 13673 13677 +4
- Partials 2180 2183 +3 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
#1601 was meant to be behavior-preserving with respect to the currently pinned revm/op-revm versions, so OpHardfork mirrored every OpSpecId variant, including OSAKA. EDR currently does not support Osaka-equivalent execution in OP.
op-revm replaced the Osaka variant with Karst (the actual OP hardfork) in 20.0.0, published to crates.io through ethereum-optimism/optimism#20068.
This PR removes the variant, since we don't support Karst in OP. It is dormant, unreachable from the napi API (which stops at Isthmus) and never constructed (activations end at Jovian), so the removal is behavior-preserving. Whenever Karst is supported, the variant should be incorporated again under the right name.