fix code coverage - stack too deep issues

[AmadiMichael]

No description provided.

[AmadiMichael]

Note: forge coverage works if lib-keccak merges this ethereum-optimism/lib-keccak#6 and we update the dependency

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 44.30%. Comparing base (16a5d61) to head (7a2eb56).
Report is 10 commits behind head on develop.

Additional details and impacted files

@@             Coverage Diff             @@
##           develop   #12843      +/-   ##
===========================================
- Coverage    44.44%   44.30%   -0.14%     
===========================================
  Files          801      801              
  Lines        72001    72001              
===========================================
- Hits         31999    31900      -99     
- Misses       37393    37494     +101     
+ Partials      2609     2607       -2     

Flag	Coverage Δ
cannon-go-tests-32	61.85% <ø> (-2.02%)	⬇️
cannon-go-tests-64	56.72% <ø> (-1.66%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

see 18 files with indirect coverage changes

[AmadiMichael]

Also, for forge coverage to work, we'll need to make test_callWithMinGas_noLeakageHigh_succeeds and test_callWithMinGas_noLeakageLow_succeeds to work for 2 gas values each.

[smartcontracts]

It will take some time to get this PR merged as it changes prod contracts but it's a valuable reference because it allows us to generate coverage reports either way.

[AmadiMichael]

Also, for forge coverage to work, we'll need to make test_callWithMinGas_noLeakageHigh_succeeds and test_callWithMinGas_noLeakageLow_succeeds to work for 2 gas values each.

this is now implemented.

[AmadiMichael]

Note: forge coverage works if lib-keccak merges this ethereum-optimism/lib-keccak#6 and we update the dependency.

This is now updated

[mds1]

accidental semver bump?

[AmadiMichael]

Nope, lib-keccak was updated

[mds1]

That makes sense, but do we normally bump semvers when a dependency bump happens? I'm supportive of it because it changes the bytecode, but I suspect historically we've been bad at this and need a way to better automate semver bumps (cc @smartcontracts)

[AmadiMichael]

Yeah it's actually on the to-do @smartcontracts

[smartcontracts]

CI will at least force you to semver bump when dependencies are changed

[mds1]

We are planning to simplify things and remove contract-specific semvers as part of the OPCM Upgrades work in ethereum-optimism/design-docs#159. That design review is today (at 1030am PT)

[mds1]

Does it still solve stack too deep if we remove proposer and challenger from this struct and have those as individual inputs? The rationale being we can then keep the interface between the PDG and regular FDG more similar, and have them both take the same FDGConstructorParams arg, then PDF just takes two extra args

[smartcontracts]

@mds1 yeah I was thinking the same, would be nicer that way imo.

[AmadiMichael]

It compiles with forge coverage this way but would need to be excluded from interfaces check. Couldn't find a way to use this struct for both contracts without breaking the interfaces check for IPermissionedDisputeGame.sol. The only option is verbose and probably unneccesarily more costly gas-wise.

constructor (DGConstructorParams memory _params) FaultDisputeGame(FaultDisputeGame.DGConstructorParams({
        _gameType: _params._gameType,
        _absolutePrestate: _params._absolutePrestate,
        ...
})) {
        ...
}

[smartcontracts]

Hmm, ok, I'll take a look at this and see if I can get it to pass the checks

[mds1]

Overall pretty small diff to fix stack too deep. This is awesome, nice work!

[AmadiMichael]

This PR was broken down into multiple PRs and merged into develop.