diff --git a/PR_REQUEST.md b/PR_REQUEST.md new file mode 100644 index 0000000..cfd17d9 --- /dev/null +++ b/PR_REQUEST.md @@ -0,0 +1,52 @@ +# PR Request: Add Two-Step Governance Admin Transfer for Escrow + +## Summary +This PR adds a secure two-step admin transfer mechanism to the TalentTrust escrow governance layer in `contracts/escrow`. + +## What changed +- Implemented governance storage and management in `contracts/escrow/src/governance.rs` +- Added protocol parameter support and governance state keys in `contracts/escrow/src/types.rs` +- Updated `contracts/escrow/src/lib.rs` to expose governance functions and types +- Added/updated governance tests in `contracts/escrow/src/test/governance.rs` +- Clarified governance documentation in `docs/escrow/governance-security.md` + +## New public governance behavior +- `initialize_protocol_governance(admin, ...) -> bool` +- `initialize_governance(admin) -> bool` +- `update_protocol_parameters(...) -> bool` +- `propose_governance_admin(new_admin) -> bool` +- `accept_governance_admin() -> bool` +- `get_protocol_parameters() -> ProtocolParameters` +- `get_governance_admin() -> Option
` +- `get_pending_governance_admin() -> Option
` + +## Security model enforced +- Current governance admin must authenticate to propose a new admin +- Proposed admin must authenticate to accept transfer +- Pending admin state is cleared on acceptance +- A new proposal overwrites any existing pending admin +- Current admin retains control until the transfer is accepted + +## Files changed +- `contracts/escrow/src/types.rs` +- `contracts/escrow/src/lib.rs` +- `contracts/escrow/src/governance.rs` +- `contracts/escrow/src/test/governance.rs` +- `contracts/escrow/src/test/mainnet_readiness.rs` +- `docs/escrow/governance-security.md` + +## Test plan +Run locally in the repo root: + +```bash +cargo fmt --all +cargo test -p escrow +cargo test -p escrow governance +cargo test test::performance -p escrow +cargo fmt --all -- --check +``` + +## Notes +- This PR targets the escrow governance layer only. +- The implementation is intentionally scoped to the governance admin transfer flow and its supporting protocol parameter state. +- The document is written for reviewers and release notes. diff --git a/contracts/escrow/src/governance.rs b/contracts/escrow/src/governance.rs new file mode 100644 index 0000000..98bfc81 --- /dev/null +++ b/contracts/escrow/src/governance.rs @@ -0,0 +1,205 @@ +use crate::{types::ProtocolParameters, DataKey, EscrowError}; +use soroban_sdk::{contractimpl, symbol_short, Address, Env, Symbol}; + +impl Escrow { + fn validate_protocol_parameters( + env: &Env, + min_milestone_amount: i128, + max_milestones: u32, + min_reputation_rating: i128, + max_reputation_rating: i128, + ) -> ProtocolParameters { + if min_milestone_amount <= 0 { + env.panic_with_error(EscrowError::InvalidProtocolParameters); + } + if max_milestones == 0 { + env.panic_with_error(EscrowError::InvalidProtocolParameters); + } + if min_reputation_rating <= 0 { + env.panic_with_error(EscrowError::InvalidProtocolParameters); + } + if min_reputation_rating > max_reputation_rating { + env.panic_with_error(EscrowError::InvalidProtocolParameters); + } + + ProtocolParameters { + min_milestone_amount, + max_milestones, + min_reputation_rating, + max_reputation_rating, + } + } + + fn governance_admin(env: &Env) -> Address { + env.storage() + .persistent() + .get(&DataKey::GovernanceAdmin) + .unwrap_or_else(|| env.panic_with_error(EscrowError::GovernanceNotInitialized)) + } + + fn set_governance_readiness(env: &Env) { + let mut checklist = Self::load_checklist(env); + checklist.governed_params_set = true; + env.storage() + .persistent() + .set(&DataKey::ReadinessChecklist, &checklist); + } +} + +#[contractimpl] +impl Escrow { + /// One-time governance initialization. + /// Sets the governance admin and initial protocol parameters. + pub fn initialize_protocol_governance( + env: Env, + admin: Address, + min_milestone_amount: i128, + max_milestones: u32, + min_reputation_rating: i128, + max_reputation_rating: i128, + ) -> bool { + if env + .storage() + .persistent() + .get::<_, Address>(&DataKey::GovernanceAdmin) + .is_some() + { + panic!("protocol governance is already initialized"); + } + + admin.require_auth(); + let params = Self::validate_protocol_parameters( + &env, + min_milestone_amount, + max_milestones, + min_reputation_rating, + max_reputation_rating, + ); + env.storage().persistent().set(&DataKey::GovernanceAdmin, &admin); + env.storage() + .persistent() + .set(&DataKey::ProtocolParameters, ¶ms); + Self::set_governance_readiness(&env); + + env.events().publish( + (symbol_short!("governance"), Symbol::new(&env, "initialized")), + (admin.clone(), env.ledger().timestamp()), + ); + true + } + + /// Convenience governance initialization using default protocol parameters. + pub fn initialize_governance(env: Env, admin: Address) -> bool { + let defaults = ProtocolParameters::default(); + Self::initialize_protocol_governance( + env, + admin, + defaults.min_milestone_amount, + defaults.max_milestones, + defaults.min_reputation_rating, + defaults.max_reputation_rating, + ) + } + + /// Update governance protocol parameters. Requires governance admin auth. + pub fn update_protocol_parameters( + env: Env, + min_milestone_amount: i128, + max_milestones: u32, + min_reputation_rating: i128, + max_reputation_rating: i128, + ) -> bool { + if env + .storage() + .persistent() + .get::<_, Address>(&DataKey::GovernanceAdmin) + .is_none() + { + panic!("protocol governance is not initialized"); + } + + let admin = Self::governance_admin(&env); + admin.require_auth(); + + let params = Self::validate_protocol_parameters( + &env, + min_milestone_amount, + max_milestones, + min_reputation_rating, + max_reputation_rating, + ); + env.storage() + .persistent() + .set(&DataKey::ProtocolParameters, ¶ms); + Self::set_governance_readiness(&env); + + env.events().publish( + (symbol_short!("governance"), Symbol::new(&env, "parameters_updated")), + (admin.clone(), env.ledger().timestamp()), + ); + true + } + + /// Returns the currently active protocol parameters. + pub fn get_protocol_parameters(env: Env) -> ProtocolParameters { + env.storage() + .persistent() + .get(&DataKey::ProtocolParameters) + .unwrap_or_default() + } + + /// Returns the current governance admin if initialized. + pub fn get_governance_admin(env: Env) -> Option
{ + env.storage().persistent().get(&DataKey::GovernanceAdmin) + } + + /// Returns the pending governance admin, if one has been proposed. + pub fn get_pending_governance_admin(env: Env) -> Option
{ + env.storage() + .persistent() + .get(&DataKey::PendingGovernanceAdmin) + } + + /// Propose a new governance admin. Current admin must authenticate. + pub fn propose_governance_admin(env: Env, new_admin: Address) -> bool { + let admin = Self::governance_admin(&env); + admin.require_auth(); + + env.storage() + .persistent() + .set(&DataKey::PendingGovernanceAdmin, &new_admin); + env.events().publish( + ( + symbol_short!("governance"), + Symbol::new(&env, "admin_proposed"), + ), + (admin.clone(), new_admin.clone(), env.ledger().timestamp()), + ); + true + } + + /// Accept proposed governance admin privileges. Pending admin must authorize. + pub fn accept_governance_admin(env: Env) -> bool { + let pending_admin: Address = env + .storage() + .persistent() + .get(&DataKey::PendingGovernanceAdmin) + .unwrap_or_else(|| env.panic_with_error(EscrowError::InvalidState)); + pending_admin.require_auth(); + + env.storage() + .persistent() + .set(&DataKey::GovernanceAdmin, &pending_admin); + env.storage() + .persistent() + .remove(&DataKey::PendingGovernanceAdmin); + env.events().publish( + ( + symbol_short!("governance"), + Symbol::new(&env, "admin_accepted"), + ), + (pending_admin.clone(), env.ledger().timestamp()), + ); + true + } +} diff --git a/contracts/escrow/src/lib.rs b/contracts/escrow/src/lib.rs index 696afbf..4e7caa4 100644 --- a/contracts/escrow/src/lib.rs +++ b/contracts/escrow/src/lib.rs @@ -31,12 +31,15 @@ use soroban_sdk::{contract, contractimpl, symbol_short, Address, Env, Symbol, Ve mod types; pub use types::{ ContractStatus, ContractSummary, DataKey, DepositMode, EscrowError, Milestone, - MilestoneSummary, ReadinessChecklist, CONTRACT_SUMMARY_SCHEMA_VERSION, + MilestoneSummary, ProtocolParameters, ReadinessChecklist, + CONTRACT_SUMMARY_SCHEMA_VERSION, }; mod amount_validation; pub use amount_validation::{safe_add_amounts, safe_subtract_amounts, AmountValidationError}; +mod governance; + mod ttl; pub use ttl::{ LEDGERS_PER_DAY, PENDING_APPROVAL_BUMP_THRESHOLD, PENDING_APPROVAL_TTL_LEDGERS, diff --git a/contracts/escrow/src/test/governance.rs b/contracts/escrow/src/test/governance.rs index 158d196..627ab1a 100644 --- a/contracts/escrow/src/test/governance.rs +++ b/contracts/escrow/src/test/governance.rs @@ -1,6 +1,6 @@ use super::register_client; use crate::{EscrowError, ProtocolParameters}; -use soroban_sdk::{testutils::Address as _, vec, Address, Env}; +use soroban_sdk::{testutils::Address as _, Address, Env}; #[test] fn protocol_parameters_default_before_governance_is_initialized() { @@ -45,7 +45,6 @@ fn initialize_governance_twice_panics() { let admin = Address::generate(&env); client.initialize_governance(&admin); - // Second initialization should panic let admin2 = Address::generate(&env); client.initialize_governance(&admin2); } @@ -73,12 +72,11 @@ fn update_protocol_parameters_without_initialization_panics() { let (env, contract_id) = setup(); let client = EscrowClient::new(&env, &contract_id); - // Try to update without initializing governance client.update_protocol_parameters(&100_i128, &10_u32, &1_i128, &10_i128); } #[test] -#[should_panic(expected = "minimum milestone amount must be positive")] +#[should_panic(expected = "invalid protocol parameters")] fn update_protocol_parameters_with_zero_min_milestone_panics() { let (env, contract_id) = setup(); let client = EscrowClient::new(&env, &contract_id); @@ -86,12 +84,7 @@ fn update_protocol_parameters_with_zero_min_milestone_panics() { let admin = Address::generate(&env); client.initialize_governance(&admin); - let contract_id = client.create_contract(&escrow_client, &freelancer, &milestones); - assert!(client.deposit_funds(&contract_id, &285_i128)); - assert!(client.release_milestone(&contract_id, &0)); - assert!(client.release_milestone(&contract_id, &1)); - assert!(client.release_milestone(&contract_id, &2)); - assert!(client.issue_reputation(&contract_id, &5_i128)); + client.update_protocol_parameters(&0_i128, &4_u32, &1_i128, &5_i128); } #[test] @@ -105,16 +98,31 @@ fn governance_admin_transfer_is_two_step() { assert!(client.initialize_protocol_governance(&admin, &10_i128, &4_u32, &1_i128, &5_i128)); assert!(client.propose_governance_admin(&next_admin)); - assert_eq!( - client.get_pending_governance_admin(), - Some(next_admin.clone()) - ); + assert_eq!(client.get_pending_governance_admin(), Some(next_admin.clone())); assert!(client.accept_governance_admin()); assert_eq!(client.get_governance_admin(), Some(next_admin)); assert_eq!(client.get_pending_governance_admin(), None); } +#[test] +fn governance_propose_overwrites_pending_admin() { + let env = Env::default(); + env.mock_all_auths(); + let client = register_client(&env); + + let admin = Address::generate(&env); + let first_pending = Address::generate(&env); + let second_pending = Address::generate(&env); + assert!(client.initialize_protocol_governance(&admin, &10_i128, &4_u32, &1_i128, &5_i128)); + + assert!(client.propose_governance_admin(&first_pending)); + assert_eq!(client.get_pending_governance_admin(), Some(first_pending.clone())); + + assert!(client.propose_governance_admin(&second_pending)); + assert_eq!(client.get_pending_governance_admin(), Some(second_pending.clone())); +} + #[test] fn governance_rejects_invalid_parameter_updates() { let env = Env::default(); diff --git a/contracts/escrow/src/test/mainnet_readiness.rs b/contracts/escrow/src/test/mainnet_readiness.rs index 2603cc5..3167e0c 100644 --- a/contracts/escrow/src/test/mainnet_readiness.rs +++ b/contracts/escrow/src/test/mainnet_readiness.rs @@ -81,7 +81,9 @@ fn initialize_protocol_governance_sets_governed_params() { fn update_protocol_parameters_sets_governed_params() { let (env, contract_id) = setup(); let client = EscrowClient::new(&env, &contract_id); + let admin = Address::generate(&env); + client.initialize_governance(&admin); client.update_protocol_parameters(&50_i128, &8_u32, &1_i128, &5_i128); let info = client.get_mainnet_readiness_info(); diff --git a/contracts/escrow/src/types.rs b/contracts/escrow/src/types.rs index 1213d63..622eccd 100644 --- a/contracts/escrow/src/types.rs +++ b/contracts/escrow/src/types.rs @@ -20,6 +20,9 @@ pub enum DataKey { // Client migration PendingClientMigration(u32), // Protocol / governance + GovernanceAdmin, + PendingGovernanceAdmin, + ProtocolParameters, ProtocolFeeBps, AccumulatedProtocolFees, ReadinessChecklist, @@ -62,9 +65,11 @@ pub enum EscrowError { EmergencyActive = 30, NotInitialized = 31, AlreadyInitialized = 32, + InvalidProtocolParameters = 33, + GovernanceNotInitialized = 34, // Additional errors referenced in tests - FreelancerMismatch = 33, - EmptyRefundRequest = 34, + FreelancerMismatch = 35, + EmptyRefundRequest = 36, DuplicateMilestoneInRefund = 35, PotentialOverflow = 36, NonPositiveAmount = 37, @@ -122,6 +127,26 @@ impl Default for ReadinessChecklist { } } +#[contracttype] +#[derive(Clone, Debug, Eq, PartialEq)] +pub struct ProtocolParameters { + pub min_milestone_amount: i128, + pub max_milestones: u32, + pub min_reputation_rating: i128, + pub max_reputation_rating: i128, +} + +impl Default for ProtocolParameters { + fn default() -> Self { + ProtocolParameters { + min_milestone_amount: 1, + max_milestones: 16, + min_reputation_rating: 1, + max_reputation_rating: 5, + } + } +} + // ─── Indexer summary types ──────────────────────────────────────────────────── pub const CONTRACT_SUMMARY_SCHEMA_VERSION: u32 = 1; diff --git a/docs/escrow/governance-security.md b/docs/escrow/governance-security.md index dda07d9..eb5ffd0 100644 --- a/docs/escrow/governance-security.md +++ b/docs/escrow/governance-security.md @@ -135,6 +135,7 @@ client.update_protocol_parameters(&100_i128, &16_u32, &1_i128, &5_i128); - Can be called multiple times (overwrites pending admin) - Does not immediately transfer control - Pending admin can be queried via `get_pending_governance_admin()` +- Overwrites any existing pending admin when re-proposed **Usage**: ```rust @@ -149,6 +150,7 @@ client.propose_governance_admin(&new_admin); **Security Properties**: - Requires pending admin authentication - Completes the transfer atomically +- Current admin retains control until acceptance - Clears pending admin state - New admin immediately has full control