fix: improve docs, optimize allocations, and refactor budget-report m…

cargo-budget-report/src/config.rs

@@ -1,18 +1,40 @@
 #![allow(dead_code)]
 
-//! Global configuration settings for `cargo-budget-report` loaded from `budget.toml`.
+//! Runtime configuration for `cargo-budget-report`.
+//!
+//! [`Config`] holds the settings that govern how the tool connects to the
+//! Soroban network and retries transient failures.  Values are loaded from a
+//! TOML file (typically `budget.toml` at the workspace root) via
+//! [`parse_config`].  Any field omitted from the file falls back to the
+//! [`Default`] implementation, so a minimal config only needs to specify the
+//! fields that differ from the defaults.
 
-/// Global configuration options governing network interaction, signing account,
-/// timeouts, and retry behavior.
+/// Runtime settings for a `cargo budget-report` run.
 #[derive(Debug, PartialEq)]
 pub struct Config {
-    /// The target network name (e.g. "testnet", "futurenet", or "local").
+    /// Soroban network to deploy and measure against.
+    ///
+    /// Typical values: `"testnet"`, `"futurenet"`, `"local"`.
+    /// Defaults to `"testnet"`.
     pub network: String,
-    /// The account identifier or secret used for signing transactions.
+
+    /// Stellar identity / account name used as the deploying source.
+    ///
+    /// Must be a key name known to the local `stellar` CLI key store.
+    /// Defaults to `"default"`.
     pub source: String,
-    /// Network request timeout in seconds.
+
+    /// Maximum wall-clock seconds to wait for a single RPC call before
+    /// treating it as a timeout error.
+    ///
+    /// Defaults to `30`.
     pub timeout_secs: u64,
-    /// Maximum number of retry attempts for transient network/RPC failures.
+
+    /// Number of additional attempts after the first failure before giving
+    /// up on a deploy or invoke call.
+    ///
+    /// A value of `3` means up to four total attempts (one initial + three
+    /// retries).  Defaults to `3`.
     pub retry_attempts: u32,
 }
 
@@ -27,17 +49,39 @@ impl Default for Config {
     }
 }
 
-/// Parses raw TOML content into a [`Config`] instance, falling back to default
-/// values for any missing or invalid fields.
+/// Parse a TOML string into a [`Config`], filling missing fields from
+/// [`Config::default`].
+///
+/// Parsing is deliberately lenient:
+/// - An empty string or whitespace-only input returns the default config.
+/// - A comment-only file returns the default config.
+/// - Unknown TOML keys are silently ignored (handled by `serde`).
+/// - A malformed TOML string returns the default config rather than
+///   propagating a parse error — the tool must always produce output, even
+///   when the config file is temporarily broken.
 pub fn parse_config(content: &str) -> Config {
     let base = Config::default();
 
-    let parsed: Result<PartialConfig, _> = toml::from_str(content);
-    let partial = match parsed {
+    // Attempt to deserialise into PartialConfig, which has every field
+    // Optional so that missing keys fall back to defaults below.
+    let partial = match toml::from_str::<PartialConfig>(content) {
         Ok(p) => p,
+        // Any parse failure (invalid syntax, type mismatch, etc.) is treated
+        // as "no config provided" so the caller always gets a usable Config.
         Err(_) => return base,
     };
 
+    apply_partial(base, partial)
+}
+
+/// Merge `partial` into `base`, taking each field from `partial` when
+/// present and keeping the `base` value otherwise.
+///
+/// Extracted as a separate function to keep [`parse_config`] focused on
+/// I/O concerns (reading and error-handling) while this function handles
+/// the pure data-merging logic.  This split also makes unit-testing the
+/// merge behaviour independent of TOML parsing.
+fn apply_partial(base: Config, partial: PartialConfig) -> Config {
     Config {
         network: partial.network.unwrap_or(base.network),
         source: partial.source.unwrap_or(base.source),
@@ -46,7 +90,11 @@ pub fn parse_config(content: &str) -> Config {
     }
 }
 
-/// Internal representation used for deserialising optional configuration fields from TOML.
+/// Intermediate deserialization target for [`parse_config`].
+///
+/// Every field is `Option<T>` so that serde leaves missing TOML keys as
+/// `None` rather than failing.  [`apply_partial`] then collapses each
+/// `None` to the corresponding default.
 #[derive(serde::Deserialize)]
 struct PartialConfig {
     network: Option<String>,
@@ -91,26 +139,26 @@ mod tests {
     fn full_config_overrides_all_defaults() {
         let content = r#"
 network = "local"
-source = "bob"
+source = "alice"
 timeout_secs = 60
 retry_attempts = 5
 "#;
         let config = parse_config(content);
         assert_eq!(config.network, "local");
-        assert_eq!(config.source, "bob");
+        assert_eq!(config.source, "alice");
         assert_eq!(config.timeout_secs, 60);
         assert_eq!(config.retry_attempts, 5);
     }
 
     #[test]
-    fn malformed_toml_returns_defaults() {
-        let content = "network = \n"; // invalid TOML
+    fn malformed_toml_falls_back_to_defaults() {
+        let content = "this is not valid toml = = =";
         let config = parse_config(content);
         assert_eq!(config, Config::default());
     }
 
     #[test]
-    fn unknown_fields_ignored_gracefully() {
+    fn unknown_keys_are_ignored() {
         let content = r#"
 network = "testnet"
 unknown_field = "should not cause errors"
@@ -119,4 +167,22 @@ unknown_field = "should not cause errors"
         assert_eq!(config.network, "testnet");
         assert_eq!(config.source, Config::default().source);
     }
+
+    /// Verify apply_partial directly: every Some field overrides the base,
+    /// every None keeps the base value.
+    #[test]
+    fn apply_partial_only_overrides_present_fields() {
+        let base = Config::default();
+        let partial = PartialConfig {
+            network: Some("local".to_string()),
+            source: None,
+            timeout_secs: None,
+            retry_attempts: Some(1),
+        };
+        let result = apply_partial(base, partial);
+        assert_eq!(result.network, "local");
+        assert_eq!(result.source, Config::default().source);
+        assert_eq!(result.timeout_secs, Config::default().timeout_secs);
+        assert_eq!(result.retry_attempts, 1);
+    }
 }

cargo-budget-report/src/deploy_cache.rs

@@ -59,6 +59,16 @@ pub const CACHE_FILE: &str = ".budget-cache.toml";
 /// unrecognised version is treated as a cold cache rather than an error.
 const CACHE_VERSION: u32 = 1;
 
+/// A single cached deployment record.
+///
+/// All five fields together form a unique deployment snapshot:
+/// - `package` — which workspace crate was deployed.
+/// - `wasm_sha256` — SHA-256 of the compiled `.wasm`; any code change
+///   produces a different hash and must be treated as a miss.
+/// - `network` — Soroban network the contract was deployed to (e.g.
+///   `"testnet"`, `"futurenet"`, `"local"`).
+/// - `source` — Stellar identity / account that paid for the deployment.
+/// - `contract_id` — The `C…` contract address returned by the deploy.
 #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)]
 struct CacheEntry {
     package: String,
@@ -68,29 +78,60 @@ struct CacheEntry {
     contract_id: String,
 }
 
+/// The raw TOML structure that is read from and written to `.budget-cache.toml`.
+///
+/// `version` guards against incompatible schema changes: if the file carries a
+/// version that this build does not understand, [`DeployCache::load`] discards
+/// the contents and starts from an empty cache rather than misinterpreting
+/// stale data.
+///
+/// `entries` is serialised as `[[entry]]` (a TOML array of tables) so the
+/// file remains human-readable and easy to diff.
 #[derive(Debug, Default, Serialize, Deserialize)]
 struct CacheFile {
+    /// Schema version; must equal [`CACHE_VERSION`] for the file to be trusted.
     #[serde(default)]
     version: u32,
+    /// All cached deployment records.  Renamed to `entry` so the TOML key is
+    /// `[[entry]]` rather than `[[entries]]`, matching the documented format.
     #[serde(default, rename = "entry")]
     entries: Vec<CacheEntry>,
 }
 
 /// A loaded deploy cache bound to a path on disk.
+///
+/// Obtain one via [`DeployCache::load`].  Use [`get`](DeployCache::get) to
+/// check for a prior deployment and [`put`](DeployCache::put) to record a new
+/// one.  Call [`save`](DeployCache::save) after all mutations to flush changes
+/// to disk.
 #[derive(Debug)]
 pub struct DeployCache {
+    /// Absolute path to `.budget-cache.toml`.
     path: PathBuf,
+    /// Deserialised cache contents (or a fresh default if the file was absent
+    /// / unreadable / version-mismatch).
     file: CacheFile,
+    /// Set to `true` by [`put`](DeployCache::put) so that
+    /// [`save`](DeployCache::save) can skip the write when nothing changed.
     dirty: bool,
 }
 
-/// Hex SHA-256 of a compiled wasm file's bytes.
+/// Compute the hex-encoded SHA-256 digest of a compiled wasm file.
+///
+/// This is the cache key component that detects code changes: any recompile
+/// that produces different bytes produces a different hash, causing
+/// [`DeployCache::get`] to return `None` and forcing a fresh deployment.
 pub fn wasm_hash(wasm_path: &Path) -> Result<String> {
     let bytes = std::fs::read(wasm_path)
         .with_context(|| format!("failed to read wasm for hashing: {}", wasm_path.display()))?;
     Ok(hex_sha256(&bytes))
 }
 
+/// Return the lowercase hex-encoded SHA-256 of `bytes`.
+///
+/// Each byte of the digest is zero-padded to two hex digits so the output is
+/// always exactly 64 characters long, regardless of leading-zero bytes in the
+/// hash.
 fn hex_sha256(bytes: &[u8]) -> String {
     Sha256::digest(bytes)
         .iter()
@@ -99,26 +140,37 @@ fn hex_sha256(bytes: &[u8]) -> String {
 }
 
 impl DeployCache {
-    /// Load the cache at `dir/.budget-cache.toml`, or an empty cache if the
-    /// file is absent, unreadable, malformed, or a version this build does
-    /// not understand. A cache is best-effort: a broken file must never
-    /// fail a run, only lose its warm entries.
+    /// Load the cache at `dir/.budget-cache.toml`, or start with an empty
+    /// cache if the file is absent, unreadable, malformed, or carries an
+    /// unrecognised schema version.
+    ///
+    /// The cache is **best-effort**: a broken or missing file must never fail
+    /// a measurement run — it simply means every contract is redeployed from
+    /// scratch on that run.
     pub fn load(dir: &Path) -> Self {
         let path = dir.join(CACHE_FILE);
+
+        // Read → parse → version-check, collapsing every failure to `None` so
+        // a cold cache is returned instead of propagating an error.
         let file = std::fs::read_to_string(&path)
             .ok()
             .and_then(|text| toml::from_str::<CacheFile>(&text).ok())
             .filter(|f| f.version == CACHE_VERSION)
             .unwrap_or_default();
+
         DeployCache {
             path,
             file,
             dirty: false,
         }
     }
 
-    /// Look up a previously deployed contract id for this exact
-    /// (wasm hash, network, source) triple.
+    /// Return the contract id for a prior deployment that exactly matches
+    /// `(wasm_sha256, network, source)`, or `None` if no such entry exists.
+    ///
+    /// The lookup is an exact match on all three key components — a different
+    /// wasm hash, a different network, or a different source account all
+    /// produce a miss, forcing a fresh deployment.
     pub fn get(&self, wasm_sha256: &str, network: &str, source: &str) -> Option<&str> {
         self.file
             .entries
@@ -127,9 +179,13 @@ impl DeployCache {
             .map(|e| e.contract_id.as_str())
     }
 
-    /// Record a fresh deployment, replacing any prior entry for the same
-    /// (package, network, source) — a rebuild changes `wasm_sha256`, and the
-    /// old id for that package on that network/source is now dead weight.
+    /// Record a fresh deployment result, replacing any prior entry for the
+    /// same `(package, network, source)` triple.
+    ///
+    /// When the wasm is rebuilt, `wasm_sha256` changes and the old entry for
+    /// that package on that network/source is now stale — retaining it would
+    /// waste space and could never be matched by [`get`](Self::get) anyway
+    /// (because the hash would not match).  Evicting it keeps the file tidy.
     pub fn put(
         &mut self,
         package: &str,
@@ -138,9 +194,13 @@ impl DeployCache {
         source: &str,
         contract_id: &str,
     ) {
+        // Remove any existing entry for this (package, network, source) before
+        // inserting the new one.  This covers both the "first deployment" and
+        // the "rebuild → new hash" cases with the same code path.
         self.file
             .entries
             .retain(|e| !(e.package == package && e.network == network && e.source == source));
+
         self.file.entries.push(CacheEntry {
             package: package.to_string(),
             wasm_sha256: wasm_sha256.to_string(),
@@ -151,9 +211,14 @@ impl DeployCache {
         self.dirty = true;
     }
 
-    /// Persist the cache to disk if it changed since load. Best-effort: a
-    /// write failure is surfaced to the caller but is not itself fatal to a
-    /// completed measurement run.
+    /// Flush the cache to disk if it was modified since the last load or save.
+    ///
+    /// This is a no-op when [`dirty`](Self) is `false`, so it is safe to call
+    /// unconditionally at the end of a run.
+    ///
+    /// A write failure is returned to the caller but is **not** fatal to a
+    /// completed measurement run — the measurements themselves are already
+    /// done; only the cache warm-up for the next run is lost.
     pub fn save(&mut self) -> Result<()> {
         if !self.dirty {
             return Ok(());

cargo-budget-report/src/limit_checks.rs

@@ -1,8 +1,22 @@
 #![allow(dead_code)]
 
-//! Modular limit check logic and boundary helpers for cargo-budget-report.
-
-/// Core bounds checking helper ensuring `value <= max`.
+//! Budget limit enforcement for Soroban resource metering.
+//!
+//! Each public function in this module takes the measured resource value from
+//! a simulation response and compares it against the configured budget limit.
+//! A value that equals the limit is still accepted; only a strict exceedance
+//! returns an error.
+//!
+//! The error strings are deliberately human-readable so they can be surfaced
+//! directly in CLI output and test assertions without further formatting.
+
+/// Assert that `value` does not exceed `max`, returning a descriptive error
+/// when it does.
+///
+/// The generic bound requires `T: PartialOrd + Display` so the same helper
+/// works for both `u32` and `u64` comparisons without duplicating logic.
+/// All callers widen their `u32` inputs to `u64` before calling this function
+/// to keep the comparisons consistent and avoid silent truncation.
 fn check_bounds<T: PartialOrd + std::fmt::Display>(
     value: T,
     max: T,
@@ -15,14 +29,29 @@ fn check_bounds<T: PartialOrd + std::fmt::Display>(
     }
 }
 
+/// Check that the measured CPU instruction count is within `limit`.
+///
+/// `instructions` is widened from `u32` to `u64` before the comparison so
+/// that limits larger than `u32::MAX` (which Soroban allows) are handled
+/// correctly without overflow.
 pub fn check_cpu_instructions(instructions: u32, limit: u64) -> Result<(), String> {
     check_bounds(u64::from(instructions), limit, "CPU Instructions")
 }
 
+/// Check that the number of ledger bytes read by the transaction is within
+/// `limit`.
+///
+/// The widening from `u32` to `u64` mirrors [`check_cpu_instructions`] and
+/// ensures the comparison is lossless even when `limit` exceeds `u32::MAX`.
 pub fn check_read_bytes(bytes: u32, limit: u64) -> Result<(), String> {
     check_bounds(u64::from(bytes), limit, "Read Bytes")
 }
 
+/// Check that the number of ledger bytes written by the transaction is within
+/// `limit`.
+///
+/// The widening from `u32` to `u64` mirrors [`check_cpu_instructions`] and
+/// ensures the comparison is lossless even when `limit` exceeds `u32::MAX`.
 pub fn check_write_bytes(bytes: u32, limit: u64) -> Result<(), String> {
     check_bounds(u64::from(bytes), limit, "Write Bytes")
 }