-
Notifications
You must be signed in to change notification settings - Fork 682
feat: support shanghai hardfork
#4272
Conversation
Deploying with
|
| Latest commit: |
7ffc5c3
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://cb5ba1b5.ganache.pages.dev |
| Branch Preview URL: | https://update-ethereumjs.ganache.pages.dev |
shanghai hardfork
|
Note: CI failed due to time outs several times. I'm concerned that this PR introduces a performance regression and need to test Test timings in a more controlled manner (locally) before merging. |
f49ef2f to
4962f97
Compare
Co-authored-by: jeffsmale90 <[email protected]>
Co-authored-by: jeffsmale90 <[email protected]>
…nto update-ethereumjs
| function getCoinbase() public view virtual returns (uint256) { | ||
| uint256 balance = address(block.coinbase).balance; | ||
| require(balance != 0, "coinbase balance is not zero"); | ||
| return gasleft(); |
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.
accessCoinBase might be better, but it doesn't hint at what the return value is 🤷
| // this test uses the `network: "goerli"` option, which requires an | ||
| // infura key; when run our tests it must be provided as an environment | ||
| // variable. | ||
| if (!process.env.INFURA_KEY) this.skip(); |
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.
Are we able to check if we are in CI and fail if the INFURA_KEY is not set?
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.
Yes, github sets an env var we could check. This is a pattern we copied from elsewhere to skip tests that a run without providing an Infura key.
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.
Sounds like a good future improvement :)
| }).timeout(10000); | ||
|
|
||
| it("allows unlimited init code in transaction when the allowUnlimitedInitCodeSize option is set", async () => { | ||
| const largeInitCode = "0x" + "00".repeat(49153); // larger than init code allowance |
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.
Data.toString(0, 49153) would do the same 🤷
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.
Seems like a reasonable change to me! I think this test was a copy+paste of a similar test that predates Data.toString , so I just didn't bother to change the format.
…' and 'allowUnlimitedContractSize'
eth_call). A new test was added for this.allowUnlimitedInitCodeSizeto disable this EIP to continue to allow for uploading very large contracts. In most cases it will need to be combined with theallowUnlimitedContractSizeoption. Two new tests were added for this.withdrawalsandwithdrawalsRoot.console.loga deprecation notice if the SELFDESTRUCT opcode is hit, but I feel like this sort of warning should happen at compilation, and not by a dev net client like Ganache. Also, maintaining the deprecation notice forever would suck. 🤔This also fixes two bugs:
sizewas not computed correctly (persisted databases will be updated and migrated automatically)accessListfees, if there are any.