Merge pull request #162 from bancorprotocol/carbon-vortex-2.0

fix upgradeable contract and health tests

contracts/helpers/TestUpgradeable.sol

@@ -13,9 +13,8 @@ contract TestUpgradeable is Upgradeable {
     // solhint-disable func-name-mixedcase
 
     function __TestUpgradeable_init() internal onlyInitializing {
-        __Upgradeable_init();
-
         __TestUpgradeable_init_unchained();
+        __Upgradeable_init();
     }
 
     function __TestUpgradeable_init_unchained() internal onlyInitializing {

contracts/utility/Upgradeable.sol

@@ -42,7 +42,7 @@ abstract contract Upgradeable is IUpgradeable, AccessControlEnumerableUpgradeabl
      * @dev performs contract-specific initialization
      */
     function __Upgradeable_init_unchained() internal onlyInitializing {
-        _initializations = version() - 1;
+        _initializations = version();
 
         // set up administrative roles
         _setRoleAdmin(ROLE_ADMIN, ROLE_ADMIN);
@@ -81,10 +81,13 @@ abstract contract Upgradeable is IUpgradeable, AccessControlEnumerableUpgradeabl
      *
      * - this must and can be called only once per-upgrade
      */
-    function postUpgrade(bytes calldata data) external {
+    function postUpgrade(bool checkVersion, bytes calldata data) external {
         uint16 initializations = _initializations + 1;
-        if (initializations != version()) {
+        uint16 _version = version();
+        if (checkVersion && initializations != _version) {
             revert AlreadyInitialized();
+        } else if (!checkVersion) {
+            initializations = _version;
         }
 
         _initializations = initializations;

contracts/vortex/CarbonVortex.sol

@@ -168,7 +168,7 @@ contract CarbonVortex is ICarbonVortex, Upgradeable, ReentrancyGuardUpgradeable,
      * @inheritdoc Upgradeable
      */
     function version() public pure override(IVersioned, Upgradeable) returns (uint16) {
-        return 1;
+        return 2;
     }
 
     /**

data/named-accounts.ts

@@ -82,7 +82,7 @@ const TestNamedAccounts = {
         ...getAddress(mantle, '0xf89d7b9c864f589bbF53a82105107622B35EaA40')
     },
     daiWhale: {
-        ...getAddress(mainnet, '0x66F62574ab04989737228D18C3624f7FC1edAe14'),
+        ...getAddress(mainnet, '0xD1668fB5F690C59Ab4B0CAbAd0f8C1617895052B'),
         ...getAddress(base, '0xe9b14a1Be94E70900EDdF1E22A4cB8c56aC9e10a'),
         ...getAddress(arbitrum, '0xd85E038593d7A098614721EaE955EC2022B9B91B'),
         ...getAddress(mantle, ZERO_ADDRESS)

deploy/scripts/mainnet/0016-CarbonVortex-upgrade.ts

@@ -0,0 +1,25 @@
+import { DeployFunction } from 'hardhat-deploy/types';
+import { HardhatRuntimeEnvironment } from 'hardhat/types';
+import { DeployedContracts, upgradeProxy, InstanceName, setDeploymentMetadata } from '../../../utils/Deploy';
+import { NATIVE_TOKEN_ADDRESS } from '../../../utils/Constants';
+
+/**
+ * upgrade carbon vortex 2.0 to v2:
+ * add maxInput to trade function
+ * fix upgradeable contract
+ */
+const func: DeployFunction = async ({ getNamedAccounts }: HardhatRuntimeEnvironment) => {
+    const { deployer, bnt, vault, oldVortex } = await getNamedAccounts();
+    const carbonController = await DeployedContracts.CarbonController.deployed();
+
+    await upgradeProxy({
+        name: InstanceName.CarbonVortex,
+        from: deployer,
+        args: [carbonController.address, vault, oldVortex, bnt, NATIVE_TOKEN_ADDRESS, bnt],
+        checkVersion: false
+    });
+
+    return true;
+};
+
+export default setDeploymentMetadata(__filename, func);

deploy/scripts/network/0003-CarbonController.ts

@@ -25,7 +25,8 @@ const func: DeployFunction = async ({ getNamedAccounts }: HardhatRuntimeEnvironm
     await upgradeProxy({
         name: InstanceName.CarbonController,
         from: deployer,
-        args: [voucher.address, carbonController.address]
+        args: [voucher.address, carbonController.address],
+        checkVersion: false
     });
 
     // Set the carbon controller address in the voucher contract

deploy/tests/mainnet/0016-carbon-vortex-upgrade.ts

@@ -0,0 +1,36 @@
+import { CarbonVortex, ProxyAdmin } from '../../../components/Contracts';
+import { DeployedContracts, describeDeployment } from '../../../utils/Deploy';
+import { expect } from 'chai';
+import { ethers } from 'hardhat';
+
+describeDeployment(__filename, () => {
+    let proxyAdmin: ProxyAdmin;
+    let carbonVortex: CarbonVortex;
+
+    beforeEach(async () => {
+        proxyAdmin = await DeployedContracts.ProxyAdmin.deployed();
+        carbonVortex = await DeployedContracts.CarbonVortex.deployed();
+    });
+
+    it('should deploy and configure the carbon vortex contract', async () => {
+        expect(await proxyAdmin.getProxyAdmin(carbonVortex.address)).to.equal(proxyAdmin.address);
+        expect(await carbonVortex.version()).to.equal(2);
+    });
+
+    it('carbon vortex implementation should be initialized', async () => {
+        const implementationAddress = await proxyAdmin.getProxyImplementation(carbonVortex.address);
+        const carbonControllerImpl: CarbonVortex = await ethers.getContractAt(
+            'CarbonVortex',
+            implementationAddress
+        );
+        // hardcoding gas limit to avoid gas estimation attempts (which get rejected instead of reverted)
+        const tx = await carbonControllerImpl.initialize({ gasLimit: 6000000 });
+        await expect(tx.wait()).to.be.reverted;
+    });
+
+    it('cannot call postUpgrade on carbon vortex', async () => {
+        // hardcoding gas limit to avoid gas estimation attempts (which get rejected instead of reverted)
+        const tx = await carbonVortex.postUpgrade(true, "0x", { gasLimit: 6000000 });
+        await expect(tx.wait()).to.be.reverted;
+    });
+});

deploy/tests/mainnet/carbon.ts

@@ -47,16 +47,18 @@ import { ethers, getNamedAccounts } from 'hardhat';
     let carbonVortex: CarbonVortex;
 
     let daoMultisig: SignerWithAddress;
+    let oldVortex: string;
 
     shouldHaveGap('CarbonController');
     shouldHaveGap('Pairs', '_lastPairId');
     shouldHaveGap('Strategies', '_strategyCounter');
     shouldHaveGap('Voucher', '_useGlobalURI');
-    shouldHaveGap('CarbonVortex', '_totalBurned');
+    shouldHaveGap('CarbonVortex', '_totalCollected');
     shouldHaveGap('CarbonPOL', '_marketPriceMultiply');
 
     before(async () => {
         ({ daoMultisig } = await getNamedSigners());
+        ({ oldVortex } = await getNamedAccounts());
     });
 
     beforeEach(async () => {
@@ -74,8 +76,11 @@ import { ethers, getNamedAccounts } from 'hardhat';
             await expectRoleMembers(voucher, Roles.Upgradeable.ROLE_ADMIN, [daoMultisig.address]);
             await expectRoleMembers(carbonVortex, Roles.Upgradeable.ROLE_ADMIN, [daoMultisig.address]);
 
-            // expect fee burner to have fee manager role in Carbon
-            await expectRoleMembers(carbonController, Roles.CarbonController.ROLE_FEES_MANAGER, [carbonVortex.address]);
+            // expect carbon vortex to have fee manager role in Carbon
+            await expectRoleMembers(carbonController, Roles.CarbonController.ROLE_FEES_MANAGER, [
+                oldVortex,
+                carbonVortex.address
+            ]);
 
             // expect carbonController to have minter role in voucher
             await expectRoleMembers(voucher, Roles.Voucher.ROLE_MINTER, [carbonController.address]);

deploy/tests/network/0001-proxy-admin.ts

@@ -16,6 +16,6 @@ describeDeployment(__filename, () => {
     });
 
     it('should deploy and configure the proxy admin contract', async () => {
-        await expect(await proxyAdmin.owner()).to.equal(deployer);
+        expect(await proxyAdmin.owner()).to.equal(deployer);
     });
 });

deploy/tests/network/0002-voucher.ts

@@ -24,4 +24,10 @@ describeDeployment(__filename, () => {
         const tx = await voucherImpl.initialize(true, '1', '1', { gasLimit: 6000000 });
         await expect(tx.wait()).to.be.reverted;
     });
+
+    it('cannot call postUpgrade on voucher', async () => {
+        // hardcoding gas limit to avoid gas estimation attempts (which get rejected instead of reverted)
+        const tx = await voucher.postUpgrade(true, '0x', { gasLimit: 6000000 });
+        await expect(tx.wait()).to.be.reverted;
+    });
 });

deploy/tests/network/0003-carbon-controller.ts

@@ -27,4 +27,10 @@ describeDeployment(__filename, () => {
         const tx = await carbonControllerImpl.initialize({ gasLimit: 6000000 });
         await expect(tx.wait()).to.be.reverted;
     });
+
+    it('cannot call postUpgrade on carbon controller', async () => {
+        // hardcoding gas limit to avoid gas estimation attempts (which get rejected instead of reverted)
+        const tx = await carbonController.postUpgrade(true, '0x', { gasLimit: 6000000 });
+        await expect(tx.wait()).to.be.reverted;
+    });
 });

package.json

@@ -32,7 +32,7 @@
     "test:nightly": "NIGHTLY=1 CI=1 forge test",
     "test:deploy": "TEST_FORK=1 ./deployments/run-fork.sh HARDHAT_NETWORK=tenderly mocha --require hardhat/register --extension ts --recursive --exit --timeout 600000 --bail --no-exit 'deploy/tests/network/**/*.ts'",
     "test:deploy:mainnet": "TEST_FORK=1 ./deployments/run-fork.sh HARDHAT_NETWORK=tenderly mocha --require hardhat/register --extension ts --recursive --exit --timeout 600000 --bail --no-exit 'deploy/tests/mainnet/**/*.ts'",
-    "test:health": "pnpm test:deploy",
+    "test:health": "pnpm test:deploy:mainnet",
     "export:storage": "pnpm cleanbuild && hardhat run deployments/storage-layout.ts",
     "deploy:prepare": "rm -rf ./node_modules && rm pnpm-lock.yaml && pnpm install && pnpm cleanbuild",
     "deploy:network": "./deployments/run-network.sh hardhat deploy",

test/forge/CarbonVortex.t.sol

@@ -188,7 +188,7 @@ contract CarbonVortexTest is TestFixture {
 
     function testShouldBeInitialized() public view {
         uint16 version = carbonVortex.version();
-        assertEq(version, 1);
+        assertEq(version, 2);
     }
 
     function testShouldntBeAbleToReinitialize() public {

test/utility/Upgradeable.ts

@@ -51,16 +51,16 @@ describe('Upgradeable', () => {
             });
 
             it('should allow executing the post-upgrade callback', async () => {
-                await expect(upgradeable.postUpgrade([])).not.to.be.reverted;
+                await expect(upgradeable.postUpgrade(true, [])).not.to.be.reverted;
 
                 await upgradeable.setVersion((await upgradeable.version()) + 1);
 
-                await expect(upgradeable.postUpgrade([])).not.to.be.reverted;
+                await expect(upgradeable.postUpgrade(true, [])).not.to.be.reverted;
             });
 
             it('should not allow executing the post-upgrade callback twice per-version', async () => {
-                await expect(upgradeable.postUpgrade([])).not.to.be.reverted;
-                await expect(upgradeable.postUpgrade([])).to.be.revertedWithError('AlreadyInitialized');
+                await expect(upgradeable.postUpgrade(true, [])).not.to.be.reverted;
+                await expect(upgradeable.postUpgrade(true, [])).to.be.revertedWithError('AlreadyInitialized');
             });
         });
 
@@ -73,7 +73,7 @@ describe('Upgradeable', () => {
                 });
 
                 it('should revert when attempting to execute the post-upgrade callback', async () => {
-                    await expect(upgradeable.postUpgrade([])).to.be.revertedWithError('AlreadyInitialized');
+                    await expect(upgradeable.postUpgrade(true, [])).to.be.revertedWithError('AlreadyInitialized');
                 });
             }
         });

utils/Deploy.ts

@@ -329,6 +329,7 @@ export const deployProxy = async (options: DeployOptions, proxy: ProxyOptions =
 // ]
 interface UpgradeProxyOptions extends DeployOptions {
     postUpgradeArgs?: TypedParam[];
+    checkVersion?: boolean;
 }
 
 export const upgradeProxy = async (options: UpgradeProxyOptions) => {
@@ -351,16 +352,17 @@ export const upgradeProxy = async (options: UpgradeProxyOptions) => {
         const values = postUpgradeArgs.map(({ value }) => value);
         const abiCoder = new AbiCoder();
 
-        upgradeCallData = [abiCoder.encode(types, values)];
+        upgradeCallData = abiCoder.encode(types, values);
     } else {
-        upgradeCallData = [ZERO_BYTES];
+        upgradeCallData = ZERO_BYTES;
     }
+    const checkVersion = options.checkVersion ?? true;
 
     const proxyOptions = {
         proxyContract: PROXY_CONTRACT,
         owner: await proxyAdmin.owner(),
         viaAdminContract: InstanceName.ProxyAdmin,
-        execute: { onUpgrade: { methodName: POST_UPGRADE, args: upgradeCallData } }
+        execute: { onUpgrade: { methodName: POST_UPGRADE, args: [checkVersion, upgradeCallData] } }
     };
 
     Logger.log(`  upgrading proxy ${contractName} V${prevVersion}`);