fix: validate governance on scheduled update path with regression tests

[Nife-tanny]

Overview

Issue #562 asks that the scheduled _update_governance path validate a new governance address exactly like the direct update_governance path, and that both paths be tested ("Test both", "Show the scheduled-path validation test").

Finding from the actual code: both paths already route through the same validate_governance helper — direct path at settlement_contract/src/admin.rs (update_governance, ~line 90) and scheduled path at _update_governance (~line 451, reached via Operation::UpdateGovernance in execute). This has been true in upstream history since before the July module-split refactor. What was genuinely missing was the issue-required regression test for the scheduled path and any in-code documentation of the shared-check invariant. This PR adds both, so the two paths cannot drift apart again.

What the validation actually checks (confirmed from code, not assumed)

validate_governance (settlement_contract/src/storage.rs) does not perform a cross-contract fee-interface probe. Such a call during init/update_governance was intentionally removed in issue #124 (PR #679) because it creates a reentrancy/DoS vector — a broken or self-recursive governance contract can trap or call back into the not-yet-initialized contract. Governance fee-config validity is instead deferred to first use via try_invoke_contract. The shared check is therefore: reject empty/zero addresses with InvalidGovernance (#309).

I deliberately did not re-add a cross-contract fee-interface probe to either path: that would contradict #124's documented rationale, and the issue asks both paths to share the same validation, which they now demonstrably do.

The scheduled-path validation test (issue requirement)

/// The scheduled path (`Operation::UpdateGovernance` executed through the
/// timelock) must enforce the exact same `validate_governance` check as the
/// direct `update_governance` entry point. `schedule` is admin-gated but does
/// not inspect the operation payload, so a zero governance address must be
/// rejected with `InvalidGovernance` (#309) at execution time instead of
/// being stored.
#[test]
#[should_panic(expected = "Error(Contract, #309)")]
fn scheduled_update_governance_rejects_zero_address() {
    let (env, client, admins, _) = setup();
    let zero_address = Address::from_string(&soroban_sdk::String::from_str(
        &env,
        "GAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAWHF",
    ));
    let operation = Operation::UpdateGovernance(zero_address);

    client.schedule(&admins, &operation, &DEFAULT_TIMELOCK_DELAY_SECONDS);

    env.ledger()
        .with_mut(|ledger| ledger.timestamp += DEFAULT_TIMELOCK_DELAY_SECONDS);

    // The scheduled path must fail the same validation the direct path does.
    client.execute(&admins.get(0).unwrap(), &operation);
}

schedule is admin-gated but never inspects the operation payload, so validation must happen at execution time on the scheduled path. This test fails on any tree where _update_governance omits the check (the zero address would be stored and no panic would occur), and passes once both paths call validate_governance — it exercises the scheduled execution path end-to-end, not the direct one.

Additional tests added:

• scheduled_update_governance_accepts_valid_address — scheduled path still accepts a valid governance address.
• update_governance_rejects_zero_address — direct-path negative control (same InvalidGovernance rejection).

New ledger snapshot files for the three new tests are committed per repo convention (CONTRIBUTING.md).

Summary

• Both governance-update paths call the same validate_governance; no code-path divergence exists. This is now documented directly on update_governance and _update_governance (shared-validation policy, issue update_governance doesn't validate the new governance supports the fee interface #562).
• The change is behavior-preserving for the contract: it adds no new runtime logic, only test coverage and documentation.
• New tests verified locally: cargo test -p settlement_contract update_governance → 6 passed; 0 failed (including both scheduled-path tests and the direct-path negative test), run in an isolated throwaway worktree with only the documented base compile fixes applied.
• cargo test --workspace on the committed tree is still blocked by pre-existing base breakage (below); the error inventory after this change is identical to base — no new errors are introduced by this PR.

Pre-existing failures (base branch 300cd07, intentionally left untouched)

These fail identically on the base branch with a clean tree, before this change:

• settlement_contract (lib): Val: From<Symbol> unsatisfied at storage.rs:326.
• settlement_contract (lib test): stale 4-arg client.init(...) calls in governance_error_tests.rs (missing deployer), missing deployer in admin_tests.rs (×2), client.execute(&op) arity errors in timelock_tests.rs (×10), missing admins in schedule_collision_tests.rs/timelock_tests.rs, and unresolved GovernanceContract/Symbol/events().all() in governance_error_tests.rs.
• governance_contract (lib test): stale 3-arg init calls and vec![env, ...] vs vec![&env, ...] in real_auth_tests.rs, anchor_no_event_error_tests.rs, and lib.rs test code.
• cargo fmt --all --check: fails on pre-existing formatting drift (e.g. bettapay_common/src/storage.rs).
• cargo clippy --workspace --all-targets --all-features -- -D warnings: cannot complete for the same compile errors.

Per the task constraints these were left untouched and are flagged rather than silently fixed.

Local verification status

• cargo test --workspace: does not pass on the base due to the pre-existing failures above (this is not caused by this PR). New tests were verified passing in isolation as described above.
• cargo fmt --all -- --check: pre-existing failure (unchanged by this PR).
• cargo clippy --workspace --all-targets -- -D warnings: pre-existing failure (unchanged by this PR).

Closes #562

[drips-wave]

@Nife-tanny Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits.

You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀

Learn more about application limits