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

bug(forge test): --gas-report does reporting certain contracts (assembled bytecode in constructor) #6129

Closed
2 tasks done
0xth0mas opened this issue Oct 26, 2023 · 10 comments · Fixed by #9232
Closed
2 tasks done
Assignees
Labels
A-compatibility Area: compatibility A-gas-snapshots Area: gas snapshotting/reporting C-forge Command: forge Cmd-forge-test Command: forge test P-low Priority: low T-bug Type: bug

Comments

@0xth0mas
Copy link

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 (a839414 2023-10-26T09:23:09.940495000Z)

What command(s) is the bug in?

forge test --gas-report

Operating System

Windows

Describe the bug

I am working on proof of concept custom bytecode contracts where all of the functions are stubbed out so that the contract can be compiled/verified on block explorers with a generated ABI but the runtime bytecode is assembled in the constructor based on constructor arguments.

Tests run successfully and report gas total gas consumption for the tests but when attempting to run the gas report there is no report generated for the contract.

I created an example project using the default Counters contract that follows the same design pattern being applied to the proof of concept contracts that I am working on. In the example the default Counters contract runs tests and generates a gas report as expected however the BytecodeCounters contract will run tests and not generate a gas report.

https://github.com/0xth0mas/foundry-bytecode-gas-report-example

@0xth0mas 0xth0mas added the T-bug Type: bug label Oct 26, 2023
@gakonst gakonst added this to Foundry Oct 26, 2023
@github-project-automation github-project-automation bot moved this to Todo in Foundry Oct 26, 2023
@mattsse
Copy link
Member

mattsse commented Oct 26, 2023

thanks for the repro will investigate

@mattsse mattsse self-assigned this Oct 26, 2023
@mds1
Copy link
Collaborator

mds1 commented Oct 26, 2023

@mattsse
Copy link
Member

mattsse commented Oct 27, 2023

I am working on proof of concept custom bytecode contracts where all of the functions are stubbed out so that the contract can be compiled/verified on block explorers with a generated ABI but the runtime bytecode is assembled in the constructor based on constructor arguments.

can you please explain what impact this has on the defined functions when used

because I think if you do this then everything is inlined, right? so there's no actual call happening that we can track and decode

@0xth0mas
Copy link
Author

0xth0mas commented Oct 27, 2023

Using the counter example as a reference...
image

Lines 8-10 store the bytecode that I wrote that implements number, setNumber and increment to memory.
Line 11 returns the memory area that the bytecode is stored in, telling the EVM that that bytecode is going to be the runtime bytecode for the contract that it needs to save and execute on each call.
Lines 15-17 exist for the compiler to generate the same ABI as the regular counters example.

That makes the deployed bytecode this:
59351559341117600D57601A565B5959FD5B6000526004601CFD5B593560E01C8063D09DE08A1460415780633FB5C1CB14604C5780638381F58A146055575959FD5B595460010159555959F35B60043559555959F35B59595459525990F3

The first portion 59351559341117600D57601A565B5959FD checks calldata and msg.value to ensure they are non-zero and zero respectively and reverts if either is not the case.

Next 5B6000526004601CFD is a little revert snippet for custom reverts.

Then the function selector...
5B593560E01C -- get the selector
8063D09DE08A14604157 -- jump if it's increment
80633FB5C1CB14604C57 -- jump if it's setNumber
80638381F58A14605557 -- jump if it's number
5959FD -- revert if we didn't match selector

5B595460010159555959F3 -- increment - load from storage, add one, store to storage, return nothing
5B60043559555959F3 -- setNumber - load from calldata, store to storage, return nothing
5B59595459525990F3 -- number - load from storage, store to memory, return 0x00,0x20

All calls should execute the deployed runtime bytecode in the VM the same as a call to a contract whose runtime bytecode was created by solc. This is like using a different compiler that wraps in Solidity to create the ABI, simplify integration with other contracts that are not hyper aggresively optimized, and make them verifiable on block explorers. The Solidity compiler will generate some deployment bytecode assuming that those functions are basically just empty returns but its intention of what the runtime bytecode will be in the deployment bytecode is overridden by the runtime bytecode returned from the constructor.

@0xth0mas
Copy link
Author

Here's a deployed example with the functions called -
https://goerli.etherscan.io/address/0x69db6d03923b11d9c4391653b51526acb24c3ef4

@parkerburchett
Copy link

parkerburchett commented Nov 21, 2023

I am running into a similar issue of --gas-report not doing anything.

Component

Forge

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

Foundry
Foundryup

$npm run foundryup --version
> 8.5.1

OS:

Linux

What version of Foundry are you on?

foundryup: installing foundry (version nightly, tag nightly-7b452656f722fc560f0414db3ce24a1f2972a8b7)
foundryup: downloading latest forge, cast, anvil, and chisel
################################################################################################### 100.0%
foundryup: downloading manpages
################################################################################################### 100.0%
foundryup: installed - forge 0.2.0 (7b45265 2023-11-21T00:22:00.899582254Z)
foundryup: installed - cast 0.2.0 (7b45265 2023-11-21T00:22:00.519258276Z)
foundryup: installed - anvil 0.2.0 (7b45265 2023-11-21T00:22:00.520547511Z)
foundryup: installed - chisel 0.2.0 (7b45265 2023-11-21T00:22:00.525692023Z)

What command(s) is the bug in?

forge test --gas-report --match-test testNotGettingGasReport

Minimal Example

pragma solidity 0.8.17;

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

contract DummyTest is Test {

    function testNotGettingGasReport () public {
        uint256 a;
        for (uint256 i; i < 10; i++) {
            a = i * 1e18;
        }

        assertTrue(a < 100e18);
    }
}

This passes but does not give a gas report

[PASS] testNotGettingGasReport() (gas: 2369)
Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 322.30µs

@0xpolarzero
Copy link

Running into the exact same issue as well.

This is a bit annoying when running benchmarks (i.e. comparing to similar implementations).

Right now, the two solutions are either to use Hardhat (which does indeed support "bytecode" contracts, or to log gas usage (with gasleft()).

A third solution is this forge-gas-metering library, but it would currently only support a single call per test.

It would be amazing to have this supported, especially as it seems to be one of the most efficient ways to optimize gas usage; hence my eagerness to be able to demonstrate it!

Thanks.


For reproduction:

src/BytecodeDrop.sol

// SPDX-License-Identifier: MIT
pragma solidity 0.8.20;

contract BytecodeDrop {
    constructor() {
        assembly {
            mstore(0x00, 0x59351559341117600D57601A565B5959FD5B6000526004601CFD5B593560E01C)
            mstore(0x20, 0x6382947ABE14602D575B3434FD5B602435600401604435600401808203813583)
            mstore(0x40, 0x3581036055576004357F23b872dd000000000000000000000000000000000000)
            mstore(0x60, 0x0000000000000000000034523360045230602452606435604452343460643434)
            mstore(0x80, 0x855AF1156055577Fa9059cbb0000000000000000000000000000000000000000)
            mstore(0xA0, 0x000000000000000034528160051B60200185018560015B906020018083031560)
            mstore(0xC0, 0xDC5790813560045285820335602452343460443434885AF11660B6565B156055)
            mstore(0xE0, 0x573434F300000000000000000000000000000000000000000000000000000000)
            return(0x00, 0xE4)
        }
    }

    /**
     * @notice Airdrop ERC20 tokens to a list of addresses
     * @param _token The address of the ERC20 contract
     * @param _addresses The addresses to airdrop to
     * @param _amounts The amounts to airdrop
     * @param _totalAmount The total amount to airdrop
     */
    function airdropERC20(
        address _token,
        address[] calldata _addresses,
        uint256[] calldata _amounts,
        uint256 _totalAmount
    ) external {}
}

test/mocks/Mock_ERC20.sol

// SPDX-License-Identifier: MIT
pragma solidity 0.8.20;

import {ERC20} from "@solady/tokens/ERC20.sol";

contract Mock_ERC20 is ERC20 {
    constructor(uint256 _initialAmount) {
        _mint(msg.sender, _initialAmount);
    }

    function name() public pure override returns (string memory) {
        return "Mock_ERC20";
    }

    function symbol() public pure override returns (string memory) {
        return "M20";
    }
}

test/BytecodeDrop.t.sol

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.0;

import {BytecodeDrop} from "src/BytecodeDrop.sol";
import {Mock_ERC20} from "test/mocks/Mock_ERC20.sol";
import {Test} from "forge-std/Test.sol";

contract BytecodeDropTest is Test {
    BytecodeDrop bytecodeDrop;
    Mock_ERC20 erc20;

    uint256 constant NUM_RECIPIENTS = 2;
    uint256 constant AMOUNT = 10e18;
    uint256 constant TOTAL_AMOUNT = AMOUNT * NUM_RECIPIENTS;

    constructor() {
        bytecodeDrop = new BytecodeDrop();
        erc20 = new Mock_ERC20(AMOUNT * 2);
    }

    function test_BytecodeDrop() public {
        address[] memory addresses = new address[](NUM_RECIPIENTS);
        uint256[] memory amounts = new uint256[](NUM_RECIPIENTS);

        for (uint256 i = 0; i < NUM_RECIPIENTS; i++) {
            addresses[i] = vm.addr(i + 1);
            amounts[i] = AMOUNT;
        }

        erc20.approve(address(bytecodeDrop), TOTAL_AMOUNT);
        bytecodeDrop.airdropERC20(address(erc20), addresses, amounts, AMOUNT * 2);
    }
}

Run the test: forge test --mt test_BytecodeDrop --gas-report

Output (only calls to the mock ERC20):
image

@DaniPopes
Copy link
Member

Test and setup functions are excluded from gas reports on purpose, see

// ignore any test/setup functions
let should_include = !(name.is_test() || name.is_invariant_test() || name.is_setup());
if should_include {

I guess this should be toggleable given the demand?

@zerosnacks zerosnacks changed the title forge test --gas-report not reporting certain contracts bug(forge test): --gas-report does reporting certain contracts (assembled bytecode in constructor) Jul 4, 2024
@zerosnacks zerosnacks added Cmd-forge-test Command: forge test C-forge Command: forge A-gas-snapshots Area: gas snapshotting/reporting A-compatibility Area: compatibility labels Jul 4, 2024
@zerosnacks zerosnacks added this to the v1.0.0 milestone Jul 26, 2024
@zerosnacks zerosnacks mentioned this issue Aug 1, 2024
2 tasks
@grandizzy grandizzy removed this from the v1.0.0 milestone Oct 24, 2024
@grandizzy grandizzy added the P-low Priority: low label Oct 24, 2024
@nksazonov
Copy link

nksazonov commented Oct 26, 2024

I have the similar problem: bytecode imported from a json file, deployed via create in a test file covering all functionality of an ERC20. Nothing except for tests execution results generates after executing forge test --gas-report.

This forces me to use gasSnapshots, which basically explicitly duplicates what the --gas-report should be able to do.

@mattsse , are there any estimates on when this is going to be supported? It is exactly a year since this was reported (interesting coincidence).

@grandizzy grandizzy assigned grandizzy and unassigned mattsse Oct 28, 2024
@grandizzy grandizzy moved this from Todo to Ready For Review in Foundry Oct 30, 2024
@github-project-automation github-project-automation bot moved this from Ready For Review to Done in Foundry Oct 30, 2024
@grandizzy grandizzy moved this from Done to Completed in Foundry Nov 4, 2024
@nksazonov
Copy link

nksazonov commented Dec 24, 2024

Re:

I guess this [test and setup functions] should be toggleable given the demand?

by @DaniPopes and #9232 by @grandizzy

I don't think the gas_reports_include_tests config parameter solution fully solves the issue. Indeed, it allows to report gas usage by test contracts instead of the contracts that are tested. However, the main issue still remains - inability to meter gas for contracts with logic created NOT by solc (example by @0xth0mas and mine).

@mattsse gave a hint in his comment:

because I think if you do this then everything is inlined, right? so there's no actual call happening that we can track and decode

However, I am not sure if this is true given that the test contract DOES make an external call to the contract being tested...
Are there other problems involved? Could you clarify whether it would be possible to achieve gas metering in contracts NOT created with new MyContract(...)?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-compatibility Area: compatibility A-gas-snapshots Area: gas snapshotting/reporting C-forge Command: forge Cmd-forge-test Command: forge test P-low Priority: low T-bug Type: bug
Projects
Status: Completed
Development

Successfully merging a pull request may close this issue.

9 participants