From 48c139802b29901f13470971abc04b2939b6dd15 Mon Sep 17 00:00:00 2001 From: Hikmah Oladele Date: Fri, 28 Aug 2026 10:47:28 +0000 Subject: [PATCH 1/3] fix: map governance failures to typed error (#482) --- settlement_contract/src/storage.rs | 15 +++++ .../src/tests/governance_error_tests.rs | 63 +++++++++++++++++++ 2 files changed, 78 insertions(+) diff --git a/settlement_contract/src/storage.rs b/settlement_contract/src/storage.rs index 468d7473..1d4600d9 100644 --- a/settlement_contract/src/storage.rs +++ b/settlement_contract/src/storage.rs @@ -1,3 +1,18 @@ +//! Settlement contract storage and governance cross-contract helpers. +//! +//! Governance is reached via two cross-contract paths. Both must fail safely +//! with the typed [`SettlementError::GovernanceCallFailed`] (code 311) instead +//! of an untyped host panic: +//! +//! - **Read path** — [`read_governance_fee_rule`], reached from +//! `calculate_fee_split` when no merchant or default rule is set. A governance +//! trap, host error, or unexpected return value surfaces as +//! `GovernanceCallFailed`; a `None` config falls through to the bootstrap default. +//! - **Write path** — [`validate_fee_against_governance`], reached from +//! `set_settlement_rule` / `set_default_rule`. Any governance call failure +//! (contract trap, host error, or contract-returned error) surfaces as +//! `GovernanceCallFailed`, so a broken or mis-deployed governance contract +//! cannot abort the transaction with a raw panic. use soroban_sdk::{panic_with_error, Address, Env, Symbol, Val, Vec}; use bettapay_common::{ diff --git a/settlement_contract/src/tests/governance_error_tests.rs b/settlement_contract/src/tests/governance_error_tests.rs index 947236a2..93755540 100644 --- a/settlement_contract/src/tests/governance_error_tests.rs +++ b/settlement_contract/src/tests/governance_error_tests.rs @@ -39,6 +39,30 @@ mod panicking_gov { use panicking_gov::PanickingGovernance; +// A second failing-governance stub that returns a *typed error* rather than +// trapping. This exercises the `Ok(Err(_))` branch of `try_invoke_contract` +// (a contract that deliberately rejects the read), as opposed to the trap +// branch exercised by `PanickingGovernance`. +mod erroring_gov { + use soroban_sdk::{contract, contractimpl, Env}; + use crate::GovFeeConfig; + use crate::errors::SettlementError; + + /// A governance stub whose `get_fee_config` returns a typed error. + #[contract] + pub struct ErroringGovernance; + + #[contractimpl] + impl ErroringGovernance { + #[allow(unused_variables)] + pub fn get_fee_config(env: Env) -> Result, SettlementError> { + Err(SettlementError::GovernanceCallFailed) + } + } +} + +use erroring_gov::ErroringGovernance; + /// Helper: directly injects a governance address into the settlement contract's /// instance storage, bypassing `validate_governance` (which would itself call /// `get_fee_config` and fail against the panicking stub). @@ -182,3 +206,42 @@ fn write_path_set_default_rule_governance_failure_surfaces_typed_error() { client.set_default_rule(&soroban_sdk::vec![&env, admin], &rule); } + +/// Focused variant of the write-path test: governance returns a typed error +/// (not a trap). This drives `validate_fee_against_governance` through the +/// `Ok(Err(_))` branch of `try_invoke_contract`. +/// +/// Expected: the typed `GovernanceCallFailed` error (code 311) — a deliberate +/// governance rejection must not surface as an untyped host panic. +#[test] +#[should_panic(expected = "Error(Contract, #311)")] +fn write_path_governance_error_surfaces_typed_error() { + let env = Env::default(); + env.mock_all_auths(); + + let erroring_gov = env.register_contract(None, ErroringGovernance); + let empty_gov = super::register_governance(&env); + + let admin = Address::generate(&env); + let recovery = Address::generate(&env); + let merchant = Address::generate(&env); + + let contract_id = env.register_contract(None, SettlementContract); + let client = SettlementContractClient::new(&env, &contract_id); + client.init(&soroban_sdk::vec![&env, admin.clone()], &1, &empty_gov, &recovery); + client.register_merchant(&soroban_sdk::vec![&env, admin.clone()], &merchant); + + // Directly inject the error-returning governance address. + inject_governance(&env, &contract_id, &erroring_gov); + + let rule = SettlementRule { + platform_fee_bps: 100, + network_fee_bps: 50, + settlement_delay_ledger: 0, + auto_settle: false, + }; + + // set_settlement_rule calls validate_fee_against_governance, which must + // surface GovernanceCallFailed instead of an untyped host panic. + client.set_settlement_rule(&soroban_sdk::vec![&env, admin], &merchant, &rule); +} From 55a17ec4ed6ed33ec7feb353c4f6f560acefcca3 Mon Sep 17 00:00:00 2001 From: Hikmah Oladele <178912792+Hikmaholadele@users.noreply.github.com> Date: Fri, 28 Aug 2026 13:57:48 +0000 Subject: [PATCH 2/3] fix(tests): replace assert_eq! with assert! for bool comparison in admin_tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replace assert_eq!(client.is_paused(), true, ...) with assert!(client.is_paused(), ...) to satisfy clippy::bool_assert_comparison. Run cargo fmt --all for workspace formatting. 🤖 Generated with Codebuff Co-Authored-By: Codebuff --- bettapay_common/src/storage.rs | 28 ++++++-- governance_contract/src/lib.rs | 19 +++-- governance_contract/src/real_auth_tests.rs | 10 ++- settlement_contract/src/admin.rs | 5 +- settlement_contract/src/lib.rs | 12 ++-- settlement_contract/src/payments.rs | 6 +- settlement_contract/src/tests/admin_tests.rs | 12 +++- .../src/tests/event_topic_conformity_tests.rs | 6 +- .../src/tests/governance_error_tests.rs | 41 ++++++++--- .../src/tests/integration_tests.rs | 70 ++++++++++++++----- .../src/tests/timelock_tests.rs | 46 ++++++++---- 11 files changed, 183 insertions(+), 72 deletions(-) diff --git a/bettapay_common/src/storage.rs b/bettapay_common/src/storage.rs index 7b165824..bc9f48c8 100644 --- a/bettapay_common/src/storage.rs +++ b/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 = 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" + ); } } diff --git a/governance_contract/src/lib.rs b/governance_contract/src/lib.rs index 82f89b76..4f44a0be 100644 --- a/governance_contract/src/lib.rs +++ b/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
= 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, diff --git a/governance_contract/src/real_auth_tests.rs b/governance_contract/src/real_auth_tests.rs index ceefb899..828c8128 100644 --- a/governance_contract/src/real_auth_tests.rs +++ b/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(); diff --git a/settlement_contract/src/admin.rs b/settlement_contract/src/admin.rs index d71dbf25..95f051a3 100644 --- a/settlement_contract/src/admin.rs +++ b/settlement_contract/src/admin.rs @@ -11,8 +11,7 @@ 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_governance, validate_nonzero_address, + read_threshold, validate_admins_and_threshold, validate_governance, validate_nonzero_address, verify_admin_auth, write_admins, }; use crate::types::{DataKey, Operation, ScheduledOp, SettlementRule}; @@ -448,7 +447,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() { diff --git a/settlement_contract/src/lib.rs b/settlement_contract/src/lib.rs index 7a47af68..bcf8d60a 100644 --- a/settlement_contract/src/lib.rs +++ b/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. /// diff --git a/settlement_contract/src/payments.rs b/settlement_contract/src/payments.rs index 4af7a08d..0b1d1cdd 100644 --- a/settlement_contract/src/payments.rs +++ b/settlement_contract/src/payments.rs @@ -220,9 +220,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(), diff --git a/settlement_contract/src/tests/admin_tests.rs b/settlement_contract/src/tests/admin_tests.rs index 2460cbd9..4bd0e4bf 100644 --- a/settlement_contract/src/tests/admin_tests.rs +++ b/settlement_contract/src/tests/admin_tests.rs @@ -307,10 +307,13 @@ fn register_merchant_rejects_admin_address() { #[should_panic(expected = "Error(Contract, #5)")] fn set_default_rule_rejected_when_paused() { let (_env, client, admins, _merchant) = setup(); - + // Pause the contract to simulate an emergency state client.pause(&admins); - assert_eq!(client.is_paused(), true, "Contract must be paused before testing rejection"); + assert!( + client.is_paused(), + "Contract must be paused before testing rejection" + ); // Attempt to set a valid default rule; this should be rejected due to the pause state let rule = SettlementRule { @@ -416,7 +419,10 @@ fn executes_contract_wasm_upgrade_successfully() { // Empty wasm has no `supports_interface` — upgrade must fail. 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 remains operational after the rejected upgrade. let live_client = SettlementContractClient::new(&env, &client.address); diff --git a/settlement_contract/src/tests/event_topic_conformity_tests.rs b/settlement_contract/src/tests/event_topic_conformity_tests.rs index ee5af54c..cab82a8e 100644 --- a/settlement_contract/src/tests/event_topic_conformity_tests.rs +++ b/settlement_contract/src/tests/event_topic_conformity_tests.rs @@ -150,7 +150,11 @@ fn upgrade_uses_canonical_topic() { let result = client.try_upgrade(&admins, &bad_hash); assert!(result.is_err(), "non-conforming wasm must be rejected"); // No event emitted on failure. - assert_eq!(env.events().all().len(), before, "no event on failed upgrade"); + assert_eq!( + env.events().all().len(), + before, + "no event on failed upgrade" + ); } #[test] diff --git a/settlement_contract/src/tests/governance_error_tests.rs b/settlement_contract/src/tests/governance_error_tests.rs index 93755540..29aee357 100644 --- a/settlement_contract/src/tests/governance_error_tests.rs +++ b/settlement_contract/src/tests/governance_error_tests.rs @@ -20,8 +20,8 @@ use soroban_sdk::{Address, Env}; // --------------------------------------------------------------------------- mod panicking_gov { - use soroban_sdk::{contract, contractimpl, Env}; use crate::GovFeeConfig; + use soroban_sdk::{contract, contractimpl, Env}; /// A governance stub whose `get_fee_config` always traps (simulates a /// broken or mis-deployed governance contract). @@ -44,9 +44,9 @@ use panicking_gov::PanickingGovernance; // (a contract that deliberately rejects the read), as opposed to the trap // branch exercised by `PanickingGovernance`. mod erroring_gov { - use soroban_sdk::{contract, contractimpl, Env}; - use crate::GovFeeConfig; use crate::errors::SettlementError; + use crate::GovFeeConfig; + use soroban_sdk::{contract, contractimpl, Env}; /// A governance stub whose `get_fee_config` returns a typed error. #[contract] @@ -99,7 +99,12 @@ fn read_path_governance_failure_surfaces_typed_error() { let contract_id = env.register_contract(None, SettlementContract); let client = SettlementContractClient::new(&env, &contract_id); - client.init(&soroban_sdk::vec![&env, admin.clone()], &1, &empty_gov, &recovery); + client.init( + &soroban_sdk::vec![&env, admin.clone()], + &1, + &empty_gov, + &recovery, + ); client.register_merchant(&soroban_sdk::vec![&env, admin.clone()], &merchant); @@ -125,7 +130,12 @@ fn read_path_governance_none_falls_through_to_bootstrap() { let contract_id = env.register_contract(None, SettlementContract); let client = SettlementContractClient::new(&env, &contract_id); - client.init(&soroban_sdk::vec![&env, admin.clone()], &1, &empty_gov, &recovery); + client.init( + &soroban_sdk::vec![&env, admin.clone()], + &1, + &empty_gov, + &recovery, + ); client.register_merchant(&soroban_sdk::vec![&env, admin], &merchant); // Empty governance returns None — bootstrap default should apply (100 bps platform, 5 network). @@ -158,7 +168,12 @@ fn write_path_governance_failure_surfaces_typed_error() { let contract_id = env.register_contract(None, SettlementContract); let client = SettlementContractClient::new(&env, &contract_id); - client.init(&soroban_sdk::vec![&env, admin.clone()], &1, &empty_gov, &recovery); + client.init( + &soroban_sdk::vec![&env, admin.clone()], + &1, + &empty_gov, + &recovery, + ); client.register_merchant(&soroban_sdk::vec![&env, admin.clone()], &merchant); // Directly inject the panicking governance address. @@ -192,7 +207,12 @@ fn write_path_set_default_rule_governance_failure_surfaces_typed_error() { let contract_id = env.register_contract(None, SettlementContract); let client = SettlementContractClient::new(&env, &contract_id); - client.init(&soroban_sdk::vec![&env, admin.clone()], &1, &empty_gov, &recovery); + client.init( + &soroban_sdk::vec![&env, admin.clone()], + &1, + &empty_gov, + &recovery, + ); // Directly inject the panicking governance address. inject_governance(&env, &contract_id, &panicking_gov); @@ -228,7 +248,12 @@ fn write_path_governance_error_surfaces_typed_error() { let contract_id = env.register_contract(None, SettlementContract); let client = SettlementContractClient::new(&env, &contract_id); - client.init(&soroban_sdk::vec![&env, admin.clone()], &1, &empty_gov, &recovery); + client.init( + &soroban_sdk::vec![&env, admin.clone()], + &1, + &empty_gov, + &recovery, + ); client.register_merchant(&soroban_sdk::vec![&env, admin.clone()], &merchant); // Directly inject the error-returning governance address. diff --git a/settlement_contract/src/tests/integration_tests.rs b/settlement_contract/src/tests/integration_tests.rs index e22eaed0..e85962ae 100644 --- a/settlement_contract/src/tests/integration_tests.rs +++ b/settlement_contract/src/tests/integration_tests.rs @@ -999,21 +999,44 @@ pub struct ReentrantGovernanceMock; impl ReentrantGovernanceMock { pub fn get_fee_config(env: Env) -> Option { // Attempt reentrancy if attack is armed - if let Some(target_settle) = env.storage().instance().get::<_, Address>(&Symbol::new(&env, "target_settle")) { + if let Some(target_settle) = env + .storage() + .instance() + .get::<_, Address>(&Symbol::new(&env, "target_settle")) + { let settle_client = SettlementContractClient::new(&env, &target_settle); - let merchant: Address = env.storage().instance().get(&Symbol::new(&env, "target_merchant")).unwrap(); - let reference: BytesN<32> = env.storage().instance().get(&Symbol::new(&env, "target_ref")).unwrap(); - + let merchant: Address = env + .storage() + .instance() + .get(&Symbol::new(&env, "target_merchant")) + .unwrap(); + let reference: BytesN<32> = env + .storage() + .instance() + .get(&Symbol::new(&env, "target_ref")) + .unwrap(); + // This should fail with DuplicatePaymentReference because the dummy record locks it let _ = settle_client.try_store_payment_reference(&merchant, &reference, &1000); } None } - - pub fn setup_attack(env: Env, target_settle: Address, target_merchant: Address, target_ref: BytesN<32>) { - env.storage().instance().set(&Symbol::new(&env, "target_settle"), &target_settle); - env.storage().instance().set(&Symbol::new(&env, "target_merchant"), &target_merchant); - env.storage().instance().set(&Symbol::new(&env, "target_ref"), &target_ref); + + pub fn setup_attack( + env: Env, + target_settle: Address, + target_merchant: Address, + target_ref: BytesN<32>, + ) { + env.storage() + .instance() + .set(&Symbol::new(&env, "target_settle"), &target_settle); + env.storage() + .instance() + .set(&Symbol::new(&env, "target_merchant"), &target_merchant); + env.storage() + .instance() + .set(&Symbol::new(&env, "target_ref"), &target_ref); } } @@ -1025,10 +1048,10 @@ fn store_payment_reference_prevents_reentrancy() { let settle_admin = Address::generate(&env); let settle_recovery = Address::generate(&env); let settle_admins = soroban_sdk::vec![&env, settle_admin.clone()]; - + let mock_gov_id = env.register_contract(None, ReentrantGovernanceMock); let settle_id = env.register_contract(None, SettlementContract); - + let settle_client = SettlementContractClient::new(&env, &settle_id); settle_client.init(&settle_admins, &1, &mock_gov_id, &settle_recovery); @@ -1041,7 +1064,12 @@ fn store_payment_reference_prevents_reentrancy() { env.invoke_contract::<()>( &mock_gov_id, &Symbol::new(&env, "setup_attack"), - soroban_sdk::vec![&env, settle_id.into_val(&env), merchant.into_val(&env), reference.into_val(&env)], + soroban_sdk::vec![ + &env, + settle_id.into_val(&env), + merchant.into_val(&env), + reference.into_val(&env) + ], ); // This call triggers read_rule_or_default -> read_governance_fee_rule -> get_fee_config on our mock @@ -1059,7 +1087,10 @@ fn store_payment_reference_prevents_reentrancy() { } } } - assert_eq!(store_count, 1, "payment_stored should be emitted exactly once"); + assert_eq!( + store_count, 1, + "payment_stored should be emitted exactly once" + ); } // --------------------------------------------------------------------------- @@ -1122,7 +1153,7 @@ fn off_chain_settlement_readiness_logic() { .unwrap(); assert_eq!(record.ledger, 1000); assert_eq!(record.settlement_delay_ledger, 10); - + // Demonstrate off-chain readiness check let is_ready = |current_ledger: u32, r: &PaymentRecord| -> bool { current_ledger >= r.ledger + r.settlement_delay_ledger @@ -1162,8 +1193,15 @@ fn set_settlement_rule_emits_fallback_and_updated_events() { last_event_sym = sym; } else if sym == Symbol::new(&env, bettapay_common::events::SETTLEMENT_RULE_UPDATED_EVENT) { update_found = true; - assert!(fallback_found, "bootstrap_fallback must precede settlement_rule_updated"); - assert_eq!(last_event_sym, Symbol::new(&env, bettapay_common::events::BOOTSTRAP_FALLBACK_EVENT), "events must be sequential"); + assert!( + fallback_found, + "bootstrap_fallback must precede settlement_rule_updated" + ); + assert_eq!( + last_event_sym, + Symbol::new(&env, bettapay_common::events::BOOTSTRAP_FALLBACK_EVENT), + "events must be sequential" + ); last_event_sym = sym; } } diff --git a/settlement_contract/src/tests/timelock_tests.rs b/settlement_contract/src/tests/timelock_tests.rs index b5aa6dfd..172e346b 100644 --- a/settlement_contract/src/tests/timelock_tests.rs +++ b/settlement_contract/src/tests/timelock_tests.rs @@ -32,16 +32,16 @@ fn schedule_rejects_non_admin_and_insufficient_delay() { let operation = Operation::RegisterMerchant(merchant); let non_admin = Address::generate(&env); - assert!(client - .try_schedule(&soroban_sdk::vec![&env, non_admin], &operation, &DEFAULT_TIMELOCK_DELAY_SECONDS) - .is_err()); assert!(client .try_schedule( - &admins, + &soroban_sdk::vec![&env, non_admin], &operation, - &(DEFAULT_TIMELOCK_DELAY_SECONDS - 1), + &DEFAULT_TIMELOCK_DELAY_SECONDS ) .is_err()); + assert!(client + .try_schedule(&admins, &operation, &(DEFAULT_TIMELOCK_DELAY_SECONDS - 1),) + .is_err()); } #[test] @@ -60,7 +60,10 @@ fn admin_can_cancel_but_non_admin_cannot() { let operation = Operation::RegisterMerchant(merchant); client.schedule(&admins, &operation, &DEFAULT_TIMELOCK_DELAY_SECONDS); assert!(client - .try_cancel(&soroban_sdk::vec![&env, Address::generate(&env)], &operation) + .try_cancel( + &soroban_sdk::vec![&env, Address::generate(&env)], + &operation + ) .is_err()); client.cancel(&admins, &operation); @@ -113,11 +116,7 @@ fn expired_schedule_cannot_execute() { let (env, client, admins, merchant) = setup(); let operation = Operation::RegisterMerchant(merchant); - client.schedule( - &admins, - &operation, - &DEFAULT_TIMELOCK_DELAY_SECONDS, - ); + client.schedule(&admins, &operation, &DEFAULT_TIMELOCK_DELAY_SECONDS); // `schedule` bumps the persistent entry to 30 days (518,400 ledgers). // Keep the contract instance alive while advancing past only the @@ -138,7 +137,12 @@ fn expired_schedule_cannot_execute() { client.execute(&operation); } -fn setup_multisig() -> (Env, SettlementContractClient<'static>, soroban_sdk::Vec
, Address) { +fn setup_multisig() -> ( + Env, + SettlementContractClient<'static>, + soroban_sdk::Vec
, + Address, +) { let env = Env::default(); env.mock_all_auths(); let a1 = Address::generate(&env); @@ -191,8 +195,16 @@ fn timelocked_transfer_admin_parity_with_direct_path() { // --- Direct path --- client.transfer_admin(&initial_admins, &new_admins, &new_threshold); - assert_eq!(client.get_admin(), new_admins, "direct path stores full admin set"); - assert_eq!(client.get_threshold(), new_threshold, "direct path stores threshold"); + assert_eq!( + client.get_admin(), + new_admins, + "direct path stores full admin set" + ); + assert_eq!( + client.get_threshold(), + new_threshold, + "direct path stores threshold" + ); // Reset back to single-admin so the timelock path starts from a clean state. let reset_admins = soroban_sdk::vec![&env, a1.clone()]; @@ -200,7 +212,11 @@ fn timelocked_transfer_admin_parity_with_direct_path() { // --- Timelocked path --- let operation = Operation::TransferAdmin(new_admins.clone(), new_threshold); - client.schedule(&soroban_sdk::vec![&env, a1.clone()], &operation, &DEFAULT_TIMELOCK_DELAY_SECONDS); + client.schedule( + &soroban_sdk::vec![&env, a1.clone()], + &operation, + &DEFAULT_TIMELOCK_DELAY_SECONDS, + ); env.ledger() .with_mut(|ledger| ledger.timestamp += DEFAULT_TIMELOCK_DELAY_SECONDS); From cc61ee95dffa8efcefb8b591a22163bc18736105 Mon Sep 17 00:00:00 2001 From: Hikmah Oladele <178912792+Hikmaholadele@users.noreply.github.com> Date: Mon, 31 Aug 2026 09:30:14 +0000 Subject: [PATCH 3/3] fix: resolve compilation, formatting, and clippy errors MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fix compilation errors from API changes merged from main: - Add missing deployer parameter to init() calls in settlement and governance tests - Fix _env/env variable naming mismatch in governance tests - Fix soroban_sdk::vec! macro syntax in governance tests - Add missing executor parameter to execute() calls in timelock tests - Import GovernanceContract/GovernanceContractClient from governance_contract - Add missing Symbol/Events imports for test assertions - Fix governance FeeConfig type reference - Remove unused imports and prefix unused variables with underscore - Apply cargo fmt formatting across workspace 🤖 Generated with Codebuff Co-Authored-By: Codebuff --- .../src/anchor_no_event_error_tests.rs | 3 +- governance_contract/src/lib.rs | 48 ++++++----- governance_contract/src/real_auth_tests.rs | 6 +- settlement_contract/src/admin.rs | 33 ++++++-- settlement_contract/src/merchant.rs | 3 +- settlement_contract/src/payments.rs | 4 +- settlement_contract/src/settlement.rs | 4 +- settlement_contract/src/storage.rs | 6 +- settlement_contract/src/tests/admin_tests.rs | 2 + .../src/tests/governance_error_tests.rs | 82 +++++++++++++++---- .../src/tests/integration_tests.rs | 8 +- .../src/tests/recovery_admin_set_tests.rs | 8 +- .../src/tests/schedule_collision_tests.rs | 2 +- .../src/tests/timelock_tests.rs | 60 ++++++++------ 14 files changed, 181 insertions(+), 88 deletions(-) diff --git a/governance_contract/src/anchor_no_event_error_tests.rs b/governance_contract/src/anchor_no_event_error_tests.rs index 16a847e2..fd866864 100644 --- a/governance_contract/src/anchor_no_event_error_tests.rs +++ b/governance_contract/src/anchor_no_event_error_tests.rs @@ -224,7 +224,8 @@ fn change_threshold_emits_no_event_when_insufficient_signatures() { let recovery = Address::generate(&env); let contract_id = env.register_contract(None, GovernanceContract); let client = GovernanceContractClient::new(&env, &contract_id); - client.init(&admins, &2, &recovery); + let deployer = Address::generate(&env); + client.init(&deployer, &admins, &2, &recovery); let single_signer = vec![&env, a1.clone()]; let prev = env.events().all().len(); diff --git a/governance_contract/src/lib.rs b/governance_contract/src/lib.rs index 782d6c82..e732b959 100644 --- a/governance_contract/src/lib.rs +++ b/governance_contract/src/lib.rs @@ -339,7 +339,13 @@ impl GovernanceContract { /// # Errors /// /// Panics with `GovernanceError::AlreadyInitialized` if already initialised. - pub fn init(env: Env, deployer: Address, admins: Vec
, threshold: u32, recovery_address: Address) { + pub fn init( + env: Env, + deployer: Address, + admins: Vec
, + threshold: u32, + recovery_address: Address, + ) { if env.storage().instance().has(&DataKey::Admin) { panic_with_error!(&env, GovernanceError::AlreadyInitialized); } @@ -392,11 +398,7 @@ impl GovernanceContract { pub fn update_recovery_address(env: Env, signers: Vec
, new_recovery: Address) { verify_admin_auth(&env, &signers, read_threshold(&env)); let admin = signers.get(0).unwrap(); - assert_not_zero( - &env, - &new_recovery, - GovernanceError::InvalidRecoveryAddress, - ); + assert_not_zero(&env, &new_recovery, GovernanceError::InvalidRecoveryAddress); env.storage() .instance() .set(&CommonDataKey::RecoveryAddress, &new_recovery); @@ -931,7 +933,7 @@ mod tests { let recovery_address = Address::generate(&env); let contract_id = env.register_contract(None, GovernanceContract); let client = GovernanceContractClient::new(&env, &contract_id); - let deployer = Address::generate(&env); + let deployer = Address::generate(&env); client.init(&deployer, &admins, &2, &recovery_address); (env, client, admins, recovery_address) } @@ -1032,8 +1034,8 @@ mod tests { #[test] #[should_panic(expected = "Error(Contract, #1)")] fn governance_rejects_double_initialization() { - let (_env, client, admins, recovery) = setup(); - let deployer = Address::generate(&env); + let (env, client, admins, recovery) = setup(); + let deployer = Address::generate(&env); client.init(&deployer, &admins, &2, &recovery); } @@ -1046,8 +1048,8 @@ mod tests { let recovery = Address::generate(&env); let contract_id = env.register_contract(None, GovernanceContract); let client = GovernanceContractClient::new(&env, &contract_id); - let deployer = Address::generate(&env); - client.init(&deployer, &vec![env, admin], &0, &recovery); + let deployer = Address::generate(&env); + client.init(&deployer, &soroban_sdk::vec![&env, admin], &0, &recovery); } #[test] @@ -1443,7 +1445,8 @@ mod tests { let contract_id = env.register_contract(None, GovernanceContract); let client = GovernanceContractClient::new(&env, &contract_id); - let result = client.try_init(&admins, &threshold, &recovery); + let deployer = Address::generate(&env); + let result = client.try_init(&deployer, &admins, &threshold, &recovery); if threshold == 0 || threshold > admin_count { prop_assert!(result.is_err()); } else { @@ -1471,8 +1474,13 @@ mod tests { let client = GovernanceContractClient::new(&env, &contract_id); assert!(!client.is_initialized()); - let deployer = Address::generate(&env); - client.init(&deployer, &vec![env, admin.clone()], &1, &recovery_address); + let deployer = Address::generate(&env); + client.init( + &deployer, + &soroban_sdk::vec![&env, admin.clone()], + &1, + &recovery_address, + ); assert!(client.is_initialized()); } @@ -1487,7 +1495,7 @@ mod tests { let client = GovernanceContractClient::new(&env, &contract_id); let admins = vec![&env, admin.clone()]; - let deployer = Address::generate(&env); + let deployer = Address::generate(&env); client.init(&deployer, &admins, &1, &recovery_address); assert!(client.is_initialized()); assert_eq!(client.get_admin(), admins); @@ -1552,7 +1560,7 @@ mod tests { let contract_id = env.register_contract(None, GovernanceContract); let client = GovernanceContractClient::new(&env, &contract_id); - let deployer = Address::generate(&env); + let deployer = Address::generate(&env); client.init(&deployer, &admins, &1, &recovery); assert_eq!(client.get_threshold(), 1); @@ -1576,7 +1584,7 @@ mod tests { let contract_id = env.register_contract(None, GovernanceContract); let client = GovernanceContractClient::new(&env, &contract_id); - let deployer = Address::generate(&env); + let deployer = Address::generate(&env); client.init(&deployer, &admins, &1, &recovery); // Current threshold is 1, needs 2 signatures for change_threshold, but only 1 provided. @@ -1599,7 +1607,7 @@ mod tests { let contract_id = env.register_contract(None, GovernanceContract); let client = GovernanceContractClient::new(&env, &contract_id); - let deployer = Address::generate(&env); + let deployer = Address::generate(&env); client.init(&deployer, &admins, &1, &recovery); // Threshold 3 > admins.len() 2 — must fail with InvalidThreshold, not auth. @@ -1620,7 +1628,7 @@ mod tests { let contract_id = env.register_contract(None, GovernanceContract); let client = GovernanceContractClient::new(&env, &contract_id); - let deployer = Address::generate(&env); + let deployer = Address::generate(&env); client.init(&deployer, &admins, &2, &recovery); client.change_threshold(&admins, &0); @@ -1832,7 +1840,7 @@ mod tests { let recovery_address = Address::generate(&env); let contract_id = env.register_contract(None, GovernanceContract); let client = GovernanceContractClient::new(&env, &contract_id); - let deployer = Address::generate(&env); + let deployer = Address::generate(&env); client.init(&deployer, &admins, &1, &recovery_address); client.pause(&admins); diff --git a/governance_contract/src/real_auth_tests.rs b/governance_contract/src/real_auth_tests.rs index 828c8128..cce0ebb4 100644 --- a/governance_contract/src/real_auth_tests.rs +++ b/governance_contract/src/real_auth_tests.rs @@ -54,7 +54,8 @@ fn admin_transfer_and_threshold_change_require_real_authorization() { let recovery = Address::generate(&env); let contract_id = env.register_contract(None, GovernanceContract); let client = GovernanceContractClient::new(&env, &contract_id); - client.init(&admins, &1, &recovery); + let deployer = Address::generate(&env); + client.init(&deployer, &admins, &1, &recovery); env.mock_auths(&[]); assert!(client.try_change_threshold(&admins, &2).is_err()); @@ -96,7 +97,8 @@ fn initialization_and_upgrade_require_real_authorization() { let client = GovernanceContractClient::new(&env, &contract_id); let admins = soroban_sdk::vec![&env, admin]; env.mock_auths(&[]); - assert!(client.try_init(&admins, &1, &recovery).is_err()); + let deployer = Address::generate(&env); + assert!(client.try_init(&deployer, &admins, &1, &recovery).is_err()); let (env, client, admins) = super::setup(); let wasm_hash = env diff --git a/settlement_contract/src/admin.rs b/settlement_contract/src/admin.rs index 7e6c90ce..99c8af14 100644 --- a/settlement_contract/src/admin.rs +++ b/settlement_contract/src/admin.rs @@ -11,9 +11,8 @@ use crate::errors::SettlementError; use crate::storage::{ assert_not_paused, is_merchant_registered_and_bump_ttl, read_admin, read_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, - verify_admin_auth, write_admins, + read_recovery_address, read_rule_or_default, read_threshold, validate_admins_and_threshold, + validate_governance, validate_nonzero_address, verify_admin_auth, write_admins, }; use crate::types::{DataKey, Operation, ScheduledOp, SettlementRule}; use crate::{ @@ -389,14 +388,20 @@ impl SettlementContract { env.storage().persistent().remove(&key); match operation { - Operation::UpdateGovernance(new_gov) => Self::_update_governance(&env, &executor, new_gov), + Operation::UpdateGovernance(new_gov) => { + Self::_update_governance(&env, &executor, new_gov) + } Operation::CancelRecovery => Self::_cancel_recovery(&env, &executor), Operation::TransferAdmin(new_admins, new_threshold) => { Self::_transfer_admin(&env, &executor, new_admins, new_threshold) } Operation::Upgrade(wasm_hash) => Self::_upgrade(&env, &executor, wasm_hash), - Operation::RegisterMerchant(merchant) => Self::_register_merchant(&env, &executor, merchant), - Operation::UnregisterMerchant(merchant) => Self::_unregister_merchant(&env, &executor, merchant), + Operation::RegisterMerchant(merchant) => { + Self::_register_merchant(&env, &executor, merchant) + } + Operation::UnregisterMerchant(merchant) => { + Self::_unregister_merchant(&env, &executor, merchant) + } Operation::SetSettlementRule(merchant, rule) => { Self::_set_settlement_rule(&env, &executor, merchant, rule) } @@ -474,7 +479,12 @@ impl SettlementContract { events::emit_recovery_cancelled(env, executor); } - fn _transfer_admin(env: &Env, executor: &Address, new_admins: Vec
, new_threshold: u32) { + fn _transfer_admin( + env: &Env, + _executor: &Address, + new_admins: Vec
, + new_threshold: u32, + ) { let old_admin = read_admin(env); validate_admins_and_threshold(env, &new_admins, new_threshold); // Enforce admin/merchant exclusivity in both directions (issue #692). @@ -522,7 +532,7 @@ impl SettlementContract { SettlementError::EmptyAddress, SettlementError::ZeroAddress, ); - + // Prevent an admin from being registered as a merchant let admins = read_admins(env); for i in 0..admins.len() { @@ -602,7 +612,12 @@ impl SettlementContract { ); } - fn _set_settlement_rule(env: &Env, executor: &Address, merchant: Address, rule: SettlementRule) { + fn _set_settlement_rule( + env: &Env, + executor: &Address, + merchant: Address, + rule: SettlementRule, + ) { assert_not_paused(env); if !is_merchant_registered_and_bump_ttl(env, merchant.clone()) { diff --git a/settlement_contract/src/merchant.rs b/settlement_contract/src/merchant.rs index 5e37fc44..88679818 100644 --- a/settlement_contract/src/merchant.rs +++ b/settlement_contract/src/merchant.rs @@ -9,8 +9,7 @@ use crate::storage::{ }; use crate::types::{DataKey, SettlementRule}; use crate::{ - SettlementContract, SettlementContractClient, MERCHANT_TTL_BUMP, - MERCHANT_TTL_THRESHOLD, + SettlementContract, SettlementContractClient, MERCHANT_TTL_BUMP, MERCHANT_TTL_THRESHOLD, }; #[contractimpl] diff --git a/settlement_contract/src/payments.rs b/settlement_contract/src/payments.rs index fddf6a5a..2e21c0b2 100644 --- a/settlement_contract/src/payments.rs +++ b/settlement_contract/src/payments.rs @@ -9,8 +9,8 @@ use crate::storage::{ }; use crate::types::{DataKey, FeeSplit, PaymentRecord, SettlementRule}; use crate::{ - SettlementContract, SettlementContractClient, MAX_PAYMENTS_BATCH, - PAYMENT_TTL_BUMP, PAYMENT_TTL_THRESHOLD, + SettlementContract, SettlementContractClient, MAX_PAYMENTS_BATCH, PAYMENT_TTL_BUMP, + PAYMENT_TTL_THRESHOLD, }; /// Computes the platform, network, and merchant fee amounts for an amount using ceil-based rounding. diff --git a/settlement_contract/src/settlement.rs b/settlement_contract/src/settlement.rs index fa39273f..0b420e13 100644 --- a/settlement_contract/src/settlement.rs +++ b/settlement_contract/src/settlement.rs @@ -7,8 +7,8 @@ use bettapay_common::{ use crate::errors::SettlementError; use crate::storage::{ - assert_not_paused, is_merchant_registered_and_bump_ttl, read_fallback_rule, read_rule_or_default, - read_threshold, validate_fee_against_governance, verify_admin_auth, + assert_not_paused, is_merchant_registered_and_bump_ttl, read_fallback_rule, + read_rule_or_default, read_threshold, validate_fee_against_governance, verify_admin_auth, }; use crate::types::{DataKey, SettlementRule}; use crate::{ diff --git a/settlement_contract/src/storage.rs b/settlement_contract/src/storage.rs index ede2d17f..3eb4c80d 100644 --- a/settlement_contract/src/storage.rs +++ b/settlement_contract/src/storage.rs @@ -13,10 +13,10 @@ //! (contract trap, host error, or contract-returned error) surfaces as //! `GovernanceCallFailed`, so a broken or mis-deployed governance contract //! cannot abort the transaction with a raw panic. -use soroban_sdk::{panic_with_error, Address, Env, Symbol, Val, Vec}; +use soroban_sdk::{panic_with_error, Address, Env, IntoVal, Symbol, Val, Vec}; use bettapay_common::{ - events::{self, PendingRecovery}, + events::PendingRecovery, storage::{self, CommonDataKey}, }; @@ -338,7 +338,7 @@ pub(crate) fn read_min_payment_amount(env: &Env) -> i128 { return crate::MIN_PAYMENT_AMOUNT; }; let mut args = Vec::::new(env); - args.push_back(Symbol::new(env, "min_payment").into()); + args.push_back(Symbol::new(env, "min_payment").into_val(env)); match env.try_invoke_contract::, SettlementError>( &governance, &Symbol::new(env, "get_system_param"), diff --git a/settlement_contract/src/tests/admin_tests.rs b/settlement_contract/src/tests/admin_tests.rs index 70dcf266..44594b89 100644 --- a/settlement_contract/src/tests/admin_tests.rs +++ b/settlement_contract/src/tests/admin_tests.rs @@ -28,6 +28,7 @@ fn emits_event_on_initialization() { let contract_id = env.register_contract(None, SettlementContract); let client = SettlementContractClient::new(&env, &contract_id); + let deployer = Address::generate(&env); client.init( &deployer, &soroban_sdk::vec![&env, admin.clone()], @@ -538,6 +539,7 @@ fn recovery_executes_after_delay() { let contract_id = env.register_contract(None, SettlementContract); let client = SettlementContractClient::new(&env, &contract_id); + let deployer = Address::generate(&env); client.init( &deployer, &soroban_sdk::vec![&env, admin.clone()], diff --git a/settlement_contract/src/tests/governance_error_tests.rs b/settlement_contract/src/tests/governance_error_tests.rs index 30d0d59e..6d4e5fe2 100644 --- a/settlement_contract/src/tests/governance_error_tests.rs +++ b/settlement_contract/src/tests/governance_error_tests.rs @@ -10,8 +10,9 @@ use crate::types::DataKey; use crate::*; -use soroban_sdk::testutils::Address as _; -use soroban_sdk::{Address, Env}; +use governance_contract::{GovernanceContract, GovernanceContractClient}; +use soroban_sdk::testutils::{Address as _, Events}; +use soroban_sdk::{Address, Env, FromVal, Symbol}; // --------------------------------------------------------------------------- // Failing governance stub — lives in its own module to avoid symbol collisions @@ -61,8 +62,6 @@ mod erroring_gov { } } -use erroring_gov::ErroringGovernance; - /// Helper: directly injects a governance address into the settlement contract's /// instance storage, bypassing `validate_governance` (which would itself call /// `get_fee_config` and fail against the panicking stub). @@ -100,7 +99,13 @@ fn read_path_governance_failure_surfaces_typed_error() { let contract_id = env.register_contract(None, SettlementContract); let client = SettlementContractClient::new(&env, &contract_id); let deployer = Address::generate(&env); - client.init(&deployer, &soroban_sdk::vec![&env, admin.clone()], &1, &empty_gov, &recovery); + client.init( + &deployer, + &soroban_sdk::vec![&env, admin.clone()], + &1, + &empty_gov, + &recovery, + ); client.register_merchant(&soroban_sdk::vec![&env, admin.clone()], &merchant); @@ -127,7 +132,13 @@ fn read_path_governance_none_falls_through_to_bootstrap() { let contract_id = env.register_contract(None, SettlementContract); let client = SettlementContractClient::new(&env, &contract_id); let deployer = Address::generate(&env); - client.init(&deployer, &soroban_sdk::vec![&env, admin.clone()], &1, &empty_gov, &recovery); + client.init( + &deployer, + &soroban_sdk::vec![&env, admin.clone()], + &1, + &empty_gov, + &recovery, + ); client.register_merchant(&soroban_sdk::vec![&env, admin], &merchant); // Empty governance returns None — bootstrap default should apply (100 bps platform, 5 network). @@ -161,7 +172,13 @@ fn write_path_governance_failure_surfaces_typed_error() { let contract_id = env.register_contract(None, SettlementContract); let client = SettlementContractClient::new(&env, &contract_id); let deployer = Address::generate(&env); - client.init(&deployer, &soroban_sdk::vec![&env, admin.clone()], &1, &empty_gov, &recovery); + client.init( + &deployer, + &soroban_sdk::vec![&env, admin.clone()], + &1, + &empty_gov, + &recovery, + ); client.register_merchant(&soroban_sdk::vec![&env, admin.clone()], &merchant); // Directly inject the panicking governance address. @@ -196,7 +213,13 @@ fn write_path_set_default_rule_governance_failure_surfaces_typed_error() { let contract_id = env.register_contract(None, SettlementContract); let client = SettlementContractClient::new(&env, &contract_id); let deployer = Address::generate(&env); - client.init(&deployer, &soroban_sdk::vec![&env, admin.clone()], &1, &empty_gov, &recovery); + client.init( + &deployer, + &soroban_sdk::vec![&env, admin.clone()], + &1, + &empty_gov, + &recovery, + ); // Directly inject the panicking governance address. inject_governance(&env, &contract_id, &panicking_gov); @@ -216,8 +239,8 @@ fn write_path_set_default_rule_governance_failure_surfaces_typed_error() { // --------------------------------------------------------------------------- mod reentrant_gov { - use soroban_sdk::{contract, contractimpl, IntoVal, Address, Env, Symbol}; use crate::{GovFeeConfig, SettlementContractClient}; + use soroban_sdk::{contract, contractimpl, Address, Env, Symbol}; /// A governance stub that attempts to call back into SettlementContract /// during `get_fee_config` (simulates reentrancy). @@ -263,7 +286,9 @@ fn init_succeeds_with_panicking_governance() { let client = SettlementContractClient::new(&env, &contract_id); // init must succeed directly with panicking_gov without cross-calling it + let deployer = Address::generate(&env); client.init( + &deployer, &soroban_sdk::vec![&env, admin.clone()], &1, &panicking_gov, @@ -290,7 +315,8 @@ fn update_governance_succeeds_with_panicking_governance() { let client = SettlementContractClient::new(&env, &contract_id); let admins = soroban_sdk::vec![&env, admin]; - client.init(&admins, &1, &empty_gov, &recovery); + let deployer = Address::generate(&env); + client.init(&deployer, &admins, &1, &empty_gov, &recovery); client.update_governance(&admins, &panicking_gov); assert_eq!(client.get_governance(), panicking_gov); @@ -317,7 +343,9 @@ fn init_succeeds_with_reentrant_governance_and_prevents_double_init() { soroban_sdk::vec![&env, contract_id.to_val()], ); + let deployer = Address::generate(&env); client.init( + &deployer, &soroban_sdk::vec![&env, admin.clone()], &1, &reentrant_gov_id, @@ -329,6 +357,7 @@ fn init_succeeds_with_reentrant_governance_and_prevents_double_init() { // Reentry / second initialization must panic with AlreadyInitialized let res = client.try_init( + &deployer, &soroban_sdk::vec![&env, admin], &1, &reentrant_gov_id, @@ -346,16 +375,22 @@ fn read_path_governance_valid_config_used() { let env = Env::default(); env.mock_all_auths(); - let gov_id = env.register_contract(None, crate::GovernanceContract); - let gov_client = crate::GovernanceContractClient::new(&env, &gov_id); + let gov_id = env.register_contract(None, GovernanceContract); + let gov_client = GovernanceContractClient::new(&env, &gov_id); let gov_admin = Address::generate(&env); let recovery = Address::generate(&env); - gov_client.init(&soroban_sdk::vec![&env, gov_admin.clone()], &1, &recovery); + let gov_deployer = Address::generate(&env); + gov_client.init( + &gov_deployer, + &soroban_sdk::vec![&env, gov_admin.clone()], + &1, + &recovery, + ); // Set governance fee config: 250 platform bps, 50 network bps gov_client.set_fee_config( &soroban_sdk::vec![&env, gov_admin], - &GovFeeConfig { + &governance_contract::FeeConfig { platform_fee_bps: 250, network_fee_bps: 50, }, @@ -365,7 +400,14 @@ fn read_path_governance_valid_config_used() { let merchant = Address::generate(&env); let contract_id = env.register_contract(None, SettlementContract); let client = SettlementContractClient::new(&env, &contract_id); - client.init(&soroban_sdk::vec![&env, admin.clone()], &1, &gov_id, &recovery); + let deployer = Address::generate(&env); + client.init( + &deployer, + &soroban_sdk::vec![&env, admin.clone()], + &1, + &gov_id, + &recovery, + ); client.register_merchant(&soroban_sdk::vec![&env, admin], &merchant); let split = client.calculate_fee_split(&merchant, &10_000); @@ -388,7 +430,14 @@ fn read_path_governance_none_emits_bootstrap_fallback_event() { let contract_id = env.register_contract(None, SettlementContract); let client = SettlementContractClient::new(&env, &contract_id); - client.init(&soroban_sdk::vec![&env, admin.clone()], &1, &empty_gov, &recovery); + let deployer = Address::generate(&env); + client.init( + &deployer, + &soroban_sdk::vec![&env, admin.clone()], + &1, + &empty_gov, + &recovery, + ); client.register_merchant(&soroban_sdk::vec![&env, admin], &merchant); client.calculate_fee_split(&merchant, &10_000); @@ -409,4 +458,3 @@ fn read_path_governance_none_emits_bootstrap_fallback_event() { "BOOTSTRAP_FALLBACK_EVENT must be emitted when degrading to bootstrap defaults" ); } - diff --git a/settlement_contract/src/tests/integration_tests.rs b/settlement_contract/src/tests/integration_tests.rs index 9ddbc850..806966bc 100644 --- a/settlement_contract/src/tests/integration_tests.rs +++ b/settlement_contract/src/tests/integration_tests.rs @@ -1061,7 +1061,13 @@ fn store_payment_reference_prevents_reentrancy() { let settle_client = SettlementContractClient::new(&env, &settle_id); let deployer = Address::generate(&env); - settle_client.init(&deployer, &settle_admins, &1, &mock_gov_id, &settle_recovery); + settle_client.init( + &deployer, + &settle_admins, + &1, + &mock_gov_id, + &settle_recovery, + ); let merchant = Address::generate(&env); settle_client.register_merchant(&settle_admins, &merchant); diff --git a/settlement_contract/src/tests/recovery_admin_set_tests.rs b/settlement_contract/src/tests/recovery_admin_set_tests.rs index d925fbf6..4de89194 100644 --- a/settlement_contract/src/tests/recovery_admin_set_tests.rs +++ b/settlement_contract/src/tests/recovery_admin_set_tests.rs @@ -49,7 +49,13 @@ fn setup_with_admins( let contract_id = env.register_contract(None, SettlementContract); let client = SettlementContractClient::new(env, &contract_id); let deployer = Address::generate(env); - client.init(&deployer, admins, &threshold, &governance, &recovery_address); + client.init( + &deployer, + admins, + &threshold, + &governance, + &recovery_address, + ); (client, recovery_address) } diff --git a/settlement_contract/src/tests/schedule_collision_tests.rs b/settlement_contract/src/tests/schedule_collision_tests.rs index 830a6386..1aabdca9 100644 --- a/settlement_contract/src/tests/schedule_collision_tests.rs +++ b/settlement_contract/src/tests/schedule_collision_tests.rs @@ -69,7 +69,7 @@ fn schedule_detects_collision_with_unrelated_pending_operation() { #[test] #[should_panic(expected = "Error(Contract, #11)")] fn execute_rejects_operation_that_only_collides_on_hash() { - let (env, client, _admins, merchant) = setup(); + let (env, client, admins, merchant) = setup(); let operation = Operation::RegisterMerchant(merchant); let unrelated_bytes = soroban_sdk::Bytes::from_slice(&env, b"not this operation's xdr"); diff --git a/settlement_contract/src/tests/timelock_tests.rs b/settlement_contract/src/tests/timelock_tests.rs index cf31d622..ce04d7ce 100644 --- a/settlement_contract/src/tests/timelock_tests.rs +++ b/settlement_contract/src/tests/timelock_tests.rs @@ -1,8 +1,6 @@ //! Regression coverage for the settlement administrative timelock. -use crate::{ - Operation, SettlementContractClient, SettlementRule, DEFAULT_TIMELOCK_DELAY_SECONDS, -}; +use crate::{Operation, SettlementContractClient, SettlementRule, DEFAULT_TIMELOCK_DELAY_SECONDS}; use soroban_sdk::testutils::{Address as _, Ledger}; use soroban_sdk::{Address, Env}; @@ -16,7 +14,9 @@ fn scheduled_operation_executes_only_after_delay() { let operation = Operation::TransferAdmin(new_admins.clone(), 1); client.schedule(&admins, &operation, &DEFAULT_TIMELOCK_DELAY_SECONDS); - assert!(client.try_execute(&admins.get(0).unwrap(), &operation).is_err()); + assert!(client + .try_execute(&admins.get(0).unwrap(), &operation) + .is_err()); assert_eq!(client.get_admin(), admins); env.ledger() @@ -25,7 +25,9 @@ fn scheduled_operation_executes_only_after_delay() { assert_eq!(client.get_admin(), soroban_sdk::vec![&env, new_admin]); assert_eq!(client.get_threshold(), 1); - assert!(client.try_execute(&admins.get(0).unwrap(), &operation).is_err()); + assert!(client + .try_execute(&admins.get(0).unwrap(), &operation) + .is_err()); } #[test] @@ -71,7 +73,9 @@ fn admin_can_cancel_but_non_admin_cannot() { env.ledger() .with_mut(|ledger| ledger.timestamp += DEFAULT_TIMELOCK_DELAY_SECONDS); - assert!(client.try_execute(&admins.get(0).unwrap(), &operation).is_err()); + assert!(client + .try_execute(&admins.get(0).unwrap(), &operation) + .is_err()); assert!(client .try_cancel(&soroban_sdk::vec![&env, admins.get(0).unwrap()], &operation) .is_err()); @@ -90,7 +94,9 @@ fn multisig_schedule_and_cancel_require_two_of_three_signers() { client.schedule(&two_signers, &operation, &DEFAULT_TIMELOCK_DELAY_SECONDS); assert!(client.try_cancel(&one_signer, &operation).is_err()); client.cancel(&two_signers, &operation); - assert!(client.try_execute(&admins.get(0).unwrap(), &operation).is_err()); + assert!(client + .try_execute(&admins.get(0).unwrap(), &operation) + .is_err()); } #[test] @@ -104,7 +110,9 @@ fn multisig_schedule_and_execute_apply_operation_after_delay() { env.ledger() .with_mut(|ledger| ledger.timestamp += DEFAULT_TIMELOCK_DELAY_SECONDS - 1); - assert!(client.try_execute(&admins.get(0).unwrap(), &operation).is_err()); + assert!(client + .try_execute(&admins.get(0).unwrap(), &operation) + .is_err()); assert!(!client.is_merchant_registered(&merchant)); env.ledger().with_mut(|ledger| ledger.timestamp += 1); @@ -224,7 +232,7 @@ fn timelocked_transfer_admin_parity_with_direct_path() { env.ledger() .with_mut(|ledger| ledger.timestamp += DEFAULT_TIMELOCK_DELAY_SECONDS); - client.execute(&admins.get(0).unwrap(), &operation); + client.execute(&a1, &operation); assert_eq!( client.get_admin(), @@ -410,27 +418,27 @@ fn test_execute_uniform_auth_all_variants() { env.ledger() .with_mut(|ledger| ledger.timestamp += DEFAULT_TIMELOCK_DELAY_SECONDS); - // Disable caller-auth mocking: any `require_auth` inside `execute` now - // fails with `Unauthorized`. Every variant must still execute. - env.set_auths(&[]); + // Execute every variant: `execute` requires executor auth, which is + // covered by the `mock_all_auths()` enabled at the start of the test. + let executor = admins.get(0).unwrap(); - client.execute(&op_update_governance); + client.execute(&executor, &op_update_governance); assert_eq!(client.get_governance(), new_gov); - client.execute(&op_cancel_recovery); + client.execute(&executor, &op_cancel_recovery); assert!(client.try_execute_recovery().is_err()); - client.execute(&op_transfer_admin); + client.execute(&executor, &op_transfer_admin); assert_eq!(client.get_admin(), new_admins); assert_eq!(client.get_threshold(), 1); - client.execute(&op_register_merchant); + client.execute(&executor, &op_register_merchant); assert!(client.is_merchant_registered(&merchant)); - client.execute(&op_unregister_merchant); + client.execute(&executor, &op_unregister_merchant); assert!(!client.is_merchant_registered(&merchant2)); - client.execute(&op_set_settlement_rule); + client.execute(&executor, &op_set_settlement_rule); let stored_rule = client.get_settlement_rule(&merchant3).unwrap(); assert_eq!(stored_rule.platform_fee_bps, rule.platform_fee_bps); assert_eq!(stored_rule.network_fee_bps, rule.network_fee_bps); @@ -440,10 +448,10 @@ fn test_execute_uniform_auth_all_variants() { ); assert_eq!(stored_rule.auto_settle, rule.auto_settle); - client.execute(&op_clear_settlement_rule); + client.execute(&executor, &op_clear_settlement_rule); assert!(client.get_settlement_rule(&merchant4).is_none()); - client.execute(&op_set_default_rule); + client.execute(&executor, &op_set_default_rule); let stored_default = client.get_default_rule().unwrap(); assert_eq!(stored_default.platform_fee_bps, rule.platform_fee_bps); assert_eq!(stored_default.network_fee_bps, rule.network_fee_bps); @@ -456,9 +464,8 @@ fn test_execute_uniform_auth_all_variants() { // `Upgrade` runs last: the test host lets an empty Wasm stand in for a // valid contract, and `execute`'s `_upgrade` (unlike the admin-gated // `upgrade` path) does not probe `supports_interface`. So this arm - // succeeds — and succeeding with caller-auth mocking disabled is the - // proof that it has no auth gate either. - client.execute(&op_upgrade); + // succeeds. + client.execute(&executor, &op_upgrade); } /// Focused regression for the variant named in issue #561: a scheduled @@ -477,10 +484,9 @@ fn scheduled_cancel_recovery_executes_without_caller_auth() { env.ledger() .with_mut(|ledger| ledger.timestamp += DEFAULT_TIMELOCK_DELAY_SECONDS); - // No caller auth is mocked: the old primary-admin `require_auth` would - // fail here with `Unauthorized`. - env.set_auths(&[]); - client.execute(&op); + // `execute` requires executor auth, which is covered by the + // `mock_all_auths()` enabled at the start of the test. + client.execute(&admins.get(0).unwrap(), &op); // The pending recovery is gone. assert!(client.try_execute_recovery().is_err());