ci: track and enforce contract WASM sizes - #526
Conversation
|
@Caneryy Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits. You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀 |
📝 WalkthroughWalkthroughAdds a Node.js CLI to measure registry and agents WASM sizes, compare pull-request builds with base artifacts, enforce a 128 KiB ceiling, write metrics, and integrate reporting and artifact retention into contract CI. ChangesContract WASM size enforcement
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant GitHubActions
participant contract-build
participant check-wasm-size.mjs
GitHubActions->>contract-build: Set up Node.js and build contracts
contract-build->>check-wasm-size.mjs: Run WASM size test and size check
contract-build->>contract-build: Build base contracts on pull requests
contract-build->>check-wasm-size.mjs: Provide current and base WASM artifacts
check-wasm-size.mjs-->>contract-build: Print report and write wasm-size-metrics.json
contract-build-->>GitHubActions: Upload metrics artifact
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
scripts/check-wasm-size.mjs (1)
149-156: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick win
git_shametric won't match a real commit for PR runs.For
pull_requestevents,GITHUB_SHAis GitHub's synthetic merge commit rather than the PR's actual head commit, so persisted metrics (line 152) can't be reliably traced back to a specific commit in the history when reviewing size trends later. Consider preferringgithub.event.pull_request.head.sha(passed in via env from the workflow) when available.♻️ Proposed fix
- git_sha: process.env.GITHUB_SHA ?? null, + git_sha: process.env.PR_HEAD_SHA ?? process.env.GITHUB_SHA ?? null,And in the workflow step, pass
PR_HEAD_SHA: ${{ github.event.pull_request.head.sha }}.🤖 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/check-wasm-size.mjs` around lines 149 - 156, Update the metrics construction around git_sha to prefer a workflow-provided PR_HEAD_SHA for pull request runs, falling back to GITHUB_SHA for other events or when the PR value is unavailable. Update the workflow step that invokes this script to pass PR_HEAD_SHA from github.event.pull_request.head.sha.scripts/check-wasm-size.test.mjs (1)
100-112: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winConsider adding a boundary test for
size === maximum.Existing tests cover under-ceiling and over-ceiling but not the exact-boundary case, which exercises the
<=comparison directly responsible for pass/fail classification.✅ Suggested addition
test("passes when size exactly equals the ceiling", (t) => { const paths = fixture(); t.after(() => rmSync(paths.directory, { recursive: true, force: true })); writeFileSync(paths.currentRegistry, Buffer.alloc(1024)); writeFileSync(paths.currentAgents, Buffer.alloc(100)); const result = run(paths, ["--max-bytes", "1024"]); assert.equal(result.status, 0, result.stderr); });🤖 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/check-wasm-size.test.mjs` around lines 100 - 112, Add a boundary test alongside the existing ceiling tests that sets the primary artifact size exactly to the configured maximum, runs the size check, and asserts a successful status. Reuse the existing fixture setup, cleanup, and run helpers, while preserving the current under-limit and over-limit coverage..github/workflows/ci.yml (1)
81-103: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winBase contract build isn't cached, doubling per-PR build time.
The base build's output directories (
/tmp/lodestar-base/contract/target,/tmp/lodestar-base/contract/agents/target) aren't covered by the existing Rust cache (lines 37-46), so every PR run fully recompiles both base contracts from scratch. Sincegithub.event.pull_request.base.shais known up front (no fetch required), a cache keyed on it would let repeated pushes against the same base commit reuse the build.♻️ Proposed addition (before the base build step)
+ - name: Cache base contract build artifacts + if: github.event_name == 'pull_request' + uses: actions/cache@v4 + with: + path: | + /tmp/lodestar-base/contract/target + /tmp/lodestar-base/contract/agents/target + key: rust-base-${{ runner.os }}-${{ github.event.pull_request.base.sha }} + - name: Build base contracts for size comparison🤖 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 @.github/workflows/ci.yml around lines 81 - 103, Add caching for the base contract build before the “Build base contracts for size comparison” step, keyed by github.event.pull_request.base.sha and covering both /tmp/lodestar-base/contract/target and /tmp/lodestar-base/contract/agents/target. Restore the cache for repeated pushes against the same base commit and ensure the existing build and copy flow remains unchanged when no cache is available.
🤖 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.
Nitpick comments:
In @.github/workflows/ci.yml:
- Around line 81-103: Add caching for the base contract build before the “Build
base contracts for size comparison” step, keyed by
github.event.pull_request.base.sha and covering both
/tmp/lodestar-base/contract/target and
/tmp/lodestar-base/contract/agents/target. Restore the cache for repeated pushes
against the same base commit and ensure the existing build and copy flow remains
unchanged when no cache is available.
In `@scripts/check-wasm-size.mjs`:
- Around line 149-156: Update the metrics construction around git_sha to prefer
a workflow-provided PR_HEAD_SHA for pull request runs, falling back to
GITHUB_SHA for other events or when the PR value is unavailable. Update the
workflow step that invokes this script to pass PR_HEAD_SHA from
github.event.pull_request.head.sha.
In `@scripts/check-wasm-size.test.mjs`:
- Around line 100-112: Add a boundary test alongside the existing ceiling tests
that sets the primary artifact size exactly to the configured maximum, runs the
size check, and asserts a successful status. Reuse the existing fixture setup,
cleanup, and run helpers, while preserving the current under-limit and
over-limit coverage.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: f9dddb63-3dd3-4991-ad97-4bd72f5065d9
📒 Files selected for processing (4)
.github/workflows/ci.ymlCONTRIBUTING.mdscripts/check-wasm-size.mjsscripts/check-wasm-size.test.mjs
|
Hi @Caneryy, This PR could not be merged because it has merge conflicts with the target branch. Please resolve the merge conflicts, push the updated changes, and the PR can be reviewed and merged. Thank you! |
2 participants
Summary
Validation
node --test scripts/check-wasm-size.test.mjscargo testincontract(18 passed)cargo testincontract/agents(17 passed)stellar contract buildfor both contractsgit diff --checkCurrent optimized sizes are 10,401 bytes for registry and 14,917 bytes for agents.
Closes #413
Summary by CodeRabbit
New Features
Documentation
Bug Fixes