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(levm): replace specId with Fork #1762

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

varun-doshi
Copy link

Motivation

Ref #1741

Description

Replaces specId with Fork

  • First change is in file crates/vm/levm/src/environment.rs
    • Changes in other relevant files to accept the changes and fix cascading errors

Closes #1741

@varun-doshi varun-doshi requested a review from a team as a code owner January 21, 2025 06:40
Copy link
Contributor

@ilitteri ilitteri left a comment

Choose a reason for hiding this comment

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

The PR looks good. It is aligned with what I had in mind. Left a couple of comments, suggestions and requested some changes.

crates/common/types/genesis.rs Outdated Show resolved Hide resolved
crates/common/types/genesis.rs Show resolved Hide resolved
crates/common/types/genesis.rs Show resolved Hide resolved
crates/vm/levm/src/environment.rs Outdated Show resolved Hide resolved
crates/vm/levm/src/gas_cost.rs Outdated Show resolved Hide resolved
crates/vm/levm/src/gas_cost.rs Outdated Show resolved Hide resolved
crates/vm/levm/src/precompiles.rs Outdated Show resolved Hide resolved
crates/vm/vm.rs Outdated Show resolved Hide resolved
crates/vm/vm.rs Show resolved Hide resolved
@fborello-lambda fborello-lambda changed the title feat: replace specId with Fork feat(levm): replace specId with Fork Jan 21, 2025
@fborello-lambda fborello-lambda changed the title feat(levm): replace specId with Fork refactor(levm): replace specId with Fork Jan 21, 2025
Copy link
Contributor

@tomip01 tomip01 left a comment

Choose a reason for hiding this comment

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

You should remove unused imports

Copy link
Contributor

@lima-limon-inc lima-limon-inc left a comment

Choose a reason for hiding this comment

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

LGTM!

fork_summary_for_slack(reports, Fork::Byzantium),
fork_summary_for_slack(reports, Fork::Berlin),
fork_summary_for_slack(reports, Fork::Constantinople),
fork_summary_for_slack(reports, Fork::Paris),
Copy link
Contributor

Choose a reason for hiding this comment

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

Move this between Shanghai and Homestead (it is an error for Paris to be this low in the list)

fork_summary_for_github(reports, Fork::Byzantium),
fork_summary_for_github(reports, Fork::Berlin),
fork_summary_for_github(reports, Fork::Constantinople),
fork_summary_for_github(reports, Fork::Paris),
Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto.

writeln!(f, "{}", fork_summary_shell(&self.0, Fork::Byzantium))?;
writeln!(f, "{}", fork_summary_shell(&self.0, Fork::Berlin))?;
writeln!(f, "{}", fork_summary_shell(&self.0, Fork::Constantinople))?;
writeln!(f, "{}", fork_summary_shell(&self.0, Fork::Paris))?;
Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto.

"Shanghai" => Fork::Shanghai,
"Cancun" => Fork::Cancun,
"Prague" => Fork::Prague,
_ => Fork::Cancun,
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not correct. But implementing TryFrom<&str> for Fork is not that worth. Let's roll back this change.

crates/vm/vm.rs Outdated
@@ -220,12 +220,12 @@ cfg_if::cfg_if! {
});
let mut block_cache: CacheDB = HashMap::new();
let block_header = &block.header;
let spec_id = spec_id(&state.chain_config()?, block_header.timestamp);
let fork=state.chain_config()?.fork(block_header.timestamp);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
let fork=state.chain_config()?.fork(block_header.timestamp);
let fork = state.chain_config()?.fork(block_header.timestamp);

Copy link
Contributor

@ilitteri ilitteri left a comment

Choose a reason for hiding this comment

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

After addressing this final comments and solving the conflicts with main, we'll be ready to merge

@varun-doshi varun-doshi requested a review from ilitteri January 23, 2025 10:52
Copy link
Contributor

@fborello-lambda fborello-lambda left a comment

Choose a reason for hiding this comment

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

Looks good 👍 , just some comments, would it be possible to integrate this changes with the latest changes in main?
For example this line

Also, based on the CI, a cargo fmt is needed.

@varun-doshi
Copy link
Author

Looks good 👍 , just some comments, would it be possible to integrate this changes with the latest changes in main? For example this line

Also, based on the CI, a cargo fmt is needed.

I'll get this done

@lima-limon-inc
Copy link
Contributor

Hi @varun-doshi !

Just a heads up, this PR (#1782) got recently merged. It did use some the SpecID type to implement some of its logic.

Thank you very much for your help ^_^

@varun-doshi
Copy link
Author

Hi @varun-doshi !

Just a heads up, this PR (#1782) got recently merged. It did use some the SpecID type to implement some of its logic.

Thank you very much for your help ^_^

Should I switch that to Fork?

@lima-limon-inc
Copy link
Contributor

Should I switch that to Fork?

Hi! I don't believe there's need for that. As soon as you update your branch with main, those changes will get pulled and you'll get compilation errors.

Does that help? Don't doubt ask!

Thanks in advance ^_^

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.

LEVM: Replace SpecId with Fork
5 participants