diff --git a/packages/core/CHANGELOG.md b/packages/core/CHANGELOG.md index a243e2590..6b1886995 100644 --- a/packages/core/CHANGELOG.md +++ b/packages/core/CHANGELOG.md @@ -1,5 +1,9 @@ # Changelog +## Unreleased + +- Fix Hardhat compile error when overriding interface functions with public constant variables. ([#1091](https://github.com/OpenZeppelin/openzeppelin-upgrades/pull/1091)) + ## 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)) diff --git a/packages/core/contracts/test/NamespacedToModify.sol b/packages/core/contracts/test/NamespacedToModify.sol index d94418e43..ffa7b0668 100644 --- a/packages/core/contracts/test/NamespacedToModify.sol +++ b/packages/core/contracts/test/NamespacedToModify.sol @@ -220,7 +220,20 @@ contract UsesAddress { contract HasFunctionWithRequiredReturn { struct S { uint x; } - function foo(S calldata s) internal pure returns (S calldata) { + modifier myModifier() { + _; + } + function foo(S calldata s) internal pure myModifier returns (S calldata) { + return s; + } +} + +library LibWithRequiredReturn { + struct S { uint x; } + modifier myModifier() { + _; + } + function foo(S calldata s) myModifier internal pure returns (S calldata) { return s; } } @@ -322,3 +335,28 @@ interface InterfaceWithNamespace { uint256 y; } } + +interface IHasConstantGetter { + function a() external view returns (bytes32); +} + +contract HasConstantGetter is IHasConstantGetter { + bytes32 public override constant a = bytes32("foo"); +} + +abstract contract AbstractHasConstantGetter { + function a() virtual external pure returns (bytes32) { + // Virtual with default implementation + return bytes32("foo"); + } +} + +contract HasConstantGetterOverride is AbstractHasConstantGetter { + bytes32 public override constant a = bytes32("foo"); +} + +contract HasFunctionOverride is AbstractHasConstantGetter { + function a() override virtual external pure returns (bytes32) { + return bytes32("foo2"); + } +} \ No newline at end of file diff --git a/packages/core/src/cli/cli.test.ts.snap b/packages/core/src/cli/cli.test.ts.snap index b8b9cfa67..c8b362076 100644 Binary files a/packages/core/src/cli/cli.test.ts.snap and b/packages/core/src/cli/cli.test.ts.snap differ diff --git a/packages/core/src/cli/validate/project-report.test.ts.snap b/packages/core/src/cli/validate/project-report.test.ts.snap index 885aea7d7..7f7ea43e3 100644 Binary files a/packages/core/src/cli/validate/project-report.test.ts.snap and b/packages/core/src/cli/validate/project-report.test.ts.snap differ diff --git a/packages/core/src/cli/validate/validate-upgrade-safety.test.ts.snap b/packages/core/src/cli/validate/validate-upgrade-safety.test.ts.snap index 3b110f25e..288a82495 100644 Binary files a/packages/core/src/cli/validate/validate-upgrade-safety.test.ts.snap and b/packages/core/src/cli/validate/validate-upgrade-safety.test.ts.snap differ diff --git a/packages/core/src/cli/validate/validations.test.ts.snap b/packages/core/src/cli/validate/validations.test.ts.snap index 2e97cff8d..6de70eda2 100644 Binary files a/packages/core/src/cli/validate/validations.test.ts.snap and b/packages/core/src/cli/validate/validations.test.ts.snap differ diff --git a/packages/core/src/scripts/migrate-oz-cli-project.test.ts.snap b/packages/core/src/scripts/migrate-oz-cli-project.test.ts.snap index bfbaf16f2..de79d5adf 100644 Binary files a/packages/core/src/scripts/migrate-oz-cli-project.test.ts.snap and b/packages/core/src/scripts/migrate-oz-cli-project.test.ts.snap differ diff --git a/packages/core/src/storage-0.8.test.ts.snap b/packages/core/src/storage-0.8.test.ts.snap index e4c55d8de..af308e012 100644 Binary files a/packages/core/src/storage-0.8.test.ts.snap and b/packages/core/src/storage-0.8.test.ts.snap differ diff --git a/packages/core/src/storage-memory-0.5.test.ts.snap b/packages/core/src/storage-memory-0.5.test.ts.snap index 993bc161f..8d042d80c 100644 Binary files a/packages/core/src/storage-memory-0.5.test.ts.snap and b/packages/core/src/storage-memory-0.5.test.ts.snap differ diff --git a/packages/core/src/storage-namespaced-conflicts-layout.test.ts.snap b/packages/core/src/storage-namespaced-conflicts-layout.test.ts.snap index 1ac895a84..e9b204ac8 100644 Binary files a/packages/core/src/storage-namespaced-conflicts-layout.test.ts.snap and b/packages/core/src/storage-namespaced-conflicts-layout.test.ts.snap differ diff --git a/packages/core/src/storage-namespaced-conflicts.test.ts.snap b/packages/core/src/storage-namespaced-conflicts.test.ts.snap index 1ac895a84..e9b204ac8 100644 Binary files a/packages/core/src/storage-namespaced-conflicts.test.ts.snap and b/packages/core/src/storage-namespaced-conflicts.test.ts.snap differ diff --git a/packages/core/src/storage-namespaced-layout.test.ts.snap b/packages/core/src/storage-namespaced-layout.test.ts.snap index d2368085c..ab6d9f892 100644 Binary files a/packages/core/src/storage-namespaced-layout.test.ts.snap and b/packages/core/src/storage-namespaced-layout.test.ts.snap differ diff --git a/packages/core/src/storage-namespaced.test.ts.snap b/packages/core/src/storage-namespaced.test.ts.snap index c15ed47aa..fa70a8eaf 100644 Binary files a/packages/core/src/storage-namespaced.test.ts.snap and b/packages/core/src/storage-namespaced.test.ts.snap differ diff --git a/packages/core/src/storage.test.ts.snap b/packages/core/src/storage.test.ts.snap index c63baba41..1d7ccca64 100644 Binary files a/packages/core/src/storage.test.ts.snap and b/packages/core/src/storage.test.ts.snap differ diff --git a/packages/core/src/storage/report-gap.test.ts.snap b/packages/core/src/storage/report-gap.test.ts.snap index 840a42c52..35235c4d4 100644 Binary files a/packages/core/src/storage/report-gap.test.ts.snap and b/packages/core/src/storage/report-gap.test.ts.snap differ diff --git a/packages/core/src/storage/report-rename-retype.test.ts.snap b/packages/core/src/storage/report-rename-retype.test.ts.snap index ffd806259..0fe000153 100644 Binary files a/packages/core/src/storage/report-rename-retype.test.ts.snap and b/packages/core/src/storage/report-rename-retype.test.ts.snap differ diff --git a/packages/core/src/storage/report.test.ts.snap b/packages/core/src/storage/report.test.ts.snap index d468591db..5c9418f5b 100644 Binary files a/packages/core/src/storage/report.test.ts.snap and b/packages/core/src/storage/report.test.ts.snap differ diff --git a/packages/core/src/utils/make-namespaced.test.ts.md b/packages/core/src/utils/make-namespaced.test.ts.md index fbe58a3fc..3a8a81aa1 100644 --- a/packages/core/src/utils/make-namespaced.test.ts.md +++ b/packages/core/src/utils/make-namespaced.test.ts.md @@ -53,13 +53,13 @@ Generated by [AVA](https://avajs.dev). } StorageWithComment $StorageWithComment_random;␊ ␊ ␊ - function foo() public {}␊ + function foo() virtual public ;␊ ␊ ␊ - function foo1(uint a) public {}␊ + function foo1(uint a) virtual public ;␊ ␊ ␊ - function foo2(uint a) internal {}␊ + function foo2(uint a) virtual internal ;␊ struct MyStruct { uint b; }␊ ␊ // keccak256(abi.encode(uint256(keccak256("example.main")) - 1)) & ~bytes32(uint256(0xff));␊ @@ -68,7 +68,7 @@ Generated by [AVA](https://avajs.dev). ␊ function _getMainStorage() private pure returns (bool $) {}␊ ␊ - function _getXTimesY() internal view returns (bool) {}␊ + function _getXTimesY() virtual internal view returns (uint256) ;␊ ␊ ␊ ␊ @@ -80,10 +80,10 @@ Generated by [AVA](https://avajs.dev). ␊ ␊ ␊ - function foo3() public {}␊ + function foo3() virtual public ;␊ ␊ ␊ - function foo4() public {}␊ + function foo4() virtual public ;␊ ␊ ␊ enum $astId_id_random { dummy }␊ @@ -99,7 +99,7 @@ Generated by [AVA](https://avajs.dev). ␊ enum $astId_id_random { dummy }␊ ␊ - function foo() pure public returns (bool) {}␊ + function foo() virtual pure public returns (uint) ;␊ }␊ ␊ abstract contract UsingFunction is HasFunction {␊ @@ -119,11 +119,11 @@ Generated by [AVA](https://avajs.dev). abstract contract Consumer {␊ enum $astId_id_random { dummy }␊ ␊ - function usingFreeFunction() pure public returns (bool) {}␊ + function usingFreeFunction() virtual pure public returns (bytes4) ;␊ ␊ - function usingConstant() pure public returns (bool) {}␊ + function usingConstant() virtual pure public returns (bytes4) ;␊ ␊ - function usingLibraryFunction() pure public returns (bool) {}␊ + function usingLibraryFunction() virtual pure public returns (bytes4) ;␊ }␊ ␊ abstract contract HasConstantWithSelector {␊ @@ -150,7 +150,7 @@ Generated by [AVA](https://avajs.dev). abstract contract UsingForDirectives {␊ enum $astId_id_random { dummy }␊ ␊ - function usingFor(uint x) pure public returns (bool) {}␊ + function usingFor(uint x) virtual pure public returns (uint) ;␊ }␊ ␊ ␊ @@ -190,7 +190,14 @@ Generated by [AVA](https://avajs.dev). ␊ abstract contract HasFunctionWithRequiredReturn {␊ struct S { uint x; }␊ - function foo(S calldata s) internal pure returns (bool) {}␊ + enum $astId_id_random { dummy }␊ + function foo(S calldata s) virtual internal pure returns (S calldata) ;␊ + }␊ + ␊ + library LibWithRequiredReturn {␊ + struct S { uint x; }␊ + enum $astId_id_random { dummy }␊ + function foo(S calldata s) internal pure returns (bool) {}␊ }␊ ␊ ␊ @@ -204,17 +211,17 @@ Generated by [AVA](https://avajs.dev). ␊ abstract contract HasNatSpecWithMultipleReturns {␊ ␊ - function hasMultipleReturnsInContract() public pure returns (bool, bool) {}␊ + function hasMultipleReturnsInContract() virtual public pure returns (uint, uint) ;␊ ␊ ␊ - function hasMultipleNamedReturnsInContract() public pure returns (bool a, bool b) {}␊ + function hasMultipleNamedReturnsInContract() virtual public pure returns (uint a, uint b) ;␊ ␊ ␊ - function hasReturnsDocumentedAsParamsInContract() public pure returns (bool a, bool b) {}␊ + function hasReturnsDocumentedAsParamsInContract() virtual public pure returns (uint a, uint b) ;␊ }␊ ␊ interface IHasExternalViewFunction {␊ - function foo() external view returns (bool);␊ + function foo() external view returns (uint256);␊ }␊ ␊ abstract contract HasExternalViewFunction is IHasExternalViewFunction {␊ @@ -240,7 +247,7 @@ Generated by [AVA](https://avajs.dev). enum $astId_id_random { dummy }␊ ␊ // Regular function but payable␊ - function hasPayable() public payable {}␊ + function hasPayable() virtual public payable ;␊ ␊ bytes4 constant PAYABLE_SELECTOR = this.hasPayable.selector;␊ }␊ @@ -261,7 +268,26 @@ Generated by [AVA](https://avajs.dev). uint256 y;␊ }␊ }␊ - `, + ␊ + interface IHasConstantGetter {␊ + function a() external view returns (bytes32);␊ + }␊ + ␊ + abstract contract HasConstantGetter is IHasConstantGetter {␊ + bytes32 public override constant a = bytes32("foo");␊ + }␊ + ␊ + abstract contract AbstractHasConstantGetter {␊ + function a() virtual external pure returns (bytes32) ;␊ + }␊ + ␊ + abstract contract HasConstantGetterOverride is AbstractHasConstantGetter {␊ + bytes32 public override constant a = bytes32("foo");␊ + }␊ + ␊ + abstract contract HasFunctionOverride is AbstractHasConstantGetter {␊ + function a() override virtual external pure returns (bytes32) ;␊ + }`, }, 'contracts/test/NamespacedToModifyImported.sol': { content: `// SPDX-License-Identifier: MIT␊ @@ -324,13 +350,13 @@ Generated by [AVA](https://avajs.dev). } StorageWithComment $StorageWithComment_random;␊ ␊ /// @notice some natspec␊ - function foo() public {}␊ + function foo() virtual public ;␊ ␊ /// @param a docs␊ - function foo1(uint a) public {}␊ + function foo1(uint a) virtual public ;␊ ␊ /// @param a docs␊ - function foo2(uint a) internal {}␊ + function foo2(uint a) virtual internal ;␊ struct MyStruct { uint b; }␊ ␊ // keccak256(abi.encode(uint256(keccak256("example.main")) - 1)) & ~bytes32(uint256(0xff));␊ @@ -339,7 +365,7 @@ Generated by [AVA](https://avajs.dev). ␊ function _getMainStorage() private pure returns (bool $) {}␊ ␊ - function _getXTimesY() internal view returns (bool) {}␊ + function _getXTimesY() virtual internal view returns (uint256) ;␊ ␊ /// @notice standlone natspec␊ ␊ @@ -355,14 +381,14 @@ Generated by [AVA](https://avajs.dev). /**␊ * doc block without natspec␊ */␊ - function foo3() public {}␊ + function foo3() virtual public ;␊ ␊ /**␊ * doc block with natspec␊ *␊ * @notice some natspec␊ */␊ - function foo4() public {}␊ + function foo4() virtual public ;␊ ␊ /**␊ * @dev a custom error inside a contract␊ @@ -380,7 +406,7 @@ Generated by [AVA](https://avajs.dev). /// @param myInt an integer␊ enum $astId_id_random { dummy }␊ ␊ - function foo() pure public returns (bool) {}␊ + function foo() virtual pure public returns (uint) ;␊ }␊ ␊ abstract contract UsingFunction is HasFunction {␊ @@ -400,11 +426,11 @@ Generated by [AVA](https://avajs.dev). abstract contract Consumer {␊ enum $astId_id_random { dummy }␊ ␊ - function usingFreeFunction() pure public returns (bool) {}␊ + function usingFreeFunction() virtual pure public returns (bytes4) ;␊ ␊ - function usingConstant() pure public returns (bool) {}␊ + function usingConstant() virtual pure public returns (bytes4) ;␊ ␊ - function usingLibraryFunction() pure public returns (bool) {}␊ + function usingLibraryFunction() virtual pure public returns (bytes4) ;␊ }␊ ␊ abstract contract HasConstantWithSelector {␊ @@ -442,7 +468,7 @@ Generated by [AVA](https://avajs.dev). abstract contract UsingForDirectives {␊ enum $astId_id_random { dummy }␊ ␊ - function usingFor(uint x) pure public returns (bool) {}␊ + function usingFor(uint x) virtual pure public returns (uint) ;␊ }␊ ␊ /**␊ @@ -494,7 +520,14 @@ Generated by [AVA](https://avajs.dev). ␊ abstract contract HasFunctionWithRequiredReturn {␊ struct S { uint x; }␊ - function foo(S calldata s) internal pure returns (bool) {}␊ + enum $astId_id_random { dummy }␊ + function foo(S calldata s) virtual internal pure returns (S calldata) ;␊ + }␊ + ␊ + library LibWithRequiredReturn {␊ + struct S { uint x; }␊ + enum $astId_id_random { dummy }␊ + function foo(S calldata s) internal pure returns (bool) {}␊ }␊ ␊ /**␊ @@ -520,23 +553,23 @@ Generated by [AVA](https://avajs.dev). * @return uint 1␊ * @return uint 2␊ */␊ - function hasMultipleReturnsInContract() public pure returns (bool, bool) {}␊ + function hasMultipleReturnsInContract() virtual public pure returns (uint, uint) ;␊ ␊ /**␊ * @return a first␊ * @return b second␊ */␊ - function hasMultipleNamedReturnsInContract() public pure returns (bool a, bool b) {}␊ + function hasMultipleNamedReturnsInContract() virtual public pure returns (uint a, uint b) ;␊ ␊ /**␊ * @param a first␊ * @param b second␊ */␊ - function hasReturnsDocumentedAsParamsInContract() public pure returns (bool a, bool b) {}␊ + function hasReturnsDocumentedAsParamsInContract() virtual public pure returns (uint a, uint b) ;␊ }␊ ␊ interface IHasExternalViewFunction {␊ - function foo() external view returns (bool);␊ + function foo() external view returns (uint256);␊ }␊ ␊ abstract contract HasExternalViewFunction is IHasExternalViewFunction {␊ @@ -562,7 +595,7 @@ Generated by [AVA](https://avajs.dev). enum $astId_id_random { dummy }␊ ␊ // Regular function but payable␊ - function hasPayable() public payable {}␊ + function hasPayable() virtual public payable ;␊ ␊ bytes4 constant PAYABLE_SELECTOR = this.hasPayable.selector;␊ }␊ @@ -583,7 +616,26 @@ Generated by [AVA](https://avajs.dev). uint256 y;␊ }␊ }␊ - `, + ␊ + interface IHasConstantGetter {␊ + function a() external view returns (bytes32);␊ + }␊ + ␊ + abstract contract HasConstantGetter is IHasConstantGetter {␊ + bytes32 public override constant a = bytes32("foo");␊ + }␊ + ␊ + abstract contract AbstractHasConstantGetter {␊ + function a() virtual external pure returns (bytes32) ;␊ + }␊ + ␊ + abstract contract HasConstantGetterOverride is AbstractHasConstantGetter {␊ + bytes32 public override constant a = bytes32("foo");␊ + }␊ + ␊ + abstract contract HasFunctionOverride is AbstractHasConstantGetter {␊ + function a() override virtual external pure returns (bytes32) ;␊ + }`, }, 'contracts/test/NamespacedToModifyImported.sol': { content: `// SPDX-License-Identifier: MIT␊ @@ -626,7 +678,7 @@ Generated by [AVA](https://avajs.dev). ␊ abstract contract HasFunction {␊ enum $astId_id_random { dummy }␊ - function foo() pure public returns (bool) {}␊ + function foo() virtual pure public returns (uint) ;␊ }␊ ␊ abstract 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 299d67b4d..fcb3dc417 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 c104dc31f..850f33f58 100644 --- a/packages/core/src/utils/make-namespaced.ts +++ b/packages/core/src/utils/make-namespaced.ts @@ -3,7 +3,7 @@ import { Node } from 'solidity-ast/node'; import { SolcInput, SolcOutput } from '../solc-api'; import { getStorageLocationAnnotation } from '../storage/namespace'; import { assert, assertUnreachable } from './assert'; -import { FunctionDefinition, VariableDeclaration } from 'solidity-ast'; +import { ContractDefinition, FunctionDefinition, VariableDeclaration } from 'solidity-ast'; const OUTPUT_SELECTION = { '*': { @@ -19,10 +19,14 @@ const OUTPUT_SELECTION = { * This makes the following modifications to the input: * - Adds a state variable for each namespaced struct definition * - For each contract, for all node types that are not needed for storage layout or may call functions and constructors, converts them to dummy enums with random id + * - Mark contracts as abstract, since state variables are converted to dummy enums which would not generate public getters for inherited interface functions * - Converts all using for directives (at file level and in contracts) to dummy enums with random id (do not delete them to avoid orphaning possible NatSpec documentation) * - Converts all custom errors and constants (at file level) to dummy enums with the same name (do not delete them since they might be imported by other files) * - Replaces functions as follows: - * - For regular function and free function, keep declarations since they may be referenced by constants (free functions may also be imported by other files). But simplify compilation by removing modifiers and body, and replace return parameters with bools which can be default initialized. + * - For regular function and free function, keep declarations since they may be referenced by constants (free functions may also be imported by other files). But simplify compilation as follows: + * - Avoid having to initialize return parameters: convert function to virtual if possible, or convert return parameters to bools which can be default initialized. + * - Delete modifiers + * - Delete function bodies * - Constructors are not needed, since we removed anything that may call constructors. Convert to dummy enums to avoid orphaning possible NatSpec. * - Fallback and receive functions are not needed, since they don't have signatures. Convert to dummy enums to avoid orphaning possible NatSpec. * @@ -72,7 +76,7 @@ export function makeNamespacedInput(input: SolcInput, output: SolcOutput, solcVe for (const contractNode of contractNodes) { switch (contractNode.nodeType) { case 'FunctionDefinition': { - replaceFunction(contractNode, orig, modifications); + replaceFunction(contractNode, orig, modifications, contractDef); break; } case 'VariableDeclaration': { @@ -243,28 +247,62 @@ function toDummyEnumWithAstId(astId: number) { return `enum $astId_${astId}_${(Math.random() * 1e6).toFixed(0)} { dummy }`; } -function replaceFunction(node: FunctionDefinition, orig: Buffer, modifications: Modification[]) { +function replaceFunction( + node: FunctionDefinition, + orig: Buffer, + modifications: Modification[], + contractDef?: ContractDefinition, +) { switch (node.kind) { case 'freeFunction': case 'function': { + let virtual = node.virtual; + + if ( + contractDef !== undefined && + contractDef.contractKind === 'contract' && + node.visibility !== 'private' && + !virtual + ) { + assert(node.kind !== 'freeFunction'); + + // If this is a contract function and not private, it could possibly override an interface function. + // We don't want to change its return parameters because that might cause a mismatch with the interface. + // Simply convert the function to virtual (if not already) + modifications.push(makeInsertAfter(node.parameters, ' virtual ')); + virtual = true; + } + if (node.modifiers.length > 0) { + // Delete modifiers for (const modifier of node.modifiers) { modifications.push(makeDelete(modifier, orig)); } } - if (node.returnParameters.parameters.length > 0) { - modifications.push( - makeReplace( - node.returnParameters, - orig, - `(${node.returnParameters.parameters.map(param => toReturnParameterReplacement(param)).join(', ')})`, - ), - ); - } - if (node.body) { - modifications.push(makeReplace(node.body, orig, '{}')); + // Delete body + if (virtual) { + modifications.push(makeReplace(node.body, orig, ';')); + } else { + // This is a non-virtual function with a body, so that means it is not an interface function. + assert(contractDef?.contractKind !== 'interface'); + // Since all contract functions that are non-private were converted to virtual above, this cannot possibly override another function. + assert(!node.overrides); + + // The remaining scenarios may mean this is a library function, free function, or private function. + // In any of these cases, we can convert the return parameters to bools so that they can be default initialized. + if (node.returnParameters.parameters.length > 0) { + modifications.push( + makeReplace( + node.returnParameters, + orig, + `(${node.returnParameters.parameters.map(param => toReturnParameterReplacement(param)).join(', ')})`, + ), + ); + } + modifications.push(makeReplace(node.body, orig, '{}')); + } } break; diff --git a/packages/core/src/utils/parse-type-id.test.ts.snap b/packages/core/src/utils/parse-type-id.test.ts.snap index aa77cd8fa..7812ae9dd 100644 Binary files a/packages/core/src/utils/parse-type-id.test.ts.snap and b/packages/core/src/utils/parse-type-id.test.ts.snap differ