Skip to content
This repository has been archived by the owner on Apr 12, 2021. It is now read-only.

tests: fix the enums to match what is batch submitted #303

Closed
wants to merge 2 commits into from
Closed

Conversation

tynes
Copy link
Collaborator

@tynes tynes commented Mar 8, 2021

Description

This is a requirement for ethereum-optimism/go-ethereum#255
The enums that were batch submitted to L1 were different than what the contracts expected, so geth had to fix them before passing along to the OVM. The above PR removes any logic in geth around modifying the transactions, it just passes the data through

Metadata

Fixes

  • Fixes # [Link to Issue]

Contributing Agreement

@tynes tynes requested a review from smartcontracts March 8, 2021 22:14
@@ -23,7 +23,7 @@ contract OVM_SequencerEntrypoint {
/*********
* Enums *
*********/

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These whitespace diffs add noise to code review. We need linter rules that prevent such things from getting into the codebase

@tynes
Copy link
Collaborator Author

tynes commented Mar 8, 2021

I'd expect the integration tests to fail

Copy link
Collaborator

@ben-chain ben-chain left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All the changes here look good to me; it's always been downstream that blocks here, so if you can get this through CI and have the clear upgrade path, I'm comfortable merging.

@tynes
Copy link
Collaborator Author

tynes commented Mar 8, 2021

All the changes here look good to me; it's always been downstream that blocks here, so if you can get this through CI and have the clear upgrade path, I'm comfortable merging.

We will have to run our state diff tests because this will change the state root from the beginning, so checking state roots that match will not work. I pushed https://github.com/ethereum-optimism/contracts/tree/v0.1.4/enums to base these changes off of the contracts that are deployed to production right now

Copy link
Collaborator

@smartcontracts smartcontracts left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR will conflict with #300. If we think that #300 will be included then we can "fix" this issue by getting rid of transaction types in Geth. Wdyt?

@tynes
Copy link
Collaborator Author

tynes commented Mar 9, 2021

We should prioritize #300 altho I am leaning towards the need to leave in the unmodified OVM_SequencerEntrypoint.sol and make a OVM_SequencerEntrypointv2.sol or something like that so we can have test coverage of the existing transactions.

I also think its not a bad idea to add a version() -> string getter to the contracts to make version management easier as contracts are updated on chain, where the string follows semver

@tynes tynes closed this Mar 11, 2021
@maurelian maurelian deleted the fix/enum branch March 11, 2021 16:55
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants