fix: Replace hardcoded solana_ prefix with BlockChainType lookup

[ritik4ever]

This commit addresses issue #409 by replacing the fragile string-based detection of Solana networks with proper blockchain type lookup.

Changes:

• Added validate_with_networks() method to Monitor ConfigLoader impl
• Updated validate_monitor_references() to use proper type detection
• Maintained backward compatibility with fallback to heuristic approach

Benefits:

• Works with custom Solana network names (not just solana_*)
• Prevents false positives from misleading network names
• Type-safe validation using BlockChainType enum
• No breaking changes to existing code

This builds on the Solana support added in #408 by making the network type detection more robust and flexible.

Fixes #409

I confirm that I have read and hereby agree to the OpenZeppelin Contributor License Agreement

Summary by CodeRabbit

• Improvements
  • Monitor configuration validation now includes network-aware checks to ensure configurations are validated against available networks
  • Enhanced error reporting with clearer messages when configuration validation fails
  • Strengthened overall validation process for improved system reliability

_{✏️ Tip: You can customize this high-level summary in your review settings.}

[github-actions]

All contributors have signed the CLA ✍️ ✅
_{Posted by the CLA Assistant Lite bot.}

[coderabbitai]

Important

Review skipped

Auto incremental reviews are disabled on this repository.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Walkthrough

Replaces hardcoded Solana network detection heuristics with runtime validation using actual network configurations. Introduces a new validate_with_networks() method accepting network metadata, refactors existing validation to delegate through this method for backward compatibility, and integrates network-aware validation into repository-level validation checks.

Changes

Cohort / File(s)	Summary
Monitor configuration validation src/models/config/monitor_config.rs	Adds public method validate_with_networks(networks: Option<&HashMap<String, Network>>) -> Result<(), ConfigError> that performs runtime BlockChainType lookups instead of string-matching heuristics. Refactors existing validate() to delegate to this new method with None, preserving backward compatibility. Extends public imports to include BlockChainType and Network types.
Repository-level validation integration src/repositories/monitor.rs	Integrates network-aware validation into validate_monitor_references() by calling monitor.validate_with_networks(Some(networks)) for each monitor, appending additional error context on validation failure. Supplements existing checks for non-existent triggers, networks, and script path validations.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~30 minutes

Poem

🐰 No more solana_ prefix guessing games,
Runtime checks bring clarity to network names,
From heuristic hints to types we truly know,
Validation flows more robust as it goes! ✨

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 inconclusive)

Check name	Status	Explanation	Resolution
Description check	❓ Inconclusive	The PR description provides a clear summary of changes, issue reference, and benefits, but does not follow the repository's template structure with explicit Testing Process, Checklist sections, or unit/integration test confirmation.	Add explicit Testing Process section describing test execution, and complete the Checklist items (unit tests, integration tests, documentation updates) with confirmation of what was added or an explanation if not applicable.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and specifically describes the main change: replacing hardcoded solana_ prefix with BlockChainType lookup, which is the primary technical improvement in this PR.
Linked Issues check	✅ Passed	The PR successfully addresses issue #409 by implementing a network-aware validation method (validate_with_networks) that uses BlockChainType lookup instead of string matching, with fallback support for backward compatibility.
Out of Scope Changes check	✅ Passed	All changes are directly related to the stated objective: adding validate_with_networks() method to Monitor, updating validate_monitor_references(), and maintaining backward compatibility, with no unrelated modifications detected.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[ritik4ever]

Please review my PR @shahnami

[coderabbitai]

Actionable comments posted: 1

🤖 Fix all issues with AI agents

In `@src/repositories/monitor.rs`:
- Around line 447-538: Add a new unit test that constructs a Network (use the
same Network struct and set its blockchain_type to BlockchainType::Solana),
insert it into the networks HashMap used by
MonitorRepository::validate_monitor_references (or call validate_with_networks
if available), then create two monitors via MonitorBuilder: one Solana monitor
using a function/event signature without parentheses and one identical monitor
but validate against a non-Solana network; assert that validation succeeds when
the network's blockchain_type is Solana and that an error is returned when the
blockchain_type is not Solana (use MonitorRepository::<NetworkRepository,
TriggerRepository>::validate_monitor_references or validate_with_networks and
check result.is_ok()/unwrap_err()). Ensure the test references MonitorBuilder,
MonitorRepository::validate_monitor_references (or validate_with_networks), and
BlockchainType::Solana so reviewers can locate the changes.

🧹 Nitpick comments (4)

src/models/config/monitor_config.rs (2)

163-173: Misleading doc comment.

The doc comment on lines 168-169 states this is "a public method exposed through an inherent impl on Monitor", but it appears to be defined inside the impl ConfigLoader for Monitor block. If validate_with_networks is part of the ConfigLoader trait, the comment should reflect that; if it's meant to be an inherent impl, it should be in a separate impl Monitor block.

Suggested doc comment fix

-	/// This is a public method exposed through an inherent impl on Monitor to allow
-	/// proper blockchain type detection in repository-level validation
+	/// This method enables proper blockchain type detection when network configurations
+	/// are available, falling back to heuristic detection for backward compatibility
 	pub fn validate_with_networks(

---

446-463: Missing test coverage for network-aware validation.

The new validate_with_networks() method with actual network context (Some(&networks)) is not tested. Consider adding tests that verify:

1. A monitor with a custom Solana network name (e.g., "my-solana-rpc") is correctly identified as a Solana monitor when network configs are provided
2. Parenthesis-less signatures pass validation for custom-named Solana networks

Example test for network-aware validation

#[test]
fn test_validate_with_networks_custom_solana_name() {
    use crate::models::{BlockChainType, Network};
    
    // Create a network with custom name but Solana type
    let mut networks = HashMap::new();
    // Note: You'll need to construct a Network with BlockChainType::Solana
    // networks.insert("my-custom-solana".to_string(), solana_network);
    
    let monitor = MonitorBuilder::new()
        .name("TestMonitor")
        .networks(vec!["my-custom-solana".to_string()])
        .function("transferSol", None)  // No parentheses - Solana style
        .build();
    
    // Should pass with network context
    assert!(monitor.validate_with_networks(Some(&networks)).is_ok());
    
    // Would fail without network context (heuristic doesn't match)
    assert!(monitor.validate_with_networks(None).is_err());
}

src/repositories/monitor.rs (2)

103-114: Validation is now performed twice with potential redundancy.

This block re-validates the entire monitor configuration, but most checks (name, networks, trigger conditions, script validation) already passed during Monitor::load_from_path(). The only differentiation is the Solana detection mechanism.

Consider either:

1. Documenting why double validation is acceptable (e.g., "re-validates with network context for accurate blockchain type detection")
2. Or extracting only the signature validation logic that benefits from network context into a separate method

This is acceptable as-is since the validation is fast, but the redundancy could cause confusion for future maintainers.

---

116-157: Duplicate trigger condition validation.

This block validates script paths, extensions, and timeout, but identical checks already run in Monitor::validate_with_networks() via validate_script_config() (lines 236-242 in monitor_config.rs). Both validate:

• Script path existence
• Extension matches language
• Timeout > 0

Since validate_with_networks(Some(networks)) is now called at line 105, this entire block is redundant and creates a maintenance burden.

Consider removing redundant validation

 			// Validate monitor configuration with network context
 			// This allows proper blockchain type detection instead of relying on naming conventions
 			if let Err(e) = monitor.validate_with_networks(Some(networks)) {
 				validation_errors.push(format!(
 					"Monitor '{}' configuration validation failed: {}",
 					monitor_name, e
 				));
 				metadata.insert(
 					format!("monitor_{}_validation_error", monitor_name),
 					e.to_string(),
 				);
 			}
-
-			// Validate custom trigger conditions
-			for condition in &monitor.trigger_conditions {
-				let script_path = Path::new(&condition.script_path);
-				if !script_path.exists() {
-					validation_errors.push(format!(
-						"Monitor '{}' has a custom filter script that does not exist: {}",
-						monitor_name, condition.script_path
-					));
-				}
-				// ... rest of duplicate validation ...
-			}

[shahnami]

Please review my PR @shahnami

Thanks @ritik4ever, the git diff is showing a lot more changes for some reason, I think it's related to adding whitespaces or something which is causing the diff to believe there are a lot more changes than there actually are. Could you fix this, and ensure you're formatting it correctly?

[ritik4ever]

Please review my PR @shahnami

Thanks @ritik4ever, the git diff is showing a lot more changes for some reason, I think it's related to adding whitespaces or something which is causing the diff to believe there are a lot more changes than there actually are. Could you fix this, and ensure you're formatting it correctly?

thanks , i will fix it .

[ritik4ever]

I confirm that I have read and hereby agree to the OpenZeppelin Contributor License Agreement

[shahnami]

Thanks for tackling the issue @ritik4ever!

The problem identification is spot-on, however I think there's a cleaner architectural approach we can take here.

Some concerns with the current approach:

1. Monitors get validated twice: once in load_from_path() → validate() (heuristic fallback), and again in validate_monitor_references() with actual network types.
2. The validate() path still falls back to prefix matching when networks is None, so the bug persists for some code paths.
3. validate_with_networks is added inside the trait impl but isn't part of ConfigLoader, which is a bit awkward.

If I can suggest an alternative approach..since signature validation is inherently context-dependent (needs network type), I'd suggest moving it entirely to the repository layer where that context already exists:

1. Remove signature validation from Monitor::validate() and keep it focused on monitor-intrinsic properties only.
2. Add a method on BlockChainType for validation rules, something like:

pub struct SignatureRules {
    pub requires_parentheses: bool,
    // Future: pub max_signature_length: Option<usize>,
    // Future: pub allowed_characters: &'static str,
}
impl BlockChainType {
    pub fn signature_rules(&self) -> SignatureRules {
        match self {
            BlockChainType::EVM | BlockChainType::Stellar | BlockChainType::Midnight => {
                SignatureRules { requires_parentheses: true }
            }
            BlockChainType::Solana => {
                SignatureRules { requires_parentheses: false }
            }
        }
    }
}

This pattern is nice because the exhaustive match forces handling new blockchain types, there is a single source of truth for per-chain behavior and it's easy to extend with more rules later.

3. Add a helper in the repository for signature validation:

// In repositories/monitor.rs
fn validate_monitor_signatures(monitor, networks, errors) {
    for network_slug in monitor.networks {
        let rules = networks[network_slug].network_type.signature_rules();
        if rules.requires_parentheses {
            // validate function/event signatures have parens
        }
    }
}

and then call it from validate_monitor_references().

What do you think? Happy to discuss if you have questions about this approach!

[ritik4ever]

Thanks for tackling the issue @ritik4ever!

The problem identification is spot-on, however I think there's a cleaner architectural approach we can take here.

Some concerns with the current approach:

1. Monitors get validated twice: once in load_from_path() → validate() (heuristic fallback), and again in validate_monitor_references() with actual network types.
2. The validate() path still falls back to prefix matching when networks is None, so the bug persists for some code paths.
3. validate_with_networks is added inside the trait impl but isn't part of ConfigLoader, which is a bit awkward.

If I can suggest an alternative approach..since signature validation is inherently context-dependent (needs network type), I'd suggest moving it entirely to the repository layer where that context already exists:

1. Remove signature validation from Monitor::validate() and keep it focused on monitor-intrinsic properties only.
2. Add a method on BlockChainType for validation rules, something like:

pub struct SignatureRules {
    pub requires_parentheses: bool,
    // Future: pub max_signature_length: Option<usize>,
    // Future: pub allowed_characters: &'static str,
}
impl BlockChainType {
    pub fn signature_rules(&self) -> SignatureRules {
        match self {
            BlockChainType::EVM | BlockChainType::Stellar | BlockChainType::Midnight => {
                SignatureRules { requires_parentheses: true }
            }
            BlockChainType::Solana => {
                SignatureRules { requires_parentheses: false }
            }
        }
    }
}

This pattern is nice because the exhaustive match forces handling new blockchain types, there is a single source of truth for per-chain behavior and it's easy to extend with more rules later.

3. Add a helper in the repository for signature validation:

// In repositories/monitor.rs
fn validate_monitor_signatures(monitor, networks, errors) {
    for network_slug in monitor.networks {
        let rules = networks[network_slug].network_type.signature_rules();
        if rules.requires_parentheses {
            // validate function/event signatures have parens
        }
    }
}

and then call it from validate_monitor_references().

What do you think? Happy to discuss if you have questions about this approach!

@shahnami Thanks for the suggestions! Just want to confirm before I push:

My plan:

1. Add SignatureRules struct + signature_rules() method on BlockChainType
2. Remove signature validation from Monitor::validate() (keep only intrinsic checks)
3. Add validate_monitor_signatures() helper in the repository, called from validate_monitor_references()

Does this look right? Any preferences on where SignatureRules should live - alongside BlockChainType in blockchain/mod.rs or somewhere else?

[shahnami]

Does this look right? Any preferences on where SignatureRules should live - alongside BlockChainType in blockchain/mod.rs or somewhere else?

I think that's fine for now, alongside BlockChainType

[shahnami]

Hey @ritik4ever , looks like CI is failing. Can you please ensure you the pre-commit hooks installed?

[shahnami]

@ritik4ever lets get those CI checks to pass. It looks like you need to update the lockfile?

[ritik4ever]

@shahnami Rebased and conflicts resolved. The deploy preview failures are expected for external PRs. Ready for review when you have time!

[shahnami]

@shahnami Rebased and conflicts resolved. The deploy preview failures are expected for external PRs. Ready for review when you have time!

There's something wrong with the diff (~130k LoC).

[ritik4ever]

@shahnami Rebased and conflicts resolved. The deploy preview failures are expected for external PRs. Ready for review when you have time!

There's something wrong with the diff (~130k LoC).

i will fix it.

[shahnami]

@ritik4ever can we work on updating lockfile for CI to pass (also make sure you pull latest main branch)

[ritik4ever]

hii @shahnami Tests pass locally but CI is failing. Could you check if there's an issue with the CI workflow? The test output shows:

• test_validate_solana_network_type_detection ... ok
• test_signature_rules ... ok