Add local gas snapshot baseline#147
Merged
Merged
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review to trigger a review and subscribe this PR to future pushes, or @claude review once for a one-time review.
Tip: disable this comment in your organization's Code Review settings.
Contributor
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds a deterministic local Foundry gas snapshot test and committed baseline for core Stream operations; wires snapshot checks into Makefile/CI/check scripts; extends manifest generation and tests to include the baseline; updates release artifacts/checksums, docs, and ops state. ChangesLocal gas snapshot baseline
Sequence DiagramsequenceDiagram
participant StreamGasSnapshotTest
participant Foundry
participant CI_Run
participant ReleaseManifestGenerator
StreamGasSnapshotTest->>Foundry: run `forge snapshot --match-path test/StreamGasSnapshot.t.sol`
Foundry->>ReleaseManifestGenerator: produce/compare gas-snapshot against `release-artifacts/baselines/v0.1.0/gas-snapshot.snap`
CI_Run->>Foundry: CI step runs `forge snapshot --check`
Foundry-->>CI_Run: logs result to `ci-logs/forge-gas-snapshot.log`
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/tooling.md (1)
24-52:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd gas snapshot check to the Local Checks command sequence.
The Makefile's
checktarget includesgas-snapshot-checkas a dependency (see relevant code snippet:check: build test gas-snapshot-check size ...), but the bash command sequence documenting whatmake checkruns does not list the gas snapshot check command. Users reading the "Local Checks" section will not see thatgas-snapshot-checkexecutes when they runmake check.Add the command between the
forge testandforge build --sizeslines:forge snapshot --match-path test/StreamGasSnapshot.t.sol --check release-artifacts/baselines/v0.1.0/gas-snapshot.snapThis ensures the documentation accurately reflects the complete set of checks integrated into
make check.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/tooling.md` around lines 24 - 52, The docs miss the gas snapshot check listed in the Local Checks sequence; add the missing command invoked by the Makefile target gas-snapshot-check into the bash snippet shown in tooling.md between the existing forge test -vvv and forge build --sizes lines: insert the forge snapshot --match-path test/StreamGasSnapshot.t.sol --check release-artifacts/baselines/v0.1.0/gas-snapshot.snap command so the documented sequence matches what the Makefile's check target actually runs.
🧹 Nitpick comments (1)
scripts/test_release_manifest.py (1)
250-253: ⚡ Quick winAssert the gas snapshot digest, not just its path.
Right now this new coverage only proves the manifest points at the expected filename. A regression that keeps the path but records the wrong
sha256orsize_byteswould still pass, which weakens the evidence this PR is adding. Please assert those fields againstgenerator.file_sha256(paths["gas_snapshot"])andpaths["gas_snapshot"].stat().st_sizeas well.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/test_release_manifest.py` around lines 250 - 253, The test currently only asserts the gas snapshot path; extend it to also assert the snapshot's sha256 and size_bytes match the actual file: call generator.file_sha256(paths["gas_snapshot"]) and compare to manifest["release_artifacts"]["gas_snapshot_baseline"]["sha256"], and get paths["gas_snapshot"].stat().st_size and compare to manifest["release_artifacts"]["gas_snapshot_baseline"]["size_bytes"]; keep the existing path assert and add these two assertions near the same block in scripts/test_release_manifest.py.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/generate_release_manifest.py`:
- Line 22: DEFAULT_GAS_SNAPSHOT currently hard-codes "v0.1.0" which will
silently pin all manifests to that snapshot; change DEFAULT_GAS_SNAPSHOT to be
derived from the release/protocol version used to build the manifest (e.g.,
compose the Path using the release/version variable used elsewhere in the script
or the parsed --version/--release arg) or, if a user supplies --gas-snapshot,
validate that the snapshot path's version component matches the discovered
protocol/release version(s) and raise an error if it does not; update references
to DEFAULT_GAS_SNAPSHOT and the --gas-snapshot handling so the default is
dynamic and a mismatch fails fast.
---
Outside diff comments:
In `@docs/tooling.md`:
- Around line 24-52: The docs miss the gas snapshot check listed in the Local
Checks sequence; add the missing command invoked by the Makefile target
gas-snapshot-check into the bash snippet shown in tooling.md between the
existing forge test -vvv and forge build --sizes lines: insert the forge
snapshot --match-path test/StreamGasSnapshot.t.sol --check
release-artifacts/baselines/v0.1.0/gas-snapshot.snap command so the documented
sequence matches what the Makefile's check target actually runs.
---
Nitpick comments:
In `@scripts/test_release_manifest.py`:
- Around line 250-253: The test currently only asserts the gas snapshot path;
extend it to also assert the snapshot's sha256 and size_bytes match the actual
file: call generator.file_sha256(paths["gas_snapshot"]) and compare to
manifest["release_artifacts"]["gas_snapshot_baseline"]["sha256"], and get
paths["gas_snapshot"].stat().st_size and compare to
manifest["release_artifacts"]["gas_snapshot_baseline"]["size_bytes"]; keep the
existing path assert and add these two assertions near the same block in
scripts/test_release_manifest.py.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 32e2f305-d00d-46fd-87ab-5882d3b159eb
⛔ Files ignored due to path filters (1)
release-artifacts/baselines/v0.1.0/gas-snapshot.snapis excluded by!**/*.snap
📒 Files selected for processing (21)
.github/workflows/ci.ymlCHANGELOG.mdMakefiledocs/known-blockers.mddocs/release-policy.mddocs/status.mddocs/tooling.mdops/AUTONOMOUS_RUN.mdops/ROADMAP.mdrelease-artifacts/README.mdrelease-artifacts/latest/SHA256SUMSrelease-artifacts/latest/release-checksums.jsonrelease-artifacts/latest/release-manifest.jsonscripts/check.ps1scripts/check.shscripts/generate_release_manifest.pyscripts/test_release_checksums.pyscripts/test_release_manifest.pytest/README.mdtest/StreamGasSnapshot.t.soltest/helpers/CharacterizationTestBase.sol
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/generate_release_manifest.py`:
- Around line 276-285: The current validation only checks gas_snapshot_path.name
and gas_snapshot_path.parent.name; change it to compare the full normalized path
against the canonical baseline path (BASELINE_DIR / f"v{protocol_versions[0]}" /
GAS_SNAPSHOT_FILENAME) and raise ReleaseManifestError if they differ so only
files under the release-artifacts/baselines/<version>/gas-snapshot.snap are
allowed; update the check around gas_snapshot_path (and related variables
GAS_SNAPSHOT_FILENAME, expected_path) to resolve/absolute both sides before
comparing to prevent accepting paths like /tmp/v0.1.0/gas-snapshot.snap.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ac8cd6f9-de17-4411-b1ca-c342c10c2e90
📒 Files selected for processing (7)
docs/tooling.mdops/AUTONOMOUS_RUN.mdrelease-artifacts/latest/SHA256SUMSrelease-artifacts/latest/release-checksums.jsonrelease-artifacts/latest/release-manifest.jsonscripts/generate_release_manifest.pyscripts/test_release_manifest.py
✅ Files skipped from review due to trivial changes (5)
- release-artifacts/latest/SHA256SUMS
- release-artifacts/latest/release-checksums.json
- docs/tooling.md
- ops/AUTONOMOUS_RUN.md
- release-artifacts/latest/release-manifest.json
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Summary
Closes #146
Validation
forge fmt test\StreamGasSnapshot.t.sol test\helpers\CharacterizationTestBase.solforge test --match-path test\StreamGasSnapshot.t.sol -vvvforge snapshot --match-path test\StreamGasSnapshot.t.sol --snap release-artifacts\baselines\v0.1.0\gas-snapshot.snapforge snapshot --match-path test\StreamGasSnapshot.t.sol --check release-artifacts\baselines\v0.1.0\gas-snapshot.snappython scripts\test_release_manifest.pypython scripts\test_release_checksums.pypython scripts\generate_release_manifest.py --checkpython scripts\generate_release_checksums.py --checkpython scripts\test_changelog_check.pypython scripts\check_changelog.pypython -m py_compile scripts\generate_release_manifest.py scripts\test_release_manifest.py scripts\generate_release_checksums.py scripts\test_release_checksums.pybash -n scripts/check.sh scripts/bootstrap-ec2.shscripts\check.ps1andscripts\bootstrap-windows.ps1make checkpowershell -NoProfile -ExecutionPolicy Bypass -File scripts\check.ps1git diff --checkSummary by CodeRabbit
Tests
Documentation
Chores