CLI: Support --exclude option (#1065)

Co-authored-by: Hadrien Croubois <[email protected]>

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 <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 <REFERENCE_CONTRACT>`  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 `<dirName>:` before the contract name or fully qualified name in the `--reference` option or `@custom:oz-upgrades-from` annotation, where `<dirName>` is the directory short name. Each directory short name must be unique, including compared to the main build info directory.
-* `--unsafeAllow "<VALIDATION_ERRORS>"` - 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 <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 <REFERENCE_CONTRACT>` - 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 "<BUILD_INFO_DIR>[,<BUILD_INFO_DIR>...]"` - 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 `<dirName>:` before the contract name or fully qualified name in the `--reference` option or `@custom:oz-upgrades-from` annotation, where `<dirName>` 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 "<GLOB_PATTERN>" [--exclude "<GLOB_PATTERN>"...]` - 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 "<VALIDATION_ERROR>[,<VALIDATION_ERROR>...]"` - 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<ProjectReport>
 ----
 
@@ -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 `<dirName>:` before the contract name or fully qualified name in the `reference` param or `@custom:oz-upgrades-from` annotation, where `<dirName>` 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:*
 

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))

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;
+  }
+}

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;
+  }
+}

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;
+    }
+}

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 {
+}

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");
+    }
+}

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;
+}

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 {
+}

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 {
+}

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"
   }
 }

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<unknown>) {
   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'));
+});