Skip to content

execution: make ConfigureEvm independent of chainspec argument#10748

Merged
klkvr merged 13 commits intoparadigmxyz:mainfrom
tcoratger:chain_spec
Sep 11, 2024
Merged

execution: make ConfigureEvm independent of chainspec argument#10748
klkvr merged 13 commits intoparadigmxyz:mainfrom
tcoratger:chain_spec

Conversation

@tcoratger
Copy link
Contributor

@tcoratger tcoratger commented Sep 6, 2024

Should close #10741

mattsse
mattsse requested changes Sep 6, 2024
View reviewed changes
Copy link
Collaborator

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

wow that was fast, tysm as always

ptal @klkvr

crates/ethereum/evm/src/lib.rs Outdated
#[non_exhaustive]
pub struct EthEvmConfig;
pub struct EthEvmConfig {
chain_spec: ChainSpec,
Copy link
Collaborator

@mattsse mattsse Sep 6, 2024

Choose a reason for hiding this comment

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

and this should likely be Arc

@mattsse mattsse added C-enhancement New feature or request M-changelog This change should be included in the changelog labels Sep 6, 2024
@tcoratger @mattsse
Update crates/ethereum/evm/src/lib.rs
2f5f42c 
Co-authored-by: Matthias Seitz <matthias.seitz@outlook.de>
@tcoratger
Copy link
Contributor Author

tcoratger commented Sep 6, 2024

I think it conflicts with #10752 cc @klkvr.

Let me know if we should continue here on on the other one

@mattsse
Copy link
Collaborator

mattsse commented Sep 6, 2024

sorry guys, this was a comms issue on my end...

I leave it up to @klkvr how to proceed since both prs are now very very similar

klkvr
klkvr reviewed Sep 6, 2024
View reviewed changes
crates/optimism/evm/src/lib.rs Outdated

/// Optimism-related EVM configuration.
#[derive(Debug, Default, Clone, Copy)]
#[derive(Debug, Default, Clone)]
Copy link
Member

@klkvr klkvr Sep 6, 2024

Choose a reason for hiding this comment

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

this shouldn't have Default too, it causes failing tests

@mattsse I'm wondering if ChainSpec should have Default at all? feels like ChainSpec::empty() would be more explicit and safer

Copy link
Contributor Author

@tcoratger tcoratger Sep 6, 2024

Choose a reason for hiding this comment

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

@mattsse Should we open an issue about this?

@tcoratger
Copy link
Contributor Author

tcoratger commented Sep 6, 2024

sorry guys, this was a comms issue on my end...

I leave it up to @klkvr how to proceed since both prs are now very very similar

No problem, I've learned doing this so no problem at all.

@klkvr Let me know if you want to take it over or to move on here :) Maybe if we want to do something like ChainSpec::empty() we can do this in a prep PR to avoid mixing up things here wdyt?

@klkvr
Copy link
Member

klkvr commented Sep 6, 2024

@tcoratger there are some failing tests due to removed Default impls, I've addressed some of them in #10752 already, feel free to continue working on top of it or just use it as reference

@klkvr
Copy link
Member

klkvr commented Sep 6, 2024

@klkvr Let me know if you want to take it over or to move on here :) Maybe if we want to do something like ChainSpec::empty() we can do this in a prep PR to avoid mixing up things here wdyt?

yep sure, no need to do this here

@tcoratger
Copy link
Contributor Author

tcoratger commented Sep 6, 2024

@tcoratger there are some failing tests due to removed Default impls, I've addressed some of them in #10752 already, feel free to continue working on top of it or just use it as reference

Ok let me check that here taking your impl as ref

@klkvr klkvr mentioned this pull request Sep 6, 2024
Closed
@tcoratger tcoratger requested a review from onbjerg as a code owner September 6, 2024 17:08
tcoratger
tcoratger commented Sep 6, 2024
View reviewed changes
@tcoratger tcoratger mentioned this pull request Sep 7, 2024
Closed
klkvr
klkvr requested changes Sep 9, 2024
View reviewed changes
tcoratger and others added 2 commits September 9, 2024 19:42
@tcoratger
fix comments
f098512
@tcoratger @klkvr
Update crates/optimism/evm/src/lib.rs
0fc1b0f 
Co-authored-by: Arsenii Kulikov <klkvrr@gmail.com>
tcoratger and others added 5 commits September 9, 2024 10:43
@tcoratger @klkvr
Update crates/rpc/rpc/src/eth/core.rs
7950678 
Co-authored-by: Arsenii Kulikov <klkvrr@gmail.com>
@tcoratger @klkvr
Update crates/rpc/rpc/src/eth/helpers/state.rs
a80193e 
Co-authored-by: Arsenii Kulikov <klkvrr@gmail.com>
@tcoratger @klkvr
Update crates/rpc/rpc/src/eth/helpers/transaction.rs
9d801d1 
Co-authored-by: Arsenii Kulikov <klkvrr@gmail.com>
@tcoratger
fix conflicts related to comments
3314e91
@tcoratger
fix OptimismPayloadBuilder
aa0ae0e
@mattsse mattsse requested a review from klkvr September 11, 2024 12:32
mattsse
mattsse approved these changes Sep 11, 2024
View reviewed changes
Copy link
Collaborator

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

thanks,

this seems good to me
pending @klkvr

klkvr
klkvr approved these changes Sep 11, 2024
View reviewed changes
Copy link
Member

@klkvr klkvr left a comment •
edited
Loading

Choose a reason for hiding this comment

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

lgtm

pending conflicts

@tcoratger
Copy link
Contributor Author

tcoratger commented Sep 11, 2024

lgtm

pending conflicts

@klkvr fixed

@klkvr klkvr added this pull request to the merge queue Sep 11, 2024
Merged via the queue into paradigmxyz:main with commit 2b75415 Sep 11, 2024
@klkvr klkvr mentioned this pull request Sep 20, 2024
Merged
theochap pushed a commit to ethereum-optimism/optimism that referenced this pull request Jan 22, 2026
@tcoratger @mattsse @klkvr
execution: make ConfigureEvm independent of chainspec argument (par…
8b0c1af 
…adigmxyz/reth#10748)

Co-authored-by: Matthias Seitz <matthias.seitz@outlook.de>
Co-authored-by: Arsenii Kulikov <klkvrr@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mattsse mattsse mattsse approved these changes

@klkvr klkvr klkvr approved these changes

@rakita rakita Awaiting requested review from rakita

@joshieDo joshieDo Awaiting requested review from joshieDo joshieDo is a code owner

@shekhirin shekhirin Awaiting requested review from shekhirin shekhirin is a code owner

@Rjected Rjected Awaiting requested review from Rjected Rjected is a code owner

@emhane emhane Awaiting requested review from emhane

@fgimenez fgimenez Awaiting requested review from fgimenez

@rkrasiuk rkrasiuk Awaiting requested review from rkrasiuk

@gakonst gakonst Awaiting requested review from gakonst gakonst is a code owner

@onbjerg onbjerg Awaiting requested review from onbjerg

Assignees

No one assigned

Labels

C-enhancement New feature or request M-changelog This change should be included in the changelog

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Make ConfigureEvm independent of chainspec argument

3 participants

@tcoratger @mattsse @klkvr