Import changes from deprecated contracts repo #476
Conversation
🦋 Changeset detectedLatest commit: 93f81c3 The changes in this PR will be included in the next version bump. This PR includes changesets to release 5 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
maurelian
changed the title
| * @param _creationCode Code to pass into CREATE for deployment. | ||
| * @param _address OVM address being deployed to. | ||
| */ | ||
| function safeCREATE( | ||
| uint _gasLimit, | ||
| uint, // _gasLimit |
There was a problem hiding this comment.
Hmmm, I think this was me, but I think Kelvin would want to remove this param internally to be more consistent with the EVM, so cc @smartcontracts
|
Some modifications to geth are required for the integration tests to pass
|
Right, this change was in ethereum-optimism/contracts#300, which was abandoned, but my understanding is that the changes are still desired. If necessary I can separate out the changes into multiple PRs. |
There was a problem hiding this comment.
Thanks for porting these changes over! 😄
Right, this change was in ethereum-optimism/contracts#300, which was abandoned, but my understanding is that the changes are still desired.
The RLP changes are definitely desired, but I think we should have those in a separate PR so that reviewers can look at the contract changes and geth changes together. I also looked through the state manager in geth and luckily it looks like the nonce type change here won't require any geth work. Can you make this PR be for everything except the RLP changes? I don't think those even need to be a PR until @smartcontracts has done some more work there; I think we're gonna modify the contract account be an L2-compiled contract now anyway.
This reverts commit af9f9a6.
|
OK, fingers crossed just reverting the commit does it. I did a quick scan of the revert diff, and it looks OK to me. |
| * @param _in RLP uint64 value. | ||
| * @return Decoded uint64. | ||
| */ | ||
| function readUint64( |
| @@ -744,6 +744,24 @@ const test_ovmCREATE: TestDefinition = { | |||
| }, | |||
| ], | |||
| }, | |||
| { | |||
| name: 'ovmCREATE(UNSAFE_CODE)', | |||
| steps: [ | |||
There was a problem hiding this comment.
This was already the case right? This test does not seem to be accompanied by any extra code in the safety checker.
| decodedTx.data | ||
| ); | ||
| if (decodedTx.value > 0) { | ||
| Lib_SafeExecutionManagerWrapper.safeREQUIRE( |
There was a problem hiding this comment.
Why is this needed? Was it the case before, was it a bug?
Also, it seems like integration tests are failing, did you run them locally?
|
I had hoped to import a few different changes from the old repo, in such a way that would preserve the commit history.
|
Description
This PR is brings in the remaining changes that were left behind in the contracts repo, with a clean commit history.
Additional context
This branch includes changes which were not made by @smartcontracts and @ben-chain. They should do a sanity check on their code.