fix(#467): standardize validation ordering for set_settlement_rule and set_default_rule

Makefile

@@ -22,10 +22,6 @@ clean:
 
 fmt:
 	cargo fmt --all -- --check
-.PHONY: fmt test check clippy all
-
-fmt:
-	cargo fmt --all --check
 
 test:
 	cargo test --workspace
@@ -47,5 +43,4 @@ wasm_size: optimize
 check_codeowners:
 	bash scripts/check_codeowners.sh
 
-all: fmt check clippy test test_scripts wasm_size check_codeowners
-all: fmt check clippy test
+all: fmt check clippy test test_scripts check_codeowners

bettapay_common/src/storage.rs

@@ -169,12 +169,20 @@ mod compatibility_tests {
     fn common_data_key_encoding_matches_legacy() {
         let env = Env::default();
         let mut map: soroban_sdk::Map<Val, u32> = soroban_sdk::Map::new(&env);
-        
+
         map.set(LegacyDataKey::RecoveryAddress.into_val(&env), 1u32);
-        assert_eq!(map.get(CommonDataKey::RecoveryAddress.into_val(&env)), Some(1u32), "RecoveryAddress encoding mismatch");
+        assert_eq!(
+            map.get(CommonDataKey::RecoveryAddress.into_val(&env)),
+            Some(1u32),
+            "RecoveryAddress encoding mismatch"
+        );
 
         map.set(LegacyDataKey::PendingRecovery.into_val(&env), 2u32);
-        assert_eq!(map.get(CommonDataKey::PendingRecovery.into_val(&env)), Some(2u32), "PendingRecovery encoding mismatch");
+        assert_eq!(
+            map.get(CommonDataKey::PendingRecovery.into_val(&env)),
+            Some(2u32),
+            "PendingRecovery encoding mismatch"
+        );
 
         // Note: Paused was also a unit variant in the legacy DataKey.
         // We'll just define another legacy enum for it or reuse the same.
@@ -184,11 +192,19 @@ mod compatibility_tests {
             Paused,
             SystemParam(soroban_sdk::Symbol),
         }
-        
+
         map.set(LegacyDataKey2::Paused.into_val(&env), 3u32);
-        assert_eq!(map.get(CommonDataKey::Paused.into_val(&env)), Some(3u32), "Paused encoding mismatch");
+        assert_eq!(
+            map.get(CommonDataKey::Paused.into_val(&env)),
+            Some(3u32),
+            "Paused encoding mismatch"
+        );
 
         map.set(LegacyDataKey::Threshold.into_val(&env), 4u32);
-        assert_eq!(map.get(CommonDataKey::Threshold.into_val(&env)), Some(4u32), "Threshold encoding mismatch");
+        assert_eq!(
+            map.get(CommonDataKey::Threshold.into_val(&env)),
+            Some(4u32),
+            "Threshold encoding mismatch"
+        );
     }
 }

governance_contract/src/lib.rs

@@ -494,7 +494,9 @@ impl GovernanceContract {
 
         let new_admins = soroban_sdk::vec![&env, pending.new_admin.clone()];
         env.storage().instance().set(&DataKey::Admin, &new_admins);
-        env.storage().instance().set(&CommonDataKey::Threshold, &1u32);
+        env.storage()
+            .instance()
+            .set(&CommonDataKey::Threshold, &1u32);
         env.storage()
             .instance()
             .remove(&CommonDataKey::PendingRecovery);
@@ -701,7 +703,9 @@ impl GovernanceContract {
         let key = DataKey::Anchor(asset.clone());
         let old_anchor: Option<Address> = env.storage().persistent().get(&key);
         env.storage().persistent().set(&key, &anchor.clone());
-        env.storage().persistent().extend_ttl(&key, ANCHOR_TTL_THRESHOLD, ANCHOR_TTL_BUMP);
+        env.storage()
+            .persistent()
+            .extend_ttl(&key, ANCHOR_TTL_THRESHOLD, ANCHOR_TTL_BUMP);
         env.events().publish(
             (Symbol::new(&env, events::ANCHOR_UPSERTED_EVENT), asset),
             (old_anchor, anchor),
@@ -902,8 +906,8 @@ mod real_auth_tests;
 mod tests {
     use super::*;
     use proptest::prelude::*;
-    use soroban_sdk::testutils::{Address as _, Events};
     use soroban_sdk::testutils::storage::Persistent;
+    use soroban_sdk::testutils::{Address as _, Events};
     use soroban_sdk::{vec, Bytes, FromVal, String};
 
     fn setup() -> (
@@ -945,7 +949,10 @@ mod tests {
         let bad_hash = upload_test_wasm(&env); // empty wasm — no supports_interface
 
         let result = client.try_upgrade(&admins, &bad_hash);
-        assert!(result.is_err(), "upgrade with non-conforming wasm must be rejected");
+        assert!(
+            result.is_err(),
+            "upgrade with non-conforming wasm must be rejected"
+        );
 
         // Contract is intact after the failed upgrade.
         let live_client = GovernanceContractClient::new(&env, &client.address);
@@ -1103,7 +1110,7 @@ mod tests {
     #[should_panic(expected = "Error(Contract, #4)")]
     fn set_fee_config_rejects_fees_exceeding_ceiling() {
         let (_env, client, admins, _recovery) = setup();
-        
+
         // Sum exceeds BPS_DENOMINATOR
         let cfg = FeeConfig {
             platform_fee_bps: 5_000,
@@ -1117,7 +1124,7 @@ mod tests {
     #[should_panic(expected = "Error(Contract, #4)")]
     fn set_fee_config_rejects_individual_fee_exceeding_max() {
         let (_env, client, admins, _recovery) = setup();
-        
+
         // Individual fee exceeds MAX_FEE_BPS (governance trust root)
         let cfg = FeeConfig {
             platform_fee_bps: 5_001,

governance_contract/src/real_auth_tests.rs

@@ -17,7 +17,9 @@ fn fee_anchor_and_system_param_writes_require_real_authorization() {
     let key = Symbol::new(&env, "real_auth");
     env.mock_auths(&[]);
 
-    assert!(client.try_set_fee_config(&admins, &valid_fee_config()).is_err());
+    assert!(client
+        .try_set_fee_config(&admins, &valid_fee_config())
+        .is_err());
     assert!(client.try_upsert_anchor(&admins, &asset, &anchor).is_err());
     assert!(client.try_update_system_param(&admins, &key, &1).is_err());
 }
@@ -40,11 +42,7 @@ fn admin_transfer_and_threshold_change_require_real_authorization() {
     env.mock_auths(&[]);
 
     assert!(client
-        .try_transfer_admin(
-            &admins,
-            &soroban_sdk::vec![&env, replacement_admin],
-            &1,
-        )
+        .try_transfer_admin(&admins, &soroban_sdk::vec![&env, replacement_admin], &1,)
         .is_err());
 
     let env = Env::default();

settlement_contract/src/admin.rs

@@ -10,6 +10,9 @@ use bettapay_common::{
 use crate::errors::SettlementError;
 use crate::storage::{
     assert_not_paused, is_merchant_registered_and_bump_ttl, read_admin, read_admins,
+    read_governance, read_pending_recovery, read_recovery_address, read_rule_or_default,
+    read_threshold, validate_admins_and_threshold, validate_fee_against_governance,
+    validate_governance, validate_nonzero_address, verify_admin_auth, write_admins,
     read_fallback_rule, read_governance, read_optional_primary_admin, read_pending_recovery,
     read_recovery_address, read_rule_or_default, read_threshold,
     validate_admins_and_threshold, validate_governance, validate_nonzero_address,
@@ -448,7 +451,7 @@ impl SettlementContract {
             SettlementError::ZeroAddress,
         );
         let admin = read_admin(env);
-        
+
         // Prevent an admin from being registered as a merchant
         let admins = read_admins(env);
         for i in 0..admins.len() {
@@ -527,6 +530,11 @@ impl SettlementContract {
         assert_not_paused(env);
         let admin = read_admin(env);
 
+        // Standard validation order (shared with the direct path in settlement.rs):
+        // 1. Merchant existence
+        // 2. Fee range (hardcoded protocol bounds)
+        // 3. Governance ceiling
+        // 4. Settlement delay
         if !is_merchant_registered_and_bump_ttl(env, merchant.clone()) {
             panic_with_error!(env, SettlementError::MerchantMissing);
         }
@@ -542,6 +550,7 @@ impl SettlementContract {
         if rule.platform_fee_bps + rule.network_fee_bps > BPS_DENOMINATOR {
             panic_with_error!(env, SettlementError::InvalidFeeBps);
         }
+        validate_fee_against_governance(env, &rule);
         if rule.settlement_delay_ledger > MAX_SETTLEMENT_DELAY_LEDGER {
             panic_with_error!(env, SettlementError::InvalidSettlementDelay);
         }
@@ -595,6 +604,10 @@ impl SettlementContract {
         assert_not_paused(env);
         let admin = read_admin(env);
 
+        // Standard validation order (shared with the direct path in settlement.rs):
+        // 1. Fee range (hardcoded protocol bounds)
+        // 2. Governance ceiling
+        // 3. Settlement delay
         if new_rule.platform_fee_bps > BPS_DENOMINATOR || new_rule.network_fee_bps > BPS_DENOMINATOR
         {
             panic_with_error!(env, SettlementError::InvalidFeeBps);
@@ -605,6 +618,7 @@ impl SettlementContract {
         if new_rule.platform_fee_bps > MAX_FEE_BPS || new_rule.network_fee_bps > MAX_FEE_BPS {
             panic_with_error!(env, SettlementError::InvalidFeeBps);
         }
+        validate_fee_against_governance(env, &new_rule);
         if new_rule.settlement_delay_ledger > MAX_SETTLEMENT_DELAY_LEDGER {
             panic_with_error!(env, SettlementError::InvalidSettlementDelay);
         }

settlement_contract/src/lib.rs

@@ -33,17 +33,17 @@
 //!
 //! ## Settlement Boundary (Off-Chain Execution)
 //!
-//! This contract calculates and securely locks the fee split for each payment in a `PaymentRecord` and emits a 
+//! This contract calculates and securely locks the fee split for each payment in a `PaymentRecord` and emits a
 //! `payment_stored` event. It does **not** transfer tokens, hold funds, or expose an in-contract `settle` function.
 //!
 //! Settlement execution is intentionally designed to be **off-chain**:
 //! 1. **Indexers** listen to `payment_stored` events and read the `PaymentRecord` state.
-//! 2. **Readiness** is verified off-chain by evaluating if the current ledger sequence satisfies the delay: 
+//! 2. **Readiness** is verified off-chain by evaluating if the current ledger sequence satisfies the delay:
 //!    `current_ledger >= record.ledger + record.settlement_delay_ledger`.
-//! 3. **Execution** happens via a separate off-chain payout engine that processes transfers (batching where 
+//! 3. **Execution** happens via a separate off-chain payout engine that processes transfers (batching where
 //!    appropriate based on `auto_settle` preferences) and tracks settlement state externally.
 //!
-//! The in-contract flags (`settlement_delay_ledger`, `auto_settle`) are strictly informational directives 
+//! The in-contract flags (`settlement_delay_ledger`, `auto_settle`) are strictly informational directives
 //! enforcing standardized agreement parameters for off-chain consumers; they do not trigger on-chain state transitions.
 //!
 //! ## Event Conventions
@@ -184,7 +184,9 @@ use bettapay_common::constants::MIN_FEE_BPS;
 use soroban_sdk::contract;
 
 pub use errors::SettlementError;
-pub use types::{Bps, FeeSplit, GovFeeConfig, Operation, PaymentRecord, ScheduledOp, SettlementRule};
+pub use types::{
+    Bps, FeeSplit, GovFeeConfig, Operation, PaymentRecord, ScheduledOp, SettlementRule,
+};
 
 /// Minimum gross payment amount, in the asset's smallest unit.
 ///

settlement_contract/src/payments.rs

@@ -221,9 +221,9 @@ impl SettlementContract {
         }
 
         // ISSUE 495: Reentrancy guard.
-        // We write a dummy record to storage immediately so that if the external 
-        // read_governance_fee_rule call results in a reentrant call back to this 
-        // contract, the `has` check above will catch it. This dummy record is 
+        // We write a dummy record to storage immediately so that if the external
+        // read_governance_fee_rule call results in a reentrant call back to this
+        // contract, the `has` check above will catch it. This dummy record is
         // overwritten by the actual record at the end of this function.
         let dummy_record = PaymentRecord {
             merchant: merchant.clone(),

settlement_contract/src/settlement.rs

@@ -28,8 +28,11 @@ impl SettlementContract {
         verify_admin_auth(&env, &signers, read_threshold(&env));
         let admin = signers.get(0).unwrap();
 
-        validate_fee_against_governance(&env, &rule);
-
+        // Standard validation order (shared with the scheduled path in admin.rs):
+        // 1. Merchant existence
+        // 2. Fee range (hardcoded protocol bounds)
+        // 3. Governance ceiling
+        // 4. Settlement delay
         if !is_merchant_registered_and_bump_ttl(&env, merchant.clone()) {
             panic_with_error!(&env, SettlementError::MerchantMissing);
         }
@@ -45,6 +48,7 @@ impl SettlementContract {
         if rule.platform_fee_bps + rule.network_fee_bps > BPS_DENOMINATOR {
             panic_with_error!(&env, SettlementError::InvalidFeeBps);
         }
+        validate_fee_against_governance(&env, &rule);
         if rule.settlement_delay_ledger > MAX_SETTLEMENT_DELAY_LEDGER {
             panic_with_error!(&env, SettlementError::InvalidSettlementDelay);
         }
@@ -99,8 +103,10 @@ impl SettlementContract {
         verify_admin_auth(&env, &signers, read_threshold(&env));
         let admin = signers.get(0).unwrap();
 
-        validate_fee_against_governance(&env, &new_rule);
-
+        // Standard validation order:
+        // 1. Fee range (hardcoded protocol bounds)
+        // 2. Governance ceiling
+        // 3. Settlement delay
         if new_rule.platform_fee_bps > BPS_DENOMINATOR || new_rule.network_fee_bps > BPS_DENOMINATOR
         {
             panic_with_error!(&env, SettlementError::InvalidFeeBps);
@@ -111,6 +117,7 @@ impl SettlementContract {
         if new_rule.platform_fee_bps > MAX_FEE_BPS || new_rule.network_fee_bps > MAX_FEE_BPS {
             panic_with_error!(&env, SettlementError::InvalidFeeBps);
         }
+        validate_fee_against_governance(&env, &new_rule);
         if new_rule.settlement_delay_ledger > MAX_SETTLEMENT_DELAY_LEDGER {
             panic_with_error!(&env, SettlementError::InvalidSettlementDelay);
         }