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

fix(config): enable optimizer when optimizer_runs set in config #9673

Merged
merged 7 commits into from
Jan 13, 2025

Conversation

yash-atreya
Copy link
Member

@yash-atreya yash-atreya commented Jan 13, 2025

Motivation

Closes #9665

Solution

  • Change the optimizer and optimizer_runs keys to Option<bool> and Option<usize> respectively
  • Detect whether the optimizer_runs key has been set in the config and is nonzero. If yes, then set config.optimizer = true.
  • If optimizer = true but optimizer_runs has not been set. optimizers_runs will default to 200.

crates/config/src/lib.rs Outdated Show resolved Hide resolved
@yash-atreya yash-atreya added T-bug Type: bug C-forge Command: forge T-feature Type: feature and removed T-bug Type: bug labels Jan 13, 2025
@yash-atreya yash-atreya changed the title fix(config): enable optimizer when optimizer_runs set in config fix(config): enable optimizer when optimizer_runs set in config Jan 13, 2025
yash-atreya and others added 3 commits January 13, 2025 18:35
Co-authored-by: DaniPopes <[email protected]>
Copy link
Member

@zerosnacks zerosnacks left a comment

Choose a reason for hiding this comment

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

Nice, lgtm!

Copy link
Collaborator

@grandizzy grandizzy left a comment

Choose a reason for hiding this comment

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

lgtm! made some tests on sepolia base with deploying with different optimizer and optimizer_config settings and they behave properly

@yash-atreya yash-atreya merged commit 017c59d into master Jan 13, 2025
22 checks passed
@yash-atreya yash-atreya deleted the yash/fix-9665 branch January 13, 2025 14:55
@elopez
Copy link

elopez commented Jan 14, 2025

Hi folks @yash-atreya @zerosnacks @grandizzy, it looks like the change of optimizer to an Option<bool> also changed the format of on-disk hardhat-style build-info json artifacts, which affects external tooling (e.g. crytic/crytic-compile#581). Was that change intentional?

@grandizzy
Copy link
Collaborator

Hi folks @yash-atreya @zerosnacks @grandizzy, it looks like the change of optimizer to an Option<bool> also changed the format of on-disk hardhat-style build-info json artifacts, which affects external tooling (e.g. crytic/crytic-compile#581). Was that change intentional?

hi @elopez the change was intentional but we weren't aware of this side effect, will be looking into. Meanwhile you could just pin foundry to stable which doesn't contain the breaking changes (see announcement on top of release notes https://github.com/foundry-rs/foundry/releases/tag/stable)

Copy link

@Missjones2829 Missjones2829 left a comment

Choose a reason for hiding this comment

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

Merge

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-forge Command: forge T-feature Type: feature
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

Restore solc optimizer when optimizer_runs is nonzero
6 participants