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

refactor: move evm-spec-id to config #5786

Merged
merged 4 commits into from
Sep 6, 2023

Conversation

mattsse
Copy link
Member

@mattsse mattsse commented Sep 5, 2023

Motivation

prep for #5782

this makes it easier to access the spec id required for the evm from the config

Solution

@mattsse mattsse added the T-debt Type: code debt label Sep 5, 2023
Copy link
Member

@Evalir Evalir left a comment

Choose a reason for hiding this comment

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

makes sense, i like this! we currently reason about the spec id as an evm—related util when it's an important part of the config. just adding a few commits to inline & remove the utility on the evm crate

crates/config/src/lib.rs Show resolved Hide resolved
crates/config/src/lib.rs Show resolved Hide resolved
@Evalir Evalir force-pushed the matt/move-evm-spec-to-config branch from 4d5a77f to f9d1720 Compare September 6, 2023 02:06
Copy link
Member

@Evalir Evalir left a comment

Choose a reason for hiding this comment

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

made some changes—feel free to rollback if you prefer to have both!

@@ -112,24 +111,6 @@ pub fn halt_to_instruction_result(halt: Halt) -> InstructionResult {
}
}

/// Converts an `EvmVersion` into a `SpecId`.
Copy link
Member

Choose a reason for hiding this comment

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

removed this as this was duped from this pr onwards

@@ -255,6 +259,24 @@ where
Ok(num)
}

/// Returns the [SpecId] derived from [EvmVersion]
#[inline]
pub fn evm_spec_id(evm_version: &EvmVersion) -> SpecId {
Copy link
Member

Choose a reason for hiding this comment

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

wrapped this in a simple util function, and we use this on config

@mattsse
Copy link
Member Author

mattsse commented Sep 6, 2023

gg

@mattsse mattsse merged commit 1ac45a5 into foundry-rs:master Sep 6, 2023
mikelodder7 pushed a commit to LIT-Protocol/foundry that referenced this pull request Sep 12, 2023
* refactor: move evm-spec-id to config

* chore: make util general and wrap it on config

* chore: remove duped util on evm crate

* chore: fix fixtures

---------

Co-authored-by: Enrique Ortiz <hi@enriqueortiz.dev>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-debt Type: code debt
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants