From 8a4e8fffb1d3faf775d9b27e9a88a5e6eeca0bf2 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?=C3=9Fingen?= <bingen@liquity.org>
Date: Thu, 25 Apr 2024 16:42:35 +0100
Subject: [PATCH] fix: Optimize calls to TroveManager on trove open

---
 contracts/src/BorrowerOperations.sol       |  3 +--
 contracts/src/Interfaces/ITroveManager.sol |  4 +---
 contracts/src/TroveManager.sol             | 11 ++++++-----
 contracts/test/AccessControlTest.js        | 13 -------------
 4 files changed, 8 insertions(+), 23 deletions(-)

diff --git a/contracts/src/BorrowerOperations.sol b/contracts/src/BorrowerOperations.sol
index 3dffedb2..0c4bb895 100644
--- a/contracts/src/BorrowerOperations.sol
+++ b/contracts/src/BorrowerOperations.sol
@@ -209,12 +209,11 @@ contract BorrowerOperations is LiquityBase, Ownable, CheckContract, IBorrowerOpe
         );
 
         // Set the stored Trove properties and mint the NFT
-        vars.stake = contractsCache.troveManager.setTrovePropertiesOnOpen(
+        (vars.stake, vars.arrayIndex) = contractsCache.troveManager.setTrovePropertiesOnOpen(
             _owner, troveId, _ETHAmount, vars.compositeDebt, _annualInterestRate
         );
 
         sortedTroves.insert(troveId, _annualInterestRate, _upperHint, _lowerHint);
-        vars.arrayIndex = contractsCache.troveManager.addTroveIdToArray(troveId);
         emit TroveCreated(_owner, troveId, vars.arrayIndex);
 
         // Pull ETH tokens from sender and move them to the Active Pool
diff --git a/contracts/src/Interfaces/ITroveManager.sol b/contracts/src/Interfaces/ITroveManager.sol
index 6cc71935..07d1cfc2 100644
--- a/contracts/src/Interfaces/ITroveManager.sol
+++ b/contracts/src/Interfaces/ITroveManager.sol
@@ -46,8 +46,6 @@ interface ITroveManager is IERC721, ILiquityBase {
 
     function updateStakeAndTotalStakes(uint256 _troveId) external returns (uint256);
 
-    function addTroveIdToArray(uint256 _troveId) external returns (uint256 index);
-
     function getPendingETHReward(uint256 _troveId) external view returns (uint256);
 
     function getPendingBoldDebtReward(uint256 _troveId) external view returns (uint256);
@@ -105,7 +103,7 @@ interface ITroveManager is IERC721, ILiquityBase {
         uint256 _coll,
         uint256 _debt,
         uint256 _annualInterestRate
-    ) external returns (uint256);
+    ) external returns (uint256, uint256);
 
     function troveIsStale(uint256 _troveId) external view returns (bool);
 
diff --git a/contracts/src/TroveManager.sol b/contracts/src/TroveManager.sol
index 192362a5..28ba7719 100644
--- a/contracts/src/TroveManager.sol
+++ b/contracts/src/TroveManager.sol
@@ -1312,9 +1312,7 @@ contract TroveManager is ERC721, LiquityBase, Ownable, CheckContract, ITroveMana
     }
 
     // Push the trove's id to the Trove list, and record the corresponding array index on the Trove struct
-    function addTroveIdToArray(uint256 _troveId) external override returns (uint256) {
-        _requireCallerIsBorrowerOperations();
-
+    function _addTroveIdToArray(uint256 _troveId) internal returns (uint256) {
         /* Max array size is 2**128 - 1, i.e. ~3e30 troves. No risk of overflow, since troves have minimum Bold
         debt of liquidation reserve plus MIN_NET_DEBT. 3e30 Bold dwarfs the value of all wealth in the world ( which is < 1e15 USD). */
 
@@ -1571,7 +1569,7 @@ contract TroveManager is ERC721, LiquityBase, Ownable, CheckContract, ITroveMana
         uint256 _coll,
         uint256 _debt,
         uint256 _annualInterestRate
-    ) external returns (uint256) {
+    ) external returns (uint256, uint256) {
         _requireCallerIsBorrowerOperations();
         // TODO: optimize gas for writing to this struct
         Troves[_troveId].status = Status.active;
@@ -1584,8 +1582,11 @@ contract TroveManager is ERC721, LiquityBase, Ownable, CheckContract, ITroveMana
         // mint ERC721
         _mint(_owner, _troveId);
 
+        uint256 index = _addTroveIdToArray(_troveId);
+
         // Record the Trove's stake (for redistributions) and update the total stakes
-        return _updateStakeAndTotalStakes(_troveId);
+        uint256 stake = _updateStakeAndTotalStakes(_troveId);
+        return (stake, index);
     }
 
     function updateTroveDebtAndInterest(uint256 _troveId, uint256 _entireTroveDebt, uint256 _newAnnualInterestRate)
diff --git a/contracts/test/AccessControlTest.js b/contracts/test/AccessControlTest.js
index a1354e14..71957915 100644
--- a/contracts/test/AccessControlTest.js
+++ b/contracts/test/AccessControlTest.js
@@ -126,19 +126,6 @@ contract(
         }
       });
 
-      // addTroveIdToArray
-      it("addTroveIdToArray(): reverts when called by an account that is not BorrowerOperations", async () => {
-        // Attempt call from alice
-        try {
-          const txAlice = await troveManager.addTroveIdToArray(th.addressToTroveId(bob), {
-            from: alice,
-          });
-        } catch (err) {
-          assert.include(err.message, "revert");
-          // assert.include(err.message, "Caller is not the BorrowerOperations contract")
-        }
-      });
-
       // updateTroveColl
       it("updateTroveColl(): reverts when called by an account that is not BorrowerOperations", async () => {
         // Attempt call from alice