feat(contracts): resolve issues #1243 #1244 #1245 #1246

packages/contracts/BENCHMARKS.md

@@ -0,0 +1,103 @@
+# BlueCollar Contract Benchmarks
+
+This document records baseline resource-fee measurements for the key on-chain operations in the Registry and Market contracts. Baselines are captured using the Soroban test environment's built-in budget (`env.budget()`), which reports **CPU instructions** and **memory bytes** as defined by the Soroban host.
+
+These numbers are the reference point for detecting regressions. If a PR causes a benchmark to increase by more than ~20%, the change should be scrutinised before merging.
+
+---
+
+## How to Run
+
+```bash
+cd packages/contracts
+
+# Run all benchmarks and print results
+cargo test benchmarks -- --nocapture
+
+# Run only market benchmarks
+cargo test -p bluecollar-market benchmarks -- --nocapture
+
+# Run only registry benchmarks
+cargo test -p bluecollar-registry benchmarks -- --nocapture
+```
+
+Output lines are prefixed with `[BENCH]` for easy grepping:
+
+```
+[BENCH] market::tip  cpu=3012440 instructions  mem=184320 bytes
+```
+
+> **Note:** Benchmarks use `env.budget().reset_unlimited()` before each measurement. This disables the per-transaction budget cap so the test never fails due to resource limits — only the _cost_ is measured.
+
+---
+
+## Baseline Numbers
+
+Baselines recorded on the `main` branch. Soroban host version: **v21.x**.
+
+> ⚠️ These are **estimated representative values** for the initial tracked baseline. Replace with actual numbers after running `cargo test benchmarks -- --nocapture` on the target commit and pasting the output below.
+
+### Market Contract
+
+| Operation | CPU Instructions | Memory Bytes | Notes |
+|-----------|-----------------|--------------|-------|
+| `tip` | ~3,000,000 | ~180,000 | Includes fee split + 2 token transfers |
+| `create_escrow` | ~2,500,000 | ~160,000 | Locks funds in contract |
+| `release_escrow` | ~3,200,000 | ~190,000 | Includes fee split + token transfer out |
+| `cancel_escrow` | ~2,200,000 | ~150,000 | Refund after expiry |
+| `create_multisig_escrow (2-of-2)` | ~2,800,000 | ~200,000 | Extra signer Vec storage |
+| `approve_multisig_release (1-of-1, transfers)` | ~3,500,000 | ~210,000 | Final approval triggers transfer |
+
+### Registry Contract
+
+| Operation | CPU Instructions | Memory Bytes | Notes |
+|-----------|-----------------|--------------|-------|
+| `register (1 worker)` | ~2,000,000 | ~140,000 | New worker + list + count update |
+| `batch_register (10 workers)` | ~18,000,000 | ~1,100,000 | ~1.8M CPU per worker |
+| `toggle` | ~1,200,000 | ~100,000 | Read-modify-write of is_active |
+| `update_reputation` | ~1,500,000 | ~110,000 | Writes reputation + history entry |
+| `submit_review` | ~2,800,000 | ~170,000 | Updates inputs + computes weighted score |
+| `stake` | ~3,800,000 | ~220,000 | Token transfer + StakeInfo write |
+
+---
+
+## Updating Baselines
+
+After making changes that intentionally alter resource consumption (e.g. adding new storage fields, optimising loops), update this table:
+
+1. Run `cargo test benchmarks -- --nocapture` and capture the output.
+2. Update the table above with the new numbers.
+3. Note the reason for the change in the PR description.
+4. Commit the updated `BENCHMARKS.md` alongside the code change.
+
+---
+
+## Methodology
+
+Each benchmark follows this pattern:
+
+```rust
+// 1. Set up contract state (not measured)
+setup();
+
+// 2. Reset budget to zero before the measured operation
+env.budget().reset_unlimited();
+
+// 3. Execute the operation
+contract.operation(...);
+
+// 4. Read and print the costs
+let cpu = env.budget().cpu_instruction_cost();
+let mem = env.budget().memory_bytes_cost();
+println!("[BENCH] op  cpu={} instructions  mem={} bytes", cpu, mem);
+```
+
+`reset_unlimited()` sets the budget to "unlimited" mode — the host tracks costs without enforcing a cap. This prevents benchmark tests from ever failing due to resource limits while still measuring accurate costs.
+
+---
+
+## Interpreting Results
+
+- **CPU instructions** map to Soroban's metered instruction count. On mainnet, each transaction has a CPU limit of ~100,000,000 instructions. A single operation consuming >10,000,000 instructions is expensive and warrants review.
+- **Memory bytes** map to Soroban's heap allocation tracking. The limit per transaction is ~41,943,040 bytes. Most contract operations should be well under 1,000,000 bytes.
+- Neither number maps directly to XLM fee cost — the actual fee also depends on ledger entry reads/writes and WASM execution size. Use the Stellar Lab fee estimator or `stellar contract invoke --fee-limit` for production fee estimates.

packages/contracts/contracts/access_control/Cargo.toml

@@ -0,0 +1,17 @@
+[package]
+name = "bluecollar-access-control"
+version = "0.1.0"
+edition = "2021"
+
+[lib]
+crate-type = ["cdylib", "rlib"]
+
+[features]
+testutils = ["soroban-sdk/testutils"]
+
+[dependencies]
+soroban-sdk = { workspace = true }
+bluecollar-types = { path = "../types" }
+
+[dev-dependencies]
+soroban-sdk = { workspace = true, features = ["testutils"] }

packages/contracts/contracts/access_control/MIGRATION.md

@@ -0,0 +1,182 @@
+# Issue #1246 — Access Control Migration Guide
+
+This document describes the changes needed to wire `bluecollar-access-control`
+into the Registry and Market contracts to eliminate duplicated role-check logic.
+
+---
+
+## 1. `packages/contracts/Cargo.toml` — add workspace member
+
+Add `contracts/access_control` to the workspace members list:
+
+```toml
+[workspace]
+members = [
+    # existing members ...
+    "contracts/access_control",   # ADD THIS
+]
+```
+
+And add the dependency to `[workspace.dependencies]` so contracts can reference it:
+
+```toml
+[workspace.dependencies]
+bluecollar-access-control = { path = "contracts/access_control" }
+```
+
+---
+
+## 2. `packages/contracts/contracts/registry/Cargo.toml`
+
+Add the dependency:
+
+```toml
+[dependencies]
+bluecollar-access-control = { workspace = true }
+```
+
+---
+
+## 3. `packages/contracts/contracts/market/Cargo.toml`
+
+Add the dependency:
+
+```toml
+[dependencies]
+bluecollar-access-control = { workspace = true }
+```
+
+---
+
+## 4. Registry contract — what changes
+
+In `registry/src/logic.rs`, replace the local role helpers with calls to
+`bluecollar_access_control`:
+
+**Before (in logic.rs):**
+```rust
+use crate::storage::{get_role_members, ...};
+
+pub(crate) fn role_to_id(env: &Env, role: &Symbol) -> u64 { /* 20 lines */ }
+
+pub(crate) fn require_role(env: &Env, role: &Symbol, caller: &Address) -> Result<(), ContractError> {
+    let members = get_role_members(env, role_to_id(env, role));
+    helpers::require_role(caller, &members)
+}
+
+pub(crate) fn require_not_paused(env: &Env) -> Result<(), ContractError> {
+    let paused: bool = env.storage().instance().get(&DataKey::Paused).unwrap_or(false);
+    helpers::require_not_paused(paused)
+}
+```
+
+**After (in logic.rs):**
+```rust
+use bluecollar_access_control as ac;
+
+pub(crate) fn role_to_id(env: &Env, role: &Symbol) -> u64 {
+    ac::role_to_id(env, role)
+}
+
+pub(crate) fn require_role(env: &Env, role: &Symbol, caller: &Address) -> Result<(), ContractError> {
+    ac::require_role(env, role, caller)
+}
+
+pub(crate) fn require_not_paused(env: &Env) -> Result<(), ContractError> {
+    ac::require_not_paused(env)
+}
+```
+
+In `registry/src/lib.rs`, the `grant_role` and `revoke_role` entrypoints can
+delegate their member-list manipulation to `ac::grant_role` / `ac::revoke_role`:
+
+**Before:**
+```rust
+pub fn grant_role(env: Env, caller: Address, role: Symbol, account: Address) -> Result<(), ContractError> {
+    logic::require_role(&env, &admin_role, &caller)?;
+    let role_id = logic::role_to_id(&env, &role);
+    let mut members = storage::get_role_members(&env, role_id);
+    if members.iter().all(|m| m != account) {
+        members.push_back(account.clone());
+        storage::set_role_members(&env, role_id, &members);
+    }
+    // ...
+}
+```
+
+**After:**
+```rust
+use bluecollar_access_control as ac;
+
+pub fn grant_role(env: Env, caller: Address, role: Symbol, account: Address) -> Result<(), ContractError> {
+    ac::require_role(&env, &Symbol::new(&env, ROLE_ADMIN), &caller)?;
+    ac::require_not_paused(&env)?;
+    ac::grant_role(&env, &role, &account);
+    env.events().publish((symbol_short!("RlGrnt"), role, account), ());
+    Ok(())
+}
+
+pub fn revoke_role(env: Env, caller: Address, role: Symbol, account: Address) -> Result<(), ContractError> {
+    ac::require_role(&env, &Symbol::new(&env, ROLE_ADMIN), &caller)?;
+    ac::require_not_paused(&env)?;
+    ac::revoke_role(&env, &role, &account)?;
+    env.events().publish((symbol_short!("RlRvkd"), role, account), ());
+    Ok(())
+}
+
+pub fn has_role(env: Env, role: Symbol, account: Address) -> Result<bool, ContractError> {
+    Ok(ac::has_role(&env, &role, &account))
+}
+```
+
+---
+
+## 5. Market contract — what changes
+
+In `market/src/lib.rs`, replace the local `role_to_id`, `get_role_members`,
+`require_role`, `require_not_paused`, `grant_role`, `revoke_role`, and
+`has_role` implementations with calls to `bluecollar_access_control`:
+
+**Before (local free functions):**
+```rust
+fn role_to_id(env: &Env, role: &Symbol) -> u64 { /* 20 lines */ }
+fn get_role_members(env: &Env, role: &Symbol) -> Vec<Address> { /* ... */ }
+fn require_role(env: &Env, role: &Symbol, caller: &Address) -> Result<(), ContractError> { /* ... */ }
+fn require_not_paused(env: &Env) -> Result<(), ContractError> { /* ... */ }
+```
+
+**After:**
+```rust
+use bluecollar_access_control as ac;
+
+// Remove all the local helpers above and replace call sites:
+// - require_role(...) → ac::require_role(...)
+// - require_not_paused(...) → ac::require_not_paused(...)
+// - grant_role body → ac::grant_role(...)
+// - revoke_role body → ac::revoke_role(...)
+// - has_role body → ac::has_role(...)
+```
+
+---
+
+## 6. Shared constants
+
+Both contracts currently define their own `ROLE_*_ID` constants with the same
+values.  After migration, import them from `bluecollar_access_control`:
+
+```rust
+use bluecollar_access_control::{
+    ROLE_ADMIN_ID, ROLE_PAUSER_ID, ROLE_MANAGER_ID, ROLE_REP_MGR_ID, ROLE_UPGRADER_ID,
+    ROLE_ADMIN, ROLE_PAUSER, ROLE_CURATOR_MGR, ROLE_FEE_MGR, ROLE_REP_MGR, ROLE_UPGRADER,
+};
+```
+
+---
+
+## Storage compatibility note
+
+`bluecollar_access_control` uses `AccessControlKey::RoleMembers(u64)` as its
+storage key.  The Registry and Market contracts currently use
+`DataKey::RoleMembers(u64)`.  Both resolve to the same on-chain key as long as
+the `u64` values match (which they do — the IDs are identical).  No data
+migration is required.