Merge branch 'v2.3-fix-review' into ed/62-notional-position

packages/perennial-account/contracts/Controller.sol

@@ -31,10 +31,10 @@ contract Controller is Factory, IController {
     uint256 constant MAX_MARKETS_PER_GROUP = 4;
 
     /// @dev USDC stablecoin address
-    Token6 public USDC; // solhint-disable-line var-name-mixedcase
+    Token6 public immutable USDC; // solhint-disable-line var-name-mixedcase
 
     /// @dev DSU address
-    Token18 public DSU; // solhint-disable-line var-name-mixedcase
+    Token18 public immutable DSU; // solhint-disable-line var-name-mixedcase
 
     /// @inheritdoc IController
     IMarketFactory public marketFactory;
@@ -104,10 +104,17 @@ contract Controller is Factory, IController {
             IMarket market = groupToMarkets[owner][group][i];
             RebalanceConfig memory marketRebalanceConfig = _rebalanceConfigs[owner][group][address(market)];
             (bool canMarketRebalance, Fixed6 imbalance) =
-                RebalanceLib.checkMarket(marketRebalanceConfig, groupCollateral, actualCollateral[i]);
+                RebalanceLib.checkMarket(
+                    marketRebalanceConfig,
+                    groupToMaxRebalanceFee[owner][group],
+                    groupCollateral,
+                    actualCollateral[i]
+                );
             imbalances[i] = imbalance;
             canRebalance = canRebalance || canMarketRebalance;
         }
+
+        // if group does not exist or was deleted, arrays will be empty and function will return (0, false, 0)
     }
 
     /// @inheritdoc IController

packages/perennial-account/contracts/Controller_Arbitrum.sol

@@ -11,9 +11,8 @@ import { Controller_Incentivized } from "./Controller_Incentivized.sol";
 contract Controller_Arbitrum is Controller_Incentivized, Kept_Arbitrum {
     /// @dev Creates instance of Controller which compensates keepers
     /// @param implementation Pristine collateral account contract
-    /// @param keepConfig Configuration used to compensate keepers
-    constructor(address implementation, KeepConfig memory keepConfig, IVerifierBase nonceManager)
-    Controller_Incentivized(implementation, keepConfig, nonceManager) {}
+    constructor(address implementation, IVerifierBase nonceManager)
+    Controller_Incentivized(implementation, nonceManager) {}
 
     /// @dev Use the Kept_Arbitrum implementation for calculating the dynamic fee
     function _calldataFee(

packages/perennial-account/contracts/Controller_Incentivized.sol

@@ -32,34 +32,35 @@ import { Withdrawal } from "./types/Withdrawal.sol";
 /// @notice Controller which compensates keepers for handling or relaying messages. Subclass to handle differences in
 /// gas calculations on different chains.
 abstract contract Controller_Incentivized is Controller, IRelayer, Kept {
+    /// @dev Handles relayed messages for nonce cancellation
+    IVerifierBase public immutable nonceManager;
+
     /// @dev Configuration used to calculate keeper compensation
     KeepConfig public keepConfig;
 
-    /// @dev Handles relayed messages for nonce cancellation
-    IVerifierBase public nonceManager;
-
     /// @dev Creates instance of Controller which compensates keepers
     /// @param implementation_ Pristine collateral account contract
-    /// @param keepConfig_ Configuration used to compensate keepers
-    constructor(address implementation_, KeepConfig memory keepConfig_, IVerifierBase nonceManager_)
+    constructor(address implementation_, IVerifierBase nonceManager_)
     Controller(implementation_) {
-        keepConfig = keepConfig_;
         nonceManager = nonceManager_;
     }
 
     /// @notice Configures message verification and keeper compensation
     /// @param marketFactory_ Contract used to validate delegated signers
     /// @param verifier_ Contract used to validate collateral account message signatures
     /// @param chainlinkFeed_ ETH-USD price feed used for calculating keeper compensation
+    /// @param keepConfig_ Configuration used to compensate keepers
     function initialize(
         IMarketFactory marketFactory_,
         IAccountVerifier verifier_,
-        AggregatorV3Interface chainlinkFeed_
+        AggregatorV3Interface chainlinkFeed_,
+        KeepConfig memory keepConfig_
     ) external initializer(1) {
         __Factory__initialize();
         __Kept__initialize(chainlinkFeed_, DSU);
         marketFactory = marketFactory_;
         verifier = verifier_;
+        keepConfig = keepConfig_;
     }
 
     /// @inheritdoc IController

packages/perennial-account/contracts/libs/RebalanceLib.sol

@@ -2,7 +2,7 @@
 pragma solidity ^0.8.13;
 
 import { Fixed6, Fixed6Lib } from "@equilibria/root/number/types/Fixed6.sol";
-import { UFixed6 } from "@equilibria/root/number/types/UFixed6.sol";
+import { UFixed6, UFixed6Lib } from "@equilibria/root/number/types/UFixed6.sol";
 import { IController } from "../interfaces/IController.sol";
 import { RebalanceConfig } from "../types/RebalanceConfig.sol";
 
@@ -11,26 +11,36 @@ import { RebalanceConfig } from "../types/RebalanceConfig.sol";
 library RebalanceLib {
     /// @dev Compares actual market collateral for owner with their account's target
     /// @param marketConfig Rebalance group configuration for this market
+    /// @param maxFee Maximum fee paid to keeper for rebalancing the group
     /// @param groupCollateral Owner's collateral across all markets in the group
     /// @param marketCollateral Owner's actual amount of collateral in this market
     /// @return canRebalance True if actual collateral in this market is outside of configured threshold
     /// @return imbalance Amount which needs to be transferred to balance the market
     function checkMarket(
         RebalanceConfig memory marketConfig,
+        UFixed6 maxFee,
         Fixed6 groupCollateral,
         Fixed6 marketCollateral
     ) internal pure returns (bool canRebalance, Fixed6 imbalance) {
         // determine how much collateral the market should have
         Fixed6 targetCollateral = groupCollateral.mul(Fixed6Lib.from(marketConfig.target));
 
-        // if market is empty, prevent divide-by-zero condition
-        if (marketCollateral.eq(Fixed6Lib.ZERO)) return (false, targetCollateral);
-        // calculate percentage difference between target and actual collateral
-        Fixed6 pctFromTarget = Fixed6Lib.ONE.sub(targetCollateral.div(marketCollateral));
-        // if this percentage exceeds the configured threshold, the market may be rebelanced
-        canRebalance = pctFromTarget.abs().gt(marketConfig.threshold);
+        // if target is zero, prevent divide-by-zero condition
+        if (targetCollateral.eq(Fixed6Lib.ZERO)) {
+            imbalance = marketCollateral.mul(Fixed6Lib.NEG_ONE);
+            // can rebalance if market is not empty and imbalance exceeds max fee paid to keeper
+            canRebalance = !marketCollateral.eq(Fixed6Lib.ZERO) && imbalance.abs().gt(maxFee);
+            return (canRebalance, imbalance);
+        }
+        // calculate percentage difference between actual and target collateral
+        Fixed6 pctFromTarget = Fixed6Lib.ONE.sub(marketCollateral.div(targetCollateral));
 
         // return negative number for surplus, positive number for deficit
         imbalance = targetCollateral.sub(marketCollateral);
+
+        // if this percentage exceeds the configured threshold,
+        // and the amount to rebalance exceeds max fee paid to keeper, the market may be rebalanced
+        canRebalance = pctFromTarget.abs().gt(marketConfig.threshold)
+            && imbalance.abs().gt(maxFee);
     }
 }

packages/perennial-account/contracts/types/RebalanceConfig.sol

@@ -7,7 +7,7 @@ import { UFixed6, UFixed6Lib } from "@equilibria/root/number/types/UFixed6.sol";
 struct RebalanceConfig {
     /// @dev Percentage of collateral from the group to deposit into the market
     UFixed6 target;
-    /// @dev Percentage away from the target at which keepers may rebalance
+    /// @dev Ratio of market collateral to target at which keepers may rebalance
     UFixed6 threshold;
 }
 

packages/perennial-account/contracts/types/RebalanceConfigChange.sol

@@ -15,7 +15,8 @@ struct RebalanceConfigChange {
     address[] markets;
     /// @dev Target allocation for markets in the aforementioned array
     RebalanceConfig[] configs;
-    /// @dev Largest amount to compensate a relayer/keeper for rebalancing the group in DSU
+    /// @dev Largest amount to compensate a relayer/keeper for rebalancing the group in DSU.
+    /// This amount also prevents keepers from rebalancing imbalances smaller than the keeper fee.
     UFixed6 maxFee;
     /// @dev Common information for collateral account actions
     Action action;

packages/perennial-account/test/helpers/arbitrumHelpers.ts

@@ -152,15 +152,13 @@ export async function deployAndInitializeController(
 // deploys an instance of the Controller with Arbitrum-specific keeper compensation mechanisms
 export async function deployControllerArbitrum(
   owner: SignerWithAddress,
-  keepConfig: IKept.KeepConfigStruct,
   nonceManager: IVerifier,
   overrides?: CallOverrides,
 ): Promise<Controller_Arbitrum> {
   const accountImpl = await new Account__factory(owner).deploy(USDC_ADDRESS, DSU_ADDRESS, DSU_RESERVE)
   accountImpl.initialize(constants.AddressZero)
   const controller = await new Controller_Arbitrum__factory(owner).deploy(
     accountImpl.address,
-    keepConfig,
     nonceManager.address,
     overrides ?? {},
   )

packages/perennial-account/test/integration/Controller.ts

@@ -295,6 +295,10 @@ describe('ControllerBase', () => {
     })
 
     it('handles groups with no collateral', async () => {
+      const [groupCollateral, canRebalance] = await controller.callStatic.checkGroup(userA.address, 1)
+      expect(groupCollateral).to.equal(0)
+      expect(canRebalance).to.be.false
+
       await expect(controller.rebalanceGroup(userA.address, 1, TX_OVERRIDES)).to.be.revertedWithCustomError(
         controller,
         'ControllerGroupBalancedError',
@@ -318,6 +322,96 @@ describe('ControllerBase', () => {
       expect(groupCollateral).to.equal(parse6decimal('15000'))
       expect(canRebalance).to.be.false
     })
+
+    it('rebalances markets with no collateral when others are within threshold', async () => {
+      // reconfigure group such that ETH market has threshold higher than it's imbalance
+      const message = {
+        group: 1,
+        markets: [ethMarket.address, btcMarket.address],
+        configs: [
+          { target: parse6decimal('0.9'), threshold: parse6decimal('0.15') },
+          { target: parse6decimal('0.1'), threshold: parse6decimal('0.03') },
+        ],
+        maxFee: constants.Zero,
+        ...(await createAction(userA.address)),
+      }
+      const signature = await signRebalanceConfigChange(userA, verifier, message)
+      await expect(controller.connect(keeper).changeRebalanceConfigWithSignature(message, signature)).to.not.be.reverted
+
+      // transfer funds only to the ETH market
+      await transfer(parse6decimal('10000'), userA, ethMarket)
+
+      await expect(controller.rebalanceGroup(userA.address, 1, TX_OVERRIDES))
+        .to.emit(dsu, 'Transfer')
+        .withArgs(ethMarket.address, accountA.address, utils.parseEther('1000'))
+        .to.emit(dsu, 'Transfer')
+        .withArgs(accountA.address, btcMarket.address, utils.parseEther('1000'))
+        .to.emit(controller, 'GroupRebalanced')
+        .withArgs(userA.address, 1)
+
+      // ensure group collateral unchanged and cannot rebalance
+      const [groupCollateral, canRebalance] = await controller.callStatic.checkGroup(userA.address, 1)
+      expect(groupCollateral).to.equal(parse6decimal('10000'))
+      expect(canRebalance).to.be.false
+    })
+
+    it('should not rebalance empty market configured to be empty', async () => {
+      // reconfigure group such that BTC market is empty
+      const message = {
+        group: 1,
+        markets: [ethMarket.address, btcMarket.address],
+        configs: [
+          { target: parse6decimal('1'), threshold: parse6decimal('0.05') },
+          { target: parse6decimal('0'), threshold: parse6decimal('0.05') },
+        ],
+        maxFee: constants.Zero,
+        ...(await createAction(userA.address)),
+      }
+      const signature = await signRebalanceConfigChange(userA, verifier, message)
+      await expect(controller.connect(keeper).changeRebalanceConfigWithSignature(message, signature)).to.not.be.reverted
+
+      // transfer funds to the ETH market
+      await transfer(parse6decimal('2500'), userA, ethMarket)
+
+      // ensure group balanced
+      await expect(controller.rebalanceGroup(userA.address, 1, TX_OVERRIDES)).to.be.revertedWithCustomError(
+        controller,
+        'ControllerGroupBalancedError',
+      )
+    })
+
+    it('should rebalance non-empty market configured to be empty', async () => {
+      // reconfigure group such that BTC market is empty
+      const message = {
+        group: 1,
+        markets: [ethMarket.address, btcMarket.address],
+        configs: [
+          { target: parse6decimal('1'), threshold: parse6decimal('0.05') },
+          { target: parse6decimal('0'), threshold: parse6decimal('0.05') },
+        ],
+        maxFee: constants.Zero,
+        ...(await createAction(userA.address)),
+      }
+      const signature = await signRebalanceConfigChange(userA, verifier, message)
+      await expect(controller.connect(keeper).changeRebalanceConfigWithSignature(message, signature)).to.not.be.reverted
+
+      // transfer funds to both markets
+      await transfer(parse6decimal('2500'), userA, ethMarket)
+      await transfer(parse6decimal('2500'), userA, btcMarket)
+
+      await expect(controller.rebalanceGroup(userA.address, 1, TX_OVERRIDES))
+        .to.emit(dsu, 'Transfer')
+        .withArgs(btcMarket.address, accountA.address, utils.parseEther('2500'))
+        .to.emit(dsu, 'Transfer')
+        .withArgs(accountA.address, ethMarket.address, utils.parseEther('2500'))
+        .to.emit(controller, 'GroupRebalanced')
+        .withArgs(userA.address, 1)
+
+      // ensure group collateral unchanged and cannot rebalance
+      const [groupCollateral, canRebalance] = await controller.callStatic.checkGroup(userA.address, 1)
+      expect(groupCollateral).to.equal(parse6decimal('5000'))
+      expect(canRebalance).to.be.false
+    })
   })
 
   describe('#transfer', () => {