Skip to content
This repository has been archived by the owner on Feb 26, 2024. It is now read-only.

Fix BYOC/Solc Config Bug #1467

Merged
merged 18 commits into from
Dec 7, 2018
Merged

Fix BYOC/Solc Config Bug #1467

merged 18 commits into from
Dec 7, 2018

Conversation

CruzMolina
Copy link
Contributor

@CruzMolina CruzMolina commented Nov 27, 2018

Bug Fix

As noted in #1452, there is a mismatch between the documentation for BYOC/V5 and the way compilers.solc is handled in the codebase. This would cause user-configured compiler settings like optimizer, runs, and evmVersion to be omitted and use the default Config values instead.

New Expected Behavior

Using a new BYOC/V5 format (example below) will use the solc values specified and result in optimized code.

module.exports = {
  ...
  compilers: {
        solc: {
            version: "0.5.0",
            optimizer: {
                enabled: true,
                runs: 1000
            }
        }
    }
};

This will also work:

module.exports = {
  ...
  compilers: {
        solc: {
            version: "0.5.0",
            settings: {
                optimizer: {
                    enabled: true,
                    runs: 1000
                }
            }
        }
    }
};

The V4 solc format (example below) is also still supported.

module.exports = {
...
solc: {
        //version: "0.5.0", <---- currently not supported on V4 format
        optimizer: {
            enabled: true,
            runs: 1000
        }
    }
};

as well as:

module.exports = {
...
solc: {
        //version: "0.5.0", <---- currently not supported on V4 format
        settings: {
            optimizer: {
                enabled: true,
                runs: 1000
            }
        }
    }
};

@CruzMolina CruzMolina changed the title V5 optimizer Fix BYOC/Solc Config Bug Nov 27, 2018
@CruzMolina
Copy link
Contributor Author

Coverage would be nice...

@gnidan
Copy link
Contributor

gnidan commented Nov 27, 2018

Thoughts on this: Including settings is the correct form, but we can allow the form where optimizer (or other specific keys) are specified without the wrapping settings.

My reasoning is that:

  • optimizer is arguably solc specific: we do not control the data format for the optimizer settings.
  • The object description for "compiler configs", e.g. compilers.solc, should be generic.
  • By defining a generic settings property for all compiler objects, we can maintain conformity across compilers.

Including this because I already wrote it: (maybe it provides clarity?)

compilers: {
  "<whatever-compiler>": {  // this object should be agnostic to language
    version: "...",
    /* anotherTruffleGenericSetting: "..." */
    settings: {  // a pseudo "black box"
      /* compilerSpecificSetting: "..." */
    }
}

@coveralls
Copy link

Coverage Status

Coverage increased (+0.4%) to 67.937% when pulling 9ea9ec0 on v5-optimizer into 49a9e7e on next.

@eggplantzzz
Copy link
Contributor

So I'm understanding that the optimizer settings are going to go under config.compilers[<compiler>].settings. What does index.js.orig do?

@CruzMolina
Copy link
Contributor Author

Ah, thanks for spotting that @eggplantzzz . pushed the wrong file 😆

Copy link
Contributor

@gnidan gnidan left a comment

Choose a reason for hiding this comment

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

Feel free to take or leave my suggestion!

packages/truffle-compile/index.js Outdated Show resolved Hide resolved
Copy link
Contributor

@gnidan gnidan left a comment

Choose a reason for hiding this comment

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

ooh this looks great! nice test improvements

@CruzMolina CruzMolina merged commit 65c2cac into next Dec 7, 2018
@CruzMolina CruzMolina deleted the v5-optimizer branch December 7, 2018 04:45
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants