fix(forge): record gas for nested deployed contracts

[grandizzy]

Motivation

Closes #9300
Atm the gas reports are not collected for Calls and Creates if not the top level call (comment says // Only include Calls and Creates as only these calls are isolated in inspector. but check returns if one of these kinds, so they're not added)

foundry/crates/forge/src/gas_report.rs

Lines 97 to 106 in 4817280

	// Only include top-level calls which account for calldata and base (21.000) cost.
	// Only include Calls and Creates as only these calls are isolated in inspector.
	if trace.depth > 1 &&
	(trace.kind == CallKind::Call ||
	trace.kind == CallKind::Create ||
	trace.kind == CallKind::Create2 ||
	trace.kind == CallKind::EOFCreate)
	{
	return;
	}

E.g. for a Parent contract that creates Child which in turn creates AnotherChild, running tests with traces and gas report will look like

[PASS] test_gas() (gas: 314505)
Traces:
  [314505] NestedDeploy::test_gas()
    ├─ [254857] → new Parent@0x5615dEB798BB3E4dFa0139dFa1b3D433Cc23b72f
    │   ├─ [125076] → new Child@0x104fBc016F4bb334D775a19E8A6510109AC63E00
    │   │   ├─ [20275] → new AnotherChild@0x41C3c259514f88211c4CA2fd805A93F8F9A57504
    │   │   │   └─ ← [Return] 101 bytes of code
    │   │   └─ ← [Return] 252 bytes of code
    │   └─ ← [Return] 165 bytes of code
...

Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 2.34ms (1.49ms CPU time)
| src/Counter.sol:Child contract |                 |       |        |       |         |
|--------------------------------|-----------------|-------|--------|-------|---------|
| Deployment Cost                | Deployment Size |       |        |       |         |
| 0                              | 0               |       |        |       |         |
| Function Name                  | min             | avg   | median | max   | # calls |
| w                              | 26256           | 26256 | 26256  | 26256 | 1       |


| src/Counter.sol:Parent contract |                 |     |        |     |         |
|---------------------------------|-----------------|-----|--------|-----|---------|
| Deployment Cost                 | Deployment Size |     |        |     |         |
| 254857                          | 770             |     |        |     |         |
| Function Name                   | min             | avg | median | max | # calls |
| child                           | 182             | 182 | 182    | 182 | 1       |

All contracts and gas are displayed in traces but deployment cost and size for Child and AnotherChild reports are missing (one more thing that is not inline between traces and gas reports is that in traces the creation bytecode size is shown whereas in gas report the deployment bytecode size)

@klkvr tests are passing but probably there's a good reason for the check, please advise

Solution

• include all calls at all depths in gas reports

[klkvr]

the idea here is to only record samples which were isolated (because --gas-report runs with --isolate by default)

that way the final report would contain values as close to real onchain transactions as possible. but if we'd also include the subcalls than the resulted value would be some average between the top-level vs non-top-level usage

[grandizzy]

the idea here is to only record samples which were isolated (because --gas-report runs with --isolate by default)

that way the final report would contain values as close to real onchain transactions as possible. but if we'd also include the subcalls than the resulted value would be some average between the top-level vs non-top-level usage

makes sense, closing as not something we want to change. One could just create dedicated test (create and call fns) for nested contracts to get proper gas reports.

[klkvr]

we could also consider recording subcalls usage into separate metric, so that you can track gas usage dynamics even if certain methods are not called at top level at all

[grandizzy]

we could also consider recording subcalls usage into separate metric, so that you can track gas usage dynamics even if certain methods are not called at top level at all

yeah, I think having several metrics would be confusing and raise more questions, IMO better to proper document how to generate accurate gas report

[sakulstra]

@grandizzy I see.

My point is/was that the current reported 0 seems like a bug.
The reason why i originally noticed is that we use this value to monitor contract size for upgradable contracts and obviously on 0 we never spot any regression.

I think it's an okayish workaround to have to manually deploy factory deployed contracts once for the gas metering, but then again for me this seems a bit confusing. If i do forge build --sizes i get sizes for all contracts, while with forge test --gas-report i only get them for some.

I don't know enough about the internals here, but shouldn't the bytecode size be static?
Would it be an option to use the artifacts/build output for the sizes and only derive the gas from the tests?

[grandizzy]

@grandizzy I see.

My point is/was that the current reported 0 seems like a bug. The reason why i originally noticed is that we use this value to monitor contract size for upgradable contracts and obviously on 0 we never spot any regression.

I think it's an okayish workaround to have to manually deploy factory deployed contracts once for the gas metering, but then again for me this seems a bit confusing. If i do forge build --sizes i get sizes for all contracts, while with forge test --gas-report i only get them for some.

I don't know enough about the internals here, but shouldn't the bytecode size be static? Would it be an option to use the artifacts/build output for the sizes and only derive the gas from the tests?

ah, ok, so if that's only re deployment size / create calls I think that could be added, was under the impression gas too, will check

[sakulstra]

ah, ok, so if that's only re deployment size / create calls I think that could be added, was under the impression gas too, will check

I mean the issue as you noted exists on both, but on gas i now understand the rational for why it exists (and we don't rely on the gas field for anything, so 🤷 )