refactor: streamline fee validation methods and enhance reward scenario tests

Author: SidestreamSweatyPumpkin

src/test/BondingManagerRetroactiveRewardCalculationFix.sol

@@ -27,98 +27,22 @@ contract BondingManagerRetroactiveRewardCalculationFix is BondingManagerRetroact
         assertEq(infoAddr, address(newBondingManagerTarget));
     }
 
-    function _delta(uint256 a, uint256 b) internal pure returns (int256) {
-        return a >= b ? int256(a - b) : -int256(b - a);
-    }
-
-    function _abs(int256 x) internal pure returns (uint256) {
-        return uint256(x >= 0 ? x : -x);
-    }
-
-    function testFeeFactorEqualityAfterUpgrade() public {
-        // Pre-upgrade measurement
-        CHEATS.revertToState(baselineSnapshot);
-        FeeMetrics memory baselineConsecutive = _scenarioConsecutive();
-        CHEATS.revertToState(baselineSnapshot);
-        FeeMetrics memory baselineMissed = _scenarioMissed();
-        int256 preUpgradeDelta = _delta(
-            baselineMissed.cumulativeFeeFactorInc,
-            baselineConsecutive.cumulativeFeeFactorInc
-        );
-
-        // Post-upgrade measurement
-        CHEATS.revertToState(baselineSnapshot);
-        _deployUpgrade();
-        FeeMetrics memory postConsecutive = _scenarioConsecutive();
-        CHEATS.revertToState(baselineSnapshot);
+    function _scenarioConsecutive() internal override returns (FeeMetrics memory) {
         _deployUpgrade();
-        FeeMetrics memory postMissed = _scenarioMissed();
-        int256 postUpgradeDelta = _delta(postMissed.cumulativeFeeFactorInc, postConsecutive.cumulativeFeeFactorInc);
-
-        // Sanity: identical commission parameters preserved
-        assertEq(postConsecutive.feeShare, postMissed.feeShare, "feeShare mismatch");
-        assertEq(postConsecutive.commission, postMissed.commission, "commission mismatch");
-
-        // Post-upgrade delta less than 1% of pre-upgrade delta
-        uint256 deltaTolerance = _abs(preUpgradeDelta) / 100000;
-        if (deltaTolerance == 0) deltaTolerance = 1;
-
-        assertTrue(_abs(postUpgradeDelta) <= deltaTolerance, "fee factor deflation persists");
-
-        emit log_named_uint("preUpgrade_cumulativeFeeFactor_missed", baselineMissed.cumulativeFeeFactor);
-        emit log_named_uint("preUpgrade_cumulativeFeeFactor_consec", baselineConsecutive.cumulativeFeeFactor);
-        emit log_named_uint("aftUpgrade_cumulativeFeeFactor_missed", postMissed.cumulativeFeeFactor);
-        emit log_named_uint("aftUpgrade_cumulativeFeeFactor_consec", postConsecutive.cumulativeFeeFactor);
-        emit log_named_int("preUpgrade_cumulativeFeeFactor_delta", preUpgradeDelta);
-        emit log_named_int("aftUpgrade_cumulativeFeeFactor_delta", postUpgradeDelta);
+        return super._scenarioConsecutive();
     }
 
-    function testDelegatorNoFeeLossAfterUpgrade() public {
-        // Pre-upgrade measurement
-        CHEATS.revertToState(baselineSnapshot);
-        _bondDelegator(testDelegator, 1 ether);
-        _scenarioConsecutive();
-        CHEATS.prank(testDelegator);
-        BONDING_MANAGER.claimEarnings(type(uint256).max);
-        (, uint256 baseFeesCons, , , , , ) = BONDING_MANAGER.getDelegator(testDelegator);
-
-        CHEATS.revertToState(baselineSnapshot);
-        _bondDelegator(testDelegator, 1 ether);
-        _scenarioMissed();
-        CHEATS.prank(testDelegator);
-        BONDING_MANAGER.claimEarnings(type(uint256).max);
-        (, uint256 baseFeesMiss, , , , , ) = BONDING_MANAGER.getDelegator(testDelegator);
-        int256 preUpgradeDeltaFees = _delta(baseFeesMiss, baseFeesCons);
-
-        // Post-upgrade measurement
-        CHEATS.revertToState(baselineSnapshot);
+    function _scenarioMissed() internal override returns (FeeMetrics memory) {
         _deployUpgrade();
-        _bondDelegator(testDelegator, 1 ether);
-        _scenarioConsecutive();
-        CHEATS.prank(testDelegator);
-        BONDING_MANAGER.claimEarnings(type(uint256).max);
-        (, uint256 postFeesCons, , , , , ) = BONDING_MANAGER.getDelegator(testDelegator);
-
-        CHEATS.revertToState(baselineSnapshot);
-        _deployUpgrade();
-        _bondDelegator(testDelegator, 1 ether);
-        _scenarioMissed();
-        CHEATS.prank(testDelegator);
-        BONDING_MANAGER.claimEarnings(type(uint256).max);
-        (, uint256 postFeesMiss, , , , , ) = BONDING_MANAGER.getDelegator(testDelegator);
-        int256 postUpgradeDeltaFees = _delta(postFeesMiss, postFeesCons);
-
-        // Post-upgrade delta less than 1% of pre-upgrade delta
-        uint256 deltaToleranceFees = _abs(preUpgradeDeltaFees) / 100000;
-        if (deltaToleranceFees == 0) deltaToleranceFees = 1;
+        return super._scenarioMissed();
+    }
 
-        assertTrue(_abs(postUpgradeDeltaFees) <= deltaToleranceFees, "delegator fee loss not mitigated sufficiently");
+    function _validateFeeFactor(uint256 consecutive, uint256 missed) internal override {
+        // We use 1e12 as a safe tolerance for rounding noise
+        assertApproxEqAbs(consecutive, missed, 1e12, "Fee factor deflation persists");
+    }
 
-        emit log_named_uint("preUpgrade_delegatorFees_consec", baseFeesCons);
-        emit log_named_uint("preUpgrade_delegatorFees_missed", baseFeesMiss);
-        emit log_named_uint("aftUpgrade_delegatorFees_consec", postFeesCons);
-        emit log_named_uint("aftUpgrade_delegatorFees_missed", postFeesMiss);
-        emit log_named_int("preUpgrade_delegatorFees_delta", preUpgradeDeltaFees);
-        emit log_named_int("aftUpgrade_delegatorFees_delta", postUpgradeDeltaFees);
+    function _validateDelegatorFees(uint256 consecutive, uint256 missed) internal override {
+        assertEq(consecutive, missed, "Delegator fee loss persists");
     }
 }

src/test/BondingManagerRetroactiveRewardCalculationPoC.sol

@@ -58,7 +58,7 @@ contract BondingManagerRetroactiveRewardCalculationPoC is GovernorBaseTest {
         uint256 rewardFactorBefore; // cumulativeRewardFactor Previous round
     }
 
-    function _scenarioConsecutive() internal returns (FeeMetrics memory m) {
+    function _scenarioConsecutive() internal virtual returns (FeeMetrics memory m) {
         _nextRound(); // R1, normal reward claim
         CHEATS.prank(transcoder);
         BONDING_MANAGER.reward();
@@ -96,7 +96,7 @@ contract BondingManagerRetroactiveRewardCalculationPoC is GovernorBaseTest {
         m.rewardFactorBefore = rewardFactorBefore;
     }
 
-    function _scenarioMissed() internal returns (FeeMetrics memory m) {
+    function _scenarioMissed() internal virtual returns (FeeMetrics memory m) {
         _nextRound(); // R1, normal reward claim
         CHEATS.prank(transcoder);
         BONDING_MANAGER.reward();
@@ -135,7 +135,18 @@ contract BondingManagerRetroactiveRewardCalculationPoC is GovernorBaseTest {
         m.rewardFactorBefore = rewardFactorBefore;
     }
 
-    function testCompareConsecutiveAndMissedRewardClaims() public {
+    function _validateFeeFactor(uint256 consecutive, uint256 missed) internal virtual {
+        // Assert the bug exists (deflation is huge > 1e16)
+        uint256 deflation = consecutive - missed;
+        assertGt(deflation, 1e16, "Large fee deflation NOT detected");
+    }
+
+    function _validateDelegatorFees(uint256 consecutive, uint256 missed) internal virtual {
+        // Assert delegator fee loss exists
+        assertNotEq(consecutive, missed, "Delegator fee loss NOT detected");
+    }
+
+    function testCompareConsecutiveAndMissedRewardClaims() public virtual {
         FeeMetrics memory consecutive = _scenarioConsecutive();
         CHEATS.revertToState(baselineSnapshot);
         FeeMetrics memory missed = _scenarioMissed();
@@ -145,7 +156,7 @@ contract BondingManagerRetroactiveRewardCalculationPoC is GovernorBaseTest {
         assertEq(consecutive.commission, missed.commission, "commission mismatch");
 
         // Missed reward path deflates fee factor increment for identical fee update
-        assertLt(missed.cumulativeFeeFactorInc, consecutive.cumulativeFeeFactorInc, "fee not deflated");
+        _validateFeeFactor(consecutive.cumulativeFeeFactorInc, missed.cumulativeFeeFactorInc);
     }
 
     function _bondDelegator(address delegator, uint256 amount) internal {
@@ -158,22 +169,21 @@ contract BondingManagerRetroactiveRewardCalculationPoC is GovernorBaseTest {
         CHEATS.stopPrank();
     }
 
-    function testDelegatorFeeLossOnMissedReward() public {
+    function testDelegatorFeeLossOnMissedReward() public virtual {
         _bondDelegator(testDelegator, 1 ether);
-        FeeMetrics memory cons = _scenarioConsecutive();
+        _scenarioConsecutive();
         CHEATS.prank(testDelegator);
         BONDING_MANAGER.claimEarnings(type(uint256).max);
         (, uint256 feesAfterCons, , , , , ) = BONDING_MANAGER.getDelegator(testDelegator);
 
         CHEATS.revertToState(baselineSnapshot);
 
         _bondDelegator(testDelegator, 1 ether);
-        FeeMetrics memory miss = _scenarioMissed();
+        _scenarioMissed();
         CHEATS.prank(testDelegator);
         BONDING_MANAGER.claimEarnings(type(uint256).max);
         (, uint256 feesAfterMiss, , , , , ) = BONDING_MANAGER.getDelegator(testDelegator);
 
-        assertLt(miss.cumulativeFeeFactorInc, cons.cumulativeFeeFactorInc, "fees not deflated");
-        assertLt(feesAfterMiss, feesAfterCons, "delegator not losing fees");
+        _validateDelegatorFees(feesAfterCons, feesAfterMiss);
     }
 }