Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix upgradeable contract and health tests #162

Merged
merged 10 commits into from
Aug 28, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions contracts/helpers/TestUpgradeable.sol
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
9 changes: 6 additions & 3 deletions contracts/utility/Upgradeable.sol
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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) {
lbeder marked this conversation as resolved.
Show resolved Hide resolved
initializations = _version;
}

_initializations = initializations;
lbeder marked this conversation as resolved.
Show resolved Hide resolved
Expand Down
2 changes: 1 addition & 1 deletion contracts/vortex/CarbonVortex.sol
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}

/**
Expand Down
2 changes: 1 addition & 1 deletion data/named-accounts.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
25 changes: 25 additions & 0 deletions deploy/scripts/mainnet/0016-CarbonVortex-upgrade.ts
Original file line number Diff line number Diff line change
@@ -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);
3 changes: 2 additions & 1 deletion deploy/scripts/network/0003-CarbonController.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
36 changes: 36 additions & 0 deletions deploy/tests/mainnet/0016-carbon-vortex-upgrade.ts
Original file line number Diff line number Diff line change
@@ -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;
});
});
11 changes: 8 additions & 3 deletions deploy/tests/mainnet/carbon.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 () => {
Expand All @@ -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]);
Expand Down
2 changes: 1 addition & 1 deletion deploy/tests/network/0001-proxy-admin.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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);
});
});
6 changes: 6 additions & 0 deletions deploy/tests/network/0002-voucher.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
});
});
6 changes: 6 additions & 0 deletions deploy/tests/network/0003-carbon-controller.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
});
});
2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
2 changes: 1 addition & 1 deletion test/forge/CarbonVortex.t.sol
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
10 changes: 5 additions & 5 deletions test/utility/Upgradeable.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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');
});
});

Expand All @@ -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');
});
}
});
Expand Down
8 changes: 5 additions & 3 deletions utils/Deploy.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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) => {
Expand All @@ -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}`);
Expand Down
Loading