Add DAORegistry contract (SOV-P0-2.3) #639
Conversation
Add dao_registry module with DAOEntry/DAOMetadata and DAORegistry implementation. Implement register_dao, get_dao, list_daos, update_metadata with owner-only access control and clear errors. Add DaoRegistered and DaoUpdated ContractEvent variants and serialization tests. Add unit tests for registry behavior and modify contracts re-exports.
|
🔴 CRITICAL CODE REVIEW - DO NOT MERGEI've completed a comprehensive code review. This PR has 4 CRITICAL issues blocking merge plus 6 important issues. Detailed inline comments follow on each file. Merge Blocker: Please fix all CRITICAL issues before re-requesting review. |
CRITICAL-1: Type Name Collision 🔴File: pub struct DAOMetadata { // ❌ COLLISION with existing DAOMetadata in types/dao.rs
pub dao_id: [u8; 32],
pub token_addr: [u8; 32],
pub class: String,
pub metadata_hash: Option<[u8; 32]>,
pub treasury: PublicKey,
pub owner: PublicKey,
pub created_at: u64,
}Problem: This conflicts with the existing Impact: Namespace confusion when both are re-exported. Callers won't know which DAOMetadata they're getting. Fix: Rename to pub struct DAORegistryEntry {
pub dao_id: [u8; 32],
// ... rest of fields
} |
CRITICAL-2: Orphaned Test File 🔴File: pub mod core;
pub mod types;
// ❌ MISSING: #[cfg(test)] mod tests;
pub use core::DAORegistry;
pub use types::{DAOEntry, DAOMetadata};Problem: The file Impact: All tests in tests.rs are DEAD CODE and will never run. Fix: Add the test module declaration: pub mod core;
pub mod types;
#[cfg(test)]
mod tests;
pub use core::DAORegistry;
pub use types::{DAOEntry, DAOMetadata}; |
CRITICAL-3: Silent Failure on Contract Deserialization 🔴File: if transaction.memo.len() > 4 && &transaction.memo[0..4] == b"ZHTP" {
let contract_data = &transaction.memo[4..];
let (call, signature): (ContractCall, Signature) =
bincode::deserialize(contract_data)?;
calls.push((call, signature));
}
// ❌ If memo format is invalid, we silently return Ok(Vec::new()) with NO ERROR!
Ok(calls) // Returns empty Vec, caller has no idea what happenedProblem: When a contract execution transaction arrives with a malformed memo (missing "ZHTP" marker or corrupted data), the function returns an empty Vec instead of an error. Impact:
Fix: Explicitly validate and return errors: if transaction.transaction_type == TransactionType::ContractExecution {
if transaction.memo.len() <= 4 {
return Err(anyhow!("Contract execution transaction has insufficient memo data: expected >4 bytes, got {}", transaction.memo.len()));
}
if &transaction.memo[0..4] != b"ZHTP" {
return Err(anyhow!("Invalid contract transaction marker: expected b\"ZHTP\", got {:?}", &transaction.memo[0..4]));
}
let contract_data = &transaction.memo[4..];
match bincode::deserialize::<(ContractCall, Signature)>(contract_data) {
Ok((call, signature)) => calls.push((call, signature)),
Err(e) => return Err(anyhow!("Failed to deserialize contract call: {} (memo_len={})", e, contract_data.len())),
}
}
Ok(calls) |
CRITICAL-4: Catch-All Error Swallowing in Contract Execution 🔴File: match self.executor.execute_call(call, &mut context) {
Ok(result) => results.push(result),
Err(e) => {
error!("Contract execution failed: {}", e);
// ❌ PROBLEM: Error is logged but then...
results.push(ContractResult::failure(context.gas_used));
// ❌ We push a failure result (not propagate error)
}
// ❌ Function returns Ok(results) - CALLER CAN'T TELL IF EXECUTION SUCCEEDED OR FAILED!
}Problem: Contract execution failures are caught, logged, but then returned as successful results. The caller gets Impact:
Fix: Add detailed context to error logs: match self.executor.execute_call(call, &mut context) {
Ok(result) => results.push(result),
Err(e) => {
error!(
"Contract execution failed: method={}, caller={:?}, contract_type={:?}, error: {}",
call.method,
call.get_caller(),
call.contract_type,
e
);
results.push(ContractResult::failure(context.gas_used));
}
} |
IMPORTANT-1: Missing Default Trait Implementation 🟠File: Problem: The Impact: Inconsistent API. Users expecting Fix: Add Default implementation: impl Default for DAORegistry {
fn default() -> Self {
Self::new()
}
} |
IMPORTANT-2: Duplicate Struct Types - DAOEntry and DAOMetadata are Identical 🟠File: pub struct DAOEntry {
pub dao_id: [u8; 32],
pub token_addr: [u8; 32],
pub class: String,
pub metadata_hash: Option<[u8; 32]>,
pub treasury: PublicKey,
pub owner: PublicKey,
pub created_at: u64,
}
pub struct DAOMetadata {
// ❌ IDENTICAL 7 fields!
pub dao_id: [u8; 32],
pub token_addr: [u8; 32],
pub class: String,
pub metadata_hash: Option<[u8; 32]>,
pub treasury: PublicKey,
pub owner: PublicKey,
pub created_at: u64,
}In core.rs get_dao(): Ok(DAOMetadata {
dao_id: entry.dao_id, // ❌ Manually copy all 7 fields!
token_addr: entry.token_addr,
class: entry.class.clone(),
metadata_hash: entry.metadata_hash,
treasury: entry.treasury.clone(),
owner: entry.owner.clone(),
created_at: entry.created_at,
})Problem: Code duplication and maintenance burden. If a field is added to one, the other is forgotten. Fix: Remove duplication: // Option 1: Type alias
pub type DAOMetadata = DAOEntry;
// Option 2: Return reference from get_dao()
pub fn get_dao(&self, token_addr: [u8; 32]) -> Result<&DAOEntry, String> |
IMPORTANT-3: Untyped class Field - Uses String Instead of DAOType Enum 🟠File: pub struct DAOEntry {
pub dao_id: [u8; 32],
pub token_addr: [u8; 32],
pub class: String, // ❌ UNTYPED: Accepts any string
pub metadata_hash: Option<[u8; 32]>,
// ...
}In tests: .register_dao(token, "NP".to_string(), None, treasury, owner) // ❌ String, not type-safeProblem: Using Solution: The codebase already has Fix: Use the existing use crate::types::dao::DAOType;
pub struct DAOEntry {
pub dao_id: [u8; 32],
pub token_addr: [u8; 32],
pub class: DAOType, // ✅ Type-safe, only NP or FP allowed
pub metadata_hash: Option<[u8; 32]>,
// ...
}
// In tests:
.register_dao(token, DAOType::NP, None, treasury, owner) // ✅ Type-safe |
IMPORTANT-4: Weak Error Messages in DAORegistry 🟠File: pub fn get_dao(&self, token_addr: [u8; 32]) -> Result<DAOMetadata, String> {
match self.token_index.get(&token_addr) {
Some(dao_id) => match self.registry.get(dao_id) {
Some(entry) => Ok(...),
None => Err("DAO entry not found for id".to_string()), // ❌ Which id? No context!
},
None => Err("DAO not found for token address".to_string()), // ❌ Which address? No help!
}
}Problem: Error messages don't include actual values being looked up. Developers cannot debug or understand what tokens are registered. Fix: Include actual values and registry state in error messages: pub fn get_dao(&self, token_addr: [u8; 32]) -> Result<DAOMetadata, String> {
match self.token_index.get(&token_addr) {
Some(dao_id) => match self.registry.get(dao_id) {
Some(entry) => Ok(...),
None => {
// Data corruption: index points to missing DAO
Err(format!(
"Data corruption: token index points to non-existent DAO (token_addr={:?}, dao_id={:?}, registry_size={})",
hex::encode(&token_addr),
hex::encode(dao_id),
self.registry.len()
))
}
},
None => Err(format!(
"DAO not found for token address: {} (registry_size={}, token_indices={})",
hex::encode(&token_addr),
self.registry.len(),
self.token_index.len()
)),
}
} |
IMPORTANT-5: Incomplete Error Propagation Chain 🟠File: let mut context = ExecutionContext::new(
call.get_caller().map_err(|e| anyhow\!(e))?, // ❌ Error wrapped but no context
height,
timestamp,
self.calculate_gas_limit(transaction)?, // ❌ If fails, no txn context
transaction.id().into(),
);
let caller_key = self.extract_public_key(transaction)?; // ❌ No txn height/context
if \!self.executor.validate_signature(&call, &signature, &caller_key)? { // ❌ Which call?
return Err(anyhow\!("Invalid signature")); // ❌ Which signature failed? Which call?
}Problem: Multiple fallible operations chain with
Fix: Add contextual error messages: let caller_result = call.get_caller()
.map_err(|e| anyhow\!("Failed to extract caller from contract call: {}", e))?;
let gas_limit = self.calculate_gas_limit(transaction)
.map_err(|e| anyhow\!("Failed to calculate gas limit for transaction (txid={:?}): {}", transaction.id(), e))?;
let mut context = ExecutionContext::new(
caller_result,
height,
timestamp,
gas_limit,
transaction.id().into(),
);
let caller_key = self.extract_public_key(transaction)
.map_err(|e| anyhow\!("Failed to extract public key for signature validation: {}", e))?;
if \!self.executor.validate_signature(&call, &signature, &caller_key)? {
return Err(anyhow\!(
"Signature validation failed for contract call (method={}, caller={:?}, txid={:?})",
call.method,
caller_result,
transaction.id()
));
} |
IMPORTANT-6: Insufficient Test Coverage 🟠File: Current Coverage: Only 3 tests covering ~40% of functionality Critical Test Gaps:
Fix: Add comprehensive tests before merge. Minimum 5 critical tests needed: #[test]
fn test_duplicate_token_registration_fails() { ... }
#[test]
fn test_registry_token_index_sync_invariant() { ... }
#[test]
fn test_update_metadata_nonexistent_dao_fails() { ... }
#[test]
fn test_event_data_matches_registered_dao() { ... }
#[test]
fn test_field_immutability_after_creation() { ... } |
There was a problem hiding this comment.
⛔ REVIEW SUMMARY - DO NOT MERGE
Comprehensive code review completed. This PR has critical issues blocking merge.
Issues Found:
- 4 CRITICAL issues (lines 17-26, lines 74-90, lines 53-59, lines 74-85 across files)
- 6 IMPORTANT issues (code quality, type safety, error handling, test coverage)
Merge Blocker:
ALL 4 critical issues MUST be fixed before re-requesting review:
- ✋ Type name collision:
DAOMetadataconflicts with existing type in types/dao.rs - ✋ Orphaned test file: tests.rs not declared in mod.rs (dead code)
- ✋ Silent failures: Contract deserialization failures ignored instead of errored
- ✋ Error swallowing: Contract execution failures masked as success
See detailed inline comments above for:
- Exact code locations
- Why each issue matters
- Specific code fixes needed
Add a new DAORegistry contract (Phase-0 SOV-P0-2.3).
Changes:
Notes:
Please review and merge to .