Conversation
WalkthroughA benchmarking system for the aztec-state-migration library is introduced, consisting of a MinimalBenchmark Noir contract with multiple test functions measuring migration complexity, a Python utility script that compiles contracts and extracts circuit metrics, and benchmark documentation. Configuration updates add the new contract to the workspace. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Python as benchmark.py
participant Nargo as Nargo Compiler
participant Contracts as Noir Contracts
participant Artifacts as Artifact JSONs
participant MarkdownGen as Markdown Generator
participant Output as BENCHMARKS.md
User->>Python: npm run benchmark
alt --skip-compile not set
Python->>Nargo: Compile minimal_benchmark
Nargo->>Contracts: Process contract code
Contracts->>Artifacts: Generate JSON artifacts
end
Python->>Artifacts: Extract function metrics
Artifacts->>Python: ACIR sizes, function names, parameters
Python->>Python: Calculate marginal costs per note
Python->>MarkdownGen: Pass extracted functions & reference
MarkdownGen->>MarkdownGen: Generate results table & sections
MarkdownGen->>Output: Write BENCHMARKS.md
Output->>User: Display benchmark report
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@noir/test-contracts/minimal-benchmark/Nargo.toml`:
- Line 5: The Nargo.toml entry compiler_version currently reads ">=0.18.0" which
violates the repo policy requiring Noir v1.0.0-beta.x; update the
compiler_version field in Nargo.toml to the beta toolchain used for these
benchmarks (e.g., set compiler_version to "1.0.0-beta.18") so it matches the
repo's Noir beta version policy and the nargo version used to generate the
benchmarks.
In `@scripts/benchmark.py`:
- Around line 172-177: The ratio calculation can raise ZeroDivisionError when
reference["acir_bytes"] is zero; in the for loop over constrained (where
acir_kb, ratio, f are computed and lines.append is called) guard the division by
checking reference["acir_bytes"] first and set ratio to a safe sentinel (e.g.,
float('inf') or a string like "N/A") if it's zero, otherwise compute ratio =
f["acir_bytes"] / reference["acir_bytes"]; then use that guarded ratio in the
lines.append call.
- Around line 94-104: Replace the broad/silent exception handling in
get_nargo_version and the nearby try/except blocks (the generic "except
Exception:" handlers around lines 102 and 126) with targeted error handling that
logs or re-raises the caught exception so failures during parse/process don't
get swallowed; in get_nargo_version capture the subprocess exception (and
parsing errors) and include the exception message in a log or exception before
returning "unknown". Also guard the benchmark ratio computation that divides by
reference["acir_bytes"] by checking that reference["acir_bytes"] is non‑zero
(handle zero by skipping the ratio, using None/inf, or emitting a clear error)
to avoid ZeroDivisionError. Ensure the changes reference the get_nargo_version
function and the code that computes the ratio from reference["acir_bytes"] so
reviewers can locate the fixes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: bdf539fc-e10f-45a7-950d-70ffbd2225fc
📒 Files selected for processing (6)
BENCHMARKS.mdnoir/Nargo.tomlnoir/test-contracts/minimal-benchmark/Nargo.tomlnoir/test-contracts/minimal-benchmark/src/main.nrpackage.jsonscripts/benchmark.py
DamianStraszak
left a comment
There was a problem hiding this comment.
Tested and it works. Looking good.
Summary by CodeRabbit
New Features
Documentation