Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(optimism): Reuse L1BlockInfoTx from Maili #1

Closed
wants to merge 1 commit into from
Closed

Conversation

refcell
Copy link

@refcell refcell commented Jan 9, 2025

Description

Kona as well as other libraries + applications use optimism types from maili. maili-protocol exposes the L1BlockInfoTx type that provides all the functionality used in revm's custom L1BlockInfo type.

This PR removes the redundant L1BlockInfo type from revm, using maili_protocol::L1BlockInfoTx instead.

@refcell refcell self-assigned this Jan 9, 2025
@refcell refcell requested review from clabby and emhane January 9, 2025 23:02
Copy link
Member

@emhane emhane left a comment

Choose a reason for hiding this comment

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

looking good! I would leave the tests though, can never have too many tests. easier to upstream that way as well. do they not work out of the box with replacing the type?

@refcell
Copy link
Author

refcell commented Jan 10, 2025

I believe the tests mostly cover the tx l1 cost functions which we should upstream to this pr since the cost functions are there.

@emhane
Copy link
Member

emhane commented Jan 10, 2025

I believe the tests mostly cover the tx l1 cost functions which we should upstream to this pr since the cost functions are there.

could also be done as 2 steps though to make it faster for dragan to review securely if he recognises his tests

crates/optimism/Cargo.toml Outdated Show resolved Hide resolved
@refcell
Copy link
Author

refcell commented Jan 11, 2025

This should be good to go now except for the git dep - i've added back the tests.

@refcell refcell requested a review from emhane January 11, 2025 01:01
@refcell
Copy link
Author

refcell commented Jan 13, 2025

Opened into upstream revm here

@refcell refcell closed this Jan 13, 2025
@emhane emhane added the upstream A pr solving this issue has been opened in upstream label Jan 13, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
upstream A pr solving this issue has been opened in upstream
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants