From 88e2c1bbc782f3894d6c05eb3280dce51313a57e Mon Sep 17 00:00:00 2001 From: Eric Lau Date: Thu, 18 Jul 2024 14:29:37 -0400 Subject: [PATCH] Keep constants for namespaced compilation (#1046) --- packages/core/CHANGELOG.md | 4 + .../contracts/test/NamespacedToModify.sol | 25 +++++++ packages/core/package.json | 2 +- .../core/src/utils/make-namespaced.test.ts | 2 +- .../core/src/utils/make-namespaced.test.ts.md | 69 ++++++++++++------ .../src/utils/make-namespaced.test.ts.snap | Bin 1645 -> 1973 bytes packages/core/src/utils/make-namespaced.ts | 62 +++++++++++----- 7 files changed, 121 insertions(+), 43 deletions(-) diff --git a/packages/core/CHANGELOG.md b/packages/core/CHANGELOG.md index 866beb00f..d85f58be1 100644 --- a/packages/core/CHANGELOG.md +++ b/packages/core/CHANGELOG.md @@ -1,5 +1,9 @@ # Changelog +## 1.34.2 (2024-07-18) + +- Fix Hardhat compile error when constants have references to other constants. ([#1046](https://github.com/OpenZeppelin/openzeppelin-upgrades/pull/1046)) + ## 1.34.1 (2024-06-18) - Fix unexpected validation error when function parameter has internal function pointer. ([#1038](https://github.com/OpenZeppelin/openzeppelin-upgrades/pull/1038)) diff --git a/packages/core/contracts/test/NamespacedToModify.sol b/packages/core/contracts/test/NamespacedToModify.sol index 7175a6166..c78387a6c 100644 --- a/packages/core/contracts/test/NamespacedToModify.sol +++ b/packages/core/contracts/test/NamespacedToModify.sol @@ -116,6 +116,10 @@ contract Consumer { } } +contract HasConstantWithSelector { + bytes4 constant CONTRACT_CONSTANT_USING_SELECTOR = HasFunction.foo.selector; +} + function plusTwo(uint x) pure returns (uint) { return x + 2; } @@ -176,17 +180,38 @@ enum FreeEnum { MyEnum } */ error CustomErrorOutsideContract(Example example); +int8 constant MAX_SIZE_C = 2; + contract StructArrayUsesConstant { uint16 private constant MAX_SIZE = 10; struct NotNamespaced { uint16 a; uint256[MAX_SIZE] b; + uint256[MAX_SIZE_C] c; } /// @custom:storage-location erc7201:uses.constant struct MainStorage { uint256 x; uint256[MAX_SIZE] y; + uint256[MAX_SIZE_C] c; } +} + +address constant MY_ADDRESS = address(0); +uint constant CONVERTED_ADDRESS = uint160(MY_ADDRESS); + +interface IDummy { +} + +contract UsesAddress { + IDummy public constant MY_CONTRACT = IDummy(MY_ADDRESS); +} + +contract HasFunctionWithRequiredReturn { + struct S { uint x; } + function foo(S calldata s) internal pure returns (S calldata) { + return s; + } } \ No newline at end of file diff --git a/packages/core/package.json b/packages/core/package.json index e5dce6b1a..5a4a9c88f 100644 --- a/packages/core/package.json +++ b/packages/core/package.json @@ -1,6 +1,6 @@ { "name": "@openzeppelin/upgrades-core", - "version": "1.34.1", + "version": "1.34.2", "description": "", "repository": "https://github.com/OpenZeppelin/openzeppelin-upgrades/tree/master/packages/core", "license": "MIT", diff --git a/packages/core/src/utils/make-namespaced.test.ts b/packages/core/src/utils/make-namespaced.test.ts index ed29bafe7..cde61804d 100644 --- a/packages/core/src/utils/make-namespaced.test.ts +++ b/packages/core/src/utils/make-namespaced.test.ts @@ -38,7 +38,7 @@ async function testMakeNamespaced( // Run hardhat compile on the modified input and make sure it has no errors const modifiedOutput = await hardhatCompile(modifiedInput, solcVersion); - t.is(modifiedOutput.errors, undefined); + t.is(modifiedOutput.errors, undefined, JSON.stringify(modifiedInput.sources, null, 2)); normalizeIdentifiers(modifiedInput); t.snapshot(modifiedInput); diff --git a/packages/core/src/utils/make-namespaced.test.ts.md b/packages/core/src/utils/make-namespaced.test.ts.md index 98e13bb75..926e3b73f 100644 --- a/packages/core/src/utils/make-namespaced.test.ts.md +++ b/packages/core/src/utils/make-namespaced.test.ts.md @@ -46,22 +46,22 @@ Generated by [AVA](https://avajs.dev). } SecondaryStorage $SecondaryStorage_random;␊ ␊ /// @notice some natspec␊ - enum $astId_id_random { dummy }␊ + function foo() public {}␊ ␊ /// @param a docs␊ - enum $astId_id_random { dummy }␊ + function foo1(uint a) public {}␊ ␊ /// @param a docs␊ - enum $astId_id_random { dummy }␊ + function foo2(uint a) internal {}␊ struct MyStruct { uint b; }␊ ␊ // keccak256(abi.encode(uint256(keccak256("example.main")) - 1)) & ~bytes32(uint256(0xff));␊ bytes32 private constant MAIN_STORAGE_LOCATION =␊ 0x183a6125c38840424c4a85fa12bab2ab606c4b6d0e7cc73c0c06ba5300eab500;␊ ␊ - enum $astId_id_random { dummy }␊ + function _getMainStorage() private pure returns (bool) {}␊ ␊ - enum $astId_id_random { dummy }␊ + function _getXTimesY() internal view returns (bool) {}␊ ␊ /// @notice standlone natspec␊ ␊ @@ -77,14 +77,14 @@ Generated by [AVA](https://avajs.dev). /**␊ * doc block without natspec␊ */␊ - enum $astId_id_random { dummy }␊ + function foo3() public {}␊ ␊ /**␊ * doc block with natspec␊ *␊ * @notice some natspec␊ */␊ - enum $astId_id_random { dummy }␊ + function foo4() public {}␊ ␊ /**␊ * @dev a custom error inside a contract␊ @@ -99,35 +99,39 @@ Generated by [AVA](https://avajs.dev). }␊ ␊ contract HasFunction {␊ - enum $astId_id_random { dummy }␊ - enum $astId_id_random { dummy }␊ + ␊ + function foo() pure public returns (bool) {}␊ }␊ ␊ contract UsingFunction is HasFunction {␊ enum $astId_id_random { dummy }␊ }␊ ␊ - enum FreeFunctionUsingSelector { dummy }␊ + function FreeFunctionUsingSelector() pure returns (bool) {}␊ ␊ - enum CONSTANT_USING_SELECTOR { dummy }␊ + bytes4 constant CONSTANT_USING_SELECTOR = HasFunction.foo.selector;␊ ␊ library Lib {␊ - enum $astId_id_random { dummy }␊ + function usingSelector() pure public returns (bool) {}␊ ␊ - enum $astId_id_random { dummy }␊ + function plusOne(uint x) pure public returns (bool) {}␊ }␊ ␊ contract Consumer {␊ enum $astId_id_random { dummy }␊ ␊ - enum $astId_id_random { dummy }␊ + function usingFreeFunction() pure public returns (bool) {}␊ ␊ - enum $astId_id_random { dummy }␊ + function usingConstant() pure public returns (bool) {}␊ ␊ - enum $astId_id_random { dummy }␊ + function usingLibraryFunction() pure public returns (bool) {}␊ }␊ ␊ - enum plusTwo { dummy }␊ + contract HasConstantWithSelector {␊ + bytes4 constant CONTRACT_CONSTANT_USING_SELECTOR = HasFunction.foo.selector;␊ + }␊ + ␊ + function plusTwo(uint x) pure returns (bool) {}␊ ␊ /**␊ * @notice originally orphaned natspec␊ @@ -137,7 +141,7 @@ Generated by [AVA](https://avajs.dev). * @dev plusThree␊ * @param x x␊ */␊ - enum plusThree { dummy }␊ + function plusThree(uint x) pure returns (bool) {}␊ ␊ /// @notice originally orphaned natspec 2␊ ␊ @@ -146,9 +150,9 @@ Generated by [AVA](https://avajs.dev). * @param x x␊ * @param y y␊ */␊ - enum $astId_id_random { dummy }␊ + function plusThree(uint x, uint y) pure returns (bool) {}␊ ␊ - enum originallyNoDocumentation { dummy }␊ + function originallyNoDocumentation() pure {}␊ ␊ /**␊ * @param foo foo␊ @@ -158,7 +162,7 @@ Generated by [AVA](https://avajs.dev). contract UsingForDirectives {␊ enum $astId_id_random { dummy }␊ ␊ - enum $astId_id_random { dummy }␊ + function usingFor(uint x) pure public returns (bool) {}␊ }␊ ␊ /**␊ @@ -179,19 +183,38 @@ Generated by [AVA](https://avajs.dev). */␊ enum CustomErrorOutsideContract { dummy }␊ ␊ + int8 constant MAX_SIZE_C = 2;␊ + ␊ contract StructArrayUsesConstant {␊ uint16 private constant MAX_SIZE = 10;␊ ␊ struct NotNamespaced {␊ uint16 a;␊ uint256[MAX_SIZE] b;␊ + uint256[MAX_SIZE_C] c;␊ }␊ ␊ /// @custom:storage-location erc7201:uses.constant␊ struct MainStorage {␊ uint256 x;␊ uint256[MAX_SIZE] y;␊ + uint256[MAX_SIZE_C] c;␊ } MainStorage $MainStorage_random;␊ + }␊ + ␊ + address constant MY_ADDRESS = address(0);␊ + uint constant CONVERTED_ADDRESS = uint160(MY_ADDRESS);␊ + ␊ + interface IDummy {␊ + }␊ + ␊ + contract UsesAddress {␊ + IDummy public constant MY_CONTRACT = IDummy(MY_ADDRESS);␊ + }␊ + ␊ + contract HasFunctionWithRequiredReturn {␊ + struct S { uint x; }␊ + function foo(S calldata s) internal pure returns (bool) {}␊ }`, }, 'contracts/test/NamespacedToModifyImported.sol': { @@ -234,8 +257,8 @@ Generated by [AVA](https://avajs.dev). pragma solidity 0.7.6;␊ ␊ contract HasFunction {␊ - enum $astId_id_random { dummy }␊ - enum $astId_id_random { dummy }␊ + ␊ + function foo() pure public returns (bool) {}␊ }␊ ␊ contract UsingFunction is HasFunction {␊ diff --git a/packages/core/src/utils/make-namespaced.test.ts.snap b/packages/core/src/utils/make-namespaced.test.ts.snap index 3ce1084276eb76f268ed1ddd729d03db1c6679eb..c805dc2f1d8c82538b48ae6a7b7afa2088e91e84 100644 GIT binary patch literal 1973 zcmV;m2TJ%sRzV3P~ zZsb%K7@@0yw2)FujF2RLvB@3qXGX z;U5@CRg_C=3{a{U(3qNj9awZ2_u{N$L$PPbZu2H#oZ40}b*zOM^#MLmy8w^dEr&Mg z+k^>HI@KkO+pYsVrmB72Hfg<+<90)c zy&Msn87fw8wru7Q(-#MWv_bqG7@wzN21&kVRx_tbcdI>KCwi5Mi7%3CVrmH~?l2crU5EFIFC^7@ ze=fD0{9*yMFD0fr{?vYt~aG#SlN zT{k$?NTD{$6Uq+(EL{95BP0+Oiv2ppW&xx=J4>XLg>EJ(Zmd_TE9=$TjY@g_TCGxA zD-|KHpvzHYIrvOCLwx0s85&e)kmk`E)dlDfk=qZ&K*K~OTMd_QSbpk6?W35q2%q9M z2|JYe5@4e?JQ83bU6f`#y5yQ~!b2||3&>#MS0E___7(={Rm!}3%`V}OxOrfcA9}+Ez{PbIVeyPzI3t;Sq1HBG)bUO{S%-%% z@fl>bi{-2IERL8XAo&2~tF(-_(@WmN9#nMVVZYb!u~QGu?!>VdE+osj@K?+dsA{_F z6@{{rr)$GkRSfJOF)3JYIa=4{T%li; z=xYOAbACZ5E82D2>E1PE*)NsIieFZvuLUaZ1a*I$P%GX+hDQ?Nai~j)3;1&Z$jkv3 zrr2dEiK#OT)9||#z{aM@2yu5Okp6|kJ7W;LkEMXX0t>&?<;wX_=H_N}O8 zyDC5|BkJ8c7#o#Wh2!^PMFHU}5`_2Vl1Xl+D6Alx!%99=??MBr7VnVQByYK}+s#dX zXYBP{1cRGR`;sh=dg_}BG9V{;-6g~$wflf{wtuMG`gh8s-`1&UilDs#C)#vyR84nt z@$><^aUazgZ2EJ#0d1ReeH#S(8PSjVWI|Ey!T53_)*3n1uQhT~Rln(WHWV@OTddVF zrlv;ED$0qoQsFVN=etkiBwiGr6e+R*bFvf;A1)l+y=Y}^0UjUZXAx**muG?2gAGzE2(~bphr(lfuM@7sV2Ujm?ern@$;atfKKUx0$S1$R)UW9w`Q#5^_&fc7<&%E_if6@! H&lmszY^Bae literal 1645 zcmV-z29o(fRzVwf#6XHKnNM6*aA>S-_;8 z_nyWtsFQVU@40x7v=_=wotjj%6!lqE{S+j&DVzvF?L96jKgK8%+;N#fxHRY=Ap8ph zsfuz{O&FBwG-y&yzYP!_#%D?BBgIzKlTQF=0z90w}3#E3LD$b5$(poFWow{UuEygxK zkgaGn943hAvx7sLz}P=dl7yY@f~{v2zOhsN&XG21#WY2lT7QsAq>)gTuD!;mU zXMUR+#O7qaXabqmC}CI7PO*|{FuZ4B#Dgu2-qmtzxx98YlL_O1wwl;#)CoGuAowkS z#OsF3g=1ZTKfq4r;ZS04Cm_O%)x5TNh4?Hv3sYNialu>=wFC3A&lHIBelE2|ex-n# zSG$&4{?c&==f&8h%CK!Z7T7zoah$*_nwYiX7M8=s{$}ECGxgBSF7}Jbl^zD$j)3Su z3@k613Em)vBof=T&_&Ee(X3K4U_?jAbSsZfYy`UA0WxZf(WYsWMmkiC&3OZg{(lmng6-^5wHeZrZTXastPX@p$PGqF$S8&SRSCo|77K~o*e#VS>!nKdcDY!( zSuJmEZ*4%Dh~w&1XOO7THr0nRV#Jr;fE?vogq@b`Q?pP)8g-Xf?m2O0c|xM&<}kWP zZCtOn;IXlfZ78_gr_aj}A>KPpQrtd7N3QrEnI>vVV5H9jbZCs_xf9f(d>>`VIV7y^ zV3U|#dc#=@wIVA{@|^@kR+OAg$AI|R!Y?O!ILcR0a~y9BrzCMC9JWcBS5_Yqeu{fA zG*I4}5SGu_g|y9r3AOhLqrywSLAlp_H8Q)3WoN7xk?0Kk-iNnY|E3i>xgl~k! zgcks>`dBRVDm3m?%f-7}=o(t|Yl5ICDmfy$vjpX1AS9?9gM#L*P{cca{V0PuR0ZpT zk-5;4%n8+XqO$H*EbHqbs_WySt{dMekAK~u@>YkA`sC@kFhR{lr+w~(I3J@LgY7h( z>vQj%?s>yG&d3X81#Vh*VH`{)?snt7?sjlByB}T66uIzK;;sgl#fPx;Qz&p;Zno$t zo6SnluK@Tn0h;IlWn>u6d0}X4NW5oPGZnN=ny6>r1S)TEjDfV=YIh4| z?9$WFLiwRj3nl#s2!F$PW?=gyH3yLV;MR#B*n|Epkx$eCEfN3L{!I+_;=ta9w%YD$ z{0@wdU`!6~YFq-yOM|-_uK;9oKv(1R+7-9bBeyGl8##Z#Ud(); const modifications: Modification[] = []; for (const node of output.sources[sourcePath].ast.nodes) { @@ -60,6 +60,10 @@ export function makeNamespacedInput(input: SolcInput, output: SolcOutput): SolcI const contractNodes = contractDef.nodes; for (const contractNode of contractNodes) { switch (contractNode.nodeType) { + case 'FunctionDefinition': { + replaceFunction(contractNode, orig, modifications); + break; + } case 'VariableDeclaration': { // If variable is a constant, keep it since it may be referenced in a struct if (contractNode.constant) { @@ -69,7 +73,6 @@ export function makeNamespacedInput(input: SolcInput, output: SolcOutput): SolcI } case 'ErrorDefinition': case 'EventDefinition': - case 'FunctionDefinition': case 'ModifierDefinition': case 'UsingForDirective': { // Replace with an enum based on astId (the original name is not needed, since nothing should reference it) @@ -100,27 +103,30 @@ export function makeNamespacedInput(input: SolcInput, output: SolcOutput): SolcI break; } - // - UsingForDirective isn't needed, but it might have NatSpec documentation which is not included in the AST. - // We convert it to a dummy enum to avoid orphaning any possible documentation. - // - ErrorDefinition, FunctionDefinition, and VariableDeclaration might be imported by other files, so they cannot be deleted. + case 'FunctionDefinition': { + replaceFunction(node, orig, modifications); + break; + } + + // - VariableDeclaration and ErrorDefinition might be imported by other files, so they cannot be deleted. // However, we need to remove their values to avoid referencing other deleted nodes. - // We do this by converting them to dummy enums, but avoiding duplicate names. - case 'UsingForDirective': - case 'ErrorDefinition': - case 'FunctionDefinition': + // We do this by converting them to dummy enums with the same name. case 'VariableDeclaration': { - // If an identifier with the same name was not previously written, replace with a dummy enum using its name. - // Otherwise replace with an enum based on astId to avoid duplicate names, which can happen if there was overloading. - // This does not need to check all identifiers from the original contract, since the original compilation - // should have failed if there were conflicts in the first place. - if ('name' in node && !replacedIdentifiers.has(node.name)) { - modifications.push(makeReplace(node, orig, toDummyEnumWithName(node.name))); - replacedIdentifiers.add(node.name); - } else { - modifications.push(makeReplace(node, orig, toDummyEnumWithAstId(node.id))); + // If variable is a constant, keep it since it may be referenced in a struct + if (node.constant) { + break; } + // Otherwise, fall through to convert to dummy enum + } + case 'ErrorDefinition': { + modifications.push(makeReplace(node, orig, toDummyEnumWithName(node.name))); break; } + // - UsingForDirective isn't needed, but it might have NatSpec documentation which is not included in the AST. + // We convert it to a dummy enum to avoid orphaning any possible documentation. + case 'UsingForDirective': + modifications.push(makeReplace(node, orig, toDummyEnumWithAstId(node.id))); + break; case 'EnumDefinition': case 'ImportDirective': case 'PragmaDirective': @@ -158,6 +164,26 @@ function toDummyEnumWithAstId(astId: number) { return `enum $astId_${astId}_${(Math.random() * 1e6).toFixed(0)} { dummy }`; } +function replaceFunction(node: FunctionDefinition, orig: Buffer, modifications: Modification[]) { + if (node.kind === 'constructor') { + modifications.push(makeDelete(node, orig)); + } else { + if (node.modifiers.length > 0) { + for (const modifier of node.modifiers) { + modifications.push(makeDelete(modifier, orig)); + } + } + + if (node.returnParameters.parameters.length > 0) { + modifications.push(makeReplace(node.returnParameters, orig, '(bool)')); + } + + if (node.body) { + modifications.push(makeReplace(node.body, orig, '{}')); + } + } +} + function getPositions(node: Node) { const [start, length] = node.src.split(':').map(Number); const end = start + length;