Reserve supports multiple exchanges (#6299)

### Description

Allows the reserve to allow multiple exchanges to pull assets without a limit.

### Added governable variables:
`isExchangeSpender`

### Other changes

some more testing

### Tested

Unit test added.

### Related issues

- Closes #6165

### Backwards compatibility

During the migrations, an exchange needs to be set.

packages/protocol/contracts/stability/Reserve.sol

@@ -55,6 +55,9 @@ contract Reserve is
   uint256 public frozenReserveGoldStartDay;
   uint256 public frozenReserveGoldDays;
 
+  mapping(address => bool) public isExchangeSpender;
+  address[] public exchangeSpenderAddresses;
+
   event TobinTaxStalenessThresholdSet(uint256 value);
   event DailySpendingRatioSet(uint256 ratio);
   event TokenAdded(address indexed token);
@@ -67,6 +70,8 @@ contract Reserve is
   event ReserveGoldTransferred(address indexed spender, address indexed to, uint256 value);
   event TobinTaxSet(uint256 value);
   event TobinTaxReserveRatioSet(uint256 value);
+  event ExchangeSpenderAdded(address indexed exchangeSpender);
+  event ExchangeSpenderRemoved(address indexed exchangeSpender);
 
   modifier isStableToken(address token) {
     require(isToken[token], "token addr was never registered");
@@ -78,7 +83,7 @@ contract Reserve is
    * @return The storage, major, minor, and patch version of the contract.
    */
   function getVersionNumber() external pure returns (uint256, uint256, uint256, uint256) {
-    return (1, 1, 1, 0);
+    return (1, 1, 2, 0);
   }
 
   function() external payable {} // solhint-disable no-empty-blocks
@@ -302,10 +307,50 @@ contract Reserve is
    * @param spender The address that is to be no longer allowed to spend Reserve funds.
    */
   function removeSpender(address spender) external onlyOwner {
-    isSpender[spender] = false;
+    delete isSpender[spender];
     emit SpenderRemoved(spender);
   }
 
+  function isAllowedToSpendExchange(address spender) public view returns (bool) {
+    return
+      isExchangeSpender[spender] || (registry.getAddressForOrDie(EXCHANGE_REGISTRY_ID) == spender);
+  }
+
+  /**
+   * @notice Gives an address permission to spend Reserve without limit.
+   * @param spender The address that is allowed to spend Reserve funds.
+   */
+  function addExchangeSpender(address spender) external onlyOwner {
+    require(!isExchangeSpender[spender], "Address is already Exchange Spender");
+    isExchangeSpender[spender] = true;
+    exchangeSpenderAddresses.push(spender);
+    emit ExchangeSpenderAdded(spender);
+  }
+
+  /**
+   * @notice Takes away an address's permission to spend Reserve funds without limits.
+   * @param spender The address that is to be no longer allowed to spend Reserve funds.
+   */
+  function removeExchangeSpender(address spender, uint256 index) external onlyOwner {
+    isExchangeSpender[spender] = false;
+    uint256 numAddresses = exchangeSpenderAddresses.length;
+    require(index < numAddresses, "Index is invalid");
+    require(spender == exchangeSpenderAddresses[index], "Index does not match spender");
+    uint256 newNumAddresses = numAddresses.sub(1);
+
+    if (index != newNumAddresses) {
+      exchangeSpenderAddresses[index] = exchangeSpenderAddresses[newNumAddresses];
+    }
+
+    exchangeSpenderAddresses[newNumAddresses] = address(0x0);
+    exchangeSpenderAddresses.length = newNumAddresses;
+    emit ExchangeSpenderRemoved(spender);
+  }
+
+  function getExchangeSpenders() public view returns (address[] memory) {
+    return exchangeSpenderAddresses;
+  }
+
   /**
    * @notice Transfer gold to a whitelisted address subject to reserve spending limits.
    * @param to The address that will receive the gold.
@@ -345,11 +390,8 @@ contract Reserve is
    * @param value The amount of gold to transfer.
    * @return Returns true if the transaction succeeds.
    */
-  function transferExchangeGold(address payable to, uint256 value)
-    external
-    onlyRegisteredContract(EXCHANGE_REGISTRY_ID)
-    returns (bool)
-  {
+  function transferExchangeGold(address payable to, uint256 value) external returns (bool) {
+    require(isAllowedToSpendExchange(msg.sender), "Address not allowed to spend");
     return _transferGold(to, value);
   }
 

packages/protocol/contracts/stability/interfaces/IReserve.sol

@@ -11,4 +11,8 @@ interface IReserve {
   function getOrComputeTobinTax() external returns (uint256, uint256);
   function getTokens() external view returns (address[] memory);
   function getReserveRatio() external view returns (uint256);
+  function addExchangeSpender(address) external;
+  function removeExchangeSpender(address, uint256) external;
+  function addSpender(address) external;
+  function removeSpender(address) external;
 }

packages/protocol/test/stability/reserve.ts

@@ -1,6 +1,7 @@
 import { CeloContractName } from '@celo/protocol/lib/registry-utils'
 import {
   assertEqualBN,
+  assertLogMatches2,
   assertRevert,
   assertSameAddress,
   timeTravel,
@@ -35,6 +36,7 @@ contract('Reserve', (accounts: string[]) => {
   const nonOwner: string = accounts[1]
   const spender: string = accounts[2]
   const exchangeAddress: string = accounts[3]
+  const exchangeSpenderAddress: string = accounts[4]
   const aTobinTaxStalenessThreshold: number = 600
   const aTobinTax = toFixed(0.005)
   const aTobinTaxReserveRatio = toFixed(2)
@@ -48,6 +50,7 @@ contract('Reserve', (accounts: string[]) => {
     mockSortedOracles = await MockSortedOracles.new()
     await registry.setAddressFor(CeloContractName.SortedOracles, mockSortedOracles.address)
     await registry.setAddressFor(CeloContractName.Exchange, exchangeAddress)
+
     await reserve.initialize(
       registry.address,
       aTobinTaxStalenessThreshold,
@@ -283,6 +286,115 @@ contract('Reserve', (accounts: string[]) => {
     })
   })
 
+  describe('#addExchangeSpender(exchangeAddress)', () => {
+    it('only allows owner', async () => {
+      await assertRevert(reserve.addExchangeSpender(nonOwner, { from: nonOwner }))
+    })
+
+    it('should emit addExchangeSpender event on add', async () => {
+      const resp = await reserve.addExchangeSpender(exchangeSpenderAddress)
+      const log = resp.logs[0]
+      assert.equal(resp.logs.length, 1)
+      assertLogMatches2(log, {
+        event: 'ExchangeSpenderAdded',
+        args: {
+          exchangeSpender: exchangeSpenderAddress,
+        },
+      })
+    })
+
+    it('has the right list of exchange spenders after addition', async () => {
+      await reserve.addExchangeSpender(exchangeAddress)
+      await reserve.addExchangeSpender(accounts[1])
+      const spenders = await reserve.getExchangeSpenders()
+      assert.equal(spenders.length, 2)
+      assert.equal(spenders[0], exchangeAddress)
+      assert.equal(spenders[1], accounts[1])
+    })
+  })
+
+  describe('#removeExchangeSpender(exchangeAddress)', () => {
+    beforeEach(async () => {
+      await reserve.addExchangeSpender(exchangeSpenderAddress)
+    })
+
+    it('only allows owner', async () => {
+      await assertRevert(reserve.removeExchangeSpender(nonOwner, 0, { from: nonOwner }))
+    })
+
+    it('should emit removeExchangeSpender event on remove', async () => {
+      const resp = await reserve.removeExchangeSpender(exchangeSpenderAddress, 0)
+      const log = resp.logs[0]
+      assert.equal(resp.logs.length, 1)
+      assertLogMatches2(log, {
+        event: 'ExchangeSpenderRemoved',
+        args: {
+          exchangeSpender: exchangeSpenderAddress,
+        },
+      })
+    })
+
+    it('has the right list of exchange spenders', async () => {
+      const spenders = await reserve.getExchangeSpenders()
+      assert.equal(spenders.length, 1)
+      assert.equal(spenders[0], exchangeSpenderAddress)
+    })
+
+    it('has the right list of exchange after removing one', async () => {
+      await reserve.removeExchangeSpender(exchangeSpenderAddress, 0)
+      const spenders = await reserve.getExchangeSpenders()
+      assert.equal(spenders.length, 0)
+    })
+
+    it("can't be removed twice", async () => {
+      await reserve.removeExchangeSpender(exchangeSpenderAddress, 0)
+      await assertRevert(reserve.removeExchangeSpender(exchangeSpenderAddress, 0))
+    })
+
+    it("can't delete an index out of range", async () => {
+      await assertRevert(reserve.removeExchangeSpender(exchangeSpenderAddress, 1))
+    })
+
+    it('removes from a big array', async () => {
+      await reserve.addExchangeSpender(accounts[1])
+      await reserve.removeExchangeSpender(exchangeSpenderAddress, 0)
+      const spenders = await reserve.getExchangeSpenders()
+      assert.equal(spenders.length, 1)
+      assert.equal(spenders[0], accounts[1])
+    })
+
+    it("doesn't remove an address with the wrong index", async () => {
+      await reserve.addExchangeSpender(accounts[1])
+      await assertRevert(reserve.removeExchangeSpender(exchangeSpenderAddress, 1))
+    })
+  })
+
+  describe('#addSpender(spender)', () => {
+    it('emits on add', async () => {
+      const addSpenderTx = await reserve.addSpender(spender)
+
+      const addExchangeSpenderTxLogs = addSpenderTx.logs.filter((x) => x.event === 'SpenderAdded')
+      assert(addExchangeSpenderTxLogs.length === 1, 'Did not receive event')
+    })
+
+    it('only allows owner', async () => {
+      await assertRevert(reserve.addSpender(nonOwner, { from: nonOwner }))
+    })
+  })
+
+  describe('#removeSpender(spender)', () => {
+    it('emits on remove', async () => {
+      const addSpenderTx = await reserve.removeSpender(spender)
+
+      const addExchangeSpenderTxLogs = addSpenderTx.logs.filter((x) => x.event === 'SpenderRemoved')
+      assert(addExchangeSpenderTxLogs.length === 1, 'Did not receive event')
+    })
+
+    it('only allows owner', async () => {
+      await assertRevert(reserve.removeSpender(nonOwner, { from: nonOwner }))
+    })
+  })
+
   describe('#transferExchangeGold()', () => {
     const aValue = 10000
     let otherReserveAddress: string = ''
@@ -293,7 +405,7 @@ contract('Reserve', (accounts: string[]) => {
       await reserve.addOtherReserveAddress(otherReserveAddress)
     })
 
-    it('should allow a exchange to call transferExchangeGold', async () => {
+    it('should allow an exchange to call transferExchangeGold', async () => {
       await reserve.transferExchangeGold(nonOwner, aValue, { from: exchangeAddress })
     })
 
@@ -305,6 +417,14 @@ contract('Reserve', (accounts: string[]) => {
       await assertRevert(reserve.transferExchangeGold(nonOwner, aValue, { from: nonOwner }))
     })
 
+    it('should not allow removed exchange spender addresses to call transferExchangeGold', async () => {
+      await reserve.addExchangeSpender(exchangeSpenderAddress)
+      await reserve.removeExchangeSpender(exchangeSpenderAddress, 0)
+      await assertRevert(
+        reserve.transferExchangeGold(nonOwner, aValue, { from: exchangeSpenderAddress })
+      )
+    })
+
     it('should not allow freezing more gold than is available', async () => {
       await assertRevert(reserve.setFrozenGold(aValue + 1, 1))
     })