From d34ca7975872ebc18184117899589c3e2334b934 Mon Sep 17 00:00:00 2001 From: Jagadeesh Date: Wed, 29 Jul 2026 02:43:09 +0530 Subject: [PATCH 1/2] fix(test): correct event-count assertion in genuine-transfer test env.events().all() reflects only the most recent contract invocation, not a running total across separate calls (consistent with every other event test in this file). The test compared post-transfer event count against a pre-transfer count captured after a *different* call (set_service_metadata), expecting accumulation that never happens, so it failed on every run regardless of contract behavior. Fixed to assert directly on this call's own event count. --- contracts/escrow/src/test.rs | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/contracts/escrow/src/test.rs b/contracts/escrow/src/test.rs index acc83e0..9bd774d 100644 --- a/contracts/escrow/src/test.rs +++ b/contracts/escrow/src/test.rs @@ -1101,16 +1101,21 @@ fn test_transfer_service_ownership_genuine_transfer_emits_event() { let new_owner = Address::generate(&env); let desc = String::from_str(&env, "inference service"); client.set_service_metadata(&svc, &desc, &owner); - // Capture event count before transfer. - let events_before = env.events().all(); - let count_before = events_before.len(); // Perform genuine transfer. client.transfer_service_ownership(&owner, &svc, &new_owner); - // Exactly one new event (owner_chg). + // env.events().all() reflects only the most recent contract invocation + // (confirmed by every other event test in this file, e.g. + // assert_usage_event_count re-checking a count of 1 after each of + // several sequential record_usage calls), not a running total since + // the start of the test. The prior `set_service_metadata` call's + // `meta_set` event is therefore not present here — only this + // transfer's own event is. Comparing against a pre-call count of + // events from a *different* invocation was the bug; checking this + // call's own event count directly is the fix. let events_after = env.events().all(); assert_eq!( events_after.len(), - count_before + 1, + 1, "genuine transfer must emit exactly one event" ); let (_addr, topics, data) = events_after.last().unwrap(); From bcf4da67c93a33f59c12cabf5fb2bb099caec5d2 Mon Sep 17 00:00:00 2001 From: Jagadeesh Date: Wed, 29 Jul 2026 02:36:43 +0530 Subject: [PATCH 2/2] refactor(fees): extract shared service-usable check into a helper --- contracts/escrow/src/lib.rs | 42 ++++++++++++++++++++---------------- contracts/escrow/src/test.rs | 30 ++++++++++++++++++++++++++ 2 files changed, 54 insertions(+), 18 deletions(-) diff --git a/contracts/escrow/src/lib.rs b/contracts/escrow/src/lib.rs index c647ec9..735c656 100644 --- a/contracts/escrow/src/lib.rs +++ b/contracts/escrow/src/lib.rs @@ -405,6 +405,28 @@ fn write_flag(env: &Env, key: &DataKey, value: bool) { env.storage().persistent().set(key, &value); } +/// Reject a `service_id` that is not currently usable, shared by every +/// entrypoint that either records usage against a service or attaches a +/// price to one (`record_usage`, `set_service_price`). +/// +/// Panics with [`EscrowError::ServiceNotRegistered`] when strict +/// registration is enabled (`RequireServiceRegistration`) and the service +/// has not been registered. Panics with [`EscrowError::ServiceDisabled`] +/// when the service has been explicitly disabled, regardless of the +/// strict-registration setting. +fn ensure_service_usable(env: &Env, service_id: &Symbol) { + // 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())) + { + panic_with_error!(env, EscrowError::ServiceNotRegistered); + } + if read_flag(env, &DataKey::ServiceDisabled(service_id.clone())) { + panic_with_error!(env, EscrowError::ServiceDisabled); + } +} + // Shared access-control helpers. // // Admin-gated entrypoints and the pause gate repeat the same small blocks of @@ -778,16 +800,7 @@ 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())) - { - panic_with_error!(&env, EscrowError::ServiceNotRegistered); - } - if read_flag(&env, &DataKey::ServiceDisabled(service_id.clone())) { - panic_with_error!(&env, EscrowError::ServiceDisabled); - } + ensure_service_usable(&env, &service_id); // Per-agent blocklist takes precedence over the allowlist: a blocked // agent is rejected even if also allow-listed. if read_flag(&env, &DataKey::AgentBlocked(agent.clone())) { @@ -1243,14 +1256,7 @@ impl Escrow { if price_stroops < 0 { panic_with_error!(&env, EscrowError::RequestsMustBePositive); } - if read_flag(&env, &DataKey::RequireServiceRegistration) - && !read_flag(&env, &DataKey::ServiceRegistered(service_id.clone())) - { - panic_with_error!(&env, EscrowError::ServiceNotRegistered); - } - if read_flag(&env, &DataKey::ServiceDisabled(service_id.clone())) { - panic_with_error!(&env, EscrowError::ServiceDisabled); - } + ensure_service_usable(&env, &service_id); // Global price-bounds check. Defaults: floor = 0, ceiling = i128::MAX. // If a floor above 0 is configured, a price of 0 ("free service") is // explicitly **forbidden** — the admin must lower the floor to 0 first diff --git a/contracts/escrow/src/test.rs b/contracts/escrow/src/test.rs index 9bd774d..095fa6c 100644 --- a/contracts/escrow/src/test.rs +++ b/contracts/escrow/src/test.rs @@ -3471,6 +3471,36 @@ fn test_set_price_flag_toggled_mid_life() { client.set_service_price(&svc, &200i128); // strict + unregistered: rejected } +/// `record_usage` and `set_service_price` share one gate +/// (`ensure_service_usable`, extracted from what used to be two duplicated +/// inline checks). This locks in that the two entrypoints reject a +/// disabled service with the identical error, regardless of which one is +/// called first — proving the extraction did not desync their behavior. +#[test] +#[should_panic(expected = "Error(Contract, #12)")] +fn test_record_usage_and_set_service_price_share_disabled_gate() { + let env = Env::default(); + let (client, admin) = setup_initialized(&env); + let svc = Symbol::new(&env, "infer"); + let agent = Address::generate(&env); + client.set_service_disabled(&svc, &true); + // set_service_price already rejects (proven by test_set_price_rejects_disabled_service); + // record_usage must reject with the identical error code via the same helper. + client.record_usage(&agent, &svc, &1u32); +} +#[test] +#[should_panic(expected = "Error(Contract, #7)")] +fn test_record_usage_and_set_service_price_share_registration_gate() { + let env = Env::default(); + let (client, admin) = setup_initialized(&env); + let svc = Symbol::new(&env, "phantom"); + let agent = Address::generate(&env); + client.set_require_service_registration(&true); + // test_set_price_strict_rejects_unregistered_service proves the price side; + // record_usage must reject with the identical error code via the same helper. + client.record_usage(&agent, &svc, &1u32); +} + /// A service owner can settle their own service via `settle_all`. #[test] fn test_owner_can_settle_own_service() {