From 8c42f067648e6028e674117a279a805271076c3c Mon Sep 17 00:00:00 2001 From: Eric Lau Date: Wed, 2 Oct 2024 10:49:02 -0400 Subject: [PATCH] Fix Hardhat compile error when library or interface has namespace struct, print warning for library (#1086) --- packages/core/CHANGELOG.md | 8 +++ packages/core/contracts/test/Namespaced.sol | 62 +++++++++++++++++- .../contracts/test/NamespacedInLibrary.sol | 28 ++++++++ .../contracts/test/NamespacedToModify.sol | 19 +++++- packages/core/package.json | 2 +- ...torage-namespaced-outside-contract.test.ts | 31 ++++++--- packages/core/src/storage-namespaced.test.ts | 26 ++++++++ .../core/src/utils/make-namespaced.test.ts.md | 40 ++++++++++- .../src/utils/make-namespaced.test.ts.snap | Bin 2665 -> 2753 bytes packages/core/src/utils/make-namespaced.ts | 4 ++ packages/core/src/validate/run.ts | 34 ++++++++-- 11 files changed, 234 insertions(+), 20 deletions(-) create mode 100644 packages/core/contracts/test/NamespacedInLibrary.sol 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 404ae3a147a70a746f84567873bdba4e10623fe9..299d67b4d7f1453ce3055c325863e85d1889a418 100644 GIT binary patch literal 2753 zcmV;y3O@BgRzV$FX(?b+XbpE!nN61R)@2(oaO*gdQf z9T`^7bqw3WJ$sir_IKXXb?o2==gquwTH!zHv^up0e?Jcof#=)sd|G|(CH1+j_BV*; zsA~9n_Uu_jeL+$4iaHB4xtICdsh8Ag6I(kj-XUQ_gPMkJI0Hp}QB~gsiD8dT9LKPB zY|-;aspqs!U8WIRDD*Gr_%}RARg{}*Orcyaz6Gj3_Vx*iPyDE{rs!_{fSaa+p zhuF?iJ#Ok=YTX`X4$(b!Fi0QR-+||+xtKxnaogoS0FvLu z$)*xye*=;a<78(NWIuaFQGWhPjBF<1g!I0;{pIx)ytZ7rrmd{6uNGHJtJ*4Fzt+Lar8aJtaC@z|rmeQu^dh;gY1db@ zqE=jM<7+F$BEjuz#iDQe&`_-%;z%ond%#z!=Q4sAaa?BEXs%7EiLT6t=8g!s-!vc* zKbVtDy>F0*QSKqH&@2PMy>v4t5MyoR$u|5*JaHhp;FiiJmIi} zOry;pTcb^*EtC#5a?`ZPw2;i%GhKVj@^TF7pUAcqn3LNjY%Dt_w3dx|EX@UX#kc42 zIX699jy-f%I0qW=?I!SmutbW-o7?3|vvm^J%i+ge)_h2#E=!s5Oq^#IO#Mh)3*L@3 z$mR}NDKmBq$jIiPPB*CL@@*K)h8AOujFzmGOxcFPc&)!r>=VWvER?Z>-cyu^YL&kn zz$PwU!mf%ald6nSw&gmup_59Wu%rVFw=VCx_gjtHSE{WFxW1HH-g=cnnK3-LYm+e6 z#EiM}%WG+yxhEEcE_?f~UwG81Gd#KYb(Sy0FR}ch;j4j&w}V0}-qEVOjWpgR3_Dh# z=z{)*K*^aSs_t0Doz!VCN~h5UIgcz|XT-K8yFF-?H#WAbjRsiG@0}|`=_H~=UdH#T z+s*2RMD1CzI5!jlY%`)L>433OZ9_!BLDt?x?6R*7H-S&>$D^!YkV0Tck0?6gs2!K5 zsBQ917gmnGEyCP4c~Ff9C29YrzsE;5-UiZOJ@eZOWJ@JiMk+Xz6Ui>NZ@Z>rK*{P2 zY^QFO3rNaCY0Nx+^`j&p?8!jw6e&JqkdBC6w(nquyW@#UHEkU`jULgsWn&ZcO&c|1 z(hfB%NEE}`@MyyvDOEPmc%tLeOckGLxf6|X7+D)&#cJP+rF&4lN2;BcL5{T%hIa?(hiW;ySd7)7tZO5OLIXkJ?2dKV*d`sq;K(P& z3ATQag|HQq7%wfnLZVi0R=01JE7jJmyY&h`!pXHo)?DK?W|}hoqyszX;0y-MzQ6Y| zGx+7v$xj6vq-WBB9fVSo*1BES;WB+sob;^0*QSBZV6Krx`S?y=L!0fmCYFMu?K-H3 z2cdD~C3VMZmzIs!5H7;Flkb!tl)t!Hm3`_!bQXrPqV=MDTHt?qjtckxr+Q%)O8~*! zGo|2IcbR`yPJWkZh$~}skZ~GV2+I70VHLh`M45SrIt+sHav|eV`|{D(r+mWGYW~;f z6y^8u{PkSy2KiLl4f2bs`qA(LSP>0p&PQ*Ar`22mnpV&I0#AuzcK9|sJ1dCZ0)l#s z=yZf=M)>1-?vHSUJS@ueo)`RR*~A0tIzBklpG!q&&?4F+q=%rofj-CPNh1N+Txw1L zn+ag^_yL>mT~L(oU-)kUHV-cz8?gDo`3Yb%0c@ToU?b*bQ3t}$Q_N4&V*ojVwa1St zP_OD#v&TnvMzcLaseHPKIU}XDlzwn|T&Qb`GNJjR%|jcG z(C6{swX$L{w?im5CkP4Y_s6^5GO-e(Ei;^?_X4uU&8aj#W8{Dzg&y~)Bn7f0H!~4Nl`9b8Uaiffne>@lLn?y7BCGK z(onH*JqbupZ9EHr^dB++>HNpg@eg=DH4==@EdWVrG!X4Mb}t)X{;@g&VD>Q3-%4bk z9f=eduZI}%N5J?oJkz5w;#Yy>bE7fhF96BL2#omgfxkQXV}rjr^|*-yVzsaa>1vES z@X?p;&%`CS@coa#cJIRTtNfS==r{RT0{VMCk$|3?Qk3VXMkF8v1g}j!sRZ;NU_vhM Hz&ro|2EtjF literal 2665 zcmV-v3YPUjRzV9?oir_yUWh*%4AffUw=V+C{mz@rs$=ALV*@NTOfr}w*Z&s6rUpZuEGn%yIgj`s;NuuiN#R)~r; zqwm<7W#GQGXWG_}-d9y@<40E(3b_k8_9IP8Q)}?|EAS9_egMx)()kyq^V{-wiDFAq z@OtUerJVFaPAcT28KB93k-eRJQJU^yW7omEBxq=vJx$f@VNSXzN$-KgpvNM%tr@!( z@A;F^bHOqlst}7S^e^c6H#|tnGmO7>ig?CqCxIrZfTSuy5?e8dYEh=JuEQTca%Cek}a9mC=uQ~5vVxB!V; zY5VfXO44<|BP^L2j~KT1ohL6XEuq%EjfabynnDbVEH+f=*18&@x6oFjUF=i5t7Bw& zA&lNGm)1*_^6g>~%NF+o@*RTe1FZLZ1RWL;{49a!Hx$RRP5l=90y$(6Rwrg5fe2M@ zR?5q_h|5yav1V{Ckm-P^E%x zIg7v)RZNGXhAqD_T}>QUQ})%2v0sa}Jb}WGHG8krHG4htF=UsLV(L10)sq9h)Cm&X z{294ozX{AWxA&xTiVL_FZwt?h|LQsh&lIR@nsf80@5nt(L5D{vB+GMbXoRyVD*+|^ zA=JPbxoj@zwcxw1}4B z_v`3gc?c#OC}%Br%8*Kc(3az(~+1S;2C zY>hTGnJXP=O!_6^ha4Or@U`~!s=va1)X)PM_M4EH%nrF|Gb8fn} z9DC@raP~D|+l}u5Zixhsw|AAS3sNO>@Ik9JUQ((a?OX%xFnl$tK;$!>xR>lGeN(;l5%ygJJh;+0rl(eQ0w#9Mx$747KM-a-oN;)WfoP;@~5 zlCNa(n5sKcamRHUX6ck&kh92ARZ1*Nu-l_fbz@_v-fDs6yxzGolukTK#AW=TzSFL6 z2-L0>%X1?Uz*gi%Nf(Tb8XG(U4%7A~VpTnDm;FCInx{1gI1xpKOGUZYTd&W>*I+^G;I^xtv*qhWg`>xOdB?1+zt)H zPZXot@OZ-!@CXiBem>3%*X1QRJV|ALIXj;>`rvp*dbj);mF6w@wa}E zg|HEk7%eT_LZZ=Z*LUt#YxU0E`^_3V!ilv-+FYYGW|A`AqIPF=3txXO0{JDk}<=LIQMmF1ZdRPb!*|AX{4+G8f53E=Qq0uFp$hG|3ixr%v7dJG_fv-Vl34fU34D%SYOL^kULs(4nRicrKb41z-t ze%QoFybfNLLpA=y5XaK>>#jks3!_6X;{WU5buGnc0}8|zC+v`+KxZ7$2uuJ?QsqU2 z`9!G;D`~t007e+^4V4_gT5&TFBpk|*U#v=Ht= z38pi_be;~T!?V>W>5WhpcKxogzDp6w!`CmgcJYK!CMx=aA-QmGs?BOc>uRvA)4ETis!u%aOupLwHLo@aW z)oix8dWV_7H!gu!fn)E}b1@d|;&-_Uc%MthfMj9-L^fE{y$1^Iu)#oH?HK&!t;A+~ zj6dE4d7B__6XY!=33v;Xi(ZbkRKmIS6d+w>9E`dewQR9TM()i*>QE#mE?oVS)qmr$^_;MH;-(Xq0iaijiO=^w}x6X)tcax6P)sMgi{KK zY@YntF2LVLz21Uljo!xMw!q{A!U@RvxdJ&qc{P{&6rOXlV}hJlW+Ncyo!J=3DMQoV zYzD~reIWSp?9&E0Q5whz7E(~TbTbZaPHj93;N}M@;AY`t==di*U&sVD^9w*y$p$rD z$L^7; @@ -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,