Skip to content

Commit

Permalink
refactor: pull out magic numbers into constants
Browse files Browse the repository at this point in the history 
It should improve readability of the code.
  • Loading branch information
@danielattilasimon
danielattilasimon committed Apr 11, 2024
1 parent 1b4a575 commit 464cb76
Show file tree
Hide file tree
Showing 3 changed files with 68 additions and 44 deletions.
64 changes: 40 additions & 24 deletions contracts/src/SortedTroves.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
*
Expand Down Expand Up @@ -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);

Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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);
}
Expand Down Expand Up @@ -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;
}

/*
Expand Down Expand Up @@ -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))
);
}

Expand All @@ -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);
Expand All @@ -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);
Expand Down Expand Up @@ -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 {
Expand Down
15 changes: 9 additions & 6 deletions contracts/src/Types/TroveId.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand All @@ -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);
33 changes: 19 additions & 14 deletions contracts/src/test/SortedTroves.t.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
}

Expand Down Expand Up @@ -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
Expand All @@ -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");
Expand All @@ -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;
}

Expand All @@ -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)) {
Expand Down

0 comments on commit 464cb76

Please sign in to comment.