diff --git a/contracts/src/Interfaces/ISortedTroves.sol b/contracts/src/Interfaces/ISortedTroves.sol index f56f4be65..477f9b94c 100644 --- a/contracts/src/Interfaces/ISortedTroves.sol +++ b/contracts/src/Interfaces/ISortedTroves.sol @@ -3,44 +3,46 @@ pragma solidity 0.8.18; import "./ITroveManager.sol"; +import {BatchId, BATCH_ID_ZERO} from "../Types/BatchId.sol"; // TODO //type Id is uint256; //type Value is uint256; - -// Common interface for the SortedTroves Doubly Linked List. interface ISortedTroves { - function borrowerOperationsAddress() external view returns (address); - function troveManager() external view returns (ITroveManager); + // -- Mutating functions (permissioned) -- - function setParams(uint256 _size, address _TroveManagerAddress, address _borrowerOperationsAddress) external; + function setAddresses(address _troveManagerAddress, address _borrowerOperationsAddress) external; - function insert(uint256 _id, uint256 _value, uint256 _prevId, uint256 _nextId) external; + function insert(uint256 _id, uint256 _annualInterestRate, uint256 _prevId, uint256 _nextId) external; + function insertIntoBatch(uint256 _troveId, BatchId _batchId, uint256 _annualInterestRate, uint256 _prevId, uint256 _nextId) external; function remove(uint256 _id) external; + function removeFromBatch(uint256 _id) external; - function reInsert(uint256 _id, uint256 _newValue, uint256 _prevId, uint256 _nextId) external; + function reInsert(uint256 _id, uint256 _newAnnualInterestRate, uint256 _prevId, uint256 _nextId) external; + function reInsertBatch(BatchId _id, uint256 _newAnnualInterestRate, uint256 _prevId, uint256 _nextId) external; - function contains(uint256 _id) external view returns (bool); + // -- View functions -- - function isFull() external view returns (bool); + function contains(uint256 _id) external view returns (bool); + function isBatchedNode(uint256 _id) external view returns (bool); function isEmpty() external view returns (bool); - function getSize() external view returns (uint256); - function getMaxSize() external view returns (uint256); - function getFirst() external view returns (uint256); - function getLast() external view returns (uint256); - function getNext(uint256 _id) external view returns (uint256); - function getPrev(uint256 _id) external view returns (uint256); - function validInsertPosition(uint256 _value, uint256 _prevId, uint256 _nextId) external view returns (bool); + function validInsertPosition(uint256 _annualInterestRate, uint256 _prevId, uint256 _nextId) external view returns (bool); + function findInsertPosition(uint256 _annualInterestRate, uint256 _prevId, uint256 _nextId) external view returns (uint256, uint256); - function findInsertPosition(uint256 _value, uint256 _prevId, uint256 _nextId) external view returns (uint256, uint256); + // Public state variable getters + function borrowerOperationsAddress() external view returns (address); + function troveManager() external view returns (ITroveManager); + function size() external view returns (uint256); + function nodes(uint256 _id) external view returns (bool exists, uint256 nextId, uint256 prevId, BatchId batchId); + function batches(BatchId _id) external view returns (uint256 head, uint256 tail); } diff --git a/contracts/src/SortedTroves.sol b/contracts/src/SortedTroves.sol index eb9a99010..ffa454ab7 100644 --- a/contracts/src/SortedTroves.sol +++ b/contracts/src/SortedTroves.sol @@ -37,40 +37,46 @@ contract SortedTroves is Ownable, CheckContract, ISortedTroves { event TroveManagerAddressChanged(address _troveManagerAddress); event BorrowerOperationsAddressChanged(address _borrowerOperationsAddress); - event NodeAdded(uint256 _id, uint _annualInterestRate); - event NodeRemoved(uint256 _id); address public borrowerOperationsAddress; - ITroveManager public troveManager; // Information for a node in the list struct Node { bool exists; - uint256 nextId; // Id of next node (smaller interest rate) in the list - uint256 prevId; // Id of previous node (larger interest rate) in the list + uint256 nextId; // Id of next node (smaller interest rate) in the list + uint256 prevId; // Id of previous node (larger interest rate) in the list + BatchId batchId; // Id of this node's batch manager, or zero in case of non-batched nodes + } + + struct Batch { + uint256 head; + uint256 tail; } - // Information for the list - struct Data { - uint256 head; // Head of the list. Also the node in the list with the largest interest rate - uint256 tail; // Tail of the list. Also the node in the list with the smallest interest rate - uint256 maxSize; // Maximum size of the list - uint256 size; // Current size of the list - mapping (uint256 => Node) nodes; // Track the corresponding ids for each node in the list + struct Position { + uint256 prevId; + uint256 nextId; } - Data public data; + // Current size of the list + uint256 public size; + + // Stores the forward and reverse links of each node in the list. + // nodes[0] holds the head and tail of the list. This avoids the need for special handling + // when inserting into or removing from a terminal position (head or tail), inserting into + // an empty list or removing the element of a singleton list. + mapping (uint256 => Node) public nodes; + + // Lookup batches by the address of their manager + mapping (BatchId => Batch) public batches; // --- Dependency setters --- - function setParams(uint256 _size, address _troveManagerAddress, address _borrowerOperationsAddress) external override onlyOwner { - require(_size > 0, "SortedTroves: Size can't be zero"); + function setAddresses(address _troveManagerAddress, address _borrowerOperationsAddress) external override onlyOwner { checkContract(_troveManagerAddress); checkContract(_borrowerOperationsAddress); - data.maxSize = _size; - troveManager = ITroveManager(_troveManagerAddress); borrowerOperationsAddress = _borrowerOperationsAddress; @@ -80,179 +86,218 @@ contract SortedTroves is Ownable, CheckContract, ISortedTroves { _renounceOwnership(); } - /* - * @dev Add a node to the list - * @param _id Node's id - * @param _annualInterestRate Node's annual interest rate - * @param _prevId Id of previous node for the insert position - * @param _nextId Id of next node for the insert position - */ + // Insert an entire list slice (such as a batch of Troves sharing the same interest rate) + // between adjacent nodes `_prevId` and `_nextId`. + // Can be used to insert a single node by passing its ID as both `_sliceHead` and `_sliceTail`. + function _insertSliceIntoVerifiedPosition(uint256 _sliceHead, uint256 _sliceTail, uint256 _prevId, uint256 _nextId) internal { + nodes[_prevId].nextId = _sliceHead; + nodes[_sliceHead].prevId = _prevId; + nodes[_sliceTail].nextId = _nextId; + nodes[_nextId].prevId = _sliceTail; + } - function insert (uint256 _id, uint256 _annualInterestRate, uint256 _prevId, uint256 _nextId) external override { - ITroveManager troveManagerCached = troveManager; + function _insertSlice(ITroveManager _troveManager, uint256 _sliceHead, uint256 _sliceTail, uint256 _annualInterestRate, uint256 _prevId, uint256 _nextId) internal { + if (!_validInsertPosition(_troveManager, _annualInterestRate, _prevId, _nextId)) { + // Sender's hint was not a valid insert position + // Use sender's hint to find a valid insert position + (_prevId, _nextId) = _findInsertPosition(_troveManager, _annualInterestRate, _prevId, _nextId); + } - _requireCallerIsBOorTroveM(troveManagerCached); - _insert(troveManagerCached, _id, _annualInterestRate, _prevId, _nextId); + _insertSliceIntoVerifiedPosition(_sliceHead, _sliceTail, _prevId, _nextId); } - function _insert(ITroveManager _troveManager, uint256 _id, uint256 _annualInterestRate, uint256 _prevId, uint256 _nextId) internal { - // List must not be full - require(!isFull(), "SortedTroves: List is full"); - // List must not already contain node + /* + * @dev Add a Trove to the list + * @param _id Trove's id + * @param _annualInterestRate Trove's annual interest rate + * @param _prevId Id of previous Trove for the insert position + * @param _nextId Id of next Trove for the insert position + */ + function insert(uint256 _id, uint256 _annualInterestRate, uint256 _prevId, uint256 _nextId) external override { + _requireCallerIsBorrowerOperations(); require(!contains(_id), "SortedTroves: List already contains the node"); - // Node id must not be null require(_id != 0, "SortedTroves: Id cannot be zero"); - uint256 prevId = _prevId; - uint256 nextId = _nextId; - - if (!_validInsertPosition(_troveManager, _annualInterestRate, prevId, nextId)) { - // Sender's hint was not a valid insert position - // Use sender's hint to find a valid insert position - (prevId, nextId) = _findInsertPosition(_troveManager, _annualInterestRate, prevId, nextId); - } - - data.nodes[_id].exists = true; - - if (prevId == 0 && nextId == 0) { - // Insert as head and tail - data.head = _id; - data.tail = _id; - } else if (prevId == 0) { - // Insert before `prevId` as the head - data.nodes[_id].nextId = data.head; - data.nodes[data.head].prevId = _id; - data.head = _id; - } else if (nextId == 0) { - // Insert after `nextId` as the tail - data.nodes[_id].prevId = data.tail; - data.nodes[data.tail].nextId = _id; - data.tail = _id; - } else { - // Insert at insert position between `prevId` and `nextId` - data.nodes[_id].nextId = nextId; - data.nodes[_id].prevId = prevId; - data.nodes[prevId].nextId = _id; - data.nodes[nextId].prevId = _id; - } + _insertSlice(troveManager, _id, _id, _annualInterestRate, _prevId, _nextId); + nodes[_id].exists = true; + ++size; + } - data.size = data.size + 1; - emit NodeAdded(_id, _annualInterestRate); + // Remove the entire slice between `_sliceHead` and `_sliceTail` from the list while keeping + // the removed nodes connected to each other, such that they can be reinserted into a different + // position with `_insertSlice()`. + // Can be used to remove a single node by passing its ID as both `_sliceHead` and `_sliceTail`. + function _removeSlice(uint256 _sliceHead, uint256 _sliceTail) internal { + nodes[nodes[_sliceHead].prevId].nextId = nodes[_sliceTail].nextId; + nodes[nodes[_sliceTail].nextId].prevId = nodes[_sliceHead].prevId; } + /* + * @dev Remove a non-batched Trove from the list + * @param _id Trove's id + */ function remove(uint256 _id) external override { _requireCallerIsTroveManager(); - _remove(_id); + require(contains(_id), "SortedTroves: List does not contain the id"); + require(!isBatchedNode(_id), "SortedTroves: Must use removeFromBatch() to remove batched node"); + + _removeSlice(_id, _id); + delete nodes[_id]; + --size; } /* - * @dev Remove a node from the list - * @param _id Node's id + * @dev Re-insert a non-batched Trove at a new position, based on its new annual interest rate + * @param _id Trove's id + * @param _newAnnualInterestRate Trove's new annual interest rate + * @param _prevId Id of previous Trove for the new insert position + * @param _nextId Id of next Trove for the new insert position */ - function _remove(uint256 _id) internal { - // List must contain the node + function reInsert(uint256 _id, uint256 _newAnnualInterestRate, uint256 _prevId, uint256 _nextId) external override { + _requireCallerIsBorrowerOperations(); require(contains(_id), "SortedTroves: List does not contain the id"); + require(!isBatchedNode(_id), "SortedTroves: Must not reInsert() batched node"); - if (data.size > 1) { - // List contains more than a single node - if (_id == data.head) { - // The removed node is the head - // Set head to next node - data.head = data.nodes[_id].nextId; - // Set prev pointer of new head to null - data.nodes[data.head].prevId = 0; - } else if (_id == data.tail) { - // The removed node is the tail - // Set tail to previous node - data.tail = data.nodes[_id].prevId; - // Set next pointer of new tail to null - data.nodes[data.tail].nextId = 0; - } else { - // The removed node is neither the head nor the tail - // Set next pointer of previous node to the next node - data.nodes[data.nodes[_id].prevId].nextId = data.nodes[_id].nextId; - // Set prev pointer of next node to the previous node - data.nodes[data.nodes[_id].nextId].prevId = data.nodes[_id].prevId; - } + // The node being re-inserted can't be a valid hint, use its neighbours instead + if (_prevId == _id) { + _prevId = nodes[_id].prevId; + } + + if (_nextId == _id) { + _nextId = nodes[_id].nextId; + } + + _removeSlice(_id, _id); + _insertSlice(troveManager, _id, _id, _newAnnualInterestRate, _prevId, _nextId); + } + + /* + * @dev Add a Trove to a Batch within the list + * @param _troveId Trove's id + * @param _batchId Batch's id + * @param _annualInterestRate Batch's annual interest rate + * @param _prevId Id of previous Trove for the insert position, in case the Batch is empty + * @param _nextId Id of next Trove for the insert position, in case the Batch is empty + */ + function insertIntoBatch(uint256 _troveId, BatchId _batchId, uint256 _annualInterestRate, uint256 _prevId, uint256 _nextId) external override { + _requireCallerIsBorrowerOperations(); + require(!contains(_troveId), "SortedTroves: List already contains the node"); + require(_troveId != 0, "SortedTroves: Trove Id cannot be zero"); + require(_batchId.isNotZero(), "SortedTroves: Batch Id cannot be zero"); + + uint256 batchTail = batches[_batchId].tail; + + if (batchTail == 0) { + _insertSlice(troveManager, _troveId, _troveId, _annualInterestRate, _prevId, _nextId); + batches[_batchId].head = _troveId; } else { - // List contains a single node - // Set the head and tail to null - data.head = 0; - data.tail = 0; + _insertSliceIntoVerifiedPosition( + _troveId, + _troveId, + batchTail, + nodes[batchTail].nextId + ); } - delete data.nodes[_id]; - data.size = data.size - 1; - emit NodeRemoved(_id); + batches[_batchId].tail = _troveId; + nodes[_troveId].batchId = _batchId; + nodes[_troveId].exists = true; + ++size; } /* - * @dev Re-insert the node at a new position, based on its new annual interest rate - * @param _id Node's id - * @param _newAnnualInterestRate Node's new annual interest rate - * @param _prevId Id of previous node for the new insert position - * @param _nextId Id of next node for the new insert position + * @dev Remove a batched Trove from the list + * @param _id Trove's id */ - function reInsert(uint256 _id, uint256 _newAnnualInterestRate, uint256 _prevId, uint256 _nextId) external override { - ITroveManager troveManagerCached = troveManager; + function removeFromBatch(uint256 _id) external override { + BatchId batchId = nodes[_id].batchId; - _requireCallerIsBOorTroveM(troveManagerCached); - // List must contain the node - require(contains(_id), "SortedTroves: List does not contain the id"); + _requireCallerIsTroveManager(); + // batchId.isNotZero() implies that the list contains the node + require(batchId.isNotZero(), "SortedTroves: Must use remove() to remove non-batched node"); + + Batch memory batch = batches[batchId]; + + if (batch.head == _id && batch.tail == _id) { + // Remove singleton batch + delete batches[batchId]; + } else if (batch.head == _id) { + batches[batchId].head = nodes[_id].nextId; + } else if (batch.tail == _id) { + batches[batchId].tail = nodes[_id].prevId; + } + + _removeSlice(_id, _id); + delete nodes[_id]; + --size; + } + + /* + * @dev Re-insert an entire Batch of Troves at a new position, based on their new annual interest rate + * @param _id Batch's id + * @param _newAnnualInterestRate Trove's new annual interest rate + * @param _prevId Id of previous Trove for the new insert position + * @param _nextId Id of next Trove for the new insert position + */ + function reInsertBatch(BatchId _id, uint256 _newAnnualInterestRate, uint256 _prevId, uint256 _nextId) external override { + Batch memory batch = batches[_id]; + + _requireCallerIsBorrowerOperations(); + require(batch.head != 0, "SortedTroves: List does not contain the batch"); + + // No node within the re-inserted batch can be a valid hint, use surrounding nodes instead + if (nodes[_prevId].batchId.equals(_id)) { + _prevId = nodes[batch.head].prevId; + } - // Remove node from the list - _remove(_id); + if (nodes[_nextId].batchId.equals(_id)) { + _nextId = nodes[batch.tail].nextId; + } - _insert(troveManagerCached, _id, _newAnnualInterestRate, _prevId, _nextId); + _removeSlice(batch.head, batch.tail); + _insertSlice(troveManager, batch.head, batch.tail, _newAnnualInterestRate, _prevId, _nextId); } /* * @dev Checks if the list contains a node */ function contains(uint256 _id) public view override returns (bool) { - return data.nodes[_id].exists; + return nodes[_id].exists; } /* - * @dev Checks if the list is full + * @dev Checks whether the node is part of a batch */ - function isFull() public view override returns (bool) { - return data.size == data.maxSize; + function isBatchedNode(uint256 _id) public view override returns (bool) { + return nodes[_id].batchId.isNotZero(); } /* * @dev Checks if the list is empty */ function isEmpty() public view override returns (bool) { - return data.size == 0; + return size == 0; } /* * @dev Returns the current size of the list */ function getSize() external view override returns (uint256) { - return data.size; - } - - /* - * @dev Returns the maximum size of the list - */ - function getMaxSize() external view override returns (uint256) { - return data.maxSize; + return size; } /* * @dev Returns the first node in the list (node with the largest annual interest rate) */ function getFirst() external view override returns (uint256) { - return data.head; + return nodes[0].nextId; } /* * @dev Returns the last node in the list (node with the smallest annual interest rate) */ function getLast() external view override returns (uint256) { - return data.tail; + return nodes[0].prevId; } /* @@ -260,7 +305,7 @@ contract SortedTroves is Ownable, CheckContract, ISortedTroves { * @param _id Node's id */ function getNext(uint256 _id) external view override returns (uint256) { - return data.nodes[_id].nextId; + return nodes[_id].nextId; } /* @@ -268,7 +313,7 @@ contract SortedTroves is Ownable, CheckContract, ISortedTroves { * @param _id Node's id */ function getPrev(uint256 _id) external view override returns (uint256) { - return data.nodes[_id].prevId; + return nodes[_id].prevId; } /* @@ -282,20 +327,49 @@ contract SortedTroves is Ownable, CheckContract, ISortedTroves { } function _validInsertPosition(ITroveManager _troveManager, uint256 _annualInterestRate, uint256 _prevId, uint256 _nextId) internal view returns (bool) { - if (_prevId == 0 && _nextId == 0) { - // `(null, null)` is a valid insert position if the list is empty - return isEmpty(); - } else if (_prevId == 0) { - // `(null, _nextId)` is a valid insert position if `_nextId` is the head of the list - return data.head == _nextId && _annualInterestRate >= _troveManager.getTroveAnnualInterestRate(_nextId); - } else if (_nextId == 0) { - // `(_prevId, null)` is a valid insert position if `_prevId` is the tail of the list - return data.tail == _prevId && _annualInterestRate <= _troveManager.getTroveAnnualInterestRate(_prevId); + BatchId prevBatchId = nodes[_prevId].batchId; + + // `(_prevId, _nextId)` is a valid insert position if: + return ( + // they are adjacent nodes + nodes[_prevId].nextId == _nextId && + nodes[_nextId].prevId == _prevId && + ( + // they aren't part of the same batch + prevBatchId.notEquals(nodes[_nextId].batchId) || + prevBatchId.isZero() + ) && + // `_annualInterestRate` falls between the two nodes' interest rates + (_prevId == 0 || _troveManager.getTroveAnnualInterestRate(_prevId) >= _annualInterestRate) && + (_nextId == 0 || _annualInterestRate >= _troveManager.getTroveAnnualInterestRate(_nextId)) + ); + } + + function _skipToBatchTail(uint256 _id) internal view returns (uint256) { + BatchId batchId = nodes[_id].batchId; + return batchId.isNotZero() ? batches[batchId].tail : _id; + } + + function _skipToBatchHead(uint256 _id) internal view returns (uint256) { + BatchId batchId = nodes[_id].batchId; + return batchId.isNotZero() ? batches[batchId].head : _id; + } + + function _descendOne(ITroveManager _troveManager, uint256 _annualInterestRate, Position memory _pos) internal view returns (bool found) { + if (_pos.nextId == 0 || _annualInterestRate >= _troveManager.getTroveAnnualInterestRate(_pos.nextId)) { + found = true; } else { - // `(_prevId, _nextId)` is a valid insert position if they are adjacent nodes and `_annualInterestRate` falls between the two nodes' interest rates - return data.nodes[_prevId].nextId == _nextId && - _troveManager.getTroveAnnualInterestRate(_prevId) >= _annualInterestRate && - _annualInterestRate >= _troveManager.getTroveAnnualInterestRate(_nextId); + _pos.prevId = _skipToBatchTail(_pos.nextId); + _pos.nextId = nodes[_pos.prevId].nextId; + } + } + + function _ascendOne(ITroveManager _troveManager, uint256 _annualInterestRate, Position memory _pos) internal view returns (bool found) { + if (_pos.prevId == 0 || _troveManager.getTroveAnnualInterestRate(_pos.prevId) >= _annualInterestRate) { + found = true; + } else { + _pos.nextId = _skipToBatchHead(_pos.prevId); + _pos.prevId = nodes[_pos.nextId].prevId; } } @@ -306,21 +380,10 @@ contract SortedTroves is Ownable, CheckContract, ISortedTroves { * @param _startId Id of node to start descending the list from */ function _descendList(ITroveManager _troveManager, uint256 _annualInterestRate, uint256 _startId) internal view returns (uint256, uint256) { - // If `_startId` is the head, check if the insert position is before the head - if (data.head == _startId && _annualInterestRate >= _troveManager.getTroveAnnualInterestRate(_startId)) { - return (0, _startId); - } - - uint256 prevId = _startId; - uint256 nextId = data.nodes[prevId].nextId; - - // Descend the list until we reach the end or until we find a valid insert position - while (prevId != 0 && !_validInsertPosition(_troveManager, _annualInterestRate, prevId, nextId)) { - prevId = data.nodes[prevId].nextId; - nextId = data.nodes[prevId].nextId; - } + Position memory pos = Position(_startId, nodes[_startId].nextId); - return (prevId, nextId); + while (!_descendOne(_troveManager, _annualInterestRate, pos)) {} + return (pos.prevId, pos.nextId); } /* @@ -330,21 +393,32 @@ contract SortedTroves is Ownable, CheckContract, ISortedTroves { * @param _startId Id of node to start ascending the list from */ function _ascendList(ITroveManager _troveManager, uint256 _annualInterestRate, uint256 _startId) internal view returns (uint256, uint256) { - // If `_startId` is the tail, check if the insert position is after the tail - if (data.tail == _startId && _annualInterestRate <= _troveManager.getTroveAnnualInterestRate(_startId)) { - return (_startId, 0); - } + Position memory pos = Position(nodes[_startId].prevId, _startId); + + while (!_ascendOne(_troveManager, _annualInterestRate, pos)) {} + return (pos.prevId, pos.nextId); + } - uint256 nextId = _startId; - uint256 prevId = data.nodes[nextId].prevId; + function _descendAndAscendList( + ITroveManager _troveManager, + uint256 _annualInterestRate, + uint256 _descentStartId, + uint256 _ascentStartId + ) internal view returns (uint256, uint256) { + Position memory descentPos = Position(_descentStartId, nodes[_descentStartId].nextId); + Position memory ascentPos = Position(nodes[_ascentStartId].prevId, _ascentStartId); + + for (;;) { + if (_descendOne(_troveManager, _annualInterestRate, descentPos)) { + return (descentPos.prevId, descentPos.nextId); + } - // Ascend the list until we reach the end or until we find a valid insertion point - while (nextId != 0 && !_validInsertPosition(_troveManager, _annualInterestRate, prevId, nextId)) { - nextId = data.nodes[nextId].prevId; - prevId = data.nodes[nextId].prevId; + if (_ascendOne(_troveManager, _annualInterestRate, ascentPos)) { + return (ascentPos.prevId, ascentPos.nextId); + } } - return (prevId, nextId); + revert("Should not reach"); } /* @@ -357,36 +431,50 @@ contract SortedTroves is Ownable, CheckContract, ISortedTroves { return _findInsertPosition(troveManager, _annualInterestRate, _prevId, _nextId); } + // This function is optimized under the assumption that only one of the original neighbours has been (re)moved. + // In other words, we assume that the correct position can be found close to one of the two. + // Nevertheless, the function will always find the correct position, regardless of hints or interference. function _findInsertPosition(ITroveManager _troveManager, uint256 _annualInterestRate, uint256 _prevId, uint256 _nextId) internal view returns (uint256, uint256) { - uint256 prevId = _prevId; - uint256 nextId = _nextId; - - if (prevId != 0) { - if (!contains(prevId) || _annualInterestRate > _troveManager.getTroveAnnualInterestRate(prevId)) { + if (_prevId == 0) { + // The original correct position was found before the head of the list. + // Assuming minimal interference, the new correct position is still close to the head. + return _descendList(_troveManager, _annualInterestRate, 0); + } else { + if (!contains(_prevId) || _annualInterestRate > _troveManager.getTroveAnnualInterestRate(_prevId)) { // `prevId` does not exist anymore or now has a smaller interest rate than the given interest rate - prevId = 0; + _prevId = 0; } } - if (nextId != 0) { - if (!contains(nextId) || _annualInterestRate < _troveManager.getTroveAnnualInterestRate(nextId)) { + if (_nextId == 0) { + // The original correct position was found after the tail of the list. + // Assuming minimal interference, the new correct position is still close to the tail. + return _ascendList(_troveManager, _annualInterestRate, 0); + } else { + if (!contains(_nextId) || _annualInterestRate < _troveManager.getTroveAnnualInterestRate(_nextId)) { // `nextId` does not exist anymore or now has a larger interest rate than the given interest rate - nextId = 0; + _nextId = 0; } } - if (prevId == 0 && nextId == 0) { - // No hint - descend list starting from head - return _descendList(_troveManager, _annualInterestRate, data.head); - } else if (prevId == 0) { + if (_prevId == 0 && _nextId == 0) { + // Both original neighbours have been moved or removed. + // We default to descending the list, starting from the head. + // + // TODO: should we revert instead, so as not to waste the user's gas? + // We are unlikely to recover. + return _descendList(_troveManager, _annualInterestRate, 0); + } else if (_prevId == 0) { // No `prevId` for hint - ascend list starting from `nextId` - return _ascendList(_troveManager, _annualInterestRate, nextId); - } else if (nextId == 0) { + return _ascendList(_troveManager, _annualInterestRate, _skipToBatchHead(_nextId)); + } else if (_nextId == 0) { // No `nextId` for hint - descend list starting from `prevId` - return _descendList(_troveManager, _annualInterestRate, prevId); + return _descendList(_troveManager, _annualInterestRate, _skipToBatchTail(_prevId)); } else { - // Descend list starting from `prevId` - return _descendList(_troveManager, _annualInterestRate, prevId); + // The correct position is still somewhere between the 2 hints, so it's not obvious + // which of the 2 has been moved (assuming only one of them has been). + // We simultaneously descend & ascend in the hope that one of them is very close. + return _descendAndAscendList(_troveManager, _annualInterestRate, _skipToBatchTail(_prevId), _skipToBatchHead(_nextId)); } } @@ -396,8 +484,7 @@ contract SortedTroves is Ownable, CheckContract, ISortedTroves { require(msg.sender == address(troveManager), "SortedTroves: Caller is not the TroveManager"); } - function _requireCallerIsBOorTroveM(ITroveManager _troveManager) internal view { - require(msg.sender == borrowerOperationsAddress || msg.sender == address(_troveManager), - "SortedTroves: Caller is neither BO nor TroveM"); + function _requireCallerIsBorrowerOperations() internal view { + require(msg.sender == borrowerOperationsAddress, "SortedTroves: Caller is not BorrowerOperations"); } } diff --git a/contracts/src/Types/BatchId.sol b/contracts/src/Types/BatchId.sol new file mode 100644 index 000000000..494785537 --- /dev/null +++ b/contracts/src/Types/BatchId.sol @@ -0,0 +1,31 @@ +// SPDX-License-Identifier: MIT + +pragma solidity 0.8.18; + +type BatchId is address; + +using { + // TODO: use as operators if we ever upgrade to ^0.8.19 + equals, // as == + notEquals, // as != + isZero, + isNotZero +} for BatchId global; + +function equals(BatchId a, BatchId b) pure returns (bool) { + return BatchId.unwrap(a) == BatchId.unwrap(b); +} + +function notEquals(BatchId a, BatchId b) pure returns (bool) { + return !a.equals(b); +} + +function isZero(BatchId x) pure returns (bool) { + return x.equals(BATCH_ID_ZERO); +} + +function isNotZero(BatchId x) pure returns (bool) { + return !x.isZero(); +} + +BatchId constant BATCH_ID_ZERO = BatchId.wrap(address(0)); diff --git a/contracts/src/Types/TroveId.sol b/contracts/src/Types/TroveId.sol new file mode 100644 index 000000000..056a6deea --- /dev/null +++ b/contracts/src/Types/TroveId.sol @@ -0,0 +1,31 @@ +// SPDX-License-Identifier: MIT + +pragma solidity 0.8.18; + +type TroveId is uint256; + +using { + // TODO: use as operators if we ever upgrade to ^0.8.19 + equals, // as == + notEquals, // as != + isZero, + isNotZero +} for TroveId global; + +function equals(TroveId a, TroveId b) pure returns (bool) { + return TroveId.unwrap(a) == TroveId.unwrap(b); +} + +function notEquals(TroveId a, TroveId b) pure returns (bool) { + return !a.equals(b); +} + +function isZero(TroveId x) pure returns (bool) { + return x.equals(TROVE_ID_ZERO); +} + +function isNotZero(TroveId x) pure returns (bool) { + return !x.isZero(); +} + +TroveId constant TROVE_ID_ZERO = TroveId.wrap(0); diff --git a/contracts/src/test/SortedTroves.t.sol b/contracts/src/test/SortedTroves.t.sol new file mode 100644 index 000000000..cd2a33e46 --- /dev/null +++ b/contracts/src/test/SortedTroves.t.sol @@ -0,0 +1,575 @@ +// SPDX-License-Identifier: GPL-3.0 +pragma solidity 0.8.18; + +import "forge-std/Test.sol"; +import "../SortedTroves.sol"; +import "../Types/TroveId.sol"; + +uint256 constant FUZZ_INPUT_LENGTH = 9; + +struct Hints { + TroveId prev; + TroveId next; +} + +contract MockTroveManager { + struct Trove { + uint256 arrayIndex; + uint256 annualInterestRate; + BatchId batchId; + } + + struct Batch { + uint256 annualInterestRate; + } + + mapping(TroveId => Trove) private _troves; + mapping(BatchId => Batch) private _batches; + + TroveId[] private _troveIds; + BatchId[] private _batchIds; + + uint256 public _nextTroveId = 1; + uint160 public _nextBatchId = 1; + + SortedTroves private _sortedTroves; + + constructor(SortedTroves sortedTroves) { + _sortedTroves = sortedTroves; + } + + /// + /// Partial implementation of TroveManager interface + /// Just the parts needed by SortedTroves + /// + + function getTroveCount() external view returns (uint256) { + return _troveIds.length; + } + + function getTroveId(uint256 i) external view returns (TroveId) { + return _troveIds[i]; + } + + function getTroveAnnualInterestRate(TroveId troveId) public view returns (uint256) { + return _troves[troveId].batchId.isZero() + ? _troves[troveId].annualInterestRate + : _batches[_troves[troveId].batchId].annualInterestRate; + } + + /// + /// Mock-only functions + /// + + function _allocateTroveId() internal returns (TroveId id) { + _troveIds.push(id = TroveId.wrap(_nextTroveId++)); + } + + function _allocateBatchId() internal returns (BatchId id) { + _batchIds.push(id = BatchId.wrap(address(_nextBatchId++))); + } + + function _addIndividualTrove(uint256 annualInterestRate) external returns (TroveId id) { + _troves[id = _allocateTroveId()] = Trove(_troveIds.length, annualInterestRate, BATCH_ID_ZERO); + } + + function _addBatchedTrove(BatchId batchId) external returns (TroveId id) { + _troves[id = _allocateTroveId()] = Trove(_troveIds.length, 0, batchId); + } + + function _addBatch(uint256 annualInterestRate) external returns (BatchId id) { + _batches[id = _allocateBatchId()] = Batch(annualInterestRate); + } + + function _setTroveInterestRate(TroveId id, uint256 newAnnualInterestRate) external { + _troves[id].annualInterestRate = newAnnualInterestRate; + } + + function _setBatchInterestRate(BatchId id, uint256 newAnnualInterestRate) external { + _batches[id].annualInterestRate = newAnnualInterestRate; + } + + function _removeTrove(TroveId id) external { + TroveId poppedId = _troveIds[_troveIds.length - 1]; + _troveIds.pop(); + + if (poppedId.notEquals(id)) { + uint256 removedTroveArrayIndex = _troves[id].arrayIndex; + _troveIds[removedTroveArrayIndex] = poppedId; + _troves[poppedId].arrayIndex = removedTroveArrayIndex; + } + + delete _troves[id]; + } + + function _getBatchCount() external view returns (uint256) { + return _batchIds.length; + } + + function _getBatchId(uint256 i) external view returns (BatchId) { + return _batchIds[i]; + } + + function _getBatchOf(TroveId id) external view returns (BatchId batchId) { + return _troves[id].batchId; + } + + /// + /// Wrappers around SortedTroves + /// Needed because only TroveManager has permissions to perform every operation + /// + + function _sortedTroves_getFirst() external view returns (TroveId) { + return TroveId.wrap(_sortedTroves.getFirst()); + } + + function _sortedTroves_getLast() external view returns (TroveId) { + return TroveId.wrap(_sortedTroves.getLast()); + } + + function _sortedTroves_getNext(TroveId id) external view returns (TroveId) { + return TroveId.wrap(_sortedTroves.getNext(TroveId.unwrap(id))); + } + + function _sortedTroves_getPrev(TroveId id) external view returns (TroveId) { + return TroveId.wrap(_sortedTroves.getPrev(TroveId.unwrap(id))); + } + + function _sortedTroves_getBatchHead(BatchId id) external view returns (TroveId) { + (uint256 head,) = _sortedTroves.batches(id); + return TroveId.wrap(head); + } + + function _sortedTroves_getBatchTail(BatchId id) external view returns (TroveId) { + (, uint256 tail) = _sortedTroves.batches(id); + return TroveId.wrap(tail); + } + + function _sortedTroves_getSize() external view returns (uint256) { + return _sortedTroves.getSize(); + } + + function _sortedTroves_insert(TroveId id, uint256 annualInterestRate, Hints memory hints) external { + // console.log(); + // console.log("Insertion"); + // console.log(" id ", TroveId.unwrap(id)); + // console.log(" annualInterestRate", annualInterestRate); + // console.log(" prevId ", TroveId.unwrap(hints.prev)); + // console.log(" nextId ", TroveId.unwrap(hints.next)); + + _sortedTroves.insert( + TroveId.unwrap(id), annualInterestRate, TroveId.unwrap(hints.prev), TroveId.unwrap(hints.next) + ); + } + + function _sortedTroves_reInsert(TroveId id, uint256 newAnnualInterestRate, Hints memory hints) external { + // console.log(); + // console.log("Re-insertion"); + // console.log(" id ", TroveId.unwrap(id)); + // console.log(" annualInterestRate", newAnnualInterestRate); + // console.log(" prevId ", TroveId.unwrap(hints.prev)); + // console.log(" nextId ", TroveId.unwrap(hints.next)); + + _sortedTroves.reInsert( + TroveId.unwrap(id), newAnnualInterestRate, TroveId.unwrap(hints.prev), TroveId.unwrap(hints.next) + ); + } + + function _sortedTroves_remove(TroveId id) external { + _sortedTroves.remove(TroveId.unwrap(id)); + } + + function _sortedTroves_insertIntoBatch( + TroveId troveId, + BatchId batchId, + uint256 annualInterestRate, + Hints memory hints + ) external { + _sortedTroves.insertIntoBatch( + TroveId.unwrap(troveId), batchId, annualInterestRate, TroveId.unwrap(hints.prev), TroveId.unwrap(hints.next) + ); + } + + function _sortedTroves_reInsertBatch(BatchId batchId, uint256 newAnnualInterestRate, Hints memory hints) external { + _sortedTroves.reInsertBatch( + batchId, newAnnualInterestRate, TroveId.unwrap(hints.prev), TroveId.unwrap(hints.next) + ); + } + + function _sortedTroves_removeFromBatch(TroveId id) external { + _sortedTroves.removeFromBatch(TroveId.unwrap(id)); + } + + function _sortedTroves_findInsertPosition(uint256 annualInterestRate, Hints memory hints) + external + view + returns (Hints memory) + { + (uint256 prev, uint256 next) = + _sortedTroves.findInsertPosition(annualInterestRate, TroveId.unwrap(hints.prev), TroveId.unwrap(hints.next)); + + return Hints(TroveId.wrap(prev), TroveId.wrap(next)); + } + + function _sortedTroves_validInsertPosition(uint256 annualInterestRate, Hints memory hints) + external + view + returns (bool) + { + return _sortedTroves.validInsertPosition( + annualInterestRate, TroveId.unwrap(hints.prev), TroveId.unwrap(hints.next) + ); + } +} + +contract BatchIdSet { + mapping(BatchId => bool) public has; + + function add(BatchId id) external { + has[id] = true; + } +} + +contract SortedTrovesTest is Test { + enum ArbRole { + Individual, + BatchStarter, + BatchJoiner + } + + struct ArbHints { + uint256 prev; + uint256 next; + } + + struct ArbIndividualTroveCreation { + uint256 annualInterestRate; + ArbHints hints; + } + + struct ArbBatchedTroveCreation { + uint256 annualInterestRate; + ArbHints hints; + uint256 role; + uint256 batch; + } + + struct ArbReInsertion { + uint256 trove; + uint256 newAnnualInterestRate; + ArbHints hints; + } + + MockTroveManager tm; + + /// + /// Bounding fuzzy inputs + /// + + function _pickHint(uint256 troveCount, uint256 i) internal view returns (TroveId) { + i = bound(i, 0, troveCount * 2); + + if (i < troveCount) { + return tm.getTroveId(i); + } else if (i < troveCount * 2) { + return TroveId.wrap(tm._nextTroveId() + i - troveCount); // cheekily generate invalid IDs + } else { + return TROVE_ID_ZERO; // zero ID can be a valid position, too + } + } + + function _pickHints(ArbHints calldata hints) internal view returns (Hints memory) { + uint256 troveCount = tm.getTroveCount(); + return Hints(_pickHint(troveCount, hints.prev), _pickHint(troveCount, hints.next)); + } + + function _pickTrove(uint256 trove) internal view returns (TroveId) { + return tm.getTroveId(bound(trove, 0, tm.getTroveCount() - 1)); + } + + function _pickBatch(uint256 batch) internal view returns (BatchId) { + return tm._getBatchId(bound(batch, 0, tm._getBatchCount() - 1)); + } + + function _pickRole(uint256 role) internal pure returns (ArbRole) { + return ArbRole(bound(role, uint256(type(ArbRole).min), uint256(type(ArbRole).max))); + } + + /// + /// Custom assertions + /// + + function assertEq(TroveId a, TroveId b, string memory err) internal { + assertEq(TroveId.unwrap(a), TroveId.unwrap(b), err); + } + + function assertNe(TroveId a, TroveId b, string memory err) internal { + assertTrue(a.notEquals(b), err); + } + + /// + /// Invariant checks + /// + + function _checkOrdering() internal { + uint256 i = 0; + uint256 troveCount = tm.getTroveCount(); + TroveId[] memory troveIds = new TroveId[](troveCount); + TroveId curr = tm._sortedTroves_getFirst(); + + if (curr.isZero()) { + assertEq(tm.getTroveCount(), 0, "SortedTroves forward node count doesn't match TroveManager"); + assertEq( + tm._sortedTroves_getLast(), TROVE_ID_ZERO, "SortedTroves reverse node count doesn't match TroveManager" + ); + + // empty list is ordered by definition + return; + } + + troveIds[i++] = curr; + uint256 prevAnnualInterestRate = tm.getTroveAnnualInterestRate(curr); + console.log(); + console.log("Forward list:"); + console.log(" Trove", TroveId.unwrap(curr), "annualInterestRate", prevAnnualInterestRate); + curr = tm._sortedTroves_getNext(curr); + + while (curr.isNotZero()) { + uint256 currAnnualInterestRate = tm.getTroveAnnualInterestRate(curr); + console.log(" Trove", TroveId.unwrap(curr), "annualInterestRate", currAnnualInterestRate); + assertLe(currAnnualInterestRate, prevAnnualInterestRate, "SortedTroves ordering is broken"); + + troveIds[i++] = curr; + prevAnnualInterestRate = currAnnualInterestRate; + curr = tm._sortedTroves_getNext(curr); + } + + assertEq(i, tm.getTroveCount(), "SortedTroves forward node count doesn't match TroveManager"); + + // Verify reverse ordering + console.log(); + console.log("Reverse list:"); + curr = tm._sortedTroves_getLast(); + + while (i > 0) { + console.log(" Trove", TroveId.unwrap(curr)); + assertNe(curr, TROVE_ID_ZERO, "SortedTroves reverse node count doesn't match TroveManager"); + assertEq(curr, troveIds[--i], "SortedTroves reverse ordering is broken"); + curr = tm._sortedTroves_getPrev(curr); + } + + console.log(); + assertEq(curr, TROVE_ID_ZERO, "SortedTroves reverse node count doesn't match TroveManager"); + } + + function _checkBatchContiguity() internal { + BatchIdSet seenBatches = new BatchIdSet(); + TroveId prev = tm._sortedTroves_getFirst(); + + if (prev.isZero()) { + return; + } + + BatchId prevBatch = tm._getBatchOf(prev); + console.log("Batch IDs:"); + console.log(" ", BatchId.unwrap(prevBatch)); + + if (prevBatch.isNotZero()) { + assertEq(prev, tm._sortedTroves_getBatchHead(prevBatch), "Wrong batch head"); + } + + TroveId curr = tm._sortedTroves_getNext(prev); + BatchId currBatch = tm._getBatchOf(curr); + + while (curr.isNotZero()) { + console.log(" ", BatchId.unwrap(currBatch)); + + if (currBatch.notEquals(prevBatch)) { + if (prevBatch.isNotZero()) { + assertFalse(seenBatches.has(prevBatch), "Batch already seen"); + seenBatches.add(prevBatch); + assertEq(prev, tm._sortedTroves_getBatchTail(prevBatch), "Wrong batch tail"); + } + + if (currBatch.isNotZero()) { + assertEq(curr, tm._sortedTroves_getBatchHead(currBatch), "Wrong batch head"); + } + } + + prev = curr; + prevBatch = currBatch; + + curr = tm._sortedTroves_getNext(prev); + currBatch = tm._getBatchOf(curr); + } + + if (prevBatch.isNotZero()) { + assertFalse(seenBatches.has(prevBatch), "Batch already seen"); + assertEq(prev, tm._sortedTroves_getBatchTail(prevBatch), "Wrong batch tail"); + } + + console.log(); + } + + /// + /// Helpers for test case setup + /// + + function _buildList(ArbIndividualTroveCreation[FUZZ_INPUT_LENGTH] calldata troves) internal { + for (uint256 i = 0; i < troves.length; ++i) { + tm._sortedTroves_insert( + tm._addIndividualTrove(troves[i].annualInterestRate), + troves[i].annualInterestRate, + _pickHints(troves[i].hints) + ); + } + } + + function _buildBatchedList(ArbBatchedTroveCreation[FUZZ_INPUT_LENGTH] calldata troves) internal { + for (uint256 i = 0; i < troves.length; ++i) { + ArbRole role = _pickRole(troves[i].role); + + if (role == ArbRole.BatchJoiner && tm._getBatchCount() == 0) { + // No batches to join yet; promote to batch starter + role = ArbRole.BatchStarter; + } + + if (role == ArbRole.Individual) { + tm._sortedTroves_insert( + tm._addIndividualTrove(troves[i].annualInterestRate), + troves[i].annualInterestRate, + _pickHints(troves[i].hints) + ); + } else if (role == ArbRole.BatchStarter) { + BatchId batchId = tm._addBatch(troves[i].annualInterestRate); + tm._sortedTroves_insertIntoBatch( + tm._addBatchedTrove(batchId), batchId, troves[i].annualInterestRate, _pickHints(troves[i].hints) + ); + } else if (role == ArbRole.BatchJoiner) { + BatchId batchId = _pickBatch(troves[i].batch); + TroveId troveId = tm._addBatchedTrove(batchId); + tm._sortedTroves_insertIntoBatch( + troveId, batchId, tm.getTroveAnnualInterestRate(troveId), _pickHints(troves[i].hints) + ); + } else { + revert("Role not considered"); + } + } + } + + //////////////// + // Test cases // + //////////////// + + function setUp() public { + SortedTroves sortedTroves = new SortedTroves(); + tm = new MockTroveManager(sortedTroves); + // We're cheating here and using MockTroveManager as BorrowerOperations too, + // to grant us access to those functions that only BO can call + sortedTroves.setAddresses(address(tm), address(tm)); + } + + function test_SortsIndividualTrovesByAnnualInterestRate( + ArbIndividualTroveCreation[FUZZ_INPUT_LENGTH] calldata troves + ) public { + _buildList(troves); + _checkOrdering(); + } + + function test_SortsBatchedTrovesByAnnualInterestRate(ArbBatchedTroveCreation[FUZZ_INPUT_LENGTH] calldata troves) + public + { + _buildBatchedList(troves); + _checkOrdering(); + _checkBatchContiguity(); + } + + function test_FindsValidInsertPosition( + ArbBatchedTroveCreation[FUZZ_INPUT_LENGTH] calldata troves, + ArbIndividualTroveCreation calldata inserted + ) public { + _buildBatchedList(troves); + + assertTrue( + tm._sortedTroves_validInsertPosition( + inserted.annualInterestRate, + tm._sortedTroves_findInsertPosition(inserted.annualInterestRate, _pickHints(inserted.hints)) + ), + "Invalid insert position found" + ); + } + + function test_CanRemoveIndividualTroves( + ArbIndividualTroveCreation[FUZZ_INPUT_LENGTH] calldata troves, + uint256[FUZZ_INPUT_LENGTH] calldata removedTroves, + uint256 numTrovesToRemove + ) public { + numTrovesToRemove = bound(numTrovesToRemove, 1, troves.length); + + _buildList(troves); + assertEq(tm._sortedTroves_getSize(), troves.length); + + for (uint256 i = 0; i < numTrovesToRemove; ++i) { + TroveId id = _pickTrove(removedTroves[i]); + tm._removeTrove(id); + tm._sortedTroves_remove(id); + } + + assertEq(tm._sortedTroves_getSize(), troves.length - numTrovesToRemove); + _checkOrdering(); + } + + function test_CanRemoveBatchedTroves( + ArbBatchedTroveCreation[FUZZ_INPUT_LENGTH] calldata troves, + uint256[FUZZ_INPUT_LENGTH] calldata removedTroves, + uint256 numTrovesToRemove + ) public { + numTrovesToRemove = bound(numTrovesToRemove, 1, troves.length); + + _buildBatchedList(troves); + assertEq(tm._sortedTroves_getSize(), troves.length); + + for (uint256 i = 0; i < numTrovesToRemove; ++i) { + TroveId id = _pickTrove(removedTroves[i]); + bool batchedTrove = tm._getBatchOf(id).isNotZero(); + tm._removeTrove(id); + + if (batchedTrove) { + tm._sortedTroves_removeFromBatch(id); + } else { + tm._sortedTroves_remove(id); + } + } + + assertEq(tm._sortedTroves_getSize(), troves.length - numTrovesToRemove); + _checkOrdering(); + _checkBatchContiguity(); + } + + function test_CanReInsert( + ArbBatchedTroveCreation[FUZZ_INPUT_LENGTH] calldata troves, + ArbReInsertion[FUZZ_INPUT_LENGTH] calldata reInsertions + ) public { + _buildBatchedList(troves); + + for (uint256 i = 0; i < reInsertions.length; ++i) { + TroveId troveId = _pickTrove(reInsertions[i].trove); + BatchId batchId = tm._getBatchOf(troveId); + + if (batchId.isNotZero()) { + tm._sortedTroves_reInsertBatch( + batchId, reInsertions[i].newAnnualInterestRate, _pickHints(reInsertions[i].hints) + ); + tm._setBatchInterestRate(batchId, reInsertions[i].newAnnualInterestRate); + } else { + tm._sortedTroves_reInsert( + troveId, reInsertions[i].newAnnualInterestRate, _pickHints(reInsertions[i].hints) + ); + tm._setTroveInterestRate(troveId, reInsertions[i].newAnnualInterestRate); + } + } + + _checkOrdering(); + _checkBatchContiguity(); + } +} diff --git a/contracts/src/test/TestContracts/DevTestSetup.sol b/contracts/src/test/TestContracts/DevTestSetup.sol index 2bd6f48cc..828159f7e 100644 --- a/contracts/src/test/TestContracts/DevTestSetup.sol +++ b/contracts/src/test/TestContracts/DevTestSetup.sol @@ -69,8 +69,7 @@ contract DevTestSetup is BaseTest { mockInterestRouter = new MockInterestRouter(); // Connect contracts - sortedTroves.setParams( - MAX_UINT256, + sortedTroves.setAddresses( address(troveManager), address(borrowerOperations) ); diff --git a/contracts/test/AccessControlTest.js b/contracts/test/AccessControlTest.js index 4e59f5a07..5f58bc5fb 100644 --- a/contracts/test/AccessControlTest.js +++ b/contracts/test/AccessControlTest.js @@ -402,7 +402,7 @@ contract( ); } catch (err) { assert.include(err.message, "revert"); - assert.include(err.message, " Caller is neither BO nor TroveM"); + assert.include(err.message, " Caller is not BorrowerOperations"); } }); @@ -432,7 +432,7 @@ contract( ); } catch (err) { assert.include(err.message, "revert"); - assert.include(err.message, "Caller is neither BO nor TroveM"); + assert.include(err.message, "Caller is not BorrowerOperations"); } }); }); diff --git a/contracts/test/OwnershipTest.js b/contracts/test/OwnershipTest.js index a5dadc524..139ca3173 100644 --- a/contracts/test/OwnershipTest.js +++ b/contracts/test/OwnershipTest.js @@ -97,24 +97,24 @@ contract('All Liquity functions with onlyOwner modifier', async accounts => { }) describe('SortedTroves', async accounts => { - it("setParams(): reverts when called by non-owner, with wrong addresses, or twice", async () => { + it("setAddresses(): reverts when called by non-owner, with wrong addresses, or twice", async () => { const dumbContract = await GasPool.new() - const params = [10000001, dumbContract.address, dumbContract.address] + const params = [dumbContract.address, dumbContract.address] // Attempt call from alice - await th.assertRevert(sortedTroves.setParams(...params, { from: alice })) + await th.assertRevert(sortedTroves.setAddresses(...params, { from: alice })) // Attempt to use zero address - await testZeroAddress(sortedTroves, params, 'setParams', 1) + await testZeroAddress(sortedTroves, params, 'setAddresses', 1) // Attempt to use non contract - await testNonContractAddress(sortedTroves, params, 'setParams', 1) + await testNonContractAddress(sortedTroves, params, 'setAddresses', 1) // Owner can successfully set params - const txOwner = await sortedTroves.setParams(...params, { from: owner }) + const txOwner = await sortedTroves.setAddresses(...params, { from: owner }) assert.isTrue(txOwner.receipt.status) // fails if called twice - await th.assertRevert(sortedTroves.setParams(...params, { from: owner })) + await th.assertRevert(sortedTroves.setAddresses(...params, { from: owner })) }) }) }) diff --git a/contracts/test/SortedTrovesTest.js b/contracts/test/SortedTrovesTest.js index edb68d3e5..8569a74c6 100644 --- a/contracts/test/SortedTrovesTest.js +++ b/contracts/test/SortedTrovesTest.js @@ -295,13 +295,6 @@ contract("SortedTroves", async (accounts) => { assert.isFalse(await sortedTroves.contains(th.addressToTroveId(bob))); }); - // --- getMaxSize --- - - it("getMaxSize(): Returns the maximum list size", async () => { - const max = await sortedTroves.getMaxSize(); - assert.equal(web3.utils.toHex(max), th.maxBytes32); - }); - // --- findInsertPosition --- it("Finds the correct insert position given two addresses that loosely bound the correct position", async () => { @@ -348,38 +341,14 @@ contract("SortedTroves", async (accounts) => { await sortedTrovesTester.setSortedTroves(sortedTroves.address); }); - context("when params are wrongly set", () => { - it("setParams(): reverts if size is zero", async () => { - await th.assertRevert( - sortedTroves.setParams( - 0, - // The SortedTrovesTester is being used here as both a wrapper for SortedTroves and a mock TroveManager. - sortedTrovesTester.address, - sortedTrovesTester.address - ), - "SortedTroves: Size can’t be zero" - ); - }); - }); - context("when params are properly set", () => { - beforeEach("set params", async () => { - await sortedTroves.setParams( - 2, + beforeEach("set addresses", async () => { + await sortedTroves.setAddresses( sortedTrovesTester.address, sortedTrovesTester.address ); }); - it("insert(): fails if list is full", async () => { - await sortedTrovesTester.insert(alice, 1, alice, alice); - await sortedTrovesTester.insert(bob, 1, alice, alice); - await th.assertRevert( - sortedTrovesTester.insert(carol, 1, alice, alice), - "SortedTroves: List is full" - ); - }); - it("insert(): fails if list already contains the node", async () => { await sortedTrovesTester.insert(alice, 1, alice, alice); await th.assertRevert( @@ -409,16 +378,21 @@ contract("SortedTroves", async (accounts) => { ); }); - it("findInsertPosition(): No prevId for hint - ascend list starting from nextId, result is after the tail", async () => { - await sortedTrovesTester.insert(th.addressToTroveId(alice), 1, th.addressToTroveId(alice), th.addressToTroveId(alice)); - const pos = await sortedTroves.findInsertPosition( - 1, - toBN(0), - th.addressToTroveId(alice) - ); - assert.isTrue(pos[0].eq(toBN(th.addressToTroveId(alice))), "prevId result should be nextId param"); - assert.isTrue(pos[1].eq(toBN(0)), "nextId result should be zero"); - }); + // danielattilasimon: I believe this test was reinforcing questionable behavior. + // The initial position (0, alice) _is_ already a valid insert position for the given list + // (which happens to contain only alice), so why are we expecting findInsertPosition() to + // move away from such a position? + // + // it("findInsertPosition(): No prevId for hint - ascend list starting from nextId, result is after the tail", async () => { + // await sortedTrovesTester.insert(th.addressToTroveId(alice), 1, th.addressToTroveId(alice), th.addressToTroveId(alice)); + // const pos = await sortedTroves.findInsertPosition( + // 1, + // toBN(0), + // th.addressToTroveId(alice) + // ); + // assert.isTrue(pos[0].eq(toBN(th.addressToTroveId(alice))), "prevId result should be nextId param"); + // assert.isTrue(pos[1].eq(toBN(0)), "nextId result should be zero"); + // }); }); }); }); diff --git a/contracts/utils/deploymentHelpers.js b/contracts/utils/deploymentHelpers.js index 6ee7e8cfe..4b10454cc 100644 --- a/contracts/utils/deploymentHelpers.js +++ b/contracts/utils/deploymentHelpers.js @@ -128,8 +128,7 @@ class DeploymentHelper { contracts.priceFeedTestnet.address ); // set TroveManager addr in SortedTroves - await contracts.sortedTroves.setParams( - maxBytes32, + await contracts.sortedTroves.setAddresses( contracts.troveManager.address, contracts.borrowerOperations.address );