feat(zk-benchmarks): add profile types#3231
Conversation
🟡 Heimdall Review Status
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| use serde::{Deserialize, Serialize}; | ||
| use url::Url; | ||
|
|
||
| use crate::{Profiles, ProofConfig, ProofMode}; |
There was a problem hiding this comment.
The types ProofConfig, ProofMode, and B20SequenceWorkloadConfig (line 120) are imported from crate:: but are not defined anywhere in this crate. lib.rs also doesn't declare/re-export the config or profile modules. This means the code won't compile as-is.
Even for a draft PR stacked on other work, consider either:
- Adding stub type definitions in this PR so it compiles independently, or
- Noting explicitly which prior PR must land first and keeping this PR's base branch set to that PR's branch rather than
main.
| } | ||
|
|
||
| /// Validates semantic config requirements. | ||
| pub fn validate(&self) -> Result<()> { |
There was a problem hiding this comment.
validate() checks the profile name, but resolve_profile() (line 56) performs the same lookup and returns its own error. This means calling from_yaml → validate → resolve_profile does the profile lookup twice.
Consider either:
- Having
validatestore/cache the resolved profile, or - Removing the profile check from
validateand relying onresolve_profileto catch unknown profiles at resolution time, or - Documenting that
validateis a fast-fail check and the duplication is intentional.
mw2000
force-pushed
the
mw2000/zk-benchmarks-config
branch
from
5e03596 to
aa65c6f
Compare
|
|
||
| impl ProofConfig { | ||
| /// Validates proof configuration. | ||
| pub const fn validate(&self) -> Result<()> { |
There was a problem hiding this comment.
const fn here compiles only because the body is trivially Ok(()). The moment real validation logic is added (string checks, ensure!, etc.), const will need to be removed since eyre::Result isn't const-compatible for non-trivial operations. Drop const now to avoid a misleading API and a future breaking change if this is part of a public interface.
| pub const fn validate(&self) -> Result<()> { | |
| pub fn validate(&self) -> Result<()> { |
| /// Resolved runtime endpoint profile. | ||
| #[derive(Clone, Debug, Serialize, Deserialize)] | ||
| pub struct BenchmarkProfile { | ||
| /// Profile name. | ||
| pub name: String, | ||
| /// L2 execution RPC URL. | ||
| pub l2_rpc_url: Url, | ||
| /// Rollup RPC URL. | ||
| pub rollup_rpc_url: Url, | ||
| /// ZK prover RPC URL. | ||
| pub zk_prover_url: Url, | ||
| /// L2 chain ID. | ||
| pub l2_chain_id: u64, | ||
| } |
There was a problem hiding this comment.
BenchmarkProfile is the core type of the profile subsystem — it's what Profiles::get() returns — yet it's defined here in config.rs rather than in profile.rs. This creates a conceptual circular dependency: profile.rs imports BenchmarkProfile from config, while config.rs imports Profiles from profile.
Consider moving BenchmarkProfile to profile.rs so the dependency flows one way: config → profile (config uses profile types to resolve endpoints).
Review Summary2 findings — both low severity, no blockers. Findings
StructureThe overall design is clean: config loading with serde, named profiles with CLI overrides, and a proof mode enum. |
mw2000
changed the title
✅ base-std fork tests: all 602 passedbase/base is fully in sync with the base-std spec. |
2 participants
Adds the initial ZK benchmark scenario config/profile types as the next layer after the merged scaffold PR.