Skip to content

refactor: extract shared admin-auth and pause-gate helpers #61

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Baskarayelu wants to merge 1 commit into
base: main
Choose a base branch
from
Open

refactor: extract shared admin-auth and pause-gate helpers #61

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
174 changes: 58 additions & 116 deletions contracts/escrow/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -149,6 +149,44 @@ fn write_flag(env: &Env, key: &DataKey, value: bool) {
env.storage().persistent().set(key, &value);
}

// Shared access-control helpers.
//
// Admin-gated entrypoints and the pause gate repeat the same small blocks of
// logic. These free functions centralise that logic so every call site stays
// byte-for-byte identical in behaviour (same error codes, same checks) while
// removing the duplication. They are deliberately plain module-level `fn`s,
// not `Escrow` methods: call them directly (e.g. `require_admin(&env)`), not
// via `Self::`. When adding a new admin-gated entrypoint, start its body with
// `let admin = require_admin(&env);` (or drop the binding when the admin value
// is unused), and gate state-changing entrypoints with `ensure_not_paused`
// at the same position the existing convention dictates.

/// Load the stored admin and require its authorization.
///
/// Panics with [`EscrowError::NotInitialized`] when no admin has been set
/// (i.e. `init` has not run). Otherwise calls `admin.require_auth()` and
/// returns the admin address. This is the canonical admin gate for
/// admin-only entrypoints.
fn require_admin(env: &Env) -> Address {
let admin: Address = env
.storage()
.persistent()
.get(&DataKey::Admin)
.unwrap_or_else(|| panic_with_error!(env, EscrowError::NotInitialized));
admin.require_auth();
admin
}

/// Reject the call if the contract is currently paused.
///
/// Panics with [`EscrowError::ContractPaused`] when the `Paused` flag is set.
/// Mirrors the inline pause check used by state-changing entrypoints.
fn ensure_not_paused(env: &Env) {
if read_flag(env, &DataKey::Paused) {
panic_with_error!(env, EscrowError::ContractPaused);
}
}

#[contract]
pub struct Escrow;

Expand Down Expand Up @@ -195,9 +233,7 @@ impl Escrow {
service_id: Symbol,
requests: u32,
) -> UsageRecord {
if read_flag(&env, &DataKey::Paused) {
panic_with_error!(&env, EscrowError::ContractPaused);
}
ensure_not_paused(&env);
if requests == 0 {
panic_with_error!(&env, EscrowError::RequestsMustBePositive);
}
Expand Down Expand Up @@ -309,12 +345,7 @@ impl Escrow {
/// allowed and means "free service" (still records usage, settles to
/// zero).
pub fn set_service_price(env: Env, service_id: Symbol, price_stroops: i128) {
let admin: Address = env
.storage()
.persistent()
.get(&DataKey::Admin)
.unwrap_or_else(|| panic_with_error!(&env, EscrowError::NotInitialized));
admin.require_auth();
require_admin(&env);
if price_stroops < 0 {
panic_with_error!(&env, EscrowError::RequestsMustBePositive);
}
Expand Down Expand Up @@ -361,15 +392,8 @@ impl Escrow {
/// transfer the returned amount off-chain or via a paired token
/// contract call; this contract intentionally holds no balance.
pub fn settle(env: Env, agent: Address, service_id: Symbol) -> i128 {
if read_flag(&env, &DataKey::Paused) {
panic_with_error!(&env, EscrowError::ContractPaused);
}
let admin: Address = env
.storage()
.persistent()
.get(&DataKey::Admin)
.unwrap_or_else(|| panic_with_error!(&env, EscrowError::NotInitialized));
admin.require_auth();
ensure_not_paused(&env);
require_admin(&env);
let usage_key = DataKey::Usage(agent.clone(), service_id.clone());
let requests: u32 = env.storage().persistent().get(&usage_key).unwrap_or(0);
let price: i128 = env
Expand Down Expand Up @@ -401,12 +425,7 @@ impl Escrow {
/// Admin enables or disables the agent allowlist gate. While
/// disabled, `record_usage` does not consult the per-agent entries.
pub fn set_allowlist_enabled(env: Env, enabled: bool) {
let admin: Address = env
.storage()
.persistent()
.get(&DataKey::Admin)
.unwrap_or_else(|| panic_with_error!(&env, EscrowError::NotInitialized));
admin.require_auth();
require_admin(&env);
write_flag(&env, &DataKey::AllowlistEnabled, enabled);
}

Expand All @@ -422,24 +441,14 @@ impl Escrow {

/// Admin sets the allowlist status for a specific agent.
pub fn set_agent_allowed(env: Env, agent: Address, allowed: bool) {
let admin: Address = env
.storage()
.persistent()
.get(&DataKey::Admin)
.unwrap_or_else(|| panic_with_error!(&env, EscrowError::NotInitialized));
admin.require_auth();
require_admin(&env);
write_flag(&env, &DataKey::AgentAllowed(agent), allowed);
}

/// Admin sets the per-call lower bound on `requests` for batched
/// writes. Pass `0` to disable the floor.
pub fn set_min_requests_per_call(env: Env, min_requests: u32) {
let admin: Address = env
.storage()
.persistent()
.get(&DataKey::Admin)
.unwrap_or_else(|| panic_with_error!(&env, EscrowError::NotInitialized));
admin.require_auth();
require_admin(&env);
env.storage()
.persistent()
.set(&DataKey::MinRequestsPerCall, &min_requests);
Expand All @@ -457,12 +466,7 @@ impl Escrow {
/// Admin sets the per-call upper bound on `requests` accepted by
/// `record_usage`. Pass `u32::MAX` to effectively disable the cap.
pub fn set_max_requests_per_call(env: Env, max_requests: u32) {
let admin: Address = env
.storage()
.persistent()
.get(&DataKey::Admin)
.unwrap_or_else(|| panic_with_error!(&env, EscrowError::NotInitialized));
admin.require_auth();
require_admin(&env);
env.storage()
.persistent()
.set(&DataKey::MaxRequestsPerCall, &max_requests);
Expand All @@ -472,12 +476,7 @@ impl Escrow {
/// `record_usage` rejects unknown services with
/// EscrowError::ServiceNotRegistered.
pub fn set_require_service_registration(env: Env, required: bool) {
let admin: Address = env
.storage()
.persistent()
.get(&DataKey::Admin)
.unwrap_or_else(|| panic_with_error!(&env, EscrowError::NotInitialized));
admin.require_auth();
require_admin(&env);
write_flag(&env, &DataKey::RequireServiceRegistration, required);
}

Expand All @@ -496,12 +495,7 @@ impl Escrow {
/// service are NOT touched — call reset_usage or remove the price
/// separately if a clean wipe is required.
pub fn unregister_service(env: Env, service_id: Symbol) {
let admin: Address = env
.storage()
.persistent()
.get(&DataKey::Admin)
.unwrap_or_else(|| panic_with_error!(&env, EscrowError::NotInitialized));
admin.require_auth();
require_admin(&env);
env.storage()
.persistent()
.remove(&DataKey::ServiceRegistered(service_id));
Expand All @@ -510,24 +504,14 @@ impl Escrow {
/// Register a service so `record_usage` accepts it under strict
/// registration. Admin-gated and idempotent.
pub fn register_service(env: Env, service_id: Symbol) {
let admin: Address = env
.storage()
.persistent()
.get(&DataKey::Admin)
.unwrap_or_else(|| panic_with_error!(&env, EscrowError::NotInitialized));
admin.require_auth();
require_admin(&env);
write_flag(&env, &DataKey::ServiceRegistered(service_id), true);
}

/// Cancel a pending admin transfer. Current admin only. No-op when
/// nothing is pending.
pub fn cancel_admin_transfer(env: Env) {
let admin: Address = env
.storage()
.persistent()
.get(&DataKey::Admin)
.unwrap_or_else(|| panic_with_error!(&env, EscrowError::NotInitialized));
admin.require_auth();
require_admin(&env);
env.storage().persistent().remove(&DataKey::PendingAdmin);
}

Expand Down Expand Up @@ -559,12 +543,7 @@ impl Escrow {
/// from their own key to finish the rotation. Re-proposing
/// overwrites the prior pending entry.
pub fn propose_admin_transfer(env: Env, new_admin: Address) {
let admin: Address = env
.storage()
.persistent()
.get(&DataKey::Admin)
.unwrap_or_else(|| panic_with_error!(&env, EscrowError::NotInitialized));
admin.require_auth();
let admin = require_admin(&env);
if new_admin == admin {
panic_with_error!(&env, EscrowError::InvalidAdminProposal);
}
Expand All @@ -581,12 +560,7 @@ impl Escrow {
/// Resume operations after a previous `pause()`. Admin-gated and
/// idempotent (unpausing an already-unpaused contract is a no-op).
pub fn unpause(env: Env) {
let admin: Address = env
.storage()
.persistent()
.get(&DataKey::Admin)
.unwrap_or_else(|| panic_with_error!(&env, EscrowError::NotInitialized));
admin.require_auth();
require_admin(&env);
write_flag(&env, &DataKey::Paused, false);
env.events().publish((symbol_short!("paused"),), false);
}
Expand All @@ -595,12 +569,7 @@ impl Escrow {
/// panic with [`EscrowError::ContractPaused`]. Admin-gated and
/// idempotent (pausing an already-paused contract is a no-op write).
pub fn pause(env: Env) {
let admin: Address = env
.storage()
.persistent()
.get(&DataKey::Admin)
.unwrap_or_else(|| panic_with_error!(&env, EscrowError::NotInitialized));
admin.require_auth();
require_admin(&env);
write_flag(&env, &DataKey::Paused, true);
env.events().publish((symbol_short!("paused"),), true);
}
Expand All @@ -612,12 +581,7 @@ impl Escrow {
/// new slots are absent, so the migration body itself only stamps
/// the new SchemaVersion; no data fan-out is required.
pub fn migrate_v1_to_v2(env: Env) {
let admin: Address = env
.storage()
.persistent()
.get(&DataKey::Admin)
.unwrap_or_else(|| panic_with_error!(&env, EscrowError::NotInitialized));
admin.require_auth();
require_admin(&env);
let current: u32 = env
.storage()
.persistent()
Expand Down Expand Up @@ -647,25 +611,15 @@ impl Escrow {
/// causes `record_usage` to panic with `ServiceDisabled` for that
/// id; registration and metadata are preserved.
pub fn set_service_disabled(env: Env, service_id: Symbol, disabled: bool) {
let admin: Address = env
.storage()
.persistent()
.get(&DataKey::Admin)
.unwrap_or_else(|| panic_with_error!(&env, EscrowError::NotInitialized));
admin.require_auth();
require_admin(&env);
write_flag(&env, &DataKey::ServiceDisabled(service_id), disabled);
}

/// Admin sets human-readable metadata for a service. Persisted
/// under `DataKey::ServiceMetadata(service_id)`. Description is
/// capped at 256 UTF-8 bytes to bound storage cost.
pub fn set_service_metadata(env: Env, service_id: Symbol, description: String, owner: Address) {
let admin: Address = env
.storage()
.persistent()
.get(&DataKey::Admin)
.unwrap_or_else(|| panic_with_error!(&env, EscrowError::NotInitialized));
admin.require_auth();
require_admin(&env);
env.storage().persistent().set(
&DataKey::ServiceMetadata(service_id),
&ServiceMetadata { description, owner },
Expand All @@ -684,14 +638,7 @@ impl Escrow {
service_id: Symbol,
new_owner: Address,
) {
if env
.storage()
.persistent()
.get(&DataKey::Paused)
.unwrap_or(false)
{
panic_with_error!(&env, EscrowError::ContractPaused);
}
ensure_not_paused(&env);
caller.require_auth();
let admin: Address = env
.storage()
Expand Down Expand Up @@ -724,12 +671,7 @@ impl Escrow {
/// `meta_clr(service_id)` (topic shortened to satisfy the 9-char
/// `symbol_short!` limit).
pub fn clear_service_metadata(env: Env, service_id: Symbol) {
let admin: Address = env
.storage()
.persistent()
.get(&DataKey::Admin)
.unwrap_or_else(|| panic_with_error!(&env, EscrowError::NotInitialized));
admin.require_auth();
require_admin(&env);
env.storage()
.persistent()
.remove(&DataKey::ServiceMetadata(service_id.clone()));
Expand Down
26 changes: 26 additions & 0 deletions contracts/escrow/src/test.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -697,3 +697,29 @@ fn test_pause_pause_unpause_ends_unpaused() {

assert!(!client.is_paused());
}

// Regression coverage for the extracted `require_admin` / `ensure_not_paused`
// helpers (issue #29): the helper refactor must preserve the exact error
// codes and gating behaviour of the previously-inlined blocks.

#[test]
#[should_panic(expected = "Error(Contract, #3)")]
fn test_set_service_price_panics_not_initialized_before_init() {
let env = Env::default();
env.mock_all_auths();
let contract_id = env.register_contract(None, Escrow);
let client = EscrowClient::new(&env, &contract_id);
// No init() call: require_admin must still panic NotInitialized (#3).
client.set_service_price(&Symbol::new(&env, "infer"), &500i128);
}

#[test]
#[should_panic(expected = "Error(Contract, #4)")]
fn test_record_usage_paused_gate_via_helper() {
let env = Env::default();
let (client, _admin) = setup_initialized(&env);
client.pause();
let agent = Address::generate(&env);
// ensure_not_paused must still panic ContractPaused (#4) while paused.
client.record_usage(&agent, &Symbol::new(&env, "infer"), &1u32);
}
Loading