Zondax / filecoin-solidity

Filecoin Solidity API Library
Apache License 2.0
93 stars 43 forks source link

foundry.toml optimizer=false #295

Open snissn opened 1 year ago

snissn commented 1 year ago

are we ok with optimizer=false?

I think the external/BigNumbers.sol is the blocker on changing this to true:

with optimizer=true i see the message when i run forge build:

mikeseiler@Mikes-MacBook-Air filecoin-solidity % forge build
[⠢] Compiling...
[⠰] Compiling 52 files with 0.8.17
[⠔] Solc 0.8.17 finished in 80.03ms
Error:
Compiler run failed
error[6553]: SyntaxError: The msize instruction cannot be used when the Yul optimizer is activated because it can change its semantics. Either disable the Yul optimizer or do not use the instruction.
   --> contracts/v0.8/external/BigNumbers.sol:903:9:
    |
903 |         assembly {
    |         ^ (Relevant source part starts here and spans across multiple lines).

error[6553]: SyntaxError: The msize instruction cannot be used when the Yul optimizer is activated because it can change its semantics. Either disable the Yul optimizer or do not use the instruction.
   --> contracts/v0.8/external/BigNumbers.sol:964:9:
    |
964 |         assembly {
    |         ^ (Relevant source part starts here and spans across multiple lines).

error[6553]: SyntaxError: The msize instruction cannot be used when the Yul optimizer is activated because it can change its semantics. Either disable the Yul optimizer or do not use the instruction.
    --> contracts/v0.8/external/BigNumbers.sol:1059:9:
     |
1059 |         assembly {
     |         ^ (Relevant source part starts here and spans across multiple lines).

:link: zboto Link

emmanuelm41 commented 1 year ago

BigNumbers is not a library that we actually need on the API. It is being used internally for mocks contracts only. However, any operation devs want to make over BigInt will require to use this library, so I guess devs will import it at the end. :/

maciejwitowski commented 1 year ago

By forcing optimizer=false we are changing the defaults. It is fine as long as we have a very good reason to do it. Let's explore if it's absolutely needed though. Otherwise there's a chance of it failing in for random tools (like foundry in this case) in the future.

emmanuelm41 commented 1 year ago

I don't have a solid answer here @maciejwitowski. BugNumbers is likely to be used by devs to handle bignum operations like mul or div. There is no other lib out there for that :/

maciejwitowski commented 1 year ago

But we are using it only in tests, right? Isn't there any workaround not to use it at all so we don't enforce this optimisation flag? I'm not sure what other devs have to do with it 😕 Do you mean that the optimisations won't be applied anyway when they use BigInts in their contracts?

Stebalien commented 1 year ago

So, I'm a solidity noob, but...

Shouldn't:

https://github.com/zondax/solidity-bignumber/blob/39dbb9e2dd186a549c9ad7eb1b4b204dc6705245/src/BigNumbers.sol#L880

Be let freemem = mload(0x40)?

Schwartz10 commented 1 year ago

Hey @emmanuelm41 @maciejwitowski would love to figure out a solution to this... Our codebase can't rely safely on the filecoin-solidity library because some of the API calls we make to the precompile require BigInt, and to make use of it, we then end up using BigNumber. Any library that relies on this code then can't be optimized, so we have to use more dangerous workaround