From 9c6ecb66d77e27a51d75a60c0b5e87e1fcc6646f Mon Sep 17 00:00:00 2001 From: onahiOMOTI Date: Mon, 30 Mar 2026 10:44:34 +0000 Subject: [PATCH] refactor(common): extract shared require_admin pattern into router-common crate - Created new workspace crate `contracts/router-common` - Added `require_admin!` and `require_admin_simple!` macros to eliminate code duplication - Updated all contracts to depend on `router-common` - Replaced repetitive admin check boilerplate with the shared macro - Improves maintainability and reduces risk of inconsistent admin logic Closes #128 --- Cargo.toml | 1 + contracts/router-access/Cargo.toml | 1 + contracts/router-common/src/Cargo.toml | 7 +++++ contracts/router-common/src/lib.rs | 37 ++++++++++++++++++++++++++ contracts/router-core/Cargo.toml | 1 + contracts/router-core/src/lib.rs | 30 ++++++++++++++------- contracts/router-middleware/Cargo.toml | 1 + contracts/router-middleware/src/lib.rs | 32 +++++++++++----------- contracts/router-multicall/Cargo.toml | 1 + contracts/router-registry/Cargo.toml | 1 + contracts/router-registry/src/lib.rs | 30 ++++++++++++++------- contracts/router-timelock/Cargo.toml | 1 + contracts/router-timelock/src/lib.rs | 26 +++++++++++++----- 13 files changed, 128 insertions(+), 41 deletions(-) create mode 100644 contracts/router-common/src/Cargo.toml create mode 100644 contracts/router-common/src/lib.rs diff --git a/Cargo.toml b/Cargo.toml index ef5d796..5881580 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -2,6 +2,7 @@ name = "stellar-router" resolver = "2" members = [ + "contracts/router-common", "contracts/router-core", "contracts/router-registry", "contracts/router-access", diff --git a/contracts/router-access/Cargo.toml b/contracts/router-access/Cargo.toml index 56a8e4b..7678ccf 100644 --- a/contracts/router-access/Cargo.toml +++ b/contracts/router-access/Cargo.toml @@ -8,6 +8,7 @@ crate-type = ["cdylib", "rlib"] [dependencies] soroban-sdk = { workspace = true } +router-common = { path = "../router-common" } [dev-dependencies] soroban-sdk = { version = "21.7.6", features = ["testutils"] } diff --git a/contracts/router-common/src/Cargo.toml b/contracts/router-common/src/Cargo.toml new file mode 100644 index 0000000..f584baf --- /dev/null +++ b/contracts/router-common/src/Cargo.toml @@ -0,0 +1,7 @@ +[package] +name = "router-common" +version = "0.1.0" +edition = "2021" + +[dependencies] +soroban-sdk = { version = "22", default-features = false } # Match your other contracts' version \ No newline at end of file diff --git a/contracts/router-common/src/lib.rs b/contracts/router-common/src/lib.rs new file mode 100644 index 0000000..d368bbd --- /dev/null +++ b/contracts/router-common/src/lib.rs @@ -0,0 +1,37 @@ +#![no_std] + +use soroban_sdk::{Address, Env, Symbol}; + +/// Macro to require admin with custom error types. +/// +/// This eliminates the repetitive `require_admin` / `require_super_admin` boilerplate +/// across all router contracts while allowing each contract to use its own error enum. +#[macro_export] +macro_rules! require_admin { + ($env:expr, $caller:expr, $data_key:expr, $not_init_err:expr, $unauth_err:expr) => {{ + let admin: soroban_sdk::Address = $env + .storage() + .instance() + .get($data_key) + .ok_or($not_init_err)?; + + if &admin != $caller { + return Err($unauth_err); + } + Ok(()) + }}; +} + +/// Convenience version when using DataKey::Admin and standard error variants +#[macro_export] +macro_rules! require_admin_simple { + ($env:expr, $caller:expr, $data_key:expr, $error_type:ty) => { + $crate::require_admin!( + $env, + $caller, + $data_key, + <$error_type>::NotInitialized, + <$error_type>::Unauthorized + ) + }; +} \ No newline at end of file diff --git a/contracts/router-core/Cargo.toml b/contracts/router-core/Cargo.toml index 82bfb18..d913ca5 100644 --- a/contracts/router-core/Cargo.toml +++ b/contracts/router-core/Cargo.toml @@ -8,6 +8,7 @@ crate-type = ["cdylib", "rlib"] [dependencies] soroban-sdk = { workspace = true } +router-common = { path = "../router-common" } [dev-dependencies] soroban-sdk = { version = "21.7.6", features = ["testutils"] } diff --git a/contracts/router-core/src/lib.rs b/contracts/router-core/src/lib.rs index c87f3e4..1a88ff3 100644 --- a/contracts/router-core/src/lib.rs +++ b/contracts/router-core/src/lib.rs @@ -628,16 +628,26 @@ impl RouterCore { /// # Errors /// * [`RouterError::Unauthorized`] — if `current` is not the admin. /// * [`RouterError::NotInitialized`] — if the contract has not been initialized. - pub fn transfer_admin(env: Env, current: Address, new_admin: Address) -> Result<(), RouterError> { - current.require_auth(); - Self::require_admin(&env, ¤t)?; - env.storage().instance().set(&DataKey::Admin, &new_admin); - env.events().publish( - (Symbol::new(&env, "admin_transferred"),), - (current, new_admin), - ); - Ok(()) - } + pub fn transfer_admin(env: Env, current: Address, new_admin: Address) -> Result<(), MiddlewareError> { + current.require_auth(); + + // One-liner using the shared macro + router_common::require_admin_simple!( + &env, + ¤t, + &DataKey::Admin, + MiddlewareError + )?; + + env.storage().instance().set(&DataKey::Admin, &new_admin); + + env.events().publish( + (Symbol::new(&env, "admin_transferred"),), + (current, new_admin), + ); + + Ok(()) +} /// Returns all currently registered route names as a vector of strings. /// diff --git a/contracts/router-middleware/Cargo.toml b/contracts/router-middleware/Cargo.toml index 3d1ef54..e07f41c 100644 --- a/contracts/router-middleware/Cargo.toml +++ b/contracts/router-middleware/Cargo.toml @@ -8,6 +8,7 @@ crate-type = ["cdylib", "rlib"] [dependencies] soroban-sdk = { workspace = true } +router-common = { path = "../router-common" } [dev-dependencies] soroban-sdk = { version = "21.7.6", features = ["testutils"] } diff --git a/contracts/router-middleware/src/lib.rs b/contracts/router-middleware/src/lib.rs index a8fc485..35c2ae8 100644 --- a/contracts/router-middleware/src/lib.rs +++ b/contracts/router-middleware/src/lib.rs @@ -571,24 +571,26 @@ impl RouterMiddleware { /// # Errors /// * [`MiddlewareError::Unauthorized`] — if `current` is not the admin. /// * [`MiddlewareError::NotInitialized`] — if the contract has not been initialized. - pub fn transfer_admin( - env: Env, - current: Address, - new_admin: Address, - ) -> Result<(), MiddlewareError> { - current.require_auth(); - Self::require_admin(&env, ¤t)?; + pub fn transfer_admin(env: Env, current: Address, new_admin: Address) -> Result<(), MiddlewareError> { + current.require_auth(); - env.storage().instance().set(&DataKey::Admin, &new_admin); + // One-liner using the shared macro + router_common::require_admin_simple!( + &env, + ¤t, + &DataKey::Admin, + MiddlewareError + )?; - // ← FIXED: Emit event to match other contracts (router-access, router-core, etc.) - env.events().publish( - (Symbol::new(&env, "admin_transferred"),), - (current.clone(), new_admin.clone()), - ); + env.storage().instance().set(&DataKey::Admin, &new_admin); - Ok(()) - } + env.events().publish( + (Symbol::new(&env, "admin_transferred"),), + (current, new_admin), + ); + + Ok(()) +} // ── Helpers ─────────────────────────────────────────────────────────────── diff --git a/contracts/router-multicall/Cargo.toml b/contracts/router-multicall/Cargo.toml index 210d365..385d816 100644 --- a/contracts/router-multicall/Cargo.toml +++ b/contracts/router-multicall/Cargo.toml @@ -8,6 +8,7 @@ crate-type = ["cdylib", "rlib"] [dependencies] soroban-sdk = { workspace = true } +router-common = { path = "../router-common" } [dev-dependencies] soroban-sdk = { version = "21.7.6", features = ["testutils"] } diff --git a/contracts/router-registry/Cargo.toml b/contracts/router-registry/Cargo.toml index 3dc192f..bb9e667 100644 --- a/contracts/router-registry/Cargo.toml +++ b/contracts/router-registry/Cargo.toml @@ -8,6 +8,7 @@ crate-type = ["cdylib", "rlib"] [dependencies] soroban-sdk = { workspace = true } +router-common = { path = "../router-common" } [dev-dependencies] soroban-sdk = { version = "21.7.6", features = ["testutils"] } diff --git a/contracts/router-registry/src/lib.rs b/contracts/router-registry/src/lib.rs index ad833f0..3885c24 100644 --- a/contracts/router-registry/src/lib.rs +++ b/contracts/router-registry/src/lib.rs @@ -356,16 +356,26 @@ impl RouterRegistry { /// # Errors /// * [`RegistryError::Unauthorized`] — if `current` is not the admin. /// * [`RegistryError::NotInitialized`] — if the contract has not been initialized. - pub fn transfer_admin(env: Env, current: Address, new_admin: Address) -> Result<(), RegistryError> { - current.require_auth(); - Self::require_admin(&env, ¤t)?; - env.storage().instance().set(&DataKey::Admin, &new_admin); - env.events().publish( - (Symbol::new(&env, "admin_transferred"),), - (current, new_admin), - ); - Ok(()) - } + pub fn transfer_admin(env: Env, current: Address, new_admin: Address) -> Result<(), MiddlewareError> { + current.require_auth(); + + // One-liner using the shared macro + router_common::require_admin_simple!( + &env, + ¤t, + &DataKey::Admin, + MiddlewareError + )?; + + env.storage().instance().set(&DataKey::Admin, &new_admin); + + env.events().publish( + (Symbol::new(&env, "admin_transferred"),), + (current, new_admin), + ); + + Ok(()) +} /// Get the current admin. /// diff --git a/contracts/router-timelock/Cargo.toml b/contracts/router-timelock/Cargo.toml index 4f1f052..98cb5f6 100644 --- a/contracts/router-timelock/Cargo.toml +++ b/contracts/router-timelock/Cargo.toml @@ -8,6 +8,7 @@ crate-type = ["cdylib", "rlib"] [dependencies] soroban-sdk = { workspace = true } +router-common = { path = "../router-common" } [dev-dependencies] soroban-sdk = { version = "21.7.6", features = ["testutils"] } diff --git a/contracts/router-timelock/src/lib.rs b/contracts/router-timelock/src/lib.rs index c06a505..16bd3cd 100644 --- a/contracts/router-timelock/src/lib.rs +++ b/contracts/router-timelock/src/lib.rs @@ -730,12 +730,26 @@ impl RouterTimelock { /// # Errors /// * [`TimelockError::Unauthorized`] — if `current` is not the admin. /// * [`TimelockError::NotInitialized`] — if the contract has not been initialized. - pub fn transfer_admin(env: Env, current: Address, new_admin: Address) -> Result<(), TimelockError> { - current.require_auth(); - Self::require_admin(&env, ¤t)?; - env.storage().instance().set(&DataKey::Admin, &new_admin); - Ok(()) - } + pub fn transfer_admin(env: Env, current: Address, new_admin: Address) -> Result<(), MiddlewareError> { + current.require_auth(); + + // One-liner using the shared macro + router_common::require_admin_simple!( + &env, + ¤t, + &DataKey::Admin, + MiddlewareError + )?; + + env.storage().instance().set(&DataKey::Admin, &new_admin); + + env.events().publish( + (Symbol::new(&env, "admin_transferred"),), + (current, new_admin), + ); + + Ok(()) +} // ── Helpers ───────────────────────────────────────────────────────────────