From cb42dfed71fec0afce87a747354e91b6578ae433 Mon Sep 17 00:00:00 2001 From: JerryIdoko Date: Mon, 26 Jan 2026 14:21:08 +0000 Subject: [PATCH] feat: Configure test harness, fix Soroban parser logic, and enable coverage (Issue #21) --- .gitignore | 1 + Cargo.toml | 25 ------ libs/engine/Cargo.toml | 4 + libs/engine/src/analyzer.rs | 7 +- packages/rules/Cargo.toml | 4 + packages/rules/src/lib.rs | 24 +++++- packages/rules/src/rule_engine.rs | 82 ++++-------------- packages/rules/src/soroban/analyzer.rs | 47 ++++++---- packages/rules/src/soroban/parser.rs | 90 ++++++++------------ packages/rules/src/soroban/rule_engine.rs | 53 +++++++----- packages/rules/src/unused_state_variables.rs | 33 ++++--- 11 files changed, 168 insertions(+), 202 deletions(-) diff --git a/.gitignore b/.gitignore index f46f88a..466d861 100644 --- a/.gitignore +++ b/.gitignore @@ -49,3 +49,4 @@ cache/ artifacts/ out/ .soroban/ +tarpaulin-report.html diff --git a/Cargo.toml b/Cargo.toml index 5da336f..ed191eb 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -11,28 +11,3 @@ edition = "2021" authors = ["GasGuard Team"] license = "MIT" description = "Automated Optimization Suite for Stellar Soroban Contracts" - -[workspace.dependencies] -# Async runtime -tokio = { version = "1.0", features = ["full"] } - -# CLI and argument parsing -clap = { version = "4.0", features = ["derive"] } - -# Serialization -serde = { version = "1.0", features = ["derive"] } -serde_json = "1.0" - -# Error handling -anyhow = "1.0" -thiserror = "1.0" - -# Parsing and AST -syn = { version = "2.0", features = ["full", "extra-traits"] } -quote = "1.0" -proc-macro2 = "1.0" - -# Utilities -colored = "2.0" -walkdir = "2.0" -chrono = { version = "0.4", features = ["serde"] } diff --git a/libs/engine/Cargo.toml b/libs/engine/Cargo.toml index 11f73b2..35ef177 100644 --- a/libs/engine/Cargo.toml +++ b/libs/engine/Cargo.toml @@ -13,3 +13,7 @@ anyhow = "1.0" colored = "2.0" chrono = { version = "0.4", features = ["serde"] } walkdir = "2.0" + +[dev-dependencies] +mockall = "0.14.0" +rstest = "0.26.1" diff --git a/libs/engine/src/analyzer.rs b/libs/engine/src/analyzer.rs index 9eaebec..2a168c1 100644 --- a/libs/engine/src/analyzer.rs +++ b/libs/engine/src/analyzer.rs @@ -63,7 +63,7 @@ impl ScanAnalyzer { let mut estimated_savings_kb = 0.0; for violation in violations { - if violation.rule_name == "unused-state-variables" { + if violation.rule_name == "unused-state-variable" { unused_vars += 1; // Estimate average storage cost per unused variable // This is a rough estimate - actual costs vary by type @@ -92,6 +92,9 @@ impl ScanAnalyzer { for violation in violations { match violation.severity { ViolationSeverity::Error => errors.push(violation), + // Map High and Medium to Warnings for now + ViolationSeverity::High => warnings.push(violation), + ViolationSeverity::Medium => warnings.push(violation), ViolationSeverity::Warning => warnings.push(violation), ViolationSeverity::Info => info.push(violation), } @@ -136,4 +139,4 @@ impl fmt::Display for StorageSavings { self.monthly_ledger_rent_savings ) } -} +} \ No newline at end of file diff --git a/packages/rules/Cargo.toml b/packages/rules/Cargo.toml index 99ccb70..ee77206 100644 --- a/packages/rules/Cargo.toml +++ b/packages/rules/Cargo.toml @@ -11,3 +11,7 @@ serde = { version = "1.0", features = ["derive"] } serde_json = "1.0" thiserror = "1.0" regex = "1.10" + +[dev-dependencies] +mockall = "0.14.0" +rstest = "0.26.1" diff --git a/packages/rules/src/lib.rs b/packages/rules/src/lib.rs index e844646..8d179b0 100644 --- a/packages/rules/src/lib.rs +++ b/packages/rules/src/lib.rs @@ -3,7 +3,23 @@ pub mod unused_state_variables; pub mod vyper; pub mod soroban; -pub use rule_engine::*; -pub use unused_state_variables::*; -pub use vyper::*; -pub use soroban::*; +// Explicitly export core types to avoid ambiguity +pub use rule_engine::{Rule, RuleEngine, RuleViolation, ViolationSeverity, extract_struct_fields, find_variable_usage}; +pub use unused_state_variables::UnusedStateVariablesRule; + +// Export Soroban types specifically +pub use soroban::{ + SorobanAnalyzer, + SorobanContract, + SorobanParser, + SorobanResult, + SorobanRuleEngine, + SorobanStruct, + SorobanImpl, + SorobanFunction, + SorobanField, + SorobanParam +}; + +// Export Vyper types (keeping glob here is fine if Vyper module is clean, but let's be safe) +pub use vyper::*; \ No newline at end of file diff --git a/packages/rules/src/rule_engine.rs b/packages/rules/src/rule_engine.rs index 54a8b8e..b762a6f 100644 --- a/packages/rules/src/rule_engine.rs +++ b/packages/rules/src/rule_engine.rs @@ -1,10 +1,11 @@ +//! Rule Engine Core +//! +//! Provides the fundamental traits and AST traversal logic for the rules engine. + use serde::{Deserialize, Serialize}; use std::collections::HashSet; use syn::{Expr, Item, ItemImpl, ItemStruct, Member, Pat}; -// Re-export from soroban module -pub use crate::soroban::{SorobanRuleEngine, SorobanContract, SorobanParser, SorobanResult}; - #[derive(Debug, Clone, Serialize, Deserialize)] pub struct RuleViolation { pub rule_name: String, @@ -19,6 +20,8 @@ pub struct RuleViolation { #[derive(Debug, Clone, Serialize, Deserialize)] pub enum ViolationSeverity { Error, + High, + Medium, Warning, Info, } @@ -100,7 +103,6 @@ fn extract_variables_from_expr(expr: &Expr, used_vars: &mut HashSet) { Expr::Path(path) => { if let Some(segment) = path.path.segments.last() { let ident = segment.ident.to_string(); - // Skip common Rust keywords and types if !is_rust_keyword(&ident) { used_vars.insert(ident); } @@ -112,6 +114,10 @@ fn extract_variables_from_expr(expr: &Expr, used_vars: &mut HashSet) { used_vars.insert(ident.to_string()); } } + Expr::Assign(assign) => { + extract_variables_from_expr(&assign.left, used_vars); + extract_variables_from_expr(&assign.right, used_vars); + } Expr::MethodCall(method_call) => { extract_variables_from_expr(&method_call.receiver, used_vars); for arg in &method_call.args { @@ -211,64 +217,12 @@ fn extract_variables_from_pat(pat: &Pat, used_vars: &mut HashSet) { fn is_rust_keyword(ident: &str) -> bool { matches!( ident, - "self" - | "Self" - | "super" - | "crate" - | "mod" - | "use" - | "pub" - | "const" - | "static" - | "let" - | "fn" - | "struct" - | "enum" - | "impl" - | "trait" - | "where" - | "for" - | "while" - | "loop" - | "if" - | "else" - | "match" - | "break" - | "continue" - | "return" - | "async" - | "await" - | "move" - | "ref" - | "mut" - | "unsafe" - | "extern" - | "type" - | "union" - | "macro" - | "Some" - | "None" - | "Ok" - | "Err" - | "Result" - | "Option" - | "Vec" - | "String" - | "str" - | "bool" - | "u8" - | "u16" - | "u32" - | "u64" - | "u128" - | "i8" - | "i16" - | "i32" - | "i64" - | "i128" - | "f32" - | "f64" - | "usize" - | "isize" + "self" | "Self" | "super" | "crate" | "mod" | "use" | "pub" | "const" | "static" | "let" + | "fn" | "struct" | "enum" | "impl" | "trait" | "where" | "for" | "while" | "loop" + | "if" | "else" | "match" | "break" | "continue" | "return" | "async" | "await" + | "move" | "ref" | "mut" | "unsafe" | "extern" | "type" | "union" | "macro" | "Some" + | "None" | "Ok" | "Err" | "Result" | "Option" | "Vec" | "String" | "str" | "bool" + | "u8" | "u16" | "u32" | "u64" | "u128" | "i8" | "i16" | "i32" | "i64" | "i128" + | "f32" | "f64" | "usize" | "isize" ) -} +} \ No newline at end of file diff --git a/packages/rules/src/soroban/analyzer.rs b/packages/rules/src/soroban/analyzer.rs index 1887d00..2220840 100644 --- a/packages/rules/src/soroban/analyzer.rs +++ b/packages/rules/src/soroban/analyzer.rs @@ -92,6 +92,7 @@ impl SorobanAnalyzer { description: "Contract should have a constructor function for initialization".to_string(), suggestion: "Add a 'new' function that initializes the contract state".to_string(), line_number: 1, + column_number: 0, variable_name: contract.name.clone(), severity: ViolationSeverity::Warning, }); @@ -112,6 +113,7 @@ impl SorobanAnalyzer { description: "Consider adding an admin/owner field for access control".to_string(), suggestion: "Add an 'admin: Address' field to your contract state".to_string(), line_number: 1, + column_number: 0, variable_name: contract.name.clone(), severity: ViolationSeverity::Info, }); @@ -128,14 +130,15 @@ impl SorobanAnalyzer { // Count occurrences of field name in the source (excluding struct definition) let field_usage_count = source.matches(&field.name).count(); - // If field is only mentioned in its own declaration, it's likely unused - // (this is a heuristic - a more sophisticated analysis would be needed for production) - if field_usage_count <= 1 { + // Heuristic: Definition + Initialization = 2 occurrences. + // If it's <= 2, it's likely defined and initialized but never accessed again. + if field_usage_count <= 2 { violations.push(RuleViolation { rule_name: "unused-state-variable".to_string(), description: format!("State variable '{}' appears to be unused", field.name), suggestion: format!("Remove unused state variable '{}' to save ledger storage", field.name), line_number: field.line_number, + column_number: 0, variable_name: field.name.clone(), severity: ViolationSeverity::Warning, }); @@ -155,8 +158,9 @@ impl SorobanAnalyzer { violations.push(RuleViolation { rule_name: "inefficient-integer-type".to_string(), description: format!("Field '{}' uses {} which may be unnecessarily large", field.name, field.type_name), - suggestion: format!("Consider using a smaller integer type like u64 or u32 if the range permits", field.name), + suggestion: "Consider using a smaller integer type like u64 or u32 if the range permits".to_string(), line_number: field.line_number, + column_number: 0, variable_name: field.name.clone(), severity: ViolationSeverity::Info, }); @@ -169,6 +173,7 @@ impl SorobanAnalyzer { description: format!("Field '{}' uses String type", field.name), suggestion: "Consider using Symbol for fixed string values to save storage costs".to_string(), line_number: field.line_number, + column_number: 0, variable_name: field.name.clone(), severity: ViolationSeverity::Info, }); @@ -189,6 +194,7 @@ impl SorobanAnalyzer { description: format!("Field '{}' is private but contract fields should typically be public", field.name), suggestion: format!("Change '{}' to 'pub {}' to make it accessible", field.name, field.name), line_number: field.line_number, + column_number: 0, variable_name: field.name.clone(), severity: ViolationSeverity::Warning, }); @@ -199,7 +205,7 @@ impl SorobanAnalyzer { } /// Check for expensive operations in functions - fn check_expensive_operations(function: &SorobanFunction, source: &str) -> Vec { + fn check_expensive_operations(function: &SorobanFunction, _source: &str) -> Vec { let mut violations = Vec::new(); let function_source = &function.raw_definition; @@ -207,9 +213,10 @@ impl SorobanAnalyzer { if function_source.contains(".to_string()") || function_source.contains("String::from(") { violations.push(RuleViolation { rule_name: "expensive-string-operation".to_string(), - description: "String operations can be expensive in terms of gas/storage", - suggestion: "Consider using Symbol or Bytes for fixed data, or minimize string operations", + description: "String operations can be expensive in terms of gas/storage".to_string(), + suggestion: "Consider using Symbol or Bytes for fixed data, or minimize string operations".to_string(), line_number: function.line_number, + column_number: 0, variable_name: function.name.clone(), severity: ViolationSeverity::Medium, }); @@ -219,9 +226,10 @@ impl SorobanAnalyzer { if function_source.contains("Vec::new()") && !function_source.contains("with_capacity") { violations.push(RuleViolation { rule_name: "vec-without-capacity".to_string(), - description: "Vec::new() without capacity can cause multiple reallocations", - suggestion: "Use Vec::with_capacity() to pre-allocate memory when size is known", + description: "Vec::new() without capacity can cause multiple reallocations".to_string(), + suggestion: "Use Vec::with_capacity() to pre-allocate memory when size is known".to_string(), line_number: function.line_number, + column_number: 0, variable_name: function.name.clone(), severity: ViolationSeverity::Medium, }); @@ -231,9 +239,10 @@ impl SorobanAnalyzer { if function_source.contains(".clone()") { violations.push(RuleViolation { rule_name: "unnecessary-clone".to_string(), - description: "Clone operations increase resource usage and gas costs", - suggestion: "Avoid unnecessary cloning, use references where possible", + description: "Clone operations increase resource usage and gas costs".to_string(), + suggestion: "Avoid unnecessary cloning, use references where possible".to_string(), line_number: function.line_number, + column_number: 0, variable_name: function.name.clone(), severity: ViolationSeverity::Medium, }); @@ -254,8 +263,9 @@ impl SorobanAnalyzer { violations.push(RuleViolation { rule_name: "missing-address-validation".to_string(), description: format!("Function '{}' takes Address parameter but may lack validation", function.name), - suggestion: "Validate Address parameters to prevent invalid addresses", + suggestion: "Validate Address parameters to prevent invalid addresses".to_string(), line_number: function.line_number, + column_number: 0, variable_name: function.name.clone(), severity: ViolationSeverity::Medium, }); @@ -279,8 +289,9 @@ impl SorobanAnalyzer { violations.push(RuleViolation { rule_name: "missing-error-handling".to_string(), description: format!("Function '{}' should return Result for error handling", function.name), - suggestion: "Return Result<(), Error> to properly handle operation failures", + suggestion: "Return Result<(), Error> to properly handle operation failures".to_string(), line_number: function.line_number, + column_number: 0, variable_name: function.name.clone(), severity: ViolationSeverity::Medium, }); @@ -291,7 +302,7 @@ impl SorobanAnalyzer { } /// Check for unbounded loops - fn check_unbounded_loops(implementation: &SorobanImpl, source: &str) -> Vec { + fn check_unbounded_loops(implementation: &SorobanImpl, _source: &str) -> Vec { let mut violations = Vec::new(); for function in &implementation.functions { @@ -304,8 +315,9 @@ impl SorobanAnalyzer { violations.push(RuleViolation { rule_name: "unbounded-loop".to_string(), description: format!("Function '{}' contains potentially unbounded loop", function.name), - suggestion: "Ensure loops have clear termination conditions to prevent CPU limit exhaustion", + suggestion: "Ensure loops have clear termination conditions to prevent CPU limit exhaustion".to_string(), line_number: function.line_number, + column_number: 0, variable_name: function.name.clone(), severity: ViolationSeverity::High, }); @@ -316,7 +328,7 @@ impl SorobanAnalyzer { } /// Check for inefficient storage patterns - fn check_storage_patterns(implementation: &SorobanImpl, source: &str) -> Vec { + fn check_storage_patterns(implementation: &SorobanImpl, _source: &str) -> Vec { let mut violations = Vec::new(); // Check for multiple storage reads of the same key @@ -339,8 +351,9 @@ impl SorobanAnalyzer { violations.push(RuleViolation { rule_name: "inefficient-storage-access".to_string(), description: format!("Function '{}' performs {} storage reads - consider caching", function.name, read_count), - suggestion: "Cache frequently accessed storage values in local variables", + suggestion: "Cache frequently accessed storage values in local variables".to_string(), line_number: function.line_number, + column_number: 0, variable_name: function.name.clone(), severity: ViolationSeverity::Medium, }); diff --git a/packages/rules/src/soroban/parser.rs b/packages/rules/src/soroban/parser.rs index eb9582c..9b58cbc 100644 --- a/packages/rules/src/soroban/parser.rs +++ b/packages/rules/src/soroban/parser.rs @@ -5,7 +5,6 @@ use super::*; use regex::Regex; -use std::collections::HashMap; /// Parses Soroban contracts from source code pub struct SorobanParser; @@ -15,8 +14,9 @@ impl SorobanParser { pub fn parse_contract(source: &str, file_path: &str) -> SorobanResult { let lines: Vec<&str> = source.lines().collect(); - // Extract contract name from #[contract] attribute - let contract_name = Self::extract_contract_name(source)?; + // Extract contract name from #[contract] attribute, or fallback to first struct + let contract_name = Self::extract_contract_name(source) + .unwrap_or_else(|_| "UnknownContract".to_string()); // Parse struct definitions with #[contracttype] let contract_types = Self::parse_contract_types(&lines)?; @@ -43,8 +43,7 @@ impl SorobanParser { } } - // If no explicit name, try to infer from struct names - let struct_re = Regex::new(r#"#\s*\[\s*contracttype\s*\][\s\S]*?struct\s+(\w+)"#).unwrap(); + let struct_re = Regex::new(r#"#\s*\[\s*contracttype\s*\][\s\S]*?(?:pub\s+)?struct\s+(\w+)"#).unwrap(); if let Some(captures) = struct_re.captures(source) { if let Some(name) = captures.get(1) { return Ok(name.as_str().to_string()); @@ -62,13 +61,10 @@ impl SorobanParser { let mut i = 0; while i < lines.len() { - // Look for #[contracttype] attribute if lines[i].trim().starts_with("#[contracttype]") { let line_number = i + 1; - - // Skip to the struct definition i += 1; - while i < lines.len() && !lines[i].trim().starts_with("struct") { + while i < lines.len() && !lines[i].trim().contains("struct") { i += 1; } @@ -76,7 +72,6 @@ impl SorobanParser { break; } - // Parse the struct if let Some(soroban_struct) = Self::parse_single_struct(&lines[i..], line_number)? { structs.push(soroban_struct); } @@ -89,13 +84,11 @@ impl SorobanParser { /// Parse a single struct definition fn parse_single_struct(lines: &[&str], start_line: usize) -> SorobanResult> { - if lines.is_empty() || !lines[0].trim().starts_with("struct") { + if lines.is_empty() || !lines[0].trim().contains("struct") { return Ok(None); } let struct_line = lines[0].trim(); - - // Extract struct name let name_re = Regex::new(r"struct\s+(\w+)").unwrap(); let name = name_re.captures(struct_line) .and_then(|caps| caps.get(1)) @@ -104,16 +97,19 @@ impl SorobanParser { format!("Could not parse struct name from: {}", struct_line) ))?; - // Find the opening brace let mut brace_count = 0; let mut struct_lines = vec![struct_line]; let mut i = 1; + if struct_line.contains('{') { + brace_count += 1; + } + while i < lines.len() { let line = lines[i].trim(); struct_lines.push(line); - if line.contains('{') { + if line.contains('{') && i > 0 { brace_count += 1; } if line.contains('}') { @@ -125,7 +121,6 @@ impl SorobanParser { i += 1; } - // Parse fields let fields = Self::parse_struct_fields(&struct_lines, start_line)?; Ok(Some(SorobanStruct { @@ -139,22 +134,17 @@ impl SorobanParser { /// Parse fields from a struct definition fn parse_struct_fields(lines: &[&str], base_line: usize) -> SorobanResult> { let mut fields = Vec::new(); - - // Join lines and extract content between braces let full_content = lines.join(" "); let fields_content = Self::extract_between_braces(&full_content) .ok_or_else(|| SorobanParseError::ParseError("Could not extract struct fields".to_string()))?; - // Split by comma to get individual fields - let field_parts: Vec<&str> = fields_content.split(',').collect(); + let field_parts = Self::split_preserving_parentheses(&fields_content, ','); for (index, field_part) in field_parts.iter().enumerate() { let field_part = field_part.trim(); if field_part.is_empty() { continue; } - - // Parse field: visibility, name, and type if let Some(field) = Self::parse_field(field_part, base_line + index)? { fields.push(field); } @@ -170,23 +160,19 @@ impl SorobanParser { return Ok(None); } - // Handle visibility modifiers let (visibility, remaining) = if field_str.starts_with("pub ") { (FieldVisibility::Public, &field_str[4..]) } else { (FieldVisibility::Private, field_str) }; - // Split by colon to separate name and type let parts: Vec<&str> = remaining.split(':').collect(); - if parts.len() != 2 { - return Err(SorobanParseError::ParseError( - format!("Invalid field format: {}", field_str) - )); + if parts.len() < 2 { + return Ok(None); } let name = parts[0].trim().to_string(); - let type_name = parts[1].trim().to_string(); + let type_name = parts[1..].join(":").trim().to_string(); Ok(Some(SorobanField { name, @@ -202,11 +188,8 @@ impl SorobanParser { let mut i = 0; while i < lines.len() { - // Look for #[contractimpl] attribute if lines[i].trim().starts_with("#[contractimpl]") { let line_number = i + 1; - - // Skip to the impl definition i += 1; while i < lines.len() && !lines[i].trim().starts_with("impl") { i += 1; @@ -216,7 +199,6 @@ impl SorobanParser { break; } - // Parse the impl block if let Some(implementation) = Self::parse_single_impl(&lines[i..], line_number)? { implementations.push(implementation); } @@ -234,8 +216,6 @@ impl SorobanParser { } let impl_line = lines[0].trim(); - - // Extract target type (struct name) let target_re = Regex::new(r"impl\s+(?:.*?\s+for\s+)?(\w+)").unwrap(); let target = target_re.captures(impl_line) .and_then(|caps| caps.get(1)) @@ -244,17 +224,20 @@ impl SorobanParser { format!("Could not parse impl target from: {}", impl_line) ))?; - // Find the opening brace and parse functions let mut brace_count = 0; let mut impl_lines = vec![impl_line]; let mut i = 1; let mut functions = Vec::new(); + if impl_line.contains('{') { + brace_count += 1; + } + while i < lines.len() { let line = lines[i].trim(); impl_lines.push(line); - if line.contains('{') { + if line.contains('{') && i > 0 { brace_count += 1; } @@ -265,8 +248,13 @@ impl SorobanParser { } } - // Parse function definitions within the impl block - if brace_count == 1 && line.starts_with("pub ") && line.contains("fn ") { + // Correct logic to identify functions inside impl block: + // We allow brace_count 2 IF the current line starts the function (contains '{') + // Otherwise brace_count must be 1 (direct child of impl) + let is_fn_def = line.starts_with("pub ") && line.contains("fn "); + let correct_depth = brace_count == 1 || (brace_count == 2 && line.contains('{')); + + if is_fn_def && correct_depth { if let Some(function) = Self::parse_function(&lines[i..], start_line + i)? { functions.push(function); } @@ -294,7 +282,6 @@ impl SorobanParser { return Ok(None); } - // Extract function name let name_re = Regex::new(r"fn\s+(\w+)").unwrap(); let name = name_re.captures(func_line) .and_then(|caps| caps.get(1)) @@ -303,16 +290,10 @@ impl SorobanParser { format!("Could not parse function name from: {}", func_line) ))?; - // Extract parameters - let params = Self::extract_parameters(func_line)?; - - // Extract return type - let return_type = Self::extract_return_type(func_line)?; - - // Determine if it's a constructor (new function typically) + let params = Self::extract_parameters(func_line).unwrap_or_default(); + let return_type = Self::extract_return_type(func_line).unwrap_or(None); let is_constructor = name == "new" || name.ends_with("_init"); - // Get full function definition (find closing brace) let mut brace_count = 0; let mut func_lines = vec![func_line]; let mut i = 1; @@ -338,7 +319,7 @@ impl SorobanParser { name, params, return_type, - visibility: FunctionVisibility::Public, // All contract functions are public + visibility: FunctionVisibility::Public, is_constructor, line_number: start_line, raw_definition: func_lines.join("\n"), @@ -351,9 +332,7 @@ impl SorobanParser { .ok_or_else(|| SorobanParseError::ParseError("Could not extract parameters".to_string()))?; let mut params = Vec::new(); - - // Split by comma, handling nested parentheses - let param_parts = Self::split_preserving_parentheses(params_section, ','); + let param_parts = Self::split_preserving_parentheses(¶ms_section, ','); for param_part in param_parts { let param_part = param_part.trim(); @@ -361,11 +340,10 @@ impl SorobanParser { continue; } - // Split by colon to separate name and type let parts: Vec<&str> = param_part.split(':').collect(); - if parts.len() == 2 { + if parts.len() >= 2 { let name = parts[0].trim().to_string(); - let type_name = parts[1].trim().to_string(); + let type_name = parts[1..].join(":").trim().to_string(); params.push(SorobanParam { name, type_name }); } } @@ -375,7 +353,6 @@ impl SorobanParser { /// Extract return type from function signature fn extract_return_type(func_signature: &str) -> SorobanResult> { - // Look for -> return type pattern let return_re = Regex::new(r"->\s*([^{\n]+)").unwrap(); if let Some(captures) = return_re.captures(func_signature) { @@ -513,6 +490,7 @@ impl TokenContract { assert_eq!(struct_def.fields.len(), 2); let impl_block = &contract.implementations[0]; + // This assertion failed previously because brace counting was off assert_eq!(impl_block.functions.len(), 2); assert_eq!(impl_block.functions[0].name, "new"); assert_eq!(impl_block.functions[1].name, "get_total_supply"); diff --git a/packages/rules/src/soroban/rule_engine.rs b/packages/rules/src/soroban/rule_engine.rs index 06c5e35..f5606d1 100644 --- a/packages/rules/src/soroban/rule_engine.rs +++ b/packages/rules/src/soroban/rule_engine.rs @@ -1,9 +1,8 @@ //! Soroban-specific rule engine //! -//! This module provides a specialized rule engine for analyzing Soroban smart contracts -//! with rules tailored to Soroban's unique characteristics and gas optimization patterns. +//! This module provides a specialized rule engine for analyzing Soroban smart contracts. -use super::soroban::{SorobanAnalyzer, SorobanContract, SorobanParser, SorobanResult}; +use crate::soroban::{SorobanAnalyzer, SorobanContract, SorobanParser, SorobanResult}; use crate::{RuleViolation, ViolationSeverity}; use std::collections::HashMap; @@ -39,14 +38,14 @@ impl SorobanRuleEngine { /// Add all default Soroban rules fn add_default_rules(&mut self) { - self.add_rule(UnusedStateVariablesRule) - .add_rule(InefficientStorageAccessRule) - .add_rule(UnboundedLoopRule) - .add_rule(ExpensiveStringOperationsRule) - .add_rule(MissingConstructorRule) - .add_rule(AdminPatternRule) - .add_rule(InefficientIntegerTypesRule) - .add_rule(MissingErrorHandlingRule); + self.add_rule(UnusedStateVariablesRule::default()) + .add_rule(InefficientStorageAccessRule::default()) + .add_rule(UnboundedLoopRule::default()) + .add_rule(ExpensiveStringOperationsRule::default()) + .add_rule(MissingConstructorRule::default()) + .add_rule(AdminPatternRule::default()) + .add_rule(InefficientIntegerTypesRule::default()) + .add_rule(MissingErrorHandlingRule::default()); } /// Analyze Soroban contract source code @@ -105,6 +104,8 @@ pub trait SorobanRule: Send + Sync { fn apply(&self, contract: &SorobanContract) -> Vec; } +// --- Specific Rule Implementations --- + /// Rule for detecting unused state variables pub struct UnusedStateVariablesRule { enabled: bool, @@ -146,14 +147,15 @@ impl SorobanRule for UnusedStateVariablesRule { for contract_type in &contract.contract_types { for field in &contract_type.fields { - // Simple heuristic: if field name appears only once in source, it's likely unused + // Simple heuristic: Definition + Initialization = 2 occurrences. let occurrences = contract.source.matches(&field.name).count(); - if occurrences <= 1 { + if occurrences <= 2 { violations.push(RuleViolation { rule_name: self.id().to_string(), description: format!("State variable '{}' appears to be unused", field.name), suggestion: format!("Remove unused state variable '{}' to save ledger storage costs", field.name), line_number: field.line_number, + column_number: 0, variable_name: field.name.clone(), severity: self.severity(), }); @@ -221,8 +223,9 @@ impl SorobanRule for InefficientStorageAccessRule { violations.push(RuleViolation { rule_name: self.id().to_string(), description: format!("Function '{}' performs {} storage operations - consider caching", function.name, total_ops), - suggestion: "Cache frequently accessed storage values in local variables to reduce ledger interactions", + suggestion: "Cache frequently accessed storage values in local variables to reduce ledger interactions".to_string(), line_number: function.line_number, + column_number: 0, variable_name: function.name.clone(), severity: self.severity(), }); @@ -288,8 +291,9 @@ impl SorobanRule for UnboundedLoopRule { violations.push(RuleViolation { rule_name: self.id().to_string(), description: format!("Function '{}' contains potentially unbounded loop", function.name), - suggestion: "Ensure loops have clear termination conditions to prevent CPU limit exhaustion", + suggestion: "Ensure loops have clear termination conditions to prevent CPU limit exhaustion".to_string(), line_number: function.line_number, + column_number: 0, variable_name: function.name.clone(), severity: self.severity(), }); @@ -351,8 +355,9 @@ impl SorobanRule for ExpensiveStringOperationsRule { violations.push(RuleViolation { rule_name: self.id().to_string(), description: format!("Function '{}' uses expensive string operations", function.name), - suggestion: "Consider using Symbol or Bytes for fixed data, or minimize string operations to reduce gas costs", + suggestion: "Consider using Symbol or Bytes for fixed data, or minimize string operations to reduce gas costs".to_string(), line_number: function.line_number, + column_number: 0, variable_name: function.name.clone(), severity: self.severity(), }); @@ -411,6 +416,7 @@ impl SorobanRule for MissingConstructorRule { description: "Contract lacks a constructor function for initialization".to_string(), suggestion: "Add a 'new' function that initializes the contract state properly".to_string(), line_number: 1, + column_number: 0, variable_name: contract.name.clone(), severity: self.severity(), }] @@ -468,9 +474,10 @@ impl SorobanRule for AdminPatternRule { if !has_admin { vec![RuleViolation { rule_name: self.id().to_string(), - description: "Consider adding an admin/owner field for access control", - suggestion: "Add an 'admin: Address' field to your contract state for administrative functions", + description: "Consider adding an admin/owner field for access control".to_string(), + suggestion: "Add an 'admin: Address' field to your contract state for administrative functions".to_string(), line_number: 1, + column_number: 0, variable_name: contract.name.clone(), severity: self.severity(), }] @@ -525,8 +532,9 @@ impl SorobanRule for InefficientIntegerTypesRule { violations.push(RuleViolation { rule_name: self.id().to_string(), description: format!("Field '{}' uses {} which may be unnecessarily large", field.name, field.type_name), - suggestion: format!("Consider using a smaller integer type like u64 or u32 if the range permits"), + suggestion: "Consider using a smaller integer type like u64 or u32 if the range permits".to_string(), line_number: field.line_number, + column_number: 0, variable_name: field.name.clone(), severity: self.severity(), }); @@ -590,8 +598,9 @@ impl SorobanRule for MissingErrorHandlingRule { violations.push(RuleViolation { rule_name: self.id().to_string(), description: format!("Function '{}' should return Result for proper error handling", function.name), - suggestion: "Return Result<(), Error> to properly handle operation failures and provide better error reporting", + suggestion: "Return Result<(), Error> to properly handle operation failures and provide better error reporting".to_string(), line_number: function.line_number, + column_number: 0, variable_name: function.name.clone(), severity: self.severity(), }); @@ -640,8 +649,8 @@ impl TestContract { } "#; - let engine = SorobanRuleEngine::new() - .add_rule(UnusedStateVariablesRule::default()); + let mut engine = SorobanRuleEngine::new(); + engine.add_rule(UnusedStateVariablesRule::default()); let violations = engine.analyze(source, "test.rs").unwrap(); diff --git a/packages/rules/src/unused_state_variables.rs b/packages/rules/src/unused_state_variables.rs index d3fc5ec..716dbe4 100644 --- a/packages/rules/src/unused_state_variables.rs +++ b/packages/rules/src/unused_state_variables.rs @@ -42,7 +42,7 @@ impl Rule for UnusedStateVariablesRule { var_name, struct_name ), severity: ViolationSeverity::Warning, - line_number: 0, // Line number tracking requires proc-macro2 span features + line_number: 0, column_number: 0, variable_name: var_name.clone(), suggestion: format!( @@ -99,20 +99,29 @@ impl UnusedStateVariablesRule { fn is_soroban_contract(&self, struct_item: &ItemStruct) -> bool { // Check for Soroban contract attributes for attr in &struct_item.attrs { - if let Meta::List(meta_list) = &attr.meta { - let path_str = meta_list.path.to_token_stream().to_string(); - if path_str.contains("contractimpl") - || path_str.contains("contracttype") - || path_str.contains("stellar_contract") - { - return true; + match &attr.meta { + Meta::List(meta_list) => { + let path_str = meta_list.path.to_token_stream().to_string(); + if path_str.contains("contractimpl") + || path_str.contains("contracttype") + || path_str.contains("stellar_contract") + { + return true; + } } + Meta::Path(path) => { + let path_str = path.to_token_stream().to_string(); + if path_str.contains("contractimpl") + || path_str.contains("contracttype") + || path_str.contains("stellar_contract") + { + return true; + } + } + _ => {} } } - // Check for common Soroban trait implementations - // This is a heuristic - in practice, we'd need to check if the struct - // implements Soroban contract traits false } @@ -216,4 +225,4 @@ mod tests { // Should find no violations assert_eq!(violations.len(), 0); } -} +} \ No newline at end of file