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 breaks on skip when compiled with viaIR #3312

Closed
2 tasks done
CodeSandwich opened this issue Sep 22, 2022 · 3 comments
Closed
2 tasks done

Solidity breaks on skip when compiled with viaIR #3312

CodeSandwich opened this issue Sep 22, 2022 · 3 comments
Labels
T-bug Type: bug

Comments

@CodeSandwich
Copy link
Contributor

CodeSandwich commented Sep 22, 2022

Component

Forge

Have you ensured that all of these are up to date?

  • Foundry
  • Foundryup

What version of Foundry are you on?

forge 0.2.0 (3a462eb 2022-09-22T00:05:16.281308046Z)

What command(s) is the bug in?

forge test

Operating System

Linux

Describe the bug

I have a trivial test:

import {Test} from "forge-std/Test.sol";

contract SkipTest is Test {
    function testSkip() public {
        uint256 value = block.timestamp;
        emit log_uint(value);
        skip(1);
        emit log_uint(value);
    }
}

When compiling with via_ir = true, the output is:

[PASS] testSkip() (gas: 5134)
Logs:
  1
  2

Solidity seems to make an assumption that block.timestamp never changes during a call, so it doesn't make sense to actually store it in a variable. It makes perfect sense to optimize away the stack storage of a constant, but it breaks test EVMs with changeable timestamps.

I'm not sure if this is solveable in Foundry, it may be a Solidity issue. OTOH it's overall a good optimization, it breaks only with Foundry's test framework architecture, why should every DeFi user in the world pay gas for that?

@CodeSandwich CodeSandwich added the T-bug Type: bug label Sep 22, 2022
@gakonst gakonst added this to Foundry Sep 22, 2022
@gakonst gakonst moved this to Todo in Foundry Sep 22, 2022
@CodeSandwich
Copy link
Contributor Author

CodeSandwich commented Sep 22, 2022

A workaround I've found for this is to force the compiler to perform a seemingly non-no-op operation, e.g.

uint256 value = uint248(block.timestamp);

But NOT trivial things like + 1 - 1, the compiler is too smart for that. But I guess that it'll become harder in the future.

@mds1
Copy link
Collaborator

mds1 commented Sep 22, 2022

See #1373 for more info + potential solutions, glad you found another workaround

@CodeSandwich
Copy link
Contributor Author

Thank you! Closing it then as a duplicate.

Repository owner moved this from Todo to Done in Foundry Sep 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-bug Type: bug
Projects
Archived in project
Development

No branches or pull requests

2 participants