feat: gas benchmark setup #176
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Contract sizes: | Contract | Size (B) | Margin (B) |
|------------------------------|----------|------------|
| AccountFactory | 4,763 | 19,813 |
| ERC1967Proxy | 104 | 24,472 |
| ModularAccount | 23,890 | 686 |
| SemiModularAccount | 25,019 | -443 |
| SingleSignerValidationModule | 3,444 | 21,132 |
| TokenReceiverModule | 2,189 | 22,387 |Code coverage:
|
adamegyed
force-pushed
the
adam/gas-benchmark-setup
branch
from
b7ba8e1
to
93bbfb3
Compare
fangting-alchemy
approved these changes
Sep 16, 2024
Zer0dot
reviewed
Sep 16, 2024
| contract LightAccountGasTest is GasSnapshot { | ||
| address internal constant _LIGHT_ACCOUNT_FACTORY = 0x0000000000400CdFef5E2714E63d8040b700BC24; | ||
|
|
||
| bytes internal constant _LIGHT_ACCOUNT_FACTORY_BYTECODE = |
There was a problem hiding this comment.
So LightAccount and SimpleAccount are just here for benchmarking right-- so we're not expecting them to get any changes, as the bytecode is frozen completely, yeah?
There was a problem hiding this comment.
Yeah, these are frozen at specific version tags. I think we might be OK with removing these in the future, I just added them for now to verify that the gas benchmarking setup works.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation
As we optimize MAv2, we want accurate gas benchmarking that can be automatically calculated with low effort, and checked in to VCS. This is following the general plan outlined here.
Solution
Install
forge-gas-snapshot.Create a new foundry profile called
gas, with a separate set of tests used to generate the gas snapshots. Included in this PR are ports of the "Runtime: Account Creation" tests from aa-benchmarks, which are both very close the previously observed values in hardhat:I'm not sure how to account for the small remaining gas discrepancies, given the these setups use the exact same bytecode. But the error is small enough that I think we can proceed here.
Also, I'm not sure if we should keep these test cases in this repo long-term, or eventually migrate them somewhere else. I'm leaving them here for now, while we work to create the test cases for MAv2.
Unfortunately, the
forge-gas-snapshotdependency expectsforge-stdto be remapped differently from the rest of the repo. In theory there should be a way to use foundry's "remappings context" (more info) to specify this, but I wasn't able to get it to work, so I created separate remappings withinfoundry.tomlfor the two different profiles.Also adds a check in CI for having gas snapshots up-to-date, and moves the linting check out to a different workflow file. Both of these still use the shared CI setup action.
Now that there's a fourth linter config, this also moves the solhint config files and the slither config file into a new folder called
config/, to de-clutter the repo root.Also adds common actions as scripts in
package.json:pnpm gasruns the gas tests and updates valuespnpm gas:checkchecks that the gas tests have their updated valuespnpm fmtto runforge fmtfor all folders, and a checkerpnpm fmt:checkpnpm lintto include the newgas/folder.Delete an accidentally-committed
yarn.lock.