diff --git a/contracts/src/SortedTroves.sol b/contracts/src/SortedTroves.sol index fb3f6e371..9d979cb23 100644 --- a/contracts/src/SortedTroves.sol +++ b/contracts/src/SortedTroves.sol @@ -8,6 +8,10 @@ import "./Interfaces/IBorrowerOperations.sol"; import "./Dependencies/Ownable.sol"; import "./Dependencies/CheckContract.sol"; +// ID of head & tail of the list. Callers should stop iterating with `getNext()` / `getPrev()` +// when encountering this node ID. +uint256 constant ROOT_NODE_ID = 0; + /* * A sorted doubly linked list with nodes sorted in descending order. * @@ -35,6 +39,10 @@ import "./Dependencies/CheckContract.sol"; contract SortedTroves is Ownable, CheckContract, ISortedTroves { string constant public NAME = "SortedTroves"; + // Constants used for documentation purposes + uint256 constant UNINITIALIZED_ID = 0; + uint256 constant BAD_HINT = 0; + event TroveManagerAddressChanged(address _troveManagerAddress); event BorrowerOperationsAddressChanged(address _borrowerOperationsAddress); @@ -63,14 +71,20 @@ contract SortedTroves is Ownable, CheckContract, ISortedTroves { 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. + // nodes[ROOT_NODE_ID] 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; + constructor() { + // Technically, this is not needed as long as ROOT_NODE_ID is 0, but it doesn't hurt + nodes[ROOT_NODE_ID].nextId = ROOT_NODE_ID; + nodes[ROOT_NODE_ID].prevId = ROOT_NODE_ID; + } + // --- Dependency setters --- function setAddresses(address _troveManagerAddress, address _borrowerOperationsAddress) external override onlyOwner { @@ -116,7 +130,7 @@ contract SortedTroves is Ownable, CheckContract, ISortedTroves { function insert(uint256 _id, uint256 _annualInterestRate, uint256 _prevId, uint256 _nextId) external override { _requireCallerIsBorrowerOperations(); require(!contains(_id), "SortedTroves: List already contains the node"); - require(_id != 0, "SortedTroves: Id cannot be zero"); + require(_id != ROOT_NODE_ID, "SortedTroves: _id cannot be the root node's ID"); _insertSlice(troveManager, _id, _id, _annualInterestRate, _prevId, _nextId); nodes[_id].exists = true; @@ -186,14 +200,16 @@ contract SortedTroves is Ownable, CheckContract, ISortedTroves { 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"); + require(_troveId != ROOT_NODE_ID, "SortedTroves: _troveId cannot be the root node's ID"); + require(_batchId.isNotZero(), "SortedTroves: _batchId cannot be zero"); uint256 batchTail = batches[_batchId].tail; - if (batchTail == 0) { + if (batchTail == UNINITIALIZED_ID) { _insertSlice(troveManager, _troveId, _troveId, _annualInterestRate, _prevId, _nextId); + // Initialize the batch by setting both its head & tail to its singular node batches[_batchId].head = _troveId; + // (Tail will be set outside the "if") } else { _insertSliceIntoVerifiedPosition( _troveId, @@ -246,7 +262,7 @@ contract SortedTroves is Ownable, CheckContract, ISortedTroves { Batch memory batch = batches[_id]; _requireCallerIsBorrowerOperations(); - require(batch.head != 0, "SortedTroves: List does not contain the batch"); + require(batch.head != UNINITIALIZED_ID, "SortedTroves: List does not contain the batch"); _reInsertSlice(troveManager, batch.head, batch.tail, _newAnnualInterestRate, _prevId, _nextId); } @@ -283,14 +299,14 @@ contract SortedTroves is Ownable, CheckContract, ISortedTroves { * @dev Returns the first node in the list (node with the largest annual interest rate) */ function getFirst() external view override returns (uint256) { - return nodes[0].nextId; + return nodes[ROOT_NODE_ID].nextId; } /* * @dev Returns the last node in the list (node with the smallest annual interest rate) */ function getLast() external view override returns (uint256) { - return nodes[0].prevId; + return nodes[ROOT_NODE_ID].prevId; } /* @@ -333,8 +349,8 @@ contract SortedTroves is Ownable, CheckContract, ISortedTroves { prevBatchId.isZero() ) && // `_annualInterestRate` falls between the two nodes' interest rates - (_prevId == 0 || _troveManager.getTroveAnnualInterestRate(_prevId) >= _annualInterestRate) && - (_nextId == 0 || _annualInterestRate > _troveManager.getTroveAnnualInterestRate(_nextId)) + (_prevId == ROOT_NODE_ID || _troveManager.getTroveAnnualInterestRate(_prevId) >= _annualInterestRate) && + (_nextId == ROOT_NODE_ID || _annualInterestRate > _troveManager.getTroveAnnualInterestRate(_nextId)) ); } @@ -349,7 +365,7 @@ contract SortedTroves is Ownable, CheckContract, ISortedTroves { } function _descendOne(ITroveManager _troveManager, uint256 _annualInterestRate, Position memory _pos) internal view returns (bool found) { - if (_pos.nextId == 0 || _annualInterestRate > _troveManager.getTroveAnnualInterestRate(_pos.nextId)) { + if (_pos.nextId == ROOT_NODE_ID || _annualInterestRate > _troveManager.getTroveAnnualInterestRate(_pos.nextId)) { found = true; } else { _pos.prevId = _skipToBatchTail(_pos.nextId); @@ -358,7 +374,7 @@ contract SortedTroves is Ownable, CheckContract, ISortedTroves { } function _ascendOne(ITroveManager _troveManager, uint256 _annualInterestRate, Position memory _pos) internal view returns (bool found) { - if (_pos.prevId == 0 || _troveManager.getTroveAnnualInterestRate(_pos.prevId) >= _annualInterestRate) { + if (_pos.prevId == ROOT_NODE_ID || _troveManager.getTroveAnnualInterestRate(_pos.prevId) >= _annualInterestRate) { found = true; } else { _pos.nextId = _skipToBatchHead(_pos.prevId); @@ -428,39 +444,39 @@ contract SortedTroves is Ownable, CheckContract, ISortedTroves { // 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) { - if (_prevId == 0) { + if (_prevId == ROOT_NODE_ID) { // 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); + return _descendList(_troveManager, _annualInterestRate, ROOT_NODE_ID); } else { if (!contains(_prevId) || _troveManager.getTroveAnnualInterestRate(_prevId) < _annualInterestRate) { // `prevId` does not exist anymore or now has a smaller interest rate than the given interest rate - _prevId = 0; + _prevId = BAD_HINT; } } - if (_nextId == 0) { + if (_nextId == ROOT_NODE_ID) { // 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); + return _ascendList(_troveManager, _annualInterestRate, ROOT_NODE_ID); } 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 = BAD_HINT; } } - if (_prevId == 0 && _nextId == 0) { + if (_prevId == BAD_HINT && _nextId == BAD_HINT) { // 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) { + return _descendList(_troveManager, _annualInterestRate, ROOT_NODE_ID); + } else if (_prevId == BAD_HINT) { // No `prevId` for hint - ascend list starting from `nextId` return _ascendList(_troveManager, _annualInterestRate, _skipToBatchHead(_nextId)); - } else if (_nextId == 0) { + } else if (_nextId == BAD_HINT) { // No `nextId` for hint - descend list starting from `prevId` return _descendList(_troveManager, _annualInterestRate, _skipToBatchTail(_prevId)); } else { diff --git a/contracts/src/Types/TroveId.sol b/contracts/src/Types/TroveId.sol index 056a6deea..dbed2c1cb 100644 --- a/contracts/src/Types/TroveId.sol +++ b/contracts/src/Types/TroveId.sol @@ -2,14 +2,16 @@ pragma solidity 0.8.18; +import { ROOT_NODE_ID } from "../SortedTroves.sol"; + type TroveId is uint256; using { // TODO: use as operators if we ever upgrade to ^0.8.19 equals, // as == notEquals, // as != - isZero, - isNotZero + isEndOfList, + isNotEndOfList } for TroveId global; function equals(TroveId a, TroveId b) pure returns (bool) { @@ -20,12 +22,13 @@ 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 isEndOfList(TroveId x) pure returns (bool) { + return x.equals(TROVE_ID_END_OF_LIST); } -function isNotZero(TroveId x) pure returns (bool) { - return !x.isZero(); +function isNotEndOfList(TroveId x) pure returns (bool) { + return !x.isEndOfList(); } TroveId constant TROVE_ID_ZERO = TroveId.wrap(0); +TroveId constant TROVE_ID_END_OF_LIST = TroveId.wrap(ROOT_NODE_ID); diff --git a/contracts/src/test/SortedTroves.t.sol b/contracts/src/test/SortedTroves.t.sol index cd2a33e46..258545abd 100644 --- a/contracts/src/test/SortedTroves.t.sol +++ b/contracts/src/test/SortedTroves.t.sol @@ -267,14 +267,16 @@ contract SortedTrovesTest is Test { /// 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 + i = bound(i, 0, troveCount * 2 + 1); + + if (i == 0) { + return TROVE_ID_ZERO; + } else if (i <= troveCount) { + return tm.getTroveId(i - 1); + } else if (i <= troveCount * 2) { + return TroveId.wrap(tm._nextTroveId() + i - 1 - troveCount); // cheekily generate invalid IDs } else { - return TROVE_ID_ZERO; // zero ID can be a valid position, too + return TROVE_ID_END_OF_LIST; // head or tail can be a valid position, too } } @@ -317,10 +319,13 @@ contract SortedTrovesTest is Test { TroveId[] memory troveIds = new TroveId[](troveCount); TroveId curr = tm._sortedTroves_getFirst(); - if (curr.isZero()) { + if (curr.isEndOfList()) { 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" + tm._sortedTroves_getLast(), + TROVE_ID_END_OF_LIST, + "SortedTroves reverse node count doesn't match TroveManager" ); // empty list is ordered by definition @@ -334,7 +339,7 @@ contract SortedTrovesTest is Test { console.log(" Trove", TroveId.unwrap(curr), "annualInterestRate", prevAnnualInterestRate); curr = tm._sortedTroves_getNext(curr); - while (curr.isNotZero()) { + while (curr.isNotEndOfList()) { uint256 currAnnualInterestRate = tm.getTroveAnnualInterestRate(curr); console.log(" Trove", TroveId.unwrap(curr), "annualInterestRate", currAnnualInterestRate); assertLe(currAnnualInterestRate, prevAnnualInterestRate, "SortedTroves ordering is broken"); @@ -353,20 +358,20 @@ contract SortedTrovesTest is Test { while (i > 0) { console.log(" Trove", TroveId.unwrap(curr)); - assertNe(curr, TROVE_ID_ZERO, "SortedTroves reverse node count doesn't match TroveManager"); + assertNe(curr, TROVE_ID_END_OF_LIST, "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"); + assertEq(curr, TROVE_ID_END_OF_LIST, "SortedTroves reverse node count doesn't match TroveManager"); } function _checkBatchContiguity() internal { BatchIdSet seenBatches = new BatchIdSet(); TroveId prev = tm._sortedTroves_getFirst(); - if (prev.isZero()) { + if (prev.isEndOfList()) { return; } @@ -381,7 +386,7 @@ contract SortedTrovesTest is Test { TroveId curr = tm._sortedTroves_getNext(prev); BatchId currBatch = tm._getBatchOf(curr); - while (curr.isNotZero()) { + while (curr.isNotEndOfList()) { console.log(" ", BatchId.unwrap(currBatch)); if (currBatch.notEquals(prevBatch)) {