From 6870ad51ba8be897b2aaf5e0b5dea4c14b85e810 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mart=C3=ADn=20Triay?= Date: Wed, 16 Dec 2020 17:25:51 -0300 Subject: [PATCH] Fix error in unsafeAllowCustomTypes flag (#259) Co-authored-by: Francisco Giordano --- packages/core/CHANGELOG.md | 6 +++++ packages/core/src/storage.ts | 22 +++++++++-------- packages/plugin-hardhat/CHANGELOG.md | 6 +++++ .../plugin-hardhat/contracts/PortfolioV2.sol | 24 +++++++++++++++++++ .../test/happy-path-with-structs.js | 10 ++++++++ packages/plugin-truffle/CHANGELOG.md | 6 +++++ .../test/contracts/PortfolioV2.sol | 24 +++++++++++++++++++ .../test/test/happy-path-with-structs.js | 8 +++++++ 8 files changed, 96 insertions(+), 10 deletions(-) diff --git a/packages/core/CHANGELOG.md b/packages/core/CHANGELOG.md index 08c630153..89246a63c 100644 --- a/packages/core/CHANGELOG.md +++ b/packages/core/CHANGELOG.md @@ -1,5 +1,11 @@ # Changelog +## Unreleased + +- Fix an error in the `unsafeAllowCustomTypes` flag that would cause other storage layout incompatibilities to be ignored. ([#259](https://github.com/OpenZeppelin/openzeppelin-upgrades/pull/259)) + +Users of this flag are advised to update to this version. + ## 1.4.1 (2020-11-30) - Fix a problem in deployment logic when used with Hyperledger Besu. ([#244](https://github.com/OpenZeppelin/openzeppelin-upgrades/pull/244)) diff --git a/packages/core/src/storage.ts b/packages/core/src/storage.ts index 53f906135..702e8adf4 100644 --- a/packages/core/src/storage.ts +++ b/packages/core/src/storage.ts @@ -67,16 +67,8 @@ export function assertStorageUpgradeSafe( let errors = getStorageUpgradeErrors(original, updated); if (unsafeAllowCustomTypes) { - errors = errors - .filter(error => error.kind === 'typechange') - .filter(error => { - const { original, updated } = error; - if (original && updated) { - // Skip storage errors if the only difference seems to be the AST id number - return stabilizeTypeIdentifier(original?.type) !== stabilizeTypeIdentifier(updated?.type); - } - return error; - }); + // Types with the same name are assumed compatible + errors = errors.filter(error => !isTypechange(error) || typechangePreservesNames(error)); } if (errors.length > 0) { @@ -84,6 +76,16 @@ export function assertStorageUpgradeSafe( } } +function isTypechange(error: StorageOperation): error is StorageOperation & { kind: 'typechange' } { + return error.kind === 'typechange'; +} + +function typechangePreservesNames(error: StorageOperation & { kind: 'typechange' }): boolean { + assert(error.updated !== undefined); + assert(error.original !== undefined); + return stabilizeTypeIdentifier(error.original.type) !== stabilizeTypeIdentifier(error.updated.type); +} + class StorageUpgradeErrors extends UpgradesError { constructor(readonly errors: StorageOperation[]) { super(`New storage layout is incompatible due to the following changes`, () => { diff --git a/packages/plugin-hardhat/CHANGELOG.md b/packages/plugin-hardhat/CHANGELOG.md index 7fe589018..9ea9f8685 100644 --- a/packages/plugin-hardhat/CHANGELOG.md +++ b/packages/plugin-hardhat/CHANGELOG.md @@ -1,5 +1,11 @@ # Changelog +## Unreleased + +- Fix an error in the `unsafeAllowCustomTypes` flag that would cause other storage layout incompatibilities to be ignored. ([#259](https://github.com/OpenZeppelin/openzeppelin-upgrades/pull/259)) + +Users of this flag are advised to update to this version. + ## 1.4.2 (2020-12-04) - Fix a bug that prevented some solc errors from reaching the console. ([#253](https://github.com/OpenZeppelin/openzeppelin-upgrades/pull/253)) diff --git a/packages/plugin-hardhat/contracts/PortfolioV2.sol b/packages/plugin-hardhat/contracts/PortfolioV2.sol index 317011bc6..35b3d195c 100644 --- a/packages/plugin-hardhat/contracts/PortfolioV2.sol +++ b/packages/plugin-hardhat/contracts/PortfolioV2.sol @@ -24,3 +24,27 @@ contract PortfolioV2 { } } + +contract PortfolioV2Bad { + struct Asset { + bool enabled; + uint amount; + } + + uint insert; + mapping (string => Asset) assets; + + function initialize() public view { + console.log("Deploying PortfolioV2"); + } + + function enable(string memory name) public returns (bool) { + if (assets[name].enabled) { + return false; + } else { + assets[name] = Asset(true, 10); + return true; + } + } + +} diff --git a/packages/plugin-hardhat/test/happy-path-with-structs.js b/packages/plugin-hardhat/test/happy-path-with-structs.js index f4042111b..dff987772 100644 --- a/packages/plugin-hardhat/test/happy-path-with-structs.js +++ b/packages/plugin-hardhat/test/happy-path-with-structs.js @@ -7,6 +7,7 @@ upgrades.silenceWarnings(); test.before(async t => { t.context.Portfolio = await ethers.getContractFactory('Portfolio'); t.context.PortfolioV2 = await ethers.getContractFactory('PortfolioV2'); + t.context.PortfolioV2Bad = await ethers.getContractFactory('PortfolioV2Bad'); }); test('deployProxy without flag', async t => { @@ -32,3 +33,12 @@ test('upgradeProxy with flag', async t => { const portfolio2 = await upgrades.upgradeProxy(portfolio.address, PortfolioV2, { unsafeAllowCustomTypes: true }); await portfolio2.enable('ETH'); }); + +test('upgradeProxy with flag but incompatible layout', async t => { + const { Portfolio, PortfolioV2Bad } = t.context; + const portfolio = await upgrades.deployProxy(Portfolio, [], { unsafeAllowCustomTypes: true }); + const error = await t.throwsAsync(() => + upgrades.upgradeProxy(portfolio.address, PortfolioV2Bad, { unsafeAllowCustomTypes: true }), + ); + t.true(error.message.includes('Variable `assets` was replaced with `insert`')); +}); diff --git a/packages/plugin-truffle/CHANGELOG.md b/packages/plugin-truffle/CHANGELOG.md index 3b35bb20c..781312cdf 100644 --- a/packages/plugin-truffle/CHANGELOG.md +++ b/packages/plugin-truffle/CHANGELOG.md @@ -1,5 +1,11 @@ # Changelog +## Unreleased + +- Fix an error in the `unsafeAllowCustomTypes` flag that would cause other storage layout incompatibilities to be ignored. ([#259](https://github.com/OpenZeppelin/openzeppelin-upgrades/pull/259)) + +Users of this flag are advised to update to this version. + ## 1.3.0 (2020-11-24) - Add `silenceWarnings` to emit a single warning and silence all subsequent ones. ([#228](https://github.com/OpenZeppelin/openzeppelin-upgrades/pull/228)) diff --git a/packages/plugin-truffle/test/contracts/PortfolioV2.sol b/packages/plugin-truffle/test/contracts/PortfolioV2.sol index 317011bc6..35b3d195c 100644 --- a/packages/plugin-truffle/test/contracts/PortfolioV2.sol +++ b/packages/plugin-truffle/test/contracts/PortfolioV2.sol @@ -24,3 +24,27 @@ contract PortfolioV2 { } } + +contract PortfolioV2Bad { + struct Asset { + bool enabled; + uint amount; + } + + uint insert; + mapping (string => Asset) assets; + + function initialize() public view { + console.log("Deploying PortfolioV2"); + } + + function enable(string memory name) public returns (bool) { + if (assets[name].enabled) { + return false; + } else { + assets[name] = Asset(true, 10); + return true; + } + } + +} diff --git a/packages/plugin-truffle/test/test/happy-path-with-structs.js b/packages/plugin-truffle/test/test/happy-path-with-structs.js index 07caff650..e7698aa1d 100644 --- a/packages/plugin-truffle/test/test/happy-path-with-structs.js +++ b/packages/plugin-truffle/test/test/happy-path-with-structs.js @@ -4,6 +4,7 @@ const { deployProxy, upgradeProxy } = require('@openzeppelin/truffle-upgrades'); const Portfolio = artifacts.require('Portfolio'); const PortfolioV2 = artifacts.require('PortfolioV2'); +const PortfolioV2Bad = artifacts.require('PortfolioV2Bad'); contract('PortfolioWithoutFlag', function () { it('deployProxy', async function () { @@ -20,4 +21,11 @@ contract('PortfolioWithFlag', function () { const portfolio = await deployProxy(Portfolio, [], { unsafeAllowCustomTypes: true }); await upgradeProxy(portfolio.address, PortfolioV2, { unsafeAllowCustomTypes: true }); }); + + it('upgradeProxy with flag but incompatible layout', async function () { + const portfolio = await deployProxy(Portfolio, [], { unsafeAllowCustomTypes: true }); + await assert.rejects(upgradeProxy(portfolio.address, PortfolioV2Bad, { unsafeAllowCustomTypes: true }), error => + error.message.includes('Variable `assets` was replaced with `insert`'), + ); + }); });