-
Notifications
You must be signed in to change notification settings - Fork 615
feat: gas reports and snapshots #12724
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
Changes from 4 commits
bd476a9
15d01be
9f0c7b1
7385dab
1ecf549
fd20f41
c3e5e47
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -12,7 +12,8 @@ Alternatively you can use docker instead, it will handle installations and run t | |
|
|
||
| The `src` folder contain contracts that is to be used by the local developer testnet. It is grouped into 3 categories: | ||
|
|
||
| - `core` contains the required contracts, the bare minimum | ||
| - `core` contains the required contracts, the bare minimum. | ||
| - `governance` contains the contracts for the governance system. | ||
| - `mock` contains stubs, for now an always true verifier. | ||
| - `periphery` stuff that is nice to have, convenience contracts and functions belong in here. | ||
|
|
||
|
|
@@ -31,6 +32,16 @@ We use `forge fmt` to format. But follow a few general guidelines beyond the sta | |
| - Do `function transfer(address _to, uint256 _amount);` | ||
| - use `_` prefix for `internal` and `private` functions. | ||
|
|
||
| ## Gas snapshots and CI | ||
|
|
||
| CI will run `forge snapshot --check`. This means that as you develop, you should run `./bootstrap.sh snapshot --diff` to make sure you understand the gas cost of your changes. | ||
|
|
||
| Note: the only reason `forge snapshot` is wrapped in the bootstrap script is to standardize the output, or any future filtering. | ||
|
|
||
| When your PR is ready for review, run `./bootstrap.sh snapshot` to update the snapshot, then `./bootstrap.sh test` to make sure you're good. | ||
|
|
||
| You can also run `./bootstrap.sh gas_report` to get a gas report for the current state. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Think it would be fine to mention the |
||
|
|
||
| ## Contracts: | ||
|
|
||
| The contracts are in a very early stage, and don't worry about gas costs right now. Instead they prioritize development velocity. | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -20,7 +20,6 @@ import { | |
| import {Rollup} from "@aztec/core/Rollup.sol"; | ||
| import {Strings} from "@oz/utils/Strings.sol"; | ||
| import {Errors} from "@aztec/core/libraries/Errors.sol"; | ||
|
|
||
| import {RollupBase, IInstance} from "./base/RollupBase.sol"; | ||
|
|
||
| // solhint-disable comprehensive-interface | ||
|
|
@@ -90,6 +89,11 @@ contract MultiProofTest is RollupBase { | |
| ) | ||
| ) | ||
| ); | ||
| // skip blob check if forge gas report is true | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This and the rollup.t.sol are both pulling from the rollup base. We could instead have a value that is written in the constructor or so to set it up neatly across all of them and not having multiple places needing to go look for env vars? Could also mean that this value is just set in that one place and not even needed in the others which seems nice
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. An extra reason for this is that it would be inherited 👀 so we would avoid an issue as just encountered hehe where the ignition ones are using the blobs so it fails because not updated. |
||
| // https://github.com/foundry-rs/foundry/issues/10074 | ||
| if (vm.envOr("FORGE_GAS_REPORT", false)) { | ||
| skipBlobCheck(address(rollup)); | ||
| } | ||
|
|
||
| registry.upgrade(address(rollup)); | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not really sure how much I would care about the gas spent on tests, but more the gas spent on the functions in the contracts. e.g., a test spending 15M gas is not super important for me, but if it is a function that we use that is spending 15M gas that is something that is important