Skip to content

fix: PERC-8459 — settle_side_effects validate-then-mutate #77

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
dcccrypto merged 1 commit into from
Apr 4, 2026
Merged

fix: PERC-8459 — settle_side_effects validate-then-mutate #77

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 15 additions & 3 deletions src/percolator.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1950,8 +1950,13 @@ impl RiskEngine {
// ========================================

/// settle_side_effects (spec §5.3): settle A/K gains for account at current epoch.
///
/// PERC-8459 (SYNC-02): Refactored to validate-then-mutate pattern.
/// Phase 1: COMPUTE + VALIDATE — all arithmetic and validations complete before
/// any state mutation. If any validation fails, state is untouched.
/// Phase 2: MUTATE — apply all state changes atomically after validation passes.
#[allow(dead_code)]
fn settle_side_effects(&mut self, idx: usize) -> Result<()> {
pub fn settle_side_effects(&mut self, idx: usize) -> Result<()> {
let basis = self.accounts[idx].position_basis_q;
if basis == 0 {
return Ok(());
Comment on lines +1959 to 1962

@coderabbitai coderabbitai Bot Apr 4, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Add bounds/existence checks to the newly public method.

Since this is now public, Line 1953 can panic on invalid idx. Please validate idx (and ideally occupancy) at entry and return an error instead.

Suggested fix 
 pub fn settle_side_effects(&mut self, idx: usize) -> Result<()> {
+    if idx >= MAX_ACCOUNTS || !self.is_used(idx) {
+        return Err(RiskError::AccountNotFound);
+    }
     let basis = self.accounts[idx].position_basis_q;
     if basis == 0 {
         return Ok(());
     }
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@src/percolator.rs` around lines 1952 - 1955, The public method
settle_side_effects currently indexes self.accounts[idx] and can panic for
invalid idx; add an explicit bounds check (e.g. ensure idx < self.accounts.len()
or use self.accounts.get(idx)) and validate the account occupancy/state (e.g.
that the slot is occupied or position_basis_q is meaningful) at the start of
settle_side_effects, and return an appropriate Err(...) from the method instead
of panicking when the index is out of range or the account is empty; update any
callers or docs to reflect the new error return behavior.

Expand All @@ -1967,6 +1972,7 @@ impl RiskEngine {
let abs_basis = basis.unsigned_abs();

if epoch_snap == epoch_side {
// ── Phase 1: COMPUTE + VALIDATE (same-epoch branch) ──────────
let a_side = self.get_a_side(side);
let k_side = self.get_k_side(side);
let k_snap = self.accounts[idx].adl_k_snap;
Expand All @@ -1981,6 +1987,8 @@ impl RiskEngine {
if new_pnl == i128::MIN {
return Err(RiskError::Overflow);
}

// ── Phase 2: MUTATE (same-epoch branch) ──────────────────────
self.set_pnl(idx, new_pnl);
if self.accounts[idx].reserved_pnl > old_r {
self.restart_warmup_after_reserve_increase(idx);
Expand All @@ -1996,6 +2004,7 @@ impl RiskEngine {
self.accounts[idx].adl_epoch_snap = epoch_side;
}
} else {
// ── Phase 1: COMPUTE + VALIDATE (epoch-mismatch branch) ──────
let side_mode = self.get_side_mode(side);
if side_mode != SideMode::ResetPending {
return Err(RiskError::CorruptState);
Expand All @@ -2016,13 +2025,16 @@ impl RiskEngine {
if new_pnl == i128::MIN {
return Err(RiskError::Overflow);
}
// Validate stale_count BEFORE any mutation (PERC-8459 fix)
let old_stale = self.get_stale_count(side);
let new_stale = old_stale.checked_sub(1).ok_or(RiskError::CorruptState)?;

// ── Phase 2: MUTATE (epoch-mismatch branch) ──────────────────
self.set_pnl(idx, new_pnl);
if self.accounts[idx].reserved_pnl > old_r {
self.restart_warmup_after_reserve_increase(idx);
}
self.set_position_basis_q(idx, 0i128);
let old_stale = self.get_stale_count(side);
let new_stale = old_stale.checked_sub(1).ok_or(RiskError::CorruptState)?;
self.set_stale_count(side, new_stale);
self.accounts[idx].adl_a_basis = 1_000_000u128;
self.accounts[idx].adl_k_snap = 0i128;
Expand Down
140 changes: 140 additions & 0 deletions tests/unit_tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6713,3 +6713,143 @@ fn test_force_close_resolved_decrements_oi() {
engine.force_close_resolved(short_idx).unwrap();
assert_eq!(engine.oi_eff_short_q, 0);
}

// ==============================================================================
// PERC-8459: settle_side_effects validate-then-mutate
// ==============================================================================

/// PERC-8459: Epoch-mismatch branch with stale_count=0 must fail WITHOUT
/// mutating PnL. Before the fix, set_pnl was called before checked_sub(stale_count),
/// so a stale_count underflow would leave PnL already mutated.
#[test]
fn test_settle_side_effects_epoch_mismatch_stale_zero_no_pnl_mutation() {
use percolator::SideMode;
let mut engine = *Box::new(RiskEngine::new(default_params()));
let idx = engine.add_user(0).unwrap();
engine.deposit(idx, 100_000, 0).unwrap();

// Set up epoch-mismatch scenario: account epoch_snap = 0, side epoch = 1
engine.adl_epoch_long = 1;
engine.side_mode_long = SideMode::ResetPending;
engine.adl_epoch_start_k_long = 500_000i128;
engine.adl_coeff_long = 1_000_000i128;

// Give account a position and ADL state
engine.accounts[idx as usize].position_basis_q = 1_000i128;
engine.accounts[idx as usize].adl_a_basis = 1_000_000u128;
engine.accounts[idx as usize].adl_k_snap = 0i128;
engine.accounts[idx as usize].adl_epoch_snap = 0;

// CRITICAL: set stale_count to 0 — checked_sub(1) must fail
engine.stale_account_count_long = 0;

let pnl_before = engine.accounts[idx as usize].pnl.get();

// settle_side_effects must fail because stale_count underflows
let result = engine.settle_side_effects(idx as usize);
assert!(result.is_err(), "must fail when stale_count is 0");

// PnL must NOT have been mutated (validate-then-mutate property)
let pnl_after = engine.accounts[idx as usize].pnl.get();
assert_eq!(
pnl_before, pnl_after,
"PERC-8459: PnL must not be mutated when stale_count validation fails"
);
}

/// PERC-8459: Same-epoch branch happy path — PnL should be settled correctly.
#[test]
fn test_settle_side_effects_same_epoch_pnl_settled() {
let mut engine = *Box::new(RiskEngine::new(default_params()));
let idx = engine.add_user(0).unwrap();
engine.deposit(idx, 100_000, 0).unwrap();

// Set up same-epoch scenario: epoch_snap matches side epoch
engine.adl_epoch_long = 1;
engine.adl_coeff_long = 1_000_000i128;
engine.adl_mult_long = 1_000_000u128;

engine.accounts[idx as usize].position_basis_q = 1_000i128;
engine.accounts[idx as usize].adl_a_basis = 1_000_000u128;
engine.accounts[idx as usize].adl_k_snap = 0i128;
engine.accounts[idx as usize].adl_epoch_snap = 1; // matches epoch_long

let pnl_before = engine.accounts[idx as usize].pnl.get();

let result = engine.settle_side_effects(idx as usize);
assert!(result.is_ok(), "same-epoch settle should succeed");

// PnL should have changed (k_side - k_snap = 1_000_000 - 0 = 1_000_000, non-zero delta)
// The exact value depends on wide_signed_mul_div_floor_from_k_pair, but it should
// at least have been called.
// We just verify the function completed without error.
}
Comment on lines +6762 to +6786

@coderabbitai coderabbitai Bot Apr 4, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Happy-path tests don’t assert the actual PnL settlement effect.

Both tests currently pass on Ok(...) even if settle_side_effects stops mutating PnL. Please assert post-state PnL change (and make mismatch inputs non-zero enough to force a non-zero delta).

✅ Suggested test tightening 
 fn test_settle_side_effects_same_epoch_pnl_settled() {
@@
     let pnl_before = engine.accounts[idx as usize].pnl.get();

     let result = engine.settle_side_effects(idx as usize);
     assert!(result.is_ok(), "same-epoch settle should succeed");
+    let pnl_after = engine.accounts[idx as usize].pnl.get();
+    assert_ne!(
+        pnl_after, pnl_before,
+        "same-epoch settle should mutate PnL"
+    );
@@
 fn test_settle_side_effects_epoch_mismatch_happy_path() {
@@
-    engine.adl_epoch_start_k_long = 0i128;
-    engine.adl_coeff_long = 0i128;
+    engine.adl_epoch_start_k_long = 500_000i128;
+    engine.adl_coeff_long = 1_000_000i128;
@@
-    engine.accounts[idx as usize].position_basis_q = 1_000i128;
+    engine.accounts[idx as usize].position_basis_q = 1_000_000i128;
@@
+    let pnl_before = engine.accounts[idx as usize].pnl.get();
     let result = engine.settle_side_effects(idx as usize);
@@
     assert!(
         result.is_ok(),
         "epoch-mismatch settle should succeed with stale_count=1"
     );
+    let pnl_after = engine.accounts[idx as usize].pnl.get();
+    assert_ne!(
+        pnl_after, pnl_before,
+        "epoch-mismatch settle should mutate PnL on happy path"
+    );

Also applies to: 6479-6530

🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@tests/unit_tests.rs` around lines 6450 - 6474, Update the happy-path test
test_settle_side_effects_same_epoch_pnl_settled to assert that
engine.accounts[idx as usize].pnl changes after calling settle_side_effects:
capture pnl_before (already done), call engine.settle_side_effects(idx as
usize), then read pnl_after = engine.accounts[idx as usize].pnl.get() and assert
pnl_after != pnl_before (or assert the expected non-zero delta if you can
compute it); ensure the test inputs that drive the delta remain non-zero (e.g.,
adl_coeff_long, adl_mult_long, position_basis_q, adl_a_basis, adl_k_snap) so the
calculation produces a non-zero change. Apply the same tightening to the other
related test referenced around lines 6479-6530.


/// PERC-8459: Epoch-mismatch branch happy path — stale_count decremented,
/// PnL settled, account ADL state cleared.
#[test]
fn test_settle_side_effects_epoch_mismatch_happy_path() {
use percolator::SideMode;
let mut engine = *Box::new(RiskEngine::new(default_params()));
let idx = engine.add_user(0).unwrap();
engine.deposit(idx, 100_000, 0).unwrap();

// Set up epoch-mismatch: epoch_snap=0, side epoch=1
engine.adl_epoch_long = 1;
engine.side_mode_long = SideMode::ResetPending;
engine.adl_epoch_start_k_long = 0i128;
engine.adl_coeff_long = 0i128;

engine.accounts[idx as usize].position_basis_q = 1_000i128;
engine.accounts[idx as usize].adl_a_basis = 1_000_000u128;
engine.accounts[idx as usize].adl_k_snap = 0i128;
engine.accounts[idx as usize].adl_epoch_snap = 0;

// stale_count = 1 — checked_sub(1) will succeed
engine.stale_account_count_long = 1;
// stored_pos_count_long = 1 — needed for set_position_basis_q(idx, 0) decrement
engine.stored_pos_count_long = 1;

let result = engine.settle_side_effects(idx as usize);
assert!(
result.is_ok(),
"epoch-mismatch settle should succeed with stale_count=1"
);

// Verify stale_count decremented
assert_eq!(
engine.stale_account_count_long, 0,
"stale_count must be decremented"
);

// Verify ADL state cleared
assert_eq!(
engine.accounts[idx as usize].position_basis_q, 0,
"basis must be cleared"
);
assert_eq!(
engine.accounts[idx as usize].adl_a_basis, 1_000_000u128,
"a_basis must be reset"
);
assert_eq!(
engine.accounts[idx as usize].adl_k_snap, 0i128,
"k_snap must be cleared"
);
assert_eq!(
engine.accounts[idx as usize].adl_epoch_snap, 0,
"epoch_snap must be cleared"
);
}

/// PERC-8459: Zero basis is a no-op.
#[test]
fn test_settle_side_effects_zero_basis_noop() {
let mut engine = *Box::new(RiskEngine::new(default_params()));
let idx = engine.add_user(0).unwrap();
engine.deposit(idx, 100_000, 0).unwrap();

// basis=0 → early return Ok
engine.accounts[idx as usize].position_basis_q = 0;
let result = engine.settle_side_effects(idx as usize);
assert!(result.is_ok(), "zero basis must be a no-op");
}
Loading