refactor: reduce redundant storage reads in record_usage validation

contracts/escrow/src/lib.rs

@@ -195,12 +195,37 @@ impl Escrow {
         service_id: Symbol,
         requests: u32,
     ) -> UsageRecord {
+        // ---- Validation chain (order is part of the public contract) ----
+        //
+        // Errors MUST fire in this fixed precedence so that client SDKs and
+        // off-chain settlement loops can rely on a stable failure ordering:
+        //
+        //   1. Paused            -> #4  ContractPaused
+        //   2. requests == 0     -> #2  RequestsMustBePositive
+        //   3. requests > max    -> #8  RequestsExceedsMaxPerCall
+        //   4. requests < min    -> #9  RequestsBelowMinPerCall
+        //   5. registration      -> #7  ServiceNotRegistered
+        //   6. disabled          -> #12 ServiceDisabled
+        //   7. allowlist         -> #10 AgentNotAllowed
+        //
+        // Read-count note (before/after): the storage reads performed here are
+        // unchanged in the worst case, but several are *conditionally gated* so
+        // they never execute when their controlling flag is off:
+        //   - ServiceRegistered is only read when RequireServiceRegistration is
+        //     true (short-circuited via `&&`).
+        //   - AgentAllowed is only read when AllowlistEnabled is true (ditto).
+        // The Paused flag, the max/min caps, and ServiceDisabled are always
+        // read (unconditional gates). Each key is read at most once: the
+        // max/min caps are cached in locals below, and the usage counter (read
+        // further down) is read exactly once. No value is read twice.
+        // -------------------------------------------------------------------
         if read_flag(&env, &DataKey::Paused) {
             panic_with_error!(&env, EscrowError::ContractPaused);
         }
         if requests == 0 {
             panic_with_error!(&env, EscrowError::RequestsMustBePositive);
         }
+        // Cached: read once, compared once. Defaults to u32::MAX (no cap).
         let max_per_call: u32 = env
             .storage()
             .persistent()
@@ -209,6 +234,7 @@ impl Escrow {
         if requests > max_per_call {
             panic_with_error!(&env, EscrowError::RequestsExceedsMaxPerCall);
         }
+        // Cached: read once, compared once. Defaults to 0 (no floor).
         let min_per_call: u32 = env
             .storage()
             .persistent()
@@ -217,6 +243,8 @@ impl Escrow {
         if requests < min_per_call {
             panic_with_error!(&env, EscrowError::RequestsBelowMinPerCall);
         }
+        // Conditional read: ServiceRegistered is only touched when strict
+        // registration is enabled (the `&&` short-circuits otherwise).
         if read_flag(&env, &DataKey::RequireServiceRegistration)
             && !read_flag(&env, &DataKey::ServiceRegistered(service_id.clone()))
         {
@@ -225,6 +253,8 @@ impl Escrow {
         if read_flag(&env, &DataKey::ServiceDisabled(service_id.clone())) {
             panic_with_error!(&env, EscrowError::ServiceDisabled);
         }
+        // Conditional read: AgentAllowed is only touched when the allowlist is
+        // enabled (the `&&` short-circuits otherwise).
         if read_flag(&env, &DataKey::AllowlistEnabled)
             && !read_flag(&env, &DataKey::AgentAllowed(agent.clone()))
         {

contracts/escrow/src/test.rs

@@ -697,3 +697,171 @@ fn test_pause_pause_unpause_ends_unpaused() {
 
     assert!(!client.is_paused());
 }
+
+// ---- record_usage validation-chain error precedence -------------------
+// These assert that the fixed error ordering
+//   Paused(#4) -> ZeroRequests(#2) -> Max(#8) -> Min(#9)
+//   -> Registration(#7) -> Disabled(#12) -> Allowlist(#10)
+// is preserved after the read-ordering refactor, and that each gate still
+// fires on its own trigger.
+
+#[test]
+#[should_panic(expected = "Error(Contract, #4)")]
+fn test_record_usage_paused_beats_zero_requests() {
+    // Paused (#4) must win even when requests == 0 (which would be #2).
+    let env = Env::default();
+    let (client, _admin) = setup_initialized(&env);
+    client.pause();
+    let agent = Address::generate(&env);
+    let service_id = Symbol::new(&env, "weather_api");
+    client.record_usage(&agent, &service_id, &0u32);
+}
+
+#[test]
+#[should_panic(expected = "Error(Contract, #2)")]
+fn test_record_usage_zero_requests_beats_max() {
+    // Zero-requests (#2) must win over the max cap (#8): with max=5 and
+    // requests=0, the zero check fires first.
+    let env = Env::default();
+    let (client, _admin) = setup_initialized(&env);
+    client.set_max_requests_per_call(&5u32);
+    let agent = Address::generate(&env);
+    let service_id = Symbol::new(&env, "weather_api");
+    client.record_usage(&agent, &service_id, &0u32);
+}
+
+#[test]
+#[should_panic(expected = "Error(Contract, #8)")]
+fn test_record_usage_max_beats_min() {
+    // Max (#8) must win over min (#9): with max=5 and min=10 (an
+    // inconsistent config), a request above max trips #8 first.
+    let env = Env::default();
+    let (client, _admin) = setup_initialized(&env);
+    client.set_max_requests_per_call(&5u32);
+    client.set_min_requests_per_call(&10u32);
+    let agent = Address::generate(&env);
+    let service_id = Symbol::new(&env, "weather_api");
+    client.record_usage(&agent, &service_id, &6u32);
+}
+
+#[test]
+#[should_panic(expected = "Error(Contract, #9)")]
+fn test_record_usage_min_beats_registration() {
+    // Min (#9) must win over the registration gate (#7): with min=10 and
+    // strict registration required (service unregistered), a below-min
+    // request trips #9 before #7.
+    let env = Env::default();
+    let (client, _admin) = setup_initialized(&env);
+    client.set_min_requests_per_call(&10u32);
+    client.set_require_service_registration(&true);
+    let agent = Address::generate(&env);
+    let service_id = Symbol::new(&env, "weather_api");
+    client.record_usage(&agent, &service_id, &3u32);
+}
+
+#[test]
+#[should_panic(expected = "Error(Contract, #7)")]
+fn test_record_usage_registration_beats_disabled() {
+    // Registration (#7) must win over disabled (#12): require registration,
+    // leave the service unregistered, and also disable it. #7 fires first.
+    let env = Env::default();
+    let (client, _admin) = setup_initialized(&env);
+    client.set_require_service_registration(&true);
+    let service_id = Symbol::new(&env, "weather_api");
+    client.set_service_disabled(&service_id, &true);
+    let agent = Address::generate(&env);
+    client.record_usage(&agent, &service_id, &5u32);
+}
+
+#[test]
+#[should_panic(expected = "Error(Contract, #12)")]
+fn test_record_usage_disabled_beats_allowlist() {
+    // Disabled (#12) must win over the allowlist (#10): disable a registered
+    // service and enable a (non-matching) allowlist. #12 fires first.
+    let env = Env::default();
+    let (client, _admin) = setup_initialized(&env);
+    let service_id = Symbol::new(&env, "weather_api");
+    client.register_service(&service_id);
+    client.set_service_disabled(&service_id, &true);
+    client.set_allowlist_enabled(&true);
+    let agent = Address::generate(&env);
+    client.record_usage(&agent, &service_id, &5u32);
+}
+
+#[test]
+#[should_panic(expected = "Error(Contract, #10)")]
+fn test_record_usage_allowlist_fires_when_enabled_and_not_allowed() {
+    // Allowlist (#10) fires when enabled and the agent is not allowed.
+    let env = Env::default();
+    let (client, _admin) = setup_initialized(&env);
+    client.set_allowlist_enabled(&true);
+    let agent = Address::generate(&env);
+    let service_id = Symbol::new(&env, "weather_api");
+    client.record_usage(&agent, &service_id, &5u32);
+}
+
+#[test]
+#[should_panic(expected = "Error(Contract, #7)")]
+fn test_record_usage_registration_fires_when_required_and_unregistered() {
+    // Registration (#7) fires when required and the service is unregistered.
+    let env = Env::default();
+    let (client, _admin) = setup_initialized(&env);
+    client.set_require_service_registration(&true);
+    let agent = Address::generate(&env);
+    let service_id = Symbol::new(&env, "weather_api");
+    client.record_usage(&agent, &service_id, &5u32);
+}
+
+#[test]
+#[should_panic(expected = "Error(Contract, #12)")]
+fn test_record_usage_disabled_fires_when_service_disabled() {
+    // Disabled (#12) fires when the service is disabled.
+    let env = Env::default();
+    let (client, _admin) = setup_initialized(&env);
+    let service_id = Symbol::new(&env, "weather_api");
+    client.set_service_disabled(&service_id, &true);
+    let agent = Address::generate(&env);
+    client.record_usage(&agent, &service_id, &5u32);
+}
+
+#[test]
+#[should_panic(expected = "Error(Contract, #8)")]
+fn test_record_usage_max_fires_above_cap() {
+    // Max (#8) fires when requests exceed the configured cap.
+    let env = Env::default();
+    let (client, _admin) = setup_initialized(&env);
+    client.set_max_requests_per_call(&5u32);
+    let agent = Address::generate(&env);
+    let service_id = Symbol::new(&env, "weather_api");
+    client.record_usage(&agent, &service_id, &6u32);
+}
+
+#[test]
+#[should_panic(expected = "Error(Contract, #9)")]
+fn test_record_usage_min_fires_below_floor() {
+    // Min (#9) fires when requests fall below the configured floor.
+    let env = Env::default();
+    let (client, _admin) = setup_initialized(&env);
+    client.set_min_requests_per_call(&10u32);
+    let agent = Address::generate(&env);
+    let service_id = Symbol::new(&env, "weather_api");
+    client.record_usage(&agent, &service_id, &3u32);
+}
+
+#[test]
+fn test_record_usage_passes_all_gates_when_satisfied() {
+    // Sanity: with every gate enabled and satisfied, record_usage succeeds.
+    let env = Env::default();
+    let (client, _admin) = setup_initialized(&env);
+    let service_id = Symbol::new(&env, "weather_api");
+    let agent = Address::generate(&env);
+    client.set_max_requests_per_call(&100u32);
+    client.set_min_requests_per_call(&1u32);
+    client.set_require_service_registration(&true);
+    client.register_service(&service_id);
+    client.set_allowlist_enabled(&true);
+    client.set_agent_allowed(&agent, &true);
+
+    let record = client.record_usage(&agent, &service_id, &5u32);
+    assert_eq!(record.requests, 5);
+}