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.

Sign up for GitHub

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

Jump to bottom

refactor(levm): replace specId with Fork #1762

Merged
merged 11 commits into from
Jan 28, 2025
Merged

refactor(levm): replace specId with Fork #1762

merged 11 commits into from
Jan 28, 2025

Conversation

varun-doshi
Copy link
Contributor

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
ilitteri
ilitteri requested changes Jan 21, 2025
View reviewed changes
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
tomip01
tomip01 reviewed Jan 21, 2025
View reviewed changes
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

lima-limon-inc
lima-limon-inc approved these changes Jan 22, 2025
View reviewed changes
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!

ilitteri
ilitteri reviewed Jan 22, 2025
View reviewed changes
crates/vm/vm.rs Outdated Show resolved Hide resolved
ilitteri
ilitteri requested changes Jan 22, 2025
View reviewed changes
Copy link
Contributor

@ilitteri ilitteri 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.

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
fborello-lambda
fborello-lambda requested changes Jan 23, 2025
View reviewed changes
Copy link
Contributor

@fborello-lambda fborello-lambda 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.

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
Contributor 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
Contributor 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 ^_^

@varun-doshi
Copy link
Contributor Author

varun-doshi commented Jan 24, 2025 •
edited
Loading

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 ^_^

Just to confirm, on merging with upstream, it doesn't throw an error because specId is still a valid import. So should I keep specId or replace with Fork?

I'd assume replace with Fork because that's the primary objective of this PR

ilitteri
ilitteri reviewed Jan 24, 2025
View reviewed changes
crates/vm/levm/src/vm.rs Outdated Show resolved Hide resolved
ilitteri
ilitteri reviewed Jan 24, 2025
View reviewed changes
crates/vm/levm/src/vm.rs Outdated Show resolved Hide resolved
@lima-limon-inc
Copy link
Contributor

Hello varun! I just pushed a commit to fix the merge conflicts.

fborello-lambda
fborello-lambda approved these changes Jan 28, 2025
View reviewed changes
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.

Good Job, if the CI passes we are ready to merge!

@ilitteri
Copy link
Contributor

Good Job, if the CI passes we are ready to merge!

Update on this one: we are working on fixing the CI to make it pass for external contributors' PRs

@lima-limon-inc
Copy link
Contributor

Update on this one: we are working on fixing the CI to make it pass for external contributors' PRs

Like Ivan said varun. We are working on fixing the CI. Thank you very much for your contribution and patience.

^_^

Have a good week

@fborello-lambda fborello-lambda added this pull request to the merge queue Jan 28, 2025
Merged via the queue into lambdaclass:main with commit 69b0e30 Jan 28, 2025
23 of 26 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tomip01 tomip01 tomip01 left review comments

@fborello-lambda fborello-lambda fborello-lambda approved these changes

@ilitteri ilitteri ilitteri approved these changes

@lima-limon-inc lima-limon-inc Awaiting requested review from lima-limon-inc

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

LEVM: Replace SpecId with Fork
6 participants
@varun-doshi @lima-limon-inc @ilitteri @tomip01 @fborello-lambda @lferrigno