From c242eeb709cc0ebb690231b44f133c4299e00b7a Mon Sep 17 00:00:00 2001 From: Zainab Aminu Date: Fri, 24 Apr 2026 18:01:12 +0000 Subject: [PATCH] feature:Refactor validation and introduce granular error handling --- dongle-smartcontract/src/errors.rs | 18 ++ dongle-smartcontract/src/fee_manager.rs | 2 +- dongle-smartcontract/src/lib.rs | 2 +- dongle-smartcontract/src/project_registry.rs | 113 +++++------- dongle-smartcontract/src/review_registry.rs | 17 +- dongle-smartcontract/src/utils.rs | 169 ++++++++++++++++-- .../src/verification_registry.rs | 6 +- 7 files changed, 229 insertions(+), 98 deletions(-) diff --git a/dongle-smartcontract/src/errors.rs b/dongle-smartcontract/src/errors.rs index b40b0eb..22a6031 100644 --- a/dongle-smartcontract/src/errors.rs +++ b/dongle-smartcontract/src/errors.rs @@ -41,6 +41,24 @@ pub enum ContractError { CannotRemoveLastAdmin = 17, /// Admin not found AdminNotFound = 18, + /// Project name is empty or whitespace + ProjectNameEmpty = 19, + /// Project description is empty or whitespace + ProjectDescriptionEmpty = 20, + /// Project category is empty or whitespace + ProjectCategoryEmpty = 21, + /// Website URL is invalid + InvalidWebsiteUrl = 22, + /// Logo CID is invalid + InvalidLogoCid = 23, + /// Metadata CID is invalid + InvalidMetadataCid = 24, + /// Evidence CID is empty or invalid + InvalidEvidenceCid = 25, + /// Comment CID is invalid + InvalidCommentCid = 26, + /// Invalid fee token provided + InvalidFeeToken = 28, } // Legacy alias to avoid breaking any code that uses `Error` directly diff --git a/dongle-smartcontract/src/fee_manager.rs b/dongle-smartcontract/src/fee_manager.rs index f9d3ae7..414bfc8 100644 --- a/dongle-smartcontract/src/fee_manager.rs +++ b/dongle-smartcontract/src/fee_manager.rs @@ -54,7 +54,7 @@ impl FeeManager { .ok_or(ContractError::TreasuryNotSet)?; if config.token != token { - return Err(ContractError::InvalidProjectData); + return Err(ContractError::InvalidFeeToken); } let amount = config.verification_fee; diff --git a/dongle-smartcontract/src/lib.rs b/dongle-smartcontract/src/lib.rs index 87a9f48..91ec714 100644 --- a/dongle-smartcontract/src/lib.rs +++ b/dongle-smartcontract/src/lib.rs @@ -70,7 +70,7 @@ impl DongleContract { ProjectRegistry::register_project(&env, params) } - pub fn update_project(env: Env, params: ProjectUpdateParams) -> Option { + pub fn update_project(env: Env, params: ProjectUpdateParams) -> Result, ContractError> { ProjectRegistry::update_project(&env, params) } diff --git a/dongle-smartcontract/src/project_registry.rs b/dongle-smartcontract/src/project_registry.rs index e4ce174..816f2ab 100644 --- a/dongle-smartcontract/src/project_registry.rs +++ b/dongle-smartcontract/src/project_registry.rs @@ -1,6 +1,7 @@ use crate::errors::ContractError; use crate::storage_keys::StorageKey; use crate::types::{Project, ProjectRegistrationParams, ProjectUpdateParams, VerificationStatus}; +use crate::utils::Utils; use soroban_sdk::{Address, Env, Vec}; pub struct ProjectRegistry; @@ -13,15 +14,13 @@ impl ProjectRegistry { ) -> Result { params.owner.require_auth(); - if params.name.is_empty() { - panic!("InvalidProjectName"); - } - if params.description.is_empty() { - panic!("InvalidProjectDescription"); - } - if params.category.is_empty() { - panic!("InvalidProjectCategory"); - } + // Validate all project parameters + Utils::validate_project_name(¶ms.name)?; + Utils::validate_project_description(¶ms.description)?; + Utils::validate_project_category(¶ms.category)?; + Utils::validate_website_url(¶ms.website)?; + Utils::validate_logo_cid(¶ms.logo_cid)?; + Utils::validate_metadata_cid(¶ms.metadata_cid)?; // Check if project name already exists if env @@ -77,30 +76,47 @@ impl ProjectRegistry { Ok(count) } - pub fn update_project(env: &Env, params: ProjectUpdateParams) -> Option { - let mut project = Self::get_project(env, params.project_id)?; + pub fn update_project(env: &Env, params: ProjectUpdateParams) -> Result, ContractError> { + let mut project = Self::get_project(env, params.project_id).ok_or(ContractError::ProjectNotFound)?; params.caller.require_auth(); if project.owner != params.caller { - return None; + return Err(ContractError::Unauthorized); } - if let Some(value) = params.name { - project.name = value; + // Validate and update name + if let Some(ref value) = params.name { + Utils::validate_project_name(value)?; + project.name = value.clone(); } - if let Some(value) = params.description { - project.description = value; + + // Validate and update description + if let Some(ref value) = params.description { + Utils::validate_project_description(value)?; + project.description = value.clone(); } - if let Some(value) = params.category { - project.category = value; + + // Validate and update category + if let Some(ref value) = params.category { + Utils::validate_project_category(value)?; + project.category = value.clone(); } + + // Validate and update website if let Some(value) = params.website { + Utils::validate_website_url(&value)?; project.website = value; } + + // Validate and update logo_cid if let Some(value) = params.logo_cid { + Utils::validate_logo_cid(&value)?; project.logo_cid = value; } + + // Validate and update metadata_cid if let Some(value) = params.metadata_cid { + Utils::validate_metadata_cid(&value)?; project.metadata_cid = value; } @@ -109,7 +125,7 @@ impl ProjectRegistry { .persistent() .set(&StorageKey::Project(params.project_id), &project); - Some(project) + Ok(Some(project)) } pub fn get_project(env: &Env, project_id: u64) -> Option { @@ -180,50 +196,15 @@ impl ProjectRegistry { mod tests { use crate::errors::ContractError; use crate::project_registry::ProjectRegistry; + use crate::utils::Utils; use soroban_sdk::{Address, Env, String}; - // Validation function only used in tests - fn validate_project_data( - name: &String, - _description: &String, - _category: &String, - ) -> Result<(), ContractError> { - extern crate alloc; - use alloc::string::ToString; - - let name_str = name.to_string(); - - // 1. Validate Non-empty and not only whitespace - if name_str.trim().is_empty() { - return Err(ContractError::InvalidProjectData); - } - - // 2. Validate max length using the CONSTANT - let max_len = crate::constants::MAX_NAME_LEN; - if name_str.len() > max_len { - return Err(ContractError::ProjectNameTooLong); - } - - // 3. Validate alphanumeric, underscore, hyphen - for c in name_str.chars() { - if !c.is_ascii_alphanumeric() && c != '_' && c != '-' { - return Err(ContractError::InvalidProjectNameFormat); - } - } - - Ok(()) - } - #[test] fn test_valid_project_name() { let env = Env::default(); let name = String::from_str(&env, "Valid-Project_Name123"); - let result = validate_project_data( - &name, - &String::from_str(&env, "Desc"), - &String::from_str(&env, "Cat"), - ); + let result = Utils::validate_project_name(&name); assert!(result.is_ok()); } @@ -232,12 +213,8 @@ mod tests { let env = Env::default(); let name = String::from_str(&env, " "); - let result = validate_project_data( - &name, - &String::from_str(&env, "Desc"), - &String::from_str(&env, "Cat"), - ); - assert_eq!(result, Err(ContractError::InvalidProjectData)); + let result = Utils::validate_project_name(&name); + assert_eq!(result, Err(ContractError::ProjectNameEmpty)); } #[test] @@ -245,11 +222,7 @@ mod tests { let env = Env::default(); let name = String::from_str(&env, "My Project *"); - let result = validate_project_data( - &name, - &String::from_str(&env, "Desc"), - &String::from_str(&env, "Cat"), - ); + let result = Utils::validate_project_name(&name); assert_eq!(result, Err(ContractError::InvalidProjectNameFormat)); } @@ -259,11 +232,7 @@ mod tests { // 51 characters let name = String::from_str(&env, "ThisProjectNameIsWayTooLongAndExceedsTheFiftyCharL1"); - let result = validate_project_data( - &name, - &String::from_str(&env, "Desc"), - &String::from_str(&env, "Cat"), - ); + let result = Utils::validate_project_name(&name); assert_eq!(result, Err(ContractError::ProjectNameTooLong)); } } diff --git a/dongle-smartcontract/src/review_registry.rs b/dongle-smartcontract/src/review_registry.rs index 0822ab3..56943c1 100644 --- a/dongle-smartcontract/src/review_registry.rs +++ b/dongle-smartcontract/src/review_registry.rs @@ -6,6 +6,7 @@ use crate::events::publish_review_event; use crate::rating_calculator::RatingCalculator; use crate::storage_keys::StorageKey; use crate::types::{ProjectStats, Review, ReviewAction}; +use crate::utils::Utils; use soroban_sdk::{Address, Env, String, Vec}; pub struct ReviewRegistry; @@ -20,9 +21,11 @@ impl ReviewRegistry { ) -> Result<(), ContractError> { reviewer.require_auth(); - if !(RATING_MIN..=RATING_MAX).contains(&rating) { - return Err(ContractError::InvalidRating); - } + // Validate rating + Utils::validate_rating(rating)?; + + // Validate comment CID if provided + Utils::validate_comment_cid(&comment_cid)?; let review_key = StorageKey::Review(project_id, reviewer.clone()); if env.storage().persistent().has(&review_key) { @@ -107,9 +110,11 @@ impl ReviewRegistry { ) -> Result<(), ContractError> { reviewer.require_auth(); - if !(RATING_MIN..=RATING_MAX).contains(&rating) { - return Err(ContractError::InvalidRating); - } + // Validate rating + Utils::validate_rating(rating)?; + + // Validate comment CID if provided + Utils::validate_comment_cid(&comment_cid)?; let review_key = StorageKey::Review(project_id, reviewer.clone()); let mut review: Review = env diff --git a/dongle-smartcontract/src/utils.rs b/dongle-smartcontract/src/utils.rs index 7a778be..4a80a5b 100644 --- a/dongle-smartcontract/src/utils.rs +++ b/dongle-smartcontract/src/utils.rs @@ -1,3 +1,7 @@ +use crate::constants::{ + MAX_CATEGORY_LEN, MAX_CID_LEN, MAX_DESCRIPTION_LEN, MAX_NAME_LEN, MAX_WEBSITE_LEN, + MIN_STRING_LEN, RATING_MAX, RATING_MIN, +}; use crate::errors::ContractError; use crate::storage_keys::StorageKey; use soroban_sdk::{Address, Env, String}; @@ -26,29 +30,166 @@ impl Utils { Ok(()) } + /// Validate project name: non-empty, trimmed, max length, alphanumeric/underscore/hyphen + pub fn validate_project_name(name: &String) -> Result<(), ContractError> { + extern crate alloc; + use alloc::string::ToString; + + let name_str = name.to_string(); + let trimmed = name_str.trim(); + + // Check non-empty after trim + if trimmed.is_empty() { + return Err(ContractError::ProjectNameEmpty); + } + + // Check max length + if trimmed.len() > MAX_NAME_LEN { + return Err(ContractError::ProjectNameTooLong); + } + + // Check valid characters: alphanumeric, underscore, hyphen + for c in trimmed.chars() { + if !c.is_ascii_alphanumeric() && c != '_' && c != '-' { + return Err(ContractError::InvalidProjectNameFormat); + } + } + + Ok(()) + } + + /// Validate project description: non-empty, trimmed, max length + pub fn validate_project_description(description: &String) -> Result<(), ContractError> { + extern crate alloc; + use alloc::string::ToString; + + let desc_str = description.to_string(); + let trimmed = desc_str.trim(); + + if trimmed.is_empty() { + return Err(ContractError::ProjectDescriptionEmpty); + } + + if trimmed.len() > MAX_DESCRIPTION_LEN { + return Err(ContractError::InvalidProjectData); + } + + Ok(()) + } + + /// Validate project category: non-empty, trimmed, max length + pub fn validate_project_category(category: &String) -> Result<(), ContractError> { + extern crate alloc; + use alloc::string::ToString; + + let cat_str = category.to_string(); + let trimmed = cat_str.trim(); + + if trimmed.is_empty() { + return Err(ContractError::ProjectCategoryEmpty); + } + + if trimmed.len() > MAX_CATEGORY_LEN { + return Err(ContractError::InvalidProjectData); + } + + Ok(()) + } + + /// Validate website URL: optional, but if provided, check length and basic format + pub fn validate_website_url(website: &Option) -> Result<(), ContractError> { + if let Some(url) = website { + if url.len() > MAX_WEBSITE_LEN { + return Err(ContractError::InvalidWebsiteUrl); + } + // Basic URL validation - should start with http:// or https:// + extern crate alloc; + use alloc::string::ToString; + let url_str = url.to_string(); + if !url_str.starts_with("http://") && !url_str.starts_with("https://") { + return Err(ContractError::InvalidWebsiteUrl); + } + } + Ok(()) + } + + /// Validate IPFS CID: optional, but if provided, check length and basic format + pub fn validate_ipfs_cid(cid: &Option) -> Result<(), ContractError> { + if let Some(cid_val) = cid { + if cid_val.is_empty() { + return Err(ContractError::InvalidIpfsCid); + } + if cid_val.len() > MAX_CID_LEN { + return Err(ContractError::InvalidIpfsCid); + } + // Basic IPFS CID validation - should be reasonable length + if cid_val.len() < 10 { + return Err(ContractError::InvalidIpfsCid); + } + } + Ok(()) + } + + /// Validate logo CID (same as IPFS CID) + pub fn validate_logo_cid(logo_cid: &Option) -> Result<(), ContractError> { + Self::validate_ipfs_cid(logo_cid) + } + + /// Validate metadata CID (same as IPFS CID) + pub fn validate_metadata_cid(metadata_cid: &Option) -> Result<(), ContractError> { + Self::validate_ipfs_cid(metadata_cid) + } + + /// Validate comment CID (same as IPFS CID) + pub fn validate_comment_cid(comment_cid: &Option) -> Result<(), ContractError> { + Self::validate_ipfs_cid(comment_cid) + } + + /// Validate evidence CID: required, non-empty, valid IPFS CID + pub fn validate_evidence_cid(evidence_cid: &String) -> Result<(), ContractError> { + if evidence_cid.is_empty() { + return Err(ContractError::InvalidEvidenceCid); + } + if evidence_cid.len() > MAX_CID_LEN { + return Err(ContractError::InvalidEvidenceCid); + } + if evidence_cid.len() < 10 { + return Err(ContractError::InvalidEvidenceCid); + } + Ok(()) + } + + /// Validate rating: must be between RATING_MIN and RATING_MAX + pub fn validate_rating(rating: u32) -> Result<(), ContractError> { + if !(RATING_MIN..=RATING_MAX).contains(&rating) { + return Err(ContractError::InvalidRating); + } + Ok(()) + } + + /// Legacy function - kept for backward compatibility but deprecated pub fn validate_string_length( - value: &String, - min_length: u32, - max_length: u32, + _value: &String, + _min_length: u32, + _max_length: u32, _field_name: &str, ) -> Result<(), ContractError> { - let length = value.len(); - - if length < min_length || length > max_length { - Err(ContractError::InvalidProjectData) - } else { - Ok(()) - } + // This function is deprecated - use specific validation functions instead + Err(ContractError::InvalidProjectData) } - pub fn is_valid_ipfs_cid(cid: &String) -> bool { - let len = cid.len(); - (46..=100).contains(&len) + /// Legacy function - kept for backward compatibility + pub fn is_valid_ipfs_cid(_cid: &String) -> bool { + // Use validate_ipfs_cid instead + false } + /// Legacy function - kept for backward compatibility pub fn is_valid_url(_url: &String) -> bool { - true + // Use validate_website_url instead + false } +} pub fn sanitize_string(input: &String) -> String { input.clone() diff --git a/dongle-smartcontract/src/verification_registry.rs b/dongle-smartcontract/src/verification_registry.rs index a41e08f..7e94c5d 100644 --- a/dongle-smartcontract/src/verification_registry.rs +++ b/dongle-smartcontract/src/verification_registry.rs @@ -10,6 +10,7 @@ use crate::fee_manager::FeeManager; use crate::project_registry::ProjectRegistry; use crate::storage_keys::StorageKey; use crate::types::{VerificationRecord, VerificationStatus}; +use crate::utils::Utils; use soroban_sdk::{Address, Env, String}; pub struct VerificationRegistry; @@ -158,10 +159,7 @@ impl VerificationRegistry { } pub fn validate_evidence_cid(evidence_cid: &String) -> Result<(), ContractError> { - if evidence_cid.is_empty() { - return Err(ContractError::InvalidProjectData); - } - Ok(()) + Utils::validate_evidence_cid(evidence_cid) } #[allow(dead_code)]