Optimism - Isthmus#8260
Conversation
0d4a10a to
cffab71
Compare
| /// <summary> | ||
| /// https://github.com/ethereum-optimism/specs/blob/main/specs/protocol/preinstalls.md#create2deployer | ||
| /// </summary> | ||
| public static readonly Address Create2Deployer = new("0x13b0D85CcB8bf860b6b79AF3029fCA081AE9beF2"); |
There was a problem hiding this comment.
Our superchain.py script generates the appropriate Chainspec file which includes constants like create2DeployerAddress so we don't need to have these hardcoded:
nethermind/scripts/superchain.py
Line 90 in fd56a42
There was a problem hiding this comment.
The reason for it being amended here was the chainspec included in the optimism-package had none of these values and they were required to run it. Also, this is aligned to how op-geth works with the optimism specific values that are specified in the spec of the op stack. Probably for further discussion.
There was a problem hiding this comment.
We should probably update the scripts in the optimism-package (gen2spec.jq) then so we include these fields like we do in our superchain.py script.
Avoid `null` in chainspec files
emlautarom1
left a comment
There was a problem hiding this comment.
There are a couple of changes that seem redundant at first glance, and there is some commented out code (in tests too).
emlautarom1
left a comment
There was a problem hiding this comment.
LGTM. A couple of small comments pending (merge OptimismWithdrawalsProcessor), and some unused imports.
This comment was marked as duplicate.
This comment was marked as duplicate.
Sorry, something went wrong.
| /// <summary> | ||
| /// Builds the block context ouf the block and the release spec. | ||
| /// </summary> | ||
| protected virtual BlockExecutionContext BuildBlockContext(Block block, IReleaseSpec spec) => new(block.Header, spec); |
There was a problem hiding this comment.
Let's also put IReleaseSpec in BlockExecutionContext, so we won't have to pass SpecProvider or spec to some places!
There was a problem hiding this comment.
We definitely could do it @LukaszRozmej . Let's merge and release Isthmus first, then I'll send a small PR to make this externalized.
There was a problem hiding this comment.
Sure don't want to block urgent stuff. Just throwing improvement ideas.
flcl42
left a comment
There was a problem hiding this comment.
src/tests updated to older version probably
Resolves #8235
This PR introduces
Isthmushard fork forOptimismChanges
Types of changes
What types of changes does your code introduce?
Testing
Requires testing
If yes, did you write tests?
Notes on testing
A hard fork.
Documentation
Requires documentation update
Requires explanation in Release Notes
Link to the Optimism spec and potentially more, whatever we do on the hard fork.