feat: Configure test harness, fix Soroban parser logic, and enable coverage (Issue #21)

JerryIdoko · JerryIdoko · commit cb42dfed71fe · 2026-01-26T14:21:08.000Z
diff --git a/.gitignore b/.gitignore
@@ -49,3 +49,4 @@ cache/
 artifacts/
 out/
 .soroban/
+tarpaulin-report.html
diff --git 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
@@ -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
@@ -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
         )
     }
-}
+}
diff --git 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
@@ -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::*;
diff --git 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<String>) {
         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<String>) {
                 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<String>) {
 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"
     )
-}
+}
diff --git 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,17 +205,18 @@ impl SorobanAnalyzer {
     }
     
     /// Check for expensive operations in functions
-    fn check_expensive_operations(function: &SorobanFunction, source: &str) -> Vec<RuleViolation> {
+    fn check_expensive_operations(function: &SorobanFunction, _source: &str) -> Vec<RuleViolation> {
         let mut violations = Vec::new();
         let function_source = &function.raw_definition;
         
         // Check for string operations
         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<RuleViolation> {
+    fn check_unbounded_loops(implementation: &SorobanImpl, _source: &str) -> Vec<RuleViolation> {
         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<RuleViolation> {
+    fn check_storage_patterns(implementation: &SorobanImpl, _source: &str) -> Vec<RuleViolation> {
         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
diff --git a/packages/rules/src/soroban/rule_engine.rs b/packages/rules/src/soroban/rule_engine.rs
diff --git a/packages/rules/src/unused_state_variables.rs b/packages/rules/src/unused_state_variables.rs
