From fb269c224420c853babc53664b35703429607bf1 Mon Sep 17 00:00:00 2001 From: Baskarayelu Date: Thu, 27 Aug 2026 20:50:18 +0530 Subject: [PATCH] fix(attestation): centralize bounded metric accounting --- .../attestation_engine/METRIC_CONSISTENCY.md | 167 +++++++++++ contracts/attestation_engine/src/lib.rs | 125 ++++++++- .../src/metric_consistency_tests.rs | 265 ++++++++++++++++++ 3 files changed, 544 insertions(+), 13 deletions(-) create mode 100644 contracts/attestation_engine/METRIC_CONSISTENCY.md create mode 100644 contracts/attestation_engine/src/metric_consistency_tests.rs diff --git a/contracts/attestation_engine/METRIC_CONSISTENCY.md b/contracts/attestation_engine/METRIC_CONSISTENCY.md new file mode 100644 index 0000000..8263bad --- /dev/null +++ b/contracts/attestation_engine/METRIC_CONSISTENCY.md @@ -0,0 +1,167 @@ +# Attestation metric consistency + +This document describes the accounting guarantees implemented for issue #549. +The attestation engine stores the individual records and exposes health metrics +derived from those records. The two representations must agree after every +successful transaction and must remain unchanged after every rejected record. + +## Invariants + +The following invariants are part of the contract's storage interface: + +1. `fees_generated` is the sum of accepted `fee_generation` records for the + commitment. Fee values are non-negative integers. +2. `drawdown_percent` is a percentage in the inclusive range `0..=100`. +3. `compliance_score` is a score in the inclusive range `0..=100`. +4. `total_attestations` counts accepted records only. +5. `total_violations` counts accepted violations and accepted non-compliant + records only. +6. A verifier's count contains accepted records only. +7. The per-commitment attestation counter equals the number of records in the + commitment's history. +8. A rejected transaction does not leave a partial history, metric, or counter + update behind. + +These rules apply to both the general `attest` entrypoint and the convenience +entrypoints `record_fees` and `record_drawdown`. Keeping validation in the +shared writer prevents a convenience path from accidentally bypassing a bound +that applies to the general path. + +## Validation order + +An accepted record follows this order: + +1. Authenticate the caller when the selected entrypoint requires it. +2. Validate the commitment identifier. +3. Resolve the commitment from the configured commitment core contract. +4. Validate the attestation type and payload. +5. Validate metric-specific numeric bounds. +6. Append the record to the commitment history. +7. Update the cached commitment metrics. +8. Update global and verifier counters. +9. Emit the corresponding event. + +Metric-specific validation happens before the history append. This is +important because malformed or out-of-range data must not be stored merely to +be ignored by a later aggregation pass. + +## Fee accounting + +Fee-generation records carry a `fee_amount` string. The value must parse as an +`i128` and must be greater than or equal to zero. A malformed amount returns +`InvalidAttestationData`; a negative amount returns `InvalidFeeAmount`. + +The per-commitment fee total and the protocol-wide fee total use checked +addition. If either addition would overflow, the transaction returns +`StorageError`. Soroban transaction rollback then restores the history, +metrics, counters, and pre-existing total to their values from before the +attempt. + +The global total is updated only for a fee-generation attestation. Generic +attestations that merely contain an unrelated field named `fee_amount` do not +change the protocol-wide fee total unless their type is `fee_generation`. + +## Drawdown accounting + +Drawdown records carry a `drawdown_percent` string and accept only values from +zero through one hundred, inclusive. The boundary values are valid because a +zero drawdown is a meaningful healthy observation and one hundred represents a +complete drawdown. Negative values and values above one hundred are rejected. + +The cached drawdown metric is updated from accepted records. Read-time +aggregation also clamps the resulting percentage to the documented range, so +legacy records cannot cause an out-of-contract value to be exposed. + +## Compliance score + +The compliance score is stored as an unsigned integer and capped at one +hundred. A compliant non-violation record can increase the score, while +violation and non-compliant records lower it according to the existing scoring +rules. The cap is applied at the point of increment and the read-time score +calculation also preserves the same upper bound. + +This makes repeated successful observations safe: a long history cannot wrap +the score or expose a value higher than the documented maximum. + +## Counters and atomicity + +The contract maintains counters in three places: per commitment, globally, and +per verifier. Each counter uses checked addition. The batch path performs the +same checks before persisting its in-memory aggregate counters. + +All writes belong to the same contract invocation. If a checked operation +returns an error, the invocation fails and Soroban reverts writes from the +failed transaction. Callers can therefore retry a rejected record after fixing +the input without first repairing partially updated metrics. + +The consistency guarantee is intentionally transaction-scoped. A successful +transaction may update several related storage keys, but it never publishes a +new history length without the corresponding cached metrics and counters. + +## Read behavior + +`get_stored_health_metrics` returns the cached value and does not recalculate +or mutate storage. It returns `None` until the first record is accepted. + +`get_health_metrics` performs read-time aggregation against the canonical +commitment record and stored history. Calling it repeatedly is observational: +the result and stored attestation count remain unchanged. This prevents +monitoring dashboards or compliance checks from changing the accounting they +are observing. + +## Test matrix + +The metric consistency test module covers the following cases: + +| Case | Expected result | +| --- | --- | +| Empty history | Bounded zero baseline values | +| One accepted fee | Fee total and count increase once | +| One accepted drawdown | Boundary-safe drawdown is preserved | +| Mixed history | Score, drawdown, and count stay bounded | +| Repeated reads | Values and counters remain unchanged | +| Equivalent fee orderings | Same accepted total and count | +| Negative fee | `InvalidFeeAmount`, no history mutation | +| Invalid drawdown | `InvalidAttestationData`, no history mutation | +| Generic negative fee field | Rejected before append | +| Maximum drawdown | `100` is accepted | +| Fee accumulator overflow | `StorageError`, atomic rollback | + +The tests use a deterministic mock commitment-core contract. This keeps the +tests focused on the attestation engine's storage and aggregation behavior +while still exercising the production cross-contract lookup. + +## Operational guidance + +Clients should treat `InvalidAttestationData` and `InvalidFeeAmount` as input +errors and should not retry the same payload unchanged. A `StorageError` is a +contract-level failure; callers may retry after checking the account and +transaction context, but should not assume that a partial record was written. + +Indexers should use accepted-record events and counters as transaction-level +signals. They should not infer acceptance from a submitted transaction alone, +because a rejected invocation emits no successful record event and rolls back +its storage changes. + +For upgrades, existing histories remain readable. New writes are subject to +the bounds and checked arithmetic described above, while read-time aggregation +keeps exposed percentages and scores within their public ranges. + +## Verification + +Run the focused metric tests with: + +```text +cargo +1.88.0 test -p attestation_engine metric_consistency_tests +``` + +Run the complete Rust library test suite with: + +```text +cargo +1.88.0 test -p attestation_engine --lib +``` + +The library suite includes the existing attestation tests and the consistency +tests in this document. Repository documentation examples are maintained +separately and are not required to execute contract code in the library test +run. diff --git a/contracts/attestation_engine/src/lib.rs b/contracts/attestation_engine/src/lib.rs index 1f1bacf..4356dca 100644 --- a/contracts/attestation_engine/src/lib.rs +++ b/contracts/attestation_engine/src/lib.rs @@ -12,6 +12,8 @@ use soroban_sdk::{ }; const CURRENT_VERSION: u32 = 1; +const MAX_PERCENT: i128 = 100; +const MAX_COMPLIANCE_SCORE: u32 = 100; // ============================================================================ // Error Types @@ -616,6 +618,42 @@ impl AttestationEngineContract { } } + /// Validate numeric fields before a record is appended to history. This + /// keeps every writer (single and convenience entrypoints) on the same + /// bounds policy and ensures rejected input cannot affect cached metrics. + fn validate_metric_bounds( + e: &Env, + attestation_type: &String, + data: &Map, + ) -> Result<(), AttestationError> { + let fee_generation = String::from_str(e, "fee_generation"); + let drawdown = String::from_str(e, "drawdown"); + + if *attestation_type == fee_generation { + let key = String::from_str(e, "fee_amount"); + let value = data + .get(key) + .and_then(|raw| Self::parse_i128_from_string(e, &raw)) + .ok_or(AttestationError::InvalidAttestationData)?; + if value < 0 { + return Err(AttestationError::InvalidFeeAmount); + } + } + + if *attestation_type == drawdown { + let key = String::from_str(e, "drawdown_percent"); + let value = data + .get(key) + .and_then(|raw| Self::parse_i128_from_string(e, &raw)) + .ok_or(AttestationError::InvalidAttestationData)?; + if !(0..=MAX_PERCENT).contains(&value) { + return Err(AttestationError::InvalidAttestationData); + } + } + + Ok(()) + } + /// Check if commitment exists in core contract fn commitment_exists(e: &Env, commitment_id: &String) -> bool { let commitment_core: Address = match e.storage().instance().get(&DataKey::CoreContract) { @@ -646,7 +684,11 @@ impl AttestationEngineContract { /// Recomputes aggregate fee and volatility fields from the stored /// attestation history so cached metrics stay aligned with read-time /// aggregation. - fn update_health_metrics(e: &Env, commitment_id: &String, attestation: &Attestation) { + fn update_health_metrics( + e: &Env, + commitment_id: &String, + attestation: &Attestation, + ) -> Result<(), AttestationError> { // Get or create health metrics let key = DataKey::HealthMetrics(commitment_id.clone()); let mut metrics: HealthMetrics = @@ -689,7 +731,9 @@ impl AttestationEngineContract { if let Some(fee_amount) = Self::parse_i128_from_string(e, &fee_str) { let total_fees: i128 = e.storage().instance().get(&DataKey::TotalFees).unwrap_or(0); - let new_total = total_fees.checked_add(fee_amount).unwrap_or(total_fees); + let new_total = total_fees + .checked_add(fee_amount) + .ok_or(AttestationError::StorageError)?; e.storage().instance().set(&DataKey::TotalFees, &new_total); } } @@ -717,11 +761,15 @@ impl AttestationEngineContract { if attestation.is_compliant && attestation.attestation_type != violation { // Small bonus for compliant attestations, capped at 100 metrics.compliance_score = - core::cmp::min(100, metrics.compliance_score.saturating_add(1)); + core::cmp::min( + MAX_COMPLIANCE_SCORE, + metrics.compliance_score.saturating_add(1), + ); } // Store updated metrics e.storage().persistent().set(&key, &metrics); + Ok(()) } fn aggregate_attestation_metrics( @@ -932,6 +980,8 @@ impl AttestationEngineContract { return Err(AttestationError::InvalidAttestationData); } + Self::validate_metric_bounds(e, &attestation_type, &data)?; + // 7b. Collect attestation verification fee if configured let fee_amount: i128 = e .storage() @@ -979,12 +1029,15 @@ impl AttestationEngineContract { e.storage().persistent().set(&key, &attestations); // 10. Update health metrics - Self::update_health_metrics(e, &commitment_id, &attestation); + Self::update_health_metrics(e, &commitment_id, &attestation)?; // 11. Increment attestation counter let counter_key = DataKey::AttestationCounter(commitment_id.clone()); let counter: u64 = e.storage().persistent().get(&counter_key).unwrap_or(0); - e.storage().persistent().set(&counter_key, &(counter + 1)); + let next_counter = counter + .checked_add(1) + .ok_or(AttestationError::StorageError)?; + e.storage().persistent().set(&counter_key, &next_counter); // 11b. Batch update analytics counters let total_att: u64 = e @@ -1000,20 +1053,29 @@ impl AttestationEngineContract { let verifier_key = DataKey::VerifierAttestationCount(caller.clone()); let ver_count: u64 = e.storage().instance().get(&verifier_key).unwrap_or(0u64); + let next_total_attestations = total_att + .checked_add(1) + .ok_or(AttestationError::StorageError)?; e.storage() .instance() - .set(&DataKey::TotalAttestations, &(total_att + 1)); + .set(&DataKey::TotalAttestations, &next_total_attestations); let violation_type = String::from_str(e, "violation"); if attestation.attestation_type == violation_type || !attestation.is_compliant { + let next_total_violations = total_viol + .checked_add(1) + .ok_or(AttestationError::StorageError)?; e.storage() .instance() - .set(&DataKey::TotalViolations, &(total_viol + 1)); + .set(&DataKey::TotalViolations, &next_total_violations); } + let next_verifier_count = ver_count + .checked_add(1) + .ok_or(AttestationError::StorageError)?; e.storage() .instance() - .set(&verifier_key, &(ver_count + 1)); + .set(&verifier_key, &next_verifier_count); // 12. Emit event e.events().publish( @@ -2000,6 +2062,29 @@ impl AttestationEngineContract { } } + // Apply the same fee and drawdown bounds used by single-record + // writes so batch and non-batch histories are equivalent. + if let Err(metric_error) = + Self::validate_metric_bounds(&e, ¶ms.attestation_type, ¶ms.data) + { + if mode == BatchMode::Atomic { + e.storage().instance().remove(&DataKey::ReentrancyGuard); + errors.push_back(BatchError { + index: i, + error_code: metric_error as u32, + context: String::from_str(&e, "metric_bounds"), + }); + return BatchResultVoid::failure(&e, errors); + } else { + errors.push_back(BatchError { + index: i, + error_code: metric_error as u32, + context: String::from_str(&e, "metric_bounds"), + }); + continue; + } + } + // Create attestation record let attestation = Attestation { commitment_id: params.commitment_id.clone(), @@ -2021,18 +2106,30 @@ impl AttestationEngineContract { e.storage().persistent().set(&key, &attestations); // Update health metrics - Self::update_health_metrics(&e, ¶ms.commitment_id, &attestation); + Self::update_health_metrics(&e, ¶ms.commitment_id, &attestation) + .expect("metric aggregate invariant"); // Increment attestation counter let counter_key = DataKey::AttestationCounter(params.commitment_id.clone()); let counter: u64 = e.storage().persistent().get(&counter_key).unwrap_or(0); - e.storage().persistent().set(&counter_key, &(counter + 1)); + let next_counter = counter + .checked_add(1) + .expect("commitment attestation counter overflow"); + e.storage() + .persistent() + .set(&counter_key, &next_counter); // Update analytics counters (in memory) - total_attestations = total_attestations.checked_add(1).unwrap(); - verifier_count = verifier_count.checked_add(1).unwrap(); + total_attestations = total_attestations + .checked_add(1) + .expect("total attestation counter overflow"); + verifier_count = verifier_count + .checked_add(1) + .expect("verifier attestation counter overflow"); if attestation.attestation_type == violation_type || !attestation.is_compliant { - total_violations = total_violations.checked_add(1).unwrap(); + total_violations = total_violations + .checked_add(1) + .expect("total violation counter overflow"); } results.push_back(()); @@ -2280,3 +2377,5 @@ fn require_valid_wasm_hash(e: &Env, wasm_hash: &BytesN<32>) -> Result<(), Attest mod benchmarks; #[cfg(all(test, not(target_arch = "wasm32")))] mod tests; +#[cfg(all(test, not(target_arch = "wasm32")))] +mod metric_consistency_tests; diff --git a/contracts/attestation_engine/src/metric_consistency_tests.rs b/contracts/attestation_engine/src/metric_consistency_tests.rs new file mode 100644 index 0000000..1c80b56 --- /dev/null +++ b/contracts/attestation_engine/src/metric_consistency_tests.rs @@ -0,0 +1,265 @@ +#![cfg(test)] + +//! Deterministic metric consistency tests for issue #549. +//! +//! The mock core contract supplies the canonical commitment record so these +//! tests exercise the real attestation writers and storage aggregation paths. + +use super::*; +use soroban_sdk::testutils::Address as _; +use soroban_sdk::{contract, contractimpl, Address, Env, Map, String}; + +#[contract] +struct MockCore; + +#[contractimpl] +impl MockCore { + pub fn get_commitment(e: Env, commitment_id: String) -> Commitment { + Commitment { + commitment_id, + owner: Address::generate(&e), + nft_token_id: 1, + rules: CommitmentRules { + duration_days: 30, + max_loss_percent: 25, + commitment_type: String::from_str(&e, "balanced"), + early_exit_penalty: 10, + min_fee_threshold: 1_000, + grace_period_days: 0, + }, + amount: 1_000, + asset_address: Address::generate(&e), + created_at: 0, + expires_at: 30 * 86_400, + current_value: 1_000, + status: String::from_str(&e, "active"), + } + } +} + +struct Fixture { + env: Env, + client: AttestationEngineContractClient<'static>, + contract_id: Address, + admin: Address, + commitment_id: String, +} + +fn fixture() -> Fixture { + let env = Env::default(); + env.mock_all_auths(); + let core_id = env.register_contract(None, MockCore); + let contract_id = env.register_contract(None, AttestationEngineContract); + let client = AttestationEngineContractClient::new(&env, &contract_id); + let admin = Address::generate(&env); + let commitment_id = String::from_str(&env, "metric-consistency"); + client.initialize(&admin, &core_id); + Fixture { + env, + client, + contract_id, + admin, + commitment_id, + } +} + +fn fee(fixture: &Fixture, amount: i128) { + fixture + .client + .record_fees(&fixture.admin, &fixture.commitment_id, &amount); +} + +fn drawdown(fixture: &Fixture, amount: i128) { + fixture + .client + .record_drawdown(&fixture.admin, &fixture.commitment_id, &amount); +} + +fn violation_data(env: &Env, severity: &str) -> Map { + let mut data = Map::new(env); + data.set( + String::from_str(env, "violation_type"), + String::from_str(env, "policy"), + ); + data.set( + String::from_str(env, "severity"), + String::from_str(env, severity), + ); + data +} + +#[test] +fn empty_history_has_bounded_baseline_metrics() { + let fixture = fixture(); + let metrics = fixture + .client + .get_health_metrics(&fixture.commitment_id); + + assert_eq!(metrics.fees_generated, 0); + assert_eq!(metrics.volatility_exposure, 0); + assert_eq!(metrics.last_attestation, 0); + assert!(metrics.compliance_score <= MAX_COMPLIANCE_SCORE); +} + +#[test] +fn negative_and_out_of_range_records_are_rejected_without_history_changes() { + let fixture = fixture(); + + assert_eq!( + fixture + .client + .try_record_fees(&fixture.admin, &fixture.commitment_id, &-1), + Err(Ok(AttestationError::InvalidFeeAmount)) + ); + assert_eq!( + fixture + .client + .try_record_drawdown(&fixture.admin, &fixture.commitment_id, &101), + Err(Ok(AttestationError::InvalidAttestationData)) + ); + assert_eq!( + fixture + .client + .try_record_drawdown(&fixture.admin, &fixture.commitment_id, &-1), + Err(Ok(AttestationError::InvalidAttestationData)) + ); + assert_eq!(fixture.client.get_attestation_count(&fixture.commitment_id), 0); + assert!(fixture + .client + .get_stored_health_metrics(&fixture.commitment_id) + .is_none()); +} + +#[test] +fn equivalent_fee_sequences_produce_the_same_total() { + let first = fixture(); + let second = fixture(); + + fee(&first, 10); + fee(&first, 20); + fee(&second, 20); + fee(&second, 10); + + let first_metrics = first + .client + .get_stored_health_metrics(&first.commitment_id) + .unwrap(); + let second_metrics = second + .client + .get_stored_health_metrics(&second.commitment_id) + .unwrap(); + assert_eq!(first_metrics.fees_generated, 30); + assert_eq!(second_metrics.fees_generated, 30); + assert_eq!(first.client.get_attestation_count(&first.commitment_id), 2); + assert_eq!(second.client.get_attestation_count(&second.commitment_id), 2); +} + +#[test] +fn repeated_reads_do_not_change_metric_totals_or_counts() { + let fixture = fixture(); + fee(&fixture, 75); + drawdown(&fixture, 12); + + let before_count = fixture.client.get_attestation_count(&fixture.commitment_id); + let first = fixture.client.get_health_metrics(&fixture.commitment_id); + let second = fixture.client.get_health_metrics(&fixture.commitment_id); + + assert_eq!(first, second); + assert_eq!(first.fees_generated, 75); + assert_eq!(first.drawdown_percent, 12); + assert_eq!( + fixture.client.get_attestation_count(&fixture.commitment_id), + before_count + ); +} + +#[test] +fn mixed_history_keeps_score_and_percentages_in_documented_bounds() { + let fixture = fixture(); + drawdown(&fixture, 0); + drawdown(&fixture, 25); + for _ in 0..8 { + let data = violation_data(&fixture.env, "high"); + fixture.client.attest( + &fixture.admin, + &fixture.commitment_id, + &String::from_str(&fixture.env, "violation"), + &data, + &false, + ); + } + + let metrics = fixture + .client + .get_stored_health_metrics(&fixture.commitment_id) + .unwrap(); + assert!(metrics.compliance_score <= MAX_COMPLIANCE_SCORE); + assert!(metrics.drawdown_percent >= 0); + assert!(metrics.drawdown_percent <= MAX_PERCENT); + assert_eq!(fixture.client.get_attestation_count(&fixture.commitment_id), 10); +} + +#[test] +fn maximum_valid_drawdown_is_accepted_and_preserved() { + let fixture = fixture(); + drawdown(&fixture, MAX_PERCENT); + + let metrics = fixture + .client + .get_stored_health_metrics(&fixture.commitment_id) + .unwrap(); + assert_eq!(metrics.drawdown_percent, MAX_PERCENT); + assert!(metrics.compliance_score <= MAX_COMPLIANCE_SCORE); +} + +#[test] +fn fee_accumulator_overflow_rejects_the_record_atomically() { + let fixture = fixture(); + fixture.env.as_contract(&fixture.contract_id, || { + fixture + .env + .storage() + .instance() + .set(&DataKey::TotalFees, &i128::MAX); + }); + + assert_eq!( + fixture + .client + .try_record_fees(&fixture.admin, &fixture.commitment_id, &1), + Err(Ok(AttestationError::StorageError)) + ); + assert_eq!(fixture.client.get_attestation_count(&fixture.commitment_id), 0); + fixture.env.as_contract(&fixture.contract_id, || { + assert_eq!( + fixture + .env + .storage() + .instance() + .get::(&DataKey::TotalFees), + Some(i128::MAX) + ); + }); +} + +#[test] +fn rejected_generic_fee_payload_does_not_change_metrics() { + let fixture = fixture(); + let mut data = Map::new(&fixture.env); + data.set( + String::from_str(&fixture.env, "fee_amount"), + String::from_str(&fixture.env, "-5"), + ); + + assert_eq!( + fixture.client.try_attest( + &fixture.admin, + &fixture.commitment_id, + &String::from_str(&fixture.env, "fee_generation"), + &data, + &true, + ), + Err(Ok(AttestationError::InvalidFeeAmount)) + ); + assert_eq!(fixture.client.get_attestation_count(&fixture.commitment_id), 0); +}