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

Solidity Optimizer options #360

Merged
merged 3 commits into from
Jun 13, 2023
Merged

Conversation

moltam89
Copy link
Contributor

@moltam89 moltam89 commented Jun 1, 2023

Description

Hey guys,

This PR is about the Solidity compiler's Optimizer.

It is enabled by default in the original scaffold-eth repo.
I wasn't sure if I should just enable it for scaffold-eth-2 as well, or not. I decided to only add it as an option and update the readme, to let people know about this, before they deploy to a Live network.

Let me know what do you think.

Best,
Tamas

Additional Information

@technophile-04 technophile-04 self-requested a review June 7, 2023 06:41
@technophile-04
Copy link
Collaborator

Tysm @moltam89 🙌

After doing some research here's my opinion :

TLDR;

We should set optimizer to enabled : true

So for recent versions of Solidity, we would recommend to always use the optimizer unless you really do not care about gas costs.
~ Official solidity lang QNA 2020


Short Desc on what optimizer does :

  • First optimizer is not hardhat feature (lol I thought previously) but it comes with solidity compiler itself.Deployment costs and function call costs are two areas where the compiler optimizer impacts smart contracts gas.
  • There are several steps that the optimizer performs. A simple example would be evaluating expressions whose value is known at runtime, e.g., x + 0 is evaluated to x, where x may be a parameter known only during runtime. For more desc checkout solidity docs secion

For runs parameter checkout this answer

README.md Outdated Show resolved Hide resolved
@moltam89
Copy link
Contributor Author

Hey @technophile-04 ,

Yeah, I think you're right and we should enable the Optimiser by default.

I've updated and rebased the PR.

Cheers,
Tamas

Copy link
Collaborator

@technophile-04 technophile-04 left a comment

Choose a reason for hiding this comment

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

Tysm @moltam89 🙌, cc @carletex , @sverps , @damianmarti for final thoughts before we merge this, here is the TLDR; -> #360 (comment)

@carletex
Copy link
Member

I'm up for enabling it by default!

A nitpick:

// By default, the Solidity Optimizer is enabled
// https://docs.soliditylang.org/en/latest/internals/optimizer.html#benefits-of-optimizing-solidity-code
// https://docs.soliditylang.org/en/latest/using-the-compiler.html#optimizer-options
// https://hardhat.org/hardhat-runner/docs/config

Isn't this too much? haha

// By default, the Solidity Optimizer is enabled This seems redundant (easily inferred from optimizer: { enabled: true} :D). And maybe we don't need 3 links? I feel that the important part is the "runs" parameter, so some helper text with a link could help there (move the https://docs.soliditylang.org/en/latest/using-the-compiler.html#optimizer-options link right above "runs"?)

Anyway, everything looks good, just suggesting some tiny tweaks.

Thanks!

@scaffold-eth scaffold-eth deleted a comment from XOder64x Jun 13, 2023
@technophile-04
Copy link
Collaborator

// By default, the Solidity Optimizer is enabled This seems redundant (easily inferred from optimizer: { enabled: true} :D). And maybe we don't need 3 links? I feel that the important part is the "runs" parameter, so some helper text with a link could help there (move the https://docs.soliditylang.org/en/latest/using-the-compiler.html#optimizer-options link right above "runs"?)

Yes yes, I agree with this

Tysm @moltam89 just pushed the changes Carlos suggested at aef0a99

Also tested that deployment and verification work correctly 🙌

https://sepolia.etherscan.io/address/0x786b4Dff292876aD3aDE9CCD534373AEa1Ad8eD7#code

@technophile-04 technophile-04 merged commit 3780407 into scaffold-eth:main Jun 13, 2023
1 check passed
@moltam89
Copy link
Contributor Author

Thank you guys!

kmjones1979 added a commit that referenced this pull request Jul 3, 2023
* Update SE-2 nomenclautre  (#317)

* Fix autoConnect when contract not deployed (#292)

* oxford comma lol

* Fix icon visibility on dark mode in EtherInput (#320)

* Update README: Add Vercel ENV var to disable type/lint checking (#327)

* Fix typo (#334)

Fix typo at code snippet

* README: Updated SE-2 custom hook section (#330)

Co-authored-by: KcPele <fidekg123@gmail.com>
Co-authored-by: Shiv Bhonde <shivbhonde04@gmail.com>
Co-authored-by: Carlos Sánchez <oceanrdn@gmail.com>

* Remove duplicated HINT on README (#335)

* PR and Issue templates (#329)

* Native currency symbol and price as per target network (#322)

* useAppStore => useGlobalState (#338)

* Event history filter type (#333)

* fix: event history filter type

Fixes #332

* fix: make scaffold event history config separate type

* fix: prettier broke due to invalid syntax

* fix: ts error after generating contract

* Indexed prop + example

---------

Co-authored-by: Carlos Sánchez <oceanrdn@gmail.com>

* Update README: custom hooks section (#339)

* fix: assert type as Abi to avoid ts error (#350)

* fix: assert type as Abi to avoid ts error

Fixes #344

* assert type as Abi to avoid ts error on useScaffoldContract

---------

Co-authored-by: Carlos Sánchez <oceanrdn@gmail.com>

* Fix useEffect filter dep on event hook (#349)

* Block confirmation options / callback for useScaffoldContractWrite (#348)

* Patch useLocalStorage setting the right initial value to avoid a hydration error (#356)

Co-authored-by: sverps <sverps@gmail.com>

* Save selected contract to local storage (#326)

* Save selected contract to local storage

* refactor: Replaced native localStorage with useLocalStorage from usehooks-ts

* Fixed empty string parameter error.

* Updated parameter requirements to fix issue.

* Fixed error.

* Fixed parameter assignment error.

* Use useLocalStorage to save selected contract

---------

Co-authored-by: Damian <damianmarti@gmail.com>

* Refactor meta tags into separate component and add default thumbnail / image (#359)

* Hotfix: thumbnail.png => thumbnail.jpg

* Local Block Explorer (#351)

Co-authored-by: Shiv Bhonde <shivbhonde04@gmail.com>

* Fix daisyUI dropdown on IOS (#342)

* allow `overrides` in useScaffoldContractWrite (#345)

* allow overrides for useScaffoldContractWrite

* spread overrides and restConfig separately

* feat: proactively disable read hook when it would result in error (#250)

Fixes #249

Co-authored-by: Shiv Bhonde | shivbhonde.eth <shivbhonde04@gmail.com>

* Size prop on Address component (#365)


Co-authored-by: Carlos Sánchez <oceanrdn@gmail.com>

* Generate TS ABIs from deploy script (#368)

* enable solidity optimizer by default (#360)

* fix(Balance): font-size of symbol should be relative to current font-size, not document root (#375)

* fix(contract.ts): very long abi's now no longer go over type instantiation limit (#377)

Fixes #374

* fix(contract.ts): improve missing config check (#372)

Fixes #371

* fix listner types error when contracts missing (#379)

* zkSyncEra Testnet config (#383)

* Upgrade hardhat-deploy to 0.11.26 where zkSync is supported

* Dependency for @matterlabs/hardhat-zksync-solc

* Add zkSyncEraTestnet to networks

* Import @matterlabs/hardhat-zksync-solc

* Add artifacts-zk and cache-zk to gitignore

* yarn install

* Add zkSync mainnet config

* add hardhat-zksync-verify for contract verification

---------

Co-authored-by: moltam89 <moltam89@gmail.com>
Co-authored-by: Damian <damianmarti@gmail.com>
Co-authored-by: Shiv Bhonde <shivbhonde04@gmail.com>

* allow writeAsync by useScaffoldContractWrite to be called with updated args (#385)

* update wagmi & rainbow for walletConnectV2 (#381)

* meged changes from main, additional configuration in subgraph.yaml

---------

Co-authored-by: Shiv Bhonde | shivbhonde.eth <shivbhonde04@gmail.com>
Co-authored-by: Austin Griffith <austin@ethereum.org>
Co-authored-by: Kevin Joshi <kevinjoshi46b@gmail.com>
Co-authored-by: Carlos Sánchez <oceanrdn@gmail.com>
Co-authored-by: AlehN <natsevsky@gmail.com>
Co-authored-by: Pablo Alayeto <55535804+Pabl0cks@users.noreply.github.com>
Co-authored-by: KcPele <fidekg123@gmail.com>
Co-authored-by: Samuel | solidixen.eth <sverps@gmail.com>
Co-authored-by: Eda Akturk <edakturk96@gmail.com>
Co-authored-by: Tamas Molnar <tamas.molnar@liferay.com>
Co-authored-by: Damian Martinelli <damianmarti@gmail.com>
Co-authored-by: Alexander <a00112699@gmail.com>
Co-authored-by: port <108868128+portdeveloper@users.noreply.github.com>
Co-authored-by: moltam89 <moltam89@gmail.com>
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

3 participants