Skip to content

Ensure that block execution maintains L1 compatibility with optimism feature enabled#39

Merged
refcell merged 2 commits intoclabby/op-rethfrom
brianbland/op-executor-l1-compat
Aug 3, 2023
Merged

Ensure that block execution maintains L1 compatibility with optimism feature enabled#39
refcell merged 2 commits intoclabby/op-rethfrom
brianbland/op-executor-l1-compat

Conversation

@BrianBland
Copy link
Collaborator

@BrianBland BrianBland commented Aug 1, 2023

  • Only fetch L1 block info and compute l1 cost when the chain spec includes an Optimism config
  • Only transfer L1 Cost and Base Fee to the respective vaults when the chain spec includes an Optimism config

Addresses the following tests (#31):

reth-revm executor::tests::sanity_execution
reth-revm executor::tests::test_selfdestruct

Follow-up: Add new test cases which cover the cases where the chain has Optimism enabled

Copy link

@merklefruit merklefruit left a comment

Choose a reason for hiding this comment

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

minor comments, overall lgtm, feel free to add tests here or in a separate PR if you prefer

Choose a reason for hiding this comment

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

why is this needed? where are we cloning it? Not a big deal anyway, but just want to make sure it's needed

Copy link
Collaborator Author

@BrianBland BrianBland Aug 2, 2023

Choose a reason for hiding this comment

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

Now that l1_block_info is an Optional type, the inner L1BlockInfo must be either moved or copied when it's accessed in each loop iteration.

Actually it made more sense to simply borrow the value in this context to avoid this copy

Choose a reason for hiding this comment

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

Awesome!

@merklefruit merklefruit mentioned this pull request Aug 2, 2023
11 tasks
@BrianBland BrianBland force-pushed the brianbland/op-executor-l1-compat branch from 48b0098 to 861c2f5 Compare August 2, 2023 23:54
@BrianBland BrianBland marked this pull request as ready for review August 2, 2023 23:58
@BrianBland BrianBland requested a review from merklefruit August 3, 2023 03:30
Copy link

@merklefruit merklefruit left a comment

Choose a reason for hiding this comment

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

LGTM!

@refcell refcell merged commit 697caae into clabby/op-reth Aug 3, 2023
@emhane emhane deleted the brianbland/op-executor-l1-compat branch September 25, 2025 09:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants