Remove all unnecessary usages of upgrade: true in scenarios (compou…

…nd-finance#494) This PR removes unnecessary usages of `upgrade: true` from our scenarios. Some scenarios still have valid reasons for using it (e.g. interest rate scenarios that use it to upgrade the interest rate params), so these are not fully removed from all scenarios.
peersky · Jul 29, 2022 · 357d3ed · 357d3ed
1 parent c31589a
commit 357d3ed
Show file tree

Hide file tree

Showing 13 changed files with 39 additions and 102 deletions.
diff --git a/scenario/AllowBySigScenario.ts b/scenario/AllowBySigScenario.ts
@@ -3,7 +3,7 @@ import { expect } from 'chai';
 
 scenario(
   'Comet#allowBySig > allows a user to authorize a manager by signature',
-  { upgrade: true },
+  {},
   async ({ comet, actors }, world) => {
     const { albert, betty } = actors;
 
@@ -35,6 +35,11 @@ scenario(
   }
 );
 
+// XXX These revert scenarios need to set `upgrade: true` because Hardhat fails to
+// recognize custom errors received in fallback functions that originate from external
+// artifacts. For example, CometExt is an external artifact here unless we redeploy it
+// using the ModernConstraint.
+// Related: https://github.com/NomicFoundation/hardhat/issues/1875
 scenario(
   'Comet#allowBySig > fails if owner argument is altered',
   { upgrade: true },

diff --git a/scenario/AllowScenario.ts b/scenario/AllowScenario.ts
@@ -2,7 +2,7 @@ import { scenario } from './context/CometContext';
 import { expect } from 'chai';
 import { utils } from 'ethers';
 
-scenario('Comet#allow > allows a user to authorize a manager', { upgrade: true }, async ({ comet, actors }) => {
+scenario('Comet#allow > allows a user to authorize a manager', {}, async ({ comet, actors }) => {
   const { albert, betty } = actors;
 
   const txn = await albert.allow(betty, true);

diff --git a/scenario/ApproveThisScenario.ts b/scenario/ApproveThisScenario.ts
@@ -2,7 +2,7 @@ import { scenario } from './context/CometContext';
 import { expect } from 'chai';
 import { constants, utils } from 'ethers';
 
-scenario('Comet#approveThis > allows governor to authorize and rescind authorization for Comet ERC20', { upgrade: true }, async ({ comet, timelock, actors }, world, context) => {
+scenario('Comet#approveThis > allows governor to authorize and rescind authorization for Comet ERC20', {}, async ({ comet, timelock, actors }, world, context) => {
   let approveThisCalldata = utils.defaultAbiCoder.encode(["address", "address", "uint256"], [timelock.address, comet.address, constants.MaxUint256]);
   await context.fastGovernanceExecute(
     [comet.address],
@@ -24,7 +24,7 @@ scenario('Comet#approveThis > allows governor to authorize and rescind authoriza
   expect(await comet.isAllowed(comet.address, timelock.address)).to.be.false;
 });
 
-scenario('Comet#approveThis > allows governor to authorize and rescind authorization for non-Comet ERC20', { upgrade: true }, async ({ comet, timelock, actors }, world, context) => {
+scenario('Comet#approveThis > allows governor to authorize and rescind authorization for non-Comet ERC20', {}, async ({ comet, timelock, actors }, world, context) => {
   const baseTokenAddress = await comet.baseToken();
   const baseToken = context.getAssetByAddress(baseTokenAddress);
 
@@ -50,7 +50,7 @@ scenario('Comet#approveThis > allows governor to authorize and rescind authoriza
   expect(await baseToken.allowance(comet.address, timelock.address)).to.be.equal(0n);
 });
 
-scenario('Comet#approveThis > reverts if not called by governor', { upgrade: true }, async ({ comet, timelock, actors }) => {
+scenario('Comet#approveThis > reverts if not called by governor', {}, async ({ comet, timelock, actors }) => {
   await expect(comet.approveThis(timelock.address, comet.address, constants.MaxUint256))
     .to.be.revertedWith("custom error 'Unauthorized()'");
 });
diff --git a/scenario/CometScenario.ts b/scenario/CometScenario.ts
@@ -17,6 +17,6 @@ scenario('has assets', {}, async ({ comet, actors, assets }: CometProperties, wo
   expect(collateralAssets.map(a => a.asset)).to.have.members(contextAssets);
 });
 
-scenario('requires upgrade', { upgrade: true }, async ({ comet }, world) => {
+scenario('requires upgrade', {}, async ({ comet }, world) => {
   // Nothing currently here
 });
diff --git a/scenario/ConfiguratorScenario.ts b/scenario/ConfiguratorScenario.ts
@@ -4,7 +4,7 @@ import { utils } from 'ethers';
 import { scaleToDecimals } from './utils';
 
 scenario('upgrade governor', { upgradeAll: true }, async ({ comet, configurator, proxyAdmin, timelock, actors }, world, context) => {
-  const { admin, albert } = actors;
+  const { albert } = actors;
 
   expect(await comet.governor()).to.equal(timelock.address);
   expect((await configurator.getConfiguration(comet.address)).governor).to.equal(timelock.address);

diff --git a/scenario/ConstraintScenario.ts b/scenario/ConstraintScenario.ts
@@ -5,7 +5,6 @@ import { defactor, expect } from '../test/helpers';
 scenario(
   'Comet#constraint > collateral CometBalanceConstraint + BalanceConstraint both satisfied',
   {
-    upgrade: true,
     tokenBalances: {
       albert: { $asset0: 100 }, // in units of asset, not wei
     },
@@ -26,7 +25,6 @@ scenario(
 scenario(
   'Comet#constraint > base CometBalanceConstraint + BalanceConstraint both satisfied',
   {
-    upgrade: true,
     tokenBalances: {
       albert: { $base: 100 }, // in units of asset, not wei
     },
@@ -52,7 +50,6 @@ scenario(
 scenario(
   'Comet#constraint > negative comet base balance (borrow position)',
   {
-    upgrade: true,
     tokenBalances: {
       albert: { $base: 1000 }, // in units of asset, not wei
     },
@@ -80,7 +77,6 @@ scenario(
 scenario(
   'UtilizationConstraint > sets utilization to 25%',
   {
-    upgrade: true,
     utilization: 0.25,
   },
   async ({ comet }) => {
@@ -91,7 +87,6 @@ scenario(
 scenario(
   'UtilizationConstraint > sets utilization to 50%',
   {
-    upgrade: true,
     utilization: 0.50,
   },
   async ({ comet }) => {
@@ -102,7 +97,6 @@ scenario(
 scenario(
   'UtilizationConstraint > sets utilization to 75%',
   {
-    upgrade: true,
     utilization: 0.75,
   },
   async ({ comet }) => {
@@ -115,7 +109,6 @@ scenario(
 scenario.skip(
   'UtilizationConstraint > sets utilization to 100%',
   {
-    upgrade: true,
     utilization: 1,
   },
   async ({ comet }) => {

diff --git a/scenario/InterestRateScenario.ts b/scenario/InterestRateScenario.ts
@@ -40,7 +40,7 @@ function calculateUtilization(
 
 scenario(
   'Comet#interestRate > rates using on-chain configuration constants',
-  { upgrade: true },
+  {},
   async ({ comet, actors }) => {
     let { totalSupplyBase, totalBorrowBase, baseSupplyIndex, baseBorrowIndex } = await comet.totalsBasic();
     const supplyKink = await comet.supplyKink();
@@ -176,7 +176,7 @@ scenario(
 // XXX this test seems too fickle
 scenario.skip(
   'Comet#interestRate > when utilization is 50%',
-  { utilization: 0.5, upgrade: true },
+  { utilization: 0.5 },
   async ({ comet, actors }, world) => {
     const utilization = await comet.getUtilization();
     expect(defactor(utilization)).to.be.approximately(0.5, 0.00001);

diff --git a/scenario/LiquidationScenario.ts b/scenario/LiquidationScenario.ts
@@ -3,7 +3,7 @@ import { CometInterface } from '../build/types';
 import CometActor from './context/CometActor';
 import { event, expect } from '../test/helpers';
 
-async function getLiquidationMargin({comet, actor, baseLiquidity, factorScale}): Promise<bigint> {
+async function getLiquidationMargin({ comet, actor, baseLiquidity, factorScale }): Promise<bigint> {
   const numAssets = await comet.numAssets();
   let liquidity = baseLiquidity;
   for (let i = 0; i < numAssets; i++) {
@@ -24,15 +24,15 @@ invariant:
 isolating for timeElapsed:
 timeElapsed = -liquidationMargin / (baseBalanceOf * price / baseScale) / (borrowRate / factorScale);
 */
-async function timeUntilUnderwater({comet, actor, fudgeFactor = 0n}: {comet: CometInterface, actor: CometActor, fudgeFactor?: bigint}): Promise<number> {
+async function timeUntilUnderwater({ comet, actor, fudgeFactor = 0n }: { comet: CometInterface, actor: CometActor, fudgeFactor?: bigint }): Promise<number> {
   const baseBalance = await actor.getCometBaseBalance();
   const baseScale = (await comet.baseScale()).toBigInt();
   const basePrice = (await comet.getPrice(await comet.baseTokenPriceFeed())).toBigInt();
   const baseLiquidity = baseBalance * basePrice / baseScale;
   const utilization = await comet.getUtilization();
   const borrowRate = (await comet.getBorrowRate(utilization)).toBigInt();
   const factorScale = (await comet.factorScale()).toBigInt();
-  const liquidationMargin = await getLiquidationMargin({comet, actor, baseLiquidity, factorScale});
+  const liquidationMargin = await getLiquidationMargin({ comet, actor, baseLiquidity, factorScale });
 
   if (liquidationMargin < 0) {
     return 0; // already underwater
@@ -52,7 +52,6 @@ scenario(
       albert: { $base: -1000 },
       betty: { $base: 1000 },
     },
-    upgrade: true
   },
   async ({ comet, actors }, world) => {
     const { albert, betty } = actors;
@@ -67,7 +66,7 @@ scenario(
       })
     );
 
-    await betty.withdrawAsset({asset: baseToken, amount: baseBorrowMin}); // force accrue
+    await betty.withdrawAsset({ asset: baseToken, amount: baseBorrowMin }); // force accrue
 
     expect(await comet.isLiquidatable(albert.address)).to.be.true;
   }
@@ -86,7 +85,6 @@ scenario(
     pause: {
       absorbPaused: true,
     },
-    upgrade: true
   },
   async ({ comet, actors }, world) => {
     const { albert, betty } = actors;
@@ -101,10 +99,10 @@ scenario(
       })
     );
 
-    await betty.withdrawAsset({asset: baseToken, amount: baseBorrowMin}); // force accrue
+    await betty.withdrawAsset({ asset: baseToken, amount: baseBorrowMin }); // force accrue
 
     await expect(
-      betty.absorb({absorber: betty.address, accounts: [albert.address]})
+      betty.absorb({ absorber: betty.address, accounts: [albert.address] })
     ).to.be.revertedWith("custom error 'Paused()'");
   }
 );
@@ -122,7 +120,6 @@ scenario(
       },
       betty: { $base: 10 }
     },
-    upgrade: true
   },
   async ({ comet, actors }, world) => {
     const { albert, betty } = actors;
@@ -137,7 +134,7 @@ scenario(
 
     const lp0 = await comet.liquidatorPoints(betty.address);
 
-    await betty.absorb({absorber: betty.address, accounts: [albert.address]});
+    await betty.absorb({ absorber: betty.address, accounts: [albert.address] });
 
     const lp1 = await comet.liquidatorPoints(betty.address);
 
@@ -176,10 +173,9 @@ scenario.skip(
         $asset0: .001
       },
     },
-    upgrade: true
   },
   async ({ comet, actors }, world) => {
-    const { albert, betty, admin } = actors;
+    const { albert, betty, charles } = actors;
     const { asset: asset0Address, scale } = await comet.getAssetInfo(0);
 
     const collateralBalance = scale.toBigInt() / 1000n; // .001
@@ -192,27 +188,27 @@ scenario.skip(
       })
     );
 
-    await betty.absorb({absorber: betty.address, accounts: [albert.address]});
+    await betty.absorb({ absorber: betty.address, accounts: [albert.address] });
 
-    const txReceipt = await admin.withdrawAssetFrom({
+    const txReceipt = await charles.withdrawAssetFrom({
       src: comet.address,
-      dst: admin.address,
+      dst: charles.address,
       asset: asset0Address,
       amount: collateralBalance
     });
 
-    expect(event({receipt: txReceipt}, 0)).to.deep.equal({
+    expect(event({ receipt: txReceipt }, 0)).to.deep.equal({
       Transfer: {
         from: comet.address,
-        to: admin.address,
+        to: charles.address,
         amount: collateralBalance
       }
     });
 
-    expect(event({receipt: txReceipt}, 1)).to.deep.equal({
+    expect(event({ receipt: txReceipt }, 1)).to.deep.equal({
       WithdrawCollateral: {
         src: comet.address,
-        to: admin.address,
+        to: charles.address,
         asset: asset0Address,
         amount: collateralBalance
       }

diff --git a/scenario/PauseGuardianScenario.ts b/scenario/PauseGuardianScenario.ts
@@ -76,7 +76,6 @@ scenario(
     pause: {
       all: false,
     },
-    upgrade: true
   },
   async ({ comet, actors }) => {
     const { albert } = actors;

diff --git a/scenario/SupplyScenario.ts b/scenario/SupplyScenario.ts
@@ -6,7 +6,6 @@ import { expectApproximately, getExpectedBaseBalance, getInterest } from './util
 scenario(
   'Comet#supply > base asset',
   {
-    upgrade: true,
     tokenBalances: {
       albert: { $base: 100 }, // in units of asset, not wei
     },
@@ -38,7 +37,6 @@ scenario(
 scenario(
   'Comet#supply > collateral asset',
   {
-    upgrade: true,
     tokenBalances: {
       albert: { $asset0: 100 }, // in units of asset, not wei
     },
@@ -76,7 +74,6 @@ scenario(
 scenario(
   'Comet#supply > repay borrow',
   {
-    upgrade: true,
     tokenBalances: {
       albert: { $base: 1000 }
     },
@@ -108,7 +105,6 @@ scenario(
 scenario(
   'Comet#supplyFrom > base asset',
   {
-    upgrade: true,
     tokenBalances: {
       albert: { $base: 100 }, // in units of asset, not wei
     },
@@ -144,7 +140,6 @@ scenario(
 scenario(
   'Comet#supplyFrom > collateral asset',
   {
-    upgrade: true,
     tokenBalances: {
       albert: { $asset0: 100 }, // in units of asset, not wei
     },
@@ -187,7 +182,6 @@ scenario(
 scenario(
   'Comet#supplyFrom > repay borrow',
   {
-    upgrade: true,
     tokenBalances: {
       albert: { $base: 1000 }
     },
@@ -223,7 +217,6 @@ scenario(
 scenario(
   'Comet#supply reverts if not enough ERC20 approval',
   {
-    upgrade: true,
     tokenBalances: {
       albert: { $base: 100 }, // in units of asset, not wei
     },
@@ -246,7 +239,6 @@ scenario(
 scenario(
   'Comet#supplyFrom reverts if not enough ERC20 approval',
   {
-    upgrade: true,
     tokenBalances: {
       albert: { $base: 100 }, // in units of asset, not wei
     },
@@ -274,7 +266,6 @@ scenario(
 scenario(
   'Comet#supply reverts if not enough ERC20 balance',
   {
-    upgrade: true,
     tokenBalances: {
       albert: { $base: 10 }, // in units of asset, not wei
     },
@@ -298,7 +289,6 @@ scenario(
 scenario(
   'Comet#supplyFrom reverts if not enough ERC20 balance',
   {
-    upgrade: true,
     tokenBalances: {
       albert: { $base: 10 }, // in units of asset, not wei
     },
@@ -325,7 +315,6 @@ scenario(
 scenario(
   'Comet#supplyFrom reverts if operator not given permission',
   {
-    upgrade: true,
     tokenBalances: {
       albert: { $asset0: 100 }, // in units of asset, not wei
     },
@@ -351,7 +340,6 @@ scenario(
 scenario(
   'Comet#supply reverts when supply is paused',
   {
-    upgrade: true,
     pause: {
       supplyPaused: true,
     },
@@ -375,7 +363,6 @@ scenario(
 scenario(
   'Comet#supplyFrom reverts when supply is paused',
   {
-    upgrade: true,
     pause: {
       supplyPaused: true,
     },
@@ -400,9 +387,7 @@ scenario(
 
 scenario(
   'Comet#supply reverts if asset is not supported',
-  {
-    upgrade: true,
-  },
+  {},
   async ({ comet, actors }, world, context) => {
     // XXX requires deploying an unsupported asset (maybe via remote token constraint)
   }