unit tests for writing the token point

src/escrow/increasing/LinearIncreasingEscrow.sol

@@ -155,6 +155,7 @@ contract LinearIncreasingEscrow is
         int256 const = coefficients[0];
 
         // bound the time elapsed to the maximum time
+
         uint256 MAX_TIME = _maxTime();
         timeElapsed = timeElapsed > MAX_TIME ? MAX_TIME : timeElapsed;
 
@@ -252,6 +253,9 @@ contract LinearIncreasingEscrow is
         TokenPoint memory lastPoint = _tokenPointHistory[_tokenId][interval];
 
         if (!_isWarm(lastPoint)) return 0;
+        // TODO: BUG - if you have multiple points > 0, you won't accurately sync
+        // from the start of the lock. You need to fetch escrow.locked(tokenId).start
+        // and use that as the elapsed time for the max
         uint256 timeElapsed = _t - lastPoint.checkpointTs;
 
         return _getBias(timeElapsed, lastPoint.coefficients);
@@ -283,9 +287,7 @@ contract LinearIncreasingEscrow is
         IVotingEscrow.LockedBalance memory _newLocked
     ) internal {
         if (_tokenId == 0) revert InvalidTokenId();
-        bool isIncreasing = _newLocked.amount > _oldLocked.amount && _oldLocked.amount != 0;
-        if (isIncreasing) revert IncreaseNotSupported();
-        if (_newLocked.amount == _oldLocked.amount) revert SameDepositsNotSupported();
+        if (!validateLockedBalances(_oldLocked, _newLocked)) revert("Invalid Locked Balances");
 
         // write the token checkpoint
         (TokenPoint memory oldTokenPoint, TokenPoint memory newTokenPoint) = _tokenCheckpoint(
@@ -304,91 +306,77 @@ contract LinearIncreasingEscrow is
         // this only should happen with a decrease because we don't currently support increases
         // if we wrote into the future, we don't need to update the current state as hasn't taken place yet
         if (newTokenPoint.checkpointTs == latestPoint.ts) {
-            latestPoint = _applyTokenUpdateToGlobal(
-                oldTokenPoint,
-                newTokenPoint,
-                latestPoint,
-                isIncreasing
-            );
+            latestPoint = _applyTokenUpdateToGlobal(oldTokenPoint, newTokenPoint, latestPoint);
             // write the new global point
             _writeNewGlobalPoint(latestPoint, latestIndex);
         }
     }
 
-    /// @notice Record gper-user data to checkpoints. Used by VotingEscrow system.
+    function validateLockedBalances(
+        IVotingEscrow.LockedBalance memory _oldLocked,
+        IVotingEscrow.LockedBalance memory _newLocked
+    ) public view returns (bool) {
+        if (_newLocked.amount == _oldLocked.amount) revert SameDepositsNotSupported();
+        if (_newLocked.start < _oldLocked.start) revert WriteToPastNotSupported();
+        if (_oldLocked.amount == 0 && _newLocked.amount == 0) revert ZeroDepositsNotSupported();
+        bool isIncreasing = _newLocked.amount > _oldLocked.amount && _oldLocked.amount != 0;
+        if (isIncreasing) revert IncreaseNotSupported();
+        if (_oldLocked.amount > 0 && _newLocked.start > block.timestamp) {
+            revert ScheduledAdjustmentsNotSupported();
+        }
+        if (_oldLocked.amount == 0 && _newLocked.start < block.timestamp) {
+            revert("No front running");
+        }
+        return true;
+    }
+
+    /// @notice Record per-user data to checkpoints. Used by VotingEscrow system.
     /// @dev Curve finance style but just for users at this stage
     /// @param _tokenId NFT token ID.
-    /// @param _newLocked New locked amount / end lock time for the user
+    /// @param _newLocked New locked amount / end lock time for the tokenid
     function _tokenCheckpoint(
         uint256 _tokenId,
-        IVotingEscrow.LockedBalance memory _oldLocked,
+        IVotingEscrow.LockedBalance memory /*_oldLocked */,
         IVotingEscrow.LockedBalance memory _newLocked
     ) internal returns (TokenPoint memory oldPoint, TokenPoint memory newPoint) {
-        // instantiate a new, empty token point
-        TokenPoint memory uNew;
-
-        uint amount = _newLocked.amount;
+        uint newAmount = _newLocked.amount;
+        uint newStart = _newLocked.start;
 
-        // while the escrow has no way to decrease, we should adjust this
-        bool isExiting = amount == 0;
-
-        if (!isExiting) {
-            int256[3] memory coefficients = _getCoefficients(amount);
-            // for a new lock, write the base bias (elapsed == 0)
-            uNew.coefficients = coefficients;
-            uNew.bias = _getBias(0, coefficients);
-        }
+        // in increasing curve, for new amounts we schedule the voting power
+        // to be created at the next checkpoint, this is not enforced in this function
+        // the writtenTs is used for warmups, cooldowns and for logging
+        // safe to cast as .start is 48 bit unsigned
+        newPoint.checkpointTs = uint128(newStart);
+        newPoint.writtenTs = uint128(block.timestamp);
 
-        // check to see if we have an existing interval for this token
+        // get the old point if it exists
         uint256 tokenInterval = tokenPointIntervals[_tokenId];
+        oldPoint = _tokenPointHistory[_tokenId][tokenInterval];
 
-        // if we don't have a point, we can write to the first interval
-        TokenPoint memory lastPoint;
-        if (tokenInterval == 0) {
-            tokenPointIntervals[_tokenId] = ++tokenInterval;
-        } else {
-            lastPoint = _tokenPointHistory[_tokenId][tokenInterval];
-            // can't do this: we can only write to same point or future
-            if (lastPoint.checkpointTs > uNew.checkpointTs) revert InvalidCheckpoint();
-        }
+        // we can't write checkpoints out of order as it would interfere with searching
+        if (oldPoint.checkpointTs > newPoint.checkpointTs) revert InvalidCheckpoint();
 
-        // This needs careful thought and testing
-        // we would need to evaluate the slope and bias based on the change and recompute
-        // based on the elapsed time
-        // but we need to be hyper-aware as to whether we are reducing NOW
-        // or in the futre
-        bool isReducing = !isExiting && _newLocked.amount < _oldLocked.amount;
-        if (isReducing) {
-            // our challenge here is writing a new point if the start date is in the future.
-            // say we do a reduction
-            if (_newLocked.start > block.timestamp) revert("scheduled reductions unsupported");
-
-            // get the elapsed time
-            uint48 elapsed = _newLocked.start - _oldLocked.start;
-            int256[3] memory coefficients = _getCoefficients(amount);
-            // eval the bias vs the old lock and start the new one
-            uNew.coefficients = coefficients;
-            uNew.bias = _getBias(elapsed, lastPoint.coefficients);
-        }
+        // for all locks other than amount == 0 (an exit)
+        // we need to compute the coefficients and the bias
+        if (newAmount > 0) {
+            int256[3] memory coefficients = _getCoefficients(newAmount);
 
-        // write the new timestamp - in the case of an increasing curve
-        // we align the checkpoint to the start of the upcoming deposit interval
-        // to ensure global slope changes can be scheduled
-        // safe to cast as .start is 48 bit unsigned
-        uNew.checkpointTs = uint128(_newLocked.start);
+            // If the lock hasn't started, we use the base value for the bias
+            uint elapsed = newStart >= block.timestamp ? 0 : block.timestamp - newStart;
 
-        // log the written ts - this can be used to compute warmups and burn downs
-        uNew.writtenTs = block.timestamp.toUint128();
+            newPoint.coefficients = coefficients;
+            newPoint.bias = _getBias(elapsed, coefficients);
+        }
 
         // if we're writing to a new point, increment the interval
-        if (tokenInterval != 0 && lastPoint.checkpointTs != uNew.checkpointTs) {
+        if (oldPoint.checkpointTs != newPoint.checkpointTs) {
             tokenPointIntervals[_tokenId] = ++tokenInterval;
         }
 
         // Record the new point (or overwrite the old one)
-        _tokenPointHistory[_tokenId][tokenInterval] = uNew;
+        _tokenPointHistory[_tokenId][tokenInterval] = newPoint;
 
-        return (lastPoint, uNew);
+        return (oldPoint, newPoint);
     }
 
     // there are 2 cases here:
@@ -470,6 +458,7 @@ contract LinearIncreasingEscrow is
             return _pointHistory[_latestPointIndex];
         }
     }
+
     /// @dev iterates over the interval and looks for scheduled changes that have elapsed
     ///
     function _populateHistory() internal returns (GlobalPoint memory, uint256 latestIndex) {
@@ -522,15 +511,11 @@ contract LinearIncreasingEscrow is
     function _applyTokenUpdateToGlobal(
         TokenPoint memory _oldPoint,
         TokenPoint memory _newPoint,
-        GlobalPoint memory _latestPoint,
-        bool isIncreasing
+        GlobalPoint memory _latestPoint
     ) internal view returns (GlobalPoint memory) {
         // here we are changing the voting power immediately.
         // in the schedulling function, we have already diffed the scheduled changes
 
-        // we don't support increasing
-        if (isIncreasing) revert("Increasing unsupported");
-
         // should never happen that the checkpoint is in the future
         // this should be handled by scheulling function
         if (_newPoint.checkpointTs > block.timestamp) revert("removing in the future");

src/escrow/increasing/interfaces/IEscrowCurveIncreasing.sol

@@ -69,6 +69,9 @@ interface IEscrowCurveErrorsAndEvents {
     error InvalidCheckpoint();
     error IncreaseNotSupported();
     error SameDepositsNotSupported();
+    error ScheduledAdjustmentsNotSupported();
+    error WriteToPastNotSupported();
+    error ZeroDepositsNotSupported();
 }
 
 interface IEscrowCurveIndex {
@@ -147,3 +150,9 @@ interface IEscrowCurveIncreasing is
 {}
 
 interface IEscrowCurveIncreasingGlobal is IEscrowCurveIncreasing, IEscrowCurveGlobal {}
+
+interface IEscrowCurveEventsErrorsStorage is
+    IEscrowCurveErrorsAndEvents,
+    IEscrowCurveTokenStorage,
+    IEscrowCurveGlobalStorage
+{}

test/escrow/curve/linear/LinearBase.sol

@@ -1,12 +1,16 @@
+// SPDX-License-Identifier: UNLICENSED
 pragma solidity ^0.8.17;
 
+import {console2 as console} from "forge-std/console2.sol";
 import {TestHelpers} from "@helpers/TestHelpers.sol";
 import {console2 as console} from "forge-std/console2.sol";
 import {DaoUnauthorized} from "@aragon/osx/core/utils/auth.sol";
 
 import {IDAO} from "@aragon/osx/core/dao/IDAO.sol";
 import {DAO, createTestDAO} from "@mocks/MockDAO.sol";
+
 import {LinearIncreasingEscrow, IVotingEscrow, IEscrowCurve} from "src/escrow/increasing/LinearIncreasingEscrow.sol";
+import {IEscrowCurveEventsErrorsStorage} from "src/escrow/increasing/interfaces/IEscrowCurveIncreasing.sol";
 import {Clock} from "@clock/Clock.sol";
 import {IVotingEscrowIncreasing, ILockedBalanceIncreasing} from "src/escrow/increasing/interfaces/IVotingEscrowIncreasing.sol";
 
@@ -29,23 +33,34 @@ contract MockEscrow {
     }
 }
 
-contract LinearCurveBase is TestHelpers, ILockedBalanceIncreasing {
+/// @dev expose internal functions for testing
+contract MockLinearIncreasingEscrow is LinearIncreasingEscrow {
+    function tokenCheckpoint(
+        uint256 _tokenId,
+        IVotingEscrow.LockedBalance memory _oldLocked,
+        IVotingEscrow.LockedBalance memory _newLocked
+    ) public returns (TokenPoint memory, TokenPoint memory) {
+        return _tokenCheckpoint(_tokenId, _oldLocked, _newLocked);
+    }
+}
+
+contract LinearCurveBase is TestHelpers, ILockedBalanceIncreasing, IEscrowCurveEventsErrorsStorage {
     using ProxyLib for address;
-    LinearIncreasingEscrow internal curve;
+    MockLinearIncreasingEscrow internal curve;
     MockEscrow internal escrow;
 
     function setUp() public virtual override {
         super.setUp();
         escrow = new MockEscrow();
 
-        address impl = address(new LinearIncreasingEscrow());
+        address impl = address(new MockLinearIncreasingEscrow());
 
         bytes memory initCalldata = abi.encodeCall(
             LinearIncreasingEscrow.initialize,
             (address(escrow), address(dao), 3 days, address(clock))
         );
 
-        curve = LinearIncreasingEscrow(impl.deployUUPSProxy(initCalldata));
+        curve = MockLinearIncreasingEscrow(impl.deployUUPSProxy(initCalldata));
 
         // grant this address admin privileges
         DAO(payable(address(dao))).grant({

test/escrow/curve/linear/LinearEscrow.t.sol

@@ -8,6 +8,110 @@ import {IVotingEscrowIncreasing, ILockedBalanceIncreasing} from "src/escrow/incr
 
 import {LinearCurveBase} from "./LinearBase.sol";
 contract TestLinearIncreasingCurve is LinearCurveBase {
+    /// checkpoint
+
+    // check the token id must be passed
+
+    // check we can't support and inreasing curve
+
+    // check we can't support same deposits
+
+    /// token checkpoint
+
+    // writing a new point has the correct params and adds to the correct state
+
+    // can schedule a new point to be creted
+
+    // cannot schedule a reduction (TBC)
+
+    // a reduction can be passed if at block timestamp
+
+    // can overwrite with a point made at the same time
+
+    // can write an exit point
+
+    // cannot write before the last point
+
+    /// scheduling changes
+
+    //// increase
+
+    // scheduling a first point correctly writes an increase in the bias and slope, and a decrease in the future
+
+    // the second point correctly aggregates
+
+    //// decrease
+
+    // reverts if trying to decrease when there's nothing in the old lock or if new lock > old lock
+
+    // does nothing if past the original max
+
+    // has no impact if the deposits are the same (might revert)
+
+    // if the start date has passed, we only schedule a reduction
+
+    // if the start date is in the future, we schedule a reduction and an increase
+
+    // multiple reductions aggregate correctly
+
+    // a. expired, expired
+
+    // b. expired, started
+
+    // c. started, started
+
+    // d. schedulled, started
+
+    // e. schedulled, expired
+
+    // f. schedulled, schedulled
+
+    /// fetching global point
+
+    // fetches the latest point if the index is there
+
+    // else creates a new point w. timestamp
+
+    /// Populating history
+
+    // in the case of no history, should simply return the empty point and an index of 1
+
+    // correctly writes a single backfilled point with no scheduled curve changes
+
+    // correctly writes a single backfilled point with a scheduled curve change
+
+    // works if the schedulled change is negative
+
+    // works if the schedulled change is positive
+
+    // works exactly on the interval as expected
+
+    // works a complex case of 2 weeks of history + 2 weeks of future and correctly aggregates
+
+    /// updating the global history with the token
+
+    // reverts when increasing is true (this sucks)
+
+    // cannot apply an update if the point hasn't happened yet
+
+    // the last point must be caught up
+
+    // correctly removes the tokens accrued voting power to the global point at the correct time
+
+    // if the user is reducing, this reduction is added back in
+
+    // same with the slope
+
+    /// writing the point
+
+    // overwrites the last point if the timestamp is the same
+
+    // correctly writes a new point if the timestamp is different
+
+    /// putting it together
+
+    // complex state with a couple of user points written over time - check that supply can be checked
+
     function test_votingPowerComputesCorrect() public view {
         uint256 amount = 100e18;