CLI: Add --requireReference option

docs/modules/ROOT/pages/api-core.adoc

@@ -117,7 +117,8 @@ 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>`  The name or fully qualified name of the reference contract to use for storage layout comparisons. Can only be used along with the `--contract` option. If not specified, uses the `@custom:oz-upgrades-from` annotation in the contract that is being 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. If specified, requires either the `--reference` option to be provided or the contract to have a `@custom:oz-upgrades-from` annotation.
 * `--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`
 * `--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.
@@ -161,11 +162,12 @@ Note that this function does not throw validation errors directly. Instead, you
 
 * `buildInfoDir` - the path to the build info directory which contains JSON files with Solidity compiler input and output. Defaults to `artifacts/build-info` for Hardhat projects or `out/build-info` for Foundry projects. If your project uses a custom output directory, you must specify its build info directory here.
 * `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` - 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.
+* `reference` - Can only be used when the `contract` argument 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.
 * `opts` - an object with the following options as defined in xref:api-hardhat-upgrades.adoc#common-options[Common Options]:
 ** `unsafeAllow`
 ** `unsafeAllowRenames`
 ** `unsafeSkipStorageCheck`
+** `requireReference` - Can only be used when the `contract` argument is also provided. If specified, requires either the `reference` argument to be provided or the contract to have a `@custom:oz-upgrades-from` annotation.
 
 *Returns:*
 

packages/core/CHANGELOG.md

@@ -1,5 +1,9 @@
 # Changelog
 
+## Unreleased
+
+- Add `requireReference` option for CLI. ([#900](https://github.com/OpenZeppelin/openzeppelin-upgrades/pull/900))
+
 ## 1.30.1 (2023-10-11)
 
 - Fix Hardhat compile error when using Solidity 0.5.x. ([#892](https://github.com/OpenZeppelin/openzeppelin-upgrades/pull/892))

packages/core/ava.config.js

@@ -8,4 +8,5 @@ module.exports = {
   environmentVariables: {
     FORCE_COLOR: '0',
   },
+  timeout: '30s',
 };

packages/core/contracts/test/cli/Validate.sol

@@ -139,4 +139,16 @@ contract BothAnnotationsUnsafe {
   function sd() public {
     selfdestruct(payable(msg.sender));
   }
+}
+
+contract StorageV2_Bad_NoAnnotation {
+  uint256 public x;
+  uint256 public y;
+  uint256[49] private __gap;
+}
+
+contract StorageV2_Ok_NoAnnotation {
+  uint256 public x;
+  uint256 public y;
+  uint256[48] private __gap;
 }

packages/core/src/cli/cli.test.ts

@@ -167,6 +167,73 @@ test('validate - reference not found', async t => {
   t.true(error?.message.includes('Could not find contract NonExistent.'), error?.message);
 });
 
+test('validate - requireReference - no contract option', async t => {
+  const temp = await getTempDir(t);
+  const buildInfo = await artifacts.getBuildInfo(`contracts/test/cli/Validate.sol:StorageV1`);
+  await fs.writeFile(path.join(temp, 'validate.json'), JSON.stringify(buildInfo));
+
+  const error = await t.throwsAsync(execAsync(`${CLI} validate ${temp} --requireReference`));
+  t.true(
+    error?.message.includes('The --requireReference option can only be used along with the --contract option.'),
+    error?.message,
+  );
+});
+
+test('validate - requireReference - no reference, no upgradesFrom', async t => {
+  const temp = await getTempDir(t);
+  const buildInfo = await artifacts.getBuildInfo(`contracts/test/cli/Validate.sol:StorageV1`);
+  await fs.writeFile(path.join(temp, 'validate.json'), JSON.stringify(buildInfo));
+
+  const error = await t.throwsAsync(execAsync(`${CLI} validate ${temp} --contract StorageV1 --requireReference`));
+  t.true(error?.message.includes('does not specify what contract it upgrades from'), error?.message);
+});
+
+test('validate - requireReference - no reference, has upgradesFrom - safe', async t => {
+  const temp = await getTempDir(t);
+  const buildInfo = await artifacts.getBuildInfo(`contracts/test/cli/Validate.sol:BecomesSafe`);
+  await fs.writeFile(path.join(temp, 'validate.json'), JSON.stringify(buildInfo));
+
+  const output = (await execAsync(`${CLI} validate ${temp} --contract BecomesSafe --requireReference`)).stdout;
+  t.snapshot(output);
+});
+
+test('validate - requireReference - no reference, has upgradesFrom - unsafe', 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 --requireReference`),
+  );
+  const expectation: string[] = [`Stdout: ${(error as any).stdout}`, `Stderr: ${(error as any).stderr}`];
+  t.snapshot(expectation.join('\n'));
+});
+
+test('validate - requireReference - has reference - unsafe', async t => {
+  const temp = await getTempDir(t);
+  const buildInfo = await artifacts.getBuildInfo(`contracts/test/cli/Validate.sol:StorageV2_Bad_NoAnnotation`);
+  await fs.writeFile(path.join(temp, 'validate.json'), JSON.stringify(buildInfo));
+
+  const error = await t.throwsAsync(
+    execAsync(`${CLI} validate ${temp} --contract StorageV2_Bad_NoAnnotation --reference StorageV1 --requireReference`),
+  );
+  const expectation: string[] = [`Stdout: ${(error as any).stdout}`, `Stderr: ${(error as any).stderr}`];
+  t.snapshot(expectation.join('\n'));
+});
+
+test('validate - requireReference - has reference - safe', async t => {
+  const temp = await getTempDir(t);
+  const buildInfo = await artifacts.getBuildInfo(`contracts/test/cli/Validate.sol:StorageV2_Ok_NoAnnotation`);
+  await fs.writeFile(path.join(temp, 'validate.json'), JSON.stringify(buildInfo));
+
+  const output = (
+    await execAsync(
+      `${CLI} validate ${temp} --contract StorageV2_Ok_NoAnnotation --reference StorageV1 --requireReference`,
+    )
+  ).stdout;
+  t.snapshot(output);
+});
+
 test('validate - no upgradeable', async t => {
   const temp = await getTempDir(t);
   const buildInfo = await artifacts.getBuildInfo(`contracts/test/cli/Storage088.sol:Storage088`);

packages/core/src/cli/cli.test.ts.md

@@ -17,7 +17,8 @@ Generated by [AVA](https://avajs.dev).
     ␊
     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>  The name or fully qualified name of the reference contract to use for storage layout comparisons. Can only be used along with the --contract option. If not specified, uses the @custom:oz-upgrades-from annotation in the contract that is being 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. If specified, requires either the --reference option to be provided or the contract to have a @custom:oz-upgrades-from annotation.␊
       --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␊
       --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.␊
@@ -36,7 +37,8 @@ Generated by [AVA](https://avajs.dev).
     ␊
     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>  The name or fully qualified name of the reference contract to use for storage layout comparisons. Can only be used along with the --contract option. If not specified, uses the @custom:oz-upgrades-from annotation in the contract that is being 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. If specified, requires either the --reference option to be provided or the contract to have a @custom:oz-upgrades-from annotation.␊
       --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␊
       --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.␊
@@ -215,6 +217,51 @@ Generated by [AVA](https://avajs.dev).
     ␊
     Stderr: `
 
+## validate - requireReference - no reference, has upgradesFrom - safe
+
+> Snapshot 1
+
+    ` ✔  contracts/test/cli/Validate.sol:BecomesSafe (upgrades from contracts/test/cli/Validate.sol:MultipleUnsafe)␊
+    ␊
+    SUCCESS (1 upgradeable contracts detected, 1 passed, 0 failed)␊
+    `
+
+## validate - requireReference - no reference, has upgradesFrom - unsafe
+
+> Snapshot 1
+
+    `Stdout:  ✘  contracts/test/cli/Validate.sol:BecomesBadLayout (upgrades from contracts/test/cli/Validate.sol:BecomesSafe)␊
+    ␊
+          BecomesSafe: Deleted \`x\`␊
+            > Keep the variable even if unused␊
+    ␊
+    FAILED (1 upgradeable contract detected, 0 passed, 1 failed)␊
+    ␊
+    Stderr: `
+
+## validate - requireReference - has reference - unsafe
+
+> Snapshot 1
+
+    `Stdout:  ✘  contracts/test/cli/Validate.sol:StorageV2_Bad_NoAnnotation (upgrades from contracts/test/cli/Validate.sol:StorageV1)␊
+    ␊
+          contracts/test/cli/Validate.sol:147: Layout changed for \`__gap\` (uint256[49] -> uint256[49])␊
+            - Slot changed from 1 to 2␊
+            > Set __gap array to size 48␊
+    ␊
+    FAILED (1 upgradeable contract detected, 0 passed, 1 failed)␊
+    ␊
+    Stderr: `
+
+## validate - requireReference - has reference - safe
+
+> Snapshot 1
+
+    ` ✔  contracts/test/cli/Validate.sol:StorageV2_Ok_NoAnnotation (upgrades from contracts/test/cli/Validate.sol:StorageV1)␊
+    ␊
+    SUCCESS (1 upgradeable contracts detected, 1 passed, 0 failed)␊
+    `
+
 ## validate - no upgradeable
 
 > Snapshot 1

packages/core/src/cli/validate.ts

@@ -1,9 +1,9 @@
 import minimist from 'minimist';
 
 import { ValidateUpgradeSafetyOptions, validateUpgradeSafety } from '.';
-import { withValidationDefaults } from '..';
 import { ValidationError, errorKinds } from '../validate/run';
 import debug from '../utils/debug';
+import { withCliDefaults } from './validate/validate-upgrade-safety';
 
 const USAGE = 'Usage: npx @openzeppelin/upgrades-core validate [<BUILD_INFO_DIR>] [<OPTIONS>]';
 const DETAILS = `
@@ -14,7 +14,8 @@ Arguments:
 
 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>  The name or fully qualified name of the reference contract to use for storage layout comparisons. Can only be used along with the --contract option. If not specified, uses the @custom:oz-upgrades-from annotation in the contract that is being 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. If specified, requires either the --reference option to be provided or the contract to have a @custom:oz-upgrades-from annotation.
   --unsafeAllow "<VALIDATION_ERRORS>"  Selectively disable one or more validation errors. Comma-separated list with one or more of the following: ${errorKinds.join(
     ', ',
   )}
@@ -45,6 +46,7 @@ function parseArgs(args: string[]) {
       'unsafeSkipStorageCheck',
       'unsafeAllowCustomTypes',
       'unsafeAllowLinkedLibraries',
+      'requireReference',
     ],
     string: ['unsafeAllow', 'contract', 'reference'],
     alias: { h: 'help' },
@@ -87,10 +89,15 @@ export function getFunctionArgs(parsedArgs: minimist.ParsedArgs, extraArgs: stri
     const buildInfoDir = extraArgs.length === 1 ? undefined : extraArgs[1];
     const contract = getAndValidateString(parsedArgs, 'contract');
     const reference = getAndValidateString(parsedArgs, 'reference');
-    if (reference !== undefined && contract === undefined) {
-      throw new Error('The --reference option can only be used along with the --contract option.');
-    }
     const opts = withDefaults(parsedArgs);
+
+    if (contract === undefined) {
+      if (reference !== undefined) {
+        throw new Error('The --reference option can only be used along with the --contract option.');
+      } else if (opts.requireReference) {
+        throw new Error('The --requireReference option can only be used along with the --contract option.');
+      }
+    }
     return { buildInfoDir, contract, reference, opts };
   }
 }
@@ -117,6 +124,7 @@ function validateOptions(parsedArgs: minimist.ParsedArgs) {
         'unsafeAllow',
         'contract',
         'reference',
+        'requireReference',
       ].includes(key),
   );
   if (invalidArgs.length > 0) {
@@ -152,7 +160,8 @@ export function withDefaults(parsedArgs: minimist.ParsedArgs): Required<Validate
     unsafeAllowCustomTypes: parsedArgs['unsafeAllowCustomTypes'],
     unsafeAllowLinkedLibraries: parsedArgs['unsafeAllowLinkedLibraries'],
     unsafeAllow: getUnsafeAllowKinds(parsedArgs['unsafeAllow']),
+    requireReference: parsedArgs['requireReference'],
   };
 
-  return withValidationDefaults(allOpts);
+  return withCliDefaults(allOpts);
 }

packages/core/src/cli/validate/contract-report.ts

@@ -69,7 +69,7 @@ export class UpgradeableContractReport implements Report {
  */
 export function getContractReports(
   sourceContracts: SourceContract[],
-  opts: ValidateUpgradeSafetyOptions,
+  opts: Required<ValidateUpgradeSafetyOptions>,
   specifiedContracts?: SpecifiedContracts,
 ) {
   const upgradeableContractReports: UpgradeableContractReport[] = [];
@@ -83,7 +83,11 @@ export function getContractReports(
       sourceContracts,
       specifiedContracts?.reference,
     );
-    if (specifiedContracts !== undefined || upgradeabilityAssessment.upgradeable) {
+    if (opts.requireReference && upgradeabilityAssessment.referenceContract === undefined) {
+      throw new Error(
+        `The contract ${sourceContract.fullyQualifiedName} does not specify what contract it upgrades from. Add the \`@custom:oz-upgrades-from <REFERENCE_CONTRACT>\` annotation to the contract, or include the reference contract name when running the validate command or function.`,
+      );
+    } else if (specifiedContracts !== undefined || upgradeabilityAssessment.upgradeable) {
       const reference = upgradeabilityAssessment.referenceContract;
       const kind = upgradeabilityAssessment.uups ? 'uups' : 'transparent';
       const report = getUpgradeableContractReport(sourceContract, reference, { ...opts, kind: kind });

packages/core/src/cli/validate/validate-upgrade-safety.test.ts

@@ -6,7 +6,7 @@ import path from 'path';
 import os from 'os';
 
 import { artifacts } from 'hardhat';
-import { validateUpgradeSafety } from './validate-upgrade-safety';
+import { findSpecifiedContracts, validateUpgradeSafety, withCliDefaults } from './validate-upgrade-safety';
 import { ReferenceContractNotFound } from './find-contract';
 
 test.before(async () => {
@@ -97,3 +97,15 @@ test('invalid annotation args - upgrades', async t => {
   );
   t.true(error?.message.includes('Found 1, expected 0'));
 });
+
+test('findSpecifiedContracts - requireReference option without contract', async t => {
+  try {
+    findSpecifiedContracts([], withCliDefaults({ requireReference: true }));
+  } catch (e: any) {
+    t.true(
+      e.message.includes(
+        'The requireReference option can only be specified when the contract option is also specified.',
+      ),
+    );
+  }
+});