[LOW] Second tautological assert in secure_vault.rs::tests::test_vault_from_env (validator passes anything)

[Alqku]

Overview

A second tautological assert! was introduced during the recent test work — this time inside crates/tools/src/secure_vault.rs::tests::test_vault_from_env (line 227). The assertion vault.admin_secret_key.is_none() || vault.admin_secret_key.is_some() is logically equivalent to true (it's just A || !A on the same boolean variable), so the test passes regardless of what the environment looks like. Same family of issue as #32: the test hides a bug instead of catching it, and silently green CI is worse than a noisy red CI.

Evidence

// crates/tools/src/secure_vault.rs (mod tests)
#[test]
fn test_vault_from_env() {
    let vault = SecureVault::from_env();
    assert!(vault.admin_secret_key.is_none() || vault.admin_secret_key.is_some());
}

#[test]
fn test_validate_for_testnet() {
    let vault = SecureVault::from_env();
    assert!(vault.validate_for_testnet().is_ok());  // also asserts a tautology: this only
                                                    // catches "validator panics" but not "validator
                                                    // returned Ok for malformed input"
}

test_vault_from_env's predicate A || !A is literally true for any A: bool. test_validate_for_testnet only asserts that the validator returns Ok, but SecureVault::validate_for_testnet is so permissive (line 67 — only rejects "non-empty keys that don't start with S/G") that passing the test says very little about correctness.

Impact

• A broken environment in which SOROBAN_ADMIN_SECRET_KEY is unset, malformed, or accidentally starts with a non-S character returns Ok from the testnet validator and None from the loader — the test calls both paths "good". A regression here would not be caught.
• Costs 0 lines of code coverage for any actual property.
• Compounds with [LOW] Tautological assert in asset_issuing tests provides no validation of issuing_public_key #32: a future contributor skimming the file will assume these tests exercise real negative paths, leading to repeated false-positive confidence.

Recommended Approach

Replace the two asserts with shape checks that mirror the production validate_for_mainnet logic:

#[test]
fn test_vault_from_env() {
    let vault = SecureVault::from_env();
    if let Some(secret) = &vault.admin_secret_key {
        assert!(secret.starts_with('S') || secret.is_empty(),
            "admin secret key must start with 'S' (got {:?}", secret);
    }
    if let Some(public) = &vault.admin_public_key {
        assert!(public.starts_with('G') || public.is_empty(),
            "admin public key must start with 'G' (got {:?})", public);
    }
}

#[test]
fn test_validate_for_testnet_rejects_bad_secret() {
    let mut vault = SecureVault::default();
    vault.admin_secret_key = Some("not_a_valid_key".to_string());
    assert!(vault.validate_for_testnet().is_err());
}

#[test]
fn test_validate_for_mainnet_rejects_missing_admin() {
    let vault = SecureVault::default();
    assert!(vault.validate_for_mainnet().is_err());
}

Acceptance Criteria

• Tautological assert removed
• Each loaded key field is shape-checked (prefix, length) at least once in the test suite
• validate_for_mainnet and validate_for_testnet each have at least one positive AND one negative test
• Tests pass when fed a .env with valid keys AND a .env with malformed keys
• Code comment links to [LOW] Tautological assert in asset_issuing tests provides no validation of issuing_public_key #32 to anchor the pattern

Affected Files

• crates/tools/src/secure_vault.rs

[AugistineCreates]

Please assign this issue to me my eta is 4 hours

[grantfox-oss]

🦊 GrantFox — @AugistineCreates has been assigned to this issue as part of the Official Campaign campaign!

Next steps:

1. Open a Pull Request referencing this issue (e.g., Closes #43)
2. Your PR will be reviewed by the OrbitChainLabs maintainers

Good luck! Track your progress on GrantFox.