test(host-function-contract): harden fixture, tests, and harness docs

amm-pool-contract/src/lib.rs

@@ -78,13 +78,13 @@ impl RelayContract {
         }
     }
 }
+
 /// Internal module for AMM pool logic.
 ///
 /// Extracts the complex constant-product calculations and storage updates
 /// from the main contract implementation to improve readability and testability.
 mod pool_logic;
 
-
 #[contract]
 pub struct ConstantProductPool;
 

host-function-contract/src/lib.rs

@@ -11,15 +11,54 @@
 //! host-function overhead from other billing dimensions (such as read/write bytes or VM instructions).
 //!
 //! It also hosts the Map-operation fixtures (`map_insert`, `map_get`, `map_remove`,
-//! `map_iterate`), which isolate Soroban [`Map`] host calls without storage, event, or
-//! arithmetic side-effects, for the local-vs-network Map cost-gap measurement.
+//! `map_iterate`), which isolate Soroban [`Map`](soroban_sdk::Map) host calls without
+//! storage, event, or arithmetic side-effects, for the local-vs-network Map
+//! cost-gap measurement.
 //!
 //! See `README.md` and `MEASUREMENTS.md` at the repository root for detailed methodology
 //! and captured figures.
+//!
+//! ## Module layout
+//!
+//! Each public entry point below performs exactly one repeated host-function
+//! operation, so a measurement of that entry point isolates that operation. The
+//! reusable Map setup is factored into the private `support` submodule so the
+//! four `map_*` entry points do not each repeat the same seeding loop (#718).
+//! The public contract interface is unchanged by that factoring.
 
 #![no_std]
 
-use soroban_sdk::{contract, contractimpl, Bytes, Env, Map};
+use soroban_sdk::{contract, contractimpl, Bytes, Env};
+
+/// Internal helper components shared by the contract entry points below.
+///
+/// Factoring this logic out of the `#[contractimpl]` block keeps each public
+/// entry point short and focused on the single host-function operation it
+/// exists to measure, and gives the shared setup one place to be documented,
+/// tested, and changed (#718).
+mod support {
+    use soroban_sdk::{Env, Map};
+
+    /// Builds a fresh `Map<u32, u32>` seeded with `size` identity entries.
+    ///
+    /// Entry `i` maps the key `i` to the value `i`, so the result is
+    /// deterministic and every key is distinct. Building the map performs
+    /// exactly `size` `Map::set` host calls.
+    ///
+    /// The `map_get`, `map_remove`, and `map_iterate` fixtures each build this
+    /// identical map before measuring their own operation, so subtracting the
+    /// cost of `map_insert(size)` from theirs yields the marginal cost of the
+    /// operation under measurement.
+    ///
+    /// A `size` of `0` returns an empty map without invoking any host call.
+    pub(super) fn seed_identity_map(env: &Env, size: u32) -> Map<u32, u32> {
+        let mut map = Map::new(env);
+        for i in 0..size {
+            map.set(i, i);
+        }
+        map
+    }
+}
 
 /// Benchmark contract fixture for measuring the gap between local budget estimates
 /// and live network simulation figures for repeated host-function calls.
@@ -67,7 +106,7 @@ impl HostFunctionBenchmark {
     /// Hashes a small input buffer with SHA-256 for `iterations` count and
     /// returns the number of iterations completed.
     ///
-    /// Each iteration allocates an 8-byte `Bytes` value and passes it through
+    /// Each iteration passes the same pre-built input through
     /// `env.crypto().sha256()`, exercising the cryptographic host function
     /// category. The return value prevents dead-code elimination while
     /// keeping the function free of storage side-effects.
@@ -102,11 +141,8 @@ impl HostFunctionBenchmark {
     /// arithmetic side-effects. The return value prevents dead-code
     /// elimination. `size` is the map size / insert count.
     pub fn map_insert(env: Env, size: u32) -> u32 {
-        let mut m: Map<u32, u32> = Map::new(&env);
-
-        for i in 0..size {
-            m.set(i, i);
-        }
+        // The seeding loop *is* the measurement; the map itself is discarded.
+        let _map = support::seed_identity_map(&env, size);
 
         size
     }
@@ -118,13 +154,10 @@ impl HostFunctionBenchmark {
     /// the same `size`, so the marginal cost of the get loop is obtained by
     /// subtracting `map_insert(size)`. This isolates the cost of Map lookups.
     pub fn map_get(env: Env, size: u32) -> u32 {
-        let mut m: Map<u32, u32> = Map::new(&env);
-        for i in 0..size {
-            m.set(i, i);
-        }
+        let map = support::seed_identity_map(&env, size);
 
         for i in 0..size {
-            let _v = m.get(i);
+            let _v = map.get(i);
         }
 
         size
@@ -137,13 +170,10 @@ impl HostFunctionBenchmark {
     /// the same `size`, so the marginal cost of the remove loop is obtained by
     /// subtracting `map_insert(size)`. This isolates the cost of Map removals.
     pub fn map_remove(env: Env, size: u32) -> u32 {
-        let mut m: Map<u32, u32> = Map::new(&env);
-        for i in 0..size {
-            m.set(i, i);
-        }
+        let mut map = support::seed_identity_map(&env, size);
 
         for i in 0..size {
-            let _ = m.remove(i);
+            let _ = map.remove(i);
         }
 
         size
@@ -156,14 +186,14 @@ impl HostFunctionBenchmark {
     /// the same `size`, so the marginal cost of the iteration is obtained by
     /// subtracting `map_insert(size)`. This isolates the cost of Map
     /// iteration.
+    ///
+    /// Because the map is seeded with identity entries, the returned sum is
+    /// well-defined for tests (`0 + 1 + … + (size - 1)`).
     pub fn map_iterate(env: Env, size: u32) -> u32 {
-        let mut m: Map<u32, u32> = Map::new(&env);
-        for i in 0..size {
-            m.set(i, i);
-        }
+        let map = support::seed_identity_map(&env, size);
 
         let mut sum: u32 = 0;
-        for (_k, v) in m.iter() {
+        for (_k, v) in map.iter() {
             sum = sum.wrapping_add(v);
         }
 

host-function-contract/tests/common/mod.rs

@@ -1,14 +1,35 @@
-//! Shared helper for the host-function-contract integration tests.
+//! Shared helper for the host-function-contract integration tests (#480, #717).
 //!
-//! The tests exercise the contract as WASM — not just as native Rust — so the
-//! fixture is verified to build for the `wasm32v1-none` target used by the
-//! rest of the workspace (issue #480). This module resolves and loads the
-//! pre-built `host_function_contract.wasm` artifact, building it on demand
-//! when it is missing or stale so the tests run as part of `cargo test
-//! --workspace` with no extra setup.
+//! # Why this module exists
+//!
+//! The tests that exercise the benchmark fixture do not only run it as native
+//! Rust: they also register the pre-built `host_function_contract.wasm` artifact
+//! into the test environment, so the fixture is verified against the same
+//! `wasm32v1-none` target the rest of the workspace uses for budget
+//! measurement. Trusting a stale or hand-built artifact would let the tests
+//! pass against code that is no longer in the tree, so this module resolves the
+//! artifact path and loads the bytes, building the artifact on demand whenever
+//! it is missing or older than the contract sources.
+//!
+//! # How it fits together
+//!
+//! 1. [`workspace_root`] locates the workspace root from this crate's
+//!    `CARGO_MANIFEST_DIR` (the crate sits directly under the root).
+//! 2. [`wasm_path`] turns a target triple into the expected artifact path under
+//!    `target/<target>/release/`, honouring `CARGO_TARGET_DIR` when set.
+//! 3. [`artifact_needs_rebuild`] compares the artifact's mtime against the
+//!    contract sources in [`STALE_SOURCES`].
+//! 4. [`load_contract_wasm`] serialises the check-then-build sequence under
+//!    [`BUILD_LOCK`] and returns the artifact bytes.
 //!
 //! The artifact path honours `CARGO_TARGET_DIR` when it is set and otherwise
-//! falls back to the workspace-root `target/` directory.
+//! falls back to the workspace-root `target/` directory, so the helper works
+//! from any test working directory and with any custom target-dir setup.
+
+// Each integration-test binary in this directory declares `mod common;` and
+// uses only the helpers it needs, so the rest can look unused to a single
+// binary. Same idiom as `amm-pool-contract/tests/common/mod.rs`.
+#![allow(dead_code)]
 
 use std::path::{Path, PathBuf};
 use std::process::Command;
@@ -25,9 +46,16 @@ const STALE_SOURCES: &[&str] = &["src/lib.rs", "Cargo.toml"];
 
 /// Serialises the check-then-build sequence so tests running in parallel
 /// inside one test binary never race on the same artifact.
+///
+/// Without this, two tests could both observe a missing artifact and spawn
+/// concurrent `cargo build` invocations, or one could read the file while the
+/// other is truncating it.
 static BUILD_LOCK: Mutex<()> = Mutex::new(());
 
 /// The workspace root: this crate's manifest directory sits directly under it.
+///
+/// `CARGO_MANIFEST_DIR` is expanded at compile time, so this requires no
+/// runtime environment lookup and cannot fail for a missing variable.
 fn workspace_root() -> &'static Path {
     Path::new(env!("CARGO_MANIFEST_DIR"))
         .parent()
@@ -36,7 +64,12 @@ fn workspace_root() -> &'static Path {
 
 /// Absolute path to the `host_function_contract.wasm` artifact for `target`,
 /// honouring `CARGO_TARGET_DIR` when it is set.
+///
+/// The returned path is not guaranteed to exist: callers that need the bytes
+/// should use [`load_contract_wasm`] instead.
 pub fn wasm_path(target: &str) -> PathBuf {
+    // Mirror Cargo's own precedence: an explicit `CARGO_TARGET_DIR` wins,
+    // otherwise the workspace-root `target/` directory is used.
     let target_dir = std::env::var_os("CARGO_TARGET_DIR")
         .map(PathBuf::from)
         .unwrap_or_else(|| workspace_root().join("target"));
@@ -46,6 +79,11 @@ pub fn wasm_path(target: &str) -> PathBuf {
 /// Loads the `host_function_contract.wasm` bytes for `target`, building the
 /// artifact first when it is missing or stale.
 ///
+/// The check and the build happen under [`BUILD_LOCK`], so concurrent callers
+/// observe either a complete artifact or a single build, never a partial write.
+///
+/// # Panics
+///
 /// Panics with a clear message if the build or the read fails.
 pub fn load_contract_wasm(target: &str) -> Vec<u8> {
     let _guard = BUILD_LOCK.lock().unwrap_or_else(PoisonError::into_inner);
@@ -64,11 +102,18 @@ pub fn load_contract_wasm(target: &str) -> Vec<u8> {
 
 /// Whether the artifact at `wasm` is missing or older than the contract
 /// sources it was built from.
+///
+/// A missing artifact or an artifact whose mtime cannot be read is treated as
+/// stale, so the caller rebuilds. A source whose mtime cannot be read is
+/// skipped: an unreadable source cannot make a readable artifact stale.
 fn artifact_needs_rebuild(wasm: &Path) -> bool {
     let Ok(wasm_meta) = std::fs::metadata(wasm) else {
+        // Artifact does not exist yet — must build.
         return true;
     };
     let Ok(wasm_mtime) = wasm_meta.modified() else {
+        // The platform cannot report an mtime — rebuild rather than risk
+        // trusting an artifact we cannot compare.
         return true;
     };
     STALE_SOURCES.iter().any(|source| {
@@ -82,6 +127,14 @@ fn artifact_needs_rebuild(wasm: &Path) -> bool {
 }
 
 /// Builds the `host-function-contract` cdylib for `target` in release mode.
+///
+/// Uses the `CARGO` executable Cargo set for this build (`env!("CARGO")`)
+/// rather than assuming `cargo` is on `PATH`, so the correct toolchain is used
+/// even when the tests are launched through a rustup proxy.
+///
+/// # Panics
+///
+/// Panics if the build command cannot be spawned or exits unsuccessfully.
 fn build_contract_wasm(target: &str) {
     let status = Command::new(env!("CARGO"))
         .args([