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: Enable solc optimization #427

Merged
merged 2 commits into from Sep 2, 2021
Merged

fix: Enable solc optimization #427

merged 2 commits into from Sep 2, 2021

Conversation

wolflo
Copy link
Contributor

@wolflo wolflo commented Sep 1, 2021

Motivation

Currently, the solc bindings ignore the optimizer parameter.

Solution

Add the optimization arguments to the executed command.

This also adds some options around using the optimizer, some of which may be a little awakward as implemented. Ideally, in addition to enabling the optimizer, users should be able to:

  1. Compile without the optimizer
  2. Set the optimizer runs
  3. Set optimizer runs to 0 (i.e. we can't use 0 as a hack for turning the optimizer off)

As implemented, the current examples will work as expected (with optimization now).

Solc::new("./contracts/*").optimizer(200).build()?;

However, the optimizer no longer defaults to the optimizer ON with 200 runs (which is what the documentation used to suggest). Also, you now need to know the runs parameter you want to use in order to enable optimization.

Alternatively, we could add a .optimize(bool) method, with the downside just being verbosity. If a breaking change was acceptable, optimizer() could take an Option<usize>.

PR Checklist

  • Added Tests
  • Added Documentation
  • Updated the changelog

@gakonst
Copy link
Owner

gakonst commented Sep 1, 2021

Thanks for the PR! I'd be supportive of as many breaking changes around that as you want. So far so good - do you think you'd want to merge as-is, or do you want to add additional stuff?

@wolflo
Copy link
Contributor Author

wolflo commented Sep 1, 2021

My preference would probably be to have optimize() take an Option<usize>, with None meaning no optimization. That would give you the flexibility to have the common default (--optimize --optimize-runs 200) while still being able to set on / off / runs without chaining another method.

This would break things that previously used .optimize(200), but would restore the "expected" default of 200 runs.

What do you think about also taking arbitrary string args the way ganache does?

/// Adds an argument to pass to the `ganache-cli`.
pub fn arg<T: Into<String>>(mut self, arg: T) -> Self {
self.args.push(arg.into());
self
}

@gakonst
Copy link
Owner

gakonst commented Sep 1, 2021

Both Option and arbitrary string args SGTM!

Change optimizer() method to accept an Option<usize> (breaking).
Add args() option to pass arbitrary arguments to solc command.
@wolflo
Copy link
Contributor Author

wolflo commented Sep 2, 2021

The breaking change here is that optimizer() now takes an Option<usize> instead of usize, and optimizer(None) turns the optimizer off.

The other addition is .arg() and .args() which take raw argument strings (or an iterator of raw argument strings) and passes them to the solc command. This is going to have some rough edges because of the strict parsing of the output. Anything that changes the structure of the output by adding an additional field to the combined json (e.g. --asm) will result in

Error: trailing characters at line 3 column 1

Probably the output parsing could be made more general, but at some point it's easier to just build the command and handle the output yourself.

// No optimization
let contracts = Solc::new("./contracts/*")
    .optimizer(None)
    .build()?;

// Some(200) is default, optimizer on with 200 runs
// .arg() allows passing arbitrary args to solc command
let optimized_contracts = Solc::new("./contracts/*")
    .optimizer(Some(200))
    .arg("--metadata-hash=none")
    .build()?;

Tests

This could use some, but I'm not sure the most robust way to test Solc given that it's all reliant on the locally installed version.

@gakonst gakonst merged commit 664ccfe into gakonst:master Sep 2, 2021
gakonst added a commit that referenced this pull request Sep 2, 2021
gakonst added a commit that referenced this pull request Sep 3, 2021
meetmangukiya pushed a commit to meetmangukiya/ethers-rs that referenced this pull request Mar 21, 2022
there's an issue with Address::zero() being used as a default in clap

❯ forge test
error: Invalid value for '--tx-origin <TX_ORIGIN>':
       Invalid character '…' at position 4
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.

None yet

2 participants