Fix Hardhat compile error when library or interface has namespace str…

…uct, print warning for library (#1086)
OpenZeppelin · Oct 2, 2024 · 8c42f06 · 8c42f06
1 parent 60dc229
commit 8c42f06
Show file tree

Hide file tree

Showing 11 changed files with 234 additions and 20 deletions.
diff --git 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
@@ -178,4 +178,64 @@ contract MultipleNamespacesAndRegularVariablesV2_Bad {
     uint256 public c;
     uint128 public a;
     uint256 public b;
-}
+}
+
+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
@@ -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
@@ -304,4 +304,21 @@ contract HasSpecialFunctions {
     }
 
     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;
+    }
+}
diff --git 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
@@ -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
@@ -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
@@ -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
diff --git 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
@@ -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<string, ContractValidation>;
 
@@ -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,