diff --git a/docs/modules/ROOT/pages/api-core.adoc b/docs/modules/ROOT/pages/api-core.adoc index a9025a734..df1e6dc02 100644 --- a/docs/modules/ROOT/pages/api-core.adoc +++ b/docs/modules/ROOT/pages/api-core.adoc @@ -116,11 +116,12 @@ If any errors are found, the command will exit with a non-zero exit code and pri *Options:* -* `--contract ` The name or fully qualified name of the contract to validate. If not specified, all upgradeable contracts in the build info directory will be validated. -* `--reference ` Can only be used when the `--contract` option is also provided. The name or fully qualified name of the reference contract to use for storage layout comparisons. If not specified, uses the `@custom:oz-upgrades-from` annotation if it is defined in the contract that is being validated. -* `--requireReference` Can only be used when the `--contract` option is also provided. Not compatible with `--unsafeSkipStorageCheck`. If specified, requires either the `--reference` option to be provided or the contract to have a `@custom:oz-upgrades-from` annotation. -* `--referenceBuildInfoDirs` Optional paths of additional build info directories from previous versions of the project to use for storage layout comparisons. When using this option, refer to one of these directories using prefix `:` before the contract name or fully qualified name in the `--reference` option or `@custom:oz-upgrades-from` annotation, where `` is the directory short name. Each directory short name must be unique, including compared to the main build info directory. -* `--unsafeAllow ""` - Selectively disable one or more validation errors. Comma-separated list with one or more of the following: `state-variable-assignment, state-variable-immutable, external-library-linking, struct-definition, enum-definition, constructor, delegatecall, selfdestruct, missing-public-upgradeto, internal-function-storage` +* `--contract ` - The name or fully qualified name of the contract to validate. If not specified, all upgradeable contracts in the build info directory will be validated. +* `--reference ` - Can only be used when the `--contract` option is also provided. The name or fully qualified name of the reference contract to use for storage layout comparisons. If not specified, uses the `@custom:oz-upgrades-from` annotation if it is defined in the contract that is being validated. +* `--requireReference` - Can only be used when the `--contract` option is also provided. Not compatible with `--unsafeSkipStorageCheck`. If specified, requires either the `--reference` option to be provided or the contract to have a `@custom:oz-upgrades-from` annotation. +* `--referenceBuildInfoDirs "[,...]"` - Optional paths of additional build info directories from previous versions of the project to use for storage layout comparisons. When using this option, refer to one of these directories using prefix `:` before the contract name or fully qualified name in the `--reference` option or `@custom:oz-upgrades-from` annotation, where `` is the directory short name. Each directory short name must be unique, including compared to the main build info directory. If passing in multiple directories, separate them with commas or call the option multiple times, once for each directory. +* `--exclude "" [--exclude ""...]` - Exclude validations for contracts in source file paths that match any of the given glob patterns. For example, `--exclude "contracts/mocks/\**/*.sol"`. Does not apply to reference contracts. If passing in multiple patterns, call the option multiple times, once for each pattern. +* `--unsafeAllow "[,...]"` - Selectively disable one or more validation errors. Comma-separated list with one or more of the following: `state-variable-assignment, state-variable-immutable, external-library-linking, struct-definition, enum-definition, constructor, delegatecall, selfdestruct, missing-public-upgradeto, internal-function-storage` * `--unsafeAllowRenames` - Configure storage layout check to allow variable renaming. * `--unsafeSkipStorageCheck` - Skips checking for storage layout compatibility errors. This is a dangerous option meant to be used as a last resort. @@ -152,6 +153,8 @@ validateUpgradeSafety( contract?: string, reference?: string, opts: ValidateUpgradeSafetyOptions = {}, + referenceBuildInfoDirs?: string[], + exclude?: string[], ): Promise ---- @@ -169,6 +172,8 @@ Note that this function does not throw validation errors directly. Instead, you ** `unsafeAllowRenames` ** `unsafeSkipStorageCheck` ** `requireReference` - Can only be used when the `contract` argument is also provided. Not compatible with the `unsafeSkipStorageCheck` option. If specified, requires either the `reference` argument to be provided or the contract to have a `@custom:oz-upgrades-from` annotation. +* `referenceBuildInfoDirs` - Optional paths of additional build info directories from previous versions of the project to use for storage layout comparisons. When using this option, refer to one of these directories using prefix `:` before the contract name or fully qualified name in the `reference` param or `@custom:oz-upgrades-from` annotation, where `` is the directory short name. Each directory short name must be unique, including compared to the main build info directory. +* `exclude` - Exclude validations for contracts in source file paths that match any of the given glob patterns. *Returns:* diff --git a/packages/core/CHANGELOG.md b/packages/core/CHANGELOG.md index 7af4ee161..55378041e 100644 --- a/packages/core/CHANGELOG.md +++ b/packages/core/CHANGELOG.md @@ -1,5 +1,9 @@ # Changelog +## Unreleased + +- CLI: Support `--exclude` option. ([#1065](https://github.com/OpenZeppelin/openzeppelin-upgrades/pull/1065)) + ## 1.36.0 (2024-08-21) - Update dependency on Slang. ([#1059](https://github.com/OpenZeppelin/openzeppelin-upgrades/pull/1059)) diff --git a/packages/core/contracts/test/cli/excludes/Abstract1.sol b/packages/core/contracts/test/cli/excludes/Abstract1.sol new file mode 100644 index 000000000..64deee0f7 --- /dev/null +++ b/packages/core/contracts/test/cli/excludes/Abstract1.sol @@ -0,0 +1,10 @@ +// SPDX-License-Identifier: MIT +pragma solidity ^0.8.9; + +abstract contract Abstract1 { + uint256 public immutable x; + + constructor(uint256 _x) { + x = _x; + } +} \ No newline at end of file diff --git a/packages/core/contracts/test/cli/excludes/Abstract2.sol b/packages/core/contracts/test/cli/excludes/Abstract2.sol new file mode 100644 index 000000000..d8fea8654 --- /dev/null +++ b/packages/core/contracts/test/cli/excludes/Abstract2.sol @@ -0,0 +1,10 @@ +// SPDX-License-Identifier: MIT +pragma solidity ^0.8.9; + +abstract contract Abstract2 { + uint256 public immutable y; + + constructor(uint256 _y) { + y = _y; + } +} \ No newline at end of file diff --git a/packages/core/contracts/test/cli/excludes/AbstractUUPS.sol b/packages/core/contracts/test/cli/excludes/AbstractUUPS.sol new file mode 100644 index 000000000..85005b80b --- /dev/null +++ b/packages/core/contracts/test/cli/excludes/AbstractUUPS.sol @@ -0,0 +1,14 @@ +// SPDX-License-Identifier: MIT +pragma solidity ^0.8.9; + +import {UUPSUpgradeable} from "@openzeppelin/contracts-upgradeable/proxy/utils/UUPSUpgradeable.sol"; +import {Abstract1} from "./Abstract1.sol"; +import {Abstract2} from "./Abstract2.sol"; + +abstract contract AbstractUUPS is UUPSUpgradeable, Abstract1, Abstract2 { + uint256 public immutable z; + + constructor(uint256 _x, uint256 _y, uint256 _z) Abstract1(_x) Abstract2(_y) { + z = _z; + } +} \ No newline at end of file diff --git a/packages/core/contracts/test/cli/excludes/ImportVersionsAndBeacon.sol b/packages/core/contracts/test/cli/excludes/ImportVersionsAndBeacon.sol new file mode 100644 index 000000000..9b0890ae8 --- /dev/null +++ b/packages/core/contracts/test/cli/excludes/ImportVersionsAndBeacon.sol @@ -0,0 +1,10 @@ +// SPDX-License-Identifier: MIT +pragma solidity ^0.8.9; + +import "./V1.sol"; +import "./V2Bad1.sol"; +import "./V2Bad2.sol"; +import "@openzeppelin/contracts/proxy/beacon/UpgradeableBeacon.sol"; + +abstract contract Dummy { +} diff --git a/packages/core/contracts/test/cli/excludes/UsesAbstractUUPS.sol b/packages/core/contracts/test/cli/excludes/UsesAbstractUUPS.sol new file mode 100644 index 000000000..4a6f7f76b --- /dev/null +++ b/packages/core/contracts/test/cli/excludes/UsesAbstractUUPS.sol @@ -0,0 +1,15 @@ +// SPDX-License-Identifier: MIT +pragma solidity ^0.8.9; + +// This contract is for testing only, it is not safe for use in production. + +import {AbstractUUPS} from "./AbstractUUPS.sol"; + +contract UsesAbstractUUPS is AbstractUUPS { + constructor(uint256 _x, uint256 _y, uint256 _z) AbstractUUPS(x, y, z) { + } + + function _authorizeUpgrade(address newImplementation) internal pure override { + revert("Upgrade disabled"); + } +} \ No newline at end of file diff --git a/packages/core/contracts/test/cli/excludes/V1.sol b/packages/core/contracts/test/cli/excludes/V1.sol new file mode 100644 index 000000000..e109d3afe --- /dev/null +++ b/packages/core/contracts/test/cli/excludes/V1.sol @@ -0,0 +1,6 @@ +// SPDX-License-Identifier: MIT +pragma solidity ^0.8.9; + +contract V1 { + uint256 public x; +} \ No newline at end of file diff --git a/packages/core/contracts/test/cli/excludes/V2Bad1.sol b/packages/core/contracts/test/cli/excludes/V2Bad1.sol new file mode 100644 index 000000000..15c500859 --- /dev/null +++ b/packages/core/contracts/test/cli/excludes/V2Bad1.sol @@ -0,0 +1,6 @@ +// SPDX-License-Identifier: MIT +pragma solidity ^0.8.9; + +/// @custom:oz-upgrades-from V1 +contract V2Bad1 { +} \ No newline at end of file diff --git a/packages/core/contracts/test/cli/excludes/V2Bad2.sol b/packages/core/contracts/test/cli/excludes/V2Bad2.sol new file mode 100644 index 000000000..faba82d87 --- /dev/null +++ b/packages/core/contracts/test/cli/excludes/V2Bad2.sol @@ -0,0 +1,6 @@ +// SPDX-License-Identifier: MIT +pragma solidity ^0.8.9; + +/// @custom:oz-upgrades-from V1 +contract V2Bad2 { +} \ No newline at end of file diff --git a/packages/core/package.json b/packages/core/package.json index d1ce44559..00b792aba 100644 --- a/packages/core/package.json +++ b/packages/core/package.json @@ -69,6 +69,7 @@ "ethereumjs-util": "^7.0.3", "minimist": "^1.2.7", "proper-lockfile": "^4.1.1", - "solidity-ast": "^0.4.51" + "solidity-ast": "^0.4.51", + "minimatch": "^9.0.5" } } diff --git a/packages/core/src/cli/cli.test.ts b/packages/core/src/cli/cli.test.ts index cec9887d0..2af1a71c0 100644 --- a/packages/core/src/cli/cli.test.ts +++ b/packages/core/src/cli/cli.test.ts @@ -444,7 +444,7 @@ test('validate - references other build info dir by command with fully qualified t.snapshot(expectation.join('\n')); }); -test('validate - references multiple build info dirs by annotation', async t => { +async function setupMultipleBuildInfoDirsTest(t: ExecutionContext) { const temp = await getTempDir(t); const v1Dir = path.join(temp, 'build-info-v1'); @@ -467,6 +467,11 @@ test('validate - references multiple build info dirs by annotation', async t => ); await fs.writeFile(path.join(v2BranchDir, 'ok.json'), JSON.stringify(v2BranchBuildInfoOk)); await fs.writeFile(path.join(v2BranchDir, 'bad.json'), JSON.stringify(v2BranchBuildInfoBad)); + return { v2BranchDir, v1Dir, v2Dir }; +} + +test('validate - references multiple build info dirs by annotation', async t => { + const { v2BranchDir, v1Dir, v2Dir } = await setupMultipleBuildInfoDirsTest(t); const error = await t.throwsAsync( execAsync(`${CLI} validate ${v2BranchDir} --referenceBuildInfoDirs ${v1Dir},${v2Dir}`), @@ -475,6 +480,16 @@ test('validate - references multiple build info dirs by annotation', async t => t.snapshot(expectation.join('\n')); }); +test('validate - references multiple build info dirs by annotation - same arg multiple times', async t => { + const { v2BranchDir, v1Dir, v2Dir } = await setupMultipleBuildInfoDirsTest(t); + + const error = await t.throwsAsync( + execAsync(`${CLI} validate ${v2BranchDir} --referenceBuildInfoDirs ${v1Dir} --referenceBuildInfoDirs ${v2Dir}`), + ); + const expectation: string[] = [`Stdout: ${(error as any).stdout}`, `Stderr: ${(error as any).stderr}`]; + t.snapshot(expectation.join('\n')); +}); + test('validate - references other build info dir by annotation - ok', async t => { const temp = await getTempDir(t); const referenceDir = path.join(temp, 'build-info-v1'); @@ -572,3 +587,86 @@ test('validate - contract must not have build info dir name - fully qualified', error?.message, ); }); + +test('validate - excludes by pattern - no match', async t => { + const temp = await getTempDir(t); + const buildInfo = await artifacts.getBuildInfo(`contracts/test/cli/excludes/UsesAbstractUUPS.sol:UsesAbstractUUPS`); + await fs.writeFile(path.join(temp, 'validate.json'), JSON.stringify(buildInfo)); + + const error = await t.throwsAsync(execAsync(`${CLI} validate ${temp} --exclude "**/NoMatch.sol"`)); + const expectation: string[] = [`Stdout: ${(error as any).stdout}`, `Stderr: ${(error as any).stderr}`]; + t.snapshot(expectation.join('\n')); +}); + +test('validate - excludes by pattern - some match', async t => { + const temp = await getTempDir(t); + const buildInfo = await artifacts.getBuildInfo(`contracts/test/cli/excludes/UsesAbstractUUPS.sol:UsesAbstractUUPS`); + await fs.writeFile(path.join(temp, 'validate.json'), JSON.stringify(buildInfo)); + + const error = await t.throwsAsync(execAsync(`${CLI} validate ${temp} --exclude "**/Abstract*.sol"`)); + const expectation: string[] = [`Stdout: ${(error as any).stdout}`, `Stderr: ${(error as any).stderr}`]; + t.snapshot(expectation.join('\n')); +}); + +test('validate - excludes by pattern - all match', async t => { + const temp = await getTempDir(t); + const buildInfo = await artifacts.getBuildInfo(`contracts/test/cli/excludes/UsesAbstractUUPS.sol:UsesAbstractUUPS`); + await fs.writeFile(path.join(temp, 'validate.json'), JSON.stringify(buildInfo)); + + const output = await execAsync(`${CLI} validate ${temp} --exclude "**/excludes/*.sol"`); + t.snapshot(output); +}); + +test('validate - excludes by pattern - all match using commas within glob', async t => { + const temp = await getTempDir(t); + const buildInfo = await artifacts.getBuildInfo(`contracts/test/cli/excludes/UsesAbstractUUPS.sol:UsesAbstractUUPS`); + await fs.writeFile(path.join(temp, 'validate.json'), JSON.stringify(buildInfo)); + + const output = await execAsync(`${CLI} validate ${temp} --exclude "**/excludes/{Abstract*,UsesAbstractUUPS}.sol"`); + t.snapshot(output); +}); + +test('validate - exclude passed multiple times', async t => { + const temp = await getTempDir(t); + const buildInfo = await artifacts.getBuildInfo(`contracts/test/cli/excludes/UsesAbstractUUPS.sol:UsesAbstractUUPS`); + await fs.writeFile(path.join(temp, 'validate.json'), JSON.stringify(buildInfo)); + + const output = await execAsync( + `${CLI} validate ${temp} --exclude "**/excludes/Abstract*.sol" --exclude "**/UsesAbstractUUPS.sol"`, + ); + t.snapshot(output); +}); + +test('validate - excludes specified contract', async t => { + const temp = await getTempDir(t); + const buildInfo = await artifacts.getBuildInfo(`contracts/test/cli/Validate.sol:BecomesBadLayout`); + await fs.writeFile(path.join(temp, 'validate.json'), JSON.stringify(buildInfo)); + + const error = await t.throwsAsync( + execAsync(`${CLI} validate ${temp} --contract BecomesBadLayout --reference StorageV1 --exclude "**/Validate.sol"`), + ); + t.true( + error?.message.includes('No validation report found for contract contracts/test/cli/Validate.sol:BecomesBadLayout'), + error?.message, + ); +}); + +test('validate - excludes UpgradeableBeacon and its parents by default', async t => { + const temp = await getTempDir(t); + const buildInfo = await artifacts.getBuildInfo(`contracts/test/cli/excludes/ImportVersionsAndBeacon.sol:Dummy`); + await fs.writeFile(path.join(temp, 'validate.json'), JSON.stringify(buildInfo)); + + const error = await t.throwsAsync(execAsync(`${CLI} validate ${temp}`)); + const expectation: string[] = [`Stdout: ${(error as any).stdout}`, `Stderr: ${(error as any).stderr}`]; + t.snapshot(expectation.join('\n')); +}); + +test('validate - excludes UpgradeableBeacon and its parents by default, and excludes one contract from layout comparisions', async t => { + const temp = await getTempDir(t); + const buildInfo = await artifacts.getBuildInfo(`contracts/test/cli/excludes/ImportVersionsAndBeacon.sol:Dummy`); + await fs.writeFile(path.join(temp, 'validate.json'), JSON.stringify(buildInfo)); + + const error = await t.throwsAsync(execAsync(`${CLI} validate ${temp} --exclude "**/V2Bad1.sol"`)); + const expectation: string[] = [`Stdout: ${(error as any).stdout}`, `Stderr: ${(error as any).stderr}`]; + t.snapshot(expectation.join('\n')); +}); diff --git a/packages/core/src/cli/cli.test.ts.md b/packages/core/src/cli/cli.test.ts.md index 26f9ef3ea..bcc365e62 100644 --- a/packages/core/src/cli/cli.test.ts.md +++ b/packages/core/src/cli/cli.test.ts.md @@ -19,8 +19,9 @@ Generated by [AVA](https://avajs.dev). --contract The name or fully qualified name of the contract to validate. If not specified, all upgradeable contracts in the build info directory will be validated.␊ --reference Can only be used when the --contract option is also provided. The name or fully qualified name of the reference contract to use for storage layout comparisons. If not specified, uses the @custom:oz-upgrades-from annotation if it is defined in the contract that is being validated.␊ --requireReference Can only be used when the --contract option is also provided. Not compatible with --unsafeSkipStorageCheck. If specified, requires either the --reference option to be provided or the contract to have a @custom:oz-upgrades-from annotation.␊ - --referenceBuildInfoDirs Optional paths of additional build info directories from previous versions of the project to use for storage layout comparisons. When using this option, refer to one of these directories using prefix ':' before the contract name or fully qualified name in the --reference option or @custom:oz-upgrades-from annotation, where is the directory short name. Each directory short name must be unique, including compared to the main build info directory.␊ - --unsafeAllow "" Selectively disable one or more validation errors. Comma-separated list with one or more of the following: state-variable-assignment, state-variable-immutable, external-library-linking, struct-definition, enum-definition, constructor, delegatecall, selfdestruct, missing-public-upgradeto, internal-function-storage␊ + --referenceBuildInfoDirs "[,...]" Optional paths of additional build info directories from previous versions of the project to use for storage layout comparisons. When using this option, refer to one of these directories using prefix ':' before the contract name or fully qualified name in the --reference option or @custom:oz-upgrades-from annotation, where is the directory short name. Each directory short name must be unique, including compared to the main build info directory. If passing in multiple directories, separate them with commas or call the option multiple times, once for each directory.␊ + --exclude "" [--exclude ""...] Exclude validations for contracts in source file paths that match any of the given glob patterns. For example, --exclude "contracts/mocks/**/*.sol". Does not apply to reference contracts. If passing in multiple patterns, call the option multiple times, once for each pattern.␊ + --unsafeAllow "[,...]" Selectively disable one or more validation errors. Comma-separated list with one or more of the following: state-variable-assignment, state-variable-immutable, external-library-linking, struct-definition, enum-definition, constructor, delegatecall, selfdestruct, missing-public-upgradeto, internal-function-storage␊ --unsafeAllowRenames Configure storage layout check to allow variable renaming.␊ --unsafeSkipStorageCheck Skips checking for storage layout compatibility errors. This is a dangerous option meant to be used as a last resort.␊ ` @@ -40,8 +41,9 @@ Generated by [AVA](https://avajs.dev). --contract The name or fully qualified name of the contract to validate. If not specified, all upgradeable contracts in the build info directory will be validated.␊ --reference Can only be used when the --contract option is also provided. The name or fully qualified name of the reference contract to use for storage layout comparisons. If not specified, uses the @custom:oz-upgrades-from annotation if it is defined in the contract that is being validated.␊ --requireReference Can only be used when the --contract option is also provided. Not compatible with --unsafeSkipStorageCheck. If specified, requires either the --reference option to be provided or the contract to have a @custom:oz-upgrades-from annotation.␊ - --referenceBuildInfoDirs Optional paths of additional build info directories from previous versions of the project to use for storage layout comparisons. When using this option, refer to one of these directories using prefix ':' before the contract name or fully qualified name in the --reference option or @custom:oz-upgrades-from annotation, where is the directory short name. Each directory short name must be unique, including compared to the main build info directory.␊ - --unsafeAllow "" Selectively disable one or more validation errors. Comma-separated list with one or more of the following: state-variable-assignment, state-variable-immutable, external-library-linking, struct-definition, enum-definition, constructor, delegatecall, selfdestruct, missing-public-upgradeto, internal-function-storage␊ + --referenceBuildInfoDirs "[,...]" Optional paths of additional build info directories from previous versions of the project to use for storage layout comparisons. When using this option, refer to one of these directories using prefix ':' before the contract name or fully qualified name in the --reference option or @custom:oz-upgrades-from annotation, where is the directory short name. Each directory short name must be unique, including compared to the main build info directory. If passing in multiple directories, separate them with commas or call the option multiple times, once for each directory.␊ + --exclude "" [--exclude ""...] Exclude validations for contracts in source file paths that match any of the given glob patterns. For example, --exclude "contracts/mocks/**/*.sol". Does not apply to reference contracts. If passing in multiple patterns, call the option multiple times, once for each pattern.␊ + --unsafeAllow "[,...]" Selectively disable one or more validation errors. Comma-separated list with one or more of the following: state-variable-assignment, state-variable-immutable, external-library-linking, struct-definition, enum-definition, constructor, delegatecall, selfdestruct, missing-public-upgradeto, internal-function-storage␊ --unsafeAllowRenames Configure storage layout check to allow variable renaming.␊ --unsafeSkipStorageCheck Skips checking for storage layout compatibility errors. This is a dangerous option meant to be used as a last resort.␊ ` @@ -383,6 +385,20 @@ Generated by [AVA](https://avajs.dev). ## validate - references multiple build info dirs by annotation +> Snapshot 1 + + `Stdout: ✘ contracts/test/cli/ValidateBuildInfoV2_Branch_Bad.sol:MyContract (upgrades from build-info-v2:contracts/test/cli/ValidateBuildInfoV2_Ok.sol:MyContract)␊ + ␊ + contracts/test/cli/ValidateBuildInfoV2_Branch_Bad.sol:8: Renamed \`z\` to \`zBranch\`␊ + ␊ + ✔ contracts/test/cli/ValidateBuildInfoV2_Branch_Ok.sol:MyContract (upgrades from build-info-v1:contracts/test/cli/ValidateBuildInfoV1.sol:MyContract)␊ + ␊ + FAILED (2 upgradeable contracts detected, 1 passed, 1 failed)␊ + ␊ + Stderr: ` + +## validate - references multiple build info dirs by annotation - same arg multiple times + > Snapshot 1 `Stdout: ✘ contracts/test/cli/ValidateBuildInfoV2_Branch_Bad.sol:MyContract (upgrades from build-info-v2:contracts/test/cli/ValidateBuildInfoV2_Ok.sol:MyContract)␊ @@ -423,3 +439,119 @@ Generated by [AVA](https://avajs.dev). FAILED␊ ␊ Stderr: ` + +## validate - excludes by pattern - no match + +> Snapshot 1 + + `Stdout: ✘ contracts/test/cli/excludes/UsesAbstractUUPS.sol:UsesAbstractUUPS␊ + ␊ + contracts/test/cli/excludes/UsesAbstractUUPS.sol:9: Contract \`UsesAbstractUUPS\` has a constructor␊ + Define an initializer instead␊ + https://zpl.in/upgrades/error-001␊ + ␊ + contracts/test/cli/excludes/AbstractUUPS.sol:11: Contract \`AbstractUUPS\` has a constructor␊ + Define an initializer instead␊ + https://zpl.in/upgrades/error-001␊ + ␊ + contracts/test/cli/excludes/AbstractUUPS.sol:9: Variable \`z\` is immutable and will be initialized on the implementation␊ + If by design, annotate with '@custom:oz-upgrades-unsafe-allow state-variable-immutable'␊ + Otherwise, consider a constant variable or use a mutable variable instead␊ + https://zpl.in/upgrades/error-005␊ + ␊ + contracts/test/cli/excludes/Abstract2.sol:7: Contract \`Abstract2\` has a constructor␊ + Define an initializer instead␊ + https://zpl.in/upgrades/error-001␊ + ␊ + contracts/test/cli/excludes/Abstract2.sol:5: Variable \`y\` is immutable and will be initialized on the implementation␊ + If by design, annotate with '@custom:oz-upgrades-unsafe-allow state-variable-immutable'␊ + Otherwise, consider a constant variable or use a mutable variable instead␊ + https://zpl.in/upgrades/error-005␊ + ␊ + contracts/test/cli/excludes/Abstract1.sol:7: Contract \`Abstract1\` has a constructor␊ + Define an initializer instead␊ + https://zpl.in/upgrades/error-001␊ + ␊ + contracts/test/cli/excludes/Abstract1.sol:5: Variable \`x\` is immutable and will be initialized on the implementation␊ + If by design, annotate with '@custom:oz-upgrades-unsafe-allow state-variable-immutable'␊ + Otherwise, consider a constant variable or use a mutable variable instead␊ + https://zpl.in/upgrades/error-005␊ + ␊ + FAILED (1 upgradeable contract detected, 0 passed, 1 failed)␊ + ␊ + Stderr: ` + +## validate - excludes by pattern - some match + +> Snapshot 1 + + `Stdout: ✘ contracts/test/cli/excludes/UsesAbstractUUPS.sol:UsesAbstractUUPS␊ + ␊ + contracts/test/cli/excludes/UsesAbstractUUPS.sol:9: Contract \`UsesAbstractUUPS\` has a constructor␊ + Define an initializer instead␊ + https://zpl.in/upgrades/error-001␊ + ␊ + FAILED (1 upgradeable contract detected, 0 passed, 1 failed)␊ + ␊ + Stderr: ` + +## validate - excludes by pattern - all match + +> Snapshot 1 + + { + stderr: '', + stdout: `No upgradeable contracts detected.␊ + `, + } + +## validate - excludes by pattern - all match using commas within glob + +> Snapshot 1 + + { + stderr: '', + stdout: `No upgradeable contracts detected.␊ + `, + } + +## validate - exclude passed multiple times + +> Snapshot 1 + + { + stderr: '', + stdout: `No upgradeable contracts detected.␊ + `, + } + +## validate - excludes UpgradeableBeacon and its parents by default + +> Snapshot 1 + + `Stdout: ✘ contracts/test/cli/excludes/V2Bad1.sol:V2Bad1 (upgrades from contracts/test/cli/excludes/V1.sol:V1)␊ + ␊ + V1: Deleted \`x\`␊ + > Keep the variable even if unused␊ + ␊ + ✘ contracts/test/cli/excludes/V2Bad2.sol:V2Bad2 (upgrades from contracts/test/cli/excludes/V1.sol:V1)␊ + ␊ + V1: Deleted \`x\`␊ + > Keep the variable even if unused␊ + ␊ + FAILED (2 upgradeable contracts detected, 0 passed, 2 failed)␊ + ␊ + Stderr: ` + +## validate - excludes UpgradeableBeacon and its parents by default, and excludes one contract from layout comparisions + +> Snapshot 1 + + `Stdout: ✘ contracts/test/cli/excludes/V2Bad2.sol:V2Bad2 (upgrades from contracts/test/cli/excludes/V1.sol:V1)␊ + ␊ + V1: Deleted \`x\`␊ + > Keep the variable even if unused␊ + ␊ + FAILED (1 upgradeable contract detected, 0 passed, 1 failed)␊ + ␊ + Stderr: ` diff --git a/packages/core/src/cli/cli.test.ts.snap b/packages/core/src/cli/cli.test.ts.snap index 1b7118f66..b8b9cfa67 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.test.ts b/packages/core/src/cli/validate.test.ts index 4a864a9f9..b12904a38 100644 --- a/packages/core/src/cli/validate.test.ts +++ b/packages/core/src/cli/validate.test.ts @@ -110,3 +110,25 @@ test('withDefaults - invalid unsafeAllow', t => { )}`, }); }); + +test('withDefaults - empty unsafeAllow', t => { + const parsedArgs = minimist(['validate', 'build-info.json', '--unsafeAllow']); + t.throws(() => withDefaults(parsedArgs), { + message: `Invalid option: --unsafeAllow cannot be empty`, + }); +}); + +test('withDefaults - unsafeAllow multiple times', t => { + const parsedArgs = minimist([ + 'validate', + 'build-info.json', + '--unsafeAllow', + 'selfdestruct', + '--unsafeAllow', + 'delegatecall', + '--unsafeAllow', + 'constructor', + ]); + const opts = withDefaults(parsedArgs); + t.deepEqual(opts.unsafeAllow, ['selfdestruct', 'delegatecall', 'constructor']); +}); diff --git a/packages/core/src/cli/validate.ts b/packages/core/src/cli/validate.ts index 4e88b2adf..8d7032e53 100644 --- a/packages/core/src/cli/validate.ts +++ b/packages/core/src/cli/validate.ts @@ -16,8 +16,9 @@ Options: --contract The name or fully qualified name of the contract to validate. If not specified, all upgradeable contracts in the build info directory will be validated. --reference Can only be used when the --contract option is also provided. The name or fully qualified name of the reference contract to use for storage layout comparisons. If not specified, uses the @custom:oz-upgrades-from annotation if it is defined in the contract that is being validated. --requireReference Can only be used when the --contract option is also provided. Not compatible with --unsafeSkipStorageCheck. If specified, requires either the --reference option to be provided or the contract to have a @custom:oz-upgrades-from annotation. - --referenceBuildInfoDirs Optional paths of additional build info directories from previous versions of the project to use for storage layout comparisons. When using this option, refer to one of these directories using prefix ':' before the contract name or fully qualified name in the --reference option or @custom:oz-upgrades-from annotation, where is the directory short name. Each directory short name must be unique, including compared to the main build info directory. - --unsafeAllow "" Selectively disable one or more validation errors. Comma-separated list with one or more of the following: ${errorKinds.join( + --referenceBuildInfoDirs "[,...]" Optional paths of additional build info directories from previous versions of the project to use for storage layout comparisons. When using this option, refer to one of these directories using prefix ':' before the contract name or fully qualified name in the --reference option or @custom:oz-upgrades-from annotation, where is the directory short name. Each directory short name must be unique, including compared to the main build info directory. If passing in multiple directories, separate them with commas or call the option multiple times, once for each directory. + --exclude "" [--exclude ""...] Exclude validations for contracts in source file paths that match any of the given glob patterns. For example, --exclude "contracts/mocks/**/*.sol". Does not apply to reference contracts. If passing in multiple patterns, call the option multiple times, once for each pattern. + --unsafeAllow "[,...]" Selectively disable one or more validation errors. Comma-separated list with one or more of the following: ${errorKinds.join( ', ', )} --unsafeAllowRenames Configure storage layout check to allow variable renaming. @@ -34,6 +35,7 @@ export async function main(args: string[]): Promise { functionArgs.reference, functionArgs.opts, functionArgs.referenceBuildInfoDirs, + functionArgs.exclude, ); console.log(result.explain()); process.exitCode = result.ok ? 0 : 1; @@ -50,7 +52,7 @@ function parseArgs(args: string[]) { 'unsafeAllowLinkedLibraries', 'requireReference', ], - string: ['unsafeAllow', 'contract', 'reference', 'referenceBuildInfoDirs'], + string: ['unsafeAllow', 'contract', 'reference', 'referenceBuildInfoDirs', 'exclude'], alias: { h: 'help' }, }); const extraArgs = parsedArgs._; @@ -74,6 +76,7 @@ interface FunctionArgs { reference?: string; opts: Required; referenceBuildInfoDirs?: string[]; + exclude?: string[]; } /** @@ -93,7 +96,8 @@ export function getFunctionArgs(parsedArgs: minimist.ParsedArgs, extraArgs: stri const contract = getAndValidateString(parsedArgs, 'contract'); const reference = getAndValidateString(parsedArgs, 'reference'); const opts = withDefaults(parsedArgs); - const referenceBuildInfoDirs = getAndValidateString(parsedArgs, 'referenceBuildInfoDirs')?.split(/,+/); + const referenceBuildInfoDirs = getAndValidateStringArray(parsedArgs, 'referenceBuildInfoDirs', true); + const exclude = getAndValidateStringArray(parsedArgs, 'exclude', false); if (contract === undefined) { if (reference !== undefined) { @@ -102,18 +106,45 @@ export function getFunctionArgs(parsedArgs: minimist.ParsedArgs, extraArgs: stri throw new Error('The --requireReference option can only be used along with the --contract option.'); } } - return { buildInfoDir, contract, reference, opts, referenceBuildInfoDirs }; + return { buildInfoDir, contract, reference, opts, referenceBuildInfoDirs, exclude }; } } function getAndValidateString(parsedArgs: minimist.ParsedArgs, option: string): string | undefined { const value = parsedArgs[option]; - if (value !== undefined && value.trim().length === 0) { - throw new Error(`Invalid option: --${option} cannot be empty`); + if (value !== undefined) { + assertNonEmptyString(value, option); + } + return value; +} + +function getAndValidateStringArray( + parsedArgs: minimist.ParsedArgs, + option: string, + useCommaDelimiter: boolean, +): string[] | undefined { + const value = parsedArgs[option]; + if (value !== undefined) { + if (Array.isArray(value)) { + return value; + } else { + assertNonEmptyString(value, option); + if (useCommaDelimiter) { + return value.split(/[\s,]+/); + } else { + return [value]; + } + } } return value; } +function assertNonEmptyString(value: unknown, option: string) { + if (typeof value !== 'string' || value.trim().length === 0) { + throw new Error(`Invalid option: --${option} cannot be empty`); + } +} + function validateOptions(parsedArgs: minimist.ParsedArgs) { const invalidArgs = Object.keys(parsedArgs).filter( key => @@ -130,6 +161,7 @@ function validateOptions(parsedArgs: minimist.ParsedArgs) { 'reference', 'requireReference', 'referenceBuildInfoDirs', + 'exclude', ].includes(key), ); if (invalidArgs.length > 0) { @@ -137,18 +169,13 @@ function validateOptions(parsedArgs: minimist.ParsedArgs) { } } -function getUnsafeAllowKinds(unsafeAllow: string | undefined): ValidationError['kind'][] { +function getUnsafeAllowKinds(parseArgs: minimist.ParsedArgs): ValidationError['kind'][] { type errorKindsType = (typeof errorKinds)[number]; - if (unsafeAllow === undefined) { - return []; - } - - const unsafeAllowTokens: string[] = unsafeAllow.split(/[\s,]+/); + const unsafeAllowTokens: string[] = getAndValidateStringArray(parseArgs, 'unsafeAllow', true) ?? []; if (unsafeAllowTokens.some(token => !errorKinds.includes(token as errorKindsType))) { - // This includes empty strings throw new Error( - `Invalid option: --unsafeAllow "${unsafeAllow}". Supported values for the --unsafeAllow option are: ${errorKinds.join( + `Invalid option: --unsafeAllow "${parseArgs['unsafeAllow']}". Supported values for the --unsafeAllow option are: ${errorKinds.join( ', ', )}`, ); @@ -164,7 +191,7 @@ export function withDefaults(parsedArgs: minimist.ParsedArgs): Required, specifiedContracts?: SpecifiedContracts, + exclude?: string[], ) { const upgradeableContractReports: UpgradeableContractReport[] = []; @@ -91,20 +96,43 @@ export function getContractReports( } else if (specifiedContracts !== undefined || upgradeabilityAssessment.upgradeable) { const reference = upgradeabilityAssessment.referenceContract; const kind = upgradeabilityAssessment.uups ? 'uups' : 'transparent'; - const report = getUpgradeableContractReport(sourceContract, reference, { ...opts, kind: kind }); + const report = getUpgradeableContractReport(sourceContract, reference, { ...opts, kind: kind }, exclude); if (report !== undefined) { upgradeableContractReports.push(report); + } else if (specifiedContracts !== undefined) { + // If there was no report for the specified contract, it was excluded or is abstract. + const userAction = + exclude !== undefined + ? `Ensure the contract is not abstract and is not excluded by the exclude option.` + : `Ensure the contract is not abstract.`; + throw new ValidateCommandError( + `No validation report found for contract ${specifiedContracts.contract.fullyQualifiedName}`, + () => userAction, + ); } } } + return upgradeableContractReports; } +/** + * Gets a report for an upgradeable contract. + * Returns undefined if the contract is excluded or is abstract. + */ function getUpgradeableContractReport( contract: SourceContract, referenceContract: SourceContract | undefined, opts: ValidationOptions, + exclude?: string[], ): UpgradeableContractReport | undefined { + const excludeWithDefaults = defaultExclude.concat(exclude ?? []); + + if (excludeWithDefaults.some(glob => minimatch(getPath(contract.fullyQualifiedName), glob))) { + debug('Excluding contract: ' + contract.fullyQualifiedName); + return undefined; + } + let version; try { version = getContractVersion(contract.validationData, contract.fullyQualifiedName); @@ -119,7 +147,7 @@ function getUpgradeableContractReport( } debug('Checking: ' + contract.fullyQualifiedName); - const standaloneReport = getStandaloneReport(contract.validationData, version, opts); + const standaloneReport = getStandaloneReport(contract.validationData, version, opts, excludeWithDefaults); let reference: string | undefined; let storageLayoutReport: LayoutCompatibilityReport | undefined; @@ -145,7 +173,21 @@ function getStandaloneReport( data: ValidationData, version: Version, opts: ValidationOptions, + excludeWithDefaults: string[], ): UpgradeableContractErrorReport { - const errors = getErrors(data, version, withValidationDefaults(opts)); - return new UpgradeableContractErrorReport(errors); + const allErrors = getErrors(data, version, withValidationDefaults(opts)); + + const includeErrors = allErrors.filter(e => { + const shouldExclude = excludeWithDefaults.some(glob => minimatch(getPath(e.src), glob)); + if (shouldExclude) { + debug('Excluding error: ' + e.src); + } + return !shouldExclude; + }); + + return new UpgradeableContractErrorReport(includeErrors); +} + +function getPath(srcOrFullyQualifiedName: string): string { + return srcOrFullyQualifiedName.split(':')[0]; } diff --git a/packages/core/src/cli/validate/default-exclude.ts b/packages/core/src/cli/validate/default-exclude.ts new file mode 100644 index 000000000..041256839 --- /dev/null +++ b/packages/core/src/cli/validate/default-exclude.ts @@ -0,0 +1,3 @@ +const UPGRADEABLE_BEACON = '**/contracts/proxy/beacon/UpgradeableBeacon.sol' as const; + +export const defaultExclude: string[] = [UPGRADEABLE_BEACON] as const; diff --git a/packages/core/src/cli/validate/validate-upgrade-safety.ts b/packages/core/src/cli/validate/validate-upgrade-safety.ts index a6042ee6b..f40828187 100644 --- a/packages/core/src/cli/validate/validate-upgrade-safety.ts +++ b/packages/core/src/cli/validate/validate-upgrade-safety.ts @@ -28,6 +28,7 @@ export type SpecifiedContracts = { * @param reference The name or fully qualified name of the reference contract to use for storage layout comparisons. Can only be used along with `contract`. If not specified, uses the `@custom:oz-upgrades-from` annotation in the contract that is being validated. * @param opts Validation options, or undefined to use the default validation options. * @param referenceBuildInfoDirs Optional paths of additional build info directories from previous versions of the project to use for storage layout comparisons. When using this option, refer to one of these directories using prefix `:` before the contract name or fully qualified name in the `reference` param or `@custom:oz-upgrades-from` annotation, where `` is the directory short name. Each directory short name must be unique, including compared to the main build info directory. + * @param exclude Exclude validations for contracts in source file paths that match any of the given glob patterns. * @returns The project report. */ export async function validateUpgradeSafety( @@ -36,6 +37,7 @@ export async function validateUpgradeSafety( reference?: string, opts: ValidateUpgradeSafetyOptions = {}, referenceBuildInfoDirs?: string[], + exclude?: string[], ): Promise { const allOpts = withCliDefaults(opts); @@ -63,7 +65,7 @@ export async function validateUpgradeSafety( const specifiedContracts = findSpecifiedContracts(buildInfoDictionary, allOpts, contract, reference); - const contractReports = getContractReports(buildInfoDictionary, allOpts, specifiedContracts); + const contractReports = getContractReports(buildInfoDictionary, allOpts, specifiedContracts, exclude); return getProjectReport(contractReports, specifiedContracts !== undefined); } diff --git a/yarn.lock b/yarn.lock index 00e2cc185..728791615 100644 --- a/yarn.lock +++ b/yarn.lock @@ -4138,7 +4138,7 @@ minimatch@^5.0.1: dependencies: brace-expansion "^2.0.1" -minimatch@^9.0.4: +minimatch@^9.0.4, minimatch@^9.0.5: version "9.0.5" resolved "https://registry.yarnpkg.com/minimatch/-/minimatch-9.0.5.tgz#d74f9dd6b57d83d8e98cfb82133b03978bc929e5" integrity sha512-G6T0ZX48xgozx7587koeX9Ys2NYy6Gmv//P89sEte9V9whIapMNF4idKxnW2QtCcLiTWlb/wfCabAtAFWhhBow== @@ -5227,16 +5227,7 @@ statuses@2.0.1: resolved "https://registry.yarnpkg.com/statuses/-/statuses-2.0.1.tgz#55cb000ccf1d48728bd23c685a063998cf1a1b63" integrity sha512-RwNA9Z/7PrK06rYLIzFMlaF+l73iwpzsqRIFgbMLbTcLD6cOao82TaWefPXQvB2fOC4AjuYSEndS7N/mTCbkdQ== -"string-width-cjs@npm:string-width@^4.2.0": - version "4.2.3" - resolved "https://registry.yarnpkg.com/string-width/-/string-width-4.2.3.tgz#269c7117d27b05ad2e536830a8ec895ef9c6d010" - integrity sha512-wKyQRQpjJ0sIp62ErSZdGsjMJWsap5oRNihHhu6G7JVO/9jIB6UyevL+tXuOqrng8j/cxKTWyWUwvSTriiZz/g== - dependencies: - emoji-regex "^8.0.0" - is-fullwidth-code-point "^3.0.0" - strip-ansi "^6.0.1" - -"string-width@^1.0.2 || 2 || 3 || 4", string-width@^4.0.0, string-width@^4.1.0, string-width@^4.2.0, string-width@^4.2.2, string-width@^4.2.3: +"string-width-cjs@npm:string-width@^4.2.0", "string-width@^1.0.2 || 2 || 3 || 4", string-width@^4.0.0, string-width@^4.1.0, string-width@^4.2.0, string-width@^4.2.2, string-width@^4.2.3: version "4.2.3" resolved "https://registry.yarnpkg.com/string-width/-/string-width-4.2.3.tgz#269c7117d27b05ad2e536830a8ec895ef9c6d010" integrity sha512-wKyQRQpjJ0sIp62ErSZdGsjMJWsap5oRNihHhu6G7JVO/9jIB6UyevL+tXuOqrng8j/cxKTWyWUwvSTriiZz/g== @@ -5307,7 +5298,7 @@ string_decoder@^1.1.1: dependencies: safe-buffer "~5.2.0" -"strip-ansi-cjs@npm:strip-ansi@^6.0.1": +"strip-ansi-cjs@npm:strip-ansi@^6.0.1", strip-ansi@^6.0.0, strip-ansi@^6.0.1: version "6.0.1" resolved "https://registry.yarnpkg.com/strip-ansi/-/strip-ansi-6.0.1.tgz#9e26c63d30f53443e9489495b2105d37b67a85d9" integrity sha512-Y38VPSHcqkFrCpFnQ9vuSXmquuv5oXOKpGeT6aGrr3o3Gc9AlVa6JBfUSOCnbxGGZF+/0ooI7KrPuUSztUdU5A== @@ -5321,13 +5312,6 @@ strip-ansi@^5.0.0, strip-ansi@^5.1.0, strip-ansi@^5.2.0: dependencies: ansi-regex "^4.1.0" -strip-ansi@^6.0.0, strip-ansi@^6.0.1: - version "6.0.1" - resolved "https://registry.yarnpkg.com/strip-ansi/-/strip-ansi-6.0.1.tgz#9e26c63d30f53443e9489495b2105d37b67a85d9" - integrity sha512-Y38VPSHcqkFrCpFnQ9vuSXmquuv5oXOKpGeT6aGrr3o3Gc9AlVa6JBfUSOCnbxGGZF+/0ooI7KrPuUSztUdU5A== - dependencies: - ansi-regex "^5.0.1" - strip-ansi@^7.0.1, strip-ansi@^7.1.0: version "7.1.0" resolved "https://registry.yarnpkg.com/strip-ansi/-/strip-ansi-7.1.0.tgz#d5b6568ca689d8561370b0707685d22434faff45" @@ -5795,7 +5779,7 @@ workerpool@6.2.1: resolved "https://registry.yarnpkg.com/workerpool/-/workerpool-6.2.1.tgz#46fc150c17d826b86a008e5a4508656777e9c343" integrity sha512-ILEIE97kDZvF9Wb9f6h5aXK4swSlKGUcOEGiIYb2OOu/IrDU9iwj0fD//SsA6E5ibwJxpEvhullJY4Sl4GcpAw== -"wrap-ansi-cjs@npm:wrap-ansi@^7.0.0": +"wrap-ansi-cjs@npm:wrap-ansi@^7.0.0", wrap-ansi@^7.0.0: version "7.0.0" resolved "https://registry.yarnpkg.com/wrap-ansi/-/wrap-ansi-7.0.0.tgz#67e145cff510a6a6984bdf1152911d69d2eb9e43" integrity sha512-YVGIj2kamLSTxw6NsZjoBxfSwsn0ycdesmc4p+Q21c5zPuZ1pl+NfxVdxPtdHvmNVOQ6XSYG4AUtyt/Fi7D16Q== @@ -5822,15 +5806,6 @@ wrap-ansi@^6.2.0: string-width "^4.1.0" strip-ansi "^6.0.0" -wrap-ansi@^7.0.0: - version "7.0.0" - resolved "https://registry.yarnpkg.com/wrap-ansi/-/wrap-ansi-7.0.0.tgz#67e145cff510a6a6984bdf1152911d69d2eb9e43" - integrity sha512-YVGIj2kamLSTxw6NsZjoBxfSwsn0ycdesmc4p+Q21c5zPuZ1pl+NfxVdxPtdHvmNVOQ6XSYG4AUtyt/Fi7D16Q== - dependencies: - ansi-styles "^4.0.0" - string-width "^4.1.0" - strip-ansi "^6.0.0" - wrap-ansi@^8.1.0: version "8.1.0" resolved "https://registry.yarnpkg.com/wrap-ansi/-/wrap-ansi-8.1.0.tgz#56dc22368ee570face1b49819975d9b9a5ead214"