diff --git a/packages/core/CHANGELOG.md b/packages/core/CHANGELOG.md index dda1a933f..a243e2590 100644 --- a/packages/core/CHANGELOG.md +++ b/packages/core/CHANGELOG.md @@ -1,5 +1,13 @@ # Changelog +## 1.39.0 (2024-10-02) + +- Fix Hardhat compile error when library or interface has struct with namespace annotation. ([#1086](https://github.com/OpenZeppelin/openzeppelin-upgrades/pull/1086)) +- Log warning if library contains namespace annotation. ([#1086](https://github.com/OpenZeppelin/openzeppelin-upgrades/pull/1086)) + +> **Note** +> Namespaces should be defined in contracts according to [ERC-7201: Namespaced Storage Layouts](https://eips.ethereum.org/EIPS/eip-7201). Structs with namespace annotations defined in libraries or interfaces outside of a contract's inheritance are not included in storage layout validations. + ## 1.38.0 (2024-09-23) - Supports checking to ensure `initialOwner` is not a ProxyAdmin contract when deploying a transparent proxy from Hardhat. ([#1083](https://github.com/OpenZeppelin/openzeppelin-upgrades/pull/1083)) diff --git a/packages/core/contracts/test/Namespaced.sol b/packages/core/contracts/test/Namespaced.sol index 8f33d4f67..b1bc986b5 100644 --- a/packages/core/contracts/test/Namespaced.sol +++ b/packages/core/contracts/test/Namespaced.sol @@ -178,4 +178,64 @@ contract MultipleNamespacesAndRegularVariablesV2_Bad { uint256 public c; uint128 public a; uint256 public b; -} \ No newline at end of file +} + +interface InterfaceWithNamespace { + /// @custom:storage-location erc7201:example.main + struct MainStorage { + uint256 x; + uint256 y; + } +} + +contract UsesInterface is InterfaceWithNamespace { + // keccak256(abi.encode(uint256(keccak256("example.main")) - 1)) & ~bytes32(uint256(0xff)); + bytes32 internal constant MAIN_STORAGE_LOCATION = + 0x183a6125c38840424c4a85fa12bab2ab606c4b6d0e7cc73c0c06ba5300eab500; + + function _getMainStorage() internal pure returns (MainStorage storage $) { + assembly { + $.slot := MAIN_STORAGE_LOCATION + } + } +} + +interface InterfaceWithNamespaceV2_Ok { + /// @custom:storage-location erc7201:example.main + struct MainStorage { + uint256 x; + uint256 y; + uint256 z; + } +} + +contract UsesInterfaceV2_Ok is InterfaceWithNamespaceV2_Ok { + // keccak256(abi.encode(uint256(keccak256("example.main")) - 1)) & ~bytes32(uint256(0xff)); + bytes32 internal constant MAIN_STORAGE_LOCATION = + 0x183a6125c38840424c4a85fa12bab2ab606c4b6d0e7cc73c0c06ba5300eab500; + + function _getMainStorage() internal pure returns (MainStorage storage $) { + assembly { + $.slot := MAIN_STORAGE_LOCATION + } + } +} + +interface InterfaceWithNamespaceV2_Bad { + /// @custom:storage-location erc7201:example.main + struct MainStorage { + uint256 y; + } +} + +contract UsesInterfaceV2_Bad is InterfaceWithNamespaceV2_Bad { + // keccak256(abi.encode(uint256(keccak256("example.main")) - 1)) & ~bytes32(uint256(0xff)); + bytes32 internal constant MAIN_STORAGE_LOCATION = + 0x183a6125c38840424c4a85fa12bab2ab606c4b6d0e7cc73c0c06ba5300eab500; + + function _getMainStorage() internal pure returns (MainStorage storage $) { + assembly { + $.slot := MAIN_STORAGE_LOCATION + } + } +} diff --git a/packages/core/contracts/test/NamespacedInLibrary.sol b/packages/core/contracts/test/NamespacedInLibrary.sol new file mode 100644 index 000000000..95c54dd20 --- /dev/null +++ b/packages/core/contracts/test/NamespacedInLibrary.sol @@ -0,0 +1,28 @@ +// SPDX-License-Identifier: MIT +pragma solidity ^0.8.20; + +/// This is not considered a namespace according to ERC-7201 because the namespaced struct is outside of a contract. +library LibraryWithNamespace { + /// @custom:storage-location erc7201:example.main + struct MainStorage { + uint256 x; + uint256 y; + } +} + +contract UsesLibraryWithNamespace { + // keccak256(abi.encode(uint256(keccak256("example.main")) - 1)) & ~bytes32(uint256(0xff)); + bytes32 private constant MAIN_STORAGE_LOCATION = + 0x183a6125c38840424c4a85fa12bab2ab606c4b6d0e7cc73c0c06ba5300eab500; + + function _getMainStorage() private pure returns (LibraryWithNamespace.MainStorage storage $) { + assembly { + $.slot := MAIN_STORAGE_LOCATION + } + } + + function _getXTimesY() internal view returns (uint256) { + LibraryWithNamespace.MainStorage storage $ = _getMainStorage(); + return $.x * $.y; + } +} diff --git a/packages/core/contracts/test/NamespacedToModify.sol b/packages/core/contracts/test/NamespacedToModify.sol index 526f70258..d94418e43 100644 --- a/packages/core/contracts/test/NamespacedToModify.sol +++ b/packages/core/contracts/test/NamespacedToModify.sol @@ -304,4 +304,21 @@ contract HasSpecialFunctions { } bytes4 constant PAYABLE_SELECTOR = this.hasPayable.selector; -} \ No newline at end of file +} + +/// This is not considered a namespace according to ERC-7201 because the namespaced struct is outside of a contract. +library LibraryWithNamespace { + /// @custom:storage-location erc7201:example.main + struct MainStorage { + uint256 x; + uint256 y; + } +} + +interface InterfaceWithNamespace { + /// @custom:storage-location erc7201:example.main + struct MainStorage { + uint256 x; + uint256 y; + } +} diff --git a/packages/core/package.json b/packages/core/package.json index 929136ede..4fc426472 100644 --- a/packages/core/package.json +++ b/packages/core/package.json @@ -1,6 +1,6 @@ { "name": "@openzeppelin/upgrades-core", - "version": "1.38.0", + "version": "1.39.0", "description": "", "repository": "https://github.com/OpenZeppelin/openzeppelin-upgrades/tree/master/packages/core", "license": "MIT", diff --git a/packages/core/src/storage-namespaced-outside-contract.test.ts b/packages/core/src/storage-namespaced-outside-contract.test.ts index 98f6a2662..7074981ed 100644 --- a/packages/core/src/storage-namespaced-outside-contract.test.ts +++ b/packages/core/src/storage-namespaced-outside-contract.test.ts @@ -4,16 +4,11 @@ import { artifacts } from 'hardhat'; import { validate } from './validate'; import { solcInputOutputDecoder } from './src-decoder'; -test('namespace outside contract', async t => { +test('namespace outside contract - error', async t => { const contract = 'contracts/test/NamespacedOutsideContract.sol:Example'; - const buildInfo = await artifacts.getBuildInfo(contract); - if (buildInfo === undefined) { - throw new Error(`Build info not found for contract ${contract}`); - } - const solcOutput = buildInfo.output; - const solcInput = buildInfo.input; - const decodeSrc = solcInputOutputDecoder(solcInput, solcOutput); + const { solcOutput, decodeSrc } = await getOutputAndDecoder(contract); + const error = t.throws(() => validate(solcOutput, decodeSrc)); t.assert( error?.message.includes( @@ -22,3 +17,23 @@ test('namespace outside contract', async t => { error?.message, ); }); + +test('namespace in library - warning', async t => { + const contract = 'contracts/test/NamespacedInLibrary.sol:UsesLibraryWithNamespace'; + + const { solcOutput, decodeSrc } = await getOutputAndDecoder(contract); + validate(solcOutput, decodeSrc); + + t.pass(); +}); + +async function getOutputAndDecoder(contract: string) { + const buildInfo = await artifacts.getBuildInfo(contract); + if (buildInfo === undefined) { + throw new Error(`Build info not found for contract ${contract}`); + } + const solcOutput = buildInfo.output; + const solcInput = buildInfo.input; + const decodeSrc = solcInputOutputDecoder(solcInput, solcOutput); + return { solcOutput, decodeSrc }; +} diff --git a/packages/core/src/storage-namespaced.test.ts b/packages/core/src/storage-namespaced.test.ts index 33204b0bc..c966d6afc 100644 --- a/packages/core/src/storage-namespaced.test.ts +++ b/packages/core/src/storage-namespaced.test.ts @@ -144,3 +144,29 @@ test('multiple namespaces and regular variables bad', t => { }, }); }); + +test('interface with namespace - upgrade ok', t => { + const v1 = t.context.extractStorageLayout('InterfaceWithNamespace'); + const v2 = t.context.extractStorageLayout('InterfaceWithNamespaceV2_Ok'); + const comparison = getStorageUpgradeErrors(v1, v2); + t.deepEqual(comparison, []); +}); + +test('interface with namespace - upgrade bad', t => { + const v1 = t.context.extractStorageLayout('InterfaceWithNamespace'); + const v2 = t.context.extractStorageLayout('InterfaceWithNamespaceV2_Bad'); + const comparison = getStorageUpgradeErrors(v1, v2); + t.like(comparison, { + length: 1, + 0: { + kind: 'delete', + original: { + contract: 'InterfaceWithNamespace', + label: 'x', + type: { + id: 't_uint256', + }, + }, + }, + }); +}); diff --git a/packages/core/src/utils/make-namespaced.test.ts.md b/packages/core/src/utils/make-namespaced.test.ts.md index 293aade7b..fbe58a3fc 100644 --- a/packages/core/src/utils/make-namespaced.test.ts.md +++ b/packages/core/src/utils/make-namespaced.test.ts.md @@ -243,7 +243,25 @@ Generated by [AVA](https://avajs.dev). function hasPayable() public payable {}␊ ␊ bytes4 constant PAYABLE_SELECTOR = this.hasPayable.selector;␊ - }`, + }␊ + ␊ + ␊ + library LibraryWithNamespace {␊ + /// @custom:storage-location erc7201:example.main␊ + struct MainStorage {␊ + uint256 x;␊ + uint256 y;␊ + }␊ + }␊ + ␊ + interface InterfaceWithNamespace {␊ + /// @custom:storage-location erc7201:example.main␊ + struct MainStorage {␊ + uint256 x;␊ + uint256 y;␊ + }␊ + }␊ + `, }, 'contracts/test/NamespacedToModifyImported.sol': { content: `// SPDX-License-Identifier: MIT␊ @@ -547,7 +565,25 @@ Generated by [AVA](https://avajs.dev). function hasPayable() public payable {}␊ ␊ bytes4 constant PAYABLE_SELECTOR = this.hasPayable.selector;␊ - }`, + }␊ + ␊ + /// This is not considered a namespace according to ERC-7201 because the namespaced struct is outside of a contract.␊ + library LibraryWithNamespace {␊ + /// @custom:storage-location erc7201:example.main␊ + struct MainStorage {␊ + uint256 x;␊ + uint256 y;␊ + }␊ + }␊ + ␊ + interface InterfaceWithNamespace {␊ + /// @custom:storage-location erc7201:example.main␊ + struct MainStorage {␊ + uint256 x;␊ + uint256 y;␊ + }␊ + }␊ + `, }, 'contracts/test/NamespacedToModifyImported.sol': { content: `// SPDX-License-Identifier: MIT␊ diff --git a/packages/core/src/utils/make-namespaced.test.ts.snap b/packages/core/src/utils/make-namespaced.test.ts.snap index 404ae3a14..299d67b4d 100644 Binary files a/packages/core/src/utils/make-namespaced.test.ts.snap and b/packages/core/src/utils/make-namespaced.test.ts.snap differ diff --git a/packages/core/src/utils/make-namespaced.ts b/packages/core/src/utils/make-namespaced.ts index ef4193fd9..c104dc31f 100644 --- a/packages/core/src/utils/make-namespaced.ts +++ b/packages/core/src/utils/make-namespaced.ts @@ -91,6 +91,10 @@ export function makeNamespacedInput(input: SolcInput, output: SolcOutput, solcVe break; } case 'StructDefinition': { + // Do not add state variable in a library or interface, since that is not allowed by Solidity + if (contractDef.contractKind !== 'contract') { + continue; + } const storageLocation = getStorageLocationAnnotation(contractNode); if (storageLocation !== undefined) { const structName = contractNode.name; diff --git a/packages/core/src/validate/run.ts b/packages/core/src/validate/run.ts index 1435d83a2..bc6ff266e 100644 --- a/packages/core/src/validate/run.ts +++ b/packages/core/src/validate/run.ts @@ -23,6 +23,7 @@ import { getAnnotationArgs as getSupportedAnnotationArgs, getDocumentation } fro import { getStorageLocationAnnotation, isNamespaceSupported } from '../storage/namespace'; import { UpgradesError } from '../error'; import { assertUnreachable } from '../utils/assert'; +import { logWarning } from '../utils/log'; export type ValidationRunData = Record; @@ -267,18 +268,37 @@ function checkNamespaceSolidityVersion(source: string, solcVersion?: string, sol function checkNamespacesOutsideContract(source: string, solcOutput: SolcOutput, decodeSrc: SrcDecoder) { for (const node of solcOutput.sources[source].ast.nodes) { if (isNodeType('StructDefinition', node)) { - const storageLocation = getStorageLocationAnnotation(node); - if (storageLocation !== undefined) { - throw new UpgradesError( - `${decodeSrc(node)}: Namespace struct ${node.name} is defined outside of a contract`, - () => - `Structs with the @custom:storage-location annotation must be defined within a contract. Move the struct definition into a contract, or remove the annotation if the struct is not used for namespaced storage.`, - ); + // Namespace struct outside contract - error + assertNotNamespace(node, decodeSrc, true); + } else if (isNodeType('ContractDefinition', node) && node.contractKind === 'library') { + // Namespace struct in library - warning (don't give an error to avoid breaking changes, since this is quite common) + for (const child of node.nodes) { + if (isNodeType('StructDefinition', child)) { + assertNotNamespace(child, decodeSrc, false); + } } } } } +function assertNotNamespace(node: StructDefinition, decodeSrc: SrcDecoder, strict: boolean) { + const storageLocation = getStorageLocationAnnotation(node); + if (storageLocation !== undefined) { + const msg = `${decodeSrc(node)}: Namespace struct ${node.name} is defined outside of a contract`; + if (strict) { + throw new UpgradesError( + msg, + () => + `Structs with the @custom:storage-location annotation must be defined within a contract. Move the struct definition into a contract, or remove the annotation if the struct is not used for namespaced storage.`, + ); + } else { + logWarning(msg, [ + 'Structs with the @custom:storage-location annotation must be defined within a contract, otherwise they are not included in storage layout validations. Move the struct definition into a contract, or remove the annotation if the struct is not used for namespaced storage.', + ]); + } + } +} + function getNamespacedCompilationContext( source: string, contractDef: ContractDefinition,